Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses cases where CLI commands assume batched API responses always contain successful .data payloads, causing misleading internal crashes on API validation/request errors. It centralizes batch-response validation so commands surface the real API error message instead.
Changes:
- Enhance
assertBatchDatato extract and display batch API error details (e.g.,errorString) viaCliError('API_ERROR'). - Introduce
buildBatchNameMapand refactor inbox/thread/conversation commands to use it (andassertBatchData) instead of directly accessingresponse.data. - Add regression tests ensuring batch failures (including per-entity user lookups) produce clear errors rather than
TypeErrors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/api.ts |
Adds batch error-message extraction, strengthens assertBatchData, and introduces buildBatchNameMap to safely consume batch results. |
src/commands/thread/view.ts |
Uses shared helpers to validate batch responses and fail fast with clear errors on user lookup failures. |
src/commands/inbox.ts |
Validates inbox/unread batch results and uses buildBatchNameMap for channel name resolution, preventing .data.map crashes. |
src/commands/conversation/view.ts |
Validates conversation/messages batch results and uses buildBatchNameMap for participant names. |
src/commands/conversation/unread.ts |
Validates per-conversation batch lookups and uses buildBatchNameMap for participant names. |
src/commands/conversation/helpers.ts |
Switches user-name map construction to buildBatchNameMap for consistent batch validation behavior. |
src/__tests__/thread.test.ts |
Adds coverage for failed batched user lookup during thread view. |
src/__tests__/inbox.test.ts |
Adds coverage for surfacing a batch API validation error (limit too high) in inbox. |
src/__tests__/conversation.test.ts |
Adds coverage for failed batched user lookup during conversation view. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doistbot
left a comment
There was a problem hiding this comment.
This PR updates the CLI commands to properly surface batch API errors, improving observability and error awareness. However, the updated user lookup logic in the thread view inadvertently causes a hard failure for missing user entries instead of preserving the existing fallback behavior that gracefully skips null results.
| const userMap = new Map( | ||
| userResponses.filter((r) => r.data != null).map((r) => [r.data.id, r.data.name]), | ||
| ) | ||
| const userMap = buildBatchNameMap(userIds, userResponses, 'user') |
There was a problem hiding this comment.
[P1] This changes the thread-view user lookup path from the previous filter((r) => r.data != null) behavior to a hard failure. The surrounding rendering code still expects missing map entries and falls back to user:${id}, so a null user batch result now aborts tw thread view instead of rendering the thread. Please preserve the null-skipping behavior here (and in the similar call below) while still surfacing real error payloads.
Closes #158. The issue includes a repro.