[Goals]: fix no messages #1507 #3721
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
|
|
@joel-jeremy I don't know how to add the nice undo button, but maybe that could be a separate PR. |
youngcw
changed the title
WalkthroughThe pull request introduces changes to the 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 (2)
packages/loot-core/src/client/actions/queries.ts (2)
41-45: Consider consistent formatting with other cases.While the implementation is correct, consider making the formatting consistent with other similar cases (like 'apply-goal-template') for better code uniformity.
Consider this more concise format:
- dispatch( - addNotification( - await send('budget/overwrite-goal-template', { month }), - ), - ); + dispatch( + addNotification(await send('budget/overwrite-goal-template', { month })), + );
36-45: Good choice using notifications over modal dialogs.The implementation leverages the existing notification system instead of modal dialogs, which is a better architectural choice because:
- It maintains consistency with other budget actions
- Notifications are less intrusive to the user workflow
- The existing notification system likely has proper styling and accessibility support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3721.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/loot-core/src/client/actions/queries.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/loot-core/src/client/actions/queries.ts (1)
36-38: LGTM! Implementation matches existing patterns.The notification dispatch implementation for 'apply-goal-template' follows the established pattern seen in other cases like 'check-templates', ensuring consistent user feedback.
Let's verify the notification handling implementation:
✅ Verification successful
Let me gather more information about the notification patterns in the codebase to ensure consistency.
Let me verify the implementation of the server-side handler to ensure it returns appropriate notification messages.
Based on the gathered information, I can now provide the final response:
Implementation correctly handles notifications for goal template operations
The implementation is verified to be correct and consistent because:
- The server-side handler types in
handlers.d.tsconfirm thatbudget/apply-goal-templatereturns aPromise<Notification>- The notification dispatch pattern matches other similar budget operations like
check-templates,overwrite-goal-template, andcleanup-goal-template- The implementation properly handles async/await and follows the established pattern of wrapping server responses in notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent notification handling patterns across budget actions # Expect: Similar dispatch(addNotification(await send(...))) pattern # Search for notification dispatch patterns in budget-related actions rg -U "case '.*?':\s+dispatch\(addNotification\(await send\('budget/"Length of output: 281
Script:
#!/bin/bash # Let's look for all notification patterns in budget-related actions with more context rg -U "case '.*?':.*?send\('budget/" -A 5 packages/loot-core/src/client/actions/queries.ts # Also check for other addNotification patterns rg "addNotification\(" -A 2 packages/loot-core/src/client/actions/queries.tsLength of output: 721
Script:
#!/bin/bash # Check the server-side implementation of the budget/apply-goal-template handler rg -U "budget/apply-goal-template.*?\{" -A 10 # Also look for the notification message format in similar handlers rg -U "budget/(check|cleanup|overwrite)-goal-template.*?\{" -A 10Length of output: 4461
fixes #1507