Skip to content

Commit 31e185c

Browse files
fix(users): filter removed members from tdc users by default (#10)
* fix(users): filter removed members from `tdc users` by default `tdc users` was listing users who had been removed from the workspace. Same root cause as twist-cli#245: the Comms API returns both active and removed members with a `removed: boolean` flag. Comms SDK 0.3.0 adds an `includeRemoved` option that filters `removed === true` by default; this change bumps the SDK and threads the option through. - Bump @doist/comms-sdk 0.2.0 → 0.3.0 - `getWorkspaceUsers` accepts `{ includeRemoved? }`; cache keyed by both workspaceId and the flag - `tdc users --include-removed` opts back into the historical roster and renders a red `[removed]` chip - `removed` added to curated user JSON fields - `resolveUserRefs` / `resolveNotifyIds` now default to active-only, fixing the silent "User N is not part of workspace" 403s on channel/group writes that resolve a tombstoned name Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(users): keep removed users in display-name lookup; test cache split Addresses PR #10 review feedback: - `buildUserNameMap` now fetches `includeRemoved: true`. Historical messages from members who have since been removed need to render the author's name, not fall back to `user:123`. The active-only cache entry is left alone for ref-resolution callers. - Add a cache-collision test in `api.test.ts` that calls `getWorkspaceUsers` with both default and `includeRemoved: true` to prove the cache keys don't collide and each variant is itself cached. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 291187f commit 31e185c

9 files changed

Lines changed: 192 additions & 22 deletions

File tree

package-lock.json

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@
5151
],
5252
"dependencies": {
5353
"@doist/cli-core": "0.24.0",
54-
"@doist/comms-sdk": "0.2.0",
54+
"@doist/comms-sdk": "0.3.0",
5555
"@pnpm/tabtab": "0.5.4",
5656
"chalk": "5.6.2",
5757
"commander": "14.0.3",

skills/comms-cli/SKILL.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,8 +208,9 @@ tdc search "query" --all # Fetch all result pages
208208
tdc user # Show current user info
209209
tdc user --json # JSON output
210210
tdc user --json --full # Include all fields in JSON output
211-
tdc users # List workspace users
211+
tdc users # List active workspace users
212212
tdc users --search <text> # Filter by name/email
213+
tdc users --include-removed # Include users removed from the workspace
213214
tdc channels # List active joined workspace channels (alias of: tdc channel list)
214215
tdc channels --state all # Include archived joined channels too
215216
tdc channels --scope discoverable # Active public channels you can see but have not joined

src/commands/user.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,67 @@ describeEmptyMachineOutput('tdc users empty output', {
5656
humanMessage: 'No users found.',
5757
})
5858

59+
describe('tdc users --include-removed', () => {
60+
const active = {
61+
id: 1,
62+
fullName: 'Active',
63+
email: 'a@x',
64+
userType: 'USER',
65+
removed: false,
66+
}
67+
const removed = {
68+
id: 2,
69+
fullName: 'Ghost',
70+
email: 'ghost@x',
71+
userType: 'GUEST',
72+
removed: true,
73+
}
74+
75+
beforeEach(() => {
76+
vi.clearAllMocks()
77+
apiMocks.getCurrentWorkspaceId.mockResolvedValue(1)
78+
})
79+
80+
it('passes includeRemoved: undefined by default so the SDK applies its default filter', async () => {
81+
apiMocks.getWorkspaceUsers.mockResolvedValueOnce([active])
82+
const program = createProgram()
83+
const consoleSpy = captureConsole('log')
84+
85+
await program.parseAsync(['node', 'tdc', 'users'])
86+
87+
expect(apiMocks.getWorkspaceUsers).toHaveBeenCalledWith(1, { includeRemoved: undefined })
88+
expect(consoleSpy.mock.calls.flat().join('\n')).not.toMatch(/\[removed\]/)
89+
})
90+
91+
it('passes includeRemoved: true and annotates removed users in text output', async () => {
92+
apiMocks.getWorkspaceUsers.mockResolvedValueOnce([active, removed])
93+
const program = createProgram()
94+
const consoleSpy = captureConsole('log')
95+
96+
await program.parseAsync(['node', 'tdc', 'users', '--include-removed'])
97+
98+
expect(apiMocks.getWorkspaceUsers).toHaveBeenCalledWith(1, { includeRemoved: true })
99+
const lines = consoleSpy.mock.calls.flat().join('\n')
100+
expect(lines).toMatch(/id:2.*Ghost.*\[removed\]/)
101+
expect(lines).not.toMatch(/id:1.*Active.*\[removed\]/)
102+
})
103+
104+
it('surfaces removed in curated --json output without --full', async () => {
105+
apiMocks.getWorkspaceUsers.mockResolvedValueOnce([active, removed])
106+
const program = createProgram()
107+
const consoleSpy = captureConsole('log')
108+
109+
await program.parseAsync(['node', 'tdc', 'users', '--include-removed', '--json'])
110+
111+
const parsed = JSON.parse(consoleSpy.mock.calls[0][0])
112+
expect(parsed).toHaveLength(2)
113+
expect(parsed[0]).toMatchObject({ id: 1, removed: false })
114+
expect(parsed[1]).toMatchObject({ id: 2, removed: true })
115+
// Curated, not --full: shortName must not leak in.
116+
expect(parsed[0]).not.toHaveProperty('shortName')
117+
})
118+
})
119+
59120
describe('user --json', () => {
60121
const sampleUser = {
61122
id: 42,

src/commands/user.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@ import type { ViewOptions } from '../lib/options.js'
1111
import { colors, formatJson, formatNdjson, printEmpty } from '../lib/output.js'
1212
import { resolveWorkspaceRef } from '../lib/refs.js'
1313

14-
type UsersOptions = ViewOptions & { workspace?: string; search?: string }
14+
type UsersOptions = ViewOptions & {
15+
workspace?: string
16+
search?: string
17+
includeRemoved?: boolean
18+
}
1519

1620
async function showCurrentUser(options: ViewOptions): Promise<void> {
1721
const user = await getSessionUser()
@@ -54,7 +58,7 @@ async function listUsers(workspaceRef: string | undefined, options: UsersOptions
5458
workspaceId = await getCurrentWorkspaceId()
5559
}
5660

57-
let users = await getWorkspaceUsers(workspaceId)
61+
let users = await getWorkspaceUsers(workspaceId, { includeRemoved: options.includeRemoved })
5862

5963
if (options.search) {
6064
const search = options.search.toLowerCase()
@@ -85,7 +89,8 @@ async function listUsers(workspaceRef: string | undefined, options: UsersOptions
8589
const name = u.fullName
8690
const email = u.email ? colors.timestamp(`<${u.email}>`) : ''
8791
const type = colors.channel(`[${u.userType}]`)
88-
console.log(`${id} ${name} ${email} ${type}`)
92+
const removed = u.removed ? chalk.red(' [removed]') : ''
93+
console.log(`${id} ${name} ${email} ${type}${removed}`)
8994
}
9095
}
9196

@@ -109,6 +114,7 @@ Examples:
109114
.description('List users in a workspace')
110115
.option('--workspace <ref>', 'Workspace ID or name')
111116
.option('--search <text>', 'Filter by name/email')
117+
.option('--include-removed', 'Include users who have been removed from the workspace')
112118
.option('--json', 'Output as JSON')
113119
.option('--ndjson', 'Output as newline-delimited JSON')
114120
.option('--full', 'Include all fields in JSON output')
@@ -117,7 +123,8 @@ Examples:
117123
`
118124
Examples:
119125
tdc users
120-
tdc users --search "Jane" --json`,
126+
tdc users --search "Jane" --json
127+
tdc users --include-removed`,
121128
)
122129
.action(listUsers)
123130
}

src/lib/api.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
// Mock the SDK so we can observe how `getWorkspaceUsers` invokes
4+
// `client.workspaceUsers.getWorkspaceUsers`. The real filtering of
5+
// removed users lives in the SDK (≥0.3.0), so the contract this test
6+
// guards is "we pass `includeRemoved` through unchanged."
7+
const getWorkspaceUsersMock = vi.hoisted(() => vi.fn().mockResolvedValue([]))
8+
9+
vi.mock('@doist/comms-sdk', () => ({
10+
CommsApi: class {
11+
workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock }
12+
},
13+
}))
14+
15+
vi.mock('./auth.js', () => ({
16+
getApiToken: vi.fn().mockResolvedValue('test-token'),
17+
}))
18+
19+
vi.mock('./permissions.js', () => ({
20+
ensureWriteAllowed: vi.fn(),
21+
isMutatingMethod: vi.fn().mockReturnValue(false),
22+
}))
23+
24+
vi.mock('./spinner.js', () => ({
25+
withSpinner: <T>(_label: unknown, fn: () => Promise<T>) => fn(),
26+
}))
27+
28+
vi.mock('./progress.js', () => ({
29+
getProgressTracker: () => ({ isEnabled: () => false, emitApiCall: vi.fn() }),
30+
}))
31+
32+
const { clearWorkspaceUserCache, getWorkspaceUsers } = await import('./api.js')
33+
34+
describe('getWorkspaceUsers', () => {
35+
beforeEach(() => {
36+
getWorkspaceUsersMock.mockClear()
37+
clearWorkspaceUserCache()
38+
})
39+
40+
it('passes includeRemoved: undefined by default so the SDK applies its default filter', async () => {
41+
await getWorkspaceUsers(1585)
42+
expect(getWorkspaceUsersMock).toHaveBeenCalledWith({
43+
workspaceId: 1585,
44+
includeRemoved: undefined,
45+
})
46+
})
47+
48+
it('forwards includeRemoved: true to the SDK', async () => {
49+
await getWorkspaceUsers(1585, { includeRemoved: true })
50+
expect(getWorkspaceUsersMock).toHaveBeenCalledWith({
51+
workspaceId: 1585,
52+
includeRemoved: true,
53+
})
54+
})
55+
56+
it('caches active and include-removed variants separately', async () => {
57+
// First call seeds the active-only cache entry.
58+
await getWorkspaceUsers(1585)
59+
// Second call (same workspace, default flag) must hit cache → no extra SDK call.
60+
await getWorkspaceUsers(1585)
61+
expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(1)
62+
63+
// Switching to include-removed must NOT collide with the active entry.
64+
await getWorkspaceUsers(1585, { includeRemoved: true })
65+
expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2)
66+
expect(getWorkspaceUsersMock).toHaveBeenLastCalledWith({
67+
workspaceId: 1585,
68+
includeRemoved: true,
69+
})
70+
71+
// And the include-removed variant is itself cached.
72+
await getWorkspaceUsers(1585, { includeRemoved: true })
73+
expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2)
74+
})
75+
})

src/lib/api.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -305,19 +305,30 @@ export async function getSessionUser(): Promise<User> {
305305
return sessionUserCache
306306
}
307307

308-
const workspaceUserCache = new Map<number, WorkspaceUser[]>()
308+
const workspaceUserCache = new Map<string, WorkspaceUser[]>()
309309

310-
async function loadWorkspaceUsers(workspaceId: number): Promise<WorkspaceUser[]> {
311-
const cached = workspaceUserCache.get(workspaceId)
310+
function workspaceUserCacheKey(workspaceId: number, includeRemoved: boolean | undefined): string {
311+
return `${workspaceId}:${includeRemoved ? 'all' : 'active'}`
312+
}
313+
314+
async function loadWorkspaceUsers(
315+
workspaceId: number,
316+
includeRemoved: boolean | undefined,
317+
): Promise<WorkspaceUser[]> {
318+
const key = workspaceUserCacheKey(workspaceId, includeRemoved)
319+
const cached = workspaceUserCache.get(key)
312320
if (cached) return cached
313321
const client = await getCommsClient()
314-
const users = await client.workspaceUsers.getWorkspaceUsers({ workspaceId })
315-
workspaceUserCache.set(workspaceId, users)
322+
const users = await client.workspaceUsers.getWorkspaceUsers({ workspaceId, includeRemoved })
323+
workspaceUserCache.set(key, users)
316324
return users
317325
}
318326

319-
export async function getWorkspaceUsers(workspaceId: number): Promise<WorkspaceUser[]> {
320-
return loadWorkspaceUsers(workspaceId)
327+
export async function getWorkspaceUsers(
328+
workspaceId: number,
329+
options: { includeRemoved?: boolean } = {},
330+
): Promise<WorkspaceUser[]> {
331+
return loadWorkspaceUsers(workspaceId, options.includeRemoved)
321332
}
322333

323334
export function clearWorkspaceUserCache(): void {
@@ -335,14 +346,21 @@ export async function buildUserNameMap(
335346
workspaceId: number,
336347
client?: CommsApi,
337348
): Promise<Map<number, string>> {
338-
const cached = workspaceUserCache.get(workspaceId)
349+
// Historical messages can reference users who have since been removed;
350+
// those messages should still render the author's name rather than fall
351+
// back to `user:123`. So this map always pulls the full roster.
352+
const key = workspaceUserCacheKey(workspaceId, true)
353+
const cached = workspaceUserCache.get(key)
339354
let users: WorkspaceUser[]
340355
if (cached) {
341356
users = cached
342357
} else {
343358
const api = client ?? (await getCommsClient())
344-
users = await api.workspaceUsers.getWorkspaceUsers({ workspaceId })
345-
workspaceUserCache.set(workspaceId, users)
359+
users = await api.workspaceUsers.getWorkspaceUsers({
360+
workspaceId,
361+
includeRemoved: true,
362+
})
363+
workspaceUserCache.set(key, users)
346364
}
347365
return new Map(users.map((u) => [u.id, u.fullName]))
348366
}

src/lib/output.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,14 @@ const MESSAGE_ESSENTIAL_FIELDS = [
5858

5959
const WORKSPACE_ESSENTIAL_FIELDS = ['id', 'name', 'creator', 'plan'] as const
6060

61-
const USER_ESSENTIAL_FIELDS = ['id', 'fullName', 'email', 'timezone', 'userType'] as const
61+
const USER_ESSENTIAL_FIELDS = [
62+
'id',
63+
'fullName',
64+
'email',
65+
'timezone',
66+
'userType',
67+
'removed',
68+
] as const
6269

6370
const CHANNEL_ESSENTIAL_FIELDS = [
6471
'id',

src/lib/skills/content.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,8 +212,9 @@ tdc search "query" --all # Fetch all result pages
212212
tdc user # Show current user info
213213
tdc user --json # JSON output
214214
tdc user --json --full # Include all fields in JSON output
215-
tdc users # List workspace users
215+
tdc users # List active workspace users
216216
tdc users --search <text> # Filter by name/email
217+
tdc users --include-removed # Include users removed from the workspace
217218
tdc channels # List active joined workspace channels (alias of: tdc channel list)
218219
tdc channels --state all # Include archived joined channels too
219220
tdc channels --scope discoverable # Active public channels you can see but have not joined

0 commit comments

Comments
 (0)