Use strict typing in useSheetValue and fix bug where query is not being updated when changed #3864
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. |
joel-jeremy
changed the title
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 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
|
WalkthroughThe pull request introduces several modifications across multiple components in the Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
packages/desktop-client/src/components/accounts/Reconcile.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
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 (4)
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts (1)
Line range hint
29-44: Consider extracting initial state for improved readability.While the implementation is correct, the initial state object could be extracted to a separate constant for better readability.
Consider this refactor:
- const [result, setResult] = useState<SheetValueResult<SheetName, FieldName>>({ - name: fullSheetName, - value: bindingObj.value === undefined ? null : bindingObj.value, - query: bindingObj.query, - }); + const initialState: SheetValueResult<SheetName, FieldName> = { + name: fullSheetName, + value: bindingObj.value === undefined ? null : bindingObj.value, + query: bindingObj.query, + }; + const [result, setResult] = useState<SheetValueResult<SheetName, FieldName>>(initialState);packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx (1)
28-28: LGTM! Consider adding explicit return type for useEnvelopeSheetValueThe nullish coalescing operator ensures
forNextMonthis always a number, preventing potential issues with undefined values. This is a good defensive programming practice.Consider adding an explicit return type for
useEnvelopeSheetValueto make the nullability more apparent at the call site:// In EnvelopeBudgetComponents.ts export function useEnvelopeSheetValue(query: Query): number | null { // ... implementation }packages/desktop-client/src/components/budget/envelope/BalanceMovementMenu.tsx (1)
23-24: LGTM! Consider adding explicit type annotation for improved type safety.The nullish coalescing operator ensures
catBalancealways has a valid numeric value, preventing potential issues with undefined values in the TransferMenu component.Consider adding an explicit type annotation for additional type safety:
- const catBalance = - useEnvelopeSheetValue(envelopeBudget.catBalance(categoryId)) ?? 0; + const catBalance: number = + useEnvelopeSheetValue(envelopeBudget.catBalance(categoryId)) ?? 0;packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx (1)
390-393: Great improvements in type safety and null handling!The changes effectively address two aspects:
- Added proper type parameters to
useSheetValueenhances type safety- The nullish coalescing operator ensures a safe default of 0, preventing potential issues with undefined values in calculations
Consider extracting the sheet value resolution into a custom hook for better reusability:
function useCategoryBalance(categoryId: string, budgetType: string) { const balanceBinding = budgetType === 'rollover' ? envelopeBudget.catBalance(categoryId) : trackingBudget.catBalance(categoryId); return useSheetValue<'envelope-budget' | 'tracking-budget', typeof balanceBinding>( balanceBinding, ) ?? 0; }This would simplify the component and make the balance calculation logic reusable across other components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3864.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/accounts/Reconcile.tsx(1 hunks)packages/desktop-client/src/components/autocomplete/CategoryAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/BalanceMenu.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/BalanceMovementMenu.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx(1 hunks)packages/desktop-client/src/components/spreadsheet/useSheetValue.ts(3 hunks)
🔇 Additional comments (9)
packages/desktop-client/src/components/budget/envelope/BalanceMenu.tsx (3)
Line range hint 8-18: LGTM! Well-structured TypeScript type definitions.
The component's props are properly typed with clear interfaces and good use of TypeScript features like Omit for prop manipulation.
Line range hint 35-83: LGTM! Well-implemented menu logic.
The menu implementation shows good practices:
- Proper conditional rendering based on balance state
- Good error handling for unknown menu options
- Clean callback handling with optional chaining
- Well-structured translations
32-33: Verify useEnvelopeSheetValue hook implementation.
While the nullish coalescing operator addition is good for handling undefined values, we should verify that useEnvelopeSheetValue properly handles query updates as mentioned in the PR objectives.
Let's check the hook implementation:
✅ Verification successful
Let me search for the implementation of useEnvelopeSheetValue since it wasn't found in the previous results.
Let me search for the implementation of useSheetValue since we found that useEnvelopeSheetValue is just a wrapper around it.
Let me check the spreadsheet implementation to understand how cell updates are handled.
Based on the code analysis, I can now provide a final response about the useEnvelopeSheetValue hook and its handling of query updates.
The hook implementation properly handles query updates
The useEnvelopeSheetValue hook is a type-safe wrapper around useSheetValue, which is backed by a robust spreadsheet implementation that:
- Uses a proper observer pattern through
observeCellto track cell dependencies - Maintains an LRU cache for performance optimization
- Listens to 'cells-changed' events and notifies all observers when updates occur
- Properly cleans up observers when components unmount
The nullish coalescing operator addition is a good defensive programming practice, but it wasn't related to any query update issues. The underlying spreadsheet system correctly propagates updates to all observers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useEnvelopeSheetValue implementation to verify dependency handling
ast-grep --pattern 'function useEnvelopeSheetValue($_) {
$$$
}'
# Also check for any useEffect/useLayoutEffect usage within the same file
rg -A 5 'use(Layout)?Effect.*\(' --type ts
Length of output: 60294
Script:
#!/bin/bash
# Search for useEnvelopeSheetValue implementation
rg -A 10 'useEnvelopeSheetValue' --type ts
# Also search for any related sheet value hooks
ast-grep --pattern 'function useSheetValue($_) {
$$$
}'
Length of output: 23484
Script:
#!/bin/bash
# Search for useSheetValue implementation
rg -A 15 'export function useSheetValue' --type ts
# Also look for the spreadsheet context implementation
rg -A 10 'SpreadsheetContext' --type ts
Length of output: 3402
Script:
#!/bin/bash
# Look for the Spreadsheet class implementation
rg -A 20 'class Spreadsheet' --type ts
# Also check for any cell update related methods
rg -A 10 'updateCell|observeCell|listen' packages/loot-core/src/client/SpreadsheetProvider.tsx
Length of output: 5204
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts (3)
15-22: Well-structured type definition!
The SheetValueResult type is well-designed with proper generic constraints and nullable handling. It effectively captures the structure of sheet values while maintaining type safety.
55-69: Bug fix confirmed: Query updates now properly tracked.
The addition of bindingObj.query to the effect dependencies ensures that the component correctly responds to query changes, fixing the reported bug.
50-54: Consider cleanup for query on unmount.
While the query creation is handled, there's no explicit cleanup when the component unmounts or when the query changes.
Let's check if there's a cleanup method in the spreadsheet API:
If a cleanup method exists, consider adding:
useLayoutEffect(() => {
if (bindingObj.query) {
spreadsheet.createQuery(sheetName, bindingObj.name, bindingObj.query);
}
+ return () => {
+ if (bindingObj.query) {
+ spreadsheet.removeQuery(sheetName, bindingObj.name);
+ }
+ };packages/desktop-client/src/components/budget/envelope/BalanceMovementMenu.tsx (1)
23-24: Verify query update behavior in useEnvelopeSheetValue hook.
Since the PR mentions fixing a bug where "query is not being updated when changed", we should verify that useEnvelopeSheetValue(envelopeBudget.catBalance(categoryId)) properly updates when the query changes.
Let's check the implementation of useEnvelopeSheetValue:
✅ Verification successful
Let me try a different search strategy to find the implementation:
Let me search for the implementation of the hook's dependencies to understand how it handles query updates:
Based on the search results, I found the release note "3864.md" that directly addresses the query update issue: "Use strict typing in useSheetValue and fix bug where query is not being updated when the query object changed". Additionally, I can see that useEnvelopeSheetValue is a thin wrapper around useSheetValue, which has been fixed to properly handle query updates.
No issues with query updates in useEnvelopeSheetValue
The PR has already fixed the query update behavior in useSheetValue, which is used by useEnvelopeSheetValue. The fix ensures that sheet values are properly updated when the query object changes. Since useEnvelopeSheetValue is just a type-safe wrapper around the fixed useSheetValue, it will correctly handle query updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useEnvelopeSheetValue implementation and its dependencies
echo "Searching for useEnvelopeSheetValue implementation..."
rg -t typescript -t javascript "export.*function useEnvelopeSheetValue" -A 10
echo "Checking for related test files..."
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts envelope
Length of output: 370
Script:
#!/bin/bash
# Search for useEnvelopeSheetValue with ripgrep without file type restriction
echo "Searching for useEnvelopeSheetValue..."
rg "useEnvelopeSheetValue" -A 10
# Search for files containing "envelope" in their name
echo -e "\nSearching for envelope-related files..."
fd envelope
Length of output: 31003
Script:
#!/bin/bash
# Search for useSheetValue implementation since useEnvelopeSheetValue uses it
echo "Searching for useSheetValue implementation..."
rg "useSheetValue" -A 10
# Search for sheet value update related code
echo -e "\nSearching for sheet value update mechanisms..."
ast-grep --pattern 'function $_(sheet: $_) {
$$$
query $$$
$$$
}'
Length of output: 36448
packages/desktop-client/src/components/accounts/Reconcile.tsx (2)
39-41: LGTM: Safe handling of cleared balance
The nullish coalescing operator ensures that cleared is always a number, making the targetDiff calculation and subsequent conditional rendering more robust.
32-38: Verify query dependency handling in useSheetValue hook
The type safety improvements and default value handling look good. However, given the PR's objective to fix a bug where "query is not being updated when changed", we should verify that the useSheetValue hook properly handles query updates.
Let's verify the hook's implementation:
✅ Verification successful
Query updates are properly handled in useSheetValue hook
Looking at the implementation, the hook correctly handles query updates through the useLayoutEffect dependency array which includes bindingObj.query. When the query changes, the effect will re-run, destroying the old query binding and creating a new one with the updated query. This ensures that changes to the query object will properly trigger updates.
The implementation shows:
- Query is included in the effect dependencies:
[sheetName, bindingObj.name, bindingObj.query] - Old query binding is cleaned up through the effect cleanup function
- New query is created with
spreadsheet.createQuerywhen the effect re-runs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useSheetValue implementation to verify query dependency handling
ast-grep --pattern 'function useSheetValue<$_>($_) {
$$$
}'
Length of output: 63
Script:
#!/bin/bash
# Let's try a different pattern to find useSheetValue implementation
ast-grep --pattern 'useSheetValue = $_'
# Also search with ripgrep for any occurrence of useSheetValue
rg "useSheetValue" -A 10
Length of output: 39193
Script:
#!/bin/bash
# Let's look for the useSheetValue implementation in the spreadsheet directory
rg -l "useSheetValue" packages/desktop-client/src/components/spreadsheet/
# And then get the content of the useSheetValue implementation
cat packages/desktop-client/src/components/spreadsheet/useSheetValue.ts
Length of output: 2251
|
/update-vrt |
1 similar comment
|
/update-vrt |
|
@joel-jeremy looks like VRT is failing on this one again |
joel-jeremy
force-pushed
the
useSheetValue-fix-and-ts
branch
from
eaaff53 to
cb9f778
Compare
TS strict typing for useSheetValue and fix an issue where the sheet value uses the old query object whenever the query changes due to the query not being part of the
useLayoutEffectdependencies.