fix: throw CoderAPIError instead of bare string for user ID 0#4
fix: throw CoderAPIError instead of bare string for user ID 0#4johnstcn wants to merge 3 commits into
Conversation
Bare string throws are the "goto" of error handling — technically valid but socially unacceptable. Upgraded the GitHub user ID zero check to throw a proper CoderAPIError with a 400 status, because our errors deserve class (literally). Also added missing awaits in the test because floating promises are just errors with commitment issues.
|
ready for review |
There was a problem hiding this comment.
Pull request overview
This PR updates the Coder client to throw a CoderAPIError (instead of a bare string) when githubUserId is 0, ensuring the action’s top-level error handler preserves and surfaces the correct failure message.
Changes:
- Replace
throw "GitHub user ID cannot be 0"withthrow new CoderAPIError(..., 400)inRealCoderClient.getCoderUserByGitHubId. - Expand the unit test to assert both the error message and error type for the
githubUserId === 0case. - Regenerate
dist/index.jsto include the updated runtime behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coder-client.ts | Throws CoderAPIError for githubUserId === 0 so the caller’s instanceof Error handling keeps the message. |
| src/coder-client.test.ts | Adds assertions for both the thrown message and the CoderAPIError type. |
| dist/index.js | Updates compiled action artifact to reflect the new error behavior (and includes additional regenerated bundler output). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
🤖 Agent on behalf of mafredri
The fix itself is correct, no notes on the code.
Check: This change is a strict subset of PR #3 (test/improve-coverage), which makes the same throw "..." → throw new CoderAPIError(...) change in coder-client.ts plus the corresponding test update. If #3 lands first, this PR will conflict. Recommend closing this in favor of #3 or rebasing #3 on top of this if you want the atomic fix to land independently first.
In PR #4 the test does:
await expect(promise).rejects.toThrow("GitHub user ID cannot be 0");
await expect(promise).rejects.toThrow(CoderAPIError);While PR #3 only checks the type. PR #4's version is slightly more thorough.
En fix, två PRs, noll poäng för koordination. Det blir bättre.
|
Brought into main via #27. Closing. 🤖 This comment was created with the help of Coder Agents. |
Two hardening fixes against `src/coder-client.ts`. Closes #4 and #15. * `getCoderUserByGitHubId(0)` now throws `CoderAPIError(..., 400)` instead of a bare string, so the outer catch in `index.ts` produces a typed error with a useful message. The schema already rejects `0` upstream; this is defense in depth. * `request()` sets a default `AbortSignal.timeout(30000)` on every fetch call so a hung Coder server cannot burn CI minutes up to the job-level timeout. Callers can override via `options.signal`. Timeout failures surface as `CoderAPIError` with the endpoint and duration. 🤖 Authored by Coder Agents.
throw "GitHub user ID cannot be 0"tothrow new CoderAPIError(...)so the catch block in index.ts preserves the error messageCoderAPIErrortype assertion to the existing test