From 7da4668e1d3f4dabd1ca4828b1af7eb233ec9d70 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 28 May 2026 12:29:00 +0100 Subject: [PATCH 1/2] fix(refs): resolve unjoined-but-public channels by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveChannelRef name-branch only called getChannels (membership-scoped), so `tdc channel "Public Channel I Haven't Joined"` failed with CHANNEL_NOT_FOUND. Merge with workspaces.getPublicChannels and dedupe by id — same shape as src/commands/channel/list.ts. Ports Doist/twist-cli#249. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/refs.test.ts | 68 ++++++++++++++++++++++++++++++++++++++++---- src/lib/refs.ts | 16 ++++++++++- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index a321211..bff9eae 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -258,6 +258,17 @@ describe('resolveChannelRef', () => { const mockGetChannel = vi.fn() const mockGetChannels = vi.fn() + const mockGetPublicChannels = vi.fn() + + /** + * For name refs, resolveChannelRef merges joined channels (getChannels — membership-scoped, + * includes both active + archived) with public channels (getPublicChannels — workspace-scoped, + * finds unjoined-but-public channels). Tests default both to empty unless overridden. + */ + function mockChannelLists(joined: unknown[] = [], publicChannels: unknown[] = []) { + mockGetChannels.mockResolvedValue(joined) + mockGetPublicChannels.mockResolvedValue(publicChannels) + } beforeEach(() => { vi.clearAllMocks() @@ -266,6 +277,9 @@ describe('resolveChannelRef', () => { getChannel: mockGetChannel, getChannels: mockGetChannels, }, + workspaces: { + getPublicChannels: mockGetPublicChannels, + }, }) }) @@ -306,19 +320,18 @@ describe('resolveChannelRef', () => { expect(mockGetChannel).not.toHaveBeenCalled() }) - it('resolves exact case-insensitive name match', async () => { + it('resolves exact case-insensitive name match against joined channels', async () => { const ch = createChannel('CHGEN', 'General') - mockGetChannels.mockResolvedValue([ch, createChannel('CHLEAD', 'Leadership')]) + mockChannelLists([ch, createChannel('CHLEAD', 'Leadership')]) const result = await resolveChannelRef('general', 1) - expect(mockGetChannels).toHaveBeenCalledWith({ workspaceId: 1 }) expect(result).toEqual(ch) }) it('resolves unique substring name match', async () => { const ch = createChannel('CHMKT', 'Marketing') - mockGetChannels.mockResolvedValue([createChannel('CHGEN', 'General'), ch]) + mockChannelLists([createChannel('CHGEN', 'General'), ch]) const result = await resolveChannelRef('market', 1) @@ -326,7 +339,7 @@ describe('resolveChannelRef', () => { }) it('throws AMBIGUOUS_CHANNEL on multiple substring matches', async () => { - mockGetChannels.mockResolvedValue([ + mockChannelLists([ createChannel('CHENG', 'Engineering'), createChannel('CHEOP', 'Engineering-Ops'), ]) @@ -338,13 +351,56 @@ describe('resolveChannelRef', () => { }) it('throws CHANNEL_NOT_FOUND when no match', async () => { - mockGetChannels.mockResolvedValue([createChannel('CHGEN', 'General')]) + mockChannelLists([createChannel('CHGEN', 'General')]) await expect(resolveChannelRef('nope', 1)).rejects.toHaveProperty( 'code', 'CHANNEL_NOT_FOUND', ) }) + + it('resolves unjoined-but-public channel by name', async () => { + const publicCh = createChannel('CHPUB1', 'Old Public Channel') + mockChannelLists([createChannel('CHGEN', 'General')], [publicCh]) + + const result = await resolveChannelRef('Old Public Channel', 1) + + expect(result).toEqual(publicCh) + }) + + it('resolves unjoined-but-public channel by substring', async () => { + const publicCh = createChannel('CHSMOKE', 'tw-cli-smoke-test-channel') + mockChannelLists([createChannel('CHGEN', 'General')], [publicCh]) + + const result = await resolveChannelRef('smoke-test', 1) + + expect(result).toEqual(publicCh) + }) + + it('deduplicates channels appearing in both joined and public lists', async () => { + // A public channel the user has joined would appear in both. A substring query + // exercises the dedupe step: without it, matchByName sees two partial matches + // for the same channel id and throws AMBIGUOUS_CHANNEL. An exact-match query + // wouldn't catch a regression because matchByName returns on the first .find. + const joinedPublic = createChannel('CHJP', 'Engineering', { public: true }) + mockChannelLists([joinedPublic], [joinedPublic]) + + const result = await resolveChannelRef('eng', 1) + + expect(result).toEqual(joinedPublic) + }) + + it('throws AMBIGUOUS_CHANNEL on substring matches spanning joined and public lists', async () => { + mockChannelLists( + [createChannel('CHENG', 'Engineering')], + [createChannel('CHEOP', 'Engineering-Ops')], + ) + + await expect(resolveChannelRef('eng', 1)).rejects.toHaveProperty( + 'code', + 'AMBIGUOUS_CHANNEL', + ) + }) }) describe('resolveConversationId', () => { diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 985294b..62f8588 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -270,7 +270,21 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi } if (parsed.type === 'name') { - const channels = await client.channels.getChannels({ workspaceId }) + // getChannels is membership-scoped — it returns only channels the current user has + // joined (across active + archived). Public channels the user hasn't joined are not + // included, so name-resolving e.g. `tdc channel archive "Old Public Channel"` would + // fail with CHANNEL_NOT_FOUND even though the channel is discoverable. Merge with + // getPublicChannels (workspace-scoped, returns all public channels regardless of + // membership) and dedupe by id so a joined-and-public channel doesn't match twice. + const [joined, publicChannels] = await Promise.all([ + client.channels.getChannels({ workspaceId }), + client.workspaces.getPublicChannels(workspaceId), + ]) + const joinedIds = new Set(joined.map((channel) => channel.id)) + const channels = [ + ...joined, + ...publicChannels.filter((channel) => !joinedIds.has(channel.id)), + ] return matchByName(channels, parsed.name, { ambiguousCode: 'AMBIGUOUS_CHANNEL', notFoundCode: 'CHANNEL_NOT_FOUND', From 851dbf4851cb5d8ac05aa4be4bca2293a5b11692 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 28 May 2026 12:35:42 +0100 Subject: [PATCH 2/2] review: defer getPublicChannels + harden dedupe test - Skip workspaces.getPublicChannels when an exact match already exists in the joined list. Common case (user types channel they're in) avoids the workspace-wide call. - Dedupe test now uses distinct object instances with the same id, so it proves dedupe-by-id rather than reference equality. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/refs.test.ts | 16 +++++++++++----- src/lib/refs.ts | 24 ++++++++++++++---------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index bff9eae..c7a077b 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -320,13 +320,16 @@ describe('resolveChannelRef', () => { expect(mockGetChannel).not.toHaveBeenCalled() }) - it('resolves exact case-insensitive name match against joined channels', async () => { + it('resolves exact case-insensitive name match against joined channels without fetching public list', async () => { const ch = createChannel('CHGEN', 'General') mockChannelLists([ch, createChannel('CHLEAD', 'Leadership')]) const result = await resolveChannelRef('general', 1) expect(result).toEqual(ch) + // Common case: exact match in joined list short-circuits before the + // workspace-wide getPublicChannels call. + expect(mockGetPublicChannels).not.toHaveBeenCalled() }) it('resolves unique substring name match', async () => { @@ -378,16 +381,19 @@ describe('resolveChannelRef', () => { }) it('deduplicates channels appearing in both joined and public lists', async () => { - // A public channel the user has joined would appear in both. A substring query + // A public channel the user has joined appears in both responses. Use distinct + // object instances with the same id — that's what two API calls actually return, + // and it ensures dedupe is by id (not reference equality). A substring query // exercises the dedupe step: without it, matchByName sees two partial matches // for the same channel id and throws AMBIGUOUS_CHANNEL. An exact-match query // wouldn't catch a regression because matchByName returns on the first .find. - const joinedPublic = createChannel('CHJP', 'Engineering', { public: true }) - mockChannelLists([joinedPublic], [joinedPublic]) + const joinedCopy = createChannel('CHJP', 'Engineering', { public: true }) + const publicCopy = createChannel('CHJP', 'Engineering', { public: true }) + mockChannelLists([joinedCopy], [publicCopy]) const result = await resolveChannelRef('eng', 1) - expect(result).toEqual(joinedPublic) + expect(result).toEqual(joinedCopy) }) it('throws AMBIGUOUS_CHANNEL on substring matches spanning joined and public lists', async () => { diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 62f8588..c62e8b4 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -270,16 +270,20 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi } if (parsed.type === 'name') { - // getChannels is membership-scoped — it returns only channels the current user has - // joined (across active + archived). Public channels the user hasn't joined are not - // included, so name-resolving e.g. `tdc channel archive "Old Public Channel"` would - // fail with CHANNEL_NOT_FOUND even though the channel is discoverable. Merge with - // getPublicChannels (workspace-scoped, returns all public channels regardless of - // membership) and dedupe by id so a joined-and-public channel doesn't match twice. - const [joined, publicChannels] = await Promise.all([ - client.channels.getChannels({ workspaceId }), - client.workspaces.getPublicChannels(workspaceId), - ]) + // getChannels is membership-scoped — only channels the current user has joined + // (active + archived). Try an exact match against that list first; the common + // case (user types a channel they're in) returns without the workspace-wide + // getPublicChannels call. Fall through only when we need the unjoined-public + // set — for unjoined-but-public matches or cross-list substring resolution. + const joined = await client.channels.getChannels({ workspaceId }) + const lowerName = parsed.name.toLowerCase() + const exactJoined = joined.find((channel) => channel.name.toLowerCase() === lowerName) + if (exactJoined) return exactJoined + + // getPublicChannels is workspace-scoped (all public channels regardless of + // membership). Merge and dedupe by id so a joined-and-public channel doesn't + // match twice through matchByName's substring path. + const publicChannels = await client.workspaces.getPublicChannels(workspaceId) const joinedIds = new Set(joined.map((channel) => channel.id)) const channels = [ ...joined,