Free-form markdown frame bodies, with a Nebari design system pass - #75
Free-form markdown frame bodies, with a Nebari design system pass#75jbouder wants to merge 1 commit into
Conversation
Replaces the fixed ten-slot frame schema with a single free-form markdown body, and rebuilds the authoring, catalog, and detail surfaces around it. Backend - Frames carry a free-form markdown body instead of ten named slots; the .frame.md codec (frontmatter + body) lives in Go so the editor, import, and export all share one implementation. - Resolution, composition, and validation work on the body; the legacy slot fold-in is kept behind the compatibility path. - Frames can be flagged as templates (migration 005). Web - One markdown body editor with starter templates, plus a raw .frame.md source mode that doubles as the import surface. - Two-column authoring layout; the detail page is the same form, read-only, so reading and editing are one surface. - Table-first catalog with per-row actions, an admin Templates page, and a versions dialog for viewing and restoring past versions. Design system - Replaces the hand-rolled Card, Dialog, Input, Textarea, Select, and the raw tables/labels/checkboxes with their @Nebari registry equivalents, and migrates every call site (Select and Dialog changed API). - Fixes text-destructive, which resolved to a ~1.15:1 background tint and was effectively invisible in both themes; error text now uses text-destructive-foreground. - Extracts PageHeader/SectionHeader so page headings stop drifting (the Connect pages had lost tracking-tight and a 14px description). - Admin section nav becomes real links rather than Base UI Tabs, which emitted role="tab" with no tabpanel and broke cmd-click. - Content panels fill the viewport by flex instead of hardcoded pixel offsets that had gone stale. - Adds 10-per-page paging to the catalog, members, and templates tables. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
📄 Docs preview for |
|
@dcmcand , this pr includes 3 major changes: freeform body, templates, and a major ui overhaul. Let me know if you want me to break it up. |
There was a problem hiding this comment.
Really nice work in the core of this. The legacy read-forever boundary in legacy.go is exemplary: unexported, reachable from exactly one place, with a comment that says why it exists rather than what it does. The .frame.md codec genuinely is single-sourced through the stateless ConvertFrame RPC, the proto changes are purely additive, and the design system pass honors the managed-component rule instead of forking primitives. validate_test.go:33-77 asserting that a re-marshal must not re-emit slots: is the assertion most people would forget.
Three things gate merge. Details are inline; here is the shape of it.
Blockers
- The branch is not rebased on
main, and integrating it breaks the build. Zero git conflicts, two packages fail to compile. See the comment on the deletedslots.go. - Duplicate goose migration version 005, which stops every migration from running and prevents the server from booting. See the comment on the migration.
- A
---line inside a frontmatter block scalar truncates the document, including documents the exporter itself produces. See the comment onframemd.go.
Should fix in the same pass
frame-yaml.ts mirrors legacy.go, diverges from it, and the divergence is written into canonical storage. Not a blocker because the common case renders identically, but it silently rewrites content in one reachable case. Two inline comments cover the divergence and the persistence path.
Three inline items are genuine questions, not change requests
resolver.go:85 (losing per-term and per-prose override), service.go:120 (is_template cleared by a publish that omits the flag), and routes.tsx:35 (removing /admin and /admin/frames). All three are decisions I cannot make from the diff, and in each case "yes, that was intentional" is a complete answer. None of them needs a code change and none should block the merge. The routes.tsx one may deserve a line in the PR description, since the file list is truncated on the PR page and hides the deletions, but that is a courtesy to future readers rather than a fix.
The changes-requested status rests only on the three blockers plus the frame-yaml.ts divergence.
Items with no line to anchor to
AGENTS.mdonmaindocuments the deleted design as current::82(Doc/Slotstypes),:84-86(slots.goas single source of truth for slot keys),:87-88("one##section per slot"),:91("the child's own slots win last"),:143(reflective guards walkingframes.SlotTable). Not your omission, since AGENTS.md postdates the fork, but it becomes false on merge and should be rewritten in the rebase commit.docs/design/2026-05-21-web-app-design.md:168,224still describes per-slot typed editors and slot rendering, and:171-172namesweb/src/lib/slot-sections.ts, which this PR deletes. Same treatment as the MCP design doc (see thecompose.gocomment).- Body size is unbounded and
Resolveappends every ancestor's body, so resolved size grows with chain depth. There is dedup byref@versionfor diamonds, but no content dedup and no size or depth cap, and there is noconnect.WithReadMaxBytesanywhere inbackend/. Pre-existing, so a follow-up issue rather than this PR. The free-form body just makes one unbounded field the whole payload. - Pre-existing, flagged so it is not mistaken for new:
Resolvenever populatesacc.Extends, socompose.go's> Inherits from:line is dead on every production path (resources.go:107and:171are the only callers).compose_test.gocovers it with a hand-builtDocliteral, so the test passes while production cannot reach the branch. Identical at merge-base.
Verification
Verified at PR head 2fb6817 against main at 3e81aa5. The branch alone is green: go build ./..., go vet ./..., and go test ./backend/... all pass across 13 packages, so every failure reported here is integration-induced. main alone is also green, which rules out a pre-existing break. Merge, reverse merge, and rebase were all tested and all three produce the same result. The migration failure was reproduced through a real goose provider. The codec findings were reproduced with in-package probes. The TypeScript/Go comparison was executed on both sides with a real vitest run rather than a hand translation, and the rendering impact was checked through micromark.
Findings I withdrew while verifying, in case they come up: MCP update_frame does not clear template (write.go:75 does d := *base, so the field survives); fine-grained removal of parent guidance never existed, so only per-term and per-prose override is lost; and the migrate_legacy_test.go convention does not apply here, since it covers migrations that repair existing data and a defaulted-column ALTER repairs nothing. dark:prose-invert in MarkdownView.tsx:9 and the --background override in styles.css:234-238 were both dropped as findings, the first as pre-existing and the second as a documented exception in the UI skill.
| @@ -1,119 +0,0 @@ | |||
| package frames | |||
There was a problem hiding this comment.
Blocker: deleting this file breaks main, and git will not tell you.
The branch forked at 3dd1659 and is missing 6630022, 365627e, and 3e81aa5. git merge origin/main produces zero conflicts, so nothing forces a reviewer to look, but the merged tree does not compile:
$ go build -gcflags=-e ./...
backend/internal/mcp/write.go:84:15: d.Slots undefined (type frames.Doc has no field or method Slots)
... (12 errors total, all in internal/mcp)
backend/internal/mcp/write.go:101:26: undefined: frames.Term
$ go vet ./...
backend/internal/frames/service_test.go:622:23: undefined: frames.Slots
Two packages, and the second is invisible to go build because it is a test file. A rebase fails identically to a merge, so no integration strategy surfaces this as a conflict.
write.go is the create_frame / update_frame MCP surface from #73. It declares ten slot-named input fields and no body field, so porting it is design work rather than a mechanical fixup.
One more thing worth knowing before you start: once write.go compiles again, the reflective guard at resources_test.go:403-425 will fail by design. Its expected map has no entry for body or template, so it errors frames.Doc gained field %q: decide whether MCP writes must carry it. That guard is working correctly, and the decision it is asking for is a real one.
| -- Frames flagged as templates are offered as starting points by the authoring | ||
| -- UI's "start from a template" picker. Denormalized from the latest version's | ||
| -- `template` doc field at publish time so listing never parses content blobs. | ||
| ALTER TABLE frames ADD COLUMN is_template INTEGER NOT NULL DEFAULT 0; |
There was a problem hiding this comment.
Blocker: duplicate migration version. main already has 005_canonical_membership_email.sql (added by 365627e), so after a rebase two files claim version 5.
goose v3.27.1 rejects duplicates at provider construction (provider_collect.go:70-77, unconditional; the strict flag gates only unparseable filenames), which means no migration runs at all rather than just this one being skipped:
$ go test ./backend/internal/store/sqlite/...
migrate_test.go:23: run migrations: found duplicate migration version 5:
existing:005_canonical_membership_email.sql
current:005_frame_is_template.sql
cmd/server/main.go:40-43 exits on that error, so the merged server cannot boot. Three test packages fail: store/sqlite, store/sqlite/migrations, and orgs. Renumber to 006_frame_is_template.sql.
Minor, while you are here: this migration has no -- +goose Down section. 001 through 004, and main's 005, all have one.
|
|
||
| // MarshalMarkdown renders a Doc as a spec-conformant .frame.md document. | ||
| // MarshalMarkdown renders a Doc as a spec-conformant .frame.md document. The | ||
| // body passes through verbatim, so export followed by import is lossless. |
There was a problem hiding this comment.
Blocker: this comment is not true, and the reason is a data-loss bug.
framemd.go:161 matches any line whose TrimSpace is --- as the frontmatter terminator. YAML block scalars are indented, so an indented --- inside a multi-line frontmatter value wins as the closing delimiter. The codec does this to its own output:
Doc{Description: "Line one\n---\nLine two"} -> MarshalMarkdown -> UnmarshalMarkdown
reparse err = <nil>
Desc = "Line one" Ver = "" Vis = ""
Validate -> "version: must not be empty"
Export followed by import destroys a document the product itself wrote, and the error the author sees names the wrong field. Reorder the frontmatter so the block scalar sits after the required fields and it goes fully silent: I got Validate -> nil with maintainer swallowed into the body.
It is reachable: description is a multi-line <Textarea rows={3}> (DocMetadataHeader.tsx:74-80) bounded only at 280 runes, and MarkdownSourceEditor.tsx:78-86 is an unfiltered import surface.
This is a behavior regression rather than a brand-new bug. The terminator scan itself is unchanged, but at merge-base the strict body parser rejected the fallout (markdown: line 9: content before the first section heading). Removing the structured body removed an accidental backstop.
Fix is one comparison: lines[j] == "---", which an indented block scalar can never produce. Worth a table-driven case.
Separately, the "lossless" claims here and at :14-15 are unqualified and two other paths contradict them: :103-105 turns an empty visibility into internal on a round trip, and :123 / :202 drop leading and trailing blank lines from the body. Worth softening the comment to name what is actually preserved, since an unqualified "lossless" is what stops someone from going looking for the bug above.
| ]; | ||
| for (const [heading, items] of lists) { | ||
| if (items && items.length > 0) { | ||
| sections.push(`## ${heading}\n\n${items.map((i) => `- ${i}`).join("\n")}`); |
There was a problem hiding this comment.
Should fix: this diverges from legacy.go, and the divergence reaches storage.
The comment at :53 states the invariant ("matching legacy.go renderMarkdown"), but Go's writeLegacyBullet indents continuation lines two spaces (legacy.go:84) while this emits - ${i} with no continuation handling. Executed on both sides with identical input:
Go: "- Never leak PII.\n Redact before logging."
TS: "- Never leak PII.\nRedact before logging."
There is a third divergence in the prose branch below: .trim() here versus strings.Trim(body, "\n") in Go, so Go keeps whitespace padding that this strips.
Not filed as a blocker because the common case renders identically. CommonMark lazy continuation absorbs the un-indented line, so the harm is digest drift. It becomes a real content rewrite when a continuation line is itself block-level markup, which is reachable because ListEditor.tsx:19 used a <Textarea> per item:
Go -> 2 <li>, one nested TS -> 4 flat <li>
Two rules become four and the nesting is gone, permanently, for that frame.
Suggested fix: mirror the two-space continuation, use replace(/^\n+|\n+$/g, "") instead of .trim(), and pin both sides to one shared fixture containing a multi-line item. The fixture is the part that makes the "keep in sync" comment enforceable rather than aspirational. Right now neither side would catch this: both tests use single-line values and substring assertions.
One correction to scope: multi-line terminology definitions cannot come from the web form, since TerminologyEditor.tsx uses <Input>. They arrive only via .frame.md import or the CLI. Multi-line rules, skills, and prompts can come straight from the form.
| const restore = () => { | ||
| setRestoreError(null); | ||
| const content = new TextEncoder().encode( | ||
| serializeFrameDoc({ ...doc, version: nextVersion }), |
There was a problem hiding this comment.
This is the path that makes the frame-yaml.ts divergence matter: the TypeScript render becomes canonical stored content.
:97 parses a stored legacy version in TypeScript, this line re-serializes it, and publish stores the client's bytes verbatim (service.go sets Content: req.Msg.Content, with no server-side re-render or canonicalization). Nothing normalizes in between: form-model.ts passes body through untouched, and serializeFrameDoc only tests doc.body.trim() !== "" before emitting the raw string.
Restore is gated only on !isLatest && canEdit (:159), with no legacy-versus-body: check, so it is reachable on exactly the versions that still carry slots:. The edit path at FrameAuthoringPage.tsx:87 does the same conversion.
Either drive this through the server so no YAML is ever authored in TypeScript, or keep the mirror and pin it with the shared fixture.
| if _, err := tx.ExecContext(ctx, | ||
| `UPDATE frames SET description=?, latest_version=?, updated_at=? WHERE id=?`, | ||
| f.Description, f.LatestVersion, now, f.Id); err != nil { | ||
| `UPDATE frames SET description=?, latest_version=?, updated_at=?, is_template=? WHERE id=?`, |
There was a problem hiding this comment.
is_template has no sqlite-level test coverage. service_test.go uses store.NewMemory(), which round-trips the whole proto and exercises none of this SQL, and sqlite_test.go is untouched by the PR.
That leaves the INSERT at :224, this UPDATE, and the four Scan(..., &f.IsTemplate) sites uncovered. This change adds a column to three separate SELECT lists, which is exactly the shape where a scan-order mismatch slips through a green suite.
| <Route path="/admin/members" element={<AdminMembersPage />} /> | ||
| <Route path="/admin/frames" element={<AdminFramesPage />} /> | ||
| <Route path="/admin" element={<AdminLayout />}> | ||
| <Route index element={<Navigate to="/admin/members" replace />} /> |
There was a problem hiding this comment.
Question: was removing the /admin landing page and /admin/frames intentional? AdminHomePage.tsx, AdminFramesPage.tsx, and AdminFramesPage.test.tsx are all deleted.
No capability appears to be lost, which is why this is a question and not a finding: the delete affordance moved to the permission-gated catalog table (FramesTable.tsx:78), and /admin no longer dead-ends. But the PR body announces the new Templates page without mentioning the removals, and GitHub truncates the file list at 100 of 118 files, so the deletions are invisible to anyone reading the PR page instead of the diff. Worth a line in the description.
This is a genuine question, not a change request. If the answer is "yes, intentional", no action is needed and nothing here should block the merge. It is in the review only because it is a decision I cannot make for you from the diff.
| } | ||
| fmt.Fprintf(&b, "## %s\n\n%s\n\n", d.Heading, strings.Trim(body, "\n")) | ||
| } | ||
| if body := strings.Trim(doc.Body, "\n"); body != "" { |
There was a problem hiding this comment.
Documentation drift: docs/design/2026-05-21-mcp-endpoint-design.md section 3.4 is now false, and the comment at :11 of this file points readers straight at it.
That section still says the server "composes resolved Frame slots into markdown" and shows a canned output format with ## Terminology, ## Rules, ## Skills, ## Prompts, and ## Tool Specifications, plus "Empty slots are omitted from the rendered markdown". This function passes the body through verbatim.
It is not defensible as a frozen historical record: all three design docs carry Status: Draft - pending review, and #73 set the right precedent on this exact file by superseding section 6 with a struck-through block and a Superseded by [#51] note. Same treatment for 3.4, or drop the citation from the comment.
Unrelated nit two lines up in the same file: :28 does e.Ref + "@" + e.Version with no empty-version guard, where MarshalMarkdown does guard (framemd.go:107-110). Unreachable today for the reason in the summary, but inconsistent with its sibling.
| return nil, fmt.Errorf("parse frame yaml: %w", err) | ||
| } | ||
| return &d, nil | ||
| if d.Slots != nil && d.Body == "" { |
There was a problem hiding this comment.
This precedence rule (legacy slots: loses to body:) is almost certainly the behavior you want, but it is silent and unpinned. Executed at head with both keys present, the legacy content is discarded with no error or warning. The only legacy test, TestParse_LegacySlotsFoldIntoBody, supplies slots: with no body: key, so nothing covers the collision. One table entry would fix that.
Also user-visible from Parse, and probably unintended: unknown-key errors now name the unexported decode shape. At head, field bogus not found in type frames.docYAML; at merge-base, frames.Doc. It reaches API callers unwrapped through service.go:80-83 and :523-525, and docYAML means nothing to a client.
| > | ||
| <ArrowLeft /> | ||
| </Button> | ||
| <h1 className="text-2xl font-semibold">{frame.name}</h1> |
There was a problem hiding this comment.
Nit: this is the one page-level heading that does not go through the new PageHeader, and it is missing tracking-tight and text-foreground - the drift PageHeader's own doc comment was written to stop.
PageHeader is not drop-in here, since its layout stacks the description under the title and this header has an inline badge row, so this may just be the two classes rather than a refactor.
2 participants
Summary
Replaces the fixed ten-slot frame schema with a single free-form markdown body, and rebuilds the authoring, catalog, and detail surfaces around it. Also includes a design system pass that moves the web app onto the
@nebariregistry components.Backend
.frame.mdcodec (YAML frontmatter + body) lives in Go, so the editor, import, and export all share one implementation rather than mirroring it in TypeScript.005_frame_is_template.sql).Web
.frame.mdsource mode that doubles as the import surface.Design system pass
Moved off hand-rolled primitives onto the
@nebariregistry, migrating every call site:divCardCard/CardHeader/CardTitle/CardDescription/CardActionbg-black/50, no focus trap)Dialog+DialogHeader/Footer/Description/Close<input>/<textarea>Input/Textareawith invalid-state cues<select>Selectcomposite<table>blocksTable(adds a keyboard-accessible scroll container)<label>,accent-primarycheckboxesLabelwith realhtmlFor,Checkboxwith built-in label + descriptionSelectandDialogchanged API, so consumers were reworked — the visibility field now runs through a react-hook-formControllersince Base UI's select isn't a native control.Fixes found along the way
text-destructiveappeared in 13 places, but--destructiveis a background tint (oklch 95%) — as text on--backgroundthat's ~1.15:1 contrast, effectively invisible in both themes. All now use--destructive-foreground.PageHeader/SectionHeader. The Connect pages had losttracking-tightand their description inherited 16px instead of 14px.<Outlet/>), but Base UITabsemitsrole="tab"insiderole="tablist"with notabpanel, and button-tabs can't be cmd-clicked. Now real links styled from the design system's own exportedtabsListVariants/tabsTabVariants.<TabsIndicator/>was also dead markup — under thepillvariant its class is literallyhidden.h-fullis deliberately not used, since a percentage height doesn't resolve against a parent sized byflex-grow.Test plan
go build ./...clean;go test ./backend/...— 13 packages pass.vitest— 157 tests pass (41 files), including new coverage for pagination slicing, partial last page, disabled bounds, clamping when a list shrinks under an open page, and the empty list.tsc --noEmitclean;eslint0 errors;vite buildsucceeds.Screenshots
🤖 Generated with Claude Code