Repository navigation
Render GitHub-style callouts for notes and warnings - #313
ThePumpingLemma wants to merge 10 commits into
Conversation
Enable Commonmarker's alerts extension so `> [!NOTE]`, `[!TIP]`, `[!IMPORTANT]`, `[!WARNING]` and `[!CAUTION]` blockquotes render as callouts with a per-type icon, label and tint in light and dark mode. The label is generated content, so the marker and label stay out of comment anchors and the text extractor. The rich editor keeps the marker line as a blockquote attribute and writes it back unchanged. Adds an agent guide section, a callout-gallery demo, a slide-spec conformance example, and bumps RENDER_CACHE_VERSION to 25.
|
🤖 @codex review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 585a65edc9
ℹ️ 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>
|
🤖 Engineering cleanup in
Validation: initial focused suite 128 examples passed; expanded rendering/browser/conformance/instruction coverage 154 passed; final deck-theme/browser/instruction run 62 passed; updated gallery seed/re-seed test passed; focused RuboCop passed. Real browser checks covered reader, comments, rich edits → Raw → saved reader, escaped/unknown syntax, and presenting a callout slide. No overlapping full-suite run was started. Current head: Hampton authorized engineering preparation and a local tryout, not acceptance or merging. The neutral Note/Warning accents follow the existing guide; whether static callouts should instead receive a semantic-color exception remains Hampton's product choice. Local preview is running at http://localhost:3313/hampton/demo-content/rich-text/callouts-notes-tips-and-warnings (login Final reader, light and dark: Slide contrast before and after, both with dark app mode: |
joahg
left a comment
There was a problem hiding this comment.
🤖 Approving. This works. Callouts are applied after sanitizing, and data-callout-title is only set through Nokogiri, so script and onerror payloads in custom titles come out escaped and forged callout* classes are stripped. About 30 source shapes (nested, in lists, custom titles, CRLF, lazy continuation, empty callouts) round-trip byte-identically through the editor parser and serializer. Text extraction still leaves the marker out of the text.
There is one rare data-loss path in the editor and one place where the generated title leaks into plain text (both inline). Plus a merge-order note: #314 also bumps RENDER_CACHE_VERSION to 25, so whichever of the two lands second needs to go to 26.
Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
|
🤖 Addressed the four review threads in
Validation: 125 focused examples passed; focused RuboCop passed. A real browser edit → Raw → Done round-trip preserved the setext callout and literal no-space quote. Saved results were checked in light and dark themes. All 15 CI checks passed on Try the isolated preview at http://localhost:3313/hampton/demo-content/rich-text/callouts-notes-tips-and-warnings#review-regression-checks (login: Cache coordination reserves #311=25, #313=26, #314=27. Before integration, recheck actual main/deployed cache values and choose a fresh version for the combined renderer; reservations alone do not make arbitrary merge order safe. This completes the requested engineering review fixes. Product acceptance remains with Hampton; no approval or merge was performed. Light theme after saving: Dark theme after saving: |
joahg
left a comment
There was a problem hiding this comment.
🤖 Re-approving at 5d0b08c (the push dismissed my earlier approval). I checked the fixes for all four threads: marker-bearing setext headings now take the preserveCallout fallback, recognition requires exactly > before the marker (ATX > # [!NOTE] and two-space or tab prefixes stay literal), markdown_to_plain_text drops p.markdown-alert-title, and the cache version is reserved at 26 with #314 at 27. Thanks!
Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
|
🤖 Merged current main (
Validation: 228 focused examples passed, then 18 affected browser examples passed after the assertion fix; 174 focused examples passed after integrating #298; focused RuboCop and diff checks passed. Local integration review found no remaining blocker. All 15 expected checks passed on Refreshed the existing isolated preview on port 3313. A separate headless Chrome for Testing 150 temporary profile exercised rich edit → Raw → Done and verified callout math and setext preservation. Captured the saved warning with math in both themes below. The preview database and server remain available. Try http://localhost:3313/hampton/demo-content/rich-text/callouts-notes-tips-and-warnings (login |
Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
Folds the summary-block idea from block#314 into callouts: a teal Summary callout for the top of long analysis plans. It is formatting only and does not touch the generated plan summary, link previews or unfurls.
Signed-off-by: Hampton Lintorn-Catlin <hampton@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed62239206
ℹ️ 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".
|
🤖 Prepared David’s SUMMARY revision ( The proposed Review found and fixed two parser issues: summaries opening on a list-item line disagreed with the rich editor, and adding a summary could reset an unrelated nested ordered list from 9 to 1. Coverage now includes bullet/numbered/lettered containers, source offsets, and a real rich edit → Raw → Done save. The agent guide states the portability limit: SUMMARY is a Coplan extension; other Markdown viewers can show its literal marker in an ordinary blockquote. Render-cache version is 32. Codex additionally found a table-only callout save could discard the table; Validation: 158 focused rendering/parser/browser/instruction examples passed after fixes; five-file RuboCop and diff checks passed. The earlier broad focused run had passing demo seed/re-seed cases but three failures (one corrected test assertion for synthetic newline offsets and two browser timing failures); the affected checks all passed in the follow-up. No full-suite run was started locally. Codex reviewed the substantive new behavior and found the two parser issues plus the table-only save issue; all three findings are fixed, replied to, and resolved. The final correction passed all 20 affected callout/math browser examples and focused lint. CI on The existing isolated preview now includes Summary at the top of the gallery. A separate headless Chrome for Testing profile verified its rich edit/save and captured both themes. Login Try Edit → change a Summary bullet → Raw → Done. The marker stays |
|
🤖 @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dcb1d4d43
ℹ️ 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>










What changed and why
Reviewers of long analysis plans asked for notes and warnings that stand out from the body text. Today a
>blockquote is the only option, and it looks like any quoted aside. This PR adds callouts using the GitHub alert syntax that agents already write:> [!NOTE] > [!TIP] > [!IMPORTANT] > [!WARNING] > [!CAUTION] > [!SUMMARY]Summary callout.
> [!SUMMARY]is a teal callout for a TL;DR at the top of a long plan, such as a STAMP analysis. It replaces Add summary blocks to plan Markdown #314. It is formatting only: the generated plan summary, link previews, OG tags and Slack unfurls are unchanged. comrak only knows GitHub's five alert types, soCoPlan::Markdownparses a summary marker as a same-length[!WARNING]alert and renames it after rendering. Offsets don't move. A marker that doesn't open an alert (inside code, or>[!SUMMARY]) stays literal. The rich editor keeps the marker like the other types. The callout gallery opens with one.RENDER_CACHE_VERSIONis now 32.Rendering. Commonmarker 2.6.3 exposes comrak's
alertsextension, so it is now on inMarkdownHelper::EXTENSION_OPTIONS. Every parser that shares those options (renderer, text extractor, source targets, slide classifier) agrees on what a callout is. A new post-sanitize pass,render_callouts, turns Commonmarker's alert markup intodiv.callout.callout--<type>and moves the title intodata-callout-title. The marker is case-insensitive, and> [!WARNING] Before you migratesets a custom label. Unknown markers such as> [!FOO]and plain>quotes still render as blockquotes.Styling. Each type has an icon and label, with a tinted background and border in both app modes. Static Note and Warning use neutral accents to reserve blue for interaction and orange for viewer attention. Tip, Important and Caution retain green, violet and red accents. Deck callouts use deck tokens so the artifact stays readable and unchanged across app modes. The label and icon are CSS generated content, so the document text does not change.
Comments. The
[!NOTE]marker is not in the rendered text or inMarkdownTextExtractoroutput, and the label is not DOM text. Selection, occurrence counting and comment anchors inside a callout work as they do for any paragraph.:alertcounts as a block for the extractor's newline separators.Sanitizing. No new tags or attributes are allowed for authors.
data-callout-titleis added after sanitization. Raw HTML cannot setcalloutclasses directly because the pass removes them first. Copying Commonmarker's own alert markup gives exactly what the marker gives.Rich editor. A callout edits as a rich blockquote with the same look. The marker line, including its case and any custom title, is kept as a node attribute and written back unchanged when the body is edited. A callout pasted within the editor keeps its marker. Escaped markers and markers after an opening blank quote line stay literal. An empty callout followed immediately by unquoted text, or a marker folded into a setext heading, uses the existing source-preview block to preserve its boundaries. Recognition requires exactly one space after
>; no-space markers remain plain quotes after edits.Slides. A callout bills one unit for its label plus its children, and it no longer triggers the pull-quote layout. A new conformance example is in
docs/SLIDE_SPEC.md.Docs and demo. There is a new "Callouts" section in the Markdown agent guide. The new callout-gallery demo ("Callouts: notes, tips, and warnings") uses all six types, including Summary, a custom label, a list and SQL inside a callout, plain, escaped and unknown-marker quotes for comparison, and callouts in Coplan and Graphite decks.
docs/DEMO_LIBRARY.mdlists it.RENDER_CACHE_VERSIONis bumped to 32 for the SUMMARY parser corrections, above the earlier callout renderer versions. Recheck freshness if another renderer change lands first.Evidence
Screenshots (light and dark) to be attached.
/tmp/coplan-shots/callouts/reader-light.png/tmp/coplan-shots/callouts/reader-dark.png/tmp/coplan-shots/callouts/reader-warning-caution-light.png/tmp/coplan-shots/callouts/reader-warning-caution-dark.png/tmp/coplan-shots/callouts/editor-light.png/tmp/coplan-shots/callouts/editor-dark.pngHow to try it
bin/rails db:seed, start the server, and sign in ashampton@example.com./hampton/demo-library. Under Tables and rich text, open Callouts: notes, tips, and warnings.[!QUESTION]quote at the end stay ordinary blockquotes.> [!WARNING]is unchanged.Testing
bundle exec rspec spec/helpers/markdown_helper_spec.rb spec/system/callouts_spec.rb spec/lib/development_seed_spec.rb:326: 89 examples, 0 failures after the summary callout was added. Screenshots:/tmp/coplan-shots/callouts/summary-{light,dark,editor-light}.png.bundle exec rspec spec/helpers/markdown_helper_spec.rb: each type, lowercase marker and custom label, plain blockquote, unknown[!FOO], lists, code and nested quotes inside a callout, callout in a list item, task checkboxes, forged classes and attributes stripped, non-interactive renders, and extractor text without the marker.bundle exec rspec spec/system/callouts_spec.rb: a real browser check of rendered labels, starting and saving a comment on text inside a callout (anchor position checked against the source), the JS parse/serialize round trip for every type and edge case, and typing in a callout in the rich editor, then Raw and Done, with the marker unchanged.bundle exec rspec spec/lib/development_seed_spec.rb: the callout gallery seeds once, renders all six types, and keeps a person's edits on re-seed.bundle exec rspec spec/services/slideshows/conformance_spec.rb: the new callout example.bundle exec rspec(full suite): 3410 examples, 1 failure. The failure isspec/system/agent_connections_spec.rb:16, a Chrome renderer/close-window timeout. It fails the same way onmain(30fe52b) on this machine, so it is not caused by this change.spec/system/references_spec.rb:183passed in this run.Light and dark mode checked in a running dev server; see the screenshots above.
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.)
Maintainer follow-up:
573d5e30fixes parser edge cases and deck contrast. See the cleanup comment for updated browser evidence and current-head checks.Generated with Codex