Pinned toolbar that reserves its own row

[mairas]

On a page whose only widget captures pointer input — a chart plotter, an embedded webpage — none of the toolbar's reveal gestures reach Skip's shell, because pointer events inside an iframe never reach the embedding document. Measured on the production bundle at 900×800 with a full-page widget-iframe: mouse move, tap and swipe-down over the content all do nothing, the frame occupies y=5..795, and the only surviving route is the 5 px band above it — where a touch user needs two taps, the first raising the peek cue and the second revealing. Full gesture table in #606.

A new "Keep the toolbar on screen" setting holds the toolbar open, and the point of the change is that it does not overlay. .toolbar-host is position: fixed today and the component host has no box, so the shell's flex column gives the toolbar no space; pinned flips both, and the host becomes a non-growing row. The routed content — its flex: 1 1 auto sibling — is then shorter by exactly the toolbar's height. The dashboard grid needed no changes at all: resizeGridColumns() already derives cell height from its own host's clientHeight, and a ResizeObserver on that host already re-runs it.

Pinning outranks the automatic-reveal setting from #495: with it on, ChromeVisibilityService holds revealed true, never arms the idle timer, and makes hide() a no-op, and the auto-reveal toggle reads as disabled.

Verified in a headless browser against the production bundle, on the page from the reproduction:

	toolbar	embedded frame	overlap	toolbar button clickable
pinned	y=0..56	y=61..795, height 734	0 px	yes
default (unpinned)	y=-56, hidden	y=5..795, height 790	0 px	no — it is hidden

The frame shrinking from 790 to 734 is the whole point: reflowed, not covered. The clickability check is Playwright's actionability test, which includes "receives pointer events", so it fails for a control something is covering. The unpinned row is unchanged from before this branch.

npm run ci green — lint, snc, 180 spec files, 7 MCP-schema files. New coverage: the service's pinned mode (revealed survives an idle expiry and an explicit hide, outranks a disabled auto-reveal, hands back to the idle timer when unpinned), the settings hydration and round-trip, the effect that pushes the setting, and the host class that moves the toolbar out of overlay mode.

One deliberate scope call: the setting is global, matching how autoRevealToolbar works, not per-page. The issue asked for "global or per-page". Per-page costs a field on Dashboard plus page-manager UI, and is worth adding only if the global lever proves too blunt.

Closes #606

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: af6b25b6-d409-41ac-9d0a-0c949557ddf9

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mairas]

Code review — 6 personas

Scope: d0827e18..803f71a0. Model: Opus 5, inherited from the session.

Intent: a global setting that both holds the toolbar visible and turns it from an overlay into a real layout row, so a page whose widget swallows every reveal gesture stays navigable. Default behaviour unchanged.

Team: correctness, testing, maintainability, project-standards (always-on) · api-contract (new field on the persisted IAppConfig schema, new public service method) · adversarial (~130 changed non-test lines, app-shell layout).

P1 — High

#	File	Issue	Reviewers	Conf.	Route
1	chrome-visibility.service.ts:93	setPinned(false) revealed unconditionally, and AppComponent pushes both toolbar settings from one effect on every run — so an already-unpinned service was told false routinely, including right after setAutoReveal(false) retracted the boot dwell. Anyone who had turned automatic reveal off got the toolbar back for four seconds on every boot and every settings save: #495 undone, by a PR that changes neither that setting nor its default.	correctness, api-contract, maintainability, adversarial, testing	0.95	fixed

Five of six reviewers reached this independently. It was invisible to CI because both halves were tested in isolation and every AppComponent spec mocks the visibility service, so the effect's two calls were only ever asserted as "was called with".

P2 — Moderate

#	File	Issue	Reviewers	Conf.	Route
2	src/assets/help-docs/welcome.md:21	Three user-facing documents contradicted the shipped feature. welcome.md said "Skip has no permanent navigation bar" and README.md "Skip has no permanent chrome"; embedwidget.md still told users a full-screen cross-origin embed can leave them "stuck with no way to change pages or reveal the toolbar" — the dead end this PR removes. The #495 commit updated welcome.md in the same commit; this one updated nothing.	project-standards	0.82	fixed
3	toolbar.component.spec.ts:289	The new hover-guard test could not fail: it set revealed true before init and never advanced a timer, so the dwell that would have called reveal() was unreachable with or without the guard. It duplicated an existing case and covered nothing.	testing	0.90	fixed
4	toolbar.component.ts:72	Pinned reached the toolbar by two independent paths — the host class read SettingsService while [inert] read chrome.revealed(). Divergence would render a toolbar that looks normal and ignores every click, the hardest failure shape to diagnose.	maintainability, testing, api-contract, correctness	0.75	fixed
5	settings.service.ts:399	An anonymous or read-only session can toggle the setting, gets "Configuration saved", and loses it on reload — patchConfig drops the write and nothing upstream knows. Pre-existing and shared by every display setting; what is new is that this setting is the documented recovery path for a page with no reachable navigation.	adversarial	0.75	filed

P3 — Low

#	File	Issue	Reviewers	Conf.	Route
6	app.component.ts:146	Making pinned a signal put the effect's own write among its dependencies — convergent only because setPinned early-returns. A later edit to that guard turns it into a change-detection loop rather than a wrong value.	adversarial	0.70	fixed
7	display.component.html:156	The pin helper text described a per-page choice and omitted the profile-wide scope its sibling states, so a user pinning on a phone had no warning that a wall-mounted display loses 56 px too.	api-contract, correctness	0.70	fixed
8	toolbar.component.ts:71	@HostBinding was the only one in src/app; the three other components that bind on their host use host: metadata. It also forced a second member for the same value.	maintainability, project-standards	0.82	fixed
9	settings.service.ts:399	setPinToolbar's write path had no spec. A wrong source expression would persist the wrong flag, work in-session, and come back wrong after reload, with every test green. The #495 field has exactly this guard.	testing	0.80	fixed
10	display.component.html:164	The [disabled] binding is the only thing stopping the UI offering two settings that contradict each other, and nothing asserted it.	testing	0.80	fixed

Applied fixes

Three commits, ba69a488, 803f71a0 and the docs in ba69a488:

• setPinned is idempotent, and a genuine unpin resolves to the governing rule — the idle timer, or an immediate retraction on a display whose owner turned automatic reveal off — instead of always revealing. The first attempt at this routed the unpin through revealAuto(), which left the toolbar stuck open when auto-reveal was off; the spec caught it.
• Pinned state moved into ChromeVisibilityService as a signal. The host class and revealed now come from one place and cannot disagree, and the host binding moved to host: metadata.
• The effect reads its settings first and pushes them untracked, so the service's signal guards are no longer part of its dependency set.
• Docs: welcome.md gains the setting beside its sibling and both blanket claims became "by default"; embedwidget.md now points at the setting instead of describing a dead end; the helper text states the profile-wide scope.
• Tests: the hover guard rewritten to run the dwell to completion with the toolbar hidden; a pinned toolbar asserted non-inert; setPinToolbar persistence; the [disabled] binding; and an AppComponent block that runs against the real ChromeVisibilityService rather than a mock.

Two of these were checked against deliberately broken builds rather than assumed: removing the pinned guard fails the rewritten hover test, and restoring the old setPinned fails the new boot test. Both pass once restored.

Verification

Headless Chromium against the production bundle, on the page from the reproduction in #606:

pin	auto-reveal	toolbar after the boot dwell	embedded frame	overlap	clickable
on	on	y=0..56, shown	y=61..795, h=734	0 px	yes
on	off	y=0..56, shown	y=61..795, h=734	0 px	yes
off	off	y=-56, hidden	y=5..795, h=790	0 px	no (hidden)

Row three is finding 1's regression case, now correct. Row two is the precedence claim. The frame at 734 vs 790 is the reflow.

npm run ci green — lint, snc, 180 spec files, 7 MCP-schema files.

Pre-existing (filed)

• #611 — Display settings reports "Configuration saved" in a session that cannot persist (finding 5).

Coverage

Suppressed below the 0.60 confidence gate: none reported. No reviewer failed or timed out. Untracked files excluded from scope: four perf-harness/shot-*.mjs scratch scripts from earlier sessions.

Known gaps, unfixed on purpose. The layout itself — the rules that turn the host class into a reserved row — is CSS, and jsdom does not lay out, so deleting the :host(.pinned) block would leave every spec green while the toolbar went back to covering the page. Three reviewers said so independently; the browser table above is the only evidence, and it is not repeatable in CI. resizeGridColumns likewise has no spec, so the "the grid already handles it" claim rests on the same measurement. Closing either properly means a CI-run browser check, which this repo does not have outside perf-harness.

Verdict: ready to merge. The P1 and every P2 in the diff are fixed and verified; the one pre-existing P2 is filed.