✨ release tracking budget feature #3833
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 No assets were bigger 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
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -43,6 +43,43 @@ export function MobileNavTabs() { | |||
| padding: 10, | |||
| }; | |||
|
|
|||
| const [{ y }, api] = useSpring(() => ({ y: OPEN_DEFAULT_Y })); | |||
There was a problem hiding this comment.
Technically there are no changes here. I just re-arranged the constant definitions so that openDefault could be accessed by the NavLink definitions. It will now close the bottom nav bar whenever a item in the nav is clicked (thus navigating to a different page).
This should make the VRT tests more stable. 🤞
This comment was marked as resolved.
This comment was marked as resolved.
MatissJanis
changed the title
WalkthroughThe pull request introduces several changes across multiple files in the desktop client. In Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/mobile/MobileNavTabs.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/settings/Experimental.tsx (1)
Line range hint
65-142: Consider architectural improvements for feature flag managementWhile the implementation is functional, consider these architectural improvements:
- Centralize feature documentation
- Add feature flag deprecation tracking
- Consider A/B testing capabilities
Would you like me to propose a more robust feature flag management system that includes these improvements?
packages/desktop-client/src/components/settings/index.tsx (1)
Line range hint
108-115: Consider addressing the TODO comment about ID management.There's a low-priority TODO comment about eliminating or conditionally showing certain IDs. This technical debt should be tracked and addressed in a future update.
Would you like me to create a GitHub issue to track this technical debt item? I can help outline the requirements and considerations for managing these IDs.
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (2)
48-81: Consider adding error boundaries for animation failuresThe animation control functions are well-implemented with proper memoization and smooth transitions. The wobbly effect on cancel is a nice UX touch. However, consider wrapping the animation logic in an error boundary to gracefully handle any potential animation failures.
class AnimationErrorBoundary extends React.Component { componentDidCatch(error: Error) { // Fallback to default state api.start({ y: OPEN_DEFAULT_Y, immediate: true }); // Log error or notify monitoring service } // ... rest of implementation }
242-242: Consider enhancing accessibility for navigation tabsThe NavTab component implementation is type-safe and correctly handles the onClick prop. However, consider enhancing accessibility by adding ARIA attributes and keyboard navigation support.
function NavTab({ Icon: TabIcon, name, path, style, onClick }: NavTabProps) { return ( <NavLink to={path} + role="tab" + aria-label={name} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + onClick?.(e); + } + }} style={({ isActive }) => ({ ...styles.noTapHighlight, alignItems: 'center', color: isActive ? theme.mobileNavItemSelected : theme.mobileNavItem, display: 'flex', flexDirection: 'column', textDecoration: 'none', textAlign: 'center', ...style, })} onClick={onClick} > <TabIcon width={22} height={22} /> {name} </NavLink> ); }Also applies to: 245-245, 259-259
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
packages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-6-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3833.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/e2e/page-models/settings-page.js(0 hunks)packages/desktop-client/src/components/mobile/MobileNavTabs.tsx(5 hunks)packages/desktop-client/src/components/settings/Experimental.tsx(1 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/hooks/useFeatureFlag.ts(0 hunks)packages/loot-core/src/types/prefs.d.ts(0 hunks)
💤 Files with no reviewable changes (3)
- packages/desktop-client/e2e/page-models/settings-page.js
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/loot-core/src/types/prefs.d.ts
🔇 Additional comments (6)
packages/desktop-client/src/components/settings/Experimental.tsx (3)
Line range hint 16-26: LGTM: Well-structured type definition
The FeatureToggleProps type is well-defined with clear documentation of required and optional properties.
Line range hint 28-63: LGTM: Robust FeatureToggle implementation
The FeatureToggle component is well-implemented with:
- Proper state management using hooks
- Accessibility support with labels
- Clear visual feedback for disabled state
- Error handling
- External feedback link integration
Line range hint 1-15: Verify complete removal of reportBudget feature flag
The removal of the tracking budget feature from experimental settings suggests it's now a stable feature. Let's verify all related code has been cleaned up.
✅ Verification successful
The tracking budget feature has been released and is now a stable feature
The search results confirm that the tracking budget feature has been released as a stable feature, as evidenced by:
- Release note in
upcoming-release-notes/3833.md: "Release tracking budget feature" - No references to
reportBudgetfeature flag in the codebase - All tracking budget code is now part of the main implementation, not behind feature flags
The code cleanup is complete and consistent across the codebase. The feature has been properly integrated into the main codebase with:
- Proper type definitions
- Complete component implementations
- Well-structured queries and handlers
- Mobile and desktop support
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to reportBudget feature flag
echo "Searching for reportBudget references..."
rg -i "reportbudget|tracking.?budget"
# Search for any remaining feature flag initialization
echo "Checking feature flag initialization..."
ast-grep --pattern 'flags: { $$$ reportBudget $$$ }'
Length of output: 18864
packages/desktop-client/src/components/settings/index.tsx (1)
179-179: LGTM! Budget type settings are now a stable feature.
The removal of the feature flag conditional for BudgetTypeSettings indicates that this feature has graduated from experimental status to become a stable part of the application.
Let's verify that this change is consistent with the removal of the feature flag across the codebase:
✅ Verification successful
The removal of the feature flag is consistent with the codebase
Looking at the evidence:
- The
reportBudgetfeature flag is completely removed from theFeatureFlagtype definition inprefs.d.ts - There are no remaining references to
reportBudgetin the codebase - The
BudgetTypeSettingscomponent now directly usesuseSyncedPreffor budget type management without any feature flag checks - The component allows switching between 'rollover' (envelope) and 'report' (tracking) budgeting types as stable features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete removal of reportBudget feature flag
# Check for any remaining references to the reportBudget feature flag
rg "reportBudget" --type ts --type tsx
# Check for any remaining feature flag conditional rendering of BudgetTypeSettings
ast-grep --pattern 'useFeatureFlag($_, "reportBudget")'
Length of output: 124
Script:
#!/bin/bash
# Let's try different approaches to verify the feature flag removal
# Search for reportBudget using ripgrep without file type restrictions
rg "reportBudget"
# Search for any useFeatureFlag usage
rg "useFeatureFlag"
# Search for BudgetTypeSettings component usage
rg "BudgetTypeSettings"
# Look for feature flag related imports
rg "import.*FeatureFlag"
Length of output: 11257
Script:
#!/bin/bash
# Let's check the feature flags type definition and the BudgetTypeSettings component
# Check feature flags type definition
rg -A 10 "type FeatureFlag ="
# Look at the BudgetTypeSettings component implementation
cat packages/desktop-client/src/components/settings/BudgetTypeSettings.tsx
Length of output: 2611
packages/desktop-client/src/components/mobile/MobileNavTabs.tsx (2)
3-3: LGTM: Clean type imports and spring animation setup
The ComponentProps import and spring animation initialization are well-structured and follow React best practices.
Also applies to: 46-47
132-134: LGTM: NavTab click handler implementation
The click handler implementation correctly closes the navigation bar on navigation, which should improve test stability as mentioned in the previous review.
Solves #2999