Add Upcoming length adjustment feature flag #3651
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
SamBobBarnes
changed the title
WalkthroughThe pull request introduces a new feature flag, Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/hooks/useFeatureFlag.ts (1)
11-11: LGTM! Consider sorting feature flags alphabetically.The addition of the
upcomingLengthAdjustmentfeature flag is correct and follows the established pattern. The default value offalseis appropriate for a new feature flag.For improved readability and easier maintenance, consider sorting the feature flags alphabetically within the
DEFAULT_FEATURE_FLAG_STATEobject. This would make it easier to locate specific flags as the list grows.packages/desktop-client/src/components/settings/Upcoming.tsx (2)
46-47: LGTM: Conditional rendering based on feature flag.The conditional rendering is correctly implemented, preventing the component from rendering when the feature flag is off. This aligns with the PR objective and allows for easy toggling of the feature.
Consider adding a brief comment explaining the purpose of this check for future maintainers:
+ // Only render the component if the upcomingLengthAdjustment feature is enabled if (!enabled) return null;
Line range hint
9-47: Summary: Feature flag implementation for upcoming length adjustment.The changes in this file successfully implement the feature flag for the upcoming length adjustment feature. The key points are:
- The
useFeatureFlaghook is imported and used correctly.- The feature flag state is checked before rendering the component.
- The implementation allows for easy toggling of the feature without code changes.
These changes align well with the PR objective and follow best practices for feature flag implementation.
Consider documenting the purpose and usage of this feature flag in a central location (e.g., a README or feature flag documentation file) to help other developers understand its impact across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3651.mdis excluded by!**/*.md
📒 Files selected for processing (4)
- packages/desktop-client/src/components/settings/Experimental.tsx (1 hunks)
- packages/desktop-client/src/components/settings/Upcoming.tsx (2 hunks)
- packages/desktop-client/src/hooks/useFeatureFlag.ts (1 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/desktop-client/src/components/settings/Upcoming.tsx (2)
9-9: LGTM: New import for feature flag hook.The import statement for
useFeatureFlagis correctly placed and necessary for the new feature flag functionality.
41-42: LGTM: Feature flag initialization.The
enabledconstant is correctly initialized using theuseFeatureFlaghook with the appropriate flag name 'upcomingLengthAdjustment'. This aligns with the PR objective of adding a feature flag for the upcoming length adjustment feature.packages/desktop-client/src/components/settings/Experimental.tsx (1)
119-124: LGTM! New feature toggle added correctly.The new
FeatureTogglefor "upcomingLengthAdjustment" is implemented correctly and follows the existing pattern in the file. The use of theTranscomponent for internationalization is appropriate.There's a TODO comment about creating a feedback issue. It would be beneficial to create this issue and update the
feedbackLinkprop before merging this PR.Let's verify if the new feature flag is properly defined in the relevant files:
packages/desktop-client/src/components/settings/Experimental.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Matiss Janis Aboltins <[email protected]>
@MatissJanis is there a way to generate these? |
The readme for the desktop client package says how to generate new ones |
@MatissJanis I'm sure I'm doing something wrong, but I've been pulling my hair out for 2 days trying to get these VRT screenshots generated and getting nowhere. Most common error I get when I try to run them is I have tried to run them locally, in docker, and against localhost and my lan ip. Could this be CORS acting up? I wish there was better documentation on how to get this working. Seems like the current docs in the readme dont have any good troubleshooting tips. Any helpful tips you could give would be greatly appreciated. I have not used playwright before, so could by my env as well. I have tried this on windows and mac with the same result. |
|
My process is to:
I've never been able to use localhost and have it work so maybe just don't even try using localhost in place of the IP. I just filled in an example IP for now. Then git status should show the updated snapshots if any needed changed |
Tried it as described on my mac, and it worked first time. Dont know what I was doing wrong other than localhost. Thanks @youngcw! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/e2e/schedules.test.js (3)
1-1: LGTM, but consider maintaining import order consistency.The reordering of imports is fine, but it's a good practice to maintain a consistent import order across all files in the project. Consider using a linter or formatter to automatically manage import orders.
29-31: Remove commented-out test code.Instead of leaving the old test commented out, it's better to remove it entirely. If you need to reference this code later, you can always find it in the git history. Keeping commented-out code can lead to confusion and clutter in the codebase.
Line range hint
58-128: Consider running existing test with different feature flag states.The existing test for creating a new schedule and posting the transaction is comprehensive and remains unchanged. However, to ensure complete coverage, consider running this test with different states of the 'upcomingLengthAdjustment' feature flag as well.
You could move this test inside the feature flag loop or create a separate loop for it. This would help ensure that the new feature doesn't unexpectedly affect existing functionality. Here's a suggestion:
[true, false].forEach(upcomingLengthAdjustment => { test.describe(`with upcoming length adjustment flag ${upcomingLengthAdjustment ? 'on' : 'off'}`, () => { // ... (existing setup and teardown) test('creates a new schedule, posts the transaction and later completes it', async () => { // ... (existing test code) }); }); });This change would run the test twice, once with the feature flag on and once with it off, providing more comprehensive coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (21)
packages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-10-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-11-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-12-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-off-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-off-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-off-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-on-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-on-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-with-upcoming-length-adjustment-flag-on-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
- packages/desktop-client/e2e/schedules.test.js (2 hunks)
🧰 Additional context used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/e2e/page-models/settings-page.js (1)
Line range hint
20-33: Approve method renaming and suggest error handling improvementThe renaming of the method to
toggleExperimentalFeatureis consistent with its usage and improves clarity. The implementation correctly handles the process of toggling an experimental feature.Consider adding error handling to improve robustness:
async toggleExperimentalFeature(featureName) { - const advancedSettingsButton = this.page.getByTestId('advanced-settings'); - await advancedSettingsButton.click(); + try { + const advancedSettingsButton = this.page.getByTestId('advanced-settings'); + await advancedSettingsButton.click(); - const experimentalSettingsButton = this.page.getByTestId( - 'experimental-settings', - ); - await experimentalSettingsButton.click(); + const experimentalSettingsButton = this.page.getByTestId( + 'experimental-settings', + ); + await experimentalSettingsButton.click(); - const featureCheckbox = this.page.getByRole('checkbox', { - name: featureName, - }); - await featureCheckbox.click(); + const featureCheckbox = this.page.getByRole('checkbox', { + name: featureName, + }); + await featureCheckbox.click(); + } catch (error) { + console.error(`Failed to toggle feature '${featureName}':`, error); + throw error; + } }This change would provide better error reporting if any of the UI interactions fail, which can be helpful in debugging test failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/e2e/page-models/settings-page.js (2 hunks)
- packages/desktop-client/e2e/schedules.test.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/e2e/schedules.test.js
🧰 Additional context used
🔇 Additional comments (1)
packages/desktop-client/e2e/page-models/settings-page.js (1)
11-11: Approve method renaming: Improved clarity and flexibilityThe renaming of
enableExperimentalFeaturetotoggleExperimentalFeatureis a positive change. It more accurately reflects the method's functionality, suggesting that it can both enable and disable the feature. This improves code readability and maintainability.
SamBobBarnes
force-pushed
the
Upcoming-length-feature-flag
branch
from
5d2b1e8 to
f9a0572
Compare
Add feature flag for upcoming length adjustment feature.