Review-surface polish: window chrome, review-exit guard, theming, code blocks (attn-rd3j) - #12
Draft
angusbezzina wants to merge 4 commits into
Draft
Review-surface polish: window chrome, review-exit guard, theming, code blocks (attn-rd3j)#12angusbezzina wants to merge 4 commits into
angusbezzina wants to merge 4 commits into
Conversation
…code blocks (attn-rd3j) Nine reported defects in the native app. Three of them shared one root cause and two were misdiagnosed until measured; the notes below record what was actually wrong, since the fixes only make sense against it. Document typography was global, not scoped (attn-rd3j.3/.4/.5). Bare p/h1/ul/li selectors in base.css typed the whole page, so app chrome inherited reading-surface margins and the document's custom checkbox rule drew a checkmark pinned at left:0 inside the Share dialog — the "weird modal checkmark" was never an element in ShareDialog. Typography is now scoped to .attn-doc (carried by the editor mount and the viewer article) and the .attn-chrome opt-out class, which existed purely to undo the leak, is gone. Bullets were a faked absolutely-positioned ::before dot with no list-style fallback, so they vanished whenever the positioning context shifted; they are real ::marker glyphs now, which cannot detach from their line. The table "rail" was 13px of stolen layout (attn-rd3j.8). base.css styles ::-webkit-scrollbar globally, and in WKWebView styling it at all downgrades that element from macOS overlay scrollbars to classic space-reserving ones. Measured alternatives: scrollbar-width:auto = 17px, webkit revert = 17px, hidden = 0px — nothing restores overlay behavior. Prose blocks now hide the bar and scroll by trackpad, the treatment PathBreadcrumb already used. The rules must sit outside @layer components, because base.css's scrollbar rules are deliberately unlayered and unlayered beats layered. Review-exit membership was answered from the wrong set (attn-rd3j.2). ownerRoomForPath resolves a file to a room through the share ROOT, which for a multi-file share is the whole project — so every file "belonged" to the review. Added roomPublishesPath, which answers from published snapshots and reconciles relative snapshot paths against absolute nav paths. The same confusion is fixed in the owner auto-follow effect, which was re-selecting the room right after an explicit exit and putting review chrome on files that were never shared; it now does what its docstring already claimed. BEHAVIOR CHANGE: opening an unshared file in a shared project turns collaboration chrome off instead of leaving the chip and rail on. Syntax highlighting was never a Rust concern (attn-rd3j.10). src/markdown.rs renders no HTML and comrak's syntect feature is off by design — client-side shiki is the intended architecture. The gaps were a hardcoded 20-language allowlist that silently dropped everything else, and untagged fences getting zero decorations. Languages now resolve against shiki's full bundle with on-demand loading, and untagged fences get conservative content-based detection (confident-match-or-nothing; JSON verified by parsing it). Also: a zoom_window IPC so double-clicking the hidden titlebar zooms and restores, attached to every existing drag surface (attn-rd3j.1); the code copy button and language label moved onto a non-scrolling frame so they stay pinned over wide blocks (attn-rd3j.9); dialog bodies, the project switcher and other chrome scroll through the shared ScrollArea (attn-rd3j.5); and a Settings dialog with three-state appearance (Paper/Ink/System, durable and stamped before first paint so there is no flash of the wrong theme) plus shadcn-style typeset presets (attn-rd3j.6/.7). Verified by driving the running app, not just the tests: dblclick zoomed 960x720 -> 1512x887 -> back with buttons excluded; the copy button moved 0px while content scrolled 106px; reserved scrollbar space went 13px -> 0 on every overflowing block; the exit prompt fires, cancel preserves, confirm tears down cleanly and re-entering re-activates; system->dark resolved on a dark-mode Mac and a manual Paper choice survived a daemon restart. That pass also caught a defect of its own — the selected Appearance segment used bg-background over bg-muted/30 and was invisible in dark mode. 97 web test files and 1207 Rust tests pass (17 new unit tests). test-e2e and test-review-e2e are byte-identical to the pre-change baseline, confirmed by stashing and re-running — their failures are pre-existing. Release binary 32.09/40 MiB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
All three were already failing on main; the CI quality job runs
`cargo fmt --check` and `cargo clippy --all-targets -- -D warnings`, and both
gates were red before this branch.
- apply.rs carried a duplicate `use serde::{Deserialize, Serialize}` inside
`mod tests`, unindented at column 0. The test module derives via the fully
qualified `serde::Deserialize`, so the import was dead — and its indentation
was also what `cargo fmt --check` was failing on.
- publish_snapshot_plaintext takes 8 arguments. Allowed with a rationale,
matching the ten existing precedents (two in this same file): the argument
list IS the identity of a published snapshot, so a params struct would move
the same list one level away from the call site without simplifying it.
- The drag-drop handler's nested `if let` collapses into a let-chain, the form
already used elsewhere in this codebase.
Verified: cargo fmt --check clean, cargo clippy --all-targets -D warnings
clean, cargo test --locked green (1207), cargo build --locked green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hm (attn-rd3j.11) Four follow-ups from review of the branch. The Share dialog's file picker was capped at a fixed max-h-72. Added to the surrounding chrome that exceeded the dialog's 85vh ceiling, so the whole modal scrolled and the create button sat below the fold. The cap is now relative to the WINDOW — min(20rem, 34vh) — so list plus chrome always fits and only the list scrolls. Two approaches were tried and rejected first: flex-1 on the list collapses it to its minimum, because the dialog's own height is auto and basis-0 children make an auto-height flex container size to minimums; pinning the ScrollArea viewport with `absolute inset-0` then takes it out of flow, so the root has no intrinsic height and cannot be sized either way. Tables render inside prosemirror-tables' `.tableWrapper`, not the schema's `.prose-scroll-x` — its columnResizing plugin installs a TableView that replaces the schema's DOM. This stylesheet is hand-rolled and deliberately does not import prosemirror-tables' CSS, so that wrapper arrived with NO styles at all: unbounded width, no horizontal scrolling, no frame. It now gets the same treatment as every other wide block, which is also what makes the scrollbar-rail fix (attn-rd3j.8) actually reach tables. Code blocks and tables had zero vertical margin — the `pre` carries an inline margin:0 and the frame had none — leaving them sandwiched between the heading above and the paragraph below. Wide blocks are bordered cards, not running prose, so they take 1.5rem of block margin. The sidebar's filter empty state sat flush against its container while every sibling row is inset 10px. Settings moved from a floating bottom-right cog into the header, immediately right of the share control, and the resident/mute controls merged into the Settings dialog alongside appearance and typeset — one settings surface rather than two. The launch-at-login status listener moved up into App: a dialog is unmounted while closed, so a listener living there would drop every result the user is not watching and reopen showing startup state. Verified live: 30-file share dialog no longer scrolls its body (title and create button both on screen, list scrolls internally at 245px); an overflowing 12-column table scrolls with 0px reserved rail and stays inside the content column; code blocks sit 23px clear of neighbours; empty state insets 10px on both sides; header settings opens and switches theme. 97 web test files pass, clippy and fmt clean, e2e and review-e2e identical to the pre-change baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Fixes nine reported defects in the native app, tracked as epic
attn-rd3j(10 child issues, all closed).Three of the nine shared a single root cause and two were misdiagnosed until measured, so this is organised by what was actually wrong rather than by the original list.
base.csstyped the whole page, not the documentBare
p/h1/ul/liselectors applied everywhere, so app chrome inherited reading-surface margins and the document's custom checkbox rule drew a checkmark pinned atleft: 0inside the Share dialog. The reported "weird modal checkmark" was never an element inShareDialog— it was the leak. Typography is now scoped to.attn-doc, and the.attn-chromeopt-out class that existed purely to undo the leak is deleted along with its six consumers.The bullets were a faked absolutely-positioned
::beforedot with nolist-stylefallback, so they disappeared entirely whenever the positioning context shifted — exactly the reported symptom. They are real::markerglyphs now, which cannot detach from their line.The table "rail" was 13px of stolen layout
base.cssstyles::-webkit-scrollbarglobally, and in WKWebView styling it at all downgrades that element from macOS overlay scrollbars to classic space-reserving ones. That reserved gutter, painted with an always-on thumb, is the rail. Measured alternatives:scrollbar-width: thin)scrollbar-width: autoNothing restores overlay behaviour, so prose blocks hide the bar and scroll by trackpad — the treatment
PathBreadcrumbalready used. The rules must sit outside@layer components, becausebase.css's scrollbar rules are deliberately unlayered and unlayered beats layered; the first attempt silently lost the cascade.Review-exit membership came from the wrong set
ownerRoomForPathresolves a file to a room through the share root, which for a multi-file share is the whole project — so every file "belonged" to the review and the new guard never fired. AddedroomPublishesPath, which answers from published snapshots and reconciles relative snapshot paths (alpha.md) against absolute nav paths.The same confusion is fixed in the owner auto-follow effect, which was re-selecting the room immediately after an explicit exit and putting review chrome on files that were never shared. It now does what its own docstring already claimed.
Important
Behaviour change worth a look: opening an unshared file in a shared project now turns collaboration chrome off, rather than leaving the chip and rail on. I read that as the intended semantics, but it is the one change here that is not purely additive.
Syntax highlighting was never a Rust concern
src/markdown.rsrenders no HTML and comrak'ssyntectfeature is off by design — client-side shiki is the intended architecture and is standard practice. The real gaps were a hardcoded 20-language allowlist that silently dropped everything else, and untagged fences getting zero decorations. Languages now resolve against shiki's full 332-language bundle with on-demand loading (20 preloaded so first paint is instant) plus alias resolution, and untagged fences get conservative content-based detection — confident-match-or-nothing, with JSON verified by actually parsing it and ambiguous blocks left plain. Detected languages are labelled distinctly from author-declared ones.The rest
zoom_windowIPC; double-click zooms and restores, attached to every existing drag surface, sharingdragWindow's interactive-element exclusion.ScrollArea(measured: 0px reserved, vs 13px native).prefs.jsonand stamped into the page before the bundle loads, so launching never shows a frame of the wrong theme.Verification
Driven against the running app, not just the test suite:
system→ dark resolved on a dark-mode Mac; a manual Paper choice survived a daemon restartThat pass caught a defect of its own: the selected Appearance segment used
bg-backgroundoverbg-muted/30and was invisible in dark mode.97 web test files and 1207 Rust tests pass (17 new unit tests).
test-e2e.shandtest-review-e2e.share byte-identical to the pre-change baseline — confirmed by stashing and re-running, so their failures are pre-existing, not introduced here. Release binary 32.09 / 40 MiB.Also included
.beads/embeddeddolt/(58 MB of local Dolt state) was untracked but not ignored — added to.beads/.gitignoreso it cannot be committed by accident.🤖 Generated with Claude Code