diff options
| author | Craig Jennings <c@cjennings.net> | 2026-06-23 19:34:01 -0400 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-06-23 19:34:01 -0400 |
| commit | c5ca8b7d7ac1aa751c1bf79ad35b178f96b3ba77 (patch) | |
| tree | 06a771c8b21eb9b05ede74dd63fd475bdd4dbd60 /docs | |
| parent | 558723421f320d00a1d9c7704cae567a00e17310 (diff) | |
| download | dotemacs-c5ca8b7d7ac1aa751c1bf79ad35b178f96b3ba77.tar.gz dotemacs-c5ca8b7d7ac1aa751c1bf79ad35b178f96b3ba77.zip | |
feat(theme-studio): locate preview elements by hover and click
Hovering a data-face preview element shows its section, face, and effective value in the preview-label info line, and the element's title carries the full record: effective fg/bg plus a per-attribute source note (direct, inherited-from-X, default, or cleared-rendering-as-default). Clicking an on-pane element scrolls to and flashes its assignment row. Off-pane and cross-surface elements stay hover-only.
A single owner-qualified registry keyed by {owner, face} backs both data-face surfaces, package and UI, so the same face name under two owners never collides. The pure helpers in app-core.js take all state as arguments and return data. The one stateful adapter, previewSpan, lives in previews.js and emits the escaped markup. os() stays a package-owner wrapper over previewSpan, and a unified locateClick dispatcher replaces the per-surface click branches.
Covered by test-locate.mjs and four new browser gates. Full harness green.
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/specs/theme-studio-preview-locate-spec.org | 271 |
1 files changed, 237 insertions, 34 deletions
diff --git a/docs/specs/theme-studio-preview-locate-spec.org b/docs/specs/theme-studio-preview-locate-spec.org index bb77a2248..dee27e8c4 100644 --- a/docs/specs/theme-studio-preview-locate-spec.org +++ b/docs/specs/theme-studio-preview-locate-spec.org @@ -1,6 +1,6 @@ :PROPERTIES: :ID: fbcf0e20-1328-42b4-aa36-3401509e7816 -:STATUS: not-started +:STATUS: ready-pending-go :END: #+TITLE: Theme Studio Preview Element Locate — Spec #+AUTHOR: Craig Jennings @@ -8,14 +8,14 @@ #+TODO: TODO | DONE SUPERSEDED CANCELLED * Metadata -| Status | not-started | -|----------+----------------------------------------------------------------| -| Owner | Craig Jennings | -|----------+----------------------------------------------------------------| -| Reviewer | Craig Jennings | -|----------+----------------------------------------------------------------| -| Related | [[file:../../todo.org][todo.org: theme-studio preview locate + org-agenda app]] | -|----------+----------------------------------------------------------------| +| Status | Ready pending Craig's go — four reviews incorporated (Codex, 2026-06-23) | +|----------+------------------------------------------------------------| +| Owner | Craig Jennings | +|----------+------------------------------------------------------------| +| Reviewer | Craig Jennings | +|----------+------------------------------------------------------------| +| Related | [[file:../../todo.org][todo.org: theme-studio preview locate + org-agenda app]] | +|----------+------------------------------------------------------------| * Summary @@ -28,10 +28,10 @@ Previews render representative text styled by many faces, but there is no way to * Goals and Non-Goals ** Goals -- Hover any previewed element to see its section (owning app), element (face name), and current value/attributes. +- Hover any data-face previewed element to see its section (owning app), element (face name), and current value/attributes. - Click an element whose face is assigned on the current pane to flash it and scroll/select its assignment row. - Leave off-pane elements non-clickable, with the hover still naming section and element so the user can navigate there. -- A face -> (owning app, value, attributes) registry built from the app/face model, read by previews for both hover content and the clickable test. +- A face -> (owning app, value, attributes) registry spanning the two data-face surfaces (the UI mock via UIMAP, package previews via PKGMAP/APPS), read by previews for both hover content and the clickable test. - Let previews render substrings in faces owned by other panes live from the shared state (the org-agenda agenda mock is the first to need this); those render correctly and are the hover-only, non-clickable elements. ** Non-Goals @@ -41,21 +41,53 @@ Previews render representative text styled by many faces, but there is no way to - Not a search/filter over faces; this is point-and-identify, not query. ** Scope tiers -- v1: the face registry; hover tooltips (section + element + value/attributes) on every previewed element; click-to-row for current-pane faces; off-pane elements non-clickable; the os() preview tag that carries each element's face; one existing preview wired as the showcase. -- Out of scope: cross-pane click navigation; editing from the preview; persisting any of this. -- vNext: a "reveal in pane" affordance for off-pane elements (switch pane + scroll) if the hover-only model proves too manual. +- v1: the face registry over the two data-face surfaces (UI mock + package previews); hover tooltips (section + element + value/attributes) via the =title= attribute on every data-face previewed element; click-to-row for current-pane faces; off-pane elements non-clickable; the os() preview tag carrying owning app + face + an on-pane flag; updated preview gates that validate each data-face against its owner app; a gate-only synthetic showcase fixture (one package-owned + one =@ui= off-pane span) — no user-facing preview changes in v1. +- Out of scope: the syntax/code (=data-k=) tier in the registry — it already has its own =cp.onclick= -> =flashAssign= locate path; cross-pane click navigation; a custom (non-=title=) tooltip; editing from the preview; persisting any of this. +- vNext: folding the syntax/code tier into the unified registry; a focusable hover/focus info strip for keyboard-only wayfinding (v1 is pointer-driven); a "reveal in pane" affordance for off-pane elements (switch pane + scroll) if the hover-only model proves too manual. * Design ** For the user -Move the pointer over any colored element in a preview. A tooltip names the section it belongs to (the app, e.g. "org-faces"), the element (the face, e.g. "org-faces-todo"), and its current value and attributes (e.g. foreground #8fbf73, bold). If that face is one you can edit on the pane you are looking at, click it: the element flashes and the pane scrolls to and highlights its assignment row, ready to tune. If the element is colored by a face from another pane -- a keyword in the agenda is owned by org-faces, a tag by org-mode -- clicking does nothing, but the tooltip has already told you which section and element to go find. Every preview becomes a legend you can interrogate. +Move the pointer over any colored element in a preview. A tooltip names the section it belongs to (the app, e.g. "org-faces"), the element (the face, e.g. "org-faces-todo"), and its current value and attributes (e.g. foreground #8fbf73, bold). If that face is one you can edit on the pane you are looking at, click it: the element flashes and the pane scrolls to and highlights its assignment row, ready to tune. If the element is colored by a face from another pane -- a keyword in the agenda is owned by org-faces, a tag by org-mode -- clicking does nothing, but the hover has already told you which section and element to go find. As you move over elements, a single info line in the preview-label area updates immediately with "section > face — value", so wayfinding doesn't wait on the browser's delayed native tooltip; the =title= stays as the deterministic fallback. (v1 wayfinding is pointer-driven; keyboard-focus traversal of preview spans is a vNext accessibility item.) Every preview becomes a legend you can interrogate. ** For the implementer -- Registry: a derived map from face name to its owning app, current value, and resolved attributes, built from the same app/face assignment state the editor already holds. One source for hover content and the is-on-this-pane test. Rebuilt when assignments change. -- Preview tagging: the os(app, face, text) preview helper already wraps each element in a span keyed by face; extend it to also carry the face name and owning app as data attributes so hover and click can resolve without re-parsing. -- Hover: a tooltip (or info strip) that reads the registry for the hovered element's face and shows section + element + value/attributes. -- Click: if the element's face is owned by the current pane, flash the element and scroll/select its assignment row; otherwise the element is non-interactive (cursor and affordance reflect that). -- Cross-pane rendering: os() resolves a face's current color from the registry regardless of which pane owns it, so a preview can faithfully render off-pane faces (the agenda mock's keywords/priorities/tags) while marking them non-clickable. +- Registry: a derived map keyed by *owner-qualified identity* — ={surface, owner, face}=, where surface is =ui= or =package=, owner is an internal key (=@ui= for the UI surface, the app-key for a package), and face is the face name. Keying by face name alone is unsafe: the same face name can appear under two owners on one DOM surface, so the owner is part of the key, and =data-owner-app= carries that internal owner key (never the display label like "UI faces", which is shown separately). Each entry holds the owning app/surface, current value, and resolved attributes, built from the assignment state the editor already holds (UIMAP for UI faces, PKGMAP/APPS for package faces). The syntax/code tier (=data-k=, keyed by syntax *kind* not face) is deliberately not in the registry for v1; its code preview already locates via =cp.onclick= -> =flashAssign=. All lookups (=locateFaceMeta=, =isLocateOnPane=, =previewFaceAttrs=, =assertPreviewFaces=) take =(owner, face)=, never face alone. +- Preview tagging: rendering goes through =previewSpan(owner, face, text)=, which dispatches by the owner's surface — a package owner uses the existing =ofs=/=PKGMAP= path, =@ui= uses the =uiCss=/=resolveUiAttr=/=UIMAP= path — and emits the shared locate attributes in both branches: =data-owner-app= (the owner key), =data-face=, and a derived =locate-onpane= flag set when the owner is the pane being viewed. =os(app, face, text)= stays as a thin backward-compatible wrapper that calls =previewSpan= with the package owner, so existing previews are unchanged. +- Hover: read the registry for the hovered element's =(owner, face)= and show section + element + value/source (see the tooltip contract below). +- Click: if =locate-onpane= is set, scroll the element's assignment row into view and flash it (=flashRow=) in the owning pane's table (=flashUi= for UI faces, =flashPkg= for package faces). "Select" is deliberately not used — v1 is the =flashRow= animation only, no persistent selected-row state. Off-pane and unassigned elements have no click handler. +- Affordance: on-pane spans get =cursor:pointer= plus a =locate-onpane= class (the class the gate checks); off-pane and unassigned spans get the default cursor and no handler. Every element's =title= begins with the owning section, so an element that doesn't respond to a click explains why on hover rather than feeling broken. +- Cross-pane rendering: =os= resolves a face's current color from the registry regardless of which pane owns it, so a preview can faithfully render off-pane faces (the agenda mock's keywords/priorities/tags; the completion preview's minibuffer-prompt and highlight) while marking them hover-only. +- Gates: =assertPreviewFaces= currently asserts every =data-face= belongs to the *current* app's faces; change it to validate each element against its =data-owner-app='s face set (UIMAP keys for UI faces, =APPS[owner].faces= for package faces), so an intentional cross-pane span passes instead of failing. + +** Implementation shape (refactor boundaries) +The current paths spread preview ownership across global =PKGMAP= / =UIMAP= / =APPS= state, =os= in previews.js, click handlers in =buildMockFrame= / =buildPkgPreview=, the flash helpers in app.js, and the membership check in browser-gates.js. To keep the state matrix determinate, the logic lives as pure helpers in =app-core.js= (already the home of =faceCss=, =faceAttrs=, =buildPkgmap=, =packagesForExport=), with DOM code as thin adapters: +Pure helpers in app-core.js — all state passed in, no globals, returning data never HTML: +- =buildLocateRegistry(apps, pkgmap, uimap, map)= — the derived ={surface,owner,face}= -> value/attributes/source map (=map= is the ground palette for the =effFg=/=effBg= floors). +- =locateFaceMeta(owner, face, registry)= — value + attributes + per-attribute source for one owner-qualified face. +- =formatLocateTitle(meta)= — the =title= string from effective value + source notes. +- =previewFaceAttrs(owner, face, registry)= / owner validation — the owner-aware membership check the gate calls. +- =isLocateOnPane(owner, currentApp)= — the clickable predicate (recomputed at render time, not cached). + +The one stateful piece, =previewSpan(owner, face, text)=, lives in previews.js (not app-core.js): a thin adapter that reads the current globals (PKGMAP / UIMAP / MAP), dispatches by surface to =faceCss= (package) or =uiCss= (UI), calls the pure helpers for the locate attributes, owns HTML escaping (=esc=), and emits the span. =os= delegates to it for package owners. This is the boundary: pure data helpers in app-core.js (Node-tested), the escaped-HTML renderer in previews.js (browser-gated). + +After the gates are green, the post-feature cleanup (Phase 5) replaces the =buildPkgPreview= / =buildMockFrame= face-click branches with a single locate dispatcher; the =data-k= syntax-click path stays separate for v1. + +** Tooltip contract (v1) +The deterministic hover surface in v1 is the element's native =title= attribute (the preview-label info line mirrors it on mouse hover — see wayfinding). =value= is always the *effective rendered* color — what the eye actually sees — never the raw assignment, because a cleared or unset attribute still renders a floored/inherited default and a tooltip that disagreed with the pixels would mislead. The =title= reads, comma-separated: +- *section* — the owning app / surface display label (e.g. "org-faces", "UI faces"). +- *element* — the face name (e.g. "vertico-current"). +- *value* — the effective rendered foreground and, when set, background as hex (after =effFg=/=effBg= flooring and inherit resolution). +- *attributes* — the non-default ones among weight, slant, underline, strike, box, inherit, height, inverse, extend. +- *source note* — per attribute, how that effective value arose: "direct", "inherited from <face>", "default foreground" / "default background", or "cleared, rendering as default". + +The source note is what disambiguates the states the value alone can't: +- *direct* — a value was assigned on this face. +- *inherited* — the value comes from an =:inherit= (package via =effResolve=, UI via =resolveUiAttr=); names the source face. +- *cleared* — the assignment was explicitly removed; the note says "cleared, rendering as default fg/bg" while =value= still shows the rendered default, so the tooltip matches the pixels. +- *unassigned* — the face resolves to no owning app; the element is hover-only, never a dead click. + +** Registry lifecycle +One cached, module-level registry. It is rebuilt *once per assignment / import / reset / clear batch*, before preview spans render — hooked into the existing rebuild points (=uiSelect=->=paintUI=/=buildMockFrame=, =pkgChanged=, import replacing =UIMAP=/=PKGMAP=, =resetApp=/=resetUnlocked=) — never per hover and never per =previewSpan= call. Palette edits that change effective displayed values without changing ownership refresh the values on that same rebuild. =locate-onpane= is *not* stored in the registry: it's recomputed from the current view at render time, because switching the viewed pane changes clickability but not ownership. Budget: registry build is linear in face count, does no DOM queries, and a pure-Node test holds it under a small millisecond threshold over the full UI+package face set. * Alternatives Considered @@ -101,41 +133,153 @@ Move the pointer over any colored element in a preview. A tooltip names the sect - Decision: os() resolves any face's current color from the registry, so previews render off-pane faces live; those elements are the non-clickable, hover-only ones. - Consequences: easier -- previews read real and the agenda mock is possible; harder -- the preview must distinguish own-pane (clickable) from off-pane (hover-only) elements. +* Review findings [14/14] + +** DONE Registry scope does not cover all preview element tiers :blocking: +The spec promises "every rendered preview element" and "every previewed element", but the current theme-studio has three separate assignment surfaces: syntax/code preview spans use =data-k= and =SYNTAX/MAP= with =flashAssign=, the UI mock uses =data-face= and =UIMAP= with =flashUi=, and package previews use =data-face= and =PKGMAP/APPS= with =flashPkg=. A face -> owning-app registry built only from the app/face model would cover package panes but not syntax categories or UI faces, so an implementer would have to invent whether v1 is package-preview-only or how =@code= / =@ui= elements enter the registry. Define the registry's full key and owner model across all three tiers, or explicitly narrow v1 to package previews and adjust the "every previewed element" acceptance criteria. (blocking) + +Disposition: *modified.* The finding is right that the unscoped "every previewed element" language hides three surfaces; verified in code (data-k/flashAssign syntax, data-face/UIMAP/flashUi UI mock, data-face/PKGMAP/flashPkg package). Rather than build a registry across all three, v1 covers the *two data-face surfaces* — the UI mock (UIMAP) and package previews (PKGMAP/APPS) — and explicitly leaves the syntax/code tier out of the registry, because that tier already has its own go-from-render-to-assignment path (=cp.onclick= -> =flashAssign= on =data-k= spans). Scoping to the data-face surfaces is also what the two known consumers actually need: the org-agenda showcase mixes org-faces and org-mode (both package faces), and the planned minibuffer-completion preview needs minibuffer-prompt and highlight (UI faces). Folded into Goals, Scope tiers, Design (implementer registry model), and the acceptance criteria, which now say "every data-face previewed element." + +** DONE Cross-pane preview contract and gates are underspecified :blocking: +The current =os(app, face, text)= helper both styles an element and emits only =data-face=, while =buildPkgPreview= clicks any =data-face= by calling =flashPkg= against the current =#pkgbody= row. The browser helper =assertPreviewFaces= currently asserts every preview =data-face= belongs to the current app's =APPS[app].faces=. A preview that intentionally renders =org-faces= or =org-mode= faces from the future =org-agenda= pane will either fail the existing gate or silently produce non-clicks with no explicit hover/cursor contract. Specify the data attributes and tests for cross-pane spans, e.g. =data-owner-app= plus =data-face= and a derived clickable class/flag, state that =os= takes the owning app rather than the current preview app, and update preview gates to validate off-pane faces against their owner app. (blocking) + +Disposition: *accepted as written* — precise and code-correct. Verified: =os= at previews.js:6 emits only =data-face=; =fr.onclick=/=buildPkgPreview= route by =data-face= against the current pane; =assertPreviewFaces= (browser-gates.js:41) checks membership in the current app's faces. Folded the contract into Design (implementer): =os= carries the owning app and emits =data-owner-app= + =data-face= + a derived =locate-onpane= flag; click routes by owner (on-pane -> flash that pane's row; off-pane -> inert with a non-interactive cursor); =assertPreviewFaces= validates each =data-face= against its =data-owner-app='s face set (UIMAP for UI faces, PKGMAP/APPS for package faces) rather than the current app. Reflected in Phases 2 and 4, the acceptance criteria, and a gate note. + +** DONE Tooltip value formatting is not testable yet +The spec says the hover shows current value and attributes, but the current model has raw assignments, effective inherited values, cleared/default source states, and extra attributes such as weight, slant, underline, strike, box, inherit, height, inverse, and extend. Without a minimal display contract, tests can only assert that "some value" appears, and implementations may disagree on whether to show raw =fg=null=, effective foreground, inherited source, or only non-default attributes. Define the v1 tooltip fields and fallback strings for assigned, default/inherited, cleared, and unassigned elements; this can reuse the existing =title= hover surface if a custom tooltip is not part of v1. (non-blocking) + +Disposition: *accepted*, taking the reviewer's own escape hatch — v1 reuses the existing =title= attribute as the hover surface (no custom tooltip), with a defined field contract so it's testable. Added a "Tooltip contract (v1)" subsection to Design fixing the fields (section, face, effective fg/bg, non-default attributes) and the four fallback strings (assigned / inherited-from-X / cleared / unassigned). Reflected in the acceptance criteria and the Observability + Errors readiness dimensions. + +** DONE Deterministic test matrix is not explicit enough :blocking: +The spec names "unit-tested", "browser-gate tested", and "one existing preview wired as the showcase", but that is not enough to judge whether the corner cases are covered. This feature has a small state matrix that can fail silently: UI-owned on-pane, package-owned on-pane, package-owned off-pane, UI-owned off-pane from a package preview, missing owner, inherited value, cleared value, explicit fg/bg, non-default structural attributes, assignment edits that rebuild the registry, and view switches that change the on-pane flag. V1 should require a deterministic test matrix: pure Node tests for registry construction, owner lookup, tooltip formatting, and owner-aware preview-face validation; browser gates for exact =title= strings, =data-owner-app= / =locate-onpane= attributes, on-pane click flashing the correct row, off-pane click leaving rows unflashed, stale-registry prevention after an edit, and =assertPreviewFaces= accepting intentional off-pane spans while rejecting bad owners. Manual inspection can remain only for the subjective "is this discoverable enough?" check. (blocking) + +Disposition: *accepted as written.* The matrix matches the project's deterministic-test discipline and the existing harness — run-tests.sh auto-discovers pure-Node =*.mjs= tests and drives headless-Chrome hash gates, so both halves have a home. Added a "* Testing strategy" section enumerating the full state matrix split into pure-Node unit cases (registry construction, owner lookup, tooltip formatting including the four fallbacks, owner-aware face validation) and browser gates (exact =title= strings, =data-owner-app= / =locate-onpane= attributes, on-pane click flashes the right row, off-pane click leaves rows unflashed, post-edit registry rebuild, =assertPreviewFaces= accepts intentional off-pane spans and rejects bad owners), with manual inspection limited to the subjective discoverability read. Mirrored in the acceptance criteria and the Dev-tooling dimension. + +** DONE Refactoring boundaries are missing before the feature adds more coupling :blocking: +The current implementation already spreads preview ownership across global =PKGMAP= / =UIMAP= / =APPS= state, =os= in =previews.js=, click handlers in =buildMockFrame= and =buildPkgPreview=, row flash helpers in =app.js=, and gate membership checks in =browser-gates.js=. Adding owner-aware registry, tooltip formatting, click routing, and cross-pane rendering directly inside those paths will multiply special cases and make the test matrix harder to keep determinate. Specify the refactor shape before implementation: extract pure helpers in =app-core.js= (or another imported core module) for =buildLocateRegistry=, =locateFaceMeta=, =formatLocateTitle=, =previewFaceAttrs= / owner validation, and =isLocateOnPane=; keep DOM code as thin adapters that call those helpers. Also name the after-feature cleanup: replace the direct =buildPkgPreview= / =buildMockFrame= face-click branches with one locate dispatcher once the gates are green, while leaving the =data-k= syntax path separate for v1. (blocking) + +Disposition: *accepted*, with the refactor split into bracketing phases so each leaves a working tree. Confirmed app-core.js is already the pure-helper home (faceCss, faceAttrs, buildPkgmap, packagesForExport are pure and Node-tested), so the named helpers fit the existing pattern. Added "Phase 0 — Pure-helper extraction" before the feature work (buildLocateRegistry, locateFaceMeta, formatLocateTitle, previewFaceAttrs/owner-validation, isLocateOnPane in app-core.js, DOM left as thin adapters, each helper unit-tested) and "Phase 5 — Locate-dispatch cleanup" after the click phase (replace the buildPkgPreview / buildMockFrame face-click branches with one locate dispatcher once gates are green; the data-k syntax path stays separate for v1). Recorded in an "Implementation shape" note in Design. + +** DONE Preview affordance and row selection semantics are still ambiguous +The spec says off-pane elements are "non-clickable" and current-pane elements "scroll/select" their assignment row, but it does not define the visible affordance. Existing CSS already makes =.mock [data-face]= look clickable, and package preview spans may inherit no distinct cursor at all; an inert off-pane element with a delayed native title can feel broken rather than intentionally hover-only. V1 should define exact affordances: on-pane spans get pointer cursor plus whatever clickable class/name the gate checks; off-pane and unassigned spans get default/help cursor and no click handler; the title begins with the owning section so the user can tell why a click is unavailable; and "select row" means either an existing persistent selected-row state or only the current =flashRow= animation, but not both words. Add browser-gate assertions for the cursor/class split and for the clicked row's visible result. (non-blocking) + +Disposition: *accepted*, resolving the "select row" ambiguity toward flash-only. =flashRow= (app.js:560) already does scrollIntoView + a flash animation and there is no persistent selected-row state for these tables; adding one is scope creep, so v1 means "scroll the assignment row into view and flash it (=flashRow=)", and the word "select" is dropped. Defined the affordance split: on-pane spans get =cursor:pointer= plus a =locate-onpane= class the gate checks; off-pane and unassigned spans get the default cursor and no click handler; every element's =title= begins with the owning section so an unavailable click explains itself. Folded into Design (For the user + implementer), with matching browser-gate assertions for the cursor/class split and the flashed-row result added to the Testing strategy and acceptance criteria. + +** DONE Registry identity is under-specified and face-name keys can collide :blocking: +The design still says the registry is keyed by face name, while also spanning UI faces and package faces. That is not a safe implementation contract: =data-owner-app= exists precisely because a face name alone is not enough once package-owned spans, UI-owned spans, and future cross-pane spans share one DOM surface. It also makes missing-owner and bad-owner validation ambiguous. Define the registry key as an owner-qualified identity, e.g. ={surface:"ui", owner:"@ui", face}= and ={surface:"package", owner:<app-key>, face}=, and make =data-owner-app= an internal owner key, not the display label "UI faces". =locateFaceMeta=, =isLocateOnPane=, =previewFaceAttrs=, and =assertPreviewFaces= should all take owner+face, never face alone. Add a test with the same face name under two owners (or a synthetic constructed collision) to prove the registry does not collapse them. (blocking) + +Disposition: *accepted as written* — a real correctness bug in the draft (face-name keys collide across surfaces, and "UI faces" was being used as both display label and owner key). The registry is now keyed by an owner-qualified identity: ={surface, owner, face}=, owner being an internal key (=@ui= for the UI surface, the app-key for a package), distinct from the display label. =data-owner-app= carries that owner key. =locateFaceMeta=, =isLocateOnPane=, =previewFaceAttrs=, and =assertPreviewFaces= all take =(owner, face)=, never face alone. Folded into Design (registry model + implementation shape); a same-face-name-under-two-owners collision case is in the Testing strategy. + +** DONE Cross-surface rendering API is not explicit enough :blocking: +=os(app, face, text)= currently styles through =ofs=, which resolves =PKGMAP[app][face]= and package inherit chains. The spec says cross-pane previews can render UI faces such as =minibuffer-prompt= and =highlight=, but it does not define how =os= styles a UI-owned face from a package preview. Passing "UI faces" or =@ui= to =ofs= would not work because UI faces live in =UIMAP= and use =uiCss= / =resolveUiAttr=, not =PKGMAP=. Define the preview-span API before implementation: either a single =previewSpan(owner, face, text)= that dispatches by owner surface, or separate wrappers such as =os(app, face, text)= for package faces and =uos(face, text)= for UI faces, both sharing the locate attributes. The API should remain backward-compatible for existing package previews so Phase 2 is mechanical and stoppable. (blocking) + +Disposition: *accepted as written* — verified: =ofs= resolves =PKGMAP[app][face]= via =pkgEffFg/pkgEffBg= -> =effResolve=, while UI faces render through =uiCss= + =resolveUiAttr= over =UIMAP=, so =os= cannot style a UI face. The API is now =previewSpan(owner, face, text)= dispatching by owner surface (=@ui= -> the =uiCss=/=UIMAP= path, a package owner -> the existing =ofs=/=PKGMAP= path), emitting the shared locate attributes (=data-owner-app=, =data-face=, the =locate-onpane= class) in both branches. =os(app, face, text)= stays as a thin backward-compatible wrapper that calls =previewSpan= with the package owner, so existing previews are untouched and Phase 2 stays mechanical. Folded into Design (preview tagging + implementation shape) and the phase split. + +** DONE Tooltip source-state semantics still conflate raw and effective values :blocking: +The tooltip contract names assigned, inherited, cleared, and unassigned, but it does not say whether =value= is raw assignment, effective rendered value, or both. Current rendering floors unset foreground/background through =effFg= / =effBg=, package faces can inherit via =effResolve=, UI faces can inherit via =resolveUiAttr= for a small built-in chain, and =source:"cleared"= can still render an effective default color. A tooltip saying "cleared (no foreground)" while the preview visibly has a foreground would be confusing. Specify the exact contract: show effective rendered fg/bg as the value, and separately show source notes such as "direct", "inherited from X", "default foreground", "default background", or "cleared, rendering as default fg/bg". Cover fg-cleared, bg-cleared, inherited package value, inherited UI value, and fully unassigned in pure tests. (blocking) + +Disposition: *accepted* — this supersedes the round-1 tooltip contract (finding 3), which conflated the two. Verified the floors: =effFg/effBg= fall back to =MAP['p']=/=MAP['bg']=, package inherit via =effResolve=, UI inherit via =resolveUiAttr=, and a =cleared= source still renders an effective default. Rewrote the Tooltip contract: =value= is always the *effective rendered* fg/bg (what the eye sees), and a separate *source note* per attribute says "direct", "inherited from X", "default foreground/background", or "cleared, rendering as default" — so a cleared face reads "cleared, rendering as default fg" rather than the misleading "no foreground". Pure tests cover fg-cleared, bg-cleared, inherited-package, inherited-UI, and fully-unassigned. + +** DONE Phase boundaries are not clean enough for a stop-between-phases implementation :blocking: +The phases now include useful pre/post refactors, but Phase 2 still implies changing =os=, every preview span, owner-aware validation, cross-pane styling, and gate behavior in one step. Because =os= is used by every bespoke preview, a half-done Phase 2 can break most package previews or invalidate existing preview gates. Split the phase so each stop leaves a working tree: first add backward-compatible span helpers and owner attributes with package owners defaulting to the current app; then update =assertPreviewFaces= to accept owner-qualified package spans while preserving existing same-app previews; then add UI-owner rendering support; only then wire a showcase off-pane span. Each subphase should name the gate that proves existing previews still pass before the next step. (blocking) + +Disposition: *accepted as written.* Split the old Phase 2 into four stoppable subphases, each naming the gate that proves existing previews still pass: 2a — add =previewSpan= + owner attributes, package owner defaulting to the current app, =os= delegating to it (gate: all existing package-preview gates still pass unchanged); 2b — update =assertPreviewFaces= to accept owner-qualified package spans while same-app previews still pass (gate: existing previews green under the new validator); 2c — add =@ui= rendering support to =previewSpan= (gate: a UI face renders correctly off a package preview); 2d — wire one showcase off-pane span (gate: the off-pane span renders, is hover-only, and passes the owner-aware validator). Reflected in the Implementation phases. + +** DONE Hover-only wayfinding may be too weak with native title alone +The rejected auto-switch alternative had one strong property: it gave an immediate path to the owning pane. V1 rejects the jump, but the spec currently keeps only delayed native =title= text as the substitute, which is weak for dense previews and inaccessible to keyboard-only exploration. Pull the useful part of the rejected scenario without adding auto-navigation: update a small existing status surface (for example the preview label area or a single non-card info strip) on hover/focus with "section > face — value", while keeping =title= as the deterministic browser fallback. This preserves off-pane non-clickability but makes the wayfinding immediate and testable. If v1 deliberately stays native-title-only, record that as an accepted UX caveat and add a vNext task for a hover/focus info strip. (non-blocking) + +Disposition: *modified.* Took the immediate-wayfinding half: v1 updates a single info line in the existing preview-label area on mouse hover (=mouseover=) with "section > face — value", with =title= kept as the deterministic fallback and the gate target. Deferred the *keyboard/focus* half to vNext: the previews aren't keyboard-navigable today, so making spans focusable is a separate lift, not a bolt-on — recorded as an explicit accepted accessibility caveat (v1 wayfinding is pointer-driven) plus a vNext task for a focusable hover/focus info strip. This keeps v1 bounded while still fixing the "delayed title is too weak" complaint for the common (mouse) path. Folded into Design (For the user + a wayfinding note), Scope tiers (vNext), and a browser-gate for the info-line content. + +** DONE Registry lifecycle and performance budget are underspecified :blocking: +The likely cost of a registry over UI faces plus package faces is acceptable, but the spec does not define the cache lifecycle. Current edit paths rebuild tables/previews from many places: =uiSelect= calls =paintUI= + =buildMockFrame=, package controls call =pkgChanged=, import replaces =UIMAP= / =PKGMAP=, reset/clear paths mutate many faces, and palette edits can change effective displayed values without changing face ownership. Without an explicit lifecycle, an implementation may rebuild the registry for every =os()= span, or worse, leave stale =title= and clickability after import/reset/edit/view-switch. Define a single cached registry variable plus an invalidation/rebuild contract: rebuild once per assignment/import/reset batch before rendering preview spans; never rebuild per hover/span; recompute =locate-onpane= from current view at render time; and include a rough budget/gate such as "registry build over all current UI + package faces stays below a small millisecond threshold in a Node test" or at least "linear in face count, no DOM queries." (blocking) + +Disposition: *accepted as written.* Verified the many edit paths (=uiSelect=->=paintUI=/=buildMockFrame=, =pkgChanged=, import replacing =UIMAP=/=PKGMAP=, =resetApp=/=resetUnlocked=). Added a "Registry lifecycle" subsection to Design: one cached module-level registry; rebuilt once per assignment/import/reset/clear batch *before* preview spans render (hooked into the existing rebuild points, not per span and never per hover); =locate-onpane= is *not* stored in the registry — it's recomputed from the current view at render time, since a view switch changes clickability but not ownership; palette edits that change effective values without changing ownership refresh values on the same rebuild. Perf gate: a pure-Node test asserts the build is linear in face count, does no DOM queries, and stays under a small ms threshold over the full UI+package face set. Added to the Testing strategy. + +** DONE Pure-helper boundary and state inputs are still inconsistent :blocking: +The spec says the locate logic lives as pure helpers in =app-core.js=, but the named helper signatures still depend on state they do not accept. =buildLocateRegistry(apps, pkgmap, uimap)= is supposed to store effective rendered values and source notes, yet effective package/UI rendering also depends on default ground colors from =MAP= (=effFg=/=effBg= floor through =MAP['p']= / =MAP['bg']=) and UI/package inheritance helpers that need the current maps. Likewise =previewSpan(owner, face, text)= is listed as a pure helper, but the current render path in =previews.js= uses globals (=PKGMAP=, =MAP=, =faceCss=, =effFg=, =pkgEffFg=, =pkgEffBg=), while UI rendering uses =uiCss= / =resolveUiAttr= over =UIMAP=. Two implementers could both follow this spec and put the stateful string renderer either in =app-core.js= or in the DOM/global layer, with different test seams and cache behavior. Decide the boundary explicitly before implementation: either pass all required state into pure helpers (for example registry/map/uimap/pkgmap/current-owner plus any escaping/style dependencies), or split it into pure =locateAttrs= / =locateMeta= / =formatLocateTitle= helpers in =app-core.js= and a stateful =previewSpan= adapter in =previews.js= / =app.js=. The final spec should name the exact signatures and which layer owns HTML escaping. (blocking) + +Disposition: *accepted*, resolved toward the split (the second option). The spec previously listed =previewSpan= among the app-core.js pure helpers, which was the inconsistency — it can't be pure because it reads =PKGMAP=/=UIMAP=/=MAP= and emits escaped HTML. The boundary is now explicit. Pure helpers in app-core.js, all state passed in, no globals, returning data (never HTML): =buildLocateRegistry(apps, pkgmap, uimap, map)= (=map= added for the =effFg=/=effBg= ground floors), =locateFaceMeta(owner, face, registry)=, =formatLocateTitle(meta)=, =previewFaceAttrs(owner, face, registry)=, =isLocateOnPane(owner, currentApp)=. The one stateful piece, =previewSpan(owner, face, text)=, lives in previews.js as a thin adapter: it reads the current globals, calls the pure helpers plus the existing =faceCss=/=uiCss= style functions, owns HTML escaping (=esc=), and emits the span. =os= delegates to it. Recorded the exact signatures and the escaping owner in the Implementation shape note, Phase 0 (pure helpers only), and Phase 2a (the adapter). + +** DONE Showcase preview is not named precisely enough :blocking: +The spec repeatedly requires "one existing preview wired as the showcase" and Phase 2d says to "wire one showcase off-pane span", but it never names the preview, the exact off-pane owner(s), or the text spans to add. That matters because the natural motivating examples are not both obviously available in v1: the org-agenda app is still planned, while the completion preview appears to be a separate spec. Leaving this open forces the implementer to decide whether to mutate the current =org-mode= preview with cross-owner spans, wait for another feature, add a synthetic browser-only fixture, or build a new showcase preview as part of this work. Choose one concrete showcase path and define the exact spans and owners it must exercise (for example a named existing package preview with one package-owned off-pane span and one =@ui= span, or explicitly allow a gate-only fixture if no user-facing existing preview should change yet). The acceptance criteria and Phase 2d gate should reference that named showcase. (blocking) + +Disposition: *accepted*, choosing the gate-only fixture. Neither natural consumer is available in v1 (org-agenda is unbuilt; the completion preview is a separate spec), and grafting demo cross-owner spans onto a real preview like =org-mode= would be misleading — the org-mode preview has no business showing a UI face. So v1's showcase is a *gate-only synthetic fixture* in browser-gates (no user-facing preview changes): a host package-preview context rendering exactly two off-pane spans — one package-owned (a face from a different package app) and one =@ui= span (=minibuffer-prompt=) — asserting each renders with its owner's real color, is hover-only (no =locate-onpane=, default cursor), and passes the owner-aware =assertPreviewFaces=. The first real cross-owner preview to ship (org-agenda or the completion preview) becomes the organic showcase then. Phase 2d and the acceptance criteria now reference this named fixture. + * Implementation phases +** Phase 0 — Pure-helper extraction (pre-work) +Before any feature behavior, land the pure helpers in app-core.js — buildLocateRegistry(apps, pkgmap, uimap, map), locateFaceMeta, formatLocateTitle, previewFaceAttrs/owner-validation, isLocateOnPane — all state passed in, no globals, returning data not HTML. The stateful previewSpan adapter is NOT here; it lands in Phase 2a. Each helper gets pure-Node unit tests. Leaves the tree working: helpers exist and are tested; no preview behavior has changed yet. + ** Phase 1 — Face registry -Build the derived face -> (owning app, value, attributes) map from the app/face assignment state, rebuilt on change. Unit-tested against a constructed assignment model. +Build the derived face -> (owning app, value, attributes) map over the two data-face surfaces (UIMAP for UI faces, PKGMAP/APPS for package faces), rebuilt on assignment change, via buildLocateRegistry. The syntax/code tier stays out (it has its own data-k -> flashAssign path). Unit-tested against a constructed assignment model. -** Phase 2 — Preview tagging + cross-pane resolution -Extend os() so each previewed element carries its face name and owning app, and resolves its color from the registry regardless of owning pane. Tested on a preview that references an off-pane face. +** Phase 2 — Preview tagging + cross-pane resolution + gates (four stoppable steps) +Each step leaves every existing preview green; the named gate proves it before the next. +- *2a* — add the =previewSpan(owner, face, text)= adapter in previews.js (reads globals, owns =esc=, calls the pure helpers) emitting =data-owner-app= + =data-face= + the =locate-onpane= flag, with package owners defaulting to the current app; =os= delegates to it. Gate: all existing package-preview gates pass unchanged. +- *2b* — update =assertPreviewFaces= to validate each element against its =data-owner-app='s face set (owner-qualified). Gate: existing same-app previews still pass under the new validator; a bad owner is rejected. +- *2c* — add =@ui= rendering support to =previewSpan= (the =uiCss=/=UIMAP= path). Gate: a UI face renders correctly off a package preview. +- *2d* — add the gate-only showcase fixture (browser-gates, no user-facing preview change): a host package-preview context with one package-owned off-pane span and one =@ui= off-pane span (=minibuffer-prompt=). Gate: each renders with its owner's real color, is hover-only (no =locate-onpane=, default cursor), and passes the owner-aware validator. ** Phase 3 — Hover -Tooltip showing section + element + value/attributes for the hovered element, from the registry. Browser-gate tested. +Populate each element's =title= with section + element + value/attributes per the tooltip contract, including the inherited/cleared/unassigned fallbacks. Browser-gate tested against the four source states. ** Phase 4 — Click -Flash + scroll/select the assignment row for a current-pane element; off-pane elements non-clickable (cursor/affordance reflects it). Browser-gate tested; one existing preview wired as the showcase. +On a =locate-onpane= element, scroll its assignment row into view and flash it (=flashRow=) in the owning pane's table; off-pane and unassigned elements have no handler and the default cursor. Browser-gate tested (on-pane flashes the right row, off-pane leaves rows unflashed, cursor/class split); one existing preview wired as the showcase. + +** Phase 5 — Locate-dispatch cleanup (post-feature) +With the gates green, replace the =buildPkgPreview= / =buildMockFrame= face-click branches with a single locate dispatcher that routes by owner. The =data-k= syntax-click path stays separate for v1. Leaves the tree working: behavior identical, one click path instead of two. * Acceptance criteria -- [ ] Hovering any previewed element shows its section, element, and current value/attributes. -- [ ] Clicking a current-pane element flashes it and scrolls/selects its assignment row. +- [ ] Hovering any data-face previewed element shows its section, element, effective value, and source note via =title=, and the preview-label info line updates immediately with "section > face — value". +- [ ] A cleared face shows its rendered default as the value with a "cleared, rendering as default" source note (value matches the pixels); inherited shows "inherited from X"; unassigned shows "unassigned". +- [ ] The registry is keyed by owner-qualified identity ={surface, owner, face}=; the same face name under two owners stays distinct. +- [ ] The registry is a single cached structure rebuilt once per assignment/import/reset batch (never per hover/span); =locate-onpane= is recomputed from the current view; the build meets the linear/no-DOM/ms-threshold budget. +- [ ] Clicking an on-pane element scrolls its assignment row into view and flashes it (=flashRow=); no persistent selection state is introduced. +- [ ] On-pane spans carry =cursor:pointer= + the =locate-onpane= class; off-pane and unassigned spans carry the default cursor and no handler. - [ ] Clicking an off-pane element does nothing and is visibly non-interactive. -- [ ] A preview can render an off-pane face's real color (cross-pane resolution) and that element is hover-only. -- [ ] The registry resolves every previewed face to its owning app, and updates when an assignment changes. +- [ ] The gate-only showcase fixture renders a package-owned off-pane span and a =@ui= (=minibuffer-prompt=) span, each in its owner's real color and hover-only, and the updated =assertPreviewFaces= passes them against their owner app while rejecting a bad owner. No user-facing preview changes in v1. +- [ ] The locate logic splits cleanly: the app-core.js helpers are pure (state passed in, no globals, return data), and only =previewSpan= in previews.js reads globals and emits escaped HTML. +- [ ] The registry resolves every data-face previewed face (UI + package surfaces) to its owning app, and rebuilds when an assignment changes. +- [ ] The deterministic test matrix (below) passes: pure-Node unit cases and the browser gates, with manual inspection limited to the subjective discoverability read. * Readiness dimensions -- Data model & ownership: the registry is derived, not authoritative; the app/face assignment state remains the source of truth. -- Errors, empty states & failure: an element whose face resolves to no owning app falls back to hover-only with an "unassigned" note rather than a dead click. +- Data model & ownership: the registry is derived, not authoritative; the assignment state (UIMAP, PKGMAP/APPS) remains the source of truth. It spans the two data-face surfaces; the syntax/code tier is out of v1 (served by its own data-k path). +- Errors, empty states & failure: an element whose face resolves to no owning app falls back to hover-only with an "unassigned" note rather than a dead click; cleared and inherited sources have their own fallback strings (see the tooltip contract) so no state renders as a blank or misleading value. - Security & privacy: N/A -- browser-local theme editor state. -- Observability: the tooltip is the surface; a face that fails to resolve shows that in the tooltip. +- Observability: the element's =title= is the surface; a face that fails to resolve shows "unassigned" there rather than silently. - Performance & scale: the registry is rebuilt on assignment change, not per hover; hover/click read it in O(1) by face name. - Reuse & lost opportunities: reuses the existing os() preview helper and the assignment state; every current and future preview benefits. -- Architecture fit & weak points: the registry is the new shared structure; the weak point is keeping it in sync with edits, addressed by rebuilding on change. +- Architecture fit & weak points: the registry and the locate logic live as pure helpers in app-core.js (buildLocateRegistry, locateFaceMeta, formatLocateTitle, owner-validation, isLocateOnPane) with DOM as thin adapters; the post-feature cleanup collapses the two face-click branches into one dispatcher. The weak point is registry/edit sync, addressed by rebuilding on change; pulling the logic into tested pure helpers is what keeps the state matrix determinate. - Config surface: none beyond the existing theme-studio build. - Documentation plan: the theme-studio test suite and this spec; a note in the tool's help if the interaction isn't self-evident. -- Dev tooling: existing make theme-studio-test and the browser-gate harness. +- Dev tooling: existing — run-tests.sh drives pure-Node =*.mjs= unit tests (auto-discovered) and the headless-Chrome hash gates; the deterministic matrix (see Testing strategy) lands as new cases in both, no new targets. - Rollout, compatibility & rollback: additive; rollback removes the tagging and the registry. Existing previews keep working without the interaction. - External APIs & deps: none -- browser JS plus the existing generate pipeline. +* Testing strategy +A deterministic matrix, no flaky dependencies. Pure-Node tests (=*.mjs=, auto-discovered by run-tests.sh) cover the logic; browser hash gates cover the DOM contract; manual inspection is limited to the subjective "discoverable enough?" read. + +Pure-Node unit cases (over app-core.js helpers): +- =buildLocateRegistry= construction over a constructed apps/PKGMAP/UIMAP model; rebuild after a simulated assignment edit (stale-registry prevention). +- *owner-qualified identity:* the same face name under two owners (a constructed collision) stays two entries, never collapsed. +- =locateFaceMeta(owner, face)= lookup: UI-owned, package-owned, missing owner, bad owner. +- =formatLocateTitle= for each source state with *effective value*: direct (fg only; fg+bg; non-default structural attributes), inherited-from-X (package via effResolve, UI via resolveUiAttr), fg-cleared and bg-cleared (value shows the rendered default, note says "cleared, rendering as default"), fully unassigned. +- owner-aware face validation =previewFaceAttrs(owner, face)=: an off-pane face validates against its owner; a bad owner is rejected. +- =isLocateOnPane(owner, currentApp)= true/false across UI-owned and package-owned faces under a given current pane. +- *lifecycle/perf:* registry build is linear in face count, does no DOM queries, and stays under the ms threshold over the full UI+package set. + +Browser gates: +- exact =title= strings for representative elements in each source state (effective value + source note). +- the preview-label info line updates on mouse hover with "section > face — value" (the immediate-wayfinding surface), =title= present as fallback. +- =data-owner-app= (owner key) and =locate-onpane= attributes present and correct on on-pane vs off-pane spans. +- a =@ui= face renders correctly when emitted from a package preview (cross-surface =previewSpan=). +- on-pane click flashes the correct assignment row (=flashRow=, no persistent selection); off-pane click leaves all rows unflashed. +- the cursor/class split (pointer + =locate-onpane= on-pane; default cursor, no handler off-pane). +- =assertPreviewFaces= accepts an intentional off-pane span and rejects a bad owner. +- the per-subphase regression gate: every existing package preview still passes after 2a and 2b. +- helper purity: the app-core.js helpers are called with state passed in (no globals) in the Node tests; only =previewSpan= touches globals. +- the gate-only showcase fixture: its package-owned off-pane span and its =@ui= (=minibuffer-prompt=) span each render with the owner's color, are hover-only, and pass the owner-aware validator. + * Risks, Rabbit Holes, and Drawbacks - Registry staleness: a stale registry mislabels a hover. Dodge: rebuild on assignment change; derive, never cache independently. - Tooltip noise on dense previews: hovering everything could be busy. Dodge: tooltip on deliberate hover only, not a persistent overlay. @@ -146,3 +290,62 @@ Flash + scroll/select the assignment row for a current-pane element; off-pane el - What: initial draft. - Why: breaking the org-agenda faces into their own pane surfaced the need to go from a rendered preview element back to its assignment; the interaction (hover-identifies, click-locates-on-pane, off-pane-inert) is general and worth settling before the agenda preview consumes it. - Artifacts: the org-faces app (the bespoke-app pattern); the planned org-agenda app (first consumer); os() preview helper. + +** 2026-06-23 Tue @ 12:26:40 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Assigned =Not ready=. Added three review findings: the registry scope must cover the actual syntax/UI/package preview tiers or narrow v1, the cross-pane =os()= / data-attribute / browser-gate contract must be specified before the org-agenda showcase can use off-pane faces, and tooltip value formatting needs a minimal testable display contract. +- *Why:* The current implementation has separate =data-k= syntax assignments, =UIMAP= UI faces, and =PKGMAP/APPS= package faces, while the draft registry only named app/face assignments. The existing package preview click path and =assertPreviewFaces= gate also assume preview faces belong to the current app, which conflicts with the planned cross-pane org-agenda preview. +- *Artifacts:* =* Review findings= section in this spec; code read: =scripts/theme-studio/previews.js= (=os= / package preview spans), =scripts/theme-studio/app.js= (=flashAssign=, =flashUi=, =flashPkg=, =buildPkgPreview=), =scripts/theme-studio/browser-gates.js= (=assertPreviewFaces=, =#hovertest=). + +** 2026-06-23 Tue @ 12:32:08 -0400 — Claude Code (emacs-d) — responder +- *What changed:* Folded all three findings ([3/3] DONE). Modified finding 1: scoped the v1 registry to the two data-face surfaces (UI mock via UIMAP, package previews via PKGMAP/APPS) and left the syntax/code =data-k= tier out (it already locates via =cp.onclick= -> =flashAssign=); "every previewed element" -> "every data-face previewed element". Accepted finding 2 as written: =os= now takes the owning app and emits =data-owner-app= + =data-face= + a =locate-onpane= flag, click routes by owner, =assertPreviewFaces= validates each element against its owner app. Accepted finding 3 via the reviewer's escape hatch: v1 reuses the =title= attribute with a fixed field contract and four source-state fallbacks (assigned / inherited / cleared / unassigned) — added as a "Tooltip contract (v1)" subsection. +- *Why:* The findings were code-verified and correct. The finding-1 narrowing keeps v1 bounded while still covering both known consumers — the org-agenda showcase (package faces) and the planned minibuffer-completion preview (UI faces minibuffer-prompt + highlight, which is why the UI surface must be in v1). No new unproven obligations introduced; the response only specified existing ambiguity, so the readiness rubric rises rather than regresses. +- *Cross-spec:* enables [[file:theme-studio-completion-preview-spec.org][theme-studio-completion-preview-spec.org]] — its off-entry discoverability (minibuffer-prompt, highlight in UI Faces) depends on the UI surface being in this registry, now guaranteed by the finding-1 scope. +- *Artifacts:* findings [3/3] in this spec; code reads previews.js:6, app.js:565-570/644, browser-gates.js:41-47. + +** 2026-06-23 Tue @ 12:40:48 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Second review, =Not ready= again. Added three findings: the test plan needs an explicit deterministic state matrix (pure-Node + browser gates) rather than one happy-path showcase; the feature needs pre-work pure-helper extraction and a post-feature locate-dispatch cleanup to avoid coupling growth; and the preview affordance / "select row" semantics need exact UX and gate assertions. +- *Why:* The first response resolved scope and tooltip, but the plan still under-specified corner-case proof and would have grown coupling by wiring owner-aware behavior straight into the global/DOM paths. +- *Artifacts:* findings 4-6; code read: run-tests.sh, app-core.js, app.js (buildMockFrame, buildPkgPreview, flashRow), browser-gates.js, styles.css. + +** 2026-06-23 Tue @ 12:40:48 -0400 — Claude Code (emacs-d) — responder +- *What changed:* Folded findings 4-6 ([6/6] DONE). Accepted the deterministic matrix as written — added a "* Testing strategy" section splitting pure-Node unit cases (registry build/rebuild, owner lookup, title formatting incl. four fallbacks, owner-aware validation, isLocateOnPane) from browser gates (exact titles, data-owner-app/locate-onpane attrs, on-pane flash, off-pane unflashed, cursor/class split, gate accepts off-pane + rejects bad owner). Accepted the refactor with bracketing phases — Phase 0 pure-helper extraction in app-core.js before the feature, Phase 5 locate-dispatch cleanup after, recorded as an "Implementation shape" note. Accepted the affordance finding and resolved "select row" -> flashRow only (scroll + flash, no persistent selection), with the on-pane pointer + locate-onpane class vs off-pane default-cursor split defined. +- *Why:* All three were code-verified and match the project's deterministic-test and pure-helper-extraction discipline (app-core.js is already the pure-helper home; flashRow already does scroll + flash with no selection state). The refactor phases keep the tree working at each step; the matrix makes the corner cases provable rather than eyeballed. +- *Cross-spec:* unchanged — the completion preview still rides the UI surface in the registry. +- *Artifacts:* findings [6/6]; code reads app-core.js (pure-helper home), app.js:560 (flashRow), run-tests.sh (*.mjs + hash gates), styles.css (cursor rules). + +** 2026-06-23 Tue @ 12:50:38 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Third review, =Not ready=. Six findings (7-12, five blocking): the registry must be keyed by owner-qualified identity rather than face name (collision risk); the cross-surface render API must be defined because =os= can't style a UI face; the tooltip must show effective value plus a source note instead of conflating raw/effective; Phase 2 must split into stoppable steps since =os= is used by every preview; native-title wayfinding is weak; and the registry needs a defined cache lifecycle + perf budget. +- *Why:* The prior responses settled scope, tooltip fields, test matrix, and refactor home, but the keying/API/lifecycle contracts were still implicit and would have surfaced as correctness or coupling bugs mid-build. +- *Artifacts:* findings 7-12; code read: previews.js (=os=/=ofs=), app.js (=effFg=/=effBg=/=uiCss=/=resolveUiAttr=/=uiSelect=/=pkgChanged=/=resetApp=), app-core.js (=effResolve=), browser-gates.js. + +** 2026-06-23 Tue @ 12:50:38 -0400 — Claude Code (emacs-d) — responder +- *What changed:* Folded findings 7-12 ([12/12] DONE). Five accepted as written, one modified. Registry re-keyed to owner-qualified ={surface, owner, face}= with all lookups taking =(owner, face)= (finding 7). Defined =previewSpan(owner, face, text)= dispatching by surface, with =os= as a backward-compatible package wrapper (finding 8). Rewrote the tooltip contract: =value= is always the effective rendered color, with a per-attribute source note (direct / inherited-from-X / default / cleared-rendering-as-default), superseding the round-1 four-fallback contract (finding 9). Split Phase 2 into 2a-2d, each naming the regression gate that keeps existing previews green (finding 10). Added a "Registry lifecycle" subsection — one cached registry, rebuilt per assignment/import/reset batch, =locate-onpane= recomputed at render, linear/no-DOM/ms-budget gate (finding 12). *Modified* the wayfinding finding: took the immediate mouse-hover info line in the preview-label area (with =title= as fallback) for v1, deferred keyboard-focus traversal to vNext as a recorded accessibility caveat (finding 11). +- *Why:* All six were code-verified. The keying and API findings were genuine correctness gaps (face-name collisions; =os= structurally can't reach =UIMAP=); the tooltip and lifecycle findings prevent the tooltip lying about cleared/inherited values and the registry going stale or rebuilding per span. The phase split keeps =os=, used by every preview, from breaking mid-change. The one modify keeps v1 from growing a keyboard-navigation model while still fixing the delayed-title complaint for the mouse path. +- *Cross-spec:* unchanged — the completion preview still rides the UI surface, now via owner =@ui= in the registry. +- *Artifacts:* findings [12/12]; code reads previews.js:5-6 (=ofs=/=os=), app.js:43-44/572/649/699/752/793, app-core.js:120/155 (=effResolve=/=resolveUiAttr=). + +** 2026-06-23 Tue @ 13:00:43 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Fourth review, =Not ready=. Two implementation-readiness blockers: the pure/stateful boundary is inconsistent (=previewSpan= listed as pure but needs globals; =buildLocateRegistry= needs =MAP= for the ground floors) — name exact signatures and which layer owns HTML escaping; and the showcase is never named (org-agenda unbuilt, completion preview separate) — pick a concrete showcase or allow a gate-only fixture. +- *Why:* Two implementers could split the stateful renderer differently, and the unnamed showcase left Phase 2d's deliverable undefined. +- *Artifacts:* findings 13-14; code read: previews.js (=os=/=ofs= globals), app.js (=effFg=/=effBg=/=MAP=), app-core.js (pure-helper home). + +** 2026-06-23 Tue @ 13:00:43 -0400 — Claude Code (emacs-d) — responder +- *What changed:* Folded findings 13-14 ([14/14] DONE), both accepted with concrete resolutions. Boundary (13): split explicitly — pure helpers in app-core.js take all state as args and return data (=buildLocateRegistry(apps, pkgmap, uimap, map)=, =locateFaceMeta=, =formatLocateTitle=, =previewFaceAttrs=, =isLocateOnPane=); the one stateful piece =previewSpan(owner, face, text)= lives in previews.js, reads globals, owns =esc= HTML escaping, and lands in Phase 2a (removed from the Phase 0 pure set). Showcase (14): chose a gate-only synthetic fixture (one package-owned off-pane span + one =@ui= =minibuffer-prompt= span) with no user-facing preview change — neither natural consumer (org-agenda, completion) exists in v1, and grafting demo spans onto a real preview would mislead; the first real cross-owner preview becomes the organic showcase later. Phase 2d, scope tiers, testing strategy, and acceptance now name the fixture and the boundary. +- *Why:* Both were genuine readiness gaps — the boundary ambiguity would have produced divergent test seams, and the unnamed showcase left a phase deliverable undefined. The gate-only fixture keeps v1 from mutating user-facing previews or blocking on an unbuilt consumer. +- *Cross-spec:* unchanged — completion preview still rides =@ui= in the registry; it (or org-agenda) is the eventual organic showcase. +- *Artifacts:* findings [14/14]; code reads previews.js (=os=/=esc=), app.js:43-44 (=effFg=/=effBg= over =MAP=), app-core.js (pure-helper home). + +** 2026-06-23 Tue @ 12:37:50 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Assigned =Not ready= again. Added three findings focused on Craig's second-review questions: the test strategy needs an explicit deterministic matrix rather than one happy-path browser showcase; the feature needs pre-work pure-helper refactoring and a post-feature locate-dispatch cleanup to avoid coupling growth; and the preview affordance / "select row" semantics need exact UX and gate assertions. +- *Why:* The response resolved the first review's scope and tooltip blockers, but the remaining plan still under-specifies how to prove all corner cases. Current code routes preview ownership through globals, separate click handlers, and a current-app-only gate helper; adding cross-pane behavior without pure helper boundaries would make correctness depend on DOM side effects and manual inspection. +- *Artifacts:* new =* Review findings= entries; code read: =scripts/theme-studio/run-tests.sh= (auto-discovered browser gates), =scripts/theme-studio/app-core.js= (pure helper home), =scripts/theme-studio/app.js= (=buildMockFrame=, =buildPkgPreview=, =flashRow=), =scripts/theme-studio/browser-gates.js= (=assertPreviewFaces=), =scripts/theme-studio/styles.css= (existing preview cursor rules). + +** 2026-06-23 Tue @ 12:47:12 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Third review, =Not ready=. Added six findings after walking the implementation path as if starting the work: owner-qualified registry identity is required; cross-surface preview rendering needs an explicit API for UI-owned faces; tooltip source-state semantics must distinguish raw assignment from effective rendered value; Phase 2 must be split into stoppable substeps; native-title-only hover may not carry enough wayfinding; and the registry cache lifecycle/performance budget must be specified. +- *Why:* The spec looked ready after the prior response, but implementation would still force decisions in code. The current =os= path only knows package faces via =PKGMAP=, UI preview rendering uses =UIMAP= / =uiCss= separately, and the proposed face-name registry would not be robust once owner-qualified cross-pane spans are real. +- *Artifacts:* new =* Review findings= entries; code read: =scripts/theme-studio/previews.js= (=os= / =ofs=), =scripts/theme-studio/app.js= (=buildMockFrame=, =buildPkgPreview=, =uiCss=, =pkgChanged=, =uiSelect=, =importState=), =scripts/theme-studio/app-core.js= (=effResolve=, =resolveUiAttr=, pure-helper pattern), =scripts/theme-studio/face_data.py= and =package-inventory.json= (face-name namespace shape). + +** 2026-06-23 Tue @ 12:57:00 -0400 — Codex (emacs-d) — reviewer +- *What changed or was recommended:* Fourth review, =Not ready=. Added two blocking findings: the pure-helper/refactor boundary is still internally inconsistent because the listed helper signatures omit the state needed to compute effective rendered values and =previewSpan= is stateful despite being listed under pure helpers; and the showcase preview remains unnamed, leaving implementers to choose between mutating an existing preview, waiting for another feature, or creating a synthetic fixture. +- *Why:* The prior response closed the owner identity, API, tooltip, phase, UX, and lifecycle blockers, but these two gaps still affect architecture and phase execution. They would make Phase 0 and Phase 2d diverge by implementer preference rather than by a settled spec decision. +- *Artifacts:* findings 13-14; code read: =scripts/theme-studio/previews.js= (=ofs= / =os= use =PKGMAP= and =MAP= globals), =scripts/theme-studio/app.js= (=effFg= / =effBg= / =uiCss= / render rebuild points), =scripts/theme-studio/app-core.js= (=faceCss=, =effResolve=, =resolveUiAttr= pure-helper pattern). |
