-
Notifications
You must be signed in to change notification settings - Fork 7
Fix comment form positioning and eliminate scroll-to-top #15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c9ee643
b8be8c7
30dbb2d
3e6da74
32d7e9e
33c14f8
2cb5c9c
e8bc5e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,35 +28,43 @@ def create | |
| ) | ||
|
|
||
| broadcast_new_thread(thread) | ||
| redirect_to plan_path(@plan), notice: "Comment added." | ||
| broadcast_tab_counts | ||
|
|
||
| respond_with_stream_or_redirect("Comment added.") | ||
| end | ||
|
|
||
| def resolve | ||
| authorize!(@thread, :resolve?) | ||
| @thread.resolve!(current_user) | ||
| broadcast_thread_update(@thread) | ||
| redirect_to plan_path(@plan), notice: "Thread resolved." | ||
| broadcast_thread_move(@thread, from: "comment-threads", to: "resolved-comment-threads") | ||
| respond_with_stream_or_redirect("Thread resolved.") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These status actions now avoid redirects, but there is no corresponding client-side reposition pass for the remaining open/resolved thread cards. Since cards are laid out with persisted inline Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — added a MutationObserver on the thread lists that automatically re-runs highlightAnchors() (which includes positionThreads()) whenever threads are added or removed via turbo stream broadcasts. This also let us remove the manual setTimeout in resetCommentForm. |
||
| end | ||
|
|
||
| def accept | ||
| authorize!(@thread, :accept?) | ||
| @thread.accept!(current_user) | ||
| broadcast_thread_update(@thread) | ||
| redirect_to plan_path(@plan), notice: "Thread accepted." | ||
| broadcast_thread_move(@thread, from: "comment-threads", to: "resolved-comment-threads") | ||
| 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." | ||
| broadcast_thread_move(@thread, from: "comment-threads", to: "resolved-comment-threads") | ||
| 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." | ||
| # 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 | ||
|
|
||
| private | ||
|
|
@@ -78,12 +86,64 @@ def broadcast_new_thread(thread) | |
| ) | ||
| end | ||
|
|
||
| def broadcast_thread_update(thread) | ||
| # Broadcasts update all clients (including the submitter) via WebSocket. | ||
| # The empty turbo_stream response prevents Turbo from navigating (which causes scroll-to-top). | ||
| def respond_with_stream_or_redirect(message) | ||
| respond_to do |format| | ||
| format.turbo_stream { render turbo_stream: [] } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Rendering an empty Turbo Stream response here makes the submitting browser depend entirely on WebSocket broadcasts for thread card HTML, but those broadcasts render Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a pre-existing issue — the broadcasts have never passed current_user, even before this PR. Our changes didn't introduce the permissive viewer.nil? fallback; they just made it more visible by relying on broadcasts instead of full-page redirects. Fixing this properly (per-user turbo streams or a separate broadcast-safe partial) is worth doing but out of scope here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This response path sends an empty Turbo Stream and relies entirely on WebSocket broadcasts for the initiating user to see the result. If the submitter’s ActionCable connection is temporarily down (reconnect window, blocked WS, etc.), the mutation succeeds server-side but the page shows no local update (and the form reset handlers still run), so users can lose immediate feedback until a manual refresh. Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For Turbo requests this helper now returns an empty stream ( Useful? React with 👍 / 👎. |
||
| format.html { redirect_to plan_path(@plan), notice: 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)) | ||
| Turbo::StreamsChannel.broadcast_append_to( | ||
| @plan, | ||
| target: to, | ||
| partial: "comment_threads/thread", | ||
| locals: { thread: thread, plan: @plan } | ||
|
Comment on lines
+112
to
116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a thread is moved between tabs, this code appends the rendered thread into the destination list but never removes the existing empty-state paragraph from the initial server render ( Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @codex addressed. can you write back on this comment thread if satisfactory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To use Codex here, create an environment for this repo. |
||
| ) | ||
| broadcast_tab_counts | ||
| end | ||
|
|
||
| def broadcast_tab_counts | ||
| threads = @plan.comment_threads | ||
| open_count = threads.active.count | ||
| resolved_count = threads.archived.count | ||
|
|
||
| Turbo::StreamsChannel.broadcast_update_to( | ||
| @plan, | ||
| target: "open-thread-count", | ||
| html: open_count > 0 ? open_count.to_s : "" | ||
| ) | ||
| Turbo::StreamsChannel.broadcast_update_to( | ||
| @plan, | ||
| target: "resolved-thread-count", | ||
| html: resolved_count > 0 ? resolved_count.to_s : "" | ||
| ) | ||
|
|
||
| # Toggle empty-state placeholders | ||
| Turbo::StreamsChannel.broadcast_replace_to( | ||
| @plan, | ||
| target: "open-threads-empty", | ||
| html: %(<p class="text-sm text-muted" id="open-threads-empty" #{'style="display: none;"' if open_count > 0}>No open comments.</p>) | ||
| ) | ||
| Turbo::StreamsChannel.broadcast_replace_to( | ||
| @plan, | ||
| target: "resolved-threads-empty", | ||
| html: %(<p class="text-sm text-muted" id="resolved-threads-empty" #{'style="display: none;"' if resolved_count > 0}>No resolved comments.</p>) | ||
| ) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,12 @@ def create | |
| locals: { comment: comment } | ||
| ) | ||
|
|
||
| redirect_to plan_path(@plan), notice: "Reply added." | ||
| # The broadcast above updates all clients (including the submitter) via WebSocket. | ||
| # The empty turbo_stream response prevents Turbo from navigating (which causes scroll-to-top). | ||
| respond_to do |format| | ||
| format.turbo_stream { render turbo_stream: [] } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why empty turbo streams for these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a comment for explanation. now there's no blue banner at the top. the comment appearing in the form seems confirmation enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For Turbo requests this now renders an empty stream and relies entirely on ActionCable delivery for the initiating user to see their own reply; if the websocket is disconnected or delayed, the comment is persisted but no UI change appears because there is no redirect and no DOM operation in the HTTP response. Returning the append/prepend as part of the Turbo response would preserve correctness for the actor even when cable delivery is unavailable. Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is broadcast_append_to not sufficient? when would that fail but format.turbo_stream work?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning the append in both the HTTP response and the broadcast would cause duplicate comments in the UI for the submitter (Turbo doesn't deduplicate appends). The WebSocket-down scenario is real but already degrades the entire real-time experience — other people's comments, thread moves, and tab count updates all rely on the same broadcast channel. If we want to harden against that, it's a broader concern (e.g., periodic polling fallback) rather than something to solve per-action. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The reply endpoint now also responds to Turbo with an empty stream body, which makes successful replies depend entirely on the ActionCable broadcast arriving; if the client is on Turbo Drive but unsubscribed/disconnected from the plan stream, the comment is persisted yet no comment appears in the UI, encouraging accidental re-submission. Useful? React with 👍 / 👎. |
||
| format.html { redirect_to plan_path(@plan), notice: "Reply added." } | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
|
@@ -68,27 +70,53 @@ 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" | ||
|
|
||
| // 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() | ||
| } | ||
| } | ||
|
|
||
| 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 +125,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" }) | ||
| } | ||
|
|
@@ -153,6 +185,14 @@ export default class extends Controller { | |
| } | ||
|
|
||
| highlightAnchors() { | ||
| // Remove existing anchor highlights before re-highlighting | ||
| this.contentTarget.querySelectorAll("mark.anchor-highlight").forEach(mark => { | ||
| const parent = mark.parentNode | ||
| while (mark.firstChild) parent.insertBefore(mark.firstChild, mark) | ||
| parent.removeChild(mark) | ||
| }) | ||
| this.contentTarget.normalize() | ||
|
|
||
| // Build full text once for position lookups | ||
| this.fullText = this.contentTarget.textContent | ||
|
|
||
|
|
@@ -168,24 +208,73 @@ 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) | ||
| } | ||
|
|
||
| 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 | ||
|
|
||
| // 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) { | ||
| 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)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @codex address that feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To use Codex here, create an environment for this repo.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed. The MutationObserver is now disconnected before positionThreads reorders the DOM (via appendChild), and reconnected after. This prevents the self-triggering loop. |
||
|
|
||
| // 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) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| <div class="comment-form card" id="new-comment-form" data-text-selection-target="form" style="display: none;"> | ||
| <%= form_with url: plan_comment_threads_path(plan), method: :post, data: { action: "turbo:submit-end->text-selection#resetCommentForm" } do |f| %> | ||
| <input type="hidden" name="comment_thread[anchor_text]" data-text-selection-target="anchorInput" value=""> | ||
| <input type="hidden" name="comment_thread[anchor_context]" data-text-selection-target="contextInput" value=""> | ||
| <div class="comment-form__anchor" data-text-selection-target="anchorPreview" style="display: none;"> | ||
| <span class="text-sm text-muted">Commenting on:</span> | ||
| <blockquote class="comment-form__quote" data-text-selection-target="anchorQuote"></blockquote> | ||
| </div> | ||
| <div class="form-group"> | ||
| <textarea name="comment_thread[body_markdown]" id="comment_thread_body_markdown" rows="3" placeholder="Write a comment..." required></textarea> | ||
| </div> | ||
| <div class="comment-form__actions"> | ||
| <button type="submit" class="btn btn--primary btn--sm">Comment</button> | ||
| <button type="button" class="btn btn--secondary btn--sm" data-action="text-selection#cancelComment">Cancel</button> | ||
| </div> | ||
| <% end %> | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <div class="comment-thread__reply" id="<%= dom_id(thread, :reply_form) %>"> | ||
| <%= form_with url: plan_comment_thread_comments_path(plan, thread), method: :post, data: { action: "turbo:submit-end->text-selection#resetReplyForm" } do |f| %> | ||
| <div class="form-group"> | ||
| <textarea name="comment[body_markdown]" rows="2" placeholder="Reply..." required></textarea> | ||
| </div> | ||
| <button type="submit" class="btn btn--secondary btn--sm">Reply</button> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path now returns an empty Turbo Stream response, but
resolve/accept/dismiss/reopenstill only broadcastreplacefor the existing thread DOM id, so the updated thread stays in its current list instead of moving between Open/Resolved tabs and the tab counts remain stale until a full reload. Before this change, the redirect toplans#showre-rendered both collections from server state, so Turbo users now get inconsistent thread state after status changes.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated