diff --git a/engine/app/javascript/controllers/coplan/comment_nav_controller.js b/engine/app/javascript/controllers/coplan/comment_nav_controller.js index a6109189..c7a15b39 100644 --- a/engine/app/javascript/controllers/coplan/comment_nav_controller.js +++ b/engine/app/javascript/controllers/coplan/comment_nav_controller.js @@ -128,6 +128,24 @@ export default class extends Controller { this.updatePosition() } + handleAnchorsUpdated() { + this.updatePosition() + if (!this.activeMark || !this.activePopover) return + + const threadId = this.activeMark.dataset.threadId + const replacementMark = this.allHighlights.find(mark => mark.dataset.threadId === threadId) + if (replacementMark && this.findOpenPopover() === this.activePopover) { + replacementMark.classList.add("anchor-highlight--active") + this.activeMark = replacementMark + this.positionPopoverAtMark(this.activePopover, replacementMark) + return + } + + try { this.activePopover.hidePopover() } catch {} + this.activeMark = null + this.activePopover = null + } + handleScroll() { if (!this.activeMark || !this.activePopover) return try { diff --git a/engine/app/javascript/controllers/coplan/mermaid_controller.js b/engine/app/javascript/controllers/coplan/mermaid_controller.js index fd4945be..28d219bf 100644 --- a/engine/app/javascript/controllers/coplan/mermaid_controller.js +++ b/engine/app/javascript/controllers/coplan/mermaid_controller.js @@ -33,19 +33,24 @@ export default class extends Controller { ] if (sources.length === 0) return - let mermaid - try { - mermaid = await loadMermaid() - } catch { - sources.forEach(({ container }) => this.showError(container)) - return - } - - const theme = configureMermaid(mermaid) const generation = ++this.renderGeneration - for (const { container, source } of sources) { - await this.renderDiagram(mermaid, container, source, theme, generation) + try { + const mermaid = await loadMermaid() + if (!this.element.isConnected || generation !== this.renderGeneration) return + + const theme = configureMermaid(mermaid) + for (const { container, source } of sources) { + await this.renderDiagram(mermaid, container, source, theme, generation) + } + } catch { + if (this.element.isConnected && generation === this.renderGeneration) { + sources.forEach(({ container }) => this.showError(container)) + } + } finally { + if (this.element.isConnected && generation === this.renderGeneration) { + this.element.dispatchEvent(new CustomEvent("coplan:mermaid-settled", { bubbles: true })) + } } } @@ -68,7 +73,9 @@ export default class extends Controller { } catch { document.getElementById(id)?.remove() document.getElementById(`d${id}`)?.remove() - this.showError(sourceContainer) + if (this.element.isConnected && generation === this.renderGeneration) { + this.showError(sourceContainer) + } } } diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js index 6296abc4..6f3b7167 100644 --- a/engine/app/javascript/controllers/coplan/text_selection_controller.js +++ b/engine/app/javascript/controllers/coplan/text_selection_controller.js @@ -215,6 +215,11 @@ export default class extends Controller { this._showThreadPopoverFor(event.currentTarget, "pinned") } + handleMermaidSettled() { + this.highlightAnchors() + if (this._pendingThreadId) this._openLinkedThread() + } + async copyThreadLink(event) { if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return @@ -274,6 +279,22 @@ export default class extends Controller { return true } + _restoreActiveThreadPopover(threadId, mode) { + if (!threadId || !this._activePopover) return + + const replacementMark = this.contentTarget.querySelector(`mark[data-thread-id="${threadId}"]`) + if (replacementMark && this._findOpenPopover() === this._activePopover) { + this._showThreadPopoverFor(replacementMark, mode || "pinned") + return + } + + this._detachPopoverHoverListeners(this._activePopover) + try { this._activePopover.hidePopover() } catch {} + this._activeMark = null + this._activePopover = null + this._openMode = null + } + handleMarkHoverEnter(mark) { // Already showing this exact popover — just keep it alive. if (this._activePopover && this._activeMark === mark) { @@ -511,6 +532,9 @@ export default class extends Controller { } highlightAnchors() { + const activeThreadId = this._activeMark?.dataset.threadId + const activeMode = this._openMode + // Remove existing anchor highlights before re-highlighting this.contentTarget.querySelectorAll("mark.anchor-highlight").forEach(mark => { const parent = mark.parentNode @@ -550,6 +574,7 @@ export default class extends Controller { } }) + this._restoreActiveThreadPopover(activeThreadId, activeMode) this.element.dispatchEvent(new CustomEvent("coplan:anchors-updated", { bubbles: true })) } @@ -657,11 +682,31 @@ export default class extends Controller { const domId = `comment_thread_${threadId}` const mark = this.contentTarget.querySelector(`mark[data-thread-id="${domId}"]`) + // Mermaid replaces its source block asynchronously. Wait for the rendered + // label instead of opening a popover against a source mark that will detach. + if (mark?.closest('pre[lang="mermaid"]')) { + if (attempt < 10) { + setTimeout(() => this._openLinkedThread(attempt + 1), 100) + } + return + } + if (mark) { - this._pendingThreadId = null requestAnimationFrame(() => { - mark.scrollIntoView({ behavior: "instant", block: "center" }) - this.openThreadPopover({ currentTarget: mark }) + if (this._pendingThreadId !== threadId) return + + const currentMark = this.contentTarget.querySelector(`mark[data-thread-id="${domId}"]`) + if (currentMark?.isConnected) { + currentMark.scrollIntoView({ behavior: "instant", block: "center" }) + if (this._showThreadPopoverFor(currentMark, "pinned")) { + this._pendingThreadId = null + return + } + } + + if (attempt < 10) { + setTimeout(() => this._openLinkedThread(attempt + 1), 100) + } }) return } diff --git a/engine/app/views/coplan/plans/show.html.erb b/engine/app/views/coplan/plans/show.html.erb index 8f003e51..8e7a34fa 100644 --- a/engine/app/views/coplan/plans/show.html.erb +++ b/engine/app/views/coplan/plans/show.html.erb @@ -67,7 +67,7 @@ -
+
@@ -117,7 +117,7 @@ <% open_count = @threads.count(&:open?) %> <% if @threads.any? %> -
+
💬 <%= open_count %> open
diff --git a/spec/system/comment_ux_spec.rb b/spec/system/comment_ux_spec.rb index b4765b0e..7262de43 100644 --- a/spec/system/comment_ux_spec.rb +++ b/spec/system/comment_ux_spec.rb @@ -177,6 +177,103 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) expect(page).to have_content("Why not monolith?") end + it "opens a popover for a comment anchored to a Mermaid label" do + plan.current_plan_version.update!(content_markdown: <<~MARKDOWN) + # Request flow + + ```mermaid + flowchart LR + Queue["assignment — first
fetching device wins"] --> Printer + ``` + MARKDOWN + thread = create_anchored_thread( + plan: plan, + anchor_text: "firstfetching device wins", + body: "Does this claim time out?", + user: reviewer + ) + + visit plan_path(plan) + + expect(page).to have_css(".mermaid-diagram svg", wait: 10) + mark_selector = ".mermaid-diagram mark[data-thread-id='comment_thread_#{thread.id}']" + find(mark_selector, match: :first, wait: 10).click + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + + initial_theme = find(".mermaid-diagram")["data-mermaid-theme"] + replacement_theme = initial_theme == "dark" ? "light" : "dark" + page.execute_script(<<~JS) + document.documentElement.dataset.theme = "#{replacement_theme}" + window.dispatchEvent(new CustomEvent("coplan:theme-changed")) + JS + expect(page).to have_css( + ".mermaid-diagram[data-mermaid-theme='#{replacement_theme}']", + wait: 10 + ) + + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + expect(page).to have_content("Does this claim time out?") + active_mark_is_rendered = page.evaluate_script(<<~JS) + (() => { + const layout = document.querySelector('[data-controller~="coplan--text-selection"]') + const controller = window.Stimulus.getControllerForElementAndIdentifier( + layout, + "coplan--text-selection" + ) + return controller._activeMark?.isConnected && + controller._activeMark.closest(".mermaid-diagram") !== null + })() + JS + expect(active_mark_is_rendered).to be(true) + end + + it "keeps a keyboard-opened Mermaid popover attached across re-renders" do + plan.current_plan_version.update!(content_markdown: <<~MARKDOWN) + ```mermaid + flowchart LR + Queue["assignment"] --> Printer + ``` + MARKDOWN + thread = create_anchored_thread( + plan: plan, + anchor_text: "assignment", + body: "Does this need a lease?", + user: reviewer + ) + + visit plan_path(plan) + + expect(page).to have_css(".mermaid-diagram svg", wait: 10) + find("body").send_keys("j") + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + + initial_theme = find(".mermaid-diagram")["data-mermaid-theme"] + replacement_theme = initial_theme == "dark" ? "light" : "dark" + page.execute_script(<<~JS) + document.documentElement.dataset.theme = "#{replacement_theme}" + window.dispatchEvent(new CustomEvent("coplan:theme-changed")) + JS + expect(page).to have_css( + ".mermaid-diagram[data-mermaid-theme='#{replacement_theme}']", + wait: 10 + ) + + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + active_mark_is_rendered = page.evaluate_script(<<~JS) + (() => { + const toolbar = document.querySelector('[data-controller~="coplan--comment-nav"]') + const controller = window.Stimulus.getControllerForElementAndIdentifier( + toolbar, + "coplan--comment-nav" + ) + return controller.activeMark?.isConnected && + controller.activeMark.dataset.threadId === "comment_thread_#{thread.id}" && + controller.activeMark.closest(".mermaid-diagram") !== null + })() + JS + expect(active_mark_is_rendered).to be(true) + end + it "opens a linked comment and exposes its permalink" do thread = create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Why not monolith?", user: reviewer) @@ -191,6 +288,95 @@ def create_anchored_thread(plan:, anchor_text:, body:, user:) end end + it "opens a linked comment anchored to a Mermaid label" do + plan.current_plan_version.update!(content_markdown: <<~MARKDOWN) + ```mermaid + flowchart LR + Queue["~3 min retries"] --> Printer + ``` + MARKDOWN + thread = create_anchored_thread( + plan: plan, + anchor_text: "~3 min retries", + body: "Is the retry window configurable?", + user: reviewer + ) + + visit plan_path(plan, thread: thread.id) + + expect(page).to have_css(".mermaid-diagram svg", wait: 10) + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + expect(page).to have_content("Is the retry window configurable?") + active_mark_is_rendered = page.evaluate_script(<<~JS) + (() => { + const layout = document.querySelector('[data-controller~="coplan--text-selection"]') + const controller = window.Stimulus.getControllerForElementAndIdentifier( + layout, + "coplan--text-selection" + ) + return controller._activeMark?.isConnected && + controller._activeMark.closest(".mermaid-diagram") !== null + })() + JS + expect(active_mark_is_rendered).to be(true) + end + + it "opens a pending linked comment after Mermaid rendering fails" do + plan.current_plan_version.update!(content_markdown: <<~MARKDOWN) + ```mermaid + not a valid diagram + ``` + MARKDOWN + thread = create_anchored_thread( + plan: plan, + anchor_text: "not a valid diagram", + body: "The source still needs feedback.", + user: reviewer + ) + + visit plan_path(plan) + + expect(page).to have_css(".mermaid-diagram--error", wait: 10) + page.execute_script(<<~JS) + const source = document.querySelector(".mermaid-diagram--error") + source.querySelector(".mermaid-diagram__error-message")?.remove() + source.classList.remove("mermaid-diagram--error") + source.setAttribute("lang", "mermaid") + + const layout = document.querySelector('[data-controller~="coplan--text-selection"]') + const textSelection = window.Stimulus.getControllerForElementAndIdentifier( + layout, + "coplan--text-selection" + ) + textSelection._pendingThreadId = "#{thread.id}" + textSelection._openLinkedThread(10) + + const markdown = document.querySelector('[data-controller~="coplan--mermaid"]') + const mermaid = window.Stimulus.getControllerForElementAndIdentifier( + markdown, + "coplan--mermaid" + ) + mermaid.renderDiagrams() + JS + + expect(page).to have_css(".mermaid-diagram--error", wait: 10) + expect(page).to have_css("#comment_thread_#{thread.id}_popover", visible: true) + expect(page).to have_content("The source still needs feedback.") + linked_source_is_stable = page.evaluate_script(<<~JS) + (() => { + const layout = document.querySelector('[data-controller~="coplan--text-selection"]') + const controller = window.Stimulus.getControllerForElementAndIdentifier( + layout, + "coplan--text-selection" + ) + return controller._pendingThreadId === null && + controller._activeMark?.isConnected && + controller._activeMark.closest(".mermaid-diagram--error") !== null + })() + JS + expect(linked_source_is_stable).to be(true) + end + it "shows reply form for open threads" do create_anchored_thread(plan: plan, anchor_text: "microservices architecture", body: "Why not monolith?", user: reviewer) visit plan_path(plan)