feat: Improve error handling from 3P requests on interceptor#238
Open
atilafassina wants to merge 7 commits intomainfrom
Open
feat: Improve error handling from 3P requests on interceptor#238atilafassina wants to merge 7 commits intomainfrom
atilafassina wants to merge 7 commits intomainfrom
Conversation
Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]>
…subclass coverage Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]>
2 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ExecutionResult<T> return type for Plugin.execute() so plugins can handle failures without relying on undefined, and updates the Files plugin + generated docs to use the new discriminated union shape.
Changes:
- Added
ExecutionResult<T>and changedPlugin.execute()to return{ ok: true, data } | { ok: false, status, message }instead ofT | undefined. - Updated
FilesPluginhandlers to branch onresult.okand returnresult.dataon success. - Updated unit tests and TypeDoc outputs to reflect the new API.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/appkit/src/plugin/plugin.ts | Changes execute() contract to return ExecutionResult<T> and maps errors into { ok:false }. |
| packages/appkit/src/plugin/execution-result.ts | Introduces the ExecutionResult<T> type alias. |
| packages/appkit/src/plugin/index.ts | Re-exports ExecutionResult from the plugin module. |
| packages/appkit/src/index.ts | Re-exports ExecutionResult from the top-level AppKit API surface. |
| packages/appkit/src/plugins/files/plugin.ts | Updates route handlers to use result.ok / result.data instead of checking for undefined. |
| packages/appkit/src/plugin/tests/plugin.test.ts | Updates expectations for the new ExecutionResult shape and adds status mapping tests. |
| docs/docs/api/appkit/* | Updates generated TypeDoc pages/sidebar to include ExecutionResult and the updated Plugin.execute() signature/docs. |
| docs/static/appkit-ui/styles.gen.css | Generated CSS output changes (unrelated to execution result logic). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]>
* refactor: use HTTP status code descriptions instead of hardcoded error strings Replace hardcoded error messages like "List failed" with standard HTTP status descriptions from node:http STATUS_CODES for consistent error responses. Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]> * chore: dry status error responses in files plugin Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]> * fix: standardize stream error to use _sendStatusError helper Addresses Copilot review comment about mixed error formats in the read/download stream handler. Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]> --------- Signed-off-by: Atila Fassina <[email protected]>
The _sendStatusError() helper returns STATUS_CODES[status] (e.g., "Not Found" for 404, "Conflict" for 409), but tests incorrectly expected "Internal Server Error" for all error status codes. Co-authored-by: Isaac Signed-off-by: Atila Fassina <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At the current state, the executor swallows external API errors in order to prevent the app from crashing.
While we still want to prevent the app from crashing, and these errors are not intended for user consumption, returning
undefinedfrom the interceptor means the plugins are unable to handle errors more gracefully and everything is looped as a server error.In this change we still don't throw the error, and we implement a
Resulttype, this way the integrated plugin always knows what shape to expect:When it's an error, it will receive the real status code from the API and a message. With the status code, the plugin can then decide how to handle the error.
If a success, the actual payload is under
dataand is typed properly, just as it was in the previous type shape (Promise<T | undefined>)