feat(ui): add per-surface typography customization - #1318
Open
SyahrulBhudiF wants to merge 16 commits into
Open
Conversation
…graphy # Conflicts: # apps/pi-extension/server/serverAnnotate.ts # apps/pi-extension/server/serverPlan.ts # apps/pi-extension/server/serverReview.ts # packages/server/annotate.ts # packages/server/index.ts # packages/server/review.ts # packages/shared/config.ts # packages/ui/components/ThemeProvider.tsx # packages/ui/components/ThemeTab.tsx # packages/ui/config/settings.ts
…o feat/per-surface-typography
The committed JS hash (viewer.C4G-XTIH.js) does not reproduce from any build of this tree, so shipping it would pin every exported guide and every hosted share page to an asset that does not exist under guides.show /v1/ — and /v1/ is add-only, so nothing would ever publish it after the fact. Regenerating the manifest is a maintainer release step (build:viewer + sync:manifest, verified against what is actually uploaded), not part of a typography feature. Restoring main's manifest also removes this branch's only merge conflict with main.
--pn-display-font / --pn-mono-font are defined on [data-pn-surface], so they only resolve inside that subtree — and an unresolvable var makes the whole declaration invalid rather than inherited. Plenty of the UI renders outside it: the review annotation toolbar and the Settings dialog portal to document.body, Base UI popovers mount at the body with no container, and the external line-annotation composer is a sibling of the surface div. Every bare reference there lost monospace at DEFAULT settings, with no typography configured at all. The same stylesheet is bundled into the guides.show viewer, so it shipped there too. Gives all 44 references the palette token as their fallback, the form theme.css already used once for the shortcut key cap, and adds a test that greps both stylesheets so a new bare reference cannot come back.
Dropping ", monospace" from the injected font-family means a user whose chosen face is unavailable falls through to the surrounding proportional font, and the diff pane is column-aligned — that is broken output, not a cosmetic downgrade. It reaches legacy diffFontFamily users and the read-only guides.show viewer, which still passes a bare family name. monoFontStack() handles both shapes: it quotes a bare family, leaves an existing CSS stack alone, and appends the generic only when the stack does not already end in one (a generic keyword is never quoted, since '"monospace"' is a family name).
The Code Font picker is gone but diffOptions.fontFamily stayed live on disk with no UI, and six of its nine faces (Hack, Inconsolata, Red Hat Mono, Roboto Mono, Source Code Pro, Atkinson Hyperlegible Mono) had no counterpart in the new catalog, so anyone who had picked one could neither see it nor choose it again. Folds all nine into FONT_CATALOG (ids mirrored in core's allowlist, kept in step by a test) and deletes utils/diffFonts.ts, so there is now ONE stylesheet injector and one URL per family — the old and new catalogs overlapped on four families at different weight ranges, which would have fetched the same face twice. migrateLegacyDiffFont() runs once, right after configStore.init(): it seeds typography.review.mono from the legacy value (catalog entry when the family matches, else a custom stack so a hand-edited name is not lost) and then clears the legacy key. Clearing is what makes it idempotent — without it, a user who later picked "Theme default" would read as "not migrated yet" and have the old font resurrected on the next reload. After it runs, typography is the only source of truth. Also adds a 10s timeout to the catalog loader (a stylesheet that neither loads nor errors left Settings saying "Loading font..." forever) and braces the loadFont effect, which was returning a Promise where React expects a cleanup function.
…d writes saveConfig runs for every setting, so when the value on disk failed to parse (a hand-edited typo, or a profile written by a newer build) currentTypography.ok was false and the key was dropped — the next theme toggle silently deleted the user's whole typography block. Preserve the raw value instead. Readers already validate before use, so a bad block stays inert and visible to be corrected rather than vanishing. Adds the test for that direction; the existing one only covered an invalid INCOMING value.
Adds it to the config-only settings section: the per-surface shape, catalog ids vs custom CSS stacks (and which are CDN-delivered), the all-or-nothing validation and what an unparsable block does, snapshot replace semantics, and the diffOptions.fontFamily migration. CLAUDE.md is a symlink to AGENTS.md, so one edit covers both.
…nges Regenerated with a frozen-lockfile environment; two consecutive builds reproduce these hashes byte-identically. Replaces the original PR's pin, which was built in a drifted local environment and did not reproduce. The new viewer assets still need the release-time upload to guides.show/v1/ before this feature's exports are live.
…ment keys Union resolutions in the config allowlists and settings registry (both runtimes), and the guide-viewer manifest regenerated on the combined tree in a frozen-lockfile environment (two consecutive builds reproduce the hashes).
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
2 participants
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.
Pull Request: Per-surface typography customization
Summary
Implements Issue #1275.
Plannotator users can now customize typography independently for each surface:
Each surface supports separate:
Example
The Typography settings use the same theme-card selection UI as the existing theme picker. The selected font is shown with the active theme styling and a live preview.
Included
font-familystack supportVerification
Passed focused tests and type checks:
Closes #1275.
Maintainer follow-up
Pushed on top of the contributor's commits. The feature design, config durability and Pi parity were already sound; these are the review's fix list.
1. Reverted the viewer manifest bump.
packages/core/guide-viewer-manifest.tspinnedviewer.C4G-XTIH.js, a hash that does not reproduce from any build of this tree. Since guides.show/v1/is add-only, shipping it would have pointed every exported guide and hosted share page at an asset that will never exist there. Manifest regeneration is a maintainer release step, verified against what is actually uploaded. This also removed the branch's only merge conflict with main.2. Fallbacks on every per-surface font var.
--pn-display-font/--pn-mono-fontare defined on[data-pn-surface], so they only resolve inside that subtree, and an unresolvablevar()invalidates the whole declaration rather than inheriting. Large parts of the UI render outside that subtree: the review annotation toolbar and Settings dialog portal todocument.body, Base UI popovers mount at the body with no container, and the external line-annotation composer is a sibling of the surface div. All of those lost monospace at default settings, with no typography configured, and the same stylesheet is bundled into the guides.show viewer. All 44 references now carry the palette token as their fallback, the form the PR already used once attheme.css:1101. A new test greps both stylesheets so a bare reference cannot come back.3. Restored the generic monospace fallback in the diff CSS.
usePierreTheme.tsandDiffHunkPreview.tsxhad dropped, monospace, so a legacydiffFontFamilyuser whose face is unavailable fell through to the surrounding proportional font. The diff pane is column-aligned, so that is broken output. A sharedmonoFontStack()handles both value shapes: it quotes a bare family name (still what the read-only guides.show viewer passes), leaves an existing CSS stack alone, and appends the generic only when the stack does not already end in one.4. Migrated
diffFontFamily. The PR removed the Code Font picker but left the setting live with no UI, and six of its nine faces (Hack, Inconsolata, Red Hat Mono, Roboto Mono, Source Code Pro, Atkinson Hyperlegible Mono) had no counterpart in the new catalog, so anyone who had chosen one could neither see it nor pick it again. All nine are now folded intoFONT_CATALOG, with the ids mirrored in core's allowlist and kept in step by a test, andutils/diffFonts.tsis deleted, so there is one stylesheet injector and one URL per family. The two catalogs previously overlapped on four families at different weight ranges, which would have fetched the same face twice.migrateLegacyDiffFont()runs once, immediately afterconfigStore.init(). It seedstypography.review.monofrom the legacy value, as a catalog entry when the family matches and otherwise as a custom stack so a hand-edited name is not lost, and then clears the legacy key. Clearing is what makes it idempotent: without it, a user who later chose "Theme default" would read as "not migrated yet" and have the old font resurrected on the next reload. After it runs, typography is the only source of truth for the review face.5. Fixed
saveConfigdropping an unparsabletypography.saveConfigruns for every setting, so when the value on disk failed to parse (a hand-edited typo, or a profile written by a newer build) the key was deleted by the next unrelated write, such as a theme toggle. The raw value is now preserved. Readers already validate before use, so a bad block stays inert and visible to be corrected. Added the test for that direction; the existing one only covered an invalid incoming value.6. Documented the
typographykey in the config-only settings section ofAGENTS.md(CLAUDE.mdis a symlink to it, so one edit covers both): the per-surface shape, catalog ids versus custom CSS stacks and which are CDN-delivered, the all-or-nothing validation and what an unparsable block does, snapshot replace semantics, and the migration.Also fixed two smaller things found on the way: the catalog loader now gives up after 10 seconds, so a stylesheet that neither loads nor errors no longer leaves Settings saying "Loading font..." forever, and the
loadFonteffect is braced, since it was returning a Promise where React expects a cleanup function.CDN font delivery is kept as designed.
Verification
bun run typecheckclean.bun test: 3646 pass, 15 fail, all environmental and a strict subset of the 17 failing on the PR head before these commits.DOM_TESTS=1 bun test packages/ui: 1047 pass, 0 fail.DOM_TESTS=1acrosspackages/ui packages/core packages/shared: 1177 pass, only the 3 network-dependent live paste-service tests failing.bun run --cwd apps/review buildandbun run build:hookboth succeed; the built review, hook and guides HTML contain zero bare--pn-mono-font/--pn-display-fontreferences and 45 fallback-carrying ones.git merge-treeagainstorigin/main: zero conflicts.AI-assisted (Claude) under maintainer direction.