Simplify comment threads to Resolve/Reopen, drop accept/discard - #209
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3684970053
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collapses the four-state pending/todo/discarded/resolved lifecycle to just open/resolved. There's no accept/reject mechanics anymore — a thread is either open or resolved, and either the thread's author or the plan's author can flip it either way. Authors can still delete their own comments, unchanged. - CommentThread: STATUSES/OPEN_STATUSES/CLOSED_STATUSES collapse to open/resolved; accept!/discard! replaced by reopen! - Policy: accept?/discard? removed; resolve?/reopen? unchanged - Web + agent API routes: dropped accept/discard, kept resolve/reopen (agent API's discard endpoint removed entirely, not aliased) - UI: single "Resolve (e)" button replaces Accept/Discard; e is the new keyboard shortcut (r stays reply) - Migration collapses existing pending/todo -> open and discarded -> resolved, and updates the column default - Specs, seeds, and docs (AGENTS.md, agent instructions, marketing copy) updated to match the two-state model
CommentThread::CLOSED_STATUSES collapsed to ["resolved"] only, so the mark_closed_thread_notifications_read migration (which queries that live constant) no longer matches legacy "discarded" rows. Split the test so it asserts what actually happens today instead of asserting stale discarded-row behavior.
accept! used to set resolved_by_user_id on todo threads, so migrating them straight to open left stale resolver provenance on a thread that now reads as unresolved — an open thread would show a resolver even though newly created/reopened open threads never have one.
plan.comment_threads.reload.last had no ORDER BY, so its row order was unspecified — it happened to match insertion order on MySQL but not on Postgres, where an unordered scan returned the older resolved thread instead of the newly created one, failing test-postgres in CI.
The create action still computed status: "todo"/"pending" for new threads, which is invalid now that CommentThread::STATUSES is just open/resolved — every agent-API comment creation 422'd. Drop the status assignment and let the model default (open) apply. Only surfaced by running the full suite rather than filtered files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HamptonMakes
force-pushed
the
hampton/comment-resolve-delete-494f8c
branch
from
September 4, 2026 19:49
595b54d to
81919cb
Compare
An "open" badge on every thread was noise — open is the default, uninteresting state. Only render the status pill once a thread is resolved. Flip resolved-highlight visibility to match: resolved threads now show as a dashed underline in the doc by default instead of vanishing entirely, matching the product's "history is always browsable" pitch. Pressing s now hides resolved threads for a decluttered read, instead of the reverse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HamptonMakes
added a commit
that referenced
this pull request
Sep 16, 2026
…agram-expand-39dcde * origin/main: Simplify comment threads to Resolve/Reopen, drop accept/discard (#209)
HamptonMakes
added a commit
that referenced
this pull request
Sep 16, 2026
#209 landed 40 minutes after this branch's previous commit and collapsed thread statuses to open/resolved. The popover-drift spec added here created its fixture thread with the old "pending", which is valid on this branch and invalid on the merge — so the suite was green locally and red on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HamptonMakes
added a commit
that referenced
this pull request
Sep 16, 2026
* Expand tables into a spreadsheet, and let diagrams be explored Markdown tables used to flow off the page. They're now framed: columns size to their content, long cells wrap, and anything wider than the column scrolls inside its own bordered frame with edge fades that say so. A table big enough to be worth it offers to expand into a spreadsheet takeover — both axes pinned, an arrow-driven cell cursor with a row/column crosshair, a value bar for cells the grid clips, numeric-aware column sorting with a reset, a wrap toggle, and an A1-style address readout. ⌘C copies the focused cell. Sorting works on a clone, so the document's own table — and the comment anchors that depend on its rendered text — never moves. Diagrams get the exploration they were missing. The old lightbox closed on any click, so a drag could never pan; it's replaced by a real pan/zoom surface: drag, scroll or pinch to zoom at the cursor, double-click to toggle fit and close-up, +/-/0/1 and arrows from the keyboard. Labels are legible now too — 16px in the app's own face, thicker strokes, and fonts loaded before Mermaid measures its boxes. Fitting stops before it becomes a thumbnail: inline, a diagram that would shrink past 80% keeps its real size and scrolls; expanded, fit floors at 55% and pans instead. Both features sit on one takeover surface (coplan/expander) and one affordance, with pan/zoom factored out separately — a spreadsheet wants real scrolling, not a transform. Decks opt out of table framing; a slide is a fixed artifact whose typography the deck layout already owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Review fixes: one affordance, honest copy, keyboard after the toolbar - The expand affordance is injected by JS, so Turbo cached it and the controller appended a second one on every back navigation. Marked data-turbo-temporary, with a spec that reproduces the duplicate. - ⌘C flashed "copied" even where there is no clipboard API (plain http); the flash now waits for the write to resolve. - Clicking any toolbar button moved focus off the canvas and the grid frame, which own the keys — so one click on Zoom in or Wrap killed +/-/0/1 and the arrows. Both hand focus straight back. - Phones: the expander is sized by inset rather than 100vw/100dvh (a modal's margin:auto left a sliver of backdrop down one edge), fit floors at 55% instead of opening a 2000px diagram at 15%, and the gesture hint stays — worded for touch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Fix the deck's diagram sizing, popover drift, and the floor's measurement Three real findings from the review bots. The deck sized slide diagrams with `.mermaid-diagram > svg`, and the SVG is now a grandchild under the canvas — so slides lost `max-height: 34cqi` and stage slides lost `width: 100%; height: 44cqi`. Retargeted, and the canvas itself is neutralized on a slide: the document's padded scroll box is wrong on a fixed frame. Decks also opt out of the inline legibility floor entirely, for the same reason they opt out of table framing — the floor pins a pixel width, which on a slide pins the diagram wider than the slide. The deck spec that should have caught this only asserted the diagram was wider than 200px. It now asserts the cqi rules actually resolve to a length, that the diagram fits its slide, and that none of the scroll-box treatment comes along. An open thread popover sat still while the table frame scrolled out from under its mark: `scroll` doesn't bubble, so the window listener never heard the new inner scroller. Captured on the document instead, which also covers the expanded sheet and anything nested later. The legibility floor measured `clientWidth`, which includes the canvas's 32px of padding, while the SVG's `max-width: 100%` resolves against the box inside it — so a diagram near the threshold was called legible and then shrunk past it anyway. No spec for this one: it only shows up within 32px of the threshold, and a spec pinned there would break on any padding change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Use the status main now has after the Resolve/Reopen simplification #209 landed 40 minutes after this branch's previous commit and collapsed thread statuses to open/resolved. The popover-drift spec added here created its fixture thread with the old "pending", which is valid on this branch and invalid on the merge — so the suite was green locally and red on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CommentThreadlifecycle from four states (pending/todo/discarded/resolved, with Accept/Discard/Resolve/Reopen actions) down to two:open⇄resolved. There's no accept/reject mechanics anymore — a thread is either open or resolved.CommentThreadPolicy#resolve?/#reopen?, unchanged logic, just fewer actions).PATCH .../discardendpoint is removed entirely (not aliased to resolve).a)/Discard(d) buttons and shortcuts are replaced by a single Resolve (e) button/shortcut — Gmail-style archive, keepingrfor reply as-is.CommentPolicy#delete?was already scoped to the comment's author and didn't need to change.pending/todo→openanddiscarded→resolved, and updating the column default.Test plan
spec/models/comment_thread_spec.rb,comment_thread_analytics_spec.rb,comment_thread_anchor_spec.rbupdated for the two-state model, including a newreopen!testspec/requests/comment_threads_spec.rb,spec/requests/api/v1/comments_spec.rbupdated — discard endpoint tests removed, added a 404 case for a non-creator/non-plan-author trying to resolvespec/system/comment_ux_spec.rb,spec/system/voice_commenting_spec.rbupdated for the single Resolve button andekeyboard shortcutOPENbadge + single "Resolve (e)" button, resolving clears the anchor highlight, resolved popover showsReopen, clicking it flips the thread back toopen