Optimize useSheetValue #3879
Optimize useSheetValue #3879
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 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 No assets were bigger Smaller
Unchanged No assets were unchanged |
joel-jeremy
force-pushed
the
optiminze-useSheetValue
branch
from
76034c7 to
3226417
Compare
WalkthroughThe pull request introduces several modifications across multiple files related to spreadsheet functionality. In Possibly related PRs
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
packages/desktop-client/src/components/spreadsheet/useSheetValue.tsOops! 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. 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 (6)
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts (2)
33-39: Consider strengthening the memoization dependenciesThe memoization is a good optimization, but it might miss deep changes in the binding object's query property.
Consider updating the dependency array to catch deep changes:
const bindingObj = useMemo( () => typeof binding === 'string' ? { name: binding, value: null, query: undefined } : binding, - [binding], + [typeof binding === 'string' ? binding : binding.name, binding.value, binding.query ? JSON.stringify(binding.query) : undefined], );
78-83: Consider memoizing query serializationThe
JSON.stringify(bindingObj.query.serialize())in the dependency array will run on every render. For large queries, this could impact performance.Consider memoizing the serialized query:
+ const serializedQuery = useMemo( + () => bindingObj.query ? JSON.stringify(bindingObj.query.serialize()) : null, + [bindingObj.query] + ); useLayoutEffect(() => { // ... effect code ... }, [ spreadsheet, sheetName, bindingObj.name, - bindingObj.query ? JSON.stringify(bindingObj.query.serialize()) : null, + serializedQuery, ]);packages/loot-core/src/mocks/spreadsheet.ts (1)
Line range hint
26-41: Consider adding TypeScript types for better type safety.Since this is a TypeScript file (with strict checks currently ignored), consider adding types for the parameters and return value:
- bind(sheetName, binding, cb) { + bind( + sheetName: string, + binding: { name: string }, + cb: (node: { name: string; sheet: string; value: unknown }) => void + ): () => void {packages/loot-core/src/client/SpreadsheetProvider.tsx (3)
69-71: Consider adding error handling for query creation.The new query creation logic should handle potential errors from the
createQuerycall to ensure robust error handling and prevent silent failures.if (binding.query) { - this.createQuery(sheetName, binding.name, binding.query); + try { + await this.createQuery(sheetName, binding.name, binding.query); + } catch (error) { + console.error('Failed to create query:', error); + throw error; + } }
74-74: Add TypeScript type safety for the callback parameter.The callback parameter lacks type definitions, which could lead to runtime errors. Consider adding proper TypeScript types to improve type safety and developer experience.
-bind(sheetName = '__global', binding, callback) { +type CellValue = { name: string; value: unknown }; +type CellCallback = (value: CellValue) => void; +bind(sheetName = '__global', binding: string | { name: string; value: unknown; query?: unknown }, callback: CellCallback) {Also applies to: 80-80, 84-84, 97-97
Line range hint
1-1: Consider removing@ts-strict-ignoreand fixing type issues.The presence of
@ts-strict-ignoresuggests bypassing TypeScript's strict checks. Consider addressing the underlying type issues to improve code reliability.This would involve:
- Enabling strict mode
- Adding proper type definitions
- Fixing any type-related issues that arise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts(4 hunks)packages/loot-core/src/client/SpreadsheetProvider.tsx(2 hunks)packages/loot-core/src/mocks/spreadsheet.ts(1 hunks)packages/loot-core/src/server/main.ts(2 hunks)
🔇 Additional comments (5)
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts (2)
Line range hint 54-77: Well-implemented cleanup handling
The addition of the isMounted flag and proper cleanup function effectively prevents memory leaks and race conditions from state updates after unmounting.
44-45: Verify handling of falsy values
The current condition bindingObj.value ? bindingObj.value : null will convert all falsy values (0, '', false) to null. Ensure this is the intended behavior for spreadsheet values.
✅ Verification successful
Let me check the actual usage context of the spreadsheet values to make a more informed decision.
Based on the comprehensive review of the spreadsheet implementation, I can now provide the final response:
The falsy value handling is correct and intentional
The current condition bindingObj.value ? bindingObj.value : null in useSheetValue.ts is appropriate because:
- The spreadsheet implementation (
Spreadsheetclass) explicitly handlesnullas the default/initial state for cell values - The value binding system is designed to work with
nullas a sentinel value for unset/empty cells - Test cases demonstrate that numeric values (including 0) are properly preserved when they are actual cell values
The ternary operation is used here specifically to handle the case where bindingObj.value is undefined (not passed), rather than to coerce falsy values. When a real spreadsheet value is set, it comes through the proper channels (cell updates, queries) and maintains its original value type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases or usage patterns of falsy spreadsheet values
rg -l "value: (?:0|false|''|\"\")" --type ts
Length of output: 753
Script:
#!/bin/bash
# Search for other spreadsheet value handling patterns
ast-grep --pattern 'bindingObj.value'
# Also check for any tests related to spreadsheet values
rg -l "spreadsheet.*value" --type test
Length of output: 233
Script:
#!/bin/bash
# Let's look for test files with .test.ts extension and search for spreadsheet value handling
rg -l "spreadsheet.*value" --type ts ".*\.test\.ts$"
# Also check the implementation context around the useSheetValue.ts file
cat "packages/desktop-client/src/components/spreadsheet/useSheetValue.ts"
Length of output: 2318
Script:
#!/bin/bash
# Let's check how spreadsheet values are used in the SpreadsheetProvider
cat "packages/loot-core/src/client/SpreadsheetProvider.ts"
# Also check for any test files specifically related to spreadsheets
rg -l "\.test\.(ts|tsx)$" --type ts | xargs rg -l "spreadsheet"
Length of output: 5174
Script:
#!/bin/bash
# Let's check the spreadsheet implementation and tests
cat "packages/loot-core/src/server/spreadsheet/spreadsheet.ts"
cat "packages/loot-core/src/server/spreadsheet/spreadsheet.test.ts"
# Also check the SpreadsheetProvider implementation
cat "packages/loot-core/src/client/SpreadsheetProvider.tsx"
Length of output: 21092
packages/loot-core/src/mocks/spreadsheet.ts (1)
Line range hint 26-41: LGTM! The bind method simplification looks good.
The removal of the unused fields parameter simplifies the API while maintaining the core functionality.
Let's verify that all callers have been updated to use the new signature:
✅ Verification successful
Based on the verification results, I can generate the final response:
The bind method's signature change is correctly implemented and used
The verification shows that:
- The only actual usage of the spreadsheet's
bindmethod is inuseSheetValue.ts, which correctly uses the new three-parameter signature (sheetName, bindingObj, callback). - Other
.bindoccurrences in the codebase are unrelated JavaScript's Function.prototype.bind calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for calls to bind method to ensure they match the new signature
# Expected: All calls should have 3 parameters (sheetName, binding, callback)
# Search for bind method calls
rg -A 2 '\.bind\(' --type ts
Length of output: 1257
Script:
#!/bin/bash
# Let's look at the actual implementation of useSheetValue to verify the bind usage
ast-grep --pattern 'spreadsheet.bind($$$)'
# Also search for any test files that might use the bind method
rg -A 3 '\.bind\(' --type test
Length of output: 1710
Script:
#!/bin/bash
# Let's look for test files with .test.ts extension that might use the bind method
rg -A 3 '\.bind\(' --type ts -g '*.test.ts'
# Also check the actual implementation of the spreadsheet interface to verify the bind signature
ast-grep --pattern 'interface $_Spreadsheet {
$$$
bind($$$)
$$$
}'
Length of output: 120
packages/loot-core/src/client/SpreadsheetProvider.tsx (1)
65-65: Verify the impact of removing the fields parameter.
The removal of the fields parameter from the bind method signature is a breaking change that could affect existing code.
packages/loot-core/src/server/main.ts (1)
516-516:
Potential breaking change in getCell handler's return value
The return value of handlers['getCell'] has been modified to { name: node.name, value: node.value }. This change may affect other parts of the codebase that rely on the previous return structure. Ensure that all client code consuming getCell is updated accordingly to handle the new return format.
Run the following script to identify all usages of handlers['getCell'] and verify they are compatible with the new return value:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3879.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/spreadsheet/useSheetValue.ts(4 hunks)packages/loot-core/src/shared/query.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/spreadsheet/useSheetValue.ts
🔇 Additional comments (3)
packages/loot-core/src/shared/query.ts (3)
79-83: Confirmation of calculation Flag Reset in select Method
Setting the calculation flag to false in the select method ensures that the query transitions back to a standard selection mode after a calculation. This change maintains the correct state of the query.
87-91: Accurate Configuration in calculate Method
The calculate method correctly sets selectExpressions to [{ result: expr }] and the calculation flag to true, properly configuring the query for calculation operations.
148-150: Addition of serializeAsString Method Enhances Usability
The new serializeAsString method provides a convenient way to obtain the serialized query state as a JSON string, which can be useful for caching or debugging purposes.
| get table(): string; | ||
| get tableOptions(): Readonly<Record<string, unknown>>; | ||
| get filterExpressions(): ReadonlyArray<ObjectExpression>; | ||
| get selectExpressions(): ReadonlyArray<ObjectExpression | string | '*'>; | ||
| get groupExpressions(): ReadonlyArray<ObjectExpression | string>; | ||
| get orderExpressions(): ReadonlyArray<ObjectExpression | string>; | ||
| get calculation(): boolean; | ||
| get rawMode(): boolean; | ||
| get withDead(): boolean; | ||
| get validateRefs(): boolean; | ||
| get limit(): number | null; | ||
| get offset(): number | null; |
There was a problem hiding this comment.
Potential Type Inconsistency with QueryState Getters
Converting QueryState properties to getters may cause a type mismatch when assigning an object literal to this.state in the Query constructor. The assigned object may not conform to the QueryState interface, which now expects getter methods. This could lead to type errors or unexpected behavior.
Consider updating the Query class to ensure this.state correctly implements the getters. One way to address this is to transform QueryState into a class with concrete implementations of the getters or adjust the constructor to create an object that adheres to the QueryState type, possibly by using Object.defineProperties to define getters on the state object.
Build on top of #3882. This update checks if the component was already unmounted and prevents a set state call + only call JSON.stringify when there is a query.