Move threads into Set Aside before grouping them - #409
Conversation
hey set-aside group create and group add promised to move a thread into Set Aside when it was elsewhere, and relied on HEY's group routes to do it. Those routes relocate with a plain box write. "Bubbled up" is a value of a posting's seen state rather than a flag of its own, and only HEY's move marks a thread seen, so a bubbled-up thread gathered straight out of the Imbox arrived in Set Aside still bubbled up, which the web app draws as a Bubble Up row inside the Set Aside stack. Both commands now move the threads through HEY's move first, the way hey move --to set-aside does, and then group them. A thread already in Set Aside is unchanged by the move. The unit tests model both relocation paths and assert the bubble is cleared; the smoke test does the same against a live server.
There was a problem hiding this comment.
🟡 Changes recommended
An invalid destination group can fail only after the threads have already been moved and marked seen.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves threads through HEY’s Set Aside operation before grouping, clearing Bubble Up state.
Changes:
- Adds pre-group moves for create/add operations.
- Adds unit and smoke coverage.
- Updates CLI documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/cmd/set_aside.go |
Moves threads before grouping. |
internal/cmd/set_aside_test.go |
Tests request order and Bubble Up clearing. |
tests/smoke/set_aside_test.go |
Verifies behavior against HEY. |
docs/cli.md |
Documents grouping semantics. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review: a positive but missing --to group was rejected by the group route only after the threads had already been moved into Set Aside and marked seen. The group route validates the group and relocates the thread itself, so it now runs first and the move follows it to mark the threads seen. A refused group fails before any thread is changed, and a thread already in Set Aside keeps its group because its box does not change. No extra request is needed.
jeremy
left a comment
jeremy
left a comment
There was a problem hiding this comment.
Read this against haystack and the SDK rather than taking the mechanism on faith. Verdict: ready to merge. Details, then one non-blocking nit.
The mechanism claim holds. Posting::Observable stores enum :seen, { seen: 1, unseen: 0, bubbled_up: -1 }, so "bubbled up" really is a value of seen. Box#move_in (the POST /postings/moves path, via Posting#move_to) does update! box: self, seen: true, observed_at: Time.now, which overwrites -1. Posting#move_to_box_group (both POST /boxes/:id/groups via Box::Group.create_with_postings and POST /postings/box_groups) does update!(box: box_group.box, observed_at: Time.current) and never touches seen. So the group routes alone leave a bubbled-up thread at seen: -1 inside Set Aside, exactly as the card describes, and the added move is the right repair. Posting#move_to is guarded by postable.movable_to?, but for a Topic that only refuses Reply Later, so a move into Set Aside never silently no-ops.
The reorder is correct, and the "already in Set Aside" case is safe. For a thread already in Set Aside, move_in writes the same box_id, so will_save_change_to_box_id? is false and remove_from_box_group_if_moved leaves the Box::GroupPosting alone; the group survives. observed_at gets bumped, but move_to_box_group bumps it too, so ordering within Set Aside is no different from before. An unseen (non-bubbled) Imbox thread now arrives seen, which is what hey move --to set-aside does and what the docs and agent notes now say.
Refused --to group changes nothing. Postings::BoxGroupsController#create runs @box.groups.find(params[:box_group_id]) before iterating postings, so a bad group is a 404 with no writes; set_aside.go:405 returns on that error before setAsideThreads at :408 runs. TestSetAsideGroupAddToARefusedGroupMovesNothing pins the request sequence, and I checked it still passes with the move removed, which is right: it guards the ordering, not the move.
Partial state. The remaining failure window is "group route succeeded, move failed" (set_aside.go:353 and :408). That leaves the threads grouped in Set Aside but still bubbled up, i.e. the pre-fix state, and for create the error hides the new group's id. That is repairable with hey move --to set-aside <ids> or a group add, and it is strictly better than the reverse ordering's failure (threads moved and marked seen, no group). No dangling empty group is possible from this path: create_with_postings attaches postings inside the same request. bulkAction in the SDK sends one request per call with no chunking, so there is no mid-batch partial either.
Tests. I removed both setAsideThreads calls locally: TestSetAsideGroupCreateClearsTheBubbleOnAnImboxThread and TestSetAsideGroupAddClearsTheBubbleOnAnImboxThread both fail with posting is still bubbled up in Set Aside: {box:3 seen:-1 group:44}, so they catch the bug rather than the request shape. The stateful fake in setAsideStateServer models the two haystack paths faithfully. The smoke test asserts real state: hey set-aside group view embeds generated.Posting, which carries seen and bubbled_up straight from postings/_object.jbuilder, and with the bug present the thread arrives bubbled_up: true, seen: false and got.BubbledUp || !got.Seen fails; if the fields were ever dropped from the payload it would fail loudly rather than pass. bulk_bubble_up_now is synchronous server-side, so the setup is not racy. tests/smoke is its own module, so CI's go test ./... from the root does not pick it up; it only runs under make test-smoke against a dev server. Cleanup order (LIFO: group delete first, then the two trashes) is right.
Nit, non-blocking. TestSetAsideGroupCreate (set_aside_test.go:281) and TestSetAsideGroupAdd (:313) read recorded.bodies[1] right after the request-sequence check. If the move ever regresses out, that indexes past the end and panics, which aborts the rest of the package's tests instead of reporting one clean failure (I hit exactly this when I removed the move). A length guard, or asserting the sequence with t.Fatalf, would keep the failure legible. Fine to leave for a follow-up.
Docs and agent notes read correctly; no changes to group remove / group delete / hey move behaviour.
TestSetAsideGroupCreate and TestSetAsideGroupAdd index recorded.bodies[1] right after checking the request sequence. If the move ever regresses out, that indexes past the end and panics, aborting the rest of the package's tests instead of reporting one failure. Asserting the sequence with Fatalf stops the test at the legible message.
|
Fixed — the nit from the readiness review: |
Summary
hey set-aside group createandhey set-aside group addpromised to move a thread into Set Aside when it was elsewhere, and relied on HEY's group routes to do it. Those routes relocate with a plain box write. In HEY, "bubbled up" is a value of a posting's seen state rather than a flag of its own, and only HEY's move marks a thread seen, so a bubbled-up thread gathered straight out of the Imbox arrived in Set Aside still bubbled up. The web app then draws it as a Bubble Up row inside the Set Aside stack.Both commands now follow the group route with HEY's move, the same call
hey move --to set-asidemakes, which marks the threads seen and clears the bubble. The group route runs first so that a group HEY refuses fails before any thread is changed; a thread already in Set Aside keeps its group, since its box does not change. The cost is one extra request per command.Changes
internal/cmd/set_aside.go:setAsideThreadsmoves the ids into Set Aside afterCreateGroupandAddToBoxGroup; agent notes for both commands say threads are moved and marked seen.internal/cmd/set_aside_test.go: a stateful fake models both relocation paths as HEY behaves; the two bubble tests fail without the fix. A third test checks that a refused group moves nothing. The existing request-sequence tests expect the move after the group call.tests/smoke/set_aside_test.go:TestSetAsideGroupingClearsBubbleUpbubbles a thread up, groups it withcreate, adds a second bubbled-up thread withadd, and checks both arrive seen and not bubbled up. Passes against a dev HEY server.docs/cli.md: says both commands complete the move the wayhey move --to set-asidedoes, and why.Confirmed live against a dev HEY server on current code before the fix.
Basecamp card: https://app.basecamp.com/2914079/buckets/48521764/card_tables/cards/10279322895