fix: pushModal not dispatched in GoCardless linking #3515
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request modifies the Changes
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 (
|
✅ 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
|
![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/gocardless.ts (1)
Line range hint
1-55: Suggestions for improving the overall code qualityWhile the main issue has been addressed, consider the following improvements to enhance the code quality and maintainability:
Enhance error handling in the
onMoveExternalcallback. Currently, it only returns the error response without any specific error handling or logging.Consider making the
accessValidForDaysparameter configurable instead of hardcoding it to 90 days. This could be done by accepting it as an optional parameter in theauthorizeBankfunction.Add comments to explain the purpose of functions and any complex logic, especially for the
_authorizefunction and its callbacks.Consider adding type annotations for the
dataparameter in theonSuccesscallback of theauthorizeBankfunction to improve type safety.Here's an example of how you could implement some of these suggestions:
import { type Dispatch } from 'loot-core/client/actions/types'; import { pushModal } from 'loot-core/src/client/actions/modals'; import { send } from 'loot-core/src/platform/client/fetch'; import { type GoCardlessToken } from 'loot-core/src/types/models'; // Helper function to handle GoCardless authorization process function _authorize( dispatch: Dispatch, upgradingAccountId: string | undefined, { onSuccess, onClose, accessValidForDays = 90, }: { onSuccess: (data: GoCardlessToken) => Promise<void>; onClose?: () => void; accessValidForDays?: number; }, ) { dispatch( pushModal('gocardless-external-msg', { onMoveExternal: async ({ institutionId }) => { try { const resp = await send('gocardless-create-web-token', { upgradingAccountId, institutionId, accessValidForDays, }); if ('error' in resp) { console.error('Error creating web token:', resp.error); return resp; } const { link, requisitionId } = resp; window.Actual?.openURLInBrowser(link); return send('gocardless-poll-web-token', { upgradingAccountId, requisitionId, }); } catch (error) { console.error('Error in onMoveExternal:', error); throw error; } }, onClose, onSuccess, }), ); } // Main function to initiate GoCardless bank authorization export async function authorizeBank( dispatch: Dispatch, { upgradingAccountId, accessValidForDays = 90, }: { upgradingAccountId?: string; accessValidForDays?: number; } = {}, ) { _authorize(dispatch, upgradingAccountId, { onSuccess: async (data: GoCardlessToken) => { dispatch( pushModal('select-linked-accounts', { accounts: data.accounts, requisitionId: data.id, upgradingAccountId, syncSource: 'goCardless', }), ); }, accessValidForDays, }); }These changes improve error handling, make the
accessValidForDaysparameter configurable, add comments for clarity, and improve type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/gocardless.ts (1 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/gocardless.ts (1)
48-55: LGTM! This change addresses the reported issue.The modification correctly wraps the
pushModalcall with adispatchfunction, which should resolve the issue of the modal not being displayed after a successful GoCardless callback. This change aligns with Redux patterns and maintains the existing functionality by keeping the same parameters forpushModal.To ensure this fix resolves the issue completely, please run the following verification steps:
- Test the GoCardless linking process end-to-end.
- Verify that the
SelectLinkedAccountsModalis displayed after a successful callback from GoCardless.- Check the Redux DevTools (if available) to confirm that the
pushModalaction is correctly dispatched.If possible, it would be beneficial to add or update unit tests to cover this scenario and prevent regression.
EtaoinWu
force-pushed
the
gocardless-link-dispatch-fix
branch
from
20b55f4 to
ab78b1c
Compare
|
@EtaoinWu Nice catch! Thank you for this PR. Can you please add a release note so we can get this in? :) |
EtaoinWu
changed the title
Background
Multiple reports in Discord shows that the current master branch has issues with gocardless linking: mine, valerio's, aaarzan's, ROuGGy's among others. The symptom seems to be that, after a successful callback from GoCardless, Actual refuses to pop up the next step in the linking process, which is to select accounts to sync from. Server logs and browser console logs shows no anomaly.
Contribution
I tracked down the issue, and found that in gocardless.ts, the callback
onSuccessonly generates apushModalaction without sending it to dispatch:Therefore a
SelectLinkedAccountsModalwas never generated.This was introduced in 420aad0 (#3413) two weeks ago. Latest release (v24.9.0) was not affected.
Next steps & help needed
I am unfortunately not a React dev, and lacks the environment or tool to build and test it. I hope that other contributors in this community can help verify my fix and resolve this issue.