Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
48 changes: 43 additions & 5 deletions src/commands/channel/threads.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof import('../../lib/api.js')>()),
...apiMocks,
}))

vi.mock('../../lib/refs.js', () => ({
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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 })],
Expand Down
13 changes: 10 additions & 3 deletions src/commands/channel/threads.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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),
}))
Expand Down
41 changes: 41 additions & 0 deletions src/commands/inbox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
3 changes: 2 additions & 1 deletion src/commands/inbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
assertBatchData,
buildBatchNameMap,
getCurrentWorkspaceId,
getOptionalBatchData,
getTwistClient,
} from '../lib/api.js'
import { withCaseInsensitiveChoices } from '../lib/completion.js'
Expand Down Expand Up @@ -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,
Expand Down
13 changes: 13 additions & 0 deletions src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,19 @@ export function buildBatchNameMap<T extends { id: number; name: string }>(
)
}

/**
* 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<T>(response: BatchResult<T>, 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
Expand Down
Loading