Conversation
gnapse
force-pushed
the
ernesto/inbox-state-undo
branch
2 times, most recently
from
0c70fc3 to
02b29b4
Compare
scottlovegrove
force-pushed
the
ernesto/inbox-state-undo
branch
from
02b29b4 to
ebe9108
Compare
|
@doistbot /review |
![doistbot-app[bot]](https://avatars.githubusercontent.com/in/2345791?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces a great set of reversible thread inbox commands and updates the done command to share the new bulk mutation flow. The expanded functionality is a solid improvement, though the shared flow inadvertently introduces a breaking change to the JSON output schema for thread done that could impact existing automation. A few adjustments are needed to preserve this backward compatibility, restore built-in CLI argument validation, and optimize redundant API calls during bulk operations.
| .option('--json', 'Output result as JSON') | ||
| .action(markThreadDone) | ||
| withThreadMutationOptions( | ||
| thread.command('done [thread-refs...]').description('Archive thread(s) (mark as done)'), |
There was a problem hiding this comment.
[P2] Degraded argument validation. Using [thread-refs...] makes the arguments optional at the CLI parser level, meaning tw thread done bypasses Commander's native validation and throws a generic runtime error in executeThreadMutation. Use <thread-refs...> to leverage Commander's built-in required argument validation. (Please apply this to reopen, read, unread, and restore commands as well).
| thread.command('done [thread-refs...]').description('Archive thread(s) (mark as done)'), | |
| thread.command('done <thread-refs...>').description('Archive thread(s) (mark as done)'), |
| import { pluralize } from './helpers.js' | ||
|
|
||
| type DoneOptions = MutationOptions | ||
| type ThreadMutationOptions = MutationOptions & { |
There was a problem hiding this comment.
[P2] Redundant type property. MutationOptions already includes json?: boolean and full?: boolean. Redeclaring json?: boolean here is unnecessary and deviates from the repository conventions for reusing options.
| type ThreadMutationOptions = MutationOptions & { | |
| type ThreadMutationOptions = MutationOptions & { | |
| yes?: boolean | |
| } |
| const thread = await client.threads.getThread(threadId) | ||
| await assertChannelIsPublic(thread.channelId, thread.workspaceId) | ||
|
|
||
| const beforeUnreadIds = await getUnreadThreadIds(client, thread.workspaceId) |
There was a problem hiding this comment.
[P2] Redundant API calls during bulk operations. getUnreadThreadIds fetches all unread threads for a workspace. In a bulk operation affecting multiple threads in the same workspace, this will repeatedly call the API. Consider caching the unread thread IDs per workspace inside executeThreadMutation and passing it to applyThreadMutation, or finding a more efficient way to look up the unread state.
|
|
||
| if (!dryRun && actions.length > 0) { | ||
| const refreshed = await client.threads.getThread(thread.id) | ||
| const afterUnreadIds = await getUnreadThreadIds(client, refreshed.workspaceId) |
There was a problem hiding this comment.
[P2] Redundant API call for unread state after mutation. Calling getUnreadThreadIds immediately after mutating the thread is a performance hit and could suffer from eventual consistency delays. Since the expected unread state is known after the mutation, you can rely on projectAfterState to determine isUnread without an extra network call.
| const afterUnreadIds = await getUnreadThreadIds(client, refreshed.workspaceId) | |
| const afterUnread = projectAfterState(before, mutation).isUnread | |
| return { | |
| ref, | |
| status: 'changed', | |
| actions, | |
| before, | |
| after: toSnapshot(refreshed, afterUnread), | |
| } |
| if (options.json) { | ||
| console.log(formatJson({ id: threadId, isArchived: true })) | ||
| return | ||
| const output = { |
There was a problem hiding this comment.
[P1] This changes the existing tw thread done <ref> --json payload from a flat status object into a batch envelope. Any automation that currently reads top-level id / isArchived fields will break on upgrade. Preserve the old single-ref schema for done, or gate the richer results payload behind a separate flag so existing scripts keep working.
2 participants
Summary
This PR adds reversible thread inbox-state operations and makes
thread donesafely undoable.New Commands
tw thread reopen [thread-refs...]tw thread read [thread-refs...]tw thread unread [thread-refs...]tw thread restore [thread-refs...] --unreadtw thread donenow uses the same mutation flow as the commands above.Mutation Behavior
tw thread read 123 456).--yes,--dry-run, and--json.reopenon open threads,unreadon unread threads).--jsonincludesbefore/afterfields:id,title,isArchived,inInbox,isUnread,closed,lastUpdated.Docs and Skill Content
README.mdwith examples and migration note:thread doneis reversible viathread reopen.src/lib/skills/content.tsto reflect new thread commands and options.Tests