From c9ee64305097ce0ffa0470afe3e29546699abec7 Mon Sep 17 00:00:00 2001 From: Alice Kwan Date: Mon, 23 Feb 2026 16:13:49 -0800 Subject: [PATCH 1/8] Fix comment form positioning and eliminate scroll-to-top on submission Before: - Submitting a comment (new thread or reply) triggered a full Turbo Drive redirect to plan_path, which re-rendered the page with a flash notice at the top and scrolled the browser to the top of the page. - The new comment form always appeared at the top of the sidebar regardless of where the user selected text in the plan content. - Clicking an anchor quote in a comment thread scrolled to the first occurrence of that text in the plan, not the specific instance the user originally highlighted (no context disambiguation). - Comment threads in the sidebar were ordered by DOM insertion order (newest first from broadcasts), not by their anchor's vertical position in the plan document. - Switching between Open and Resolved tabs did not reposition threads next to their anchored text. After: - Comment and thread controllers respond with turbo_stream format (empty stream for the submitting browser, letting the existing WebSocket broadcast handle DOM updates) instead of redirecting. The page stays in place with no scroll and no flash banner. - The new comment form is absolutely positioned in the sidebar at the same vertical level as the selected text, using the popover's position as reference. Focus uses preventScroll to avoid any scroll jump. - After submission, the form hides and resets via a turbo:submit-end Stimulus callback, then re-runs highlightAnchors/positionThreads to pick up the newly broadcast thread. - Reply forms reset their textarea via a separate turbo:submit-end callback. - scrollToAnchor now reads data-anchor-context from the parent thread element and uses findAndHighlightWithContext for disambiguation, scrolling to the correct occurrence. - positionThreads now sorts threads by their anchor's vertical position in the document and reorders the DOM to match, so threads appear in the sidebar in the same order as their highlighted text in the plan. - Open and Resolved thread lists are positioned independently. Tab switches trigger repositionThreads with a short delay to allow the panel to become visible before measuring positions. Extracted partials: - comment_threads/_new_comment_form.html.erb (from plans/show.html.erb) - comment_threads/_reply_form.html.erb (from comment_threads/_thread.html.erb) Co-Authored-By: Claude Opus 4.6 --- app/controllers/comment_threads_controller.rb | 21 +++-- app/controllers/comments_controller.rb | 5 +- .../controllers/text_selection_controller.js | 88 ++++++++++++++++--- .../_new_comment_form.html.erb | 17 ++++ .../comment_threads/_reply_form.html.erb | 8 ++ app/views/comment_threads/_thread.html.erb | 9 +- app/views/plans/show.html.erb | 24 +---- 7 files changed, 124 insertions(+), 48 deletions(-) create mode 100644 app/views/comment_threads/_new_comment_form.html.erb create mode 100644 app/views/comment_threads/_reply_form.html.erb diff --git a/app/controllers/comment_threads_controller.rb b/app/controllers/comment_threads_controller.rb index 9d829e62..bae738fb 100644 --- a/app/controllers/comment_threads_controller.rb +++ b/app/controllers/comment_threads_controller.rb @@ -28,35 +28,39 @@ def create ) broadcast_new_thread(thread) - redirect_to plan_path(@plan), notice: "Comment added." + + respond_to do |format| + format.turbo_stream { render turbo_stream: [] } + format.html { redirect_to plan_path(@plan), notice: "Comment added." } + end end def resolve authorize!(@thread, :resolve?) @thread.resolve!(current_user) broadcast_thread_update(@thread) - redirect_to plan_path(@plan), notice: "Thread resolved." + respond_with_stream_or_redirect("Thread resolved.") end def accept authorize!(@thread, :accept?) @thread.accept!(current_user) broadcast_thread_update(@thread) - redirect_to plan_path(@plan), notice: "Thread accepted." + respond_with_stream_or_redirect("Thread accepted.") end def dismiss authorize!(@thread, :dismiss?) @thread.dismiss!(current_user) broadcast_thread_update(@thread) - redirect_to plan_path(@plan), notice: "Thread dismissed." + respond_with_stream_or_redirect("Thread dismissed.") end def reopen authorize!(@thread, :reopen?) @thread.update!(status: "open", resolved_by_user: nil) broadcast_thread_update(@thread) - redirect_to plan_path(@plan), notice: "Thread reopened." + respond_with_stream_or_redirect("Thread reopened.") end private @@ -78,6 +82,13 @@ def broadcast_new_thread(thread) ) end + def respond_with_stream_or_redirect(message) + respond_to do |format| + format.turbo_stream { render turbo_stream: [] } + format.html { redirect_to plan_path(@plan), notice: message } + end + end + def broadcast_thread_update(thread) Turbo::StreamsChannel.broadcast_replace_to( @plan, diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 350e61b2..218d4246 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -20,7 +20,10 @@ def create locals: { comment: comment } ) - redirect_to plan_path(@plan), notice: "Reply added." + respond_to do |format| + format.turbo_stream { render turbo_stream: [] } + format.html { redirect_to plan_path(@plan), notice: "Reply added." } + end end private diff --git a/app/javascript/controllers/text_selection_controller.js b/app/javascript/controllers/text_selection_controller.js index 44f5dc3a..68d37cb9 100644 --- a/app/javascript/controllers/text_selection_controller.js +++ b/app/javascript/controllers/text_selection_controller.js @@ -68,6 +68,13 @@ export default class extends Controller { : this.selectedText this.anchorPreviewTarget.style.display = "block" + // Position the form in the sidebar at the same vertical level as the selection + const layoutRect = this.element.getBoundingClientRect() + const popoverRect = this.popoverTarget.getBoundingClientRect() + const offsetTop = popoverRect.top - layoutRect.top + this.formTarget.style.position = "absolute" + this.formTarget.style.top = `${offsetTop}px` + // Show form, hide popover this.formTarget.style.display = "block" this.popoverTarget.style.display = "none" @@ -75,20 +82,43 @@ export default class extends Controller { // Clear browser selection window.getSelection().removeAllRanges() - // Focus textarea + // Focus textarea without scrolling the page const textarea = this.formTarget.querySelector("textarea") if (textarea) { - textarea.focus() - textarea.scrollIntoView({ behavior: "smooth", block: "center" }) + textarea.focus({ preventScroll: true }) } } cancelComment(event) { event.preventDefault() + this.hideAndResetForm() + } + + resetCommentForm(event) { + if (event.detail.success) { + this.hideAndResetForm() + // Re-highlight anchors and reposition threads after the broadcast adds the new thread + setTimeout(() => { + this.highlightAnchors() + }, 100) + } + } + + resetReplyForm(event) { + if (event.detail.success) { + const form = event.target + const textarea = form.querySelector("textarea") + if (textarea) textarea.value = "" + } + } + + hideAndResetForm() { this.formTarget.style.display = "none" this.anchorInputTarget.value = "" this.contextInputTarget.value = "" this.anchorPreviewTarget.style.display = "none" + const textarea = this.formTarget.querySelector("textarea") + if (textarea) textarea.value = "" this.selectedText = null this.selectedContext = null } @@ -97,13 +127,17 @@ export default class extends Controller { const anchor = event.currentTarget.dataset.anchor if (!anchor) return + const context = event.currentTarget.closest("[data-anchor-context]")?.dataset.anchorContext || "" + // Remove existing highlights first this.contentTarget.querySelectorAll(".anchor-highlight--active").forEach(el => { el.classList.remove("anchor-highlight--active") }) - // Find and highlight the anchor text - const highlighted = this.findAndHighlight(anchor, "anchor-highlight--active") + // Find and highlight the anchor text using context for disambiguation + const highlighted = context + ? this.findAndHighlightWithContext(anchor, context, "anchor-highlight--active") + : this.findAndHighlight(anchor, "anchor-highlight--active") if (highlighted) { highlighted.scrollIntoView({ behavior: "smooth", block: "center" }) } @@ -168,24 +202,50 @@ export default class extends Controller { this.positionThreads() } + repositionThreads() { + // Small delay to let the tab panel become visible before measuring positions + setTimeout(() => this.positionThreads(), 10) + } + positionThreads() { - const allThreads = Array.from(this.element.querySelectorAll(".comment-thread")) const sidebar = this.element.querySelector(".plan-layout__sidebar") - if (!sidebar || allThreads.length === 0) return + if (!sidebar) return + + this.positionThreadList("#comment-threads", sidebar) + this.positionThreadList("#resolved-comment-threads", sidebar) + } + + positionThreadList(selector, sidebar) { + const threadList = this.element.querySelector(selector) + if (!threadList) return + + const threads = Array.from(threadList.querySelectorAll(".comment-thread")) + if (threads.length === 0) return const sidebarRect = sidebar.getBoundingClientRect() + + // Sort threads by their anchor's vertical position in the document + threads.sort((a, b) => { + const markA = a._highlightMark + const markB = b._highlightMark + const yA = markA ? markA.getBoundingClientRect().top : Infinity + const yB = markB ? markB.getBoundingClientRect().top : Infinity + return yA - yB + }) + + // Reorder DOM within this list only + threads.forEach(thread => threadList.appendChild(thread)) + + // Position threads vertically const gap = 8 let cursor = 0 - allThreads.forEach(thread => { - const anchor = thread.dataset.anchorText + threads.forEach(thread => { + const mark = thread._highlightMark let desiredY = cursor - if (anchor && anchor.length > 0) { - const mark = thread._highlightMark - if (mark) { - desiredY = mark.getBoundingClientRect().top - sidebarRect.top + sidebar.scrollTop - } + if (mark) { + desiredY = mark.getBoundingClientRect().top - sidebarRect.top + sidebar.scrollTop } const y = Math.max(desiredY, cursor) diff --git a/app/views/comment_threads/_new_comment_form.html.erb b/app/views/comment_threads/_new_comment_form.html.erb new file mode 100644 index 00000000..30fb228b --- /dev/null +++ b/app/views/comment_threads/_new_comment_form.html.erb @@ -0,0 +1,17 @@ + diff --git a/app/views/comment_threads/_reply_form.html.erb b/app/views/comment_threads/_reply_form.html.erb new file mode 100644 index 00000000..a4d351e4 --- /dev/null +++ b/app/views/comment_threads/_reply_form.html.erb @@ -0,0 +1,8 @@ +
+ <%= form_with url: plan_comment_thread_comments_path(plan, thread), method: :post, data: { action: "turbo:submit-end->text-selection#resetReplyForm" } do |f| %> +
+ +
+ + <% end %> +
diff --git a/app/views/comment_threads/_thread.html.erb b/app/views/comment_threads/_thread.html.erb index ff66d51b..1dd13e49 100644 --- a/app/views/comment_threads/_thread.html.erb +++ b/app/views/comment_threads/_thread.html.erb @@ -26,14 +26,7 @@ <% is_thread_author = viewer.nil? || thread.created_by_user_id == viewer.id %> <% if thread.status == "open" %> -
- <%= form_with url: plan_comment_thread_comments_path(plan, thread), method: :post do |f| %> -
- -
- - <% end %> -
+ <%= render partial: "comment_threads/reply_form", locals: { thread: thread, plan: plan } %>
<% if is_plan_author || is_thread_author %> diff --git a/app/views/plans/show.html.erb b/app/views/plans/show.html.erb index d605d996..d6423970 100644 --- a/app/views/plans/show.html.erb +++ b/app/views/plans/show.html.erb @@ -16,30 +16,14 @@
- + <%= render partial: "comment_threads/new_comment_form", locals: { plan: @plan } %>
- -
@@ -56,7 +40,7 @@
From 33c14f8705693aca23fe0570b28b4335ef079239 Mon Sep 17 00:00:00 2001 From: Alice Kwan Date: Tue, 24 Feb 2026 10:12:08 -0800 Subject: [PATCH 6/8] Keep reopened out-of-date threads in the archived list Out-of-date threads remain in the archived scope even when reopened, since active requires out_of_date: false. Now reopen does an in-place replace instead of a cross-tab move when the thread is out of date. Co-Authored-By: Claude Opus 4.6 --- app/controllers/comment_threads_controller.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/app/controllers/comment_threads_controller.rb b/app/controllers/comment_threads_controller.rb index 1900c943..8712bf3c 100644 --- a/app/controllers/comment_threads_controller.rb +++ b/app/controllers/comment_threads_controller.rb @@ -57,7 +57,13 @@ def dismiss def reopen authorize!(@thread, :reopen?) @thread.update!(status: "open", resolved_by_user: nil) - broadcast_thread_move(@thread, from: "resolved-comment-threads", to: "comment-threads") + # Out-of-date threads stay in the archived list even when reopened, + # since the active scope excludes out_of_date rows. + if @thread.out_of_date? + broadcast_thread_replace(@thread) + else + broadcast_thread_move(@thread, from: "resolved-comment-threads", to: "comment-threads") + end respond_with_stream_or_redirect("Thread reopened.") end @@ -89,6 +95,17 @@ def respond_with_stream_or_redirect(message) end end + # Replaces a thread in place (status changed but stays in the same list). + def broadcast_thread_replace(thread) + Turbo::StreamsChannel.broadcast_replace_to( + @plan, + target: dom_id(thread), + partial: "comment_threads/thread", + locals: { thread: thread, plan: @plan } + ) + broadcast_tab_counts + end + # Moves a thread between Open/Resolved lists and updates tab counts. def broadcast_thread_move(thread, from:, to:) Turbo::StreamsChannel.broadcast_remove_to(@plan, target: dom_id(thread)) From 2cb5c9cd2e866340dc29869957a40856f4122745 Mon Sep 17 00:00:00 2001 From: Alice Kwan Date: Tue, 24 Feb 2026 10:30:29 -0800 Subject: [PATCH 7/8] Reposition sidebar threads automatically when DOM changes Added a MutationObserver on the thread lists that triggers highlightAnchors() (which includes positionThreads()) whenever threads are added or removed via turbo stream broadcasts. This fixes stale marginTop offsets after resolve/reopen/dismiss actions. The manual setTimeout in resetCommentForm is no longer needed. Co-Authored-By: Claude Opus 4.6 --- .../controllers/text_selection_controller.js | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/javascript/controllers/text_selection_controller.js b/app/javascript/controllers/text_selection_controller.js index 09291106..3d0f3baa 100644 --- a/app/javascript/controllers/text_selection_controller.js +++ b/app/javascript/controllers/text_selection_controller.js @@ -9,11 +9,13 @@ export default class extends Controller { this.contentTarget.addEventListener("mouseup", this.handleMouseUp.bind(this)) document.addEventListener("mousedown", this.handleDocumentMouseDown.bind(this)) this.highlightAnchors() + this.observeThreadLists() } disconnect() { this.contentTarget.removeEventListener("mouseup", this.handleMouseUp.bind(this)) document.removeEventListener("mousedown", this.handleDocumentMouseDown.bind(this)) + if (this.threadListObserver) this.threadListObserver.disconnect() } handleMouseUp(event) { @@ -97,10 +99,6 @@ export default class extends Controller { resetCommentForm(event) { if (event.detail.success) { this.hideAndResetForm() - // Re-highlight anchors and reposition threads after the broadcast adds the new thread - setTimeout(() => { - this.highlightAnchors() - }, 100) } } @@ -210,6 +208,19 @@ export default class extends Controller { this.positionThreads() } + // Re-highlight and reposition when threads are added/removed via turbo stream broadcasts + observeThreadLists() { + this.threadListObserver = new MutationObserver(() => { + // Debounce — multiple mutations may fire in quick succession + clearTimeout(this._repositionTimer) + this._repositionTimer = setTimeout(() => this.highlightAnchors(), 50) + }) + + this.element.querySelectorAll(".comment-threads-list").forEach(list => { + this.threadListObserver.observe(list, { childList: true }) + }) + } + repositionThreads() { // Small delay to let the tab panel become visible before measuring positions setTimeout(() => this.positionThreads(), 10) From e8bc5e4d42ef245b4a10fb2ec28a6dd9bf1715ff Mon Sep 17 00:00:00 2001 From: Alice Kwan Date: Tue, 24 Feb 2026 10:45:19 -0800 Subject: [PATCH 8/8] Pause MutationObserver during DOM reorder to prevent rehighlight loop positionThreadList uses appendChild to reorder threads, which triggers the MutationObserver and schedules another highlightAnchors() call, creating an infinite 50ms loop. Now the observer is disconnected before reordering and reconnected after. Co-Authored-By: Claude Opus 4.6 --- .../controllers/text_selection_controller.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/javascript/controllers/text_selection_controller.js b/app/javascript/controllers/text_selection_controller.js index 3d0f3baa..2ba5d751 100644 --- a/app/javascript/controllers/text_selection_controller.js +++ b/app/javascript/controllers/text_selection_controller.js @@ -230,8 +230,18 @@ export default class extends Controller { const sidebar = this.element.querySelector(".plan-layout__sidebar") if (!sidebar) return + // Pause observer while reordering DOM to avoid triggering a rehighlight loop + if (this.threadListObserver) this.threadListObserver.disconnect() + this.positionThreadList("#comment-threads", sidebar) this.positionThreadList("#resolved-comment-threads", sidebar) + + // Re-observe after reordering + if (this.threadListObserver) { + this.element.querySelectorAll(".comment-threads-list").forEach(list => { + this.threadListObserver.observe(list, { childList: true }) + }) + } } positionThreadList(selector, sidebar) {