Plan 016: iced 3e batch 1 — chrome parity with the Mac app - #305
Merged
Conversation
…nt() seam
Vendor Inter 4.1 static Regular/Medium/SemiBold + OFL license at
third_party/inter/ (the repo's first bundled binary asset — the swash
README.roost.md/license pattern transfers; layout is its own precedent)
and load the bytes on the iced application builder with
default_font(chrome_font(Normal)). All weighted chrome text routes
through chrome::chrome_font so a future config-driven swap stays a
one-seam change; the previous Font{weight, ..Font::default()} sites
silently reset the family to system sans, and Bold maps to Semibold
because nearest-match with only R/M/SB loaded is implementation-
defined. PROJECTS header gains Semibold, tab labels Medium when
active — Mac parity (plan 016 W3).
Verified live: cosmic-text groups the three statics under one "Inter"
family (semibold/medium demonstrably render); the terminal font picker
is unaffected (monospace filter excludes Inter); terminal cells keep
their explicitly-threaded font. cursor correctness review: no findings.
Simplify skipped: small mechanical diff.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…ivider, footer chip One solid band color (#24292c) for the sidebar header/footer and the tab band, a lighter list region (#2d3235) between them, and a 1px divider drawn as the sidebar's rightmost column — inside the engine-reported width, so terminal grid math and every sidebar_width-relative offset stay untouched, and the resize grip's seam (first-child right edge) lands exactly on it. SURFACE/SURFACE_DARK retired; the tab band was darker than the sidebar (inverted contrast vs the Mac reference) and now matches the header band. Footer chip re-based to the measured Mac bezel (#34393c resting, 13pt label). Values pixel-verified live against the frozen Mac reference captures (plan 016 W1/W4; all regions within 0 RGB points of target). The Mac's own divider samples pure black; the brief's ~(26,29,30) hairline is used instead so the line doesn't notch through the full-height bands — flagged for Charlie's review, one-constant change. cursor review: one finding — test_tab_strip_pixels.py's SURFACE_DARK literal is now stale; deferred to C7 by plan (the test is not in any CI iced list yet; C7 updates the allowlist and adds it to CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…nset, unified project row BAND_HEIGHT 34→32 with pills centered via a derived BAND_PILL_PADDING_Y; ROW_HEIGHT 28→32 (the Mac reference measures the selection pill at ~30pt — the plan draft's 24 was a wrong sourceList assumption, caught and corrected against the frozen capture before commit); the project row is rebuilt to the Mac shape: 3px rollup rail with 5px vertical insets at the leading edge, selection pill inset 6px/1px radius 6, label at x=25 matching the reference exactly, and a trailing slot for the notification dot (rendered in the next commit). TERMINAL_PADDING 12→0 — the terminal grid now sits flush under the band on all four sides like the Mac's edge-pinned TerminalView; grid sizing and cell hit-testing audited (the divider lives inside the sidebar width, so zero slack stays zero). Directly-broken harness mirrors are folded here per plan: walking skeleton's origin assertions reworked for zero inset, sidebar-pixels rail/pill assertions re-pinned, both real-input harnesses moved to a named TERMINAL_PADDING = 0 (wayland imports it from the X11 twin) and live terminal_top-derived band offsets; the row-center literals land back at 52 by the corrected formula. Verified live: terminal_top == 32, pill bbox 30 tall with 6/1 insets, row pitch 34, band centering 4/4, 54 iced e2e tests green. cursor review: no findings. Simplify: derived const + cross-file import + live-metric offsets applied. test_tab_strip_pixels.py stays stale until C7 by plan (not in any CI iced list; C7 updates its allowlist and adds it to CI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
Remove the band bell button entirely — the Mac communicates the same state through badge dots, so notifying projects now show the 8px accent dot inside the selection pill's trailing slot (active project included), matching the Mac's sidebar exactly; pill badges unchanged and the inbox stays reachable via the palette command and keybind. Remove the collapsed-state ☰ (no in-window affordance when collapsed — Mac parity; reopen is alt+b / palette). Move the + button inside the scrollable as a sibling of the reorder strip so it hugs the last pill and scrolls with overflow like the Mac's ＋; it must never live inside the strip's content (the reorder gesture walks those children). Message::OpenNotifications and Message::ToggleSidebar turned out to be dead once their only constructors (the deleted buttons) were gone — keybind and palette dispatch call the underlying paths directly — so the variants, their apply() arms, and open_notifications() are removed (clippy -D warnings enforced it). Harness folds: ☰-restore via alt+b, add-tab clicks computed from the active pill's rendered right edge (+18 = strip spacing + half the button), an unsalvageable bell-position sub-test removed, and the tab-strip pixel guard's right-edge carve-out dropped (nothing is pinned at the right anymore). Verified live: no bell pixels in the band, dots at the notified projects (#4e9af1, active project included), badge intact, no ☰ when collapsed, + directly after the last pill; test_notifications 5/5 and the full local iced e2e set green except the C7-deferred test_tab_strip_pixels staleness (identical on the pre-C4 baseline). cursor review: no findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
NSSplitView-parity drag-collapse: while a seam drag is live, the
unclamped pointer-derived width crossing below MIN_WIDTH/2 (80px)
publishes GripEvent::Collapse instead of the clamped Dragged, clears
the drag in the same arm (no stale Dragged{160} can follow in the
batch), and routes to a collapse path that drops the live drag WITHOUT
committing — reopen restores the pre-drag persisted width, not the
clamped floor, which is why set_sidebar_collapsed (commit-first, kept
for keybind/palette) is deliberately bypassed. The gesture ends at
collapse; drag-back-to-reopen within one gesture is impossible (grip
leaves the tree) — recorded parity divergence, C8 documents it.
Unit matrix pins the threshold boundary (< 80, not <=), the
no-publish-after-collapse rule, the dangling-release delegation, and
the reopen-width invariant (workspace width stays 300 through a
drag-collapse; the contrast case shows commit-first would pin 160).
One pre-existing test retargeted (its fixture drag now crosses the
threshold under the new semantics). Live: sidebar resize + collapse
persistence + layout e2e all green; set_width 300 → collapse → reopen
→ 300 verified over IPC. cursor review: no findings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…n macOS The window title now composes the Mac way — active project name (fallback "Roost"), en-dash, the active tab's live OSC-7 cwd (else the project cwd) home-collapsed and left-truncated to 48 extended grapheme clusters with a leading ellipsis. The composition and abbreviation live as pure helpers in roost-ui-model (window_title.rs, 13 unit tests incl. ZWJ/flag grapheme truncation) so GTK can adopt them later; iced re-evaluates the title fn per update batch, so rename/tab-switch/cd all update with no manual invalidation — verified against the real OS window title over CGWindowList. foreground_cwd is deliberately not consulted (native proc lookup per batch; the Mac subtitle tracks OSC 7 only). Titlebar: macOS-only titlebar_transparent makes the titlebar render the window's band color (measured 36,41,44 — seamless with the PROJECTS header, vs the stock dark 80,85,87); fullsize_content_view stays off so terminal_top and the pixel lanes are untouched (the plan's abort triggers never fired). Linux window settings unchanged (cfg-gated; non-macOS shape compile-checked). unicode-segmentation was already in the dependency graph via cosmic-text — one new edge, no new crate. cursor review: no findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…e it into CI Refresh the guard's literal mirrors to the plan-016 chrome (BAND 32px, #24292c allowlist entry replacing the retired SURFACE_DARK) and add the divider assertion: the 1px hairline at the sidebar's right edge must render #1a1d1e across header/list/footer rows when expanded and vanish when collapsed. Add test_tab_strip_pixels.py and test_z_typography.py to all three ci.yml iced e2e lists — both were in the Makefile's ICED_E2E_TESTS but ran in no CI lane, so the #281 scrollbar guard and the shared typography test never gated iced on CI (gap found during plan-016 discovery). Repo-wide mirror sweep found nothing else stale. Verified live: the full newly-listed set passes locally (9 tests, --roost-fresh CI-parity run). cursor review: no findings. The macOS runner's monospace-family count is the one open question for test_z_typography's first CI run (plan §9 names the fallback). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…es 3h Roadmap: 3e is rewritten as complete (chrome parity batch 1 — Mac color system, 32px bands/rows, zero terminal inset, Inter chrome font, bell removed for Mac badge routing, no collapse affordance + drag-to-collapse, in-strip +, dynamic title + band-colored titlebar, pixel guard wired into CI) including the two review flags for Charlie (divider hairline vs literal black; zero left inset). New 3h holds the user-directed remainder (baseline typography, cursor/selection/link geometry, empty/error states, confirm-overlay modality, hover states, vibrancy spike, agent-row typography, status-dot geometry, the hover-close product decision, offscreen-tab reveal). Inventory rows trued to post-016 reality with the GTK-divergence note (GTK expected to retire rather than restyle; differences recorded, not fixed). Historical baseline tables deliberately untouched — they are dated audit records, not current-truth claims. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…an empty WM_CLASS The real-input harness found the iced window by title substring "Roost", which the dynamic project–cwd title no longer guarantees; the shed run caught the discovery timeout. winit leaves WM_CLASS (X11) / app_id (Wayland) empty when application_id is unset, so the window had no stable identity at all. Set it to ai.stridelabs.Roost.iced — matching the notification adapter's desktop-entry hint so shells group the window and its banners under one identity — and switch the harness to class-based discovery. Verified in the shed: the full real-input check passes end to end on the new chrome (incl. the reworked overflow navigation, palette pointer routing, and keybind ☰-restore segments). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR updates Iced chrome styling, dynamic window titles, sidebar drag-collapse behavior, terminal geometry, Linux input coordinates, pixel tests, and CI coverage across X11, Wayland, and macOS. ChangesIced polish batch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SidebarResize
participant GripEvent
participant Message
participant App
SidebarResize->>GripEvent: emit Collapse below threshold
GripEvent->>Message: map to SidebarDragCollapsed
Message->>App: call sidebar_drag_collapsed()
App->>App: clear transient width and collapse sidebar
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/development/iced-parity-inventory.md`:
- Around line 133-147: The New-tab affordance row currently describes a fixed,
always-reachable plus control, but the implementation in the tab-strip layout
allows it to scroll offscreen. Update the row’s status and acceptance evidence
to document the shipped scrolling behavior and corresponding
reachability/overflow test, without claiming fixed positioning outside overflow.
In `@tools/input/linux/iced_clipboard_check.py`:
- Around line 128-132: Resolve the supported absolute xdotool path once during
main’s existing shutil.which validation, then pass that resolved path to the
subprocess.run calls in the clipboard-check flow instead of the bare “xdotool”
command. Reuse the same validated path for every xdotool invocation, including
the call near the window-class search.
In `@tools/roosttest/test_tab_strip_pixels.py`:
- Around line 219-290: Update tools/roosttest/test_tab_strip_pixels.py lines
219-290 and 184-187, tools/roosttest/test_sidebar_pixels.py lines 297-328, and
tools/roosttest/test_iced_walking_skeleton.py lines 191-196 to replace
pixel-based functional E2E oracles with textual IPC assertions. If renderer-only
chrome has no textual IPC oracle, document a narrow, explicit exception at each
applicable site before retaining the pixel guards; preserve condition-based
waits and existing behavior checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e492f69-7e86-460d-96b3-6b5bc5611b62
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockthird_party/inter/Inter-Medium.ttfis excluded by!**/*.ttfthird_party/inter/Inter-Regular.ttfis excluded by!**/*.ttfthird_party/inter/Inter-SemiBold.ttfis excluded by!**/*.ttf
📒 Files selected for processing (19)
.github/workflows/ci.ymlCLAUDE.mdcrates/roost-iced/src/app.rscrates/roost-iced/src/chrome.rscrates/roost-iced/src/main.rscrates/roost-iced/src/sidebar_resize.rscrates/roost-iced/src/terminal_widget.rscrates/roost-ui-model/Cargo.tomlcrates/roost-ui-model/src/lib.rscrates/roost-ui-model/src/window_title.rsdocs/development/iced-migration-roadmap.mddocs/development/iced-parity-inventory.mdthird_party/inter/LICENSE.txtthird_party/inter/README.roost.mdtools/input/linux/iced_clipboard_check.pytools/input/linux/iced_wayland_clipboard_check.pytools/roosttest/test_iced_walking_skeleton.pytools/roosttest/test_sidebar_pixels.pytools/roosttest/test_tab_strip_pixels.py
Comment on lines
+128
to
+132
| # By class, not name: the window title is dynamic since plan 016 | ||
| # ("{project} – {cwd}"), but WM_CLASS carries the fixed | ||
| # application_id the app sets on Linux. | ||
| result = subprocess.run( | ||
| ["xdotool", "search", "--name", "Roost"], | ||
| ["xdotool", "search", "--class", "Roost"], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test -x /usr/bin/xdotoolRepository: charliek/roost
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline tools/input/linux/iced_clipboard_check.py
printf '\n--- target context ---\n'
sed -n '1,165p' tools/input/linux/iced_clipboard_check.py
printf '\n--- xdotool references ---\n'
rg -n -C 3 'xdotool|PATH|subprocess\.run' tools .github CLAUDE.md README.md 2>/dev/null || trueRepository: charliek/roost
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dependency check and main ---'
sed -n '2350,2385p' tools/input/linux/iced_clipboard_check.py
printf '%s\n' '--- CI invocation ---'
sed -n '735,760p' .github/workflows/ci.yml
printf '%s\n' '--- project guidance ---'
if [ -f CLAUDE.md ]; then sed -n '1,220p' CLAUDE.md; else printf '%s\n' 'CLAUDE.md not present'; fi
printf '%s\n' '--- available xdotool paths ---'
command -v xdotool || true
find /usr /bin /opt -type f -name xdotool -perm -0100 -print 2>/dev/null | head -20 || true
printf '%s\n' '--- behavioral probe: PATH resolution and env inheritance ---'
python3 - <<'PY'
import os, shutil, subprocess, tempfile
with tempfile.TemporaryDirectory() as d:
fake = os.path.join(d, "xdotool")
open(fake, "w").close()
os.chmod(fake, 0o700)
env = {**os.environ, "PATH": d}
print("which_with_attacker_path:", shutil.which("xdotool", path=env["PATH"]))
print("subprocess_resolves_name:", subprocess.list2cmdline(["xdotool", "search", "--class", "Roost"]))
print("child_path:", subprocess.run(
["python3", "-c", "import os; print(os.environ['PATH'])"],
env=env, capture_output=True, text=True, check=True
).stdout.strip())
PYRepository: charliek/roost
Length of output: 18018
Command Search Path Hijacking (CWE-426): Untrusted Search Path
Reachability: Internal · Exploitability: Trivial
Reachability path
● Entry
crates/roost-iced/src/main.rs:427
title
│
▼
● Hop
tools/input/linux/iced_wayland_clipboard_check.py:173
_measure_terminal_cell
│
▼
● Sink
tools/input/linux/iced_clipboard_check.py
Use one trusted absolute path for xdotool.
tools/input/linux/iced_clipboard_check.py:132 and later calls resolve xdotool through PATH. The shutil.which check in main does not prevent an unintended PATH entry from selecting the executable. Resolve and validate the supported runner path once, then reuse it for every xdotool call.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 129-129: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
[error] 132-132: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/input/linux/iced_clipboard_check.py` around lines 128 - 132, Resolve
the supported absolute xdotool path once during main’s existing shutil.which
validation, then pass that resolved path to the subprocess.run calls in the
clipboard-check flow instead of the bare “xdotool” command. Reuse the same
validated path for every xdotool invocation, including the call near the
window-class search.
Source: Linters/SAST tools
Owner
Author
There was a problem hiding this comment.
Disposition: skipped, recorded in 0292e3c's message — bare xdotool is this harness's pre-existing pattern at every call site (a CI/dev-only tool; no untrusted PATH in those environments). Hardening all call sites uniformly is reasonable future work but out of this PR's scope; changing only the one new call site would leave the file inconsistent.
There was a problem hiding this comment.
@charliek, acknowledged. The change at tools/input/linux/iced_clipboard_check.py:132 follows the existing harness pattern. A one-call change would not provide consistent hardening.
A future change can resolve and reuse an absolute xdotool path across the complete harness. Would you like me to open a follow-up issue for that work?
✏️ Learnings added
Learnt from: charliek
Repo: charliek/roost PR: 305
File: tools/input/linux/iced_clipboard_check.py:128-132
Timestamp: 2026-08-06T07:44:10.778Z
Learning: For `tools/input/linux/iced_clipboard_check.py`, bare `xdotool` invocation is the established pattern across the CI/developer-only Iced input harness. These environments do not have an untrusted `PATH`. If command-path hardening is added, update every `xdotool` call consistently in a dedicated change rather than changing one call site.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Comment on lines
+219
to
+290
| @pytest.mark.skipif( | ||
| not TEST_MODE, | ||
| reason="window.resize requires ROOST_TEST_MODE=1 in the UI's launch env", | ||
| ) | ||
| def test_sidebar_divider_hairline_only_while_expanded(roost, target, tmp_path): | ||
| """The divider is one 1px column at the sidebar's trailing edge, drawn | ||
| inside `sidebar_width` (plan 016 W1.3) so the terminal grid keeps every | ||
| pixel the sidebar leaves it — a divider that grew its own layout column | ||
| would still look right but would steal a cell. It is absent when the | ||
| sidebar is collapsed, like the Mac's NSSplitView divider.""" | ||
| if target != "iced": | ||
| pytest.skip("iced chrome hairline; mac uses NSSplitView's divider and gtk has none") | ||
|
|
||
| roost.window_resize(WINDOW_W, WINDOW_H) | ||
| _toggle_to_visible(roost) | ||
|
|
||
| artifact_dir = os.environ.get("ROOST_E2E_ARTIFACT_DIR") | ||
| base = Path(artifact_dir) if artifact_dir else tmp_path | ||
| base.mkdir(parents=True, exist_ok=True) | ||
| expanded_path, collapsed_path = base / "divider.png", base / "divider_collapsed.png" | ||
| state: dict = {"samples": [], "collapsed_samples": []} | ||
|
|
||
| def _divider_painted() -> bool: | ||
| metrics = roost.window_metrics() | ||
| sidebar_w = int(metrics["sidebar_width"]) | ||
| if metrics["sidebar_collapsed"] or sidebar_w <= 0: | ||
| return False | ||
| shot = _capture(roost, expanded_path) | ||
| if shot is None: | ||
| return False | ||
| state["x"] = sidebar_w - 1 | ||
| state["samples"] = _column_samples(shot, sidebar_w - 1, _sample_rows(shot[1])) | ||
| return all(color == DIVIDER for _y, color in state["samples"]) | ||
|
|
||
| try: | ||
| roost._wait(_divider_painted, 10.0, "sidebar divider hairline painted") | ||
| except Timeout as exc: | ||
| raise AssertionError( | ||
| f"the sidebar's trailing column (x={state.get('x')}) must render " | ||
| f"DIVIDER #{DIVIDER[0]:02x}{DIVIDER[1]:02x}{DIVIDER[2]:02x} from the " | ||
| f"band rows down; got {_fmt(state['samples'])} " | ||
| f"(screenshot: {expanded_path})" | ||
| ) from exc | ||
|
|
||
| try: | ||
| _toggle_to_collapsed(roost) | ||
|
|
||
| def _collapsed_repainted() -> bool: | ||
| shot = _capture(roost, collapsed_path) | ||
| if shot is None: | ||
| return False | ||
| state["collapsed_samples"] = _column_samples(shot, 0, _sample_rows(shot[1])) | ||
| # The leading column is the tab band once the sidebar is gone, so | ||
| # a band-colored top row proves the relayout landed and the | ||
| # samples below it are worth asserting on. | ||
| return all(c == BAND for y, c in state["collapsed_samples"] if y < BAND_HEIGHT) | ||
|
|
||
| try: | ||
| roost._wait(_collapsed_repainted, 10.0, "collapsed layout repainted to the window edge") | ||
| except Timeout as exc: | ||
| raise AssertionError( | ||
| f"with the sidebar collapsed the tab band must reach x=0; got " | ||
| f"{_fmt(state['collapsed_samples'])} (screenshot: {collapsed_path})" | ||
| ) from exc | ||
|
|
||
| assert all(color != DIVIDER for _y, color in state["collapsed_samples"]), ( | ||
| f"no divider may be drawn while the sidebar is collapsed (Mac hides " | ||
| f"its own); leading column reads {_fmt(state['collapsed_samples'])} " | ||
| f"(screenshot: {collapsed_path})" | ||
| ) | ||
| finally: | ||
| _toggle_to_visible(roost) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace pixel-only functional E2E assertions, or record a narrow exception.
These tests use screenshot pixels as their primary oracle. The repository guideline requires functional E2E tests to assert on textual content rather than pixels. If renderer-only chrome behavior has no textual IPC oracle, document a narrow exception before retaining these pixel guards.
tools/roosttest/test_tab_strip_pixels.py#L219-L290: replace the divider pixel oracle with a textual IPC assertion, or document the renderer-only exception.tools/roosttest/test_tab_strip_pixels.py#L184-L187: replace the title-pixel overflow premise with a textual assertion, or document the renderer-only exception.tools/roosttest/test_sidebar_pixels.py#L297-L328: replace rail and pill color assertions with textual assertions, or document the renderer-only exception.tools/roosttest/test_iced_walking_skeleton.py#L191-L196: replace origin-marker pixel assertions with textual assertions, or document the renderer-only exception.
As per coding guidelines, “Functional E2E tests should drive a real UI through the IPC operation set, use condition waits instead of sleeps, and assert on textual content rather than pixels.”
📍 Affects 3 files
tools/roosttest/test_tab_strip_pixels.py#L219-L290(this comment)tools/roosttest/test_tab_strip_pixels.py#L184-L187tools/roosttest/test_sidebar_pixels.py#L297-L328tools/roosttest/test_iced_walking_skeleton.py#L191-L196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/roosttest/test_tab_strip_pixels.py` around lines 219 - 290, Update
tools/roosttest/test_tab_strip_pixels.py lines 219-290 and 184-187,
tools/roosttest/test_sidebar_pixels.py lines 297-328, and
tools/roosttest/test_iced_walking_skeleton.py lines 191-196 to replace
pixel-based functional E2E oracles with textual IPC assertions. If renderer-only
chrome has no textual IPC oracle, document a narrow, explicit exception at each
applicable site before retaining the pixel guards; preserve condition-based
waits and existing behavior checks.
Source: Coding guidelines
…crisp at scale 1 The rename text_input's default relative line height gives it a fractional intrinsic height (~18.9px); centered inside the 30px pill, the 1px focus border lands on half-pixels, and tiny-skia at scale 1 blends it away — a fuzzy border on real Linux displays, and the CI tiny-skia lane's real-input check counts exact border pixels (24 exact vs 813 near-match in the failure capture; ubuntu wgpu passed because its quad pipeline snaps to integers, macOS passed by layout luck). An absolute 18px line height makes both rename editors exactly 20px tall so the centering offset is integral everywhere. Verified: both backends now render the full crisp border locally (545 exact pixels), and the shed real-input check passes end to end under ICED_BACKEND=tiny-skia — the exact env of the failed lane. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
…ument the pixel-oracle exception The new-tab affordance row still described the pre-016 fixed plus; the shipped control scrolls with the strip (Mac parity, offscreen under overflow accepted). The divider test's docstring now names the renderer-only pixel-oracle exception explicitly (#281/#291 lineage) — these chrome guards have no textual IPC surface, which is the narrow exception roosttest's text-not-pixels rule anticipates. Third finding (absolute xdotool path) skipped: bare `xdotool` is the harness's pre-existing pattern across every call site in a CI/dev-only tool — disposition recorded on the review thread. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chrome parity batch 1 for the iced UI, directed live by Charlie against side-by-side pixels (plan 016; 3e is split — the remainder is the new roadmap §3h). Every design decision was pinned in the interview session; two judgment calls are flagged below for review.
What changed (9 commits)
third_party/inter/, the repo's first bundled binary asset, OFL 1.1) behind a singlechrome_font()seam; weighted sites fixed (the old..Font::default()pattern silently reset the family); PROJECTS semibold, tab labels medium/regular. Terminal cells keep the configured monospace (explicitly-threaded font — verified).BAND #24292cfor header/footer/tab bands + titlebar,LIST #2d3235sidebar region,DIVIDER1px hairline drawn inside the sidebar width (terminal grid math untouched), footer chip re-based to the measured Mac bezel with a 13pt label.TERMINAL_PADDING12→0 so the grid sits flush under the band on all four sides (terminal_top == 32).project – abbreviated cwd, 48-grapheme helper inroost-ui-modelwith 13 unit tests) + macOStitlebar_transparent(titlebar now renders the band color) + Linuxapplication_id(ai.stridelabs.Roost.iced— winit left WM_CLASS empty, unmasked by the dynamic title).test_tab_strip_pixels.pyandtest_z_typography.pyadded to all three CI iced lanes (both were Makefile-only — the Iced: tab-strip renders artifacts with several tabs open (gray bar, tab overlaps +) #281 guard never ran on CI); real-input harness reworked for the new geometry and class-based window discovery.For Charlie's review specifically
#1a1d1eper the brief so the line doesn't notch through the full-height bands. One-constant change if you want literal black.Verification
make check-iced(fmt, 247 unit tests, clippy-D warnings, boundary greps);cargo test --workspaceat the manifest/shared-crate commits; cursor correctness review on every code commit (one finding total, plan-dispositioned).cd/truncation.Dependency impact: Inter font files (binary assets, OFL license vendored),
unicode-segmentation(already in the graph via cosmic-text — one new edge). No engine/IPC changes; GTK and Swift untouched.Known-accepted:
+scrolls offscreen under tab overflow (Mac-parity behavior); #300 batch-cursor caveat neither fixed nor worsened.Plan 016 (full text lives outside the repo; substance transcribed here)
Design decisions were pinned by Charlie in a live side-by-side interview (2026-08-05): adopt the measured Mac colors (bands #24292C / list #2D3235); bundle Inter (SF unredistributable, hidden-font loading fragile, determinism for pixel tests); remove the bell and add Mac-parity sidebar dots; remove ☰ (resolves the collapse-affordance question as "no affordance"); drag-to-collapse; in-strip +; 32px band; zero terminal inset; dynamic title; GTK deliberately not updated (likely retired — divergences recorded in the inventory).
Panel review: GLM 5.2 + CodeRabbit (Codex at usage limit). Key adopted corrections: terminal_tab.rs grid sizing in the padding audit; divider inside the sidebar width (an extra row member would make the grid 1px too wide at zero padding); unified row spec (dot inside the pill); drag-collapse drops the live drag without committing; Inter family-grouping verification step; titlebar spike abort triggers (fullsize_content_view stayed off; not needed).
🤖 Generated with Claude Code
https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e
Summary by CodeRabbit