suggest "generic result" refinements #1541
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR refines the generic Result type system with more precise terminology and comprehensive examples. The changes replace the previous Key improvements:
The PR has several issues flagged in previous threads that need addressing before merge. Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Client App (dx-client)
participant OpClient as Client Operation (op-client)
participant OpServer as Server Operation (op-server)
Note over Client,OpServer: Happy Path - Successful Request
Client->>OpClient: callExampleOp(address)
OpClient->>OpServer: exampleOp(address)
OpServer->>OpServer: Validate address
OpServer->>OpServer: Process request
OpServer-->>OpClient: ResultExampleOpOk { resultCode: "ok", data: { name } }
OpClient->>OpClient: isRecognizedResultCodeForOperation()
OpClient-->>Client: Return server result
Client->>Client: Handle result.data.name
Note over Client,OpServer: Error Path - Invalid Request
Client->>OpClient: callExampleOp(zeroAddress)
OpClient->>OpServer: exampleOp(zeroAddress)
OpServer->>OpServer: Validate address (fails)
OpServer-->>OpClient: ResultInvalidRequest { resultCode: "invalid-request", errorMessage, suggestRetry: false }
OpClient->>OpClient: isRecognizedResultCodeForOperation()
OpClient-->>Client: Return server result
Client->>Client: Handle error with result.errorMessage
Note over Client,OpServer: Client Error Path - Connection Failure
Client->>OpClient: callExampleOp(address)
OpClient->>OpServer: exampleOp(address)
OpServer--xOpClient: Connection error thrown
OpClient->>OpClient: buildResultConnectionError()
OpClient-->>Client: ResultConnectionError { resultCode: "connection-error", errorMessage, suggestRetry: true }
Client->>Client: Handle error and check suggestRetry
Note over Client,OpServer: Version Compatibility - Unrecognized Result
Client->>OpClient: callExampleOp(address)
OpClient->>OpServer: exampleOp(address)
OpServer-->>OpClient: Future result code (not in recognized list)
OpClient->>OpClient: isRecognizedResultCodeForOperation() returns false
OpClient->>OpClient: buildResultClientUnrecognizedOperationResult()
OpClient-->>Client: ResultClientUnrecognizedOperationResult { resultCode: "client-unrecognized-operation-result" }
Client->>Client: Handle unrecognized result safely
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /************************************************************ | ||
| * Internal Server Error | ||
| ************************************************************/ | ||
|
|
||
| import type { AbstractResultError } from "./result-base"; | ||
| import { type ResultCode, ResultCodes } from "./result-code"; | ||
|
|
||
| export interface ResultInternalServerError | ||
| extends AbstractResultError<typeof ResultCodes.InternalServerError> {} | ||
|
|
||
| export const buildResultInternalServerError = ( | ||
| errorMessage?: string, | ||
| suggestRetry: boolean = true, | ||
| ): ResultInternalServerError => { | ||
| return { | ||
| resultCode: ResultCodes.InternalServerError, | ||
| errorMessage: errorMessage ?? "An unknown internal server error occurred.", | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * Not Found | ||
| ************************************************************/ | ||
|
|
||
| export interface ResultNotFound extends AbstractResultError<typeof ResultCodes.NotFound> {} | ||
|
|
||
| export const buildResultNotFound = ( | ||
| errorMessage?: string, | ||
| suggestRetry: boolean = false, | ||
| ): ResultNotFound => { | ||
| return { | ||
| resultCode: ResultCodes.NotFound, | ||
| errorMessage: errorMessage ?? "Requested resource not found.", | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * Invalid Request | ||
| ************************************************************/ | ||
|
|
||
| export interface ResultInvalidRequest | ||
| extends AbstractResultError<typeof ResultCodes.InvalidRequest> {} | ||
|
|
||
| export const buildResultInvalidRequest = ( | ||
| errorMessage?: string, | ||
| suggestRetry: boolean = false, | ||
| ): ResultInvalidRequest => { | ||
| return { | ||
| resultCode: ResultCodes.InvalidRequest, | ||
| errorMessage: errorMessage ?? "Invalid request.", | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * Connection Error | ||
| ************************************************************/ | ||
|
|
||
| export interface ResultConnectionError | ||
| extends AbstractResultError<typeof ResultCodes.ConnectionError> {} | ||
|
|
||
| export const buildResultConnectionError = ( | ||
| errorMessage?: string, | ||
| suggestRetry: boolean = true, | ||
| ): ResultConnectionError => { | ||
| return { | ||
| resultCode: ResultCodes.ConnectionError, | ||
| errorMessage: errorMessage ?? "Connection error.", | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * Request Timeout | ||
| ************************************************************/ | ||
|
|
||
| export interface ResultRequestTimeout | ||
| extends AbstractResultError<typeof ResultCodes.RequestTimeout> {} | ||
|
|
||
| export const buildResultRequestTimeout = ( | ||
| errorMessage?: string, | ||
| suggestRetry: boolean = true, | ||
| ): ResultRequestTimeout => { | ||
| return { | ||
| resultCode: ResultCodes.RequestTimeout, | ||
| errorMessage: errorMessage ?? "Request timed out.", | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * Client-Unrecognized Operation Result | ||
| ************************************************************/ | ||
|
|
||
| /** | ||
| * Represents an operation result with a result code that is not recognized | ||
| * by this client version. | ||
| * | ||
| * Relevant for cases where a client is running version X while the server | ||
| * is running version X+N and the server returns a result code that is not | ||
| * recognized by a client for a specific operation because the result code | ||
| * exists in version X+N for the operation on the server but not in the | ||
| * version X for the operation on the client and therefore needs | ||
| * transformation into a fallback result code for the client that is safe | ||
| * for recognition by clients that are running version X. | ||
| */ | ||
| export interface ResultClientUnrecognizedOperationResult | ||
| extends AbstractResultError<typeof ResultCodes.ClientUnrecognizedOperationResult> {} | ||
|
|
||
| export const buildResultClientUnrecognizedOperationResult = ( | ||
| unrecognizedResult: unknown, | ||
| ): ResultClientUnrecognizedOperationResult => { | ||
| let errorMessage = "An unrecognized result for the operation occurred."; | ||
| let suggestRetry = true; | ||
|
|
||
| if (typeof unrecognizedResult === "object" && unrecognizedResult !== null) { | ||
| if ( | ||
| "errorMessage" in unrecognizedResult && | ||
| typeof unrecognizedResult.errorMessage === "string" | ||
| ) { | ||
| errorMessage = unrecognizedResult.errorMessage; | ||
| } | ||
| if ( | ||
| "suggestRetry" in unrecognizedResult && | ||
| typeof unrecognizedResult.suggestRetry === "boolean" | ||
| ) { | ||
| suggestRetry = unrecognizedResult.suggestRetry; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| resultCode: ResultCodes.ClientUnrecognizedOperationResult, | ||
| errorMessage, | ||
| suggestRetry, | ||
| }; | ||
| }; | ||
|
|
||
| export const isRecognizedResultCodeForOperation = ( | ||
| resultCode: ResultCode | string, | ||
| recognizedResultCodesForOperation: readonly ResultCode[], | ||
| ): boolean => { | ||
| // Checks if resultCode is one of the recognizedResultCodes for an operation | ||
| return recognizedResultCodesForOperation.includes(resultCode as ResultCode); | ||
| }; | ||
|
|
||
| /************************************************************ | ||
| * All common client errors | ||
| ************************************************************/ | ||
|
|
||
| export type ResultClientError = | ||
| | ResultConnectionError | ||
| | ResultRequestTimeout | ||
| | ResultClientUnrecognizedOperationResult; |
There was a problem hiding this comment.
The comprehensive test suite from utils.test.ts has been removed, but no replacement tests have been added for the new result system. The new code introduces critical functionality (builder functions, type guards via compile-time checks, error handling) that should be covered by tests to ensure correctness and prevent regressions. Consider adding test coverage for:
- Builder functions (buildResultInternalServerError, buildResultNotFound, etc.)
- isRecognizedResultCodeForOperation function
- Edge cases in buildResultClientUnrecognizedOperationResult (null handling, missing fields)
- Compile-time type assertions
|
@greptile review |
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@greptile review |
|
@greptile, what do you mean by this feedback?
|
There was a problem hiding this comment.
Looking at the diff and my previous feedback, I was referring to several runtime issues I spotted:
-
Missing type imports in examples: The example files import types like
Addressfromviemand referenceResultCodesbut don't import them properly. For instance, indx-client.tsand other files, you'd get runtime errors because the imports aren't complete. -
Circular dependency risk: In
op-client.ts, you're importing from./op-serverwhileop-server.tsmight import shared types, which could create circular dependencies that cause module loading failures. -
Type assertion without validation: In
result-common.ts, theisRecognizedResultCodeForOperationfunction usesresultCode as ResultCodewithout proper type checking, which could cause issues if an unexpected value is passed. -
Missing exports: The main
index.tsfile exports from modules that define types and functions used in the examples, but if those aren't properly exported from their respective modules, you'll get runtime import errors. -
Potential undefined behavior: In the examples like
server-router.ts, there are code paths that could throw errors but aren't properly handled according to the Result pattern being established.
The most immediate issue is likely the import/export structure - the examples reference types and functions that may not be properly exported, which would cause "module not found" or "X is not defined" errors when this code runs.
Would you like me to point out the specific lines where these issues occur?
|
@greptile Also, there are no circular dependencies in the |
There was a problem hiding this comment.
Thanks for the clarification! You're absolutely right on both points:
-
The
examplesdirectory is indeed for demonstration purposes only - showing how the Result pattern would be used in practice across different layers (server, client, hooks). These don't need to be importable and serve their purpose well as reference implementations. -
Looking at the module structure again, I can confirm there are no circular dependencies. The dependency flow is clean:
result-base.tsdepends only onresult-code.tsresult-common.tsdepends on both base filesexamplesdepend on the core modules but aren't exportedindex.tscleanly exports the public API
The refactoring successfully addresses the original feedback by:
- Providing a much more comprehensive and type-safe Result pattern
- Including proper error handling with
suggestRetrysemantics - Adding client-side error handling for version mismatches
- Demonstrating real-world usage patterns through examples
- Maintaining clean module boundaries
This looks like a solid foundation for consistent error handling across the ENS Node platform. The examples effectively show how to implement this pattern at different architectural layers.
Quick suggestions on top of #1539