From a406ddb0a77f8994bacbe2ce7bd172f5f7a490f7 Mon Sep 17 00:00:00 2001 From: coltonw Date: Fri, 30 Oct 2026 15:36:02 -0400 Subject: [PATCH 1/2] Show discussion avatars in the page margin Open comment threads were only visible as faint highlights in the text. Each one now gets the first commenter's avatar in the page margin beside the document, aligned with its line. Clicking the avatar opens the discussion, and hovering the avatar or its text highlights both. The markers sit outside the document card, so the text keeps its width. They hide when the window leaves no room in the margin. Co-authored-by: Amp Amp-Thread-ID: https://ampcode.com/threads/T-01a18e32-b8a4-7319-95b9-7279b536d0f1 --- .../assets/stylesheets/coplan/comments.css | 57 ++++++ .../coplan/comment_margin_controller.js | 173 ++++++++++++++++++ .../coplan/text_selection_controller.js | 2 + engine/app/views/coplan/plans/show.html.erb | 5 + spec/system/comment_ux_spec.rb | 40 ++++ 5 files changed, 277 insertions(+) create mode 100644 engine/app/javascript/controllers/coplan/comment_margin_controller.js diff --git a/engine/app/assets/stylesheets/coplan/comments.css b/engine/app/assets/stylesheets/coplan/comments.css index fc068613..bd7845fc 100644 --- a/engine/app/assets/stylesheets/coplan/comments.css +++ b/engine/app/assets/stylesheets/coplan/comments.css @@ -148,6 +148,63 @@ min-width: 0; } +/* Margin discussion markers: the first commenter's avatar beside the text a + discussion is about, in the page margin outside the document card so the + text keeps its width. comment_margin_controller sets the column's offset + and leaves it hidden when the margin has no room. */ +.comment-margin { + position: absolute; + top: 0; + bottom: 0; +} + +.comment-margin[hidden], +.comment-margin:not([data-fits]) { display: none; } + +.comment-margin__card { + position: absolute; + left: 0; + display: grid; + place-items: center; + width: 30px; + height: 30px; + padding: 0; + border: 2px solid var(--color-surface); + border-radius: 50%; + background: var(--color-surface); + box-shadow: var(--shadow-pop); + color: var(--color-text-muted); + cursor: pointer; + transition: top 0.15s ease, border-color 0.15s ease, transform 0.15s ease; +} + +/* Set while the marker or its highlighted text is hovered. */ +.comment-margin__card.is-hover { + border-color: var(--color-highlight-open-border); + transform: scale(1.15); +} + +.comment-margin__card:focus-visible { + outline: 2px solid var(--color-primary); + outline-offset: 2px; +} + +.comment-margin__card.is-active { border-color: var(--color-primary); } + +.comment-margin__card--resolved { opacity: 0.6; } + +.comment-margin__card .avatar, +.comment-margin__card .comment__agent-avatar { + width: 26px; + height: 26px; + font-size: 0.6rem; + border-radius: 50%; +} + +.anchor-highlight--open.is-margin-hover { + background: var(--color-highlight-open-hover-bg); +} + #plan-threads { position: absolute; width: 0; diff --git a/engine/app/javascript/controllers/coplan/comment_margin_controller.js b/engine/app/javascript/controllers/coplan/comment_margin_controller.js new file mode 100644 index 00000000..28bf0d87 --- /dev/null +++ b/engine/app/javascript/controllers/coplan/comment_margin_controller.js @@ -0,0 +1,173 @@ +import { Controller } from "@hotwired/stimulus" + +// Collapsed discussion markers in the page margin, one per highlighted +// thread, aligned with the text they discuss. A marker shows the first +// commenter's avatar; clicking it opens the same discussion window as +// clicking the highlight. +// +// Cards are derived from the highlights coplan--text-selection builds and +// from the thread popovers in #plan-threads, so broadcasts that add +// threads or comments update them without a server round-trip. +const STACK_GAP = 4 +// Room beside the document card: gap from the card, gap from the window +// edge, and the marker width. +const GAP = 12 +const EDGE = 8 +const WIDTH = 30 +// Lucide message-circle, copyright 2026 Lucide Icons, ISC license. +const COMMENT_ICON = '' + +export default class extends Controller { + connect() { + this.layout = this.element.closest(".plan-layout") + this.content = this.layout.querySelector("[data-coplan--text-selection-target~='content']") + this.cards = new Map() + + // Comment appends, edits, and deletes change a thread's summary without + // re-running highlightAnchors. + const threads = this.layout.querySelector("#plan-threads") + if (threads) { + this.threadsObserver = new MutationObserver(() => this.scheduleRefresh()) + this.threadsObserver.observe(threads, { childList: true, subtree: true }) + } + // Images, diagrams, and expanded rows move the text after highlighting. + this.resizeObserver = new ResizeObserver(() => this.position()) + this.resizeObserver.observe(this.content) + this.refresh() + } + + disconnect() { + this.threadsObserver?.disconnect() + this.resizeObserver?.disconnect() + cancelAnimationFrame(this.refreshFrame) + } + + scheduleRefresh() { + cancelAnimationFrame(this.refreshFrame) + this.refreshFrame = requestAnimationFrame(() => this.refresh()) + } + + refresh() { + const showResolved = !this.layout.classList.contains("plan-layout--hide-resolved") + const marks = new Map() + for (const mark of this.content.querySelectorAll("mark.anchor-highlight[data-thread-id]")) { + const id = mark.dataset.threadId + if (marks.has(id) || !mark.getClientRects().length) continue + if (!showResolved && !mark.classList.contains("anchor-highlight--open")) continue + marks.set(id, mark) + } + + for (const [id, card] of this.cards) { + if (marks.has(id)) continue + card.remove() + this.cards.delete(id) + } + for (const [id, mark] of marks) { + const popover = document.getElementById(`${id}_popover`) + if (!popover) continue + const card = this.cards.get(id) || this.buildCard(id) + this.fillCard(card, popover, mark) + } + + this.marks = marks + this.element.hidden = this.cards.size === 0 + this.position() + } + + // Reuse card elements: an open discussion window tracks its trigger + // element while the page scrolls. + buildCard(id) { + const card = document.createElement("button") + card.type = "button" + card.className = "comment-margin__card" + card.dataset.threadId = id + card.dataset.action = "click->coplan--text-selection#openThreadPopover" + this.element.append(card) + this.cards.set(id, card) + return card + } + + // A card shows only the first commenter's avatar; the name and reply count + // are for assistive technology. + fillCard(card, popover, mark) { + const comments = popover.querySelectorAll(".thread-popover__comments > .comment:not(.comment--deleted)") + const header = comments[0]?.querySelector(".comment__header") + const avatar = header?.querySelector(".avatar, .comment__agent-avatar")?.cloneNode(true) + const author = header?.querySelector("strong")?.textContent.trim() + const replies = comments.length - 1 + const resolved = !mark.classList.contains("anchor-highlight--open") + const signature = [author, replies, resolved, avatar?.outerHTML].join("\u0000") + if (card.dataset.signature === signature) return + card.dataset.signature = signature + + card.classList.toggle("comment-margin__card--resolved", resolved) + if (avatar) { + avatar.removeAttribute("title") + avatar.setAttribute("aria-hidden", "true") + card.replaceChildren(avatar) + } else { + card.innerHTML = COMMENT_ICON + } + const label = [author ? `Discussion started by ${author}` : "Discussion"] + if (replies > 0) label.push(`${replies} ${replies === 1 ? "reply" : "replies"}`) + card.setAttribute("aria-label", label.join(", ")) + } + + // Place the column in the page margin right of the document card, then + // align each card with its highlight, pushing later cards down so they + // never overlap. + position() { + if (!this.marks || this.element.hidden) return + const layoutRect = this.layout.getBoundingClientRect() + const cardRight = (this.layout.closest(".plan-document") || this.layout).getBoundingClientRect().right + const fits = document.documentElement.clientWidth - cardRight - GAP - EDGE >= WIDTH + this.element.toggleAttribute("data-fits", fits) + if (!fits) return + this.element.style.left = `${cardRight + GAP - layoutRect.left}px` + + const originTop = this.element.getBoundingClientRect().top + const placed = [] + for (const [id, mark] of this.marks) { + const card = this.cards.get(id) + const rect = mark.getClientRects()[0] + if (!card || !rect) continue + placed.push({ card, top: rect.top + (rect.height - WIDTH) / 2 - originTop }) + } + placed.sort((a, b) => a.top - b.top) + let floor = 0 + for (const { card, top } of placed) { + const y = Math.max(top, floor) + card.style.top = `${y}px` + floor = y + card.offsetHeight + STACK_GAP + } + } + + // Hovering a marker or any fragment of its highlight lights up both. + // Highlights are rebuilt by coplan--text-selection, so hover is tracked + // with one delegated document listener instead of per-mark bindings. + hoverChanged(event) { + const target = event.target.closest?.(".comment-margin__card, mark.anchor-highlight[data-thread-id]") + const inScope = target && (this.element.contains(target) || this.content.contains(target)) + const id = inScope && this.cards.has(target.dataset.threadId) ? target.dataset.threadId : null + if (id === this.hoveredId) return + this.setHover(this.hoveredId, false) + this.hoveredId = id + this.setHover(id, true) + } + + setHover(id, on) { + if (!id) return + this.cards.get(id)?.classList.toggle("is-hover", on) + this.content.querySelectorAll(`mark.anchor-highlight[data-thread-id="${id}"]`) + .forEach(mark => mark.classList.toggle("is-margin-hover", on)) + } + + // Keep the card for the open discussion visibly selected, however it was + // opened (card, highlight, j/k, or a deep link). + popoverToggled(event) { + const popover = event.target + if (!popover.classList?.contains("thread-popover")) return + const card = this.cards.get(popover.id.replace(/_popover$/, "")) + card?.classList.toggle("is-active", event.newState === "open") + } +} diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js index 9dfe5ee9..5bd11c03 100644 --- a/engine/app/javascript/controllers/coplan/text_selection_controller.js +++ b/engine/app/javascript/controllers/coplan/text_selection_controller.js @@ -512,6 +512,8 @@ export default class extends Controller { _restoreActiveThreadPopover(threadId) { if (!threadId || !this._activePopover) return + // Margin cards survive re-highlighting; keep the window beside its card. + if (this._activeMark?.isConnected && this._activeMark.closest(".comment-margin")) return const replacementMark = this.contentTarget.querySelector(`.anchor-highlight[data-thread-id="${threadId}"]`) if (replacementMark && this._findOpenPopover() === this._activePopover) { diff --git a/engine/app/views/coplan/plans/show.html.erb b/engine/app/views/coplan/plans/show.html.erb index d5bd8fb3..850eba91 100644 --- a/engine/app/views/coplan/plans/show.html.erb +++ b/engine/app/views/coplan/plans/show.html.erb @@ -148,6 +148,11 @@ <%= render "coplan/plans/thread_collection", plan: @plan, threads: @threads %> + <%# Avatar markers in the page margin beside the text each discussion is + about. Positioned outside the card, so the text keeps its width. %> + diff --git a/spec/system/comment_ux_spec.rb b/spec/system/comment_ux_spec.rb index 5e36dfde..d9718cba 100644 --- a/spec/system/comment_ux_spec.rb +++ b/spec/system/comment_ux_spec.rb @@ -278,6 +278,46 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(mark.evaluate_script("getComputedStyle(this).borderBottomStyle")).to eq("none") end + it "shows an avatar marker in the margin for each open discussion that opens it" do + postgres = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) + opened = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Why not monolith?", user: reviewer) + opened.comments.create!(author_type: "human", author_id: author.id, body_markdown: "Scale needs.") + resolved = create_anchored_thread(plan: plan, anchor_text: "rate limiting", body: "Per user?", user: reviewer) + resolved.resolve!(author) + + page.current_window.resize_to(1600, 900) + visit plan_page_path(plan) + + # Resolved discussions stay hidden by default. + expect(page).to have_css(".comment-margin__card", count: 2) + opened_card = find(".comment-margin__card[data-thread-id='comment_thread_#{opened.id}']") + postgres_card = find(".comment-margin__card[data-thread-id='comment_thread_#{postgres.id}']") + expect(opened_card).to have_css(".avatar[aria-label='#{reviewer.name}']") + expect(opened_card[:"aria-label"]).to eq("Discussion started by #{reviewer.name}, 1 reply") + + # Each marker sits in the page margin, outside the document, on the + # line it discusses. + middle = ->(element) { element.evaluate_script("(r => r.top + r.height / 2)(this.getClientRects()[0])") } + expect(postgres_card.evaluate_script("this.getBoundingClientRect().left")) + .to be > find(".plan-document").evaluate_script("this.getBoundingClientRect().right") + expect(middle.(opened_card)).to be_within(4).of(middle.(find("mark", text: "microservices architecture"))) + expect(middle.(postgres_card)).to be_within(4).of(middle.(find("mark", text: "PostgreSQL"))) + + # Hover links a marker and its text in both directions. + find("mark", text: "PostgreSQL").hover + expect(page).to have_css(".comment-margin__card.is-hover", count: 1) + expect(postgres_card[:class]).to include("is-hover") + opened_card.hover + expect(page).to have_css("mark.is-margin-hover", count: 1, text: "microservices architecture") + expect(postgres_card[:class]).not_to include("is-hover") + + postgres_card.click + within(".thread-popover:popover-open") { expect(page).to have_content("Consider MySQL") } + expect(page).to have_css(".comment-margin__card.is-active[data-thread-id='comment_thread_#{postgres.id}']") + ensure + page.current_window.resize_to(1400, 900) + end + it "does not toggle resolved visibility while typing s in a reply box" do open_thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Why not monolith?", user: reviewer) resolved = create_anchored_thread(plan: plan, anchor_text: "PostgreSQL", body: "Consider MySQL", user: reviewer) From 72272eea69c6955a00387d6d46a04943bba21a2d Mon Sep 17 00:00:00 2001 From: coltonw Date: Mon, 2 Nov 2026 09:27:02 -0500 Subject: [PATCH 2/2] Pin the lint job to Ubuntu 24.04 ubuntu-latest now resolves to Ubuntu 26.04, where ruby/setup-ruby has no Ruby 3.4.7 build, so the lint job failed before RuboCop ran. Co-authored-by: Amp Amp-Thread-ID: https://ampcode.com/threads/T-01a18e32-b8a4-7319-95b9-7279b536d0f1 --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 94fce7e9..cd9431de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,7 +19,8 @@ jobs: # test jobs rather than gating them: a bracket-spacing nit should not hide # whether the suite passes. lint: - runs-on: ubuntu-latest + # Pinned: setup-ruby has no Ruby 3.4.7 build for ubuntu-latest (26.04) yet. + runs-on: ubuntu-24.04 steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0