Fix send types in a number of places (2/2)
#4147
Conversation
actual-github-bot
bot
changed the title
send types in a number of places (2/2)send types in a number of places (2/2)
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
jfdoming
force-pushed
the
jfdoming/fix-send-types-2
branch
from
c0e93e7 to
906b6cc
Compare
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 |
jfdoming
force-pushed
the
jfdoming/fix-send-types-2
branch
2 times, most recently
from
2635ba5 to
b4e38d5
Compare
jfdoming
changed the title
send types in a number of places (2/2)send types in a number of places (2/2)
WalkthroughThis pull request introduces a series of type safety and error handling improvements across multiple files in the CodeRabbit project. The changes span various components and modules, focusing on enhancing method signatures, refining error handling, and improving type definitions. Key modifications include updating return types in interfaces like 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: 1
🧹 Nitpick comments (4)
packages/loot-core/src/server/dashboard/types/handlers.d.ts (1)
20-22: Consider simplifying the union type.While the current type is correct, it could be simplified to reduce redundancy. Consider this alternative:
type DashboardImportResult = | { status: 'ok' } | { error: 'json-parse-error' | 'internal-error' } | { error: 'validation-error'; message: string };This achieves the same type safety with less verbosity, as TypeScript will infer undefined fields automatically.
packages/loot-core/src/server/admin/types/handlers.ts (1)
15-17: Consider creating a shared type alias.Both
user-addanduser-updateshare the same return type. Consider extracting it to a type alias:type UserModificationResult = | { error?: string; id?: undefined } | { error?: undefined; id: string };This would improve maintainability and ensure consistency between these related operations.
Also applies to: 21-23
packages/desktop-client/src/components/accounts/Account.tsx (2)
483-487: LGTM! Consider adding a debug log for the null case.The null check for
this.pagedis a good defensive programming practice. However, it might be helpful to add a debug log whenthis.pagedis null to help diagnose issues during development.if (!this.paged) { + console.debug('fetchAllIds: paged query is not initialized'); return []; }
934-937: Consider returning null instead of 0 for better error handling.While the null check for
this.pagedis good, returning 0 might mask initialization issues. Consider returningnullto make it explicit that the filtered amount couldn't be calculated.if (!this.paged) { - return 0; + return null; }This change would require updating the type annotation and handling the null case in the UI components that use this value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4147.mdis excluded by!**/*.md
📒 Files selected for processing (13)
packages/desktop-client/src/components/accounts/Account.tsx(3 hunks)packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx(2 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)packages/loot-core/src/client/query-helpers.ts(2 hunks)packages/loot-core/src/server/admin/types/handlers.ts(1 hunks)packages/loot-core/src/server/aql/compiler.ts(1 hunks)packages/loot-core/src/server/dashboard/types/handlers.d.ts(1 hunks)packages/loot-core/src/server/rules/types/handlers.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(1 hunks)packages/loot-core/src/server/schedules/types/handlers.ts(1 hunks)packages/loot-core/src/types/models/simplefin.d.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4041
File: packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx:518-544
Timestamp: 2024-12-26T01:16:39.339Z
Learning: A <Trans> inside another <Trans> does not necessarily produce two distinct strings. Instead, it merges them into a single translation entry, allowing for object literals to be used within the same translation string.
🔇 Additional comments (17)
packages/loot-core/src/types/models/simplefin.d.ts (1)
5-5: LGTM! Type definitions enhanced with essential properties.The addition of
idtoSimpleFinOrganizationandbalancetoSimpleFinAccountimproves type completeness and better represents the actual data structure.Also applies to: 13-13
packages/loot-core/src/server/schedules/types/handlers.ts (1)
18-18: LGTM! Return type enhancement improves type safety.Changing the return type from
Promise<void>toPromise<string>provides better type safety and allows for proper error handling by returning the schedule ID.packages/loot-core/src/server/admin/types/handlers.ts (1)
9-11: LGTM! Improved error handling for bulk deletion.The return type now properly handles both partial success and error cases, making it clearer for consumers to handle different scenarios.
packages/loot-core/src/server/rules/types/handlers.ts (1)
48-48: LGTM! Type safety improvementsThe changes enhance type safety by replacing
unknownreturn types with specific entity types:
rule-getnow correctly returnsPromise<RuleEntity>rules-runnow correctly returnsPromise<TransactionEntity>Also applies to: 50-52
packages/loot-core/src/client/data-hooks/transactions.ts (1)
182-182: LGTM! Added null check for safer accessThe addition of the null check
t.schedule != nullbefore accessing the schedule property prevents potential runtime errors.packages/loot-core/src/client/actions/budgets.ts (1)
151-152: Verify type inference from send functionThe removal of explicit return types in favor of inference from the
sendfunction is cleaner. However, please verify that the inferred types match the expected types in the consuming code.Run this script to check usage:
Also applies to: 155-156
✅ Verification successful
Type inference from send function verified
The inferred types from
sendmatch the server-side implementations:
validateBudgetName:Promise<{ valid: boolean; message?: string }>uniqueBudgetName:Promise<string>All usages in the codebase are consistent with these types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how these functions are used to ensure type compatibility # Find all usages of these functions echo "Checking validateBudgetName usage:" rg "validateBudgetName\(" -A 2 echo "Checking uniqueBudgetName usage:" rg "uniqueBudgetName\(" -A 2Length of output: 3273
packages/loot-core/src/client/query-helpers.ts (1)
6-6: LGTM! Added type annotation for better type safetyAdding the
Querytype annotation to therunQueryparameter improves type checking and documentation.packages/loot-core/src/types/server-handlers.d.ts (5)
53-53: LGTM! Improved type safety for transactions export.The return type change from
Promise<unknown>toPromise<string>provides better type safety by explicitly indicating that the exported query result is a string.
110-110: LGTM! Enhanced type safety for payee rule counts.The return type change from
Promise<unknown>toPromise<Record<string, number>>properly types the mapping between payee IDs and their rule counts.
183-186: LGTM! Added proper error handling types for secret operations.The return type now includes optional error and reason fields, making error handling more explicit and type-safe.
200-204: LGTM! Improved error handling for SimpleFin accounts.The return type now properly handles both success and error cases with optional fields for accounts, error code, and reason.
142-142: LGTM! Enhanced type safety for query dependencies.The return type now explicitly specifies that dependencies is an array of strings, improving type safety.
packages/loot-core/src/server/schedules/app.ts (1)
342-343: LGTM! Added explicit return value for updateSchedule.The function now properly returns the schedule ID after updating, making it consistent with its Promise return type and allowing callers to track the updated schedule.
packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx (2)
237-241: LGTM! Enhanced type flexibility for SchedulePreview props.The props type now properly handles both successful cases (string[] for dates) and error cases (string for messages).
376-378: LGTM! Improved state typing for preview dates.The state type now correctly handles all possible values: string[] for dates, string for error messages, and null for the initial state.
packages/loot-core/src/server/aql/compiler.ts (1)
1008-1010: LGTM! Enhanced type safety with readonly array.The
orderExpressionsarray is now marked as readonly, preventing accidental modifications and improving type safety.packages/desktop-client/src/components/accounts/Account.tsx (1)
716-716: LGTM! Robust error handling.The null check for
this.pagedcomplements the existingcanCalculateBalance()check, providing comprehensive error handling.
| @@ -207,7 +207,7 @@ export class LiveQuery<TResponse = unknown> { | |||
| protected fetchData = async ( | |||
| runQuery: () => Promise<{ | |||
| data: Data<TResponse>; | |||
| dependencies: Set<string>; | |||
| dependencies: string[]; | |||
There was a problem hiding this comment.
Nice! This is definitely an array of table names
jfdoming
force-pushed
the
jfdoming/fix-send-types-2
branch
from
135bd52 to
5cdbd82
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/desktop-client/src/components/modals/EditUser.tsx (1)
87-93: Consider adding explicit type annotations for better type safety.While the type narrowing with
'error' in resis good, consider adding explicit type annotations to make the expected response type clear:- const res = await send(method, user); + type SendResponse = { error: string } | { id?: string }; + const res: SendResponse = await send(method, user);This would:
- Document the expected response shape
- Catch type mismatches at compile time
- Make the code more maintainable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4147.mdis excluded by!**/*.md
📒 Files selected for processing (15)
packages/desktop-client/src/components/accounts/Account.tsx(3 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx(2 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)packages/loot-core/src/client/query-helpers.ts(2 hunks)packages/loot-core/src/platform/client/fetch/index.d.ts(1 hunks)packages/loot-core/src/server/admin/types/handlers.ts(1 hunks)packages/loot-core/src/server/aql/compiler.ts(2 hunks)packages/loot-core/src/server/dashboard/types/handlers.d.ts(1 hunks)packages/loot-core/src/server/rules/types/handlers.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(1 hunks)packages/loot-core/src/server/schedules/types/handlers.ts(1 hunks)packages/loot-core/src/types/models/simplefin.d.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/loot-core/src/client/data-hooks/transactions.ts
- packages/loot-core/src/types/models/simplefin.d.ts
- packages/loot-core/src/server/schedules/types/handlers.ts
- packages/loot-core/src/server/schedules/app.ts
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/loot-core/src/server/admin/types/handlers.ts
- packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx
- packages/loot-core/src/client/actions/budgets.ts
- packages/loot-core/src/client/query-helpers.ts
- packages/loot-core/src/server/dashboard/types/handlers.d.ts
- packages/loot-core/src/types/server-handlers.d.ts
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/server/aql/compiler.ts
[warning] 1-1:
'loot-core/shared/query' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing from loot-core/*
🪛 GitHub Actions: Test
packages/loot-core/src/server/aql/compiler.ts
[warning] 1-1: 'loot-core/shared/query' import is restricted from being used by a pattern. Please use relative imports in loot-core instead of importing from loot-core/*
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Visual regression
🔇 Additional comments (4)
packages/loot-core/src/server/rules/types/handlers.ts (2)
48-48: LGTM! Type safety improvement forrule-get.The change from
stringtoRuleEntity['id']for theidparameter and the explicitPromise<RuleEntity>return type enhances type safety.
50-52: LGTM! Enhanced type safety forrules-run.The changes properly type both:
- Input: Explicit
TransactionEntitytype for the transaction parameter- Output: Clear
Promise<TransactionEntity>return typeThis eliminates ambiguity and prevents potential type-related errors.
packages/desktop-client/src/components/modals/EditUser.tsx (1)
86-87: LGTM! Improved type safety by removing fallback object.The removal of the
|| {}fallback pattern improves type safety by ensuring thatsendmust return a properly typed response.packages/loot-core/src/server/aql/compiler.ts (1)
1010-1010: LGTM! Type signature improvement.The updated type signature for
customizeQueryis more precise and maintainable. UsingQueryStatedirectly instead of a generic type with specific structure improves type safety and readability.
jfdoming
force-pushed
the
jfdoming/fix-send-types-2
branch
from
5cdbd82 to
6b43e75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/Overview.tsx (1)
Line range hint
250-290: Improved type safety in error handling.The change from a simple boolean check to using the
inoperator is a TypeScript improvement that provides better type narrowing and safer error handling. This ensures that the error property exists on the response object before attempting to access it.However, consider adding type annotations for the
resparameter to make the expected response structure explicit:- const res = await send('dashboard-import', { filepath }); + const res = await send<{ error?: 'json-parse-error' | 'validation-error' | string; message?: string }>('dashboard-import', { filepath });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4147.mdis excluded by!**/*.md
📒 Files selected for processing (16)
packages/desktop-client/src/components/accounts/Account.tsx(3 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(1 hunks)packages/desktop-client/src/components/reports/Overview.tsx(1 hunks)packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx(2 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/client/actions/prefs.ts(1 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)packages/loot-core/src/client/query-helpers.ts(2 hunks)packages/loot-core/src/platform/client/fetch/index.d.ts(1 hunks)packages/loot-core/src/server/admin/types/handlers.ts(1 hunks)packages/loot-core/src/server/aql/compiler.ts(2 hunks)packages/loot-core/src/server/rules/types/handlers.ts(1 hunks)packages/loot-core/src/server/schedules/app.ts(1 hunks)packages/loot-core/src/server/schedules/types/handlers.ts(2 hunks)packages/loot-core/src/types/models/simplefin.d.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/loot-core/src/client/data-hooks/transactions.ts
- packages/loot-core/src/server/schedules/app.ts
- packages/loot-core/src/types/models/simplefin.d.ts
- packages/desktop-client/src/components/modals/EditUser.tsx
- packages/loot-core/src/platform/client/fetch/index.d.ts
- packages/loot-core/src/client/query-helpers.ts
- packages/loot-core/src/client/actions/budgets.ts
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/loot-core/src/server/rules/types/handlers.ts
- packages/desktop-client/src/components/select/RecurringSchedulePicker.tsx
- packages/loot-core/src/server/admin/types/handlers.ts
- packages/loot-core/src/types/server-handlers.d.ts
- packages/loot-core/src/server/aql/compiler.ts
- packages/loot-core/src/server/schedules/types/handlers.ts
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/client/actions/prefs.ts (1)
Learnt from: jfdoming
PR: actualbudget/actual#4112
File: packages/loot-core/src/client/actions/prefs.ts:45-47
Timestamp: 2025-01-11T17:39:00.532Z
Learning: The `setI18NextLanguage` function in `packages/desktop-client/src/i18n.ts` has built-in error handling that covers:
1. Special case for English when not available
2. System default language fallback
3. Handling of hyphenated language codes
4. Error logging for unknown locales
⏰ 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 (1)
packages/loot-core/src/client/actions/prefs.ts (1)
46-46: LGTM! The nullish coalescing operator addition improves type safety.The change to handle undefined language preferences with an empty string fallback aligns well with
setI18NextLanguage's built-in error handling, which includes fallback to English and system default language.
| @@ -41,7 +41,8 @@ export type Unlisten = typeof unlisten; | |||
| /** Mock functions */ | |||
| export function initServer(handlers: { | |||
| query: (query: { table: string; selectExpressions: unknown }) => Promise<{ | |||
| data: unknown; | |||
There was a problem hiding this comment.
Just curious, what issue was the unknown causing?
There was a problem hiding this comment.
This particular instance didn't cause any error. The other one in server-handlers.d.ts causes a bunch of errors like Type 'unknown' is not assignable to type 'number' because the result type of a query varies wildly based on the query being run. We could totally have better types here in the future!
Happy to revert the mock if you like, but my thinking is it would be good to keep the mock and implementation types the same
* Fix `send` types in a number of places * Add release notes * PR feedback
In #4145 I'm attempting to remove
anytypes from thesendfunction. In doing so I uncovered a bunch of places where types were set incorrectly. This PR cleans up those types (split into 2 for readability).