Tighten Dispatch type - HELP WANTED #17204
Conversation
komret
requested review from
szymonlesisz,
marekrjpolak,
mroz22 and
peter-sanderson
|
🚀 Expo preview is ready!
|
komret
force-pushed
the
chore/tighten-dispatch-type
branch
3 times, most recently
from
c3806ed to
57a9a9a
Compare
komret
added
the
no-project
|
Warning Rate limit exceeded@komret has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThe pull request refines type declarations and action payload definitions across various modules. Functions now have explicit return types—for example, the 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 (1)
suite-common/metadata-types/src/metadataTypes.ts (1)
197-197: Clean up potential type redundancy.Extending
Labelsto includePasswordManagerStatemakes sense, but there's a redundancy in theDatatype definition (line 201-204) which now hasLabels | PasswordManagerStatewhereLabelsalready includesPasswordManagerState.Consider refactoring the
Datatype definition to:-type Data = Record< - LabelableEntityKeys['fileName'], // unique "id" for mapping with labelable entitties - Labels | PasswordManagerState ->; +type Data = Record< + LabelableEntityKeys['fileName'], // unique "id" for mapping with labelable entitties + Labels +>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite/src/actions/suite/guideActions.ts(1 hunks)packages/suite/src/actions/suite/metadataActions.ts(4 hunks)packages/suite/src/actions/suite/metadataPasswordsActions.ts(3 hunks)packages/suite/src/actions/suite/metadataProviderActions.ts(1 hunks)packages/suite/src/actions/suite/modalActions.ts(1 hunks)packages/suite/src/actions/suite/routerActions.ts(1 hunks)packages/suite/src/actions/suite/suiteActions.ts(2 hunks)packages/suite/src/middlewares/suite/routerMiddleware.ts(1 hunks)packages/suite/src/reducers/suite/metadataReducer.ts(1 hunks)packages/suite/src/types/suite/index.ts(2 hunks)suite-common/metadata-types/src/metadataTypes.ts(1 hunks)suite-common/redux-utils/src/extraDependenciesType.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/suite/src/middlewares/suite/routerMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: check-project-or-issue
- GitHub Check: Releases revision Checks
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Unit Tests
- GitHub Check: Linting and formatting
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (18)
packages/suite/src/actions/suite/guideActions.ts (1)
43-43: Good type safety enhancement.Adding explicit return type
GuideActionto theunsetNodefunction improves type safety by ensuring it always returns the expected action type.packages/suite/src/actions/suite/routerActions.ts (1)
36-36: Type refinement matches actual usage.The change from
payload: AnchorTypetopayload: AnchorType | undefinedaligns with the functiononAnchorChange(line 80), which already has an optional anchor parameter. This makes the action type definition consistent with its actual usage.packages/suite/src/reducers/suite/metadataReducer.ts (1)
80-83: Good handling of undefined clientId.The added condition properly handles the case when
clientIdis undefined or falsy by removing the provider entry from theselectedProviderobject. This prevents potential errors from trying to store an undefined value as a client ID.packages/suite/src/actions/suite/metadataProviderActions.ts (1)
130-146: Improved type safety in disconnectProviderThe payload for the METADATA.REMOVE_PROVIDER action has been simplified to only include the
clientIdproperty, which is more focused and type-safe. This aligns with the PR objective of enhancing type security.packages/suite/src/actions/suite/modalActions.ts (1)
82-83: Type issue needs to be resolved properlyThis comment suppresses a TypeScript error that the author couldn't resolve. While this allows the code to compile, it introduces a technical debt that should be addressed.
Could you provide more details about the exact error message TypeScript is giving here? Understanding the error would help us determine how to properly fix the typing issue instead of suppressing it.
packages/suite/src/actions/suite/metadataPasswordsActions.ts (3)
31-36: Improved variable naming and error handlingRenaming from
responsetoproviderDetailsmakes the code more descriptive and easier to understand. The error handling now correctly passes the complete provider details object to the error handler.
58-59: Consistent use of providerDetails variableUsing
providerDetails.payloadconsistently throughout the function improves code clarity and maintainability.
79-79: More specific condition check for device stateThe condition has been refined to check not just for the existence of
statebut specifically forstaticSessionIdwhich is required for the functionality to work correctly. This prevents potential runtime errors ifstateexists butstaticSessionIdis undefined.packages/suite/src/types/suite/index.ts (3)
7-7: Added new import foraddLogfunction.This addition complements the change on line 88 where you've added the new action type. Good practice to keep imports organized together.
87-88: Extended the Action union type with logger action.Adding
ReturnType<typeof addLog>to the Action union type ensures that logger actions can be properly dispatched through your Redux store. This improves type safety by making the Action type more comprehensive.
96-96: Improved type inference in Dispatch interface.This is a key improvement to the Dispatch interface. By changing from
<Action>(action: Action)to<A extends Action>(action: A), you're ensuring that the return type is correctly inferred based on the specific action being dispatched, rather than using a generic Action type. This provides better type checking and autocompletion.packages/suite/src/actions/suite/suiteActions.ts (2)
98-100: Tightened payload type for appChanged action.You've removed the possibility of
unknowntype in the payload, restricting it to only accept validAppState['router']['app']values. This is a good practice that prevents potential type errors at runtime.
305-310: Enhanced type safety for firmware authenticity checks.You've improved the implementation by using a constant typed array (
as const) for firmware authenticity checks. This ensures that the array is treated as readonly and maintains its specific tuple type, providing better type inference in the forEach loop.packages/suite/src/actions/suite/metadataActions.ts (5)
40-40: Restricted REMOVE_PROVIDER payload to only include clientId.Using
Pick<MetadataProvider, 'clientId'>reduces the payload to only what's needed - just the clientId. This is a good practice that ensures you're only passing the minimum required data in your actions.
49-50: Improved flexibility of provider and data types.You've made two important changes:
- Changed provider type to
Omit<MetadataProvider, 'data'> & Pick<Partial<MetadataProvider>, 'data'>to allow for optional data- Made the data field explicitly accept undefined values
This gives more flexibility when working with metadata providers that might not have complete data yet.
57-57: Added support for undefined clientId in SET_SELECTED_PROVIDER action.Making clientId accept
undefinedvalues allows for clearing the selected provider, which is a necessary operation in certain scenarios.
103-103: Added optional chaining for safer property access.Using optional chaining (
?.) is a defensive programming practice that prevents runtime errors when accessing potentially undefined properties.
137-137: Removed support for undefined data in setMetadata.The commented-out
| undefinedsuggests you're no longer supporting undefined data in the setMetadata function. Make sure all callers of this function are updated to never pass undefined.Is this comment intentional or should it be removed completely?
| @@ -105,7 +105,7 @@ export type ExtraDependencies = { | |||
| localCurrency: FiatCurrencyCode; | |||
| }>; | |||
| lockDevice: ActionCreatorWithPreparedPayload<[payload: boolean], boolean>; | |||
| appChanged: ActionCreatorWithPayload<unknown>; | |||
| appChanged: ActionCreatorWithPayload<any>; | |||
There was a problem hiding this comment.
Type safety regression from unknown to any
Changing the type from ActionCreatorWithPayload<unknown> to ActionCreatorWithPayload<any> actually reduces type safety. The unknown type requires explicit type checking before use, while any allows unchecked usage, potentially leading to runtime errors.
Consider keeping unknown or using a more specific type if available:
-appChanged: ActionCreatorWithPayload<any>;
+appChanged: ActionCreatorWithPayload<unknown>;If the specific type is known (like AppState['router']['app']), consider using that instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| appChanged: ActionCreatorWithPayload<any>; | |
| appChanged: ActionCreatorWithPayload<unknown>; |
komret
removed
the
no-project
komret
force-pushed
the
chore/tighten-dispatch-type
branch
from
1c6816b to
0b7bf7d
Compare
Trying to tighten
Dispatchtype so that it provides some type security to our actions, preventing bugs like this one.1st commit changes the
Dispatchtype and fixes code wherever I more or less knew what to do.2nd commit skips type check for lines that I couldn't fix - HELP WANTED.
3rd commit fixes types related to metadata, @mroz22, please check.
QA: Some cleanup in code related to labelling and password manager. It would be good to test that nothing was broken in the process.