Fix mobile transaction form requires additional click to unfocus amount input #3274
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged No assets were unchanged |
actual-github-bot
bot
changed the title
|
cc @joel-jeremy would you mind taking a look at this as our mobile expert? :) |
|
This behavior was intentional and the changes here will break the hook that enforces this behavior (atleast on Chrome based browsers). The point of the hook is to only have one field being active/edited at any given time. If you click on another field while another one is active, it should not activate the clicked field but instead deactivate the current active field. I'm open to ideas on how to handle forms on mobile but this is what I was able to come up with so far. If we are able to remove the need for this hook while having a better UX then I'm all for it. :) |
But what's the point of that behaviour? For me it is annoying because requires doubletap between the fields. I don't think I have seen such thing in any app. |
For context - that was introduced because previously the mobile budget page was all edited inline. Which means you cant just easily click out of the currently edited field e.g. Budgeted to save it without clicking on another field e.g. Spent. Today, it's not much of an issue since the budget page fields have all been migrated to open up modals. With that, I'm okay with removing the hook altogether since the mobile app's design has now migrated away from needing this functionality. |
That makes sense, thank you. I will then go ahead and remove that hook altogether. |
|
@joel-jeremy removed |
Thanks! Now that the blur logic has changed, I now see a bug where if you create a split transaction, set an amount, click on another field e.g. category, the amount does not get set properly. We may need to start using onChange here instead of onBlur to set the values. |
Either I can't reproduce it from my end, or I don't get what's the issue. Do you have more detailed "how to reproduce"? :) |
|
Click on the payee/category after setting the amount: 1000004042.mp4The amount will only get set if you click on the empty areas on the screen |
|
@coderabbitai full review |
WalkthroughThe pull request includes significant changes to several components in the desktop client, focusing on simplifying state management related to focus handling in forms. The Changes
Assessment against linked issues
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3274.mdis excluded by!**/*.md
📒 Files selected for processing (5)
- packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (4 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (6 hunks)
- packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx (2 hunks)
- packages/desktop-client/src/components/modals/RolloverBudgetMenuModal.tsx (2 hunks)
- packages/desktop-client/src/hooks/useSingleActiveEditForm.tsx (0 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/hooks/useSingleActiveEditForm.tsx
🔇 Additional comments (9)
packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx (1)
79-79: Approve change with clarification requestThe addition of
defaultFocused={true}to theFocusableAmountInputcomponent aligns well with the PR objectives. It addresses the issue of requiring an additional click to focus on the amount input and simplifies focus management.However, there are a couple of points that need clarification:
The AI summary mentions the removal of
amountFocusedstate and associated logic, but these changes are not visible in the provided code. Could you please confirm if these changes were made and if so, provide the updated code?While setting
defaultFocused={true}solves the immediate issue, are there any potential side effects of always having the amount input focused by default? For example, could this interfere with keyboard navigation or accessibility features?To verify the removal of
amountFocusedstate and associated logic, please run the following script:packages/desktop-client/src/components/modals/RolloverBudgetMenuModal.tsx (2)
81-81: Approve change with suggestions for further verificationThe addition of
defaultFocused={true}to theFocusableAmountInputcomponent aligns well with the PR objectives. It simplifies focus management and potentially improves the user experience by reducing the number of clicks required to interact with the form.To ensure the change meets the intended goals:
- Please verify this behavior on various mobile devices to confirm it resolves the double-tap issue mentioned in the PR objectives.
- Consider any potential side effects this change might have on the overall form interaction, especially in light of the previous concerns about maintaining only one active field at a time.
✅ Verification successful
Change verified and approved
The addition of
defaultFocused={true}to theFocusableAmountInputcomponent is consistently applied across the codebase, ensuring uniform focus management and enhancing the user experience as intended by the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other instances of FocusableAmountInput that might need similar changes # Test: Search for other usages of FocusableAmountInput rg --type typescript 'FocusableAmountInput'Length of output: 243
Script:
#!/bin/bash # Description: Search for all instances of FocusableAmountInput in TypeScript and TSX files # Search for 'FocusableAmountInput' in .ts and .tsx files rg 'FocusableAmountInput' --glob '*.ts' --glob '*.tsx'Length of output: 1010
Script:
#!/bin/bash # Description: Inspect FocusableAmountInput usage in ReportBudgetMenuModal.tsx for focus management # Search for props passed to FocusableAmountInput in ReportBudgetMenuModal.tsx rg 'FocusableAmountInput\s*\(' packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx --multilineLength of output: 116
Script:
#!/bin/bash # Description: Extract props passed to FocusableAmountInput in ReportBudgetMenuModal.tsx # Search for all instances of FocusableAmountInput in JSX and display the props rg '<FocusableAmountInput\s[^>]*>' packages/desktop-client/src/components/modals/ReportBudgetMenuModal.tsx --multilineLength of output: 656
Line range hint
1-105: Overall simplification looks good, but warrants careful testingThe removal of explicit focus management using
useStateanduseEffecthooks (as mentioned in the AI summary) simplifies the component and aligns with the PR objectives. This change, combined with the addition ofdefaultFocused={true}, should improve the user experience.However, given the extent of the changes and the concerns raised in the PR comments, I recommend:
- Thorough testing of all form interactions, especially for split transactions. The PR comments mention a potential issue with setting amounts in split transactions, which should be carefully verified.
- Monitoring for any regressions related to focus management across different scenarios and device types.
- Verifying that the removal of the
useSingleActiveEditFormhook (mentioned in PR comments) doesn't introduce unexpected behaviors in other parts of the application.Consider documenting the rationale behind this simplification in the component or in a separate architecture document. This will help future maintainers understand the design decisions and the trade-offs made.
packages/desktop-client/src/components/mobile/transactions/FocusableAmountInput.tsx (1)
162-162:⚠️ Potential issueRenaming 'focused' prop to 'defaultFocused' may impact existing component usage
Changing the prop name from
focusedtodefaultFocusedmodifies the public API of theFocusableAmountInputcomponent. This can affect other parts of the codebase that rely on thefocusedprop. Please ensure that all instances whereFocusableAmountInputis used with thefocusedprop are updated to prevent any unexpected behavior.Run the following script to identify usages of the old
focusedprop:packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (5)
349-349: Consistent Disabling of Category FieldThe addition of
disabled={isOffBudget || isBudgetTransfer(transaction)}on line 349 ensures that the category field is disabled when the transaction is off-budget or a budget transfer. This maintains consistency with the intended behavior and prevents unintended category changes.
679-679: Enhance User Experience by Focusing Amount InputSetting
defaultFocused={true}on theFocusableAmountInputcomponent ensures that the amount field is focused when the page loads. This improves the user experience by allowing immediate data entry without additional clicks.
719-719: Disable Category Field for Off-Budget and Transfer TransactionsAdding
disabled={isOffBudget || isBudgetTransfer(transaction)}on line 719 aligns the parent transaction's category field behavior with the child transactions. This prevents editing the category when it's not applicable, maintaining consistency across the form.
783-783: Verify Disabling Account Change on Existing TransactionsThe account field is now disabled for existing transactions (
disabled={!adding}) on line 783. While this might prevent unintended changes, it also restricts users from correcting mistakes in the account selection.Please confirm that preventing account changes on existing transactions is intentional and doesn't hinder necessary edits. If users need the ability to change accounts, consider enabling this field.
1113-1120: Modernize Code with React HooksReplacing the
connectHOC with React hooks (useSelector,useCategories,useAccounts, etc.) simplifies the component and aligns with current best practices. This refactoring enhances readability and maintainability of the code.
| @@ -246,7 +245,7 @@ export const FocusableAmountInput = memo(function FocusableAmountInput({ | |||
| </Button> | |||
| )} | |||
| <Button | |||
| onClick={onFocus} | |||
| onClick={() => setFocused(true)} | |||
There was a problem hiding this comment.
Potential override of 'onClick' in 'Button' component
The onClick handler that sets focus may be unintentionally overridden by an onClick provided in buttonProps due to the order of prop spreading. Since ...buttonProps comes after onClick, any onClick in buttonProps will replace the internal handler, possibly preventing setFocused(true) from being called.
If the intention is to always set focus on click, consider adjusting the order and incorporating the buttonProps.onClick callback:
- <Button
- onClick={() => setFocused(true)}
- {...buttonProps}
+ <Button
+ {...buttonProps}
+ onClick={(e) => {
+ setFocused(true);
+ buttonProps?.onClick?.(e);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onClick={() => setFocused(true)} | |
| <Button | |
| {...buttonProps} | |
| onClick={(e) => { | |
| setFocused(true); | |
| buttonProps?.onClick?.(e); | |
| }} |
| onFocus={() => setFocused(true)} | ||
| onBlur={() => setFocused(false)} |
There was a problem hiding this comment.
Ensure prop handlers 'onFocus' and 'onBlur' are invoked
The onFocus and onBlur handlers passed via props to FocusableAmountInput are not being called. The internal handlers override them without invoking props.onFocus and props.onBlur. This could lead to unintended side effects if parent components rely on these handlers.
Update the handlers to include the prop callbacks:
onFocus={() => {
setFocused(true);
+ props.onFocus?.();
}}
onBlur={() => {
setFocused(false);
+ props.onBlur?.();
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onFocus={() => setFocused(true)} | |
| onBlur={() => setFocused(false)} | |
| onFocus={() => { | |
| setFocused(true); | |
| props.onFocus?.(); | |
| }} | |
| onBlur={() => { | |
| setFocused(false); | |
| props.onBlur?.(); | |
| }} |
| ...props | ||
| }: FocusableAmountInputProps) { | ||
| const [isNegative, setIsNegative] = useState(true); | ||
| const [focused, setFocused] = useState(defaultFocused ?? false); |
There was a problem hiding this comment.
Synchronize 'focused' state with 'defaultFocused' prop changes
The focused state is initialized using the defaultFocused prop. However, if defaultFocused changes after the component mounts, the focused state will not update accordingly. If the defaultFocused prop can change during the component's lifecycle, consider synchronizing the state using a useEffect hook to prevent inconsistent focus behavior.
Apply this diff to synchronize the focused state:
const [focused, setFocused] = useState(defaultFocused ?? false);
+ useEffect(() => {
+ setFocused(defaultFocused ?? false);
+ }, [defaultFocused]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [focused, setFocused] = useState(defaultFocused ?? false); | |
| const [focused, setFocused] = useState(defaultFocused ?? false); | |
| useEffect(() => { | |
| setFocused(defaultFocused ?? false); | |
| }, [defaultFocused]); |
| const transactionToEdit = transactions.find(t => t.id === transactionId); | ||
| const unserializedTransaction = unserializedTransactions.find( | ||
| t => t.id === transactionId, | ||
| ); | ||
| switch (name) { | ||
| case 'category': | ||
| dispatch( | ||
| pushModal('category-autocomplete', { | ||
| categoryGroups, | ||
| month: monthUtils.monthFromDate(unserializedTransaction.date), | ||
| onSelect: categoryId => { | ||
| onUpdate(transactionToEdit, name, categoryId); | ||
| }, | ||
| }), | ||
| ); | ||
| break; | ||
| case 'account': | ||
| dispatch( | ||
| pushModal('account-autocomplete', { | ||
| onSelect: accountId => { | ||
| onUpdate(transactionToEdit, name, accountId); | ||
| }, | ||
| }), | ||
| ); | ||
| break; | ||
| case 'payee': | ||
| dispatch( | ||
| pushModal('payee-autocomplete', { | ||
| onSelect: payeeId => { | ||
| onUpdate(transactionToEdit, name, payeeId); | ||
| }, | ||
| }), | ||
| ); | ||
| break; | ||
| default: | ||
| dispatch( | ||
| pushModal('edit-field', { | ||
| name, | ||
| month: monthUtils.monthFromDate(unserializedTransaction.date), | ||
| onSubmit: (name, value) => { | ||
| onUpdate(transactionToEdit, name, value); | ||
| }, | ||
| }), | ||
| ); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Prevent Potential Crash When Transaction Not Found
On lines 514-515, unserializedTransaction is accessed without checking if it exists. If the transaction is not found, accessing unserializedTransaction.date will cause a runtime error.
To safeguard against potential crashes, add a null check before accessing properties of unserializedTransaction:
const unserializedTransaction = unserializedTransactions.find(
t => t.id === transactionId,
);
+if (!unserializedTransaction) {
+ // Handle the case where the transaction is not found
+ return;
+}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@minajevs Could you update the branch with master? It looks like there's some merge conflicts.
Closes #3268
So the "bug" happened because
setTimeoutinonBlurhandler messed up event-loop. The current edited field was reset on the next cycle, afteronClickhandler of the next field was fired. RemovingsetTimeoutlinearizes event-loop and current edited field is reset beforeonClickis fired.