Fix comment form positioning and eliminate scroll-to-top#15
Conversation
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 <noreply@anthropic.com>
highlightAnchors() is now called after each comment submission (via resetCommentForm) to pick up newly broadcast threads. Without clearing existing <mark> elements first, each call wraps text in additional nested marks, accumulating duplicate highlights over time. Now unwraps existing mark.anchor-highlight elements and normalizes the text nodes before re-running the highlight pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex mind taking a look? |
| respond_to do |format| | ||
| format.turbo_stream { render turbo_stream: [] } | ||
| format.html { redirect_to plan_path(@plan), notice: "Comment added." } | ||
| end |
There was a problem hiding this comment.
why not use the helper set up below?
|
|
||
| redirect_to plan_path(@plan), notice: "Reply added." | ||
| respond_to do |format| | ||
| format.turbo_stream { render turbo_stream: [] } |
There was a problem hiding this comment.
why empty turbo streams for these?
There was a problem hiding this comment.
added a comment for explanation. now there's no blue banner at the top. the comment appearing in the form seems confirmation enough
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8be8c76cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @thread.resolve!(current_user) | ||
| broadcast_thread_update(@thread) | ||
| redirect_to plan_path(@plan), notice: "Thread resolved." | ||
| respond_with_stream_or_redirect("Thread resolved.") |
There was a problem hiding this comment.
This path now returns an empty Turbo Stream response, but resolve/accept/dismiss/reopen still only broadcast replace for 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 to plans#show re-rendered both collections from server state, so Turbo users now get inconsistent thread state after status changes.
Useful? React with 👍 / 👎.
|
|
||
| redirect_to plan_path(@plan), notice: "Reply added." | ||
| respond_to do |format| | ||
| format.turbo_stream { render turbo_stream: [] } |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
is broadcast_append_to not sufficient? when would that fail but format.turbo_stream work?
There was a problem hiding this comment.
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.
…ent empty turbo_stream pattern The create action in CommentThreadsController duplicated the respond_to block that already existed as respond_with_stream_or_redirect. Also added comments explaining why the turbo_stream responses return empty arrays: broadcasts handle the UI updates for all clients, and the empty response just prevents Turbo from navigating (which causes scroll-to-top). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ab counts Previously, resolve/accept/dismiss/reopen broadcast a replace on the thread's DOM id, leaving it in the wrong tab list with stale counts. Now broadcasts remove the thread from its current list and append it to the correct one, and tab counts are updated via targeted turbo stream updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex can you review again |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex I connected to github. can you review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6da74a26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Turbo::StreamsChannel.broadcast_append_to( | ||
| @plan, | ||
| target: dom_id(thread), | ||
| target: to, | ||
| partial: "comment_threads/thread", | ||
| locals: { thread: thread, plan: @plan } |
There was a problem hiding this comment.
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 (No resolved comments. / No open comments. in plans/show). If the destination tab was empty, resolving/reopening the first thread leaves contradictory UI (a real thread plus the “No … comments.” message) until a full reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex addressed. can you write back on this comment thread if satisfactory
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
The empty-state messages ("No open comments." / "No resolved comments.")
are now always present in the DOM (hidden when threads exist) and toggled
via turbo stream broadcasts alongside the tab count updates.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32d7e9e041
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @thread.update!(status: "open", resolved_by_user: nil) | ||
| broadcast_thread_update(@thread) | ||
| redirect_to plan_path(@plan), notice: "Thread reopened." | ||
| broadcast_thread_move(@thread, from: "resolved-comment-threads", to: "comment-threads") |
There was a problem hiding this comment.
Don't unconditionally move reopened threads into comment-threads: CommentThread.active excludes out_of_date rows while archived includes them, so reopening a resolved thread that is already out-of-date will be broadcast into the Open tab even though server-side scopes still classify it as archived. In that case the tab counts/empty states are computed from scopes and no longer match what was rendered, until a full reload re-sorts it back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
good catch. addressed.
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 <noreply@anthropic.com>
|
@codex review holistically, not just the most recent commit? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c14f8705
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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.
Rendering an empty Turbo Stream response here makes the submitting browser depend entirely on WebSocket broadcasts for thread card HTML, but those broadcasts render comment_threads/thread without current_user; in that partial, viewer.nil? is treated as plan-author permissions, so non-plan-authors immediately see controls like Accept/Dismiss/Reopen that they are not actually allowed to use. This is a regression in action visibility for any create/move update that now stays on-page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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.
| 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.
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 marginTop values from positionThreads(), removing/moving one thread leaves siblings with stale offsets until a manual tab switch triggers repositionThreads, which causes anchor/thread misalignment in normal resolve/reopen flows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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.
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 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cb5c9cd2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }) | ||
|
|
||
| // Reorder DOM within this list only | ||
| threads.forEach(thread => threadList.appendChild(thread)) |
There was a problem hiding this comment.
observeThreadLists watches each .comment-threads-list for childList mutations, and positionThreadList then unconditionally does appendChild on every thread during each highlight pass. With 2+ threads, those appends generate new childList mutations, which schedule another highlightAnchors() call, creating a continuous 50ms rehighlight/reposition cycle that can cause persistent CPU churn and UI jitter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Good catch — fixed. The MutationObserver is now disconnected before positionThreads reorders the DOM (via appendChild), and reconnected after. This prevents the self-triggering loop.
| # 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.
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 👍 / 👎.
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 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8bc5e4d42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 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.
For Turbo requests this helper now returns an empty stream (render turbo_stream: []), so the initiating browser only updates if its turbo_stream_from @plan ActionCable subscription is healthy; when WebSocket delivery is blocked or briefly disconnected, resolve/accept/dismiss/reopen/create complete server-side but the page shows no change, which can lead users to retry and submit duplicate actions.
Useful? React with 👍 / 👎.
| # 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: [] } |
There was a problem hiding this comment.
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 👍 / 👎.
2 participants
Summary
CommentsController,CommentThreadsController) now respond withturbo_streamformat instead ofredirect_to, preventing full Turbo Drive page navigation and scroll resetscrollToAnchornow usesanchor_contextto find the correct occurrence of highlighted text, not just the first match<mark>elements are unwrapped before re-runninghighlightAnchors()to prevent stacked/nested marks accumulating after each comment submissionWhat changed
Controllers:
CommentsController#createandCommentThreadsController#createreturn empty turbo_stream responses (WebSocket broadcast handles DOM updates)respond_with_stream_or_redirecthelper for the same patternStimulus (
text_selection_controller.js):openCommentFormpositions form absolutely near the selection using popover coordinatesresetCommentForm/resetReplyFormcallbacks onturbo:submit-endreset forms client-sidescrollToAnchorreadsdata-anchor-contextfor disambiguationpositionThreadssorts by anchor position, reorders DOM, positions both open and resolved lists independentlyrepositionThreadscalled on tab switch with delay for panel visibilityhighlightAnchorsclears existing marks before re-highlightingViews:
_new_comment_form.html.erband_reply_form.html.erbpartialsid="resolved-comment-threads"to resolved thread listtext-selection#repositionThreadson switchTest plan
Screen.Recording.2026-02-23.at.4.23.09.PM.mov
🤖 Generated with Claude Code