Auto-focus on navigate in all browsers #5595
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project 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
|
jfdoming
force-pushed
the
jfdoming/08-21-auto-focus-on-navigate-in-all-browsers
branch
5 times, most recently
from
e73774a to
7b7d0fd
Compare
jfdoming
force-pushed
the
jfdoming/08-21-auto-focus-on-navigate-in-all-browsers
branch
from
7b7d0fd to
ff7fb95
Compare
WalkthroughAdds a global hidden input focus mechanism and integrates it into mobile navigation and transaction entry. Introduces NavigableFocusProvider with hooks, wraps the app with the provider, and calls the focus function from MobileNavTabs and AddTransactionButton (decimal inputmode) before navigation. Enhances FocusableAmountInput to coordinate with the global focus element, moves input/blur/keyup handling to effects and useEventCallback, and adjusts TransactionEdit to always trigger initial amount edit on add. E2E: adds globalAmountField locator to the MobileTransactionEntryPage and updates multiple mobile transaction tests to assert focus and fill that field. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1)
137-158: Address TypeScript errors with proper event listener typing.The effect correctly manages global input event listeners, but the TypeScript errors should be resolved with proper typing instead of suppressing them.
Consider replacing the type suppressions with proper typing:
- // @ts-expect-error addEventListener missing types - el.addEventListener('blur', onBlur); - // @ts-expect-error addEventListener missing types - el.addEventListener('input', onChangeText); - // @ts-expect-error addEventListener missing types - el.addEventListener('keyup', onKeyUp); + el.addEventListener('blur', onBlur as EventListener); + el.addEventListener('input', onChangeText as EventListener); + el.addEventListener('keyup', onKeyUp as EventListener);And similarly for the cleanup:
- // @ts-expect-error addEventListener missing types - el.removeEventListener('blur', onBlur); - // @ts-expect-error addEventListener missing types - el.removeEventListener('input', onChangeText); - // @ts-expect-error addEventListener missing types - el.removeEventListener('keyup', onKeyUp); + el.removeEventListener('blur', onBlur as EventListener); + el.removeEventListener('input', onChangeText as EventListener); + el.removeEventListener('keyup', onKeyUp as EventListener);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
upcoming-release-notes/5595.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts(2 hunks)packages/desktop-client/e2e/transactions.mobile.test.ts(4 hunks)packages/desktop-client/src/components/NavigableFocusProvider.tsx(1 hunks)packages/desktop-client/src/components/mobile/MobileNavTabs.tsx(4 hunks)packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx(2 hunks)packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx(5 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx(2 hunks)packages/desktop-client/src/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/desktop-client/src/components/mobile/MobileNavTabs.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
- packages/desktop-client/e2e/transactions.mobile.test.ts
- packages/desktop-client/src/components/mobile/transactions/AddTransactionButton.tsx
- packages/desktop-client/src/components/NavigableFocusProvider.tsx
- packages/desktop-client/e2e/page-models/mobile-transaction-entry-page.ts
- packages/desktop-client/src/index.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Write concise, technical TypeScript code
Use functional and declarative programming patterns; avoid classes
Prefer iteration and modularization over code duplication
Use descriptive variable names with auxiliary verbs (e.g., isLoaded, hasError)
Structure files with sections in order: exported page/component, GraphQL queries, helpers, static content, types
Favor named exports for components and utilities
Prefer interfaces over types in TypeScript
Avoid enums; use objects or maps instead
Avoid using any or unknown unless absolutely necessary; prefer existing types
Avoid type assertions with as or non-null !; prefer satisfies
Use the function keyword for pure functions
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements
Files:
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Use declarative JSX, keeping JSX minimal and readable
Files:
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
🧠 Learnings (2)
📚 Learning: 2024-10-24T18:57:16.406Z
Learnt from: lelemm
PR: actualbudget/actual#3732
File: packages/desktop-client/src/components/util/AmountInput.tsx:102-108
Timestamp: 2024-10-24T18:57:16.406Z
Learning: In `packages/desktop-client/src/components/util/AmountInput.tsx`, zero amounts are handled in the `onSwitch` function, so zero amounts are intentionally ignored in the `fireUpdate` function.
Applied to files:
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
📚 Learning: 2024-10-22T05:34:56.976Z
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx:261-277
Timestamp: 2024-10-22T05:34:56.976Z
Learning: In React components (TypeScript), avoid using hooks like `useCallback` inside callbacks or nested functions. Hooks must be called at the top level of functional components.
Applied to files:
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx
🧬 Code graph analysis (1)
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (2)
packages/desktop-client/src/components/NavigableFocusProvider.tsx (1)
useNavigableFocusRef(85-93)packages/loot-core/src/shared/util.ts (3)
currencyToAmount(444-464)reapplyThousandSeparators(217-238)appendDecimals(240-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
🔇 Additional comments (8)
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (8)
10-10: LGTM: Import additions align with enhanced focus management.The new imports (
ChangeEvent,useEventCallback,useNavigableFocusRef) are appropriate for the enhanced focus coordination with the global focus manager and proper TypeScript event handling.Also applies to: 18-18, 28-28
65-66: LGTM: Global focus reference integration.The integration with the global focus system is clean and follows the established pattern from the NavigableFocusProvider.
67-75: LGTM: Enhanced focus management with global coordination.The focus logic correctly checks if the global focus element is active before attempting to focus the local input, providing seamless coordination between the global and local focus systems.
83-94: LGTM: Event handler modernization with useEventCallback.The conversion to
useEventCallbackis appropriate for performance optimization. The logic for Backspace and Enter key handling is preserved correctly.
100-100: LGTM: Global focus cleanup on text application.Blurring the global focus element when applying text ensures consistent state management between the global and local focus systems.
121-126: LGTM: onBlur handler modernization.The conversion to
useEventCallbackis appropriate, and the existing blur logic with default prevention check is preserved correctly.
128-135: LGTM: Properly typed onChange handler.The conversion to a properly typed
ChangeEvent<HTMLInputElement>handler withuseEventCallbackis excellent. The logic for text processing with thousand separators and decimals is preserved correctly.
167-167: LGTM: Direct event handler assignment.The change from value-callback form to direct
onChangeTextassignment is consistent with the modernized event handling approach.
Awesome! I think the VRT is flaky now 😞 got it "working," pushed a cosmetic change and it broke again. I'll have a look when I'm back but if you have any ideas ff to push to my branch |
|
Would it be possible to not auto-focus the amount if its an existing transaction? So if you open a transaction from the account page, it would be nice to not focus the amount where usually I want to make something as cleared or change the category in that case |
Just to clarify, you're requesting for a behavioral change vs master, right? On Android 25.8 it currently auto-focuses even on edit |
Correct. The current behavior feels sub-optimal. It seems best to NOT auto-focus whrn viewing existing transactions. |
No description provided.