Repository navigation
feat(channel): support default thread audiences - #72
jacobo-doist wants to merge 7 commits into
Conversation
|
@doistbot /review |
doistbot
left a comment
There was a problem hiding this comment.
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)
src/commands/channel/default-audience.ts:46: Resolving each group ref via
resolveGroupRefcauses one fullgetWorkspaceGroups/groups.getGroupscall per name ref, so--default-groups "Design,Backend"fetches the whole group list twice.resolveChannelMemberRefsalready 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).src/commands/channel/members.ts:55: The slim JSON now carries the same audience data twice:
useDefaultRecipients,defaultGroups, anddefaultRecipientsare all already present insidedefaultAudience(asdefaultAudience.useDefaultRecipients,defaultAudience.groups[].id, anddefaultAudience.users[].id). Keeping onlydefaultAudienceremoves the duplicate state and keeps the curated payload smaller.src/commands/channel/default-audience.ts:51: When both
--default-groupsand--default-usersare supplied, the group refs fully resolve beforeresolveUserRefsstarts. The two resolutions are independent, so the command currently pays the sum of their latencies. Run the group and user resolution concurrently (e.g. viaPromise.all) to reduce wall-clock time.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-testvalidateDefaultAudienceOptionsdirectly instead of spinning up the full CLI for each combination.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 ofresolveGroupRefcalls, 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 expectedresolveGroupRefcalls per case.src/commands/channel/members.test.ts:166:
client.channels.getChannelis never called bylistChannelMembers(channel resolution is mocked viarefsMocks.resolveChannelRef), so this assertion passes vacuously and guards no behavior. Remove it rather than implying the command avoids an extra channel fetch.
|
@doistbot Fixed two suggestions in separate commits:
Left the other suggestions unchanged:
-- replied by Clanker |
|
I couldn't confidently route this request. Commands supported here are: For review, leave a comment |
|
@doistbot /review |
doistbot
left a comment
There was a problem hiding this comment.
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)
src/commands/channel/update.ts:132: In the name-ref branch, channel resolution and audience resolution run sequentially, summing their network latencies.
resolveChannelRefandbuildDefaultAudienceArgsare independent onceworkspaceIdis known —resolveChannelRefalready asserts the returned channel belongs toworkspaceId, so usingworkspaceIdhere is equivalent. Run them together instead:const [channel, audience] = await Promise.all([resolveChannelRef(channelRef, workspaceId), hasAudienceRefs ? buildDefaultAudienceArgs(options, workspaceId) : undefined]), then assigndefaultAudiencefromaudiencewhen present.src/commands/channel/create.ts:38: Missed concurrency: when both
--usersand--default-usersare passed,resolveUserRefs(options.users, workspaceId)(line 34) andbuildDefaultAudienceArgs(which resolves--default-usersagainst the same workspace) run sequentially, so the command pays the sum of both latencies. They're independent — wrap both in aPromise.allsinceworkspaceIdis already resolved.
Adds options to set the default audience for new threads in a channel.
Both
tdc channel createandtdc channel updateaccept:--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-runshows the resolved IDs before making changes.channel members list --jsonincludes the default audience with group names and user names and emails.Updates
@doist/comms-sdkto 3.9.0 for the audience request types.