[TypeScript] Add types to SpreadsheetProvider #4409
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
joel-jeremy
changed the title
joel-jeremy
changed the title
| setAmount(integerToCurrency(Math.max(node.value as number, 0))); | ||
| })(); | ||
| }, []); | ||
| useSheetValue<'envelope-budget', 'to-budget'>('to-budget', ({ value }) => { |
There was a problem hiding this comment.
Replaced with useSheetValue which does the same thing
WalkthroughThis pull request implements several updates across multiple modules. In the desktop client, the Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 2
🧹 Nitpick comments (1)
packages/loot-core/src/client/SpreadsheetProvider.tsx (1)
28-38: Consider improving type organization and documentation.The TODO comment suggests making the
Bindingtype generic. This would improve type flexibility and reusability.Consider updating the type definition to be generic:
-type Binding = - | string - | { name: string; value?: unknown | null; query?: Query | undefined }; +type Binding<T = unknown> = + | string + | { name: string; value?: T | null; query?: Query | undefined };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4409.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/desktop-client/src/components/budget/envelope/HoldMenu.tsx(1 hunks)packages/desktop-client/src/components/spreadsheet/useSheetValue.ts(1 hunks)packages/loot-core/package.json(1 hunks)packages/loot-core/src/client/SpreadsheetProvider.tsx(5 hunks)packages/loot-core/src/platform/server/sqlite/unicodeLike.ts(1 hunks)packages/loot-core/src/server/db/index.ts(3 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)packages/loot-core/webpack/webpack.browser.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (14)
packages/loot-core/src/platform/server/sqlite/unicodeLike.ts (1)
1-3: LGTM! Type-safe LRU cache implementation.The changes properly implement type safety for the LRU cache by:
- Using named import
LRUCachefrom the upgraded package- Adding appropriate type parameters
<string, RegExp>to specify key-value typespackages/loot-core/src/server/db/index.ts (2)
11-11: LGTM! Type-safe LRU cache import.The import statement correctly uses the named import
LRUCachefrom the upgraded package.
135-135: LGTM! Type-safe query cache implementation.The changes properly implement type safety for the query cache by:
- Adding appropriate type parameters
<string, string>to specify key-value types- Maintaining consistency in cache initialization across the codebase
Also applies to: 148-148
packages/loot-core/package.json (1)
33-33:❓ Verification inconclusive
Verify compatibility with lru-cache v11.0.2.
The upgrade from v5.1.1 to v11.0.2 is a major version bump that might introduce breaking changes.
🌐 Web query:
What are the breaking changes between lru-cache v5 and v11?💡 Result:
The
lru-cachelibrary has undergone several significant breaking changes between versions 5 and 11:
Version 7
- New algorithm/data structure for better performance ([1][7])
- API renames:
stale→allowStalemaxAge→ttllength→sizeCalculationdel()→delete()reset()→clear()- Deprecated positional parameters in
set(),get(), andhas()in favor of options objects- Changed serialization format for
dump()/load()([1])
Version 8
- TypeScript rewrite with major type definition changes ([1][7])
- Node.js requirement bumped to ≥16.14 (removed AbortController polyfill) ([1][7])
- Keys/values can no longer be
nullorundefined([1][7])- Renamed
fetchContextoption tocontext([1][7])- Made internal properties truly private ([1])
Version 9
- Removed default export (only named exports available) ([1][7])
- Reintroduced minimal polyfill with usage warnings ([1])
Version 10
- Changed
cache.fetch()return type fromPromise<V | void>toPromise<V | undefined>(TypeScript impact) ([1])- Made
cache.set(key, undefined)equivalent to deletion ([1])
Version 11
- Dropped Node.js <20 support ([1][3])
- Removed deprecated APIs and legacy polyfills ([1])
Key architectural impacts:
- Migration from JavaScript to TypeScript
- Multiple option/method renames
- Progressive tightening of Node.js version requirements
- Serialization format incompatibility between versions
- Changed type definitions affecting TypeScript users
Developers upgrading should check both their usage of renamed APIs and their Node.js runtime compatibility. The library now requires modern JavaScript environments but offers better performance and type safety ([1][7][3]).
Citations:
- 1: https://github.com/isaacs/node-lru-cache/blob/main/CHANGELOG.md
- 2: https://en.wikipedia.org/wiki/Cache_replacement_policies
- 3: Dependency package lru-cache has a new version available Azure/azure-sdk-for-js#30410
- 4: https://algo.monster/liteproblems/146
- 5: https://algo.monster/problems/lru_cache
- 6: https://arxiv.org/pdf/1806.10853
- 7: https://www.npmjs.com/package/lru-cache?activeTab=versions
- 8: isaacs/node-lru-cache@v11.0.1...v11.0.2
- 9: Breaking changes not documented isaacs/node-lru-cache#115
- 10: https://www.mtu.edu/cs/graduate/dissertations/pdfs/efficient-modeling-of-random-sampling-based-iru-cache.pdf
Attention: Verify lru-cache v11 Compatibility
The upgrade from v5.1.1 to v11.0.2 is a major change that introduces several breaking modifications. Please confirm that the usage of lru-cache across the codebase is updated accordingly. In particular:
- API Renames & Options Changes:
- Update option names:
maxAge→ttl,stale→allowStale, andlength→sizeCalculation.- Adjust method calls:
del()should now bedelete(), andreset()should be replaced withclear().- TypeScript & Internal Changes:
- Ensure TypeScript definitions are properly aligned given the complete rewrite in newer versions.
- Node.js Runtime:
- Verify that the deployment environment meets the new requirement (Node.js ≥20).
Please review all locations where lru-cache is used to confirm the new API is correctly integrated.
packages/desktop-client/src/components/budget/envelope/HoldMenu.tsx (2)
1-1: LGTM! Import statements are now more focused and type-safe.The changes improve code organization by:
- Explicitly importing types from React
- Replacing
useSpreadsheetwith the more specificuseSheetValuehookAlso applies to: 12-12
21-23: LGTM! Sheet value handling is now more direct and efficient.The implementation:
- Directly uses
useSheetValuefor accessing the budget amount- Properly handles type conversion with
integerToCurrency- Ensures non-negative values with
Math.maxpackages/desktop-client/src/components/spreadsheet/useSheetValue.ts (1)
14-20: LGTM! Type definitions are now more focused and precise.The
SheetValueResulttype is streamlined by removing the unnecessaryqueryproperty, making the interface cleaner and more maintainable.packages/loot-core/webpack/webpack.browser.config.js (1)
65-70:❓ Verification inconclusive
Verify the impact of disabling fully specified imports.
The new rule allows for more flexible module resolution by setting
fullySpecified: false. While this can improve developer experience, it's important to ensure it doesn't introduce any unexpected side effects.Let's check for any potential issues with module resolution:
🏁 Script executed:
#!/bin/bash # Search for .js imports that might be affected by this change rg -t typescript -t javascript "from ['\"](\.{1,2}/[^'\"]+)(?<!\.js)['\"]" -g '!dist' -g '!node_modules'Length of output: 380
Action: Verify module resolution behavior with updated regex tooling
The webpack rule update setting
fullySpecified: falseis intended to ease module imports, but our initial verification command couldn't run as expected due to regex engine limitations (unsupported look-around). To accurately check if any relative.jsimports might be inadvertently affected, please re-run the search with PCRE2 enabled. For example:#!/bin/bash # Re-run the search using PCRE2 to support look-around assertions rg --pcre2 -t typescript -t javascript "from ['\"](\.{1,2}/[^'\"]+)(?<!\.js)['\"]" -g '!dist' -g '!node_modules'If this adjusted command produces no output, it would indicate that no problematic imports are present. However, manual verification is recommended to ensure no unexpected side effects occur.
packages/loot-core/src/client/SpreadsheetProvider.tsx (3)
15-18: LGTM! Context type safety is improved.The
SpreadsheetContextis now properly typed withundefined, making the type system more accurate and preventing potential runtime errors.
20-26: LGTM! Error handling for context usage is now more robust.The
useSpreadsheethook now properly validates context availability with a clear error message.
41-43: LGTM! Cache implementation is now properly typed.The LRU cache initialization is now type-safe with proper generic type parameters.
packages/loot-core/src/types/server-handlers.d.ts (3)
133-136: LGTM! Type definition enhances type safety.The return type is well-defined and properly references the
SpreadsheetNodetype for bothnameandvalueproperties.
138-142: LGTM! Type definition provides flexibility with optional value.The return type is well-structured with:
- Required
nameproperty for identification- Optional
valueproperty for flexibility
144-146: LGTM! Type definition is clear and focused.The return type correctly specifies an array of cell names using the
SpreadsheetNode['name']type.
| const newCastedResult = { | ||
| name: newResult.name, | ||
| // TODO: Spreadsheets, SheetNames, SheetFields, etc must be moved to the loot-core package | ||
| value: newResult.value as Spreadsheets[SheetName][FieldName], | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider moving type definitions to improve code organization.
The TODO comment indicates that type definitions should be moved to the loot-core package. This would improve code organization and make types more accessible across the project.
Let's verify the current location of type definitions:
🏁 Script executed:
#!/bin/bash
# Search for type definitions across the codebase
rg -t typescript "type (Spreadsheets|SheetNames|SheetFields)" -g '!dist' -g '!node_modules'Length of output: 128
I've noticed that the initial search command failed because of an unrecognized file type. To gather more reliable information we should rerun the search using a glob that targets TypeScript files. For example:
Once you verify where these type definitions are currently declared (or if they're even declared here), please confirm whether they might benefit from being centralized in the loot-core package as suggested by the TODO comment.
Action: Verify the location of the type definitions for Spreadsheets, SheetNames, and SheetFields, and consider migrating them into the loot-core package for improved organization and reusability.
- File in question:
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts(Lines 58-62) - Suggestion: Centralize these type definitions (currently defined locally or imported inconsistently) in the loot-core package to avoid duplication and improve maintainability.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Corrected search for type definitions across TypeScript files in the codebase
rg "type\s+(Spreadsheets|SheetNames|SheetFields)" -g "*.{ts,tsx}" -g '!dist' -g '!node_modules'Length of output: 2241
Consolidate Spreadsheet Type Definitions Across Packages
- The TODO in
packages/desktop-client/src/components/spreadsheet/useSheetValue.tsadvises moving spreadsheet-related types to the loot-core package. - Verification shows that SheetNames and SheetFields are already defined in
packages/loot-core/src/client/queries.ts, while Spreadsheets is still defined under the desktop-client (seepackages/desktop-client/src/components/spreadsheet/index.ts). - To improve maintainability and ensure all related types are centralized, consider moving the Spreadsheets type—and possibly consolidating all spreadsheet types—into the loot-core package and updating the imports accordingly.
Co-authored-by: Michael Clark <5285928+MikesGlitch@users.noreply.github.com>
No description provided.