From adfb0f9e02a493007dc527aa9f0eab95d1b37a9b Mon Sep 17 00:00:00 2001 From: iris Date: Wed, 9 Sep 2026 19:29:58 +0200 Subject: [PATCH] shared terminal: align row icons via a shared grid, not offsets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per mara's screenshot report (agent-terminal icons not aligned in the first column) — and her explicit follow-up steer on the first version of this fix: 'dont do it by offsets at all, it should be part of the layout that they align.' Root cause traced first, not guessed: .row-glyph relied on inheriting text-indent: -1.4em from .live .row to paint its glyph in the reserved prefix slot; details.row > summary separately reset text-indent: 0 for its own hanging-caret needs, which also zeroed the inherited value for the icon nested inside it (indent inherits by computed value, not by rule). Two independently-computed offsets that had to agree by hand, and silently didn't. Replaced the whole offset scheme with a real structural fix: every row is a 2-column CSS grid (icon column, width from one shared --row-icon-col custom property, then content column), and a details row grids its own with the exact same grid-template-columns value instead of griding itself (so its
 body still stacks full
width below, not squeezed into column 2). Icon and content are placed
by explicit grid-column, not auto-placement inference, so an icon-less
row's lone child still lands in the content column. .row-content is a
new wrapper class (Row.tsx, terminal.js's row()/mutableRow()/
placeholder()) giving that content an element the grid can target by
class - a DocumentFragment (what linkify() returns) doesn't persist as
a node once appended, so without an explicit wrapper there was nothing
for the grid to place.

Caught and fixed a second real bug while building this for real (not
just reasoning about the CSS): the first draft's details.row { display:
block } had lower specificity than .live .row's display: grid and never
actually applied, squeezing  into the row's own 1.4em icon
column and wrapping its text one character per line. Needed
.live details.row to out-specify it.

Verified with a headless-chromium render of all 6 row shapes (flat
icon / flat icon-less / flat markdown-body / details icon / details
icon-less / a long-wrapping flat row) - all align and wrap correctly.
Also verified via the earlier Range.getBoundingClientRect() measurement
(glyph paint position, not just the element's own box): diff 0.00 for
both this and the previous fix, but only this one is structural rather
than two numbers that happen to still agree today.
---
 docs/web-ui/terminal-rendering.md             |  15 +-
 .../packages/agent/src/components/Row.tsx     |   6 +-
 .../packages/shared/src/terminal/terminal.css | 162 ++++++++++--------
 .../packages/shared/src/terminal/terminal.js  |  19 +-
 4 files changed, 122 insertions(+), 80 deletions(-)

diff --git a/docs/web-ui/terminal-rendering.md b/docs/web-ui/terminal-rendering.md
index 91ed9887..e37d5d46 100644
--- a/docs/web-ui/terminal-rendering.md
+++ b/docs/web-ui/terminal-rendering.md
@@ -20,11 +20,16 @@ separate client-side row model or classification step.
 
 ## Layout
 
-Every row shares one prefix column via `padding-left` + negative
-`text-indent` on `.live .row`; an icon (when set) sits in a fixed-width
-`.row-glyph` cell so icons of different rendered widths still line up.
-`
` summaries reuse the same metrics, with the disclosure caret -leading the summary text rather than the icon. +Every row is its own 2-column grid — icon column (`.row-glyph`, width +from the `--row-icon-col` custom property) then content column +(`.row-content`, or a markdown body's own `.md` class) — so an icon's +left edge lands in the same place regardless of row kind. A `
` +row grids its `` instead of itself (so the `
` body below
+can stack full-width), using the exact same `grid-template-columns` value,
+with the disclosure caret leading the summary text rather than the icon.
+One shared column definition, not two independently-computed offsets
+kept in sync by hand — see `terminal.css`'s own comment on the incident
+that motivated the change.
 
 ## Levels
 
diff --git a/frontend/packages/agent/src/components/Row.tsx b/frontend/packages/agent/src/components/Row.tsx
index 18ade774..66ea53fb 100644
--- a/frontend/packages/agent/src/components/Row.tsx
+++ b/frontend/packages/agent/src/components/Row.tsx
@@ -66,7 +66,11 @@ export function Row({ row }: { row: TermRow }) {
     return (
       
{icon} - {linkifyToNodes(row.summary)} + {/* Explicit wrapper, not bare children — the row's grid places + `.row-content` in its content column by class, not by + auto-placement inference (an icon-less row's lone child would + otherwise auto-place into the icon column instead). */} + {linkifyToNodes(row.summary)}
); } diff --git a/frontend/packages/shared/src/terminal/terminal.css b/frontend/packages/shared/src/terminal/terminal.css index 15b27300..46507897 100644 --- a/frontend/packages/shared/src/terminal/terminal.css +++ b/frontend/packages/shared/src/terminal/terminal.css @@ -42,6 +42,14 @@ anchoring would double-compensate scrollTop during loadMore() prepends, causing an erratic jump. */ overflow-anchor: none; + /* Single source of truth for the icon column's width, shared by every + row kind's own grid below (`.live .row` and `details.row > summary`). + Structural alignment: both contexts read the *same* custom property + rather than each hand-computing an offset that has to independently + agree with the other (mara: "dont do it by offsets at all, it should + be part of the layout that they align" — see the incident this + replaced, two paragraphs down). */ + --row-icon-col: 1.4em; } .live.terminal { background: transparent; @@ -74,37 +82,68 @@ transform: translateY(0); } } -/* Unified prefix column for every row kind. The glyph (`→ ← · ◆ ✓ ✗ ⌁ !`) - is the first character of the row's text content; `padding-left` reserves - the column and `text-indent: -1.4em` pulls the glyph back into it. Wrapped - continuation lines then start under the body, not under the glyph, so - wraps don't blur into the next row. `details.row` summaries reuse the - same metrics below. */ +/* Unified prefix column for every row kind, via CSS grid rather than + hand-computed offsets (see the incident this replaced, below). A flat + row is its own 2-column grid (`--row-icon-col` | content); a `details` + row instead grids its `` (see further down) so its `
`
+   body can stack under the *whole* row, full width, not just the content
+   column. `.row-glyph` and the row's actual content are both placed by
+   explicit `grid-column` (below), never by auto-placement inference —
+   an icon-less row's content still lands in column 2, not wherever the
+   grid would otherwise auto-place a lone child.
+
+   Previously: `padding-left` + a negative `text-indent` pulling the first
+   inline box (or bare first character) back into the reserved slot.
+   Replaced after a real bug, found the same session: `.row-glyph` needed
+   its own copy of that indent to actually paint there (being
+   `inline-block`, it's a block container for its own text, so it doesn't
+   inherit the *effect* just by inheriting the *value*) — and
+   `details.row > summary` separately reset `text-indent: 0` for its own
+   hanging-caret needs, which silently zeroed the *inherited* indent for
+   the icon nested inside it too, since indent inherits by computed
+   value, not by rule. A details-row icon ended up painted 1.4em right of
+   a flat row's despite both icons' own boxes already sharing a left edge
+   — two independent numbers (`padding-left`/`text-indent` here,
+   `margin-left`/`padding-left` on `details.row > summary`) that had to
+   agree by hand, and silently didn't. mara: "dont do it by offsets at
+   all, it should be part of the layout that they align" — a shared grid
+   column, read from one custom property, replaces both. */
 .live .row {
+  display: grid;
+  grid-template-columns: var(--row-icon-col) 1fr;
+  column-gap: 0.5em;
   white-space: pre-wrap;
   word-break: break-word;
   padding: 0.05em 0;
   line-height: 1.45;
   border-left: 2px solid transparent;
-  padding-left: 1.9em;
-  text-indent: -1.4em;
   margin: 0.1em 0;
 }
 .live .row + .row {
   border-top: 0;
 }
-/* Fixed-width icon column. Rows built with an `icon` (see terminal.js
-   `row()` / `details()`) put it in a `.row-glyph` element instead of as a
-   bare first character. `inline-block` with a fixed `width` means the icon
-   occupies one constant-width cell regardless of the glyph's rendered width
-   (emoji differ; some carry a variation selector), so every icon's left edge
-   lines up — a flat-row `🧠` and a `details` summary's `🖥️` share the column.
-   It's the first inline box, so the row's `text-indent: -1.4em` pulls it into
-   the reserved prefix slot exactly like a bare glyph; the following text then
-   starts at the `padding-left` (1.9em) where wrapped lines also hang. */
+/* Icon column. Every row kind's icon lands in `grid-column: 1` of
+   whichever grid is active in that context (this rule for a flat row,
+   `details.row > summary`'s own grid further down for a collapsible
+   one) — same column definition (`--row-icon-col`), so every icon's
+   left edge lines up regardless of the glyph's own rendered width
+   (emoji differ; some carry a variation selector). */
 .live .row-glyph {
-  display: inline-block;
-  width: 1.4em;
+  grid-column: 1;
+}
+/* Content column — explicit, not left to grid auto-placement. Rows
+   built with an icon (see terminal.js `row()` / Row.tsx) wrap their
+   actual content in `.row-content` (a markdown body just uses its own
+   `.md` class, styled the same way below) specifically so an icon-less
+   row's lone remaining child still lands in column 2: without an
+   explicit `grid-column`, auto-placement would put a row's *only* grid
+   item in column 1 (the first available cell), landing text in the
+   narrow icon column instead of the content one. */
+.live .row-content,
+.live .row > .md,
+.live .row > details {
+  grid-column: 2;
+  min-width: 0;
 }
 /* Row colours, keyed by severity `level` (hive-agent's `term_msg.rs`),
    not by row kind any more — mara's terminal-message redesign dropped
@@ -147,13 +186,6 @@
     text-shadow: 0 0 14px color-mix(in srgb, var(--amber) 95%, transparent);
   }
 }
-/* Any child block (markdown body, nested details) resets the parent
-   row's hanging indent so the content lays out from column 0 of the
-   body area. */
-.live .row .md,
-.live .row > details {
-  text-indent: 0;
-}
 /* "↓ N new" pill: shown when new rows arrive while the operator is
    scrolled up; click to jump to bottom. Positioned by the wrapper's
    `position: relative` (terminal-wrap supplies it; pages that skip the
@@ -217,58 +249,49 @@
   cursor: default;
   opacity: 0.6;
 }
-/* Expandable rows reuse the flat-row prefix metrics (padding-left +
-   negative text-indent). The summary's icon (when present) sits in the
-   shared `.row-glyph` column — same cell as a flat row's icon — so a
-   `details` summary's `🖥️` lines up under a flat row's `🧠`. The
-   disclosure caret (`▸ / ▾`) leads the `.summary-text` (not the icon) via
-   `::before`, so it sits where the summary text starts rather than shoving
-   the icon out of the prefix column. Icon-less summaries have no `.row-glyph`,
-   so the caret falls back into the prefix column like the old directional
-   glyph. The summary text carries no `→ / ←`; the row colour (cyan =
-   outbound tool, muted = inbound result) carries the direction. */
-details.row {
+/* Expandable rows don't grid themselves the way a flat row does — a
+   `
` needs its `` and `
` body to stack normally
+   (full row width each), not sit side by side in the row's own 2
+   columns. `.live details.row` (not just `details.row`) overrides
+   `.live .row`'s `display: grid` back to block for the `
` + element itself — needs the extra `.live` to out-specify it (two + classes beats one class + one type selector), or the override never + applies and ``/`
` get squeezed into the *row's* 2-column
+   grid instead of stacking (caught rendering this for real: a
+   `` auto-placed into the 1.4em icon column wraps its text one
+   character per line). The icon+text split happens one level down, on
+   `` alone, which grids itself with the exact same
+   `grid-template-columns: var(--row-icon-col) 1fr` as `.live .row` —
+   same column, same custom property, so a details summary's icon and a
+   flat row's icon share a left edge *because they're the same declared
+   column*, not because two independently-computed offsets happen to
+   agree. The disclosure caret (`▸ / ▾`) leads `.summary-text` (not the
+   icon) via `::before`. The summary text carries no `→ / ←`; the row
+   colour (cyan = outbound tool, muted = inbound result) carries the
+   direction. */
+.live details.row {
+  display: block;
   white-space: normal;
 }
-/* Two-column layout for expandable summary rows.
-   Left col: fixed-width icon cell. Right col: disclosure chevron +
-   text, wrapping within itself so no continuation line bleeds under
-   the icon. Overrides the flat-row hanging-indent metrics (.live .row
-   sets padding-left: 1.9em; text-indent: -1.4em) with explicit flex
-   geometry. The negative margin-left on summary cancels the details
-   container's inherited padding-left so the icon lands at the same
-   horizontal position as flat-row icons. */
 details.row > summary {
+  display: grid;
+  grid-template-columns: var(--row-icon-col) 1fr;
+  column-gap: 0.5em;
+  align-items: baseline;
   cursor: pointer;
   list-style: none;
   white-space: pre-wrap;
   word-break: break-word;
-  display: flex;
-  align-items: baseline;
-  margin-left: -1.9em;
-  padding-left: 0.5em;
-  text-indent: 0;
 }
-/* Icon column: 2em accommodates wide emoji without ink bleeding into
-   the chevron column. Overrides the inline-block + width: 1.4em set
-   on .live .row-glyph for the shared flat-row context. */
-details.row > summary > .row-glyph {
-  flex: 0 0 2em;
-  width: auto;
-  text-align: center;
-}
-/* Content column: chevron (::before) + text, wraps within the cell. */
+/* `.row-glyph`'s own `grid-column: 1` (set once, above) already applies
+   here too — nothing details-specific to repeat. `.summary-text` needs
+   its own explicit `grid-column: 2` for the same reason `.row-content`
+   does on a flat row: an icon-less summary's only child must not fall
+   back to column 1 via auto-placement. */
 details.row > summary > .summary-text {
-  flex: 1;
+  grid-column: 2;
   min-width: 0;
 }
-/* Icon-less summaries: the CSS-generated ▸/▾ acts as the hanging
-   marker. Hanging indent keeps wrapped lines under the text body,
-   not under the chevron. Mirrors the flat-row text-indent metric. */
-details.row > summary > .summary-text:only-child {
-  padding-left: 1.4em;
-  text-indent: -1.4em;
-}
 details.row > summary > .summary-text::before {
   content: "▸ ";
   color: inherit;
@@ -280,7 +303,6 @@ details.row > pre.diff-body,
 details.row > pre.tool-body {
   margin: 0.3em 0 0.4em 0;
   padding: 0.4em 0.6em;
-  text-indent: 0;
   background: rgba(255, 255, 255, 0.02);
   border-left: 2px solid var(--purple-dim);
   white-space: pre-wrap;
@@ -301,8 +323,7 @@ details.row > pre.diff-body .diff-ctx {
   color: var(--fg);
 }
 /* Markdown body inside a row (assistant text, send/recv message
-   bodies). Inline elements get muted accents; block elements
-   reset the parent row's hanging indent so content lays out cleanly. */
+   bodies). Inline elements get muted accents. */
 .live .row .md p {
   margin: 0.2em 0;
 }
@@ -323,7 +344,6 @@ details.row > pre.diff-body .diff-ctx {
   padding: 0.4em 0.6em;
   background: rgba(255, 255, 255, 0.04);
   border-left: 2px solid var(--purple-dim);
-  text-indent: 0;
   white-space: pre-wrap;
   word-break: break-word;
 }
diff --git a/frontend/packages/shared/src/terminal/terminal.js b/frontend/packages/shared/src/terminal/terminal.js
index ae659279..36d8732e 100644
--- a/frontend/packages/shared/src/terminal/terminal.js
+++ b/frontend/packages/shared/src/terminal/terminal.js
@@ -185,7 +185,7 @@ export function create(opts) {
     clearPlaceholder();
     const e = document.createElement("div");
     e.className = "row note";
-    e.textContent = text;
+    e.appendChild(contentSpan(document.createTextNode(text)));
     log.appendChild(e);
     placeholderEl = e;
   }
@@ -200,6 +200,19 @@ export function create(opts) {
     g.textContent = icon;
     return g;
   }
+  // Wraps a flat row's actual content (text/links, or the mutable-row
+  // text node) in a real element the row's own grid can place by class
+  // (`grid-column: 2` in terminal.css) — `linkify()` returns a
+  // `DocumentFragment`, which doesn't persist as a node once appended,
+  // so without this wrapper an icon-less row's content has no element of
+  // its own for the grid to target and would auto-place into the icon
+  // column instead (see terminal.css's own comment on why that matters).
+  function contentSpan(node) {
+    const c = document.createElement("span");
+    c.className = "row-content";
+    c.appendChild(node);
+    return c;
+  }
   // Build a  whose icon (if any) sits in the shared `.row-glyph`
   // column and whose text lives in a `.summary-text` span — the disclosure
   // caret (CSS `.summary-text::before`) then leads the text, not the icon,
@@ -219,7 +232,7 @@ export function create(opts) {
     const e = document.createElement("div");
     e.className = "row " + (cls || "") + (currentNoAnim ? " no-anim" : "");
     if (icon != null && icon !== "") e.appendChild(glyphSpan(icon));
-    e.appendChild(linkify(text));
+    e.appendChild(contentSpan(linkify(text)));
     log.appendChild(e);
     afterAppend(wasNearBottom);
     return e;
@@ -235,7 +248,7 @@ export function create(opts) {
     e.className = "row " + (cls || "") + (currentNoAnim ? " no-anim" : "");
     if (icon != null && icon !== "") e.appendChild(glyphSpan(icon));
     const tn = document.createTextNode(text == null ? "" : String(text));
-    e.appendChild(tn);
+    e.appendChild(contentSpan(tn));
     log.appendChild(e);
     afterAppend(wasNearBottom);
     return [e, tn];