diff --git a/AGENTS.md b/AGENTS.md index 36ec5ef..30628dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -61,11 +61,12 @@ This is a TypeScript CLI (`tw`) for Twist messaging, built with Commander.js. - **JSON output on mutating commands**: Mutating commands (create, update, delete, archive) should support `--json` output where it provides scripting value. Commands that return an object from the API (create/update) should also support `--full`. Commands where the API returns void should output a minimal status object (e.g. `{ id, deleted: true }` or `{ id, isArchived: true }`). Extend `MutationOptions` in `src/lib/options.ts` (which already includes `json` and `full`) rather than adding these fields ad hoc. Use `formatJson()` from `src/lib/output.ts` for the output. See `src/commands/away.ts` as the reference implementation. - **Spinner messages**: When adding new SDK method calls, add a corresponding entry in the `API_SPINNER_MESSAGES` map in `src/lib/api.ts`. Every user-facing API call should have a spinner message so the CLI shows progress feedback. - **Batch API responses**: When calling `client.batch(...)`, never access `.data` directly on a batch result. Use these helpers from `src/lib/api.ts`: - - `assertBatchData(response, label)` — single result; throws `CliError` on any failure. + - `assertBatchData(response, label)` — single result; throws `CliError` on any failure (including `data: null`). Use for primary data the command can't render without (e.g. the thread itself). + - `getOptionalBatchData(response, label)` — tolerant single result; returns `data` (or `null`) on success, throws only on real API errors (`code >= 400`). Use when `data: null` with a success code is a valid empty state (e.g. `getUnread` returning null for "no unread threads"). Callers typically chain `?? []` / `?? defaultValue`. - `buildBatchNameMap(ids, responses, label)` — strict parallel lookup; use when every id must resolve (e.g. channels). - `buildOptionalBatchNameMap(ids, responses, label)` — tolerant parallel lookup; skips entries with `data: null` and a success code (e.g. deleted users) but still throws on real API errors. Callers must fall back via `userMap.get(id) ?? \`user:${id}\``. Use for user lookups so a single missing user doesn't abort the whole command. - See `src/commands/inbox.ts` (strict) and `src/commands/thread/view.ts` (tolerant) for reference. + See `src/commands/inbox.ts` and `src/commands/channel/threads.ts` (mix of strict primary + tolerant unread) and `src/commands/thread/view.ts` (tolerant user map) for reference. ## Error Handling diff --git a/src/commands/channel/threads.test.ts b/src/commands/channel/threads.test.ts index 4ba50bd..45c83c5 100644 --- a/src/commands/channel/threads.test.ts +++ b/src/commands/channel/threads.test.ts @@ -11,9 +11,9 @@ const refsMocks = vi.hoisted(() => ({ resolveChannelRef: vi.fn(), })) -vi.mock('../../lib/api.js', () => ({ - getTwistClient: apiMocks.getTwistClient, - getCurrentWorkspaceId: apiMocks.getCurrentWorkspaceId, +vi.mock('../../lib/api.js', async (importOriginal) => ({ + ...(await importOriginal()), + ...apiMocks, })) vi.mock('../../lib/refs.js', () => ({ @@ -89,11 +89,14 @@ function setupClient({ unread = [], }: { threads?: Thread[] - unread?: { threadId: number }[] + unread?: { threadId: number }[] | null } = {}) { const mockGetThreads = vi.fn().mockReturnValue('threads-descriptor') const mockGetUnread = vi.fn().mockReturnValue('unread-descriptor') - const mockBatch = vi.fn().mockResolvedValue([{ data: threads }, { data: unread }]) + const mockBatch = vi.fn().mockResolvedValue([ + { code: 200, data: threads }, + { code: 200, data: unread }, + ]) apiMocks.getTwistClient.mockResolvedValue({ threads: { getThreads: mockGetThreads, getUnread: mockGetUnread }, @@ -548,6 +551,41 @@ describe('channel threads', () => { consoleSpy.mockRestore() }) + it('does not crash when unreadResp.data is null (regression: batch getUnread returning null)', async () => { + setupClient({ threads: [createThread(1)], unread: null }) + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + const program = createProgram() + + await expect( + program.parseAsync(['node', 'tw', 'channel', 'threads', '12345', '--json']), + ).resolves.not.toThrow() + + const output = JSON.parse(consoleSpy.mock.calls[0][0]) + expect(output.results).toHaveLength(1) + expect(output.results[0].isUnread).toBe(false) + + consoleSpy.mockRestore() + }) + + it('surfaces API error when threads batch sub-request fails', async () => { + const mockBatch = vi.fn().mockResolvedValue([ + { code: 403, data: { errorString: 'Channel access denied' } }, + { code: 200, data: [] }, + ]) + apiMocks.getTwistClient.mockResolvedValue({ + threads: { + getThreads: vi.fn().mockReturnValue('threads-descriptor'), + getUnread: vi.fn().mockReturnValue('unread-descriptor'), + }, + batch: mockBatch, + }) + const program = createProgram() + + await expect( + program.parseAsync(['node', 'tw', 'channel', 'threads', '12345', '--json']), + ).rejects.toThrow('Failed to fetch threads: Channel access denied') + }) + it('--full bypasses the essential-field filter in JSON output', async () => { setupClient({ threads: [createThread(1, { pinned: true })], diff --git a/src/commands/channel/threads.ts b/src/commands/channel/threads.ts index 15a774a..60b2003 100644 --- a/src/commands/channel/threads.ts +++ b/src/commands/channel/threads.ts @@ -1,6 +1,11 @@ import type { ArchiveFilter, Thread } from '@doist/twist-sdk' import chalk from 'chalk' -import { getCurrentWorkspaceId, getTwistClient } from '../../lib/api.js' +import { + assertBatchData, + getCurrentWorkspaceId, + getOptionalBatchData, + getTwistClient, +} from '../../lib/api.js' import { formatRelativeDate } from '../../lib/dates.js' import { CliError } from '../../lib/errors.js' import { isAccessible } from '../../lib/global-args.js' @@ -86,8 +91,10 @@ export async function showChannelThreads( client.threads.getUnread(workspaceId, { batch: true }), ) - const unreadThreadIds = new Set(unreadResp.data.map((u) => u.threadId)) - let threads: DecoratedThread[] = threadsResp.data.map((t) => ({ + const threadsData = assertBatchData(threadsResp, 'threads') + const unreadData = getOptionalBatchData(unreadResp, 'unread threads') ?? [] + const unreadThreadIds = new Set(unreadData.map((u) => u.threadId)) + let threads: DecoratedThread[] = threadsData.map((t) => ({ ...t, isUnread: unreadThreadIds.has(t.id), })) diff --git a/src/commands/inbox.test.ts b/src/commands/inbox.test.ts index 1da4014..7ca6a2a 100644 --- a/src/commands/inbox.test.ts +++ b/src/commands/inbox.test.ts @@ -212,4 +212,45 @@ describe('inbox batch errors', () => { program.parseAsync(['node', 'tw', 'inbox', '--unread', '--limit', '1000']), ).rejects.toThrow('Failed to fetch inbox threads: limit must be less than or equal to 500') }) + + it('treats a null unread batch response as no unread threads', async () => { + const thread = { + id: 1, + channelId: 10, + title: 't', + posted: '2026-05-01T00:00:00Z', + url: 'http://example/t', + } + const mockBatch = vi + .fn() + .mockResolvedValueOnce([ + { code: 200, data: [thread] }, + { code: 200, data: null }, + ]) + .mockResolvedValueOnce([{ code: 200, data: { id: 10, name: 'engineering' } }]) + apiMocks.getTwistClient.mockResolvedValue({ + inbox: { + getInbox: vi.fn((_args: unknown, options?: { batch?: boolean }) => + options?.batch ? { kind: 'inbox' } : Promise.resolve([]), + ), + }, + threads: { + getUnread: vi.fn((_workspaceId: number, options?: { batch?: boolean }) => + options?.batch ? { kind: 'unread' } : Promise.resolve([]), + ), + }, + channels: { getChannel: vi.fn() }, + batch: mockBatch, + }) + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + const program = createProgram() + + await program.parseAsync(['node', 'tw', 'inbox', '--json']) + + const output = JSON.parse(consoleSpy.mock.calls[0][0]) + expect(output).toHaveLength(1) + expect(output[0]).toMatchObject({ id: 1, isUnread: false }) + + consoleSpy.mockRestore() + }) }) diff --git a/src/commands/inbox.ts b/src/commands/inbox.ts index 8cbdb1d..8c94a8b 100644 --- a/src/commands/inbox.ts +++ b/src/commands/inbox.ts @@ -5,6 +5,7 @@ import { assertBatchData, buildBatchNameMap, getCurrentWorkspaceId, + getOptionalBatchData, getTwistClient, } from '../lib/api.js' import { withCaseInsensitiveChoices } from '../lib/completion.js' @@ -59,7 +60,7 @@ async function showInbox(workspaceRef: string | undefined, options: InboxOptions ) const threads = assertBatchData(threadsResponse, 'inbox threads') - const unreadData = assertBatchData(unreadResponse, 'unread threads') + const unreadData = getOptionalBatchData(unreadResponse, 'unread threads') ?? [] const unreadThreadIds = new Set(unreadData.map((u) => u.threadId)) let inboxThreads = threads.map((t) => ({ ...t, diff --git a/src/lib/api.ts b/src/lib/api.ts index a4c6bc4..590c6bb 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -404,6 +404,19 @@ export function buildBatchNameMap( ) } +/** + * Like `assertBatchData` but tolerates a null `data` with a success code as a + * valid empty state (e.g. `getUnread` returning null when there are no unread + * threads). Real API errors (`code >= 400`) still throw. Returns `null` when + * the response is a success-but-empty so the caller can fall back (e.g. `?? []`). + */ +export function getOptionalBatchData(response: BatchResult, label: string): T | null { + if (response.code < 400) { + return response.data ?? null + } + return assertBatchData(response, label) +} + /** * Like `buildBatchNameMap` but skips entries whose `data` is null with a success * code (e.g. a user that no longer exists). Real API errors (`code >= 400`) still