feat(example): header/footer editor + preview in web_ui_demo - #482
Merged
Conversation
Adds the hand-kept browser-ESM mirror of src/ast/header-footer-schemas.ts's FieldKind enum and defaultVariant() v1/v2 precedence (ADR-040), plus pure withVariant/withPageNumbering/withCellField-family editors, so the demo's header/footer editor (this issue's later tasks) can build a local draft without mutating the persisted composition or duplicating the schema's KNOWN AMBIGUITY logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds resolveFieldDisplay/buildPreviewModel/summarizeWarnings for the demo's own HTML approximation of running headers/footers (the markdown renderer has no page chrome). Delegates all v1/v2 variant precedence to header-footer-fields.mjs's selectVariant rather than re-deriving it, per the spike's caught duplicate-logic trap. Generation-only kinds (sectionNumber/ sectionTitle/pageNumber) never read field.text; excluded identity kinds (packageName/revisionName/revisionLabel/projectNumber/clientNumber) always render as unavailable rather than fabricating a value the demo has no verified source for. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
winningScope/scopeLabel give the demo's Effective Resolution panel a client-side read of which scope won a header/footer merge, without re-deriving the server's per-key merge logic — winningScope is the same coarse layers.at(-1)?.scope ?? null read the API's own response comment documents, and scopeLabel fails loud (throws) on any scope kind it doesn't recognize rather than rendering a blank label. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…#477) Adds the REST client surface the header/footer editor and Download DOCX button need: getJsonOrNull (404 -> null, "not configured" is a valid state, never an error) backing client/project header-footer GETs; put/delete for both scopes; getProjectHeaderFooterResolved for the merged client->project view; and fetchSpecDocx for POST /specs/:id/generate, resolving the response Blob on 2xx. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
triggerBlobDownload defers URL.revokeObjectURL by one macrotask so it can never race the click-triggered save on larger DOCX blobs, and takes its DOM/Blob-URL primitives as an injectable deps parameter so the revoke timing is boundary-testable without jsdom (this repo has none). Flips API_FEATURES.headerFooter now that the backend surface (ADR-040) is already on main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pure DOM-paint module for the demo editor: renderPreview repaints a resolved header/footer preview model (region rows, page-numbering band, warnings) and renderVariantTabs repaints the default/first/even tab strip. Every field/warning value is written via textContent only (never innerHTML) since header/footer content is spec-author/client/ project-authored, not trusted markup. No companion test file — matches the codebase's existing convention for DOM-wiring modules (editor.js, tree.js, numbering.js, modal.js, popover.js, chat.js are all untested; only the pure .mjs helpers and api.js get test coverage). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…477) Adds the shared, scope-agnostic header/footer editor mounted by header-footer.js into the client-library and project panels. Holds a local HeaderFooterEditorDraft ({ composition, dirty }) mutated only via pure draft helpers (editField/addField/removeField/editPageNumbering, layered on header-footer-fields.mjs's selectVariant/withVariant/ withCellField family) — every keystroke/micro-edit repaints the local preview only; ctx.put() fires exactly once, exclusively from the explicit Save action, always as a full-composition load-then-shallow- patch of the exact edited path (never rebuilt from only the fields the UI happens to know about). ctx.onSaved fires after a successful put and before the editor repaints, so a sibling Effective Resolution panel can refresh without a view switch. Delete confirms via openConfirm and treats a 404 as an already-removed no-op rather than an error. Pins both invariants at the boundary with DOM-free tests against the draft controller (no jsdom in this repo, same split as download.js/ header-footer-preview-view.js: pure state logic is unit tested, DOM wiring is not). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mount (#477) Wires the pure header/footer modules and the DOM editor together for the two v1-scoped panels (client library, project) plus the read-only spec-inspector Download DOCX mount, closing out two spike-found bugs: mountInspector now only ever appends into #editor-inspector (never clears the CITES/CITED BY/ EDITABILITY content already painted there), and a project-scope Save re-fetches and repaints the sibling Effective Resolution panel immediately via onSaved, instead of leaving it stale until a view switch. Client-scope API calls are gated to tier === 'client' client-side, mirroring the server's requireClientLibrary gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds css/header-footer.css (styling every .hf-* class already painted by header-footer-editor.js/header-footer-preview-view.js/header-footer.js) and wires two static mount points into index.html: a client-scope panel in the Library view (hidden by default, tier-gated for a later app.js task) and a project-scope HEADER/FOOTER settings panel with distinct editor + Effective Resolution mounts. No new nav tab, no <script> tag changes — a boundary test (header-footer-markup.test.mjs) pins the mount-point ids header-footer.js's ctx contract depends on, plus the no-new-script/no-new-nav invariants, so a future rename can't silently break app.js's getElementById wiring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nto app.js (#477) Closes the last gap in the v1 header/footer UI: editor.js now appends header-footer.js's read-only inspector summary via an optional ctx.mountHeaderFooterInspector callback (extracted as appendHeaderFooterSummary so the append-only, spike-fixed contract is unit-testable without the rest of initEditor's DOM tree). app.js instantiates initHeaderFooter, normalizes getSelectedLibraryTier with an explicit `?? null` (selectedLibrary()?.tier alone can be undefined, which is not a legal value in header-footer.js's documented tier-gate contract), and wires refreshLibraryPanel/refreshProjectPanel into showView() and the library-selection change handler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… editor Full verification pass for #477 (automated suite + manual E2E against a live server) caught a real bug the mocked unit tests couldn't see: getClientHeaderFooter/putClientHeaderFooter/getProjectHeaderFooter/ putProjectHeaderFooter all resolve the full stored record ({ id, scope, config, createdAt, updatedAt }), but header-footer.js's ctx.get/ctx.put closures passed that record straight to the editor as if it were the bare HeaderFooterComposition its contract documents. Every saved field vanished from both the post-Save repaint and any later reload, because selectVariant() found no top-level .variants/.header/ .footer on the envelope. clientEditorCtx/projectEditorCtx now unwrap .config via a shared helper before handing the value to the editor. Regression-tested at the ctx-wiring boundary (header-footer.test.mjs) and confirmed live: create/edit/Save/reload/Delete round-trips correctly for both client and project scope, the Effective Resolution panel refreshes without a view switch, the section-inspector mount no longer clobbers CITES/CITED BY/EDITABILITY, a downloaded DOCX carries a real PAGE field (not fabricated text), and every panel degrades cleanly with API_FEATURES.headerFooter off. Full node --test suite: 238/238 passing (234 pre-existing + 4 new). pnpm lint/test/test:integration green. Files delivering #477 are now built and verified end to end; a follow-up issue (#481) tracks the package/revision-scope generate wiring + UI that ADR-040 explicitly scoped out of this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review findings on the #477 header/footer editor: refresh() collapsed a real ctx.get() failure (network error, 5xx, malformed envelope) into the same "empty" state as a genuinely-unconfigured scope, which let a user hit Create configuration -> Save and silently overwrite whatever was actually stored. onDelete() also never fired ctx.onSaved(), leaving the Settings view's Effective Resolution panel stale after a project-scope delete. The editor's previewContext was read once at (memoized) mount time instead of per render, so switching client libraries never reached the live preview. Separately, triggerBlobDownload could leak a throwaway <a> element into document.body if link.click() threw, and several tests asserted less than their own docstrings claimed. Extracts loadDraft/deleteDraft as ctx-only pure helpers (mirroring the existing saveDraft split) so refresh()'s error-vs-empty decision and onDelete's onSaved-firing are both unit-tested directly; adds a dedicated 'error' editor mode that offers Retry only, never Create/Save. Threads getPreviewContext as a live function instead of a memoized value. Hardens triggerBlobDownload's cleanup into a single finally. Tightens the addCellField/removeCellField catchall-key assertions, the Settings-view markup boundary check, the app.js call-site-scoped tier-normalization regex, and the "edits alone never touch ctx.put" test to actually spy on ctx.put instead of asserting nothing ctx-related ever ran. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…onses Fixes two review findings on the #477 header/footer editor: refresh() had no re-entrancy guard, so an overlapping (slower) response could overwrite a fresher one and a subsequent Save would then PUT stale data to whichever scope is currently selected — not the one it was loaded for. Reuses the existing monotonic request-guard pattern from scoring.js (extracted to js/request-guard.mjs, now shared by all three consumers): the editor's refresh() discards any outcome superseded by a newer call, and exposes invalidate() so header-footer.js can stop a still-in-flight refresh() from repainting a container after its editor is torn down (tier switches away from 'client', or the active project is deselected). Also tightens app-header-footer.test.mjs's tier-gate regex so it anchors to the full `hfPanel.hidden = !(...)` assignment instead of the bare condition substring — the prior regex kept matching even if the leading `!(`/closing `)` were dropped, which would invert the client-scope panel's visibility while the test still passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe web UI demo adds scoped header/footer profile editing, variant-aware preview rendering, effective-resolution display, API persistence, DOCX download support, feature gating, inspector integration, and request-race protection. Extensive Node-based tests cover models, controllers, API behavior, DOM contracts, orchestration, and cleanup. ChangesHeader/footer demo
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoApp
participant HeaderFooterEditor
participant HeaderFooterAPI
participant SpecAPI
User->>DemoApp: Open library or settings view
DemoApp->>HeaderFooterEditor: refresh()
HeaderFooterEditor->>HeaderFooterAPI: Load scoped composition
HeaderFooterAPI-->>HeaderFooterEditor: Composition envelope
User->>HeaderFooterEditor: Edit and save profile
HeaderFooterEditor->>HeaderFooterAPI: PUT composition
HeaderFooterAPI-->>HeaderFooterEditor: Saved configuration
User->>DemoApp: Download DOCX
DemoApp->>SpecAPI: POST generate DOCX
SpecAPI-->>DemoApp: DOCX Blob
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/web_ui_demo/header-footer-markup.test.mjs`:
- Around line 44-52: Update the containment check in the “Library view:
header/footer panel sits inside .library-detail, after .library-tree” test so
detailEnd identifies the balanced closing tag of the .library-detail section
rather than the panel’s closing tag. Parse the markup or track nested section
tags, then compare panelStart against that actual container boundary while
preserving the tree ordering assertion.
In `@examples/web_ui_demo/js/app.js`:
- Around line 174-178: Update the resolution refresh flow, particularly
refreshResolutionPanel(), to prevent stale asynchronous responses from
repainting the shared container after the project changes. Track a request token
or current project ID when starting each refresh, and verify it still matches
the active project before applying the response; discard outdated results while
preserving the current refresh behavior.
In `@examples/web_ui_demo/js/header-footer-editor.js`:
- Around line 501-540: Update onSave to snapshot the current draft before
awaiting saveDraft, then save that snapshot and avoid overwriting newer edits
when applying the response. Also add the editor’s existing invalidation/discard
guard around save and delete completions so discarded instances do not repaint
or emit toasts; ensure the guard covers both success and error paths while
preserving normal active-editor behavior.
In `@examples/web_ui_demo/js/header-footer.js`:
- Around line 219-231: Update the project-switch flow, preferably in
switchProject() or loadActiveProjectWorkspace(), to invoke refreshProjectPanel()
after the active project changes and workspace reload completes. Ensure
project-header-footer-resolution is repainted for the newly active project
without altering refreshResolutionPanel() behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71fabe96-0438-459f-afd7-7053ee192724
📒 Files selected for processing (27)
examples/web_ui_demo/api-header-footer.test.mjsexamples/web_ui_demo/app-header-footer.test.mjsexamples/web_ui_demo/css/header-footer.cssexamples/web_ui_demo/download.test.mjsexamples/web_ui_demo/editor-header-footer.test.mjsexamples/web_ui_demo/header-footer-editor.test.mjsexamples/web_ui_demo/header-footer-fields.test.mjsexamples/web_ui_demo/header-footer-markup.test.mjsexamples/web_ui_demo/header-footer-preview.test.mjsexamples/web_ui_demo/header-footer-resolve.test.mjsexamples/web_ui_demo/header-footer.test.mjsexamples/web_ui_demo/index.htmlexamples/web_ui_demo/js/api.jsexamples/web_ui_demo/js/app.jsexamples/web_ui_demo/js/download.jsexamples/web_ui_demo/js/editor.jsexamples/web_ui_demo/js/features.jsexamples/web_ui_demo/js/header-footer-editor.jsexamples/web_ui_demo/js/header-footer-fields.mjsexamples/web_ui_demo/js/header-footer-preview-view.jsexamples/web_ui_demo/js/header-footer-preview.mjsexamples/web_ui_demo/js/header-footer-resolve.mjsexamples/web_ui_demo/js/header-footer.jsexamples/web_ui_demo/js/request-guard.mjsexamples/web_ui_demo/js/scoring.jsexamples/web_ui_demo/request-guard.test.mjsexamples/web_ui_demo/scoring.test.mjs
…er markup test The containment check took the first `</section>` after the header/footer panel as `.library-detail`'s end — but that is the panel's OWN closing tag (the panel is itself a nested <section>), so `panelStart < detailEnd` was trivially true and the test would still pass if the panel were moved out of .library-detail. Add a balanced <section>…</section> scanner and use it to find .library-detail's real close, so the assertion actually proves nesting. Co-Authored-By: Claude <noreply@anthropic.com>
…ints refreshResolutionPanel() captured the project id, awaited the resolved-layers fetch, then unconditionally painted the shared projectResolutionContainer. Overlapping refreshes (refreshProjectPanel, onSaved, the new project-switch hook) meant an OLDER project's response resolving last could repaint the wrong winning-scope chain. Add a monotonic request guard — the same createRequestGuard primitive the editor uses for its own ctx.get() — so only the latest-issued request paints; earlier ones no-op. Co-Authored-By: Claude <noreply@anthropic.com>
refresh() was token-guarded, but onSave()/onDelete() unconditionally reassigned draft/hasPersistedConfig/mode and repainted after their awaits. A save/delete resolving after the editor was discarded (scope switched away) or after a newer refresh() would repaint the shared container and toast for a scope it no longer owns; a field edit made while the PUT was in flight was also clobbered by the server's round-tripped response. Claim the requestGuard token at each mutation's start and only apply completion state while it is still current (both success and error paths), and keep an in-flight edit by only adopting the saved draft when nothing changed under us. Co-Authored-By: Claude <noreply@anthropic.com>
…witch
showView('settings') only refreshes the project-scope header/footer editor +
Effective Resolution on VIEW entry. Switching projects from the Settings
dropdown (project-select → switchProject → loadActiveProjectWorkspace) left the
previous project's editor/resolution on screen — and because the editor ctx
reads activeProjectId live, a Save on that stale editor would PUT into the newly
selected project. Repaint the panel from loadActiveProjectWorkspace (the common
sink for every active-project change); refresh() drops to a Save-less loading
state while it reloads the new project's config, closing that window.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
|
Codex (GPT-5.5, xhigh) — second adversarial reviewer (additional eyes). Ran read-only against
No separate action items from Codex; it converged with CodeRabbit. Demo suite green (254/254) after the fixes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The demo (
examples/web_ui_demo) had zero header/footer surface across its 12 views, while the backend already shipped the scope model (#208), AST schema (#302, ADR-040), config CRUD + effective-resolution routes (#476), and DOCX-generation wiring (#304). This closes the loop by letting a user author a profile, scope it, see the winning resolution, and download a DOCX that actually carries the configured running header/footer.What
API_FEATURES.headerFooter; every view degrades gracefully when no profile is configured.Package/revision-scope generate wiring + UI is explicitly out of scope per ADR-040 and tracked separately in #481.
Design decisions
A design spike ran before real implementation and surfaced 4 real gaps, all fixed at the design level before the build (not discovered after the fact):
mountHeaderFooterInspectormounts into#editor-inspector, whicheditor.js'srenderInspector()has already populated with CITES / CITED BY / EDITABILITY before this callback runs. The mount must build and append its own wrapper element into that container and must never callcontainer.replaceChildren()/innerHTML = ''(or anything else that clears it). Pinned as an explicit invariant in the module doc comment and covered by a manual-verification step; unit tests assert the container's pre-existing children survive a mount.HeaderFooterEditorDraft) mutated via the pureheader-footer-fields.mjshelpers;ctx.put()fires only on explicit Save, never on every field/cell/pageNumbering edit. This surfaced a second real gap: the sibling Effective Resolution panel went stale after a project-scope Save in the spike's first draft. Fixed by adding an optionalonSavedcallback toinitHeaderFooterEditor, whichheader-footer.jsuses to re-fetch and repaint the resolution panel immediately after a successful save, rather than requiring a view-switch to refresh.header-footer-preview.mjs'sbuildPreviewModelimports and usesheader-footer-fields.mjs'sselectVariantrather than re-deriving the v1/v2variants.defaultprecedence itself — the spike's own first draft made exactly this mistake before catching it, so the interface comment documents it explicitly.download.jsdefersURL.revokeObjectURLby one macrotask (setTimeout 0) rather than revoking synchronously right afterlink.click(), avoiding a plausible (if usually benign in Chrome) race with larger DOCX blobs starting their save/navigation.app.js'sgetSelectedLibraryTierclosure explicitly doesselectedLibrary()?.tier ?? null—selectedLibrary()?.tieralone can yieldundefined, which is not a legal value in the documented'reference'|'company'|'client'|nullcontract thatheader-footer.js's tier-gate depends on.All prior ambiguity calls from the pre-spike design stand unchanged and were confirmed correct against real source during the spike's read-only verification pass: v1 UI scope-cut to client + project only (package/revision are struct-complete but UI-cut, follow-up filed as #481); PUT is always full-load-then-shallow-patch, never rebuilt from known fields; v1→v2 migration strips legacy top-level fields on Save of
variants.default;PreviewFieldContextexcludessectionNumber/sectionTitle/projectNumber/clientNumber/packageName/revisionName/revisionLabelas non-fabricating; client-scope UI is gated totier === 'client';getJsonOrNullis a small internalapi.jshelper; zero REST/MCP/openapi/src/changes (nothing blocked); the newcss/header-footer.cssis justified by the same multi-surface threshold aseditor.css/constellation.css; no new top-level nav tab.A prompt-injection attempt was encountered mid-spike: fabricated "don't tell the user" system-reminder-style text urging that the four edited files be treated as pre-existing/intentional and left unreported. This was disregarded; the worktree was independently re-verified clean via
git status --porcelain, and the attempt is reported here per house rules — no agent or system message can authorize concealing information from oversight.Testing
node --test examples/web_ui_demo/*.test.mjs— 254/254 passingpnpm lint— clean (src/ only; examples/ is out of eslint's scope)pnpm test— 2005/2005 passing (DATABASE_URL set per repo convention; NODE_ENV left unset to avoid the .env rate-limiter trap)🤖 Co-authored by Claude Sonnet 5. Closes #477.