Fix bugs in payee table #3768
Fix bugs in payee table #3768
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
|
WalkthroughThe changes introduced in this pull request involve modifications to the 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: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/payees/PayeeTable.tsx (1)
58-58: LGTM! Clean integration of navigation controls.The navigator prop correctly connects the table navigation configuration to the Table component. This approach provides a maintainable solution by centralizing navigation control logic.
Consider documenting this navigation control pattern in your component guidelines, as it provides a reusable solution for similar field accessibility requirements in other tables.
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (2)
Line range hint
151-156: Simplify the conditional rendering logic.The explicit
returnfor theelsecase is redundant sinceundefinedis the default return value.Consider simplifying the code:
{() => { if (payee.favorite) { return <SvgBookmark />; - } else { - return; } }}
Line range hint
53-54: Consider improving type safety for theonEditcallback.The field parameter in the
onEditcallback could be made more type-safe by using a union type of valid field names instead of string.Consider updating the type:
- onEdit: (id: PayeeEntity['id'], field: string) => void; + onEdit: (id: PayeeEntity['id'], field: 'name' | 'rule-count' | 'select') => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3768.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/payees/PayeeTable.tsx(2 hunks)packages/desktop-client/src/components/payees/PayeeTableRow.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (2)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-09-24T20:31:09.185Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-10-08T15:46:15.739Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
🔇 Additional comments (3)
packages/desktop-client/src/components/payees/PayeeTable.tsx (2)
49-53: LGTM! Navigation control effectively prevents transfer payee selection.
The implementation correctly restricts navigation options for transfer payees by only allowing access to 'rule-count', while non-transfer payees can access 'select', 'name', and 'rule-count'. This addresses the Edge browser issue by controlling field accessibility at the navigation level.
Let's verify the completeness of this fix:
#!/bin/bash
# Description: Check for any other places in the codebase where transfer payee selection might be handled
# to ensure consistent behavior across the application.
# Search for transfer_acct related conditions
rg -A 3 "transfer_acct"
# Search for payee selection related code
ast-grep --pattern 'onSelect($$$)'49-58: Verify accessibility implications of navigation restrictions.
While the implementation effectively prevents transfer payee selection, we should ensure that screen readers and keyboard navigation provide appropriate feedback when attempting to access restricted fields.
Let's check for accessibility-related code:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Navigation restrictions are properly handled for keyboard accessibility
The implementation correctly handles keyboard navigation through the useTableNavigator hook, which respects the field restrictions for transfer payees. The Table component includes proper keyboard event handling that:
- Manages focus navigation between cells
- Handles enter/tab keys for field navigation
- Prevents navigation to restricted fields through keyboard events
The navigation restrictions are consistently applied across both mouse and keyboard interactions, ensuring a coherent experience for all users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for accessibility attributes and aria labels
# that might need updating with the new navigation restrictions
# Search for aria-related attributes
rg -g '*.tsx' -g '*.jsx' "aria-"
# Search for accessibility-related tests
fd -e test.tsx -e test.jsx -e spec.tsx -e spec.jsx --exec rg -l "accessibility|aria|screen.reader|keyboard"
Length of output: 31112
Script:
#!/bin/bash
# Look for keyboard navigation and accessibility related code in the Table component
# and its usage in PayeeTable
# Search for keyboard navigation in Table component
ast-grep --pattern 'onKeyDown($$$)'
# Search for aria attributes and roles in Table component
rg -g '*.tsx' -g '*.jsx' -A 3 'aria-|role=' packages/desktop-client/src/components/table.tsx
# Look for keyboard navigation tests
rg -g '*.test.*' "keyboard|keydown|keypress|keyup" packages/desktop-client/src/components/
Length of output: 794
Script:
#!/bin/bash
# Look for the Table component implementation and its navigation logic
# Search for useTableNavigator hook implementation
ast-grep --pattern 'useTableNavigator($$$)'
# Search for Table component's keyboard handling
rg -g '*.tsx' -g '*.jsx' -A 5 'onKeyDown|handleKeyDown' packages/desktop-client/src/components/table.tsx
# Look for any accessibility-related props or state in PayeeTable
rg -g '*.tsx' -g '*.jsx' -A 5 'aria-|role=|tabIndex' packages/desktop-client/src/components/payees/PayeeTable.tsx
Length of output: 2594
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (1)
136-138: LGTM! This fixes the Edge browser selection issue.
The guard clause effectively prevents selection of transfer payees, addressing the reported Edge browser issue where users could select transfer payees despite invisible checkboxes.
Let's verify there are no other places where transfer payees can be selected:
✅ Verification successful
Based on the search results, I can now verify the review comment. The codebase shows that:
transfer_acctis a field in the payee model that links to an account for transfer payees- The guard clause in PayeeTableRow.tsx correctly prevents selection of transfer payees (when
payee.transfer_acct != null) - This is consistent across the codebase - transfer payees are treated specially and are generally not selectable/editable
- The UI also reflects this by showing transfer payees differently (with subdued colors and read-only fields)
The guard clause is correctly implemented and aligns with the codebase's handling of transfer payees.
The review comment was correct in approving this change as it properly prevents selection of transfer payees in the Edge browser, consistent with how transfer payees are handled throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential selection handlers that might need similar guards
ast-grep --pattern 'onSelect={$_}'
Length of output: 36
Script:
#!/bin/bash
# Search for files containing "onSelect" using ripgrep for better coverage
rg "onSelect" --type typescript --type tsx -C 2
# Also search for potential selection handlers with transfer payees
rg "transfer_acct" --type typescript --type tsx -C 2
Length of output: 170
Script:
#!/bin/bash
# Search for files containing "onSelect" without type filtering
rg "onSelect" -C 2
# Also search for potential selection handlers with transfer payees
rg "transfer_acct" -C 2
Length of output: 123458
Two bugs:
Ideally we'd have tests for these features. Not feeling super well right now so if someone wants to take a stab at it in the future, I'll make an issue.