Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -157,16 +156,15 @@ reader to an apparently empty page.
(`NotificationsController#mark_plan_read`, same service)

### Inline review UI
- **Highlights**: anchored text is wrapped in `<mark>` elements — amber for `pending`, blue for `todo`, unstyled for `resolved`
- **Highlights**: anchored text is wrapped in `<mark>` 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

### 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.

Expand Down
Original file line number Diff line number Diff line change
@@ -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
4 changes: 2 additions & 2 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 3 additions & 5 deletions db/seeds/development.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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."
)

Expand All @@ -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.
Expand All @@ -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
Expand Down
42 changes: 8 additions & 34 deletions engine/app/assets/stylesheets/coplan/application.css
Original file line number Diff line number Diff line change
Expand Up @@ -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); }
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
29 changes: 1 addition & 28 deletions engine/app/controllers/coplan/api/v1/comments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
27 changes: 3 additions & 24 deletions engine/app/controllers/coplan/comment_threads_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]
Expand All @@ -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
Expand Down Expand Up @@ -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 ])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -201,12 +197,8 @@ export default class extends Controller {
}
}

acceptCurrent() {
this.submitPopoverAction("accept")
}

discardCurrent() {
this.submitPopoverAction("discard")
resolveCurrent() {
this.submitPopoverAction("resolve")
}

submitPopoverAction(action) {
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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])
Expand All @@ -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")
}
}
Loading
Loading