♻️ (typescript) ported TransactionsTable.test to TS #4108
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 No assets were 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 |
MatissJanis
changed the title
WalkthroughThe pull request introduces several modifications across multiple packages in the project, focusing primarily on type safety improvements, testing enhancements, and minor structural changes. Key changes include adding a global function Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4108.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/desktop-client/globals.d.ts(1 hunks)packages/desktop-client/package.json(0 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(21 hunks)packages/desktop-client/src/hooks/useSplitsExpanded.tsx(1 hunks)packages/loot-core/src/client/query-helpers.test.ts(1 hunks)packages/loot-core/src/mocks/index.ts(5 hunks)packages/loot-core/src/platform/client/fetch/index.d.ts(2 hunks)packages/loot-core/src/server/accounts/rules.test.ts(0 hunks)packages/loot-core/src/server/spreadsheet/spreadsheet.test.ts(2 hunks)packages/loot-core/src/shared/transactions.ts(7 hunks)packages/loot-core/src/types/models/transaction.d.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/package.json
- packages/loot-core/src/server/accounts/rules.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
[error] 240-240: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🔇 Additional comments (22)
packages/loot-core/src/shared/transactions.ts (8)
24-25: Good use ofas constfor type safetyUsing
as constfortypeandversionensures they are treated as literal types, enhancing type safety.
30-30: UpdateGenericTransactionEntitytype aligns with interface removalUpdating
GenericTransactionEntitytoTransactionEntityreflects the removal ofTransactionEntityWithError.
157-160: Function signature updated to reflect type changesUpdating the function parameter and return types to
TransactionEntityis consistent with the type changes.
283-284: Repeated use of thesatisfiesoperator—ensure TypeScript version compatibilityAs mentioned earlier, verify that the project's TypeScript version supports the
satisfiesoperator.
314-315: Repeated use of thesatisfiesoperator—ensure TypeScript version compatibilityThis is another occurrence of the
satisfiesoperator. Please ensure TypeScript 4.9 or higher is used.Also applies to: 320-320
340-340: Repeated use of thesatisfiesoperator—ensure TypeScript version compatibilityAgain, confirm that the TypeScript version in the project supports the
satisfiesoperator.
129-132: Ensure TypeScript version supports thesatisfiesoperatorThe use of the
satisfiesoperator requires TypeScript 4.9 or higher. Please verify that the project is using a compatible TypeScript version to avoid compilation issues.Run the following script to check the TypeScript version specified in
package.jsonfiles:✅ Verification successful
TypeScript version is compatible with the
satisfiesoperatorThe project uses TypeScript 5.5.4 across all packages, which fully supports the
satisfiesoperator (introduced in TypeScript 4.9).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the TypeScript version used in the project. # Expected: TypeScript version 4.9.0 or higher. rg '"typescript"\s*:\s*"[^\"]+"' package.json packages/*/package.jsonLength of output: 367
Script:
#!/bin/bash # Search for TypeScript version in package.json files rg -U '"typescript":\s*"[^"]+"|"typescript":\s*"[^"]+"' package.json packages/*/package.json # Find and show content of tsconfig.json files fd tsconfig.json --exec cat {}Length of output: 2053
84-87: Ensure consistent handling of theerrorproperty whenundefinedThe
errorproperty is now assignedundefinedwhen there is no error. Please verify that all consumers ofTransactionEntity.errorhandleundefinedappropriately and that the change fromnulltoundefineddoesn't introduce any issues.Run the following script to check for comparisons against
null:✅ Verification successful
The change from
nulltoundefinedis safe to proceedAll existing consumers of
TransactionEntity.erroralready handle the property safely:
- Uses optional chaining (
?.) for property access- Tests use
toBeFalsy()/toBeTruthy()which work with both values- TypeScript interface properly declares
erroras optional🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for places where 'transaction.error' is compared to 'null'. # Expected: No occurrences of 'transaction.error === null' or 'transaction.error !== null' rg "transaction\.error\s*===\s*null|transaction\.error\s*!==\s*null"Length of output: 70
Script:
#!/bin/bash # Search for all usages of transaction.error and type definitions echo "=== Searching for transaction.error usages ===" rg "transaction\.error" -A 2 echo -e "\n=== Searching for TransactionEntity type definition ===" ast-grep --pattern 'type TransactionEntity = { $$$ }' echo -e "\n=== Searching for interface TransactionEntity ===" ast-grep --pattern 'interface TransactionEntity { $$$ }'Length of output: 4457
packages/desktop-client/globals.d.ts (1)
12-14: Verify the necessity of adding__resetWorld()to the global scopePlease verify that adding
__resetWorld()to the global scope is necessary and does not introduce conflicts. If this function is intended for testing purposes, ensure that it is scoped appropriately.Run the following script to find usages of
__resetWorld()in the codebase:✅ Verification successful
Global scope for
__resetWorld()is appropriate for its test utility purposeThe function is correctly implemented as a global test utility:
- Serves as a Redux store reset mechanism between tests
- Automatically called in the global
afterEachhook- Only used in test files (setupTests.js and *.test.tsx)
- Follows testing best practices for cleanup utilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of '__resetWorld()' in the codebase. # Expected: Usages are limited to test files or appropriate contexts. rg '__resetWorld\(\)'Length of output: 264
Script:
#!/bin/bash # Check the implementation and context in setupTests.js cat packages/desktop-client/src/setupTests.jsLength of output: 586
packages/loot-core/src/types/models/transaction.d.ts (1)
29-33: Addition oferrorproperty enhances transaction error handlingThe optional
errorproperty inTransactionEntityallows for improved handling of split transaction errors.packages/desktop-client/src/hooks/useSplitsExpanded.tsx (1)
87-87: LGTM! Good improvement to the API.Making
initialModeoptional with a sensible default value improves the component's usability while maintaining backward compatibility.packages/loot-core/src/mocks/index.ts (3)
16-51: Well-structured type improvements for account generation.The addition of explicit types and the separation of offline vs. connected account logic improves code clarity and type safety.
68-72: Good type safety improvements for category-related functions.The addition of explicit parameter types and return types enhances the reliability of these mock generators.
Also applies to: 83-86, 95-97
110-112: Type-safe transaction generation with required fields.Using
Pickto enforce required fields is a good practice. Theaccountfield requirement prevents potential runtime errors.Also applies to: 126-130
packages/loot-core/src/server/spreadsheet/spreadsheet.test.ts (1)
13-13: Good test simplification.Removing the unused
payeeIdparameter simplifies the test setup without losing any test coverage.Also applies to: 128-129
packages/loot-core/src/client/query-helpers.test.ts (1)
125-127: Good modernization of async patterns.Converting the server initialization to use an async function improves code consistency and makes the asynchronous nature of the operation more explicit.
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (4)
24-30: LGTM! Clean type imports.The imported types from
loot-core/types/modelsare well-organized and properly scoped.
51-65: LGTM! Well-typed payees array.The
payeesarray is properly typed asPayeeEntity[]with all required properties.
118-130: LGTM! Well-defined props interface.The
LiveTransactionTablePropstype is comprehensive and properly documents all required and optional props.
182-184: Address the ts-expect-error comment.The comment indicates that
TransactionTableneeds to be migrated to TypeScript.Let's check the status of the component:
✅ Verification successful
The
@ts-expect-errorcomment is valid and should be keptThe comment accurately reflects the current state of the codebase, where
TransactionsTableis still a.jsxfile that needs to be migrated to TypeScript. The error suppression is temporary and will be resolved once the migration is complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if TransactionTable is already being worked on fd -e tsx -e ts "TransactionTable.tsx"Length of output: 38
Script:
#!/bin/bash # Find TransactionTable component with any extension fd "TransactionTable" --type f # Search for TypeScript migration related comments rg -i "typescript|ts-expect|todo.*typescript" --type js --type jsx --type ts --type tsxLength of output: 148
Script:
#!/bin/bash # List files in the transactions directory ls -la packages/desktop-client/src/components/transactions/ # Search for TransactionTable definition and related comments rg "TransactionTable" packages/desktop-client/src/components/transactions/ -A 2Length of output: 6101
packages/loot-core/src/platform/client/fetch/index.d.ts (2)
2-2: LGTM! Clean type import.The CategoryGroupEntity type import is properly scoped.
42-49: LGTM! Well-typed server initialization.The
initServerfunction signature now provides proper type safety with well-defined handlers interface.
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (1)
293-293: Consider improving type safety by handling null cases.Several functions use non-null assertions (
!) which bypass TypeScript's null checks. Consider handling these cases explicitly:
- Line 293:
field.querySelector(subSelector)!- Line 308:
field.querySelector(subSelector)!- Lines 327-331: Multiple non-null assertions
Example safer implementation:
-return field.querySelector(subSelector)!; +const element = field.querySelector(subSelector); +if (!element) { + throw new Error(`Element with selector "${subSelector}" not found`); +} +return element;Also applies to: 308-308, 327-331
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/desktop-client/package.json(0 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx(21 hunks)packages/desktop-client/src/hooks/useSplitsExpanded.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/hooks/useSplitsExpanded.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
[error] 240-240: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: typecheck
- GitHub Check: build (macos-latest)
- GitHub Check: test
- GitHub Check: api
- GitHub Check: build (windows-latest)
- GitHub Check: web
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: crdt
- GitHub Check: compare
🔇 Additional comments (5)
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx (5)
23-29: Well-structured type imports!Good use of the
typekeyword for importing type definitions, which helps with tree-shaking and keeps the runtime bundle size optimal.
239-241: Fix performance issue in categories reduction.The spread operator in the reducer's accumulator causes O(n²) time complexity.
🧰 Tools
🪛 Biome (1.9.4)
[error] 240-240: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
118-130: Well-defined props interface!The
LiveTransactionTablePropsinterface is comprehensive and properly typed, making the component's API clear and type-safe.
Line range hint
405-425: Excellent test coverage with proper type handling!The test cases thoroughly verify transaction table functionality while properly handling optional chaining for nullable fields (e.g.,
acct?.name,p?.name,category?.name).
183-183: Remove @ts-expect-error comment.The comment indicates that this will be auto-patched once TransactionTable is moved to TS. Let's track this technical debt.
Let's check if the TransactionTable component has been converted to TypeScript:
packages/desktop-client/src/components/transactions/TransactionsTable.test.tsx
Show resolved
Hide resolved
No description provided.