Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
youngcw
changed the title
📝 WalkthroughWalkthroughAdded explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/loot-core/src/server/rules/action.ts (1)
306-307: Centralize formula date formats to avoid future drift.At Line 306, this list is now duplicated across server and both client hooks. Please extract one shared constant and reuse it in all HyperFormula init sites.
As per coding guidelines:
packages/*/src/**/*.{ts,tsx,js,jsx}: Prefer iteration and modularization over code duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/rules/action.ts` around lines 306 - 307, The hard-coded dateFormats array (dateFormats: ['DD/MM/YYYY', 'YYYY-MM-DD', 'YYYY/MM/DD']) is duplicated across the server and client HyperFormula init sites; extract this array into a single exported constant (e.g., DATE_FORMATS) in a shared module and import it wherever HyperFormula is initialized (referencing the dateFormats symbol and each HyperFormula init site such as the server rule initializer in action.ts and the two client hook initializers) so all places use the shared constant instead of duplicating the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/loot-core/src/server/rules/action.ts`:
- Around line 306-307: The hard-coded dateFormats array (dateFormats:
['DD/MM/YYYY', 'YYYY-MM-DD', 'YYYY/MM/DD']) is duplicated across the server and
client HyperFormula init sites; extract this array into a single exported
constant (e.g., DATE_FORMATS) in a shared module and import it wherever
HyperFormula is initialized (referencing the dateFormats symbol and each
HyperFormula init site such as the server rule initializer in action.ts and the
two client hook initializers) so all places use the shared constant instead of
duplicating the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e271749-5b8b-4c4f-beb3-a70fe4308111
📒 Files selected for processing (4)
packages/desktop-client/src/hooks/useFormulaExecution.tspackages/desktop-client/src/hooks/useTransactionFormulaExecution.tspackages/loot-core/src/server/rules/action.tsupcoming-release-notes/7373.md
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
matt-fidd
left a comment
matt-fidd
left a comment
There was a problem hiding this comment.
I don't think I realised we have 3 separate instances of hyperformula. Do you know if this is how it's usually used?
| @@ -329,6 +329,7 @@ export function useFormulaExecution( | |||
| licenseKey: 'gpl-v3', | |||
| localeLang: typeof locale === 'string' ? locale : 'en-US', | |||
| language: 'enUS', | |||
| dateFormats: ['DD/MM/YYYY', 'YYYY-MM-DD', 'YYYY/MM/DD'], | |||
There was a problem hiding this comment.
This is not a breaking change as non-US dates are used by default, but for the future we should look at using the user date pref to set this if possible.
There was a problem hiding this comment.
I agree that using locales + the Actual format would be best.
They all are used for different things. I don't see why we couldn't have a single core set of HF code, that then gets used in 3 different ways. It took me floundering around not understanding why my changes were doing anything to figure out there were multiple instances. |
2 participants
Description
addresses #5949 (comment)
This also modifies what counts as a date. Are there other formats we should use? The trade off is that anything that matches a date format is treated as a date and will need formatted if used in a note.
Related issue(s)
Testing
Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
locale/zh-Hans.jsonsrc/hooks/useFormulaExecution.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/rules/action.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/rules/action.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged