Skip to content

feat(channel): support default thread audiences - #72

Draft
jacobo-doist wants to merge 7 commits into
mainfrom
jacobo/default-audience-cli-add
Draft

jacobo-doist wants to merge 7 commits into
mainfrom
jacobo/default-audience-cli-add

Conversation

@jacobo-doist

@jacobo-doist jacobo-doist commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Adds options to set the default audience for new threads in a channel.

Both tdc channel create and tdc channel update accept:

  • --default-groups: set the groups by ID or name.
  • --default-users: set the users by ID, email, or name.
  • --clear-default-audience: disable the default audience and empty both lists.

Each list option replaces that list. On update, a list you leave out stays unchanged. Empty lists are rejected. The clear option cannot be combined with either list option.

--dry-run shows the resolved IDs before making changes. channel members list --json includes the default audience with group names and user names and emails.

Updates @doist/comms-sdk to 3.9.0 for the audience request types.

@jacobo-doist

Copy link
Copy Markdown
Contributor Author

@doistbot /review

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adds default thread audience support to channel create/update (--default-groups, --default-users, --clear-default-audience) with validated conflicting/empty inputs, workspace-correct ref resolution, dry-run output of resolved IDs, and audience details surfaced in channel members list --json. The diff follows the repo's established patterns — CliError validation, existing ref-resolution and SDK transport paths, synced skill docs — with thorough test coverage, and no inline issues were flagged.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (6)
  • P3 src/commands/channel/default-audience.ts:46: Resolving each group ref via resolveGroupRef causes one full getWorkspaceGroups/groups.getGroups call per name ref, so --default-groups "Design,Backend" fetches the whole group list twice. resolveChannelMemberRefs already avoids this by fetching the workspace group list once and matching all name refs against it; reuse that pattern here (e.g., split id refs from name refs and fetch the list once).
  • P3 src/commands/channel/members.ts:55: The slim JSON now carries the same audience data twice: useDefaultRecipients, defaultGroups, and defaultRecipients are all already present inside defaultAudience (as defaultAudience.useDefaultRecipients, defaultAudience.groups[].id, and defaultAudience.users[].id). Keeping only defaultAudience removes the duplicate state and keeps the curated payload smaller.
  • P3 src/commands/channel/default-audience.ts:51: When both --default-groups and --default-users are supplied, the group refs fully resolve before resolveUserRefs starts. The two resolutions are independent, so the command currently pays the sum of their latencies. Run the group and user resolution concurrently (e.g. via Promise.all) to reduce wall-clock time.
  • P3 src/commands/channel/channel.test.ts:1086: This generates 24 near-identical cases (2 commands × 2 flags × 6 refs) for a single refs.split(',').some((ref) => !ref.trim()) guard. Every variant hits the same validation branch and asserts the same error code and no-mutation outcome, so the matrix adds maintenance cost without additional regression signal. Keep one truly empty and one empty-segment variant per flag, or unit-test validateDefaultAudienceOptions directly instead of spinning up the full CLI for each combination.
  • P3 src/commands/channel/channel.test.ts:991: flags[1].includes(',') couples the assertion to the exact argument layout and skips the second-group check whenever the first value has no comma (the combined --default-groups ... --default-users ... case). It also never asserts the number of resolveGroupRef calls, so extra or reordered resolutions aren't caught here. The mutation-args assertion already verifies the resolved IDs; either drop these redundant ref-call checks or assert the exact expected resolveGroupRef calls per case.
  • P3 src/commands/channel/members.test.ts:166: client.channels.getChannel is never called by listChannelMembers (channel resolution is mocked via refsMocks.resolveChannelRef), so this assertion passes vacuously and guards no behavior. Remove it rather than implying the command avoids an extra channel fetch.

Share Feedback • Review Logs

@jacobo-doist

Copy link
Copy Markdown
Contributor Author

@doistbot Fixed two suggestions in separate commits:

  • Repeated group fetches — 16bf097: default-audience.ts now shares one workspace group-list request across name references. resolveGroupRef in src/lib/refs.ts accepts that shared request, preserving its existing matching rules.
  • Sequential resolution — b551f66: default-audience.ts now resolves groups and users concurrently. Both must succeed before the mutation runs.

Left the other suggestions unchanged:

  • Duplicate JSON fields: The requested interface includes both the raw audience fields and resolved names. Keeping both supports script verification.
  • Empty-input tests: The short input table covers whitespace and empty segments in different positions. Both commands should reject them.
  • Flag-position assertions: These remain accurate for the current cases. The combined groups/users case has only one group, so it isn’t missing a second-group check. Rewriting these is optional cleanup.
  • No-extra-fetch assertion: This guards against the handler adding a direct getChannel call. It doesn’t measure requests inside the mocked resolver, but it still protects against an additional handler fetch.

-- replied by Clanker

@doistbot

Copy link
Copy Markdown
Member

I couldn't confidently route this request. Commands supported here are: /review & /chat.

For review, leave a comment @doistbot /review. For chat, leave a comment @doistbot /chat, or ask directly with @doistbot <YOUR QUESTION> when natural-language routing is enabled.

@jacobo-doist

Copy link
Copy Markdown
Contributor Author

@doistbot /review

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR adds default thread audience support to tdc channel create and tdc channel update via --default-groups, --default-users, and --clear-default-audience, with dry-run previews, audience details in channel members list --json, and a bump of @doist/comms-sdk to 3.9.0. No inline issues were flagged; the review confirmed the implementation follows the repo's existing CliError validation, ref-resolution, and output patterns, with the shared group-list fetch and concurrent group/user resolution addressing the earlier performance notes, and the security and reuse passes finding no trust-boundary, exposure, or duplication concerns.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (2)
  • P3 src/commands/channel/update.ts:132: In the name-ref branch, channel resolution and audience resolution run sequentially, summing their network latencies. resolveChannelRef and buildDefaultAudienceArgs are independent once workspaceId is known — resolveChannelRef already asserts the returned channel belongs to workspaceId, so using workspaceId here is equivalent. Run them together instead: const [channel, audience] = await Promise.all([resolveChannelRef(channelRef, workspaceId), hasAudienceRefs ? buildDefaultAudienceArgs(options, workspaceId) : undefined]), then assign defaultAudience from audience when present.
  • P3 src/commands/channel/create.ts:38: Missed concurrency: when both --users and --default-users are passed, resolveUserRefs(options.users, workspaceId) (line 34) and buildDefaultAudienceArgs (which resolves --default-users against the same workspace) run sequentially, so the command pays the sum of both latencies. They're independent — wrap both in a Promise.all since workspaceId is already resolved.

Share Feedback • Review Logs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants