Skip to content

fix: guard unreadResp.data against null in channel threads command#224

Merged
scottlovegrove merged 2 commits into
mainfrom
lmjabreu/suspicious-murdock-5a5b7d
May 13, 2026
Merged

fix: guard unreadResp.data against null in channel threads command#224
scottlovegrove merged 2 commits into
mainfrom
lmjabreu/suspicious-murdock-5a5b7d

Conversation

@lmjabreu
Copy link
Copy Markdown
Contributor

What

tw channel threads <name> --json crashed with:

{"error":{"code":"INTERNAL_ERROR","message":"Cannot read properties of undefined (reading 'map')"}}

Root cause

src/commands/channel/threads.ts:89 called .map() directly on unreadResp.data with no null guard:

const unreadThreadIds = new Set(unreadResp.data.map((u) => u.threadId))

The SDK's client.batch() returns { code, data: null } when a sub-request fails or when getUnread returns no data (e.g. the user has zero unread threads in the workspace). Calling .map() on null throws.

Fix

One-line change — add ?? [] so a null/undefined data falls back to an empty set, treating all threads as read:

const unreadThreadIds = new Set((unreadResp.data ?? []).map((u) => u.threadId))

assertBatchData() was intentionally not used here — having no unread threads is a valid state, not an error.

Test

Added a regression test to the existing src/commands/channel/threads.test.ts that mocks the batch response with data: null for the unread sub-request and asserts the command resolves without throwing, with all threads marked isUnread: false.

Verification

  • npm run type-check
  • npm test ✅ (587 tests, 0 failures)
  • Repro command: tw channel threads "Doist Component Library" --no-spinner --json

@doistbot doistbot requested a review from frankieyan May 13, 2026 12:27
Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR nicely fixes a crash in the channel threads command by adding a necessary null guard to the unread threads response. The implementation correctly handles valid empty states, and the included regression test ensures this stability is maintained. A few additional adjustments are recommended, specifically applying this same null guard to the inbox command, using assertBatchData on the primary threads response to prevent a similar crash, and simplifying the test mock setup by updating the setupClient helper.

Share FeedbackReview Logs

Comment thread src/commands/channel/threads.ts Outdated
Comment thread src/commands/channel/threads.ts Outdated
Comment thread src/commands/channel/threads.test.ts Outdated
lmjabreu and others added 2 commits May 13, 2026 14:04
When the workspace has no unread threads, `client.threads.getUnread`
can return a batch response with `data: null`, causing an uncaught
TypeError on `.map()`. Add a `?? []` fallback so the command treats
all threads as read instead of crashing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…/inbox

Address review feedback on PR #224 now that #159 is merged:

- Add `getOptionalBatchData` helper in `src/lib/api.ts` — returns `data` (or
  null) on success, throws via `assertBatchData` only on real API errors. Use
  for batch sub-requests where `data: null` with a success code is a valid
  empty state (e.g. `getUnread` when the workspace has zero unread threads).
- `channel/threads.ts`: use `assertBatchData` for primary `threadsResp` so a
  failed `getThreads` sub-request surfaces an API error instead of crashing
  with `TypeError: .map is not a function`. Use `getOptionalBatchData` for
  `unreadResp` to preserve the "no unread = empty" valid state.
- `inbox.ts`: switch `unreadResponse` from strict `assertBatchData` to
  `getOptionalBatchData` (fixes regression introduced by #159 where a
  workspace with zero unread threads would crash `tw inbox`).
- `channel/threads.test.ts`: update `setupClient` to accept `unread: null`,
  add `code: 200` to batch mock fixtures, refactor the regression test to
  use the helper, and add a new test for the threads-batch-error path.
- `inbox.test.ts`: add a regression test for the null-unread case.
- Document the four batch helpers and when to use each in AGENTS.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove force-pushed the lmjabreu/suspicious-murdock-5a5b7d branch from d973f86 to ebc641c Compare May 13, 2026 13:07
@scottlovegrove
Copy link
Copy Markdown
Collaborator

scottlovegrove commented May 13, 2026

Hey @lmjabreu — pushed a follow-up commit on top of your fix now that #159 is merged.

Quick summary of the adjustments:

  • Added a getOptionalBatchData helper in src/lib/api.ts so the "null = valid empty state" pattern is reusable (and consistent with #159's helpers).
  • Used assertBatchData on threadsResp so a failed getThreads sub-request surfaces a clean error instead of crashing the same way unreadResp did.
  • Applied the same getOptionalBatchData treatment to the unread response in inbox.ts, which was actually crashing under the same conditions after fix: surface batch API errors in commands #159 landed.
  • Refactored the regression test to use setupClient({ unread: null }), plus added tests for the new error/empty paths.
  • Documented the four batch helpers (and when to use each) in AGENTS.md.

Functionally your original fix is preserved — the new helper just makes the pattern explicit and reusable.

@scottlovegrove scottlovegrove merged commit 617a617 into main May 13, 2026
5 checks passed
@scottlovegrove scottlovegrove deleted the lmjabreu/suspicious-murdock-5a5b7d branch May 13, 2026 13:33
doist-release-bot Bot added a commit that referenced this pull request May 13, 2026
## [2.36.5](v2.36.4...v2.36.5) (2026-05-13)

### Bug Fixes

* guard unreadResp.data against null in channel threads command ([#224](#224)) ([617a617](617a617))
@doist-release-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.36.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@frankieyan
Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants