diff --git a/AGENTS.md b/AGENTS.md index b2b0bd7f..c6ca29ac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,14 +130,13 @@ it is. The comment system is central to the collaboration workflow. Domain experts leave inline feedback anchored to specific text in the plan, and the plan author triages that feedback. ### Thread lifecycle -- **Reviewer comments** start as `pending` (awaiting author triage) -- **Author's own comments** start as `todo` (self-assigned work items) -- Author triages pending feedback: **Accept** (`pending → todo`) or **Discard** (`pending → discarded`) -- Author marks completed work: **Resolve** (`todo → resolved`) -- Resolved/discarded threads can be **Reopened** back to `pending` +- Every thread starts `open`, regardless of who created it +- **Resolve** (`open → resolved`) is the only closing action — no accept/reject mechanics, just done or not done +- Resolved threads can be **Reopened** back to `open` +- Either the thread creator or the plan author can resolve/reopen (`CommentThreadPolicy#resolve?`/`#reopen?`) ### Notifications follow the thread -A closed thread (`resolved`/`discarded`) carries no unread inbox rows — its +A closed (`resolved`) thread carries no unread inbox rows — its highlight is hidden in the doc view, so a row pointing at it would send the reader to an apparently empty page. - Closing a thread sweeps its unread notifications read (`CommentThread` @@ -157,7 +156,7 @@ reader to an apparently empty page. (`NotificationsController#mark_plan_read`, same service) ### Inline review UI -- **Highlights**: anchored text is wrapped in `` elements — amber for `pending`, blue for `todo`, unstyled for `resolved` +- **Highlights**: anchored text is wrapped in `` elements — amber for `open`, unstyled for `resolved` - **Margin dots**: colored indicators in the left margin aligned to each highlight's vertical position - **Thread popovers**: native HTML Popover API (`popover="auto"`) showing the comment thread, reply form, and action buttons; positioned relative to the anchor and tracked on scroll - **Comment toolbar**: fixed bottom bar showing open thread count, j/k navigation, and a "Show resolved" toggle @@ -165,8 +164,7 @@ reader to an apparently empty page. ### Keyboard shortcuts - `j` / `k` — navigate between open threads (scrolls to highlight, opens popover) - `r` — focus the reply textarea in the current popover -- `a` — accept the current pending thread -- `d` — discard the current pending thread +- `e` — resolve the current open thread - `Enter` — submit reply; `Shift+Enter` — newline - Push-to-talk (hold to dictate a comment) is a per-user setting — `Ctrl+Space` by default, or Shift / Option / off (`CoPlan::User::VOICE_HOTKEYS`, `voice_controller.js`). A bare modifier has to be held past a delay to tell talking from typing; a chord records from the press. diff --git a/db/migrate/20260828120001_simplify_comment_thread_statuses.co_plan.rb b/db/migrate/20260828120001_simplify_comment_thread_statuses.co_plan.rb new file mode 100644 index 00000000..8dab3c61 --- /dev/null +++ b/db/migrate/20260828120001_simplify_comment_thread_statuses.co_plan.rb @@ -0,0 +1,25 @@ +class SimplifyCommentThreadStatuses < ActiveRecord::Migration[8.0] + def up + # Collapse the accept/reject mechanics into a plain open/resolved toggle: + # pending and todo (both "not yet resolved") become open; discarded + # (a rejection outcome) is treated the same as resolved (a closed thread). + # todo threads carry a resolved_by_user_id from accept! — clear it so an + # open thread never disagrees with newly created/reopened ones, which + # have no resolver. + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'open', resolved_by_user_id = NULL WHERE status IN ('pending', 'todo') + SQL + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'resolved' WHERE status = 'discarded' + SQL + change_column_default :coplan_comment_threads, :status, "open" + end + + def down + change_column_default :coplan_comment_threads, :status, "pending" + # Lossy: todo/discarded can't be distinguished from open/resolved after up. + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'pending' WHERE status = 'open' + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index 630e7f21..f0db07e2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_27_134208) do +ActiveRecord::Schema[8.1].define(version: 2026_08_28_120001) do create_table "active_admin_comments", id: { type: :string, limit: 36 }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "author_id" t.string "author_type" @@ -119,7 +119,7 @@ t.string "plan_version_id", limit: 36, null: false t.string "resolved_by_user_id", limit: 36 t.integer "start_line" - t.string "status", default: "pending", null: false + t.string "status", default: "open", null: false t.datetime "updated_at", null: false t.index ["addressed_in_plan_version_id"], name: "fk_rails_e7003e0df7" t.index ["created_by_user_id"], name: "fk_rails_88fb5e06ca" diff --git a/db/seeds/development.rb b/db/seeds/development.rb index 43f5951b..ab245c8f 100644 --- a/db/seeds/development.rb +++ b/db/seeds/development.rb @@ -807,11 +807,10 @@ def seed_comment_threads(users, plans, token) body: "The comparison writes through to the shared tier, right? Otherwise the dark-read never warms it and the ramp threshold lies." ) - # Accepted into the author's to-do list. + # Still open — nobody's resolved it yet. seed_thread( plan: showcase, user: users.fetch("mateo"), anchor: "no per-key TTL tuning", - accepted_by: priya, body: "Add one sentence on what happens when the version-stamp publish itself fails — that's the first question ops will ask." ) @@ -830,14 +829,14 @@ def seed_comment_threads(users, plans, token) end def seed_thread(plan:, user:, body:, anchor: nil, author_type: "human", - agent_name: nil, api_token: nil, resolved_by: nil, accepted_by: nil) + agent_name: nil, api_token: nil, resolved_by: nil) return if seeded_thread?(plan, body) thread = plan.comment_threads.new( plan_version: plan.current_plan_version, created_by_user: user, anchor_text: anchor, - status: "pending" + status: "open" ) # Anchors resolve against current content; if a local edit removed the # anchored sentence, skip the fixture rather than fail the whole seed. @@ -853,7 +852,6 @@ def seed_thread(plan:, user:, body:, anchor: nil, author_type: "human", agent_name: agent_name, api_token_id: api_token&.id ) - thread.accept!(accepted_by) if accepted_by thread.resolve!(resolved_by) if resolved_by thread end diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index 21052f47..e54ed81c 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -1202,9 +1202,7 @@ del.agent-flash { flex-shrink: 0; opacity: 0.8; } -.badge--pending { background: var(--color-status-considering-bg); color: var(--color-status-considering); } -.badge--todo { background: var(--color-status-developing-bg); color: var(--color-status-developing); } -.badge--discarded { background: var(--color-status-abandoned-bg); color: var(--color-status-abandoned); } +.badge--open { background: var(--color-status-considering-bg); color: var(--color-status-considering); } .badge--resolved { background: var(--color-status-live-bg); color: var(--color-status-live); } .badge--success { background: var(--color-success-soft); color: var(--color-success); } .badge--warning { background: var(--color-warning-soft); color: var(--color-warning); } @@ -2395,37 +2393,19 @@ img.avatar { background: var(--color-highlight-open-hover-bg); } -.anchor-highlight--pending { - background: var(--color-highlight-pending-bg); - border-bottom: 2px solid var(--color-highlight-pending-border); -} - -.anchor-highlight--pending:hover { - background: var(--color-highlight-pending-hover-bg); -} - -.anchor-highlight--todo { - background: var(--color-highlight-todo-bg); - border-bottom: 2px solid var(--color-highlight-todo-border); -} - -.anchor-highlight--todo:hover { - background: var(--color-highlight-todo-hover-bg); -} - .anchor-highlight--resolved { background: none; - border-bottom: none; - cursor: default; - pointer-events: none; -} - -.plan-layout--show-resolved .anchor-highlight--resolved { border-bottom: 1px dashed var(--color-text-muted); cursor: pointer; pointer-events: auto; } +.plan-layout--hide-resolved .anchor-highlight--resolved { + border-bottom: none; + cursor: default; + pointer-events: none; +} + .anchor-highlight--active { background: var(--color-highlight-active-bg); border-bottom: 2px solid var(--color-primary); @@ -3022,12 +3002,10 @@ img.avatar { current_user, so these are rendered unconditionally and hidden by default. The coplan--viewer-role controller tags each thread with the viewer's role (.viewer-is-plan-author / .viewer-is-thread-author) to reveal them. */ -.comment-actions--plan-author, .comment-actions--owner { display: none; } -.viewer-is-plan-author .comment-actions--plan-author, .viewer-is-plan-author .comment-actions--owner, .viewer-is-thread-author .comment-actions--owner { display: flex; @@ -3152,14 +3130,10 @@ img.avatar { line-height: 1; } -.content-nav__badge--pending { +.content-nav__badge--open { background: var(--color-status-considering); } -.content-nav__badge--todo { - background: var(--color-status-developing); -} - /* Toggle button when sidebar is hidden — shown outside the sidebar */ .content-nav-show-btn { position: sticky; diff --git a/engine/app/controllers/coplan/api/v1/comments_controller.rb b/engine/app/controllers/coplan/api/v1/comments_controller.rb index 7baffc88..19731a84 100644 --- a/engine/app/controllers/coplan/api/v1/comments_controller.rb +++ b/engine/app/controllers/coplan/api/v1/comments_controller.rb @@ -19,19 +19,13 @@ def show end def create - # Same initial-status rule as the web flow: the plan author's own - # comments start as "todo" (self-assigned), everyone else's as - # "pending" (awaiting author triage). - initial_status = current_user&.id == @plan.created_by_user_id ? "todo" : "pending" - thread = @plan.comment_threads.new( plan_version: @plan.current_plan_version, anchor_text: params[:anchor_text].presence, anchor_occurrence: params[:anchor_occurrence]&.to_i, start_line: params[:start_line].presence, end_line: params[:end_line].presence, - created_by_user: current_user, - status: initial_status + created_by_user: current_user ) # Atomic, matching the web flow: a thread whose first comment @@ -92,27 +86,6 @@ def resolve render json: { thread_id: thread.id, status: thread.status } end - def discard - thread = @plan.comment_threads.find_by(id: params[:id]) - unless thread - render json: { error: "Comment thread not found" }, status: :not_found - return - end - - policy = CommentThreadPolicy.new(current_user, thread) - unless policy.discard? - render json: { error: "Not authorized" }, status: :forbidden - return - end - - thread.discard!(current_user) - CreateNotificationsJob.perform_later(comment_thread_id: thread.id, actor_id: current_user.id, reason: "status_change", - actor_api_token_id: @api_token&.id) - broadcast_thread_update(thread) - - render json: { thread_id: thread.id, status: thread.status } - end - def destroy # Scope the lookup to this plan's comments so an ID from another # plan returns 404 rather than being acted on. (The policy also diff --git a/engine/app/controllers/coplan/comment_threads_controller.rb b/engine/app/controllers/coplan/comment_threads_controller.rb index 638ebf52..21eeca6d 100644 --- a/engine/app/controllers/coplan/comment_threads_controller.rb +++ b/engine/app/controllers/coplan/comment_threads_controller.rb @@ -3,15 +3,11 @@ class CommentThreadsController < ApplicationController include ActionView::RecordIdentifier before_action :set_plan - before_action :set_thread, only: [ :resolve, :accept, :discard, :reopen ] + before_action :set_thread, only: [ :resolve, :reopen ] def create authorize!(@plan, :show?) - # Author's own comments start as "todo" (self-assigned work item); - # non-author comments start as "pending" (awaiting author triage). - initial_status = current_user.id == @plan.created_by_user_id ? "todo" : "pending" - thread_params = params.expect( comment_thread: [ :anchor_text, :anchor_context, :anchor_occurrence, :start_line, :end_line, :body_markdown ] @@ -23,8 +19,7 @@ def create anchor_occurrence: thread_params[:anchor_occurrence].presence&.to_i, start_line: thread_params[:start_line].presence, end_line: thread_params[:end_line].presence, - created_by_user: current_user, - status: initial_status + created_by_user: current_user ) # Atomic: a thread without its first comment is an empty orphan whose @@ -78,25 +73,9 @@ def resolve respond_with_stream_or_redirect("Thread resolved.", streams: [ stream ]) end - def accept - authorize!(@thread, :accept?) - @thread.accept!(current_user) - CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") - stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread accepted.", streams: [ stream ]) - end - - def discard - authorize!(@thread, :discard?) - @thread.discard!(current_user) - CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") - stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread discarded.", streams: [ stream ]) - end - def reopen authorize!(@thread, :reopen?) - @thread.update!(status: "pending", resolved_by_user: nil) + @thread.reopen!(current_user) CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") stream = broadcast_thread_replace(@thread) respond_with_stream_or_redirect("Thread reopened.", streams: [ stream ]) diff --git a/engine/app/javascript/controllers/coplan/comment_nav_controller.js b/engine/app/javascript/controllers/coplan/comment_nav_controller.js index f8b6593b..b2318e36 100644 --- a/engine/app/javascript/controllers/coplan/comment_nav_controller.js +++ b/engine/app/javascript/controllers/coplan/comment_nav_controller.js @@ -39,13 +39,9 @@ export default class extends Controller { event.preventDefault() this.focusReply() break - case "a": + case "e": event.preventDefault() - this.acceptCurrent() - break - case "d": - event.preventDefault() - this.discardCurrent() + this.resolveCurrent() break case "s": event.preventDefault() @@ -201,12 +197,8 @@ export default class extends Controller { } } - acceptCurrent() { - this.submitPopoverAction("accept") - } - - discardCurrent() { - this.submitPopoverAction("discard") + resolveCurrent() { + this.submitPopoverAction("resolve") } submitPopoverAction(action) { @@ -221,11 +213,6 @@ export default class extends Controller { this.currentIndex = 0 } - // For accept (pending→todo), the thread stays open so we need to - // explicitly advance. For discard, the thread leaves openHighlights - // and the current index naturally points to the next one. - const shouldAdvance = action === "accept" - // Watch for the broadcast DOM update that replaces the thread data, // then advance to the next thread once the highlights have changed. // One pending advance at a time, with a timeout so a failed submit @@ -236,7 +223,7 @@ export default class extends Controller { this.cancelPendingAdvance() this.advanceObserver = new MutationObserver(() => { this.cancelPendingAdvance() - this.advanceAfterAction(shouldAdvance) + this.advanceAfterAction() }) this.advanceObserver.observe(threadsContainer, { childList: true, subtree: true }) this.advanceTimeout = setTimeout(() => this.cancelPendingAdvance(), 5000) @@ -252,15 +239,13 @@ export default class extends Controller { this.advanceTimeout = null } - advanceAfterAction(shouldAdvance) { + advanceAfterAction() { const highlights = this.openHighlights if (highlights.length === 0) { this.currentIndex = -1 return } - if (shouldAdvance) { - this.currentIndex = (this.currentIndex + 1) % highlights.length - } else if (this.currentIndex >= highlights.length) { + if (this.currentIndex >= highlights.length) { this.currentIndex = 0 } this.navigateTo(highlights[this.currentIndex]) @@ -275,12 +260,13 @@ export default class extends Controller { } } - // Keyboard "s": show/hide resolved-thread highlights (the visible - // toolbar checkbox is gone — this is deliberately a power-user toggle). + // Keyboard "s": resolved threads show as a dashed underline by default — + // nothing about a plan's history disappears — so this hides them instead, + // for a decluttered read of only what's still open. toggleResolved() { const planLayout = document.querySelector(".plan-layout") if (!planLayout) return - planLayout.classList.toggle("plan-layout--show-resolved") + planLayout.classList.toggle("plan-layout--hide-resolved") } } diff --git a/engine/app/javascript/controllers/coplan/content_nav_controller.js b/engine/app/javascript/controllers/coplan/content_nav_controller.js index 77379dac..50d3a4bc 100644 --- a/engine/app/javascript/controllers/coplan/content_nav_controller.js +++ b/engine/app/javascript/controllers/coplan/content_nav_controller.js @@ -307,14 +307,7 @@ export default class extends Controller { // crosses .markdown-rendered blocks. this._headings.forEach((heading, index) => { const nextHeading = this._headings[index + 1] - const threads = this.collectThreadsBetween(heading, nextHeading, this.contentTarget) - - let pendingCount = 0 - let todoCount = 0 - threads.forEach(status => { - if (status === "pending") pendingCount++ - else if (status === "todo") todoCount++ - }) + const count = this.countOpenThreadsBetween(heading, nextHeading, this.contentTarget) const item = this._itemsById?.get(heading.id) if (!item) return @@ -322,20 +315,17 @@ export default class extends Controller { const existing = item.querySelector(".content-nav__badge") if (existing) existing.remove() - const total = pendingCount + todoCount - if (total > 0) { + if (count > 0) { const badge = document.createElement("span") - const badgeType = pendingCount > 0 ? "pending" : "todo" - badge.className = `content-nav__badge content-nav__badge--${badgeType}` - badge.textContent = total + badge.className = "content-nav__badge content-nav__badge--open" + badge.textContent = count item.querySelector(".content-nav__link").appendChild(badge) } }) } - collectThreadsBetween(startHeading, endHeading, container) { + countOpenThreadsBetween(startHeading, endHeading, container) { const seen = new Set() - const threads = [] let collecting = false const walker = document.createTreeWalker(container, NodeFilter.SHOW_ELEMENT, null) @@ -349,14 +339,10 @@ export default class extends Controller { if (collecting && node.tagName === "MARK" && node.classList.contains("anchor-highlight--open")) { const threadId = node.dataset.threadId - if (threadId && !seen.has(threadId)) { - seen.add(threadId) - const status = node.classList.contains("anchor-highlight--pending") ? "pending" : "todo" - threads.push(status) - } + if (threadId) seen.add(threadId) } } - return threads + return seen.size } } diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js index 19236d96..32158b38 100644 --- a/engine/app/javascript/controllers/coplan/text_selection_controller.js +++ b/engine/app/javascript/controllers/coplan/text_selection_controller.js @@ -638,14 +638,13 @@ export default class extends Controller { threads.forEach(thread => { const anchor = thread.dataset.anchorText const occurrence = thread.dataset.anchorOccurrence - const status = thread.dataset.threadStatus || "pending" + const status = thread.dataset.threadStatus || "open" const threadId = thread.id if (anchor && anchor.length > 0) { - const isOpen = status === "pending" || status === "todo" + const isOpen = status === "open" const statusClass = isOpen ? "anchor-highlight--open" : "anchor-highlight--resolved" - const specificClass = isOpen ? `anchor-highlight--${status}` : "" - const classes = `anchor-highlight ${statusClass} ${specificClass}`.trim() + const classes = `anchor-highlight ${statusClass}`.trim() const marks = this.findAndHighlightAll(anchor, occurrence, classes) if (marks.length > 0 && threadId) { diff --git a/engine/app/models/coplan/comment_thread.rb b/engine/app/models/coplan/comment_thread.rb index 7d285530..b2fa636f 100644 --- a/engine/app/models/coplan/comment_thread.rb +++ b/engine/app/models/coplan/comment_thread.rb @@ -1,8 +1,8 @@ module CoPlan class CommentThread < ApplicationRecord - STATUSES = %w[pending todo resolved discarded].freeze - OPEN_STATUSES = %w[pending todo].freeze - CLOSED_STATUSES = %w[resolved discarded].freeze + STATUSES = %w[open resolved].freeze + OPEN_STATUSES = %w[open].freeze + CLOSED_STATUSES = %w[resolved].freeze attr_accessor :anchor_occurrence @@ -26,9 +26,9 @@ class CommentThread < ApplicationRecord before_validation :resolve_anchor_position, on: :create validate :anchor_must_resolve, on: :create - # Closing a thread settles its inbox rows. Every path that resolves or - # discards goes through an ordinary status write, so the sweep lives - # here rather than in the two controllers that trigger it. + # Closing a thread settles its inbox rows. Every path that resolves it + # goes through an ordinary status write, so the sweep lives here rather + # than in the controllers that trigger it. after_commit :mark_notifications_read_if_closed, on: :update scope :open_threads, -> { where(status: OPEN_STATUSES) } @@ -128,12 +128,8 @@ def resolve!(user) ) end - def accept!(user) - update!(status: "todo", resolved_by_user: user) - end - - def discard!(user) - update!(status: "discarded", resolved_by_user: user) + def reopen!(user) + update!(status: "open", resolved_by_user: nil) end def open? diff --git a/engine/app/policies/coplan/comment_thread_policy.rb b/engine/app/policies/coplan/comment_thread_policy.rb index a46022a1..1ca21b74 100644 --- a/engine/app/policies/coplan/comment_thread_policy.rb +++ b/engine/app/policies/coplan/comment_thread_policy.rb @@ -8,14 +8,6 @@ def resolve? record.created_by_user_id == user.id || record.plan.created_by_user_id == user.id end - def accept? - record.plan.created_by_user_id == user.id - end - - def discard? - record.plan.created_by_user_id == user.id - end - def reopen? record.created_by_user_id == user.id || record.plan.created_by_user_id == user.id end diff --git a/engine/app/views/coplan/agent_instructions/show.text.erb b/engine/app/views/coplan/agent_instructions/show.text.erb index 1c4aecac..af0862f1 100644 --- a/engine/app/views/coplan/agent_instructions/show.text.erb +++ b/engine/app/views/coplan/agent_instructions/show.text.erb @@ -675,15 +675,6 @@ Mark a comment thread as resolved (addressed by the plan author or thread creato "<%= @base %>/api/v1/plans/$PLAN_ID/comments/$THREAD_ID/resolve" | jq . ``` -### Dismiss Thread - -Dismiss a comment thread (plan author only — for comments that are out of scope or not applicable). `discard` is accepted as an alias for `dismiss`. - -```bash -<%= @curl %> -X PATCH \ - "<%= @base %>/api/v1/plans/$PLAN_ID/comments/$THREAD_ID/dismiss" | jq . -``` - ### Get a Single Thread Fetch one thread with its comments — handy when reacting to a single event without refetching the whole snapshot. diff --git a/engine/app/views/coplan/comment_threads/_thread.html.erb b/engine/app/views/coplan/comment_threads/_thread.html.erb index 6775c5c2..9ee8a149 100644 --- a/engine/app/views/coplan/comment_threads/_thread.html.erb +++ b/engine/app/views/coplan/comment_threads/_thread.html.erb @@ -7,7 +7,9 @@ data-thread-author-id="<%= thread.created_by_user_id %>">
- <%= thread.status %> + <% if thread.closed? %> + <%= thread.status %> + <% end %> <% if thread.out_of_date? %> out of date <% end %> @@ -30,9 +32,8 @@ <% if thread.open? %> <%= render partial: "coplan/comment_threads/reply_form", locals: { thread: thread, plan: plan } %> -
- <%= link_to "Accept", accept_plan_comment_thread_path(plan, thread), data: { turbo_method: :patch }, class: "btn btn--secondary btn--sm" %> - <%= link_to "Discard", discard_plan_comment_thread_path(plan, thread), data: { turbo_method: :patch }, class: "btn btn--secondary btn--sm" %> +
+ <%= link_to "Resolve", resolve_plan_comment_thread_path(plan, thread), data: { turbo_method: :patch }, class: "btn btn--secondary btn--sm" %>
<% else %>
diff --git a/engine/app/views/coplan/comment_threads/_thread_popover.html.erb b/engine/app/views/coplan/comment_threads/_thread_popover.html.erb index 926cf2cd..c92e3ecb 100644 --- a/engine/app/views/coplan/comment_threads/_thread_popover.html.erb +++ b/engine/app/views/coplan/comment_threads/_thread_popover.html.erb @@ -9,7 +9,9 @@
- <%= thread.status %> + <% if thread.closed? %> + <%= thread.status %> + <% end %> <% if thread.out_of_date? %> out of date <% end %> @@ -47,11 +49,8 @@ <% end %>
-
- <% if thread.status == "pending" %> - <%= button_to "Accept (a)", accept_plan_comment_thread_path(plan, thread), method: :patch, class: "btn btn--secondary btn--sm", form: { "data-action-name": "accept" } %> - <% end %> - <%= button_to "Discard (d)", discard_plan_comment_thread_path(plan, thread), method: :patch, class: "btn btn--secondary btn--sm", form: { "data-action-name": "discard" } %> +
+ <%= button_to "Resolve (e)", resolve_plan_comment_thread_path(plan, thread), method: :patch, class: "btn btn--secondary btn--sm", form: { "data-action-name": "resolve" } %>
<% else %>
diff --git a/engine/app/views/coplan/plans/show.html.erb b/engine/app/views/coplan/plans/show.html.erb index 98713b61..2b65e43b 100644 --- a/engine/app/views/coplan/plans/show.html.erb +++ b/engine/app/views/coplan/plans/show.html.erb @@ -167,7 +167,7 @@ <% end %> <%# Headless comment navigation — no floating toolbar, just the keyboard: - j/k step through comments, r focuses reply, a/d accept/discard, + j/k step through comments, r focuses reply, e resolves, s toggles resolved-thread visibility. Rendered even when the page loads without a single thread: comments arrive live (voice, selection, other viewers' broadcasts), and gating this on @threads.any? left the diff --git a/engine/app/views/coplan/welcome/_default_landing.html.erb b/engine/app/views/coplan/welcome/_default_landing.html.erb index 4e7807a6..9ccf3410 100644 --- a/engine/app/views/coplan/welcome/_default_landing.html.erb +++ b/engine/app/views/coplan/welcome/_default_landing.html.erb @@ -100,7 +100,7 @@

Inline, anchored comments

Select text in the rendered plan to start a thread, Google-Docs-style. Comments stay - pinned to the words they reference through triage: accept, resolve, or discard. + pinned to the words they reference until you resolve them.

diff --git a/engine/config/routes.rb b/engine/config/routes.rb index e56ed504..4b14c9a5 100644 --- a/engine/config/routes.rb +++ b/engine/config/routes.rb @@ -62,8 +62,6 @@ resources :comment_threads, only: [ :create ] do member do patch :resolve - patch :accept - patch :discard patch :reopen end resources :comments, only: [ :create, :destroy ] @@ -158,11 +156,6 @@ resources :comments, only: [ :create, :show ], controller: "comments" do post :reply, on: :member patch :resolve, on: :member - patch :discard, on: :member - # Alias: the agent instructions long documented this action as - # "dismiss" while the route said "discard" — accept both so - # agents following either name succeed. - patch :dismiss, on: :member, action: :discard end # Presence/state for an agent working this plan (drives the # "Claude is editing…" pill and subscribes the token to events). diff --git a/engine/db/migrate/20260828120000_simplify_comment_thread_statuses.rb b/engine/db/migrate/20260828120000_simplify_comment_thread_statuses.rb new file mode 100644 index 00000000..8dab3c61 --- /dev/null +++ b/engine/db/migrate/20260828120000_simplify_comment_thread_statuses.rb @@ -0,0 +1,25 @@ +class SimplifyCommentThreadStatuses < ActiveRecord::Migration[8.0] + def up + # Collapse the accept/reject mechanics into a plain open/resolved toggle: + # pending and todo (both "not yet resolved") become open; discarded + # (a rejection outcome) is treated the same as resolved (a closed thread). + # todo threads carry a resolved_by_user_id from accept! — clear it so an + # open thread never disagrees with newly created/reopened ones, which + # have no resolver. + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'open', resolved_by_user_id = NULL WHERE status IN ('pending', 'todo') + SQL + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'resolved' WHERE status = 'discarded' + SQL + change_column_default :coplan_comment_threads, :status, "open" + end + + def down + change_column_default :coplan_comment_threads, :status, "pending" + # Lossy: todo/discarded can't be distinguished from open/resolved after up. + execute <<~SQL + UPDATE coplan_comment_threads SET status = 'pending' WHERE status = 'open' + SQL + end +end diff --git a/spec/factories/comment_threads.rb b/spec/factories/comment_threads.rb index f486f183..245189c9 100644 --- a/spec/factories/comment_threads.rb +++ b/spec/factories/comment_threads.rb @@ -3,7 +3,7 @@ plan plan_version { plan.current_plan_version } created_by_user { association(:coplan_user) } - status { "pending" } + status { "open" } out_of_date { false } # Threads refuse anchors that don't resolve against the plan content, diff --git a/spec/lib/development_seed_spec.rb b/spec/lib/development_seed_spec.rb index 6f5dde71..0809788d 100644 --- a/spec/lib/development_seed_spec.rb +++ b/spec/lib/development_seed_spec.rb @@ -67,7 +67,7 @@ # Threads in every reviewer-facing state, all anchors resolved. threads = showcase.comment_threads - expect(threads.pluck(:status)).to include("pending", "todo", "resolved") + expect(threads.pluck(:status)).to include("open", "resolved") expect(threads.where(anchor_text: nil)).to exist anchored = threads.where.not(anchor_text: nil) expect(anchored.pluck(:anchor_start)).to all(be_present) diff --git a/spec/migrations/mark_closed_thread_notifications_read_spec.rb b/spec/migrations/mark_closed_thread_notifications_read_spec.rb index f2210103..13709a5b 100644 --- a/spec/migrations/mark_closed_thread_notifications_read_spec.rb +++ b/spec/migrations/mark_closed_thread_notifications_read_spec.rb @@ -7,21 +7,35 @@ let(:user) { create(:coplan_user) } let(:plan) { create(:plan, created_by_user: user) } + # Bypasses the model's current status validation (now just open/resolved) + # to plant the raw status values this historical migration actually ran + # against, back when pending/todo/discarded were valid. def notification_on(status) - thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: user, status: status) + thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: user) + thread.update_column(:status, status) create(:notification, user: user, plan: plan, comment_thread: thread) end before { migration.verbose = false } - it "marks unread rows on resolved and discarded threads read" do + it "marks unread rows on resolved threads read" do resolved = notification_on("resolved") - discarded = notification_on("discarded") migration.up expect(resolved.reload.read_at).to be_present - expect(discarded.reload.read_at).to be_present + end + + it "leaves unread rows on a legacy discarded thread alone" do + # The migration matches CommentThread::CLOSED_STATUSES, which no + # longer includes "discarded" now that thread statuses have + # collapsed to open/resolved — so a stray legacy row wouldn't be + # swept by a re-run of this historical migration today. + discarded = notification_on("discarded") + + migration.up + + expect(discarded.reload.read_at).to be_nil end it "leaves rows on open threads unread" do diff --git a/spec/models/comment_thread_anchor_spec.rb b/spec/models/comment_thread_anchor_spec.rb index 9165f04f..b355f79d 100644 --- a/spec/models/comment_thread_anchor_spec.rb +++ b/spec/models/comment_thread_anchor_spec.rb @@ -51,7 +51,7 @@ ) thread.update_columns(anchor_text: "text no longer in the plan", anchor_start: nil, anchor_end: nil) - expect(thread.reload.update(status: "todo")).to be true + expect(thread.reload.update(status: "resolved")).to be true end end diff --git a/spec/models/comment_thread_spec.rb b/spec/models/comment_thread_spec.rb index 172d17fc..528e7a80 100644 --- a/spec/models/comment_thread_spec.rb +++ b/spec/models/comment_thread_spec.rb @@ -69,16 +69,11 @@ expect(thread_record.resolved_by_user).to eq(user) end - it "accept! sets status to todo" do - thread_record.accept!(user) - expect(thread_record.status).to eq("todo") - expect(thread_record.resolved_by_user).to eq(user) - end - - it "discard! sets status and user" do - thread_record.discard!(user) - expect(thread_record.status).to eq("discarded") - expect(thread_record.resolved_by_user).to eq(user) + it "reopen! sets status back to open and clears resolved_by_user" do + thread_record.resolve!(user) + thread_record.reopen!(user) + expect(thread_record.status).to eq("open") + expect(thread_record.resolved_by_user).to be_nil end describe "clearing notifications when a thread closes" do @@ -94,22 +89,6 @@ expect(notification.reload.read_at).to be_present end - it "marks the thread's unread rows read on discard" do - notification = create(:notification, user: recipient, plan: plan, comment_thread: thread_record) - - thread_record.discard!(user) - - expect(notification.reload.read_at).to be_present - end - - it "leaves rows alone when the thread stays open" do - notification = create(:notification, user: recipient, plan: plan, comment_thread: thread_record) - - thread_record.accept!(user) - - expect(notification.reload.read_at).to be_nil - end - it "does not clear rows on an unrelated update to a closed thread" do thread_record.resolve!(user) notification = create(:notification, user: recipient, plan: plan, comment_thread: thread_record, reason: "reply") @@ -120,29 +99,21 @@ end end - it "open? returns true for pending and todo" do - thread_record.status = "pending" - expect(thread_record).to be_open - thread_record.status = "todo" + it "open? returns true for open" do + thread_record.status = "open" expect(thread_record).to be_open end - it "closed? returns true for resolved and discarded only" do + it "closed? returns true for resolved only" do thread_record.status = "resolved" expect(thread_record).to be_closed - thread_record.status = "discarded" - expect(thread_record).to be_closed - thread_record.status = "pending" - expect(thread_record).not_to be_closed - thread_record.status = "todo" + thread_record.status = "open" expect(thread_record).not_to be_closed end - it "open? returns false for resolved and discarded" do + it "open? returns false for resolved" do thread_record.status = "resolved" expect(thread_record).not_to be_open - thread_record.status = "discarded" - expect(thread_record).not_to be_open end it "open_threads scope returns only open threads" do diff --git a/spec/models/coplan/comment_thread_analytics_spec.rb b/spec/models/coplan/comment_thread_analytics_spec.rb index b1a1bb03..9fba68bb 100644 --- a/spec/models/coplan/comment_thread_analytics_spec.rb +++ b/spec/models/coplan/comment_thread_analytics_spec.rb @@ -17,17 +17,9 @@ expect(payload[:properties]).to include( plan_id: thread.plan_id, comment_thread_id: thread.id, - previous_status: "pending", + previous_status: "open", comment_count: 2, anchored: false ) end - - it "does not track when accept! or discard! are called" do - events = capture_analytics_events do - thread.accept!(user) - create(:comment_thread).discard!(user) - end - expect(events).to be_empty - end end diff --git a/spec/requests/api/v1/agent_events_spec.rb b/spec/requests/api/v1/agent_events_spec.rb index 9edfb364..3e3c3b1a 100644 --- a/spec/requests/api/v1/agent_events_spec.rb +++ b/spec/requests/api/v1/agent_events_spec.rb @@ -492,12 +492,6 @@ expect(body["comments"].length).to eq(1) end - it "accepts dismiss as an alias for discard" do - patch dismiss_api_v1_plan_comment_path(plan, thread), headers: agent_headers, as: :json - expect(response).to have_http_status(:ok) - expect(thread.reload.status).to eq("discarded") - end - it "does not leave an orphan thread when the first comment fails validation" do expect { # An empty body fails the comment, which must roll the thread back @@ -507,10 +501,10 @@ expect(response).to have_http_status(:unprocessable_content) end - it "gives the plan author's own API threads the todo initial status" do + it "gives API threads the open initial status regardless of author" do post api_v1_plan_comments_path(plan), params: { body_markdown: "note to self", agent_name: "Claude" }, headers: agent_headers, as: :json expect(response).to have_http_status(:created) - expect(JSON.parse(response.body)["status"]).to eq("todo") + expect(JSON.parse(response.body)["status"]).to eq("open") end end diff --git a/spec/requests/api/v1/comments_spec.rb b/spec/requests/api/v1/comments_spec.rb index 72beb688..389adf98 100644 --- a/spec/requests/api/v1/comments_spec.rb +++ b/spec/requests/api/v1/comments_spec.rb @@ -104,36 +104,6 @@ end end - describe "PATCH discard" do - it "discards a thread" do - patch discard_api_v1_plan_comment_path(plan, thread_record), - headers: headers, - as: :json - expect(response).to have_http_status(:ok) - body = JSON.parse(response.body) - expect(body["status"]).to eq("discarded") - expect(thread_record.reload.status).to eq("discarded") - end - - it "returns 404 for nonexistent thread" do - patch discard_api_v1_plan_comment_path(plan, "nonexistent-id"), - headers: headers, - as: :json - expect(response).to have_http_status(:not_found) - end - - it "returns 403 when user is not the plan author" do - bob = create(:coplan_user) - bob_token = create(:api_token, user: bob, raw_token: "test-token-bob") - bob_headers = { "Authorization" => "Bearer test-token-bob" } - - patch discard_api_v1_plan_comment_path(plan, thread_record), - headers: bob_headers, - as: :json - expect(response).to have_http_status(:forbidden) - end - end - # The token always knows who it speaks for, so omitting agent_name is # no longer an error — it falls back to the token's agent_name, then # its name. (It used to 422, which punished exactly the callers who diff --git a/spec/requests/api/v1/content_spec.rb b/spec/requests/api/v1/content_spec.rb index eb3396c5..6d8022f4 100644 --- a/spec/requests/api/v1/content_spec.rb +++ b/spec/requests/api/v1/content_spec.rb @@ -136,7 +136,7 @@ def put_content(body, params: {}) anchor_revision: 1, anchor_start: initial_content.index(anchor_text), anchor_end: initial_content.index(anchor_text) + anchor_text.length, - status: "todo" + status: "open" ) end diff --git a/spec/requests/comment_threads_spec.rb b/spec/requests/comment_threads_spec.rb index 03b170df..1646fde5 100644 --- a/spec/requests/comment_threads_spec.rb +++ b/spec/requests/comment_threads_spec.rb @@ -29,7 +29,7 @@ thread = CoPlan::CommentThread.last expect(thread.anchor_text).to eq("world domination") expect(thread.anchor_start).to be_present # resolved at the door - expect(thread.status).to eq("todo") # author's own comments start as todo + expect(thread.status).to eq("open") expect(thread.plan_version_id).to eq(plan.current_plan_version_id) end @@ -111,44 +111,22 @@ expect(notification.reload.read_at).to be_present end - it "discarding a thread clears its unread notifications" do - thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: bob) - notification = create(:notification, user: bob, plan: plan, comment_thread: thread) - - patch discard_plan_comment_thread_path(plan, thread) - - expect(notification.reload.read_at).to be_present - end - - it "accept thread as plan author" do - thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: alice) - patch accept_plan_comment_thread_path(plan, thread) - thread.reload - expect(thread.status).to eq("todo") - end - - it "discard thread as plan author" do - thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: alice) - patch discard_plan_comment_thread_path(plan, thread) - thread.reload - expect(thread.status).to eq("discarded") - end - it "reopen resolved thread" do thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: alice) thread.resolve!(alice) patch reopen_plan_comment_thread_path(plan, thread) thread.reload - expect(thread.status).to eq("pending") + expect(thread.status).to eq("open") expect(thread.resolved_by_user_id).to be_nil end - it "non-author cannot accept thread" do - sign_in_as(bob) + it "non-creator, non-plan-author cannot resolve thread" do + carol = create(:coplan_user) + sign_in_as(carol) thread = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: bob) - patch accept_plan_comment_thread_path(plan, thread) + patch resolve_plan_comment_thread_path(plan, thread) expect(response).to have_http_status(:not_found) thread.reload - expect(thread.status).to eq("pending") + expect(thread.status).to eq("open") end end diff --git a/spec/services/notifications/create_spec.rb b/spec/services/notifications/create_spec.rb index 5a43dd7a..206fc078 100644 --- a/spec/services/notifications/create_spec.rb +++ b/spec/services/notifications/create_spec.rb @@ -134,14 +134,6 @@ }.not_to change(CoPlan::Notification, :count) end - it "does not notify about an agent response on a discarded thread" do - thread.update!(status: "discarded", resolved_by_user: plan_author) - - expect { - described_class.call(comment_thread: thread, actor_id: create(:coplan_user).id, reason: "agent_response") - }.not_to change(CoPlan::Notification, :count) - end - it "still notifies about a human reply — somebody reopening the conversation is news" do expect { described_class.call(comment_thread: resolved_thread, actor_id: create(:coplan_user).id, reason: "reply") @@ -155,7 +147,7 @@ it "sees a close that committed after the job loaded the thread" do stale = CoPlan::CommentThread.find(thread.id) CoPlan::CommentThread.find(thread.id).resolve!(plan_author) - expect(stale.status).to eq("pending") # in-memory copy is behind + expect(stale.status).to eq("open") # in-memory copy is behind expect { described_class.call(comment_thread: stale, actor_id: create(:coplan_user).id, reason: "agent_response") @@ -163,7 +155,7 @@ end it "still notifies about a status change when the thread was reopened" do - resolved_thread.update!(status: "pending", resolved_by_user: nil) + resolved_thread.update!(status: "open", resolved_by_user: nil) expect { described_class.call(comment_thread: resolved_thread, actor_id: plan_author.id, reason: "status_change") diff --git a/spec/services/plans/commit_session_spec.rb b/spec/services/plans/commit_session_spec.rb index a92fbe7f..6d639e60 100644 --- a/spec/services/plans/commit_session_spec.rb +++ b/spec/services/plans/commit_session_spec.rb @@ -285,7 +285,7 @@ def build_session(plan:, operations_json: [], draft_content: nil, base_revision: plan: plan, plan_version: plan.current_plan_version, created_by_user: user, - status: "pending", + status: "open", anchor_text: "We should use unit tests." ) diff --git a/spec/services/plans/replace_content_spec.rb b/spec/services/plans/replace_content_spec.rb index df043537..1e7ace19 100644 --- a/spec/services/plans/replace_content_spec.rb +++ b/spec/services/plans/replace_content_spec.rb @@ -235,7 +235,7 @@ anchor_revision: 1, anchor_start: initial_content.index("My Plan"), anchor_end: initial_content.index("My Plan") + "My Plan".length, - status: "todo" + status: "open" ) end @@ -248,7 +248,7 @@ anchor_revision: 1, anchor_start: initial_content.index("unit tests"), anchor_end: initial_content.index("unit tests") + "unit tests".length, - status: "todo" + status: "open" ) end @@ -261,7 +261,7 @@ anchor_revision: 1, anchor_start: initial_content.index("Q1 2026 delivery."), anchor_end: initial_content.index("Q1 2026 delivery.") + "Q1 2026 delivery.".length, - status: "todo" + status: "open" ) end diff --git a/spec/system/comment_ux_spec.rb b/spec/system/comment_ux_spec.rb index c36baeee..da93674c 100644 --- a/spec/system/comment_ux_spec.rb +++ b/spec/system/comment_ux_spec.rb @@ -48,7 +48,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) anchor_text: anchor_text, anchor_occurrence: 1, created_by_user: user, - status: "pending" + status: "open" ) thread.comments.create!( author_type: "human", @@ -219,44 +219,28 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css("mark.anchor-highlight--open", text: "microservices architecture") end - it "renders pending highlights in amber and todo highlights in blue" do - pending_thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback", user: reviewer) - todo_thread = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) - todo_thread.accept!(author) - - visit plan_page_path(plan) - - pending_mark = find("mark.anchor-highlight--pending") - pending_border = pending_mark.evaluate_script("getComputedStyle(this).borderBottomColor") - expect(pending_border).to include("245") # amber/orange channel - - todo_mark = find("mark.anchor-highlight--todo") - todo_border = todo_mark.evaluate_script("getComputedStyle(this).borderBottomColor") - expect(todo_border).to include("130") # blue channel (59, 130, 246) - end - - it "renders resolved thread highlights unstyled by default" do + it "renders resolved thread highlights with a dashed underline by default" do thread = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) thread.resolve!(author) visit plan_page_path(plan) - # Mark is present (text must remain visible) but has no visual highlight styling + # Resolved threads stay part of the doc's browsable history by default. expect(page).to have_css("mark.anchor-highlight--resolved", text: "PostgreSQL") mark = find("mark.anchor-highlight--resolved") border = mark.evaluate_script("getComputedStyle(this).borderBottomStyle") - expect(border).to eq("none") + expect(border).to eq("dashed") end - it "shows resolved highlights with dashed underline after pressing s" do + it "hides resolved highlights after pressing s" do thread = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) thread.resolve!(author) visit plan_page_path(plan) find("body").send_keys("s") - mark = find("mark.anchor-highlight--resolved") + mark = find("mark.anchor-highlight--resolved", visible: :all) border = mark.evaluate_script("getComputedStyle(this).borderBottomStyle") - expect(border).to eq("dashed") + expect(border).to eq("none") end it "does not toggle resolved visibility while typing s in a reply box" do @@ -270,7 +254,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) find("textarea").send_keys("s") end - expect(page).not_to have_css(".plan-layout--show-resolved") + expect(page).not_to have_css(".plan-layout--hide-resolved") end end @@ -499,13 +483,24 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) end end - it "shows status-specific badge in popover" do + it "shows no status badge for an open thread" do create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback", user: reviewer) visit plan_page_path(plan) find("mark.anchor-highlight--open").click within(".thread-popover") do - expect(page).to have_css(".badge--pending") + expect(page).to have_no_css(".badge--open") + end + end + + it "shows a resolved badge once a thread is closed" do + thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback", user: reviewer) + thread.resolve!(author) + visit plan_page_path(plan) + find("mark.anchor-highlight--resolved").click + + within(".thread-popover") do + expect(page).to have_css(".badge--resolved") end end @@ -515,8 +510,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) find("mark.anchor-highlight--open").click within(".thread-popover") do - expect(page).to have_button("Accept (a)") - expect(page).to have_button("Discard (d)") + expect(page).to have_button("Resolve (e)") end end end @@ -524,17 +518,14 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) describe "thread popovers as a non-author" do before { sign_in(reviewer) } - it "does not show Accept/Discard to someone who isn't the plan author" do - # The reviewer authored this comment but does not own the plan, so they - # must not see the author-only triage actions. + it "shows Resolve to the thread's own author even though they don't own the plan" do create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback", user: reviewer) visit plan_page_path(plan) find("mark.anchor-highlight--open").click within(".thread-popover") do expect(page).to have_css("textarea[placeholder='Press r to reply']") - expect(page).to have_no_button("Accept (a)") - expect(page).to have_no_button("Discard (d)") + expect(page).to have_button("Resolve (e)") end end @@ -542,9 +533,8 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) # The reviewer authored this thread, so they may reopen it even though # they don't own the plan. thread = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) - thread.discard!(author) + thread.resolve!(author) visit plan_page_path(plan) - find("body").send_keys("s") find("mark.anchor-highlight--resolved").click within(".thread-popover") do @@ -560,9 +550,8 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) it "does not show Reopen on a closed thread the viewer neither owns nor authored" do thread = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) - thread.discard!(author) + thread.resolve!(author) visit plan_page_path(plan) - find("body").send_keys("s") find("mark.anchor-highlight--resolved").click within(".thread-popover") do @@ -603,30 +592,17 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) describe "lifecycle actions via popover" do before { sign_in(author) } - it "accepts a thread (pending → todo)" do + it "resolves a thread (open → resolved)" do thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Agree with this", user: reviewer) visit plan_page_path(plan) find("mark.anchor-highlight--open").click expect(page).to have_css(".thread-popover", visible: true) - accept_form = find(".thread-popover", visible: true).find("form[action*='accept']", visible: :all) - accept_form.find("input[type='submit'], button[type='submit']", visible: :all).click - - expect(page).not_to have_css(".thread-popover", visible: true) - expect(thread.reload.status).to eq("todo") - end - - it "discards a thread (pending → discarded)" do - thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Not relevant", user: reviewer) - visit plan_page_path(plan) - find("mark.anchor-highlight--open").click - expect(page).to have_css(".thread-popover", visible: true) - - discard_form = find(".thread-popover", visible: true).find("form[action*='discard']", visible: :all) - discard_form.find("input[type='submit'], button[type='submit']", visible: :all).click + resolve_form = find(".thread-popover", visible: true).find("form[action*='resolve']", visible: :all) + resolve_form.find("input[type='submit'], button[type='submit']", visible: :all).click expect(page).not_to have_css(".thread-popover", visible: true) - expect(thread.reload.status).to eq("discarded") + expect(thread.reload.status).to eq("resolved") end end @@ -724,7 +700,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(thread.comments.count).to eq(1) end - it "accepts a pending thread with 'a' key and auto-advances" do + it "resolves an open thread with 'e' key and auto-advances" do thread1 = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback 1", user: reviewer) thread2 = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Feedback 2", user: reviewer) visit plan_page_path(plan) @@ -734,29 +710,15 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css("mark.anchor-highlight--active") expect(find(".thread-popover", visible: true)).to have_content("Feedback 1") - # Press 'a' to accept - find("body").send_keys("a") + # Press 'e' to resolve + find("body").send_keys("e") # Wait for the thread data attribute to update via broadcast - expect(page).to have_css("[data-thread-status='todo']", visible: :all, wait: 5) - expect(thread1.reload.status).to eq("todo") + expect(page).to have_css("[data-thread-status='resolved']", visible: :all, wait: 5) + expect(thread1.reload.status).to eq("resolved") end - it "discards a pending thread with 'd' key" do - thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Not relevant", user: reviewer) - visit plan_page_path(plan) - - find("body").send_keys("j") - expect(page).to have_css("mark.anchor-highlight--active") - expect(page).to have_css(".thread-popover", visible: true) - - find("body").send_keys("d") - - expect(page).to have_css("[data-thread-status='discarded']", visible: :all, wait: 5) - expect(thread.reload.status).to eq("discarded") - end - - it "does not fire a/d shortcuts when typing in a textarea" do + it "does not fire the e shortcut when typing in a textarea" do create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Feedback", user: reviewer) visit plan_page_path(plan) @@ -764,7 +726,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css(".thread-popover", visible: true) find("body").send_keys("r") - # Type 'a' inside the textarea — should not trigger accept + # Type 'e' inside the textarea — should not trigger resolve active_el = page.evaluate_script("document.activeElement.tagName") expect(active_el).to eq("TEXTAREA") end @@ -906,7 +868,9 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) end expect(page).not_to have_css("#new-comment-form", visible: true, wait: 5) - thread = plan.comment_threads.reload.last + # Plain #last has no ORDER BY, so its row order is unspecified — it + # happens to match insertion order on MySQL but not on Postgres. + thread = plan.comment_threads.reload.order(:created_at).last expect(thread.anchor_text).to eq("Hard") expect(page).to have_css("mark.anchor-highlight", text: "Hard", wait: 5) expect(page).to have_css("##{ActionView::RecordIdentifier.dom_id(thread)}") @@ -916,7 +880,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_css("mark.anchor-highlight", text: "Hard", wait: 5) mark = find("mark[data-thread-id='#{ActionView::RecordIdentifier.dom_id(thread)}']") - expect(mark[:class]).to include("anchor-highlight--todo") + expect(mark[:class]).to include("anchor-highlight--open") mark.click expect(page).to have_css("##{ActionView::RecordIdentifier.dom_id(thread)}_popover", visible: true) expect(page).to have_content("This case needs more detail.") @@ -947,7 +911,7 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) thread = plan.comment_threads.reload.last expect(thread).to be_present expect(thread.anchor_text).to eq("microservices architecture") - expect(thread.status).to eq("todo") # author's own comments start as todo + expect(thread.status).to eq("open") expect(thread.comments.first.body_markdown).to eq("Should we reconsider this?") end end @@ -986,7 +950,7 @@ def create_cross_element_thread(plan:, anchor_text:, body:, user:) anchor_end: anchor_text.length, anchor_revision: plan.current_revision, created_by_user: user, - status: "pending" + status: "open" ) thread.comments.create!( author_type: "human", @@ -1159,7 +1123,7 @@ def create_cross_element_thread(plan:, anchor_text:, body:, user:) plan_version: table_plan.current_plan_version, anchor_text: anchor, created_by_user: reviewer, - status: "pending", + status: "open", anchor_start: 0, anchor_end: anchor.length, anchor_revision: table_plan.current_revision diff --git a/spec/system/voice_commenting_spec.rb b/spec/system/voice_commenting_spec.rb index b26157fe..095b0193 100644 --- a/spec/system/voice_commenting_spec.rb +++ b/spec/system/voice_commenting_spec.rb @@ -393,8 +393,8 @@ def stub_recorder(peak: 30, context_state: "running") # the keyboard has to work on it. Comment navigation used to render # only when the page loaded with threads already present — so on a # fresh plan the dictated comment appeared, its popover opened, and - # d/j/k did nothing at all. - it "lets the keyboard discard the first comment without a reload" do + # e/j/k did nothing at all. + it "lets the keyboard resolve the first comment without a reload" do allow(CoPlan::Ai).to receive(:transcribe).and_return("too cautious") allow(CoPlan::Ai).to receive(:call).and_return({ "text" => "Too cautious.", @@ -410,12 +410,12 @@ def stub_recorder(peak: 30, context_state: "running") thread = nil expect(page).to have_css(".voice-status", text: /Comment added/, wait: 10) expect { thread = CoPlan::CommentThread.where(plan_id: plan.id).sole }.not_to raise_error - # The auto-opened popover is the discard target. + # The auto-opened popover is the resolve target. expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true, wait: 10) - find("body").send_keys("d") + find("body").send_keys("e") - Timeout.timeout(10) { sleep 0.1 until thread.reload.status == "discarded" } + Timeout.timeout(10) { sleep 0.1 until thread.reload.status == "resolved" } expect(page).to have_no_css("mark.anchor-highlight--open", wait: 10) end