Minor prompt improvements #17426
Minor prompt improvements #17426
Conversation
marekrjpolak
force-pushed
the
chore/minor-prompt-improvements
branch
from
fac228f to
0280af3
Compare
mroz22
added
the
no-project
mroz22
requested review from
karliatto,
mroz22,
szymonlesisz and
martykan
as code owners
WalkthroughThe changes refactor how event data is structured and passed in various modules. In several API methods (e.g., firmware upload and device wipe), the parameters for the ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (3)
packages/node-utils/src/http.ts (1)
78-83: The event type refactoring improves parameter structure clarity.This change refactors the
BaseEventstype from using function signatures to direct type annotations, which makes the event data structure more explicit. This is a positive simplification that aligns with similar changes across the codebase.Consider replacing
voidwithundefinedfor the event types that don't pass data. Whilevoidworks,undefinedis more semantically accurate when representing events that don't carry data:type BaseEvents = { 'server/listening': NonNullable<net.AddressInfo>; - 'server/closing': void; - 'server/closed': void; + 'server/closing': undefined; + 'server/closed': undefined; 'server/error': string; };This addresses the static analysis warnings and follows TypeScript best practices for non-function type declarations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 81-81: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/suite-desktop-core/src/libs/main-window-proxy.ts (1)
5-7: Improved type signature for better event handlingThe event interface has been simplified from function signatures to direct type associations. This makes the interface cleaner while maintaining the same functionality.
While the implementation works as is, consider extending the TypedEmitter pattern by leveraging a more descriptive approach for event payloads:
interface MainWindowProxyEvents { - init: StrictBrowserWindow; - destroy: StrictBrowserWindow; + init: { window: StrictBrowserWindow }; + destroy: { window: StrictBrowserWindow }; }This would provide better semantic meaning and make it consistent with the object-based event pattern used elsewhere in the codebase.
packages/connect/src/device/Device.ts (1)
102-102: Consider using undefined instead of void for the event payload typeThe static analyzer flagged this line because void is potentially confusing when used outside a return type or type parameter context.
- [DEVICE.PASSPHRASE_ON_DEVICE]: void; + [DEVICE.PASSPHRASE_ON_DEVICE]: undefined;Using
undefinedhere would be more semantically accurate for an event that doesn't require payload data.🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/connect/src/api/firmware/uploadFirmware.ts(1 hunks)packages/connect/src/api/wipeDevice.ts(1 hunks)packages/connect/src/core/index.ts(5 hunks)packages/connect/src/core/onCallFirmwareUpdate.ts(1 hunks)packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/device/DeviceCommands.ts(1 hunks)packages/connect/src/device/prompts.ts(3 hunks)packages/node-utils/src/http.ts(1 hunks)packages/suite-desktop-core/src/libs/main-window-proxy.ts(1 hunks)packages/transport/src/transports/abstract.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/node-utils/src/http.ts
[error] 80-80: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 81-81: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/connect/src/device/Device.ts
[error] 102-102: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (31)
packages/node-utils/src/http.ts (5)
170-171: Event emission aligns with updated type definition.The emission of the
server/listeningevent properly passes the address object directly, which matches the updated type definition:'server/listening': NonNullable<net.AddressInfo>.
183-184: Event emission aligns with updated type definition.The emission of the
server/closingevent without arguments properly matches the updated type definition:'server/closing': void.
190-191: Event emission aligns with updated type definition.The emission of the
server/closedevent without arguments properly matches the updated type definition:'server/closed': void.
282-284: Event emission aligns with updated type definition.The emission of the
server/errorevent with a string argument properly matches the updated type definition:'server/error': string.
78-83:❓ Verification inconclusive
Verify consistency with other event emitter usages.
This refactoring is part of a broader pattern to standardize event parameter handling across the codebase. Ensure this style is consistently applied in all other TypedEmitter implementations.
🏁 Script executed:
#!/bin/bash # Description: Check for consistency in event type definitions # Look for TypedEmitter usages across the codebase echo "Searching for TypedEmitter usage patterns..." rg --type typescript "extends TypedEmitter<" -A 10 # Look for event emission patterns to ensure they match the new type definitions echo "Searching for event emission patterns..." rg --type typescript "emit\('server/" -A 1 -B 1Length of output: 365
Below is an updated verification script that uses globbing to target TypeScript files (i.e. *.ts) instead of relying on a built‑in file type. Please run this new script to re‑verify consistency across all implementations of TypedEmitter and event emission patterns:
#!/bin/bash # Description: Re‑verify consistency for event emitter usage # Using globbing to search all TypeScript files (*.ts) echo "Searching for TypedEmitter usage patterns in .ts files..." rg "extends TypedEmitter<" --glob "*.ts" -A 10 echo "Searching for event emission patterns for 'server/' events in .ts files..." rg "emit\('server/" --glob "*.ts" -A 1 -B 1Action: Please run the above script and manually verify that the usage and emission of server events align with the new event type definitions. Once confirmed, ensure that the standardized event handling strategy is applied consistently across the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 80-80: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 81-81: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/transport/src/transports/abstract.ts (1)
59-72: Improved type visibility by exporting ReadWriteErrorThis change enhances code organization by explicitly exporting the ReadWriteError type, making it available for use in other modules. This is a good practice for increasing type reusability across the codebase.
packages/connect/src/core/onCallFirmwareUpdate.ts (1)
24-28: Enhanced event parameter destructuringThe change improves code readability by using object destructuring to directly access the
payload.codeproperty from the event parameters, which aligns with the new event emission structure implemented across the codebase.packages/connect/src/api/firmware/uploadFirmware.ts (1)
13-13: Standardized event emission structureThe updated event emission now uses a consistent object structure with
deviceandpayloadproperties, which provides better organization and improves consistency with the event handling patterns used elsewhere in the codebase.packages/connect/src/api/wipeDevice.ts (1)
35-38: Restructured event emission to use a standardized formatThe change properly refactors the event emission to use a consistent object structure with
deviceandpayloadproperties. This provides better clarity and aligns with the event handling patterns used throughout the codebase.packages/connect/src/device/DeviceCommands.ts (4)
430-430: LGTM: Standardized event payload structureThe DEVICE.BUTTON event emission has been refactored to follow a consistent object-based pattern with clear property names, improving readability and type safety.
437-447: Improved error handling pattern for PIN responsesThe PIN handling logic has been refactored to a more readable early-return pattern that checks for success first before proceeding with the acknowledgment.
This pattern is more maintainable as it separates the error case from the success case cleanly, reducing nesting and making the code flow more logical.
451-460: LGTM: Consistent error handling pattern applied to passphrase responsesThe passphrase handling follows the same improved error-handling pattern as the PIN responses, maintaining consistency across the codebase.
464-468: LGTM: Word request handling also updated for consistencyThe word request handling has been updated to follow the same error-first pattern used in the other prompt handlers.
packages/connect/src/core/index.ts (5)
785-788: Improved event handler signature using destructuringThe handler now uses object destructuring to clearly extract properties from the DeviceEvents interface, making the code more explicit and type-safe.
824-826: LGTM: Consistent pattern for PIN event handlingThe PIN handler now uses the same object destructuring pattern as the button handler, improving consistency across event handlers.
845-847: LGTM: Word handler follows the same improved patternConsistent use of object destructuring in the word handler aligns with the other event handlers.
872-874: LGTM: Passphrase handler adopts the same patternThe passphrase handler now uses object destructuring, maintaining consistency with other event handlers.
900-902: LGTM: EmptyPassphrase handler updated for consistencyThis handler also adopts the new pattern, ensuring all event handlers follow the same approach.
packages/connect/src/device/Device.ts (3)
87-105: Restructured event interface for improved type safetyThe DeviceEvents interface has been updated to use object types instead of function signatures, providing clearer structure and better type checking for event handlers.
This change matches the implementation changes in the event emitters and handlers across the codebase, creating a more consistent approach to event handling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-102: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
170-170: Change in error type from Error to stringThe cancelableAction property now accepts a string error instead of an Error object, simplifying error handling.
This change aligns with the updates in other parts of the codebase where string errors are being used consistently for simpler serialization and handling.
485-485: Updated error handling in interruptionFromUser methodThe method now passes error.toString() instead of the Error object directly, matching the updated cancelableAction type.
This change ensures consistent error handling throughout the device interaction flow.
packages/connect/src/device/prompts.ts (10)
2-2: Good addition of specific import type.Adding an explicit import for the
ReadWriteErrortype improves the code's clarity and type safety. This type is used effectively in the newPromptReturnTypeunion type.
8-13: Well-structured type definitions enhance code clarity.The introduction of these type definitions significantly improves type safety and makes the code intentions clearer:
DeviceEventArgsproperly strips the callback from event argumentsDeviceEventCallbackcorrectly extracts callback typesPromptReturnTypeprovides a comprehensive union type for all possible return scenariosThis approach provides better static type checking and makes the API contract more explicit.
15-15: Good type update for consistent error handling.Changing
PromptCallbackto use string errors instead of Error objects aligns with the broader refactoring effort to standardize error handling across the codebase.
23-23: Good use ofas constfor literal type inference.Adding
as constto thesuccess: falseproperty ensures TypeScript correctly infers this as the literalfalserather than a boolean, which improves type safety for discriminated unions.
38-38: Improved function signature with object destructuring.Changing the function signature to use object destructuring instead of spread arguments makes the code more maintainable and the parameter structure more explicit. This is a great improvement for code readability and maintainability.
40-54: Enhanced error handling structure with detailed return types.The updated Promise resolution provides more structured and informative error responses, including distinguishing between transport errors and other types of errors. This makes error handling more robust for consumers of this API.
74-74: String error message is consistent with updated error handling approach.Using a string error message directly instead of creating an Error object aligns with the updated
PromptCallbacktype and the broader refactoring effort for standardized error handling.
78-78: Consistent update of helper functions.The helper functions have been properly updated to use the new object parameter structure when calling the
promptfunction. This ensures consistency throughout the codebase.Also applies to: 80-81, 83-84
48-48:❓ Verification inconclusive
Potential logic inconsistency in
isTransportErrordetermination.There seems to be a potential inconsistency in how
isTransportErroris determined. In line 48, it's set to!response.success, but in line 53, it's hardcoded totrue. Make sure this logic correctly represents your intent for categorizing errors.
🏁 Script executed:
#!/bin/bash # Investigate the cancelPrompt function to understand its return value # Look for the cancelPrompt implementation echo "Looking at cancelPrompt implementation..." rg -t typescript "cancelPrompt" --include "*.ts" -A 10 # Check how isTransportError is used elsewhere echo "Checking how isTransportError is used elsewhere..." rg -t typescript "isTransportError" --include "*.ts" -A 5Length of output: 1200
Below is an updated review comment reflecting our findings:
Attention: Review and Confirm Error Handling Logic
It appears there's an inconsistency with how
isTransportErroris set. On line 48 the property is computed as!response.success, yet a few lines later (line 53) it’s hardcoded totrue. Please verify that this override is intentional. If not, adjust the logic so that the error classification is consistent throughout this function.
- File: packages/connect/src/device/prompts.ts
- Line 48:
isTransportError: !response.success- Line 53:
isTransportError: true
69-70:❓ Verification inconclusive
Consider addressing the TypeScript error.
The
@ts-expect-errorcomment indicates a potential type mismatch that should be addressed rather than suppressed. This could lead to runtime errors if the actual types don't match expectations.
🏁 Script executed:
#!/bin/bash # Check what the Device.emit method expects and what's being passed # Find the Device class definition echo "Looking for Device class definition..." rg -t typescript "class Device" --include "*.ts" -A 5 # Find the emit method in the Device class echo "Looking for emit method in Device class..." rg -t typescript "emit\(" --include "*.ts" -A 5 # Look for other uses of the emit method with this event pattern echo "Looking for similar emit method usage patterns..." rg -t typescript "emit\(.*callback" --include "*.ts" -A 5Length of output: 1782
Attention: Review and update type handling for device.emit usage.
The current use of@ts-expect-errorat lines 69–70 inpackages/connect/src/device/prompts.tssuppresses a TypeScript error that may indicate an actual type mismatch between what is passed ({ device, callback, ...rest }) and what thedevice.emitmethod expects. This suppression risks hiding potential runtime issues. Please verify the expected parameter types fordevice.emitand update the type definitions or the payload accordingly. If suppressing the error is unavoidable, document the rationale clearly.
Description
Part of #16735 which could be merged beforehand as it does basically nothing.