🐛 fix misaligned gocardless credential popover #3942
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
|
MatissJanis
changed the title
|
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/modals/CreateAccountModal.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. WalkthroughThe pull request introduces modifications to two files: 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: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (1)
Line range hint
127-139: Consider optimizing credential reset functionsThe current implementation makes sequential API calls to reset credentials. Consider parallelizing these calls for better performance.
Example optimization for
onGoCardlessReset:const onGoCardlessReset = () => { - send('secret-set', { - name: 'gocardless_secretId', - value: null, - }).then(() => { - send('secret-set', { - name: 'gocardless_secretKey', - value: null, - }).then(() => { - setIsGoCardlessSetupComplete(false); - }); - }); + Promise.all([ + send('secret-set', { + name: 'gocardless_secretId', + value: null, + }), + send('secret-set', { + name: 'gocardless_secretKey', + value: null, + }) + ]).then(() => { + setIsGoCardlessSetupComplete(false); + }); };Similar optimization can be applied to
onSimpleFinReset.Also applies to: 140-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3942.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/modals/CreateAccountModal.tsx(7 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🔇 Additional comments (4)
packages/loot-core/src/types/server-handlers.d.ts (1)
181-181: Verify the impact of the breaking type change
The type signature change from string to string | null for the value parameter aligns with the credential reset functionality. However, we should verify that this breaking change doesn't affect other callers of this method.
✅ Verification successful
Type change for secret-set is safe and already supported by all callers
The type signature change from string to string | null for the value parameter is not a breaking change as all existing callers already handle both cases:
GoCardlessInitialiseModal.tsx: Sets string values for credentialsCreateAccountModal.tsx: Setsnullvalues during reset operationsSimpleFinInitialiseModal.tsx: Sets string values for tokens
The type change actually makes the interface more accurate by reflecting the existing behavior in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to secret-set to verify they handle the new type
ast-grep --pattern 'send("secret-set", { $$$, value: $_$$$ })'
Length of output: 64
Script:
#!/bin/bash
# Let's try a different pattern to find secret-set usage
rg -l "secret-set"
# Then examine the content of these files
rg "secret-set" -A 2 -B 2
Length of output: 4235
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (3)
33-38: LGTM! Type annotations improve code clarity
The explicit type annotations for state variables enhance type safety and make the code more maintainable.
Line range hint 248-275: Great improvement using DialogTrigger!
The implementation:
- Fixes the misaligned popover issue
- Improves accessibility with proper aria-labels
- Simplifies state management by removing manual isOpen handling
Line range hint 311-334: LGTM! Consistent implementation with GoCardless menu
The SimpleFIN menu follows the same pattern as the GoCardless menu, maintaining consistency in the codebase.
Closes #3854
Additionally..
refs andisOpenstate handling - since we are now using React-aria buttons - we can leverage the power ofDialogTrigger- which removes the need forref