-
Notifications
You must be signed in to change notification settings - Fork 88
feat(network): surface request ids in node errors #960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(network): surface request ids in node errors #960
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces plain Error throws in node request flows with request IDs and Lit Error types to improve error tracking and debugging.
- Systematically replaces
Errorconstructors with typed error classes (NodeError,LitNodeClientBadConfigError, etc.) - Adds request ID tracking to error messages and structured error information
- Updates function signatures to accept and pass through request IDs for better error traceability
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| executeJs.ts | Updates error handling in executeJs flow to use NodeError with request ID tracking |
| E2EERequestManager.ts | Replaces Error throws with NodeError/LitNodeClientBadConfigError and adds request ID parameter to error handling functions |
| E2EERequestManager.spec.ts | Adds unit test to verify request ID inclusion in error messages |
| APIFactory.ts | Updates API factory methods to use typed errors and pass request IDs to error handlers |
| BaseModuleFactory.ts | Converts plain Error throws to appropriate Lit error types with structured error information |
| createLitClient.ts | Replaces Error constructors with typed errors (LitNodeClientNotReadyError, ParamsMissingError, etc.) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| jitContext: NagaJitContext, | ||
| operationName: string | ||
| operationName: string, | ||
| requestId: string |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter requestId changed from optional (requestId?: string) to required (requestId: string) without updating the function signature documentation. This is a breaking change that could cause compilation errors in existing code that calls this function without providing a requestId.
| requestId: string | |
| requestId?: string |
| interface DecryptBatchResponseOptions { | ||
| requestId?: string; | ||
| operationName?: string; | ||
| } |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new interface DecryptBatchResponseOptions lacks JSDoc documentation explaining its purpose and parameter meanings, especially since it's used in a generic function that could be called from multiple contexts.
…uded-inside-the-thrown-error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…uded-inside-the-thrown-error
…uded-inside-the-thrown-error
…uded-inside-the-thrown-error
…uded-inside-the-thrown-error
WHAT
patchchangeset