spec: <details>/<summary> support in markdown rendering (#10259)#13345
spec: <details>/<summary> support in markdown rendering (#10259)#13345seobew wants to merge 5 commits into
Conversation
… rendering Adds specs/GH10259/product.md (12 testable behavior invariants covering collapsible rendering, malformed-input degradation, deterministic depth/count limits, accessibility, round-trip serialization, and streaming) and tech.md (FormattedTextLine::Details container variant following the Table precedent, parse_markdown block branch, imported-HTML path, editor mapping, and a per-invariant test matrix). Issue: warpdotdev#10259 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @seobew on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Every PR must be linked to a same-repo issue before Oz can review it. Next step: open or find a same-repo issue describing this change, then link it to this PR by adding See the contribution guidelines for the full readiness model. Powered by Oz |
github-actions
Bot
added
the
external-contributor
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
Next step: open or find a same-repo issue describing this change, then link it to this PR by adding Closes #123 to the PR description (or using the "Development" sidebar on GitHub). A maintainer will mark the issue ready-to-spec when it is ready. Once it is marked, comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for rendering HTML5 <details>/<summary> blocks in Warp markdown surfaces. The specs cover the core behavior, parser shape, serialization, malformed-input fallback, resource bounds, and test matrix, but several requirements are still underspecified enough to create incompatible implementations.
Concerns
- The product spec does not define deterministic handling for malformed
<summary>tags, even though malformed summary behavior is part of the stated scope. - The editor-buffer mapping does not define how details body boundaries and nesting survive in the flat buffer model, which is necessary for reliable markdown serialization.
- Product invariant 11 says non-interactive preview surfaces are enumerated in the tech spec, but the tech spec leaves that enumeration as an open question.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| 1. A block-level `<details>` element whose content is markdown renders as a collapsible section: a summary line with a disclosure indicator (▸/▾), followed by the body when expanded. | ||
| 2. The section is collapsed by default. If the `<details>` tag carries the standard `open` attribute, it renders expanded initially. | ||
| 3. Clicking the summary line (or pressing Enter/Space while it is focused) toggles the section. The toggle target is keyboard-focusable and exposed to accessibility APIs as an activatable disclosure control. Any accessibility IDs are renderer-generated; they are never derived from input attributes. | ||
| 4. The first `<summary>` child provides the summary line, rendered with inline markdown styling. If no `<summary>` is present, the summary line is the localized literal "Details". Any additional `<summary>` siblings are treated as ordinary body content. |
There was a problem hiding this comment.
<summary> tags, such as an unclosed <summary> inside <details>. The PR description calls this out, but the spec only covers missing and duplicate summaries, leaving fallback behavior ambiguous.
|
|
||
| In `crates/editor/src/content/markdown.rs`: | ||
|
|
||
| - New `BufferBlockStyle::Details { default_open }` mapping, with the summary as the block's first line and body lines following, mirroring how `Table` blocks carry structured payloads. |
There was a problem hiding this comment.
to_markdown cannot reliably know which following lines belong before emitting </details>.
| - Serialization back to markdown (`to_markdown`) emits `<details>`/`<summary>` markup, adding ` open` when `default_open` (invariant 10). | ||
| - The disclosure widget itself (collapse/expand, focus, click, Enter/Space handling, renderer-generated accessibility IDs) lives in the block renderer layer. Open/collapsed is per-view UI state initialized from `default_open`, not part of `FormattedDetails` equality — so toggling does not dirty the text delta. | ||
|
|
||
| **Open question for maintainers:** whether the renderer should reuse the existing block-folding interaction machinery (as used for command blocks) or introduce a dedicated disclosure component; the spec constrains behavior (product invariants 1-3, 11), not the component choice. Similarly, the exact list of non-interactive preview call sites (invariant 11) needs a maintainer-confirmed enumeration; candidates are the conversation/agent-history preview renderers that already render markdown without hit-testing. |
There was a problem hiding this comment.
…trip, surface enumeration - product.md: deterministic rules for malformed <summary> (unclosed, stray closer, attributes, inline-only content); invariant 11 rewritten as two-tier rendering with static fallback as the default. - tech.md: buffer round-trip concretized via DETAILS_BLOCK_MARKDOWN_LANG internal code blocks following the TABLE_BLOCK_MARKDOWN_LANG precedent (boundaries via fences, nesting as literal markdown re-parsed under the depth cap); rendering surfaces enumerated per call site (interactive: agent conversation path; static fallback: all other parse_markdown consumers); test matrix extended for 8d-8f and internal-format round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed all three review points in 2d2b79f:
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for rendering HTML5 <details>/<summary> blocks in Warp markdown surfaces. The product behavior covers malformed input, bounded nesting/widget counts, view-state semantics, serialization, static fallback rendering, and streaming. No approved implementation spec context was attached, so there is no implementation-vs-spec drift to evaluate.
Concerns
- The parser design does not specify how the details tag scanner interacts with markdown code fences or other literal regions, which can cause tags inside body code blocks to rebalance or close the container incorrectly.
- The proposed editor/internal serialization requires verbatim body markdown even though the proposed data model stores only parsed
FormattedText, so the round-trip contract is not implementable as written. - The static fallback tier claims to enumerate all non-interactive
parse_markdownconsumers, but the current checkout has additional consumers that are not assigned handling requirements.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| Add `parse_details` to the `alt((...))` block chain in `crates/markdown_parser/src/markdown_parser.rs`, before `parse_paragraph`: | ||
|
|
||
| - Matches only at block start (spec invariant 8c): optional leading spaces, `<details` + optional attributes (`open` recognized, others ignored) + `>`. | ||
| - Scans forward for the matching `</details>` with a tag-balance counter; on success, extracts the first top-level `<summary>…</summary>` (invariant 4) and recursively calls `parse_markdown` on the body with a `depth` parameter. |
There was a problem hiding this comment.
</details> inside the body code sample can prematurely close the container and violate the “body content parses as normal markdown” invariant.
|
|
||
| The buffer round-trip follows the `FormattedTable` mechanism exactly. Tables survive the flat buffer model as dedicated internal code blocks (`TABLE_BLOCK_MARKDOWN_LANG = "warp-markdown-table"`, markdown_parser.rs:39, with `to_internal_format`/`from_internal_format`, lib.rs:361-430); details blocks get the analogous `DETAILS_BLOCK_MARKDOWN_LANG = "warp-markdown-details"` internal representation: | ||
|
|
||
| - **Internal format**: line 1 is a header carrying the `open` flag and the summary serialized as inline markdown (tab-separated, like the table format); lines 2..N are the body's markdown source verbatim — including any nested `<details>` markup as literal text. `FormattedDetails::to_internal_format`/`from_internal_format` in `crates/markdown_parser/src/lib.rs` own this, mirroring `FormattedTable`. |
There was a problem hiding this comment.
FormattedDetails model only keeps body: FormattedText, which loses the original markdown source. Add raw-source storage or specify canonical re-serialization so invariant 10 can be implemented.
|
|
||
| **Interactive tier (initial implementation)** — the agent conversation block path: `app/src/ai/agent/util.rs:35` (`parse_markdown_into_text_and_code_sections`, via `parse_markdown_with_gfm_tables` at util.rs:189). This is where agent output renders and where the disclosure widget is implemented. | ||
|
|
||
| **Static-fallback tier** — all other current `parse_markdown` call sites render summary-then-body expanded with no widget. Enumerated at time of writing: |
There was a problem hiding this comment.
parse_markdown consumers; the checkout also has consumers such as notebooks, editor model conversion, CLI-agent rendering, agent API conversion, and skill parsing. Update this section so every new FormattedTextLine::Details consumer has an explicit widget or fallback path.
…source, structural fallback - product.md: invariant 8 sub-items lettered (a)-(g); new (g): tags inside fenced code blocks are literal and never open/close a container; recognition only at block start outside fences. - tech.md: closing-tag matching specified as a fence-aware line walk (no raw text scan); FormattedDetails gains body_source (verbatim slice) so the warp-markdown-details round-trip is implementable without reconstructing markdown from parsed FormattedText; rendering fallback anchored structurally at the two shared sinks (FormattedTextElement in warpui_core + editor buffer conversion) instead of per-surface enumeration, with the complete 18-file parse_markdown consumer list included as informative reference; test matrix extended (8g, byte-exact round-trip with fenced bodies). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed all three points in a74d6ae:
/oz-review |
|
I'm re-reviewing this spec-only pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for rendering <details>/<summary> markdown as collapsible sections, with explicit parser, editor round-trip, renderer fallback, streaming, accessibility, and validation expectations.
Concerns
- No blocking completeness, clarity, feasibility, issue-alignment, internal-consistency, or high-level security concerns found in the changed specs. The specs define deterministic malformed-input behavior, bounded handling for untrusted markdown, generic static fallback behavior for non-interactive surfaces, round-trip behavior, and parser/editor/renderer test coverage.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
hey @bnavetta - can you take a look at this? you have much more context on markdown rendering than i do. |
| 1. A block-level `<details>` element whose content is markdown renders as a collapsible section: a summary line with a disclosure indicator (▸/▾), followed by the body when expanded. | ||
| 2. The section is collapsed by default. If the `<details>` tag carries the standard `open` attribute, it renders expanded initially. | ||
| 3. Clicking the summary line (or pressing Enter/Space while it is focused) toggles the section. The toggle target is keyboard-focusable and exposed to accessibility APIs as an activatable disclosure control. Any accessibility IDs are renderer-generated; they are never derived from input attributes. | ||
| 4. The first `<summary>` child provides the summary line, rendered with inline markdown styling. If no `<summary>` is present, the summary line is the localized literal "Details". Any additional `<summary>` siblings are treated as ordinary body content. |
There was a problem hiding this comment.
By "ordinary body content", do you mean that the text would be shown as-is, with the <summary> tags, or would the tags be omitted?
There was a problem hiding this comment.
Good catch — the spec did not actually say. Clarified in 2632ed6: the tags render as visible plain text and the inner content as normal markdown, consistent with 8(b)/8(e) and with the principle that markup the parser did not consume is never silently dropped.
| 4. The first `<summary>` child provides the summary line, rendered with inline markdown styling. If no `<summary>` is present, the summary line is the localized literal "Details". Any additional `<summary>` siblings are treated as ordinary body content. | ||
| 5. The body content between `<summary>` (or `<details>` when no summary exists) and `</details>` is parsed as normal markdown: paragraphs, code blocks, lists, tables, and nested `<details>` all render as they would at top level. | ||
| 6. Nesting is supported up to a fixed depth of 8. This limit is deterministic: a `<details>` opening at depth 9 or greater is not rendered as a widget — its tags are rendered as plain text and its content rendered inline (current behavior), and this fallback applies consistently regardless of input size or timing. | ||
| 7. A document renders at most 512 details widgets. From the 513th onward, the same deterministic plain-text fallback as (6) applies. Both limits exist to bound resource use against untrusted input; they are constants, not heuristics. |
There was a problem hiding this comment.
Could you elaborate on this a bit? The cost of another level of <details> nesting or an additional <details> block should be fairly comparable to the cost of tracking the same content as plain text
There was a problem hiding this comment.
You are right that the content cost is comparable — the limits are not about content size. What each widget adds beyond its content is (a) a level of parser recursion (the depth limit) and (b) a keyboard-focusable element with an accessibility-tree node and event bookkeeping (the count limit). Invariant 7 now states this explicitly. Happy to adjust the constants if you would prefer different bounds.
| - (g) Details tags never take effect inside literal regions: a `<details>`, `</details>`, or `<summary>` appearing within a fenced code block is code content and neither opens nor closes a container. Outside fenced code, opening and closing tags are recognized only at the start of a block (consistent with (c)), so tags embedded mid-paragraph are plain text. | ||
| 9. Collapsed state is view state, not content: copying the block, `raw_text()` extraction, and buffer conversion always include summary and full body regardless of the current collapsed state. | ||
| 10. Round-trip: converting the rendered block to markdown (e.g. copying an agent message as markdown) emits `<details>`/`<summary>` markup, preserving the `open` attribute only if the block was expanded by default in the source. | ||
| 11. Rendering is two-tier. The interactive disclosure widget (invariants 1-3) is implemented in the agent conversation block renderer — the surface the issue targets. Every other markdown surface uses a generic static fallback: the block renders expanded, in source order (summary line first, then body), with no disclosure indicator and no interaction. The fallback is the default for any renderer that has not opted into the widget, so no surface can render details in an undefined state. The concrete call sites for both tiers are enumerated in the tech spec. |
There was a problem hiding this comment.
I think it's worth speccing out support in the notebook/plan renderer as well (RichTextEditorView). For plans in particular, support for collapsed content would be useful.
There was a problem hiding this comment.
Agreed — and the buffer-native model from your §4 comment makes this nearly free: details regions now live directly in editor buffers with HiddenLinesModel folding, so RichTextEditorView becomes the second interactive-tier surface rather than a static-fallback consumer. Product invariant 11 and tech §5 are updated to commit to it (2632ed6).
| // FormattedTextLine::Details(FormattedDetails) | ||
| ``` | ||
|
|
||
| `body_source` is the verbatim input slice between the end of the summary and the closing tag. It exists so the buffer/markdown round-trip (§4) serializes the body exactly, without reconstructing markdown from parsed `FormattedText`; `body` is derived from it (`parse_markdown(body_source)`) and used only for rendering. Cost: one extra copy of the body text per details block, bounded by input size. `PartialEq` derives over all fields; equal `body_source` implies equal `body`, so streaming delta comparisons stay consistent. |
There was a problem hiding this comment.
Generally, Warp's rich-text pipeline doesn't attempt to guarantee exact preservation of the source Markdown; I think it's fine to continue that here
There was a problem hiding this comment.
Makes sense. Dropped body_source entirely; round-trip is now canonical re-serialization from parsed structure, and product invariant 10 is reworded to match the pipeline-wide guarantee (structure and content preserved, exact source bytes not). This also removes the duplicated body storage and its equality subtleties. (2632ed6)
|
|
||
| `body_source` is the verbatim input slice between the end of the summary and the closing tag. It exists so the buffer/markdown round-trip (§4) serializes the body exactly, without reconstructing markdown from parsed `FormattedText`; `body` is derived from it (`parse_markdown(body_source)`) and used only for rendering. Cost: one extra copy of the body text per details block, bounded by input size. `PartialEq` derives over all fields; equal `body_source` implies equal `body`, so streaming delta comparisons stay consistent. | ||
|
|
||
| The whole `<details>…</details>` region becomes a single `FormattedTextLine`, like `Table`. Rationale over start/end marker lines: markers would let intervening edits produce unbalanced documents, and every renderer/serializer consumer would need to track container state; a single variant keeps the flat model's invariants intact. |
There was a problem hiding this comment.
The approach taken by Table is very much an intermediate implementation to keep our options open for eventually supporting table editing. We shouldn't try to replicate it here - details regions should be modeled directly in the buffer. Among other things, that approach makes text selection much more complicated (which is true of tables regardless).
Given that you're supporting nested details items, I'd look at lists for inspiration instead (specifically ordered lists). Details items are kind of like ordered list markers, where the block marker metadata is the summary text rather than the list number. That approach should also make it fairly doable to track nested details depth in the SumTree
There was a problem hiding this comment.
Thank you — this reshaped tech §4. A details region is now modeled directly in the buffer on the ordered-list precedent:
- A one-line
DetailsSummaryblock whose marker carries{ depth, default_open }and whose line text is the summary itself (editable/selectable, like ordered-list item text); the disclosure triangle is a render-time margin decoration like the list number, never in the char stream. - Body blocks follow as ordinary blocks, closed by a zero-content
DetailsEndsentinel. - Depth is a
StyleSummary-style +1/−1 counter over the begin/end pair, so depth-at-offset is an O(log n)SumTreeseek — the "track nested depth in the SumTree" you pointed at. - Both markers read as a single
\nin the byte iterator, so selection/copy stay trivial (the property the table mechanism loses). - Unbalanced pairs after edits get deterministic rules mirroring product 8(a)/8(b); collapse reuses
HiddenLinesModel(per-view, outside undo history), which makes invariant 9 hold by construction.
The table to_internal_format path is dropped. One judgment call worth flagging: I put the summary in the line's text content rather than in the marker metadata — reading your note strictly ("block marker metadata is the summary text") you may have meant the summary string lives on the marker itself. I chose line text because it keeps the summary editable and inline-selectable with zero new offset machinery, but it is an easy flip if you would rather keep it out of the char stream. (2632ed6)
…ound-trip - Rewrite tech spec §4 on the ordered-list precedent instead of the FormattedTable internal-code-block mechanism: DetailsSummary block marker (depth, default_open) with summary as line text, DetailsEnd sentinel, SumTree counter dimension for depth, HiddenLinesModel for folding, deterministic 8a/8b rebalancing under edits. - Drop body_source; round-trip is canonical re-serialization (product invariant 10 reworded). - Add notebook/plan editor (RichTextEditorView) as the second interactive-tier surface (invariant 11, tech §5). - Clarify duplicate <summary> rendering (tags visible as plain text) and the rationale for the depth/count limits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@bnavetta thanks for the review — all five points addressed in 2632ed6, replies inline on each thread. Summary:
Ready for another look when you have a moment. |
|
|
||
| In the parser IR (the read-only rendering model), the whole `<details>…</details>` region is a single `FormattedTextLine`. Start/end marker lines are wrong *at this layer*: `FormattedText` consumers are stateless per-line renderers and `compute_formatted_text_delta` diffs lines independently, so nothing maintains marker balance here. The editor buffer is the opposite case — it has edit machinery that already maintains paired markers — and gets a paired representation instead (§4). | ||
|
|
||
| - `LineCount::num_lines` returns `1 + body line count` (content lines, independent of collapsed state — collapsed is view state, spec invariant 9). |
There was a problem hiding this comment.
Just to be clear, the 1 is for the summary line?
There was a problem hiding this comment.
Yes — the 1 is the summary/header line the container renders above its body, present regardless of collapse state. Reworded in ecd6ccd to say so explicitly.
| Details regions are first-class buffer blocks, following the ordered-list precedent (per maintainer guidance: the `FormattedTable` internal-code-block mechanism is an intermediate implementation and is not replicated here — it makes the block multi-line and forces `TableCache`-style offset mapping onto selection). | ||
|
|
||
| - **Summary line**: a details region starts with a one-line block styled `BufferBlockStyle::DetailsSummary { depth: u8, default_open: bool }` (new variants alongside `OrderedList`/`TaskList`, text.rs:867). Exactly like an ordered-list item, the marker (`BufferText::BlockMarker`) carries the block metadata while the line's *text content is the summary itself* — editable, selectable inline text, the analogue of list-item text. The disclosure indicator is a render-time margin decoration like the list number (drawn by the render element, never present in the char stream). The list-number analogy for metadata is `depth` and `default_open`; the checkbox precedent (`TaskList { complete }`) shows a togglable bool on the marker already exists. | ||
| - **Region extent**: the body is the run of ordinary blocks following the summary line, terminated by a zero-content end sentinel block `BufferBlockStyle::DetailsEnd { depth: u8 }`. Begin/end markers form pairs the same way paired inline-style markers do (`StyleSummary`'s +1/−1 counters, text.rs:1551): a counter field added to the buffer summary increments on `DetailsSummary` and decrements on `DetailsEnd`, so nesting depth at any offset is an O(log n) `SumTree` summary seek, and the `depth` field on each marker is a cached copy of that structural depth (as `indent_level` is for lists). |
There was a problem hiding this comment.
The fact that this is zero-content makes me wonder if details start/end markers should be new top-level variants of BufferText, rather than nested in BufferBlockStyle. We assume that there's only one BufferBlockStyle active for a given character, so we'll need a way to express that something is a code block within a details section, for example - that seems easier if details are not themselves block styles.
When rendering a given block, we'd then be able to consult the SumTree to check both whether or not it's part of a details section and what its specific style is. We can then adjust styling (colors, fonts, etc.) on a per-block basis.
This seems sort of equivalent to how link start/end markers don't use quite the same modeling as data-less markers for other inline styles like bold and italic
There was a problem hiding this comment.
This is the right call — adopted in ecd6ccd, and it also settles the summary-placement question I flagged last round. §4 now models details as a top-level BufferText::Details(DetailsMarker::Start { summary, default_open } / End) zero-width span marker, directly on the Link precedent rather than as a BufferBlockStyle:
- Because it is a span, not a style, it is orthogonal to per-character block style: a code block/list inside a details region keeps its own
BufferBlockStyle, and a renderer consults theSumTreefor both "inside details, at what depth" and "this block's own style" — exactly the per-block styling adjustment you described. (You are right that this parallels how link start/end markers differ from the data-less bold/italic markers.) - Depth is the balanced counter, not a stored field: I add
details_counter/total_details_markertoStyleSummaryand aDetailsDepthdimension mirroringlink_counter/LinkCount(text.rs:1550/1743), so depth-at-offset is an O(log n) seek. - The marker is zero-width like
Link(added to the zero-width arm ofItem::summary(), text.rs:1817, and skipped by theBytesiterator, text.rs:301), so it adds no\nand selection stays trivial. - Summary now lives on the
Startpayload (your "block marker metadata is the summary text"), so editing it is a marker-data affordance like editing a link URL, not inline line-text editing.
| - **Selection/copy stay trivial**: both markers read as a single `"\n"` in the byte iterator (text.rs:~626), exactly like every other `BlockMarker`; the summary is ordinary line text and body blocks are ordinary blocks. No offset mapping, no selection special cases — the property the table mechanism loses. | ||
| - **Nesting**: a nested details region is simply a nested begin/end pair among the body blocks. `MAX_DETAILS_DEPTH` applies on conversion into the buffer; depth never multiplies block types. | ||
| - **Edit invariants (deterministic rebalancing, mirroring product 8a/8b)**: an unmatched `DetailsSummary` (its `DetailsEnd` was deleted) owns blocks to the end of the buffer — the 8(a) rule; an unmatched `DetailsEnd` is inert and skipped on serialization — the 8(b) rule. Both are properties of the pairing scan, not stored state, so no edit sequence can produce an undefined document. `line_break_behavior()` (text.rs:904): Enter on a summary line starts a new plain body block below it (the marker case, like lists); Enter in the body behaves normally. Converting a block to/from details styles goes through the existing `BufferEditAction::StyleBlock` path (`set_block_style`/`convert_block`, model.rs:1142/1175), which already shows how to preserve marker metadata across conversions. | ||
| - **Collapse = existing folding**: collapsed state reuses `HiddenLinesModel` (hidden_lines_model.rs:20) — the body range is an anchor-pair hidden region, per-editor and outside buffer content, initialized from `default_open`. This satisfies product invariant 9 by construction: hiding is view state; copy, `raw_text()`, and serialization walk the buffer, which always contains the full body. Toggling does not edit the buffer, so it cannot dirty deltas or undo history. |
There was a problem hiding this comment.
This is something to check during implementation, but copying might also respect HiddenLinesModel
There was a problem hiding this comment.
Checked the copy path against the current tree: it does not respect HiddenLinesModel. selected_text_as_plain_text → clipboard_text_in_range (buffer.rs:1039/2374) walks the selection's raw char-offset range, and the selection anchors resolve to real buffer offsets (selection_model.rs:128) with no HiddenLinesModel reference on that path — hidden ranges are only collapsed in the rendered tree (render/model/mod.rs:2691) and used for cursor navigation (selection.rs:414-459). So selecting across a collapsed details span copies the full hidden body, which is what makes invariant 9 hold today. I have written §4 to state this as an invariant the implementation must preserve (if copy is ever changed to respect folds, details would need to opt out), and added a regression test guarding it. Agree it is worth re-confirming during implementation.
|
Thanks for the second pass! I'd like the spec to clarify how details sections interact with other block-level styling, but this is generally headed in the right direction. |
Second review pass (bnavetta):
- §4 rewritten off the Link precedent: a top-level BufferText::Details
(DetailsMarker::Start{summary,default_open}/End) zero-width span
marker, orthogonal to BufferBlockStyle, so body blocks keep their
own styles and a renderer can query details-depth and block-style
independently. Resolves the per-character block-style exclusivity
problem raised in review.
- Depth is a SumTree counter dimension (LinkCount precedent), not a
stored per-marker field.
- Confirmed via source that copy does not consult HiddenLinesModel
(buffer.rs:1039/2374), so a collapsed span still copies its full
body — invariant 9 holds; noted as an invariant to preserve.
- Clarified num_lines '1' is the summary/header line.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@bnavetta thanks for the second pass — all three addressed in ecd6ccd, replies inline. Headline: details are now a zero-width span marker, not a Also: I verified against the current tree that the copy path does not consult Ready for another look. |
…w precedent (GH13735) Model block alignment as start/end BufferText markers plus a SumTree dimension (ordered-list precedent), not a FormattedTextLine::AlignedBlock wrapper. The wrapper collapses into a BufferBlockStyle when lowered into the editor content model, which violates bnavetta's warpdotdev#13345 constraint that only one BufferBlockStyle is active per character (aligned + code block can't compose). Demote the wrapper to a considered-and-rejected alternative. Also: add TUI surface disposition (best-effort within terminal width, left-aligned fallback) as an explicit scope decision; mark warpui_core::Align as a GUI-only paint strategy rather than the content model; fix the feature- gating path to crates/editor/src/content/buffer.rs:850-855; rewrite the Risks blast radius honestly for the marker design (bounded BufferText match sites) versus the rejected wrapper (20 files matching FormattedTextLine, incl. the non-obvious ipynb_parser and warp_tui); cite bnavetta's canonical round-trip ruling on invariant 6.
bnavetta
left a comment
bnavetta
left a comment
There was a problem hiding this comment.
Sorry for the delay! One last suggestion, but otherwise ready to implement
|
|
||
| ```rust | ||
| pub enum DetailsMarker { | ||
| Start { summary: String, default_open: bool }, // summary as inline-markdown source, like LinkMarker::Start(url) |
There was a problem hiding this comment.
Ah sorry, thinking about this more, since the summary is also formatted text, we'll need a way to track that formatting. We probably want 3 markers, so the buffer structure is something like:
<details:start default_open=true>This is the summary<details:end-summary>This is the body<details:end>
I think we can do that without significantly changing the SumTree setup - we'd still count start/end markers to determine depth, and when extracting the summary, we can seek or linearly scan to the nearest EndSummary marker.
With the current structure, we'd have to parse the inline summary markdown again to display it
bnavetta
added
ready-to-implement
|
Hi @seobew — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
3 participants
Closes #10259
What
Product + tech spec for #10259 (
ready-to-spec), underspecs/GH10259/in theproduct.md+tech.mdformat from CONTRIBUTING.md.<summary>), hard resource limits (nesting depth 8, 512 widgets/doc — constants, not heuristics), collapsed-state-as-view-state (copy/raw_textalways full content), accessibility contract with renderer-generated IDs, round-trip serialization, and streaming behavior.FormattedTextLine::Detailscontainer variant following theFormattedTableprecedent (single line variant, flat document model preserved), aparse_detailsbranch inparse_markdown's block chain, explicit handling in the imported-HTML path (html_parser.rs), editorBufferBlockStylemapping, and a per-invariant test matrix. Tradeoffs (start/end marker lines, tree-shaped model) are discussed and rejected with reasons.Relation to prior work
#10462 proposed a spec for the same issue; Oz's reviews there flagged non-deterministic resource caps, undefined malformed-
<summary>handling, implementation pointers not matching the checkout, and unscoped preview surfaces. This spec addresses those points directly and follows the two-file spec format. Happy to converge with the earlier author if they're still active.Open questions for maintainers
Called out in tech.md: whether the disclosure widget reuses the existing block-folding machinery or gets a dedicated component, and the confirmed enumeration of non-interactive preview call sites for invariant 11.
🤖 Generated with Claude Code