Skip to content

fix(refs): resolve unjoined-but-public channels by name #249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lmjabreu wants to merge 1 commit into
base: main
Choose a base branch
from
+74 −9
Open

fix(refs): resolve unjoined-but-public channels by name #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 58 additions & 8 deletions src/lib/refs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,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()
Expand All @@ -224,6 +235,9 @@ describe('resolveChannelRef', () => {
getChannel: mockGetChannel,
getChannels: mockGetChannels,
},
workspaces: {
getPublicChannels: mockGetPublicChannels,
},
})
})

Expand Down Expand Up @@ -265,30 +279,28 @@ 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(10, 'General')
mockGetChannels.mockResolvedValue([ch, createChannel(20, 'Leadership')])
mockChannelLists([ch, createChannel(20, 'Leadership')])

const result = await resolveChannelRef('general', 1)

expect(mockGetChannels).toHaveBeenCalledWith({ workspaceId: 1 })
expect(mockGetPublicChannels).toHaveBeenCalledWith(1)
Copy link
Copy Markdown
Member

@doistbot doistbot May 23, 2026

Choose a reason for hiding this comment

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

[P3] This assertion hard-codes the current Promise.all fetch strategy. If resolveChannelRef() is later optimized to return after an exact joined-channel match, this test will fail even though the behavior is still correct. The result assertion already covers the contract here; drop the getPublicChannels call expectation.

expect(result).toEqual(ch)
})

it('resolves unique substring name match', async () => {
const ch = createChannel(30, 'Marketing')
mockGetChannels.mockResolvedValue([createChannel(10, 'General'), ch])
mockChannelLists([createChannel(10, 'General'), ch])

const result = await resolveChannelRef('market', 1)

expect(result).toEqual(ch)
})

it('throws AMBIGUOUS_CHANNEL on multiple substring matches', async () => {
mockGetChannels.mockResolvedValue([
createChannel(10, 'Engineering'),
createChannel(20, 'Engineering-Ops'),
])
mockChannelLists([createChannel(10, 'Engineering'), createChannel(20, 'Engineering-Ops')])

await expect(resolveChannelRef('eng', 1)).rejects.toHaveProperty(
'code',
Expand All @@ -297,13 +309,51 @@ describe('resolveChannelRef', () => {
})

it('throws CHANNEL_NOT_FOUND when no match', async () => {
mockGetChannels.mockResolvedValue([createChannel(10, 'General')])
mockChannelLists([createChannel(10, 'General')])

await expect(resolveChannelRef('nope', 1)).rejects.toHaveProperty(
'code',
'CHANNEL_NOT_FOUND',
)
})

it('resolves unjoined-but-public channel by name', async () => {
const publicCh = createChannel(50, 'Old Public Channel')
mockChannelLists([createChannel(10, '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(60, 'tw-cli-smoke-test-channel')
mockChannelLists([createChannel(10, 'General')], [publicCh])

const result = await resolveChannelRef('smoke-test', 1)

expect(result).toEqual(publicCh)
})

it('deduplicates channels appearing in both joined and public lists', async () => {
Copy link
Copy Markdown
Member

@doistbot doistbot May 23, 2026

Choose a reason for hiding this comment

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

[P2] This test doesn't actually prove deduplication. matchByName() returns the first exact case-insensitive match before it checks for ambiguity, so this still passes if the same channel appears twice in the merged list. Use a non-exact query (for example, a substring match) so the test would fail without the dedupe step.

// A public channel the user has joined would appear in both. Resolution must not
// throw AMBIGUOUS_CHANNEL just because the same channel id shows up twice.
const joinedPublic = createChannel(70, 'Engineering', { public: true })
mockChannelLists([joinedPublic], [joinedPublic])

const result = await resolveChannelRef('Engineering', 1)

expect(result).toEqual(joinedPublic)
})

it('throws AMBIGUOUS_CHANNEL on substring matches spanning joined and public lists', async () => {
mockChannelLists([createChannel(10, 'Engineering')], [createChannel(20, 'Engineering-Ops')])

await expect(resolveChannelRef('eng', 1)).rejects.toHaveProperty(
'code',
'AMBIGUOUS_CHANNEL',
)
})
})

describe('resolveConversationId', () => {
Expand Down
17 changes: 16 additions & 1 deletion src/lib/refs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,22 @@ 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. `tw 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([
Copy link
Copy Markdown
Member

@doistbot doistbot May 23, 2026

Choose a reason for hiding this comment

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

[P2] This Promise.all makes every name-based channel lookup depend on workspaces.getPublicChannels, even when the ref is an exact match in the joined list. That widens the failure surface for common lookups and adds a full public-channel fetch to all resolveChannelRef() callers. A better fit here is to resolve against getChannels({ workspaceId }) first and only fetch/merge getPublicChannels(workspaceId) when the joined list alone can't produce an exact match.

client.channels.getChannels({ workspaceId }),
client.workspaces.getPublicChannels(workspaceId),
])
const byId = new Map<number, Channel>()
Copy link
Copy Markdown
Member

@doistbot doistbot May 23, 2026

Choose a reason for hiding this comment

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

[P3] This logic introduces a new Map-based approach for deduplication. src/commands/channel/list.ts handles this exact same joined/public intersection by using a Set of joined IDs. Aligning with that existing pattern reduces drift and simplifies the code:

const joinedIds = new Set(joined.map((channel) => channel.id))
const channels = [
    ...joined,
    ...publicChannels.filter((channel) => !joinedIds.has(channel.id))
]

for (const channel of joined) byId.set(channel.id, channel)
for (const channel of publicChannels) {
if (!byId.has(channel.id)) byId.set(channel.id, channel)
}
const channels = Array.from(byId.values())
return matchByName(channels, parsed.name, {
ambiguousCode: 'AMBIGUOUS_CHANNEL',
notFoundCode: 'CHANNEL_NOT_FOUND',
Expand Down
Loading