Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
31 changes: 19 additions & 12 deletions engine/app/javascript/controllers/coplan/mermaid_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }))
}
}
}

Expand All @@ -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)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -550,6 +574,7 @@ export default class extends Controller {
}
})

this._restoreActiveThreadPopover(activeThreadId, activeMode)
this.element.dispatchEvent(new CustomEvent("coplan:anchors-updated", { bubbles: true }))
}

Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions engine/app/views/coplan/plans/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
</nav>
<button type="button" class="content-nav-show-btn" data-action="coplan--content-nav#toggle" data-coplan--content-nav-target="showBtn" title="Show table of contents (])">☰ <kbd>]</kbd></button>

<div class="plan-layout__content" data-coplan--text-selection-target="content" data-coplan--content-nav-target="content">
<div class="plan-layout__content" data-coplan--text-selection-target="content" data-coplan--content-nav-target="content" data-action="coplan:mermaid-settled->coplan--text-selection#handleMermaidSettled">
<div id="plan-content-body"
data-controller="coplan--live-update"
data-coplan--live-update-revision-value="<%= @plan.current_revision %>">
Expand Down Expand Up @@ -117,7 +117,7 @@

<% open_count = @threads.count(&:open?) %>
<% if @threads.any? %>
<div class="comment-toolbar" data-controller="coplan--comment-nav" data-coplan--comment-nav-plan-id-value="<%= @plan.id %>">
<div class="comment-toolbar" data-controller="coplan--comment-nav" data-coplan--comment-nav-plan-id-value="<%= @plan.id %>" data-action="coplan:anchors-updated@document->coplan--comment-nav#handleAnchorsUpdated">
<span class="comment-toolbar__count">💬 <%= open_count %> open</span>
<div class="comment-toolbar__nav">
<button class="btn btn--secondary btn--sm" data-action="coplan--comment-nav#prev" title="Previous comment">↑</button>
Expand Down
186 changes: 186 additions & 0 deletions spec/system/comment_ux_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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<br/>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)

Expand All @@ -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)
Expand Down
Loading