Misc Cleanup #4455
Misc Cleanup #4455
Conversation
✅ 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
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
Smaller No assets were smaller Unchanged No assets were unchanged |
WalkthroughThe changes span multiple components across the project. In the desktop client, the Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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
🧹 Nitpick comments (1)
packages/loot-core/src/server/post.ts (1)
18-22: Consider adding error boundary for JSON parsing.While the JSON parsing logic is correct, consider wrapping it in a try-catch block to handle malformed JSON responses gracefully.
const contentType = res.headers.get('Content-Type') ?? ''; if (contentType.toLowerCase().indexOf('application/json') !== -1) { + try { const json = JSON.parse(text); throw new PostError(json.reason); + } catch (err) { + throw new PostError('invalid-json-response'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4455.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/modals/CreateAccountModal.tsx(1 hunks)packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx(2 hunks)packages/loot-core/bin/build-api(1 hunks)packages/loot-core/bin/build-browser(1 hunks)packages/loot-core/src/server/accounts/app.ts(1 hunks)packages/loot-core/src/server/post.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/loot-core/bin/build-api
- packages/loot-core/src/server/accounts/app.ts
- packages/loot-core/bin/build-browser
🧰 Additional context used
🧠 Learnings (1)
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (1)
Learnt from: dkhalife
PR: actualbudget/actual#3984
File: packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx:43-68
Timestamp: 2025-01-03T03:27:48.005Z
Learning: We have 2 types for accounts: `NormalizedAccount` (bank account ID field is `id`) and `AccountEntity` (database ID field is `id` plus a `account_id` that stores the bank’s ID). Accordingly, `account_id` shouldn't be replaced by `id` in `AccountEntity`.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Visual regression
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
packages/loot-core/src/server/post.ts (3)
8-12: LGTM! Improved HTTP status code handling.The change to accept all 2xx status codes as success is a good improvement, aligning with HTTP standards where all 2xx codes indicate successful responses.
14-16: Simplify error handling for 500 status.The simplified error handling for 500 status is cleaner and more maintainable.
24-34: LGTM! Well-documented tunnel error handling.The tunnel error detection is well-implemented and clearly documented.
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (1)
54-54: LGTM! Simplified function logic.The direct call to
authorizeBank(dispatch)removes unnecessary conditional branching, making the code cleaner and more maintainable.packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (2)
81-85: LGTM! Improved code readability.The extracted
upgradingIdassignment logic improves readability while correctly handling account IDs. The implementation aligns with the established pattern of distinguishing between bank account IDs and database IDs.
92-93: LGTM! Consistent usage of upgradingId.The consistent usage of the extracted
upgradingIdin bothlinkAccountSimpleFinandlinkAccountcalls maintains code clarity.Also applies to: 101-102
This PR addresses miscellaneous little nitpicks and fixes 2 bugs
The repo previously didn't run on Windows - although the rest of the bash scripts worked fine, the
findcommand to build the index of data files was defaulting tofind.exeinstead of/usr/bin/find. This meant that it built and spun up, but then I was plagued by errors because the default file system wasn't being created properly!secret-check always failed - the headers weren't being set correctly and so the token wasn't being passed. This PR fixes that issue, although I'm not sure it's actually used anywhere in the main codebase
The rest of the code is mostly just removing duplications and making things easier to read. There's one area of contention I can forsee though - I've updated
throwIfNot200to effectively function asthrowIfNot2xx, but I wasn't sure if this is the behaviour that's desired or if we want to just useresponse.ok, or if it is correct to only be 200 codes