Skip to content

Reach discussions inside collapsed details sections - #315

Merged
HamptonMakes merged 4 commits into
block:mainfrom
ThePumpingLemma:dgrochowski/details-comments
Nov 9, 2026
Merged

HamptonMakes merged 4 commits into
block:mainfrom
ThePumpingLemma:dgrochowski/details-comments

Conversation

@ThePumpingLemma

@ThePumpingLemma ThePumpingLemma commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

What changed and why

Long analysis plans often put the evidence in collapsed <details> sections so the summary stays short. Discussions anchored to text inside one of those sections were effectively hidden:

  • The highlight existed in the DOM, so the thread was treated as placed and left out of the Comments list.
  • ?thread=<id> links and j/k scrolled to an invisible highlight. The deep-link path only opened the document's own Comments disclosure, not author <details>.
  • Chrome still reports client rects for content in a closed <details>, so the margin marker appeared, but next to the summary rather than the discussed text. Opening the section only re-positioned markers; it didn't refresh them.

This PR makes them reachable, entirely on the client:

  • Every jump to a thread opens its enclosing sections first. That covers ?thread=, j/k, the review tray's "View feedback", clicking an anchor quote, and any other path through _showThreadPopoverFor. Nested sections all open. The shared helper is engine/app/javascript/coplan/collapsed_sections.js. It replaces the special case for details.document-support__disclosure.
  • Closed sections show a count badge on their <summary>. The badge has the comment icon and the number of discussions inside, in the same compact blue as the outline's comment counts, in light and dark mode. Selecting it opens the section and the first discussion. Only open threads count by default; resolved ones count while "Show resolved" (s) is on. With nested sections, the badge sits on the outermost closed summary and moves inward as sections open. Table-cell discussions inside a closed section count too.
  • Opening or closing a section refreshes the margin. Markers appear for the discussions inside and the badge goes away, then the reverse when it closes. Discussions in a closed section get no margin marker.
  • The badge's count comes from CSS (attr(data-count)), so no text gets added to the document text that anchors are matched against.

The Markdown rendering is unchanged, so RENDER_CACHE_VERSION isn't bumped. The only server-side template change renames the margin controller's toggle action to toggled.

Docs: one line in the agent Markdown guide (the <details> section), a bullet in AGENTS.md under Inline review UI, and a row in docs/DEMO_LIBRARY.md.

Evidence

Screenshots (light and dark) to be attached.

Files in /tmp/coplan-shots/details-comments/:

  • 1-badges-collapsed-light.png
  • 1-badges-collapsed-dark.png
  • 2-thread-opened-light.png
  • 2-thread-opened-dark.png

The "badges collapsed" shots show two closed sections with counts 3 and 1. The "thread opened" shots show the result of selecting the first badge: the section is open, the first discussion's window is open beside its highlight, margin markers line up with the highlights now in view, and the nested "Query plan" section has its own badge.

How to try it

  1. bin/setup --skip-server (or bin/rails db:seed on an existing dev database), then bin/dev.
  2. Sign in as hampton@example.com and open /hampton/demo-library. Under Discussions, open collapsed-discussions ("Collapsed sections: discussions in the appendix").
  3. "Raw measurements" shows a badge with 3, and "Alternatives considered" shows 1. Press s and the second badge becomes 2, because it now counts the resolved thread. Press s again.
  4. Select the 3 badge. The section opens, along with Sam's discussion on "one discount rule query". The nested "Query plan" section now shows a 1 badge.
  5. Reload, then press j repeatedly. Each discussion opens in document order, and the sections open as you reach them.
  6. Reload and open "Alternatives considered" by clicking its summary. A margin marker appears beside "Precomputing prices nightly". Close the section and the marker disappears.
  7. Open a discussion from a collapsed section, use its Copy link, collapse the section, and visit the link. The section opens and shows the discussion.

Testing

  • bundle exec rspec spec/system/collapsed_section_comments_spec.rb passes (4 examples):

    • The badge shows the right count, follows resolved visibility, and opens the section at its first discussion.
    • ?thread= for a thread in a collapsed section opens it and shows the thread.
    • j/k reach threads in a collapsed section, including wrapping.
    • Opening a section manually adds margin markers aligned with their highlights, and closing it removes them.
    • Before the change, all four examples failed. The margin example showed a marker for a hidden thread.
  • bundle exec rspec spec/lib/development_seed_spec.rb passes (35 examples). One new example covers the collapsed-discussions initial state, the nested placement of its threads, and re-seeding preserving an edited, resolved thread.

  • Full suite, bundle exec rspec: 3398 examples, 1 failure. The failure is spec/system/agent_connections_spec.rb:16 ("chromedriver: failed to close window in 20 seconds"). It fails the same way on main (30fe52b) in this environment, and the code it exercises is unrelated. The existing comment and margin specs (comment_ux_spec, comment_anchoring_spec, api_comment_anchors_spec, source_comments_spec, deck_ux_spec) all passed in that run.

  • Checked by hand on the seeded demo in light and dark mode.

  • Added or updated real JavaScript-enabled browser specs in spec/system/ for changed UI behavior, covering the user interaction and visible result; or explained why no browser UI is involved.

  • Added model, service, or request specs where needed for domain behavior, authorization, or API cases.

  • Ran bundle exec rspec, or explained why it could not run and listed the specs that did run.

  • Tested affected UI behavior in both light and dark mode, or explained why mode testing does not apply.

  • Attached a video or images demonstrating the change, if this PR adds or changes a feature or fixes a UI bug. (Screenshots to be attached by hand.)

1-badges-collapsed-dark 1-badges-collapsed-light 2-thread-opened-dark 2-thread-opened-light

A closed <details> in a plan hid the discussions anchored inside it:
the highlight existed, so the thread was left out of the Comments list,
but the reader could not see it, ?thread= links and j/k scrolled to an
invisible mark, and Chrome still reported rects, so the margin marker
sat beside the wrong text.

- Every jump to a thread (?thread=, j/k, the review tray, anchor quotes,
  any popover open) opens the enclosing sections first.
- Closed sections show a compact blue count badge on their summary
  instead of margin markers; it opens the section at the first
  discussion. Resolved threads count only while they are shown.
- Opening or closing a section refreshes the margin markers.
- Demo library: collapsed-discussions, with a nested section.
@ThePumpingLemma
ThePumpingLemma force-pushed the dgrochowski/details-comments branch from b011027 to de63df5 Compare November 7, 2026 03:49
@ThePumpingLemma
ThePumpingLemma marked this pull request as ready for review November 7, 2026 03:50
@HamptonMakes

Copy link
Copy Markdown
Collaborator

🤖 @codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-07T04:03:41.917096Z de63df5 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de63df5b19

ℹ️ 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".

Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
@HamptonMakes

HamptonMakes commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

🤖 Engineering cleanup for Hampton: pushed b9378992 to fix table/diagram discussion links inside collapsed sections. The source path now opens every enclosing section before scrolling; the nested-table regression failed before the fix and passes afterward. Updated the served Markdown guide.

  • Validation: 66 focused examples passed (collapsed sections, source comments, shared demo seeds), then 64 examples passed with the final disclosure assertions and agent-instruction checks. RuboCop and diff checks passed. No full-suite overlap with other workers.
  • Review: local review and Codex found the same source-link defect; fixed once and the addressed thread is resolved. No further engineering findings. Current head is MERGEABLE / BLOCKED; all 15 current-head checks passed and required human review remains outstanding.
  • Browser: exercised the shared demo in light and dark mode, nested badge opening, j navigation, resolved counts, table deep links, and R-to-reply focus. Counts represent discussions, not unread attention; this change adds no orange attention state.
  • TRY THIS: http://localhost:3315/hampton/demo-content/discussions/collapsed-sections-discussions-in-the-appendix — sign in as hampton@example.com, no password. Select Raw measurements’ 4 badge (includes one local table test comment), try the nested Query plan badge, or press J. S includes resolved discussions. The isolated preview remains running.

Hampton authorized preparation and routine fixes; no approval or merge was requested or performed. Product acceptance remains Hampton’s: confirm that a summary badge opening the first discussion and automatic expansion on intentional navigation feel right.

Light: section opened at its discussion
Dark: table deep link reveals its section and discussion

@joahg joahg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving. This works: revealSections covers every route to a thread (popover, ?thread=, j/k nav, scrollToAnchor, review tray, and the source-comment deep link fixed in b937899), nested sections and marks inside a <summary> are handled, and the badge adds no text node, so anchor matching is unchanged.

One a11y fix I'd like before or soon after merge, plus a few small things inline. None of them block.

One gap that #316 already covers: a thread opened into a collapsed section gets a margin card with no is-active. The popover toggle fires before the rAF that builds the card, and refresh() never re-applies the active state. #316's activeId + markActive in refresh() fixes this, so keep it when rebasing #316 onto this.

Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
@HamptonMakes

HamptonMakes commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

🤖 Addressed joahg’s review in 00778d25:

The focus, 20px target, active-card race, and undefined-thread dispatch all reproduced on the prior code and pass with these fixes. The collapsed-section/source-comment run passed 36 examples; the final 11 collapsed-section examples passed after tightening the stale-thread regression. RuboCop and diff checks passed. Light/dark browser checks verified Enter activation, summary focus after Escape, and the active margin marker. Table panels retain their existing cell-focus return behavior.

All three new review threads are answered and resolved. All 15 current-head CI checks passed. GitHub reports MERGEABLE / BLOCKED; human acceptance remains outstanding. No further local review findings. Hampton authorized engineering follow-up, not acceptance or merge; that gate remains open.

TRY THIS: http://localhost:3315/hampton/demo-content/discussions/collapsed-sections-discussions-in-the-appendix — hampton@example.com, no password. Tab to the badge beside “Alternatives considered” and press Enter. Its discussion opens, the summary keeps focus, and the margin avatar becomes active. Escape closes the discussion without losing your place. The isolated tmux preview remains running.

Light: summary retains keyboard focus and the margin marker is active
Dark: summary retains keyboard focus and the margin marker is active

@joahg joahg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Re-checked at 00778d2 and still approve. The badge now hands focus to its <summary> before the section opens, and its label says "open the first". The guard on missing thread data blocks the undefined dispatch. The badge has a 24px minimum target, and refresh() reconciles is-active with the open popover, so #315 no longer depends on #316 for that.

@HamptonMakes
HamptonMakes merged commit a247cc9 into block:main Nov 9, 2026
15 checks passed
@HamptonMakes

Copy link
Copy Markdown
Collaborator

🤖 Post-merge verification complete. Hampton merged #315 on November 9, 2026 at 15:25 UTC as a247cc983e02a6e539a3d8ed8d2b536bfc0d890c. All 12 jobs in the merge-commit CI run passed, including MySQL/PostgreSQL, browser shards, and browser stress runs. No post-merge failures found.

Release-note evidence: Discussions inside collapsed sections are now reachable through summary count badges, discussion links, and keyboard navigation. Intentional navigation opens enclosing sections, including nested sections and table/diagram discussions. Badges respect resolved visibility, preserve keyboard focus, offer a minimum 24px target, and keep newly revealed margin markers visibly active. This is merged code, not a claim that a gem release or deployment has occurred.

Evidence: review cleanup, focused tests, and light/dark screenshots. The isolated preview at http://localhost:3315 remains running in tmux (-L coplan-pr315, session preview), with its existing data preserved. No further human action remains for this worker; archiving the chat without adding another tryout gate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants