Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. What this changesAdds permanent channel deletion for human workspace owners through the browser, API, SDK, and CLI, with content previews, file cleanup, viewer notices, and faster database cascades. Example: An owner removes #old-launch containing two messages
Review scores
ProductKind: Feature · Worth it: Needs a maintainer decision Merge readiness⛔ Blocked before merge - 5 items remain This PR provides useful functionality absent from current main and merits continued review; the two previously reported client defects remain unresolved. Priority: P2 Decision needed
Before merge
Findings
Tests
Agent review detailsHow this fits togetherClickClack stores workspace conversations in SQLite or PostgreSQL and serves browser and API clients. Channel deletion checks ownership, removes content transactionally, queues file cleanup, and notifies connected clients. flowchart TD
A[Owner uses browser or CLI] --> B[Channel deletion API]
B --> C[Check human owner and protected channels]
C --> D[Database removes channel content]
D --> E[Durable file cleanup queue]
D --> F[Workspace deletion event]
F --> G[Clients remove channel and move viewers]
Technical reviewBest possible solution: Reuse transactional cascades and durable cleanup, make deletion terminal for client reads, and validate the owner-approved operation on existing databases. Do we have a high-confidence way to reproduce the issue? Yes for the introduced races: hold a channel-list or search response captured before deletion, process channel.deleted, then release it. Source shows that the response remains admissible; no reviewer-side failing run was executed. Is this the best way to solve the issue? Unclear overall: the existing transaction and cleanup owners fit the operation, but terminal client-read handling, populated upgrade verification, and product acceptance remain incomplete. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1169fb099659. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (14 earlier review cycles; latest 8 shown)
Reviewed October 8, 2026, 11:40 PM ET / October 9, 2026, 03:40 UTC (Revision 15). |
b6d6079 to
aa459dd
Compare
Owners delete a channel from Channel settings, the API, or the CLI after reviewing what will be removed and typing the channel name. The channel's messages, replies, reactions, pins, topics, read state, and exclusive uploads go in one transaction; a workspace-scoped channel.deleted event moves viewers out. The last channel and the Guests workspace's provisioned channels cannot be deleted. The CLI deletes only a channel named with --channel on its command line, never CLICKCLACK_CHANNEL or the saved default. On SQLite, cascading many message deletions scanned both the messages table and the FTS index once per row. Child-key indexes and a message-to-search-row map make a 10k-message channel in a 120k-message workspace delete in 0.3 s instead of 6.6 minutes, which also speeds up workspace deletion. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
aa459dd to
c6d1ef8
Compare
Maintainer edits
MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.
What Problem This Solves
Workspace owners can archive a channel but cannot remove a channel they no longer
want, together with its messages and files.
User Impact
User impact: owners can delete a channel from Channel settings → Delete
channel..., from the API, or with
clickclack channels delete. The dialogshows what will be removed (messages, thread replies, pins, topics, files and
their size), offers to archive instead, and enables deletion only after the
channel name is typed. Deletion is permanent. People viewing the channel are
moved to their fallback conversation with a notice. The server refuses to
delete a workspace's last channel and the Guests workspace's provisioned
channels. On SQLite, two new migrations make deleting many messages fast: a
10k-message channel in a 120k-message workspace now deletes in 0.3 s instead of
6.6 minutes. Existing workspace deletion benefits the same way.
Why This Change Was Made
channel, messages, replies, reactions, pins, channel topics, read pointers,
and notification settings go together. Uploads attached only to that channel
are removed and their objects go through the durable cleanup queue already
used for workspace deletion; uploads still used elsewhere and the workspace
icon are kept. The audit log records the channel and the removed counts.
messagesto find replies and quotes (parent_message_idandquoted_message_idhad no index), and one of the FTS5 search index, becausethe delete trigger matched the unindexed
messages_fts.message_id. Deletinga channel therefore grew with channel size × workspace size. New partial
indexes cover the child keys on both stores, and
message_search_rowsrecordseach message's FTS rowid so the search triggers update rows by rowid. Both
are needed; either alone leaves most of the cost.
channel.deletedevent tells clients to leave the channel.Earlier events for the channel stay in the log, so replay cursors remain valid;
realtime delivery already skips events whose channel no longer exists.
FOR NO KEY UPDATE) beforeit counts or selects anything, in the same workspace-first order as workspace
deletion and updates. Competing deletions cannot both pass the last-channel
check, and the workspace icon cannot move onto an upload being removed; event
appends and inserts, which take
KEY SHARE, are not blocked. Candidate uploadsare then locked
FOR UPDATEand re-checked, so an attachment elsewhere eithercommits first (and the upload is kept) or waits and fails its foreign key.
403;blocked deletions return
409with ablockercode the web app explains.clickclack channels deletedeletes only a channel named with--channelonits own command line (global or subcommand form).
CLICKCLACK_CHANNELand thesaved default channel pick the channel for everyday commands, so they never
choose what gets deleted. Without
--yesthe command prints the counts andexits.
The new migrations (
sqlite/0043,sqlite/0044,postgres/0036) share numberswith the agent question migrations in #258. Migrations apply by file name, so both
work together; whichever PR lands second can renumber to keep the sequence tidy.
Evidence
The CLI with a default channel in the environment, real server and CLI from
the same build:
Previous head `aa459dd5`:
channels delete --yesdeleted#launchfromCLICKCLACK_CHANNELThis head: the default channel is ignored; the named channel is deleted after the preview
Tests:
channeldeletiontest): thechannel's messages, topics, search results, and exclusive upload are removed
while other channels, shared uploads, and the workspace icon survive; the
deletion counts match the preview; a replay cursor pointing at one of the
channel's old events still replays forward to
channel.deleted; guard railsfor the last channel, provisioned Guests channels, archived channels, and
non-owners.
TestCascadeChildKeysUseIndexes(SQLite query plan) andTestCascadeChildKeyIndexesExist(Postgres).TestDeleteChannelSerializesLastChannelDecisionsruns two deletions of aworkspace's last two channels (one succeeds, one gets
last_channel), andTestDeleteChannelKeepsUploadsAttachedDuringDeletionattaches the channel'sonly upload elsewhere while the deletion is paused after choosing it (the
attachment either keeps the upload or fails). Both pass three times in a row;
against the previous revision they fail with
deleted=2 blocked=0 remaining=0and
attachment succeeded but kept 0 rows.TestSearchTriggersFindRowsThroughTheRecordedRowid: upgrades a database withan existing message, detaches the search rows from their message IDs, then
edits one message and deletes the other's channel; both rows are still
replaced or removed and search returns the edited message. With the old
message_idtriggers it fails withdetached rows = 2.TestChannelDeletionHTTP:403for members and bot tokens,404forunknown or already deleted channels,
204for the owner, the live andreplayed
channel.deletedevent on a member's socket, the audit entry, andthe
last_channelblocker with409.TestChannelsDeleteRequiresExplicitConfirmation: the CLI prints the previewand refuses without
--yes.TestChannelsDeleteIgnoresDefaultChannelsruns the CLI entry point withCLICKCLACK_CHANNEL, then with a saved default channel:channels delete --yessends no DELETE, while
--channelin either flag position deletes. Without thefix it fails with
CLICKCLACK_CHANNEL chose the channel to delete.tests/e2e/channel-deletion.spec.ts: an owner deletes after reviewing thepreview and typing the name; a member watching the channel is moved out with
a notice; the last channel cannot be deleted.
apps/web/src/lib/channel-deletion.test.ts: name confirmation, blockermessages, and the notice text.
pnpm fmt:check,pnpm lint,pnpm typecheck,pnpm -r typecheck, web unit tests,pnpm docs:site, andgo test ./...with
CLICKCLACK_POSTGRES_TEST_DSN, deadcode, and the embedded build iscurrent and repeatable (coverage gate 86.8%). On this machine these tests also
fail on unchanged
main:TestHTTPBodyDeadlineStillBoundsStalledRequestBodies,uploadstore TestR2HeaderNetworkLifecycle/progressing_PUT, and intermittentlyTestHTTPErrorPathsAndSPA(1 of 4 runs onmain), andTestHTTPSlashCommandRequiresChannelWriteAuthorityBeforeCallback(1 of 5 runson
main, 5 of 5 pass on this branch). The first CI run's Playwright jobfailed once in
chat.spec.ts › clicking the active conversation does not refetch its messages; locally that test passes 3 of 3 alone and the wholechat.spec.tspluschannel-deletion.spec.tspass 42 of 42 with two workers.Deleting a channel on SQLite (Python
sqlite33.45 applying the realmigrations; 120,000 messages in 12 channels; the deleted channel has 7,000
roots, 2,500 thread replies, and 500 quote replies):
mainApplying this PR's migrations to that populated database took 0.14 s. A
smaller run (30,000 messages, 3,000 deleted) shows the two fixes are
independent: 27.7 s on
main, 11.5 s with only the search row map, 17.7 s withonly the indexes, and 0.10 s with both.
AI-assisted: prepared with Claude Code; I reviewed the change and the evidence.
🤖 Generated with Claude Code