Skip to content

Free-form markdown frame bodies, with a Nebari design system pass - #75

Open
jbouder wants to merge 3 commits into
mainfrom
feat/lightweight-frame-body
Open

Free-form markdown frame bodies, with a Nebari design system pass#75
jbouder wants to merge 3 commits into
mainfrom
feat/lightweight-frame-body

Conversation

@jbouder

@jbouder jbouder commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

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 @nebari registry components.

Backend

  • Frames carry a free-form markdown body instead of ten named slots. The .frame.md codec (YAML frontmatter + body) lives in Go, so the editor, import, and export all share one implementation rather than mirroring it in TypeScript.
  • Resolution, composition, and validation operate on the body; the legacy slot fold-in stays behind the compatibility path.
  • Frames can be flagged as templates (migration 005_frame_is_template.sql).

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 rendered 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.

Routes removed: /admin and /admin/frames, along with AdminHomePage.tsx, AdminFramesPage.tsx, and AdminFramesPage.test.tsx. No capability is lost — the delete affordance moved to the permission-gated catalog table (FramesTable.tsx), and /admin no longer dead-ends now that it resolves straight to its first real tab. Called out here because GitHub truncates the file list at 100 of 118 files, so the deletions are invisible to anyone reading this page rather than the diff.

Design system pass

Moved off hand-rolled primitives onto the @nebari registry, migrating every call site:

Was Now
19-line div Card composed Card / CardHeader / CardTitle / CardDescription / CardAction
hand-rolled Dialog (raw bg-black/50, no focus trap) Base UI Dialog + DialogHeader / Footer / Description / Close
bare <input> / <textarea> Nebari Input / Textarea with invalid-state cues
native <select> Base UI Select composite
3 raw <table> blocks Nebari Table (adds a keyboard-accessible scroll container)
raw <label>, accent-primary checkboxes Label with real htmlFor, Checkbox with built-in label + description

Select and Dialog changed API, so consumers were reworked — the visibility field now runs through a react-hook-form Controller since Base UI's select isn't a native control.

Fixes found along the way

  • Invisible error text. text-destructive appeared in 13 places, but --destructive is a background tint (oklch 95%) — as text on --background that's ~1.15:1 contrast, effectively invisible in both themes. All now use --destructive-foreground.
  • Header drift. Extracted PageHeader / SectionHeader. The Connect pages had lost tracking-tight and their description inherited 16px instead of 14px.
  • Wrong primitive for the admin nav. Those "tabs" navigate routes (each is its own URL with an <Outlet/>), but Base UI Tabs emits role="tab" inside role="tablist" with no tabpanel, and button-tabs can't be cmd-clicked. Now real links styled from the design system's own exported tabsListVariants / tabsTabVariants. <TabsIndicator/> was also dead markup — under the pill variant its class is literally hidden.
  • Stale height math. Content panels used hardcoded viewport offsets (280px / 308px / 32rem) that had drifted — the edit page wasted 84px on a page that didn't even scroll. They now fill by flex. Note: h-full is deliberately not used, since a percentage height doesn't resolve against a parent sized by flex-grow.
  • Paging. 10 rows per page on the catalog, members, and templates tables. The page clamps on read rather than resetting in an effect, since these lists shrink underneath the control constantly (search, delete).

Test plan

Re-run after rebasing onto main (the branch had forked at #58 and was missing #72, #73, #76, #77):

  • go build ./..., go vet ./..., go test ./... — all clean on the rebased tree.
  • vitest161 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 --noEmit clean; biome check clean on changed files.
  • Manually verified in light and dark mode: catalog, connect hub + provider, frame detail, new/edit (form and markdown modes), admin members + templates, versions dialog.

Screenshots

Screenshot 2026-08-20 at 7 16 35 PM Screenshot 2026-08-20 at 7 16 44 PM Screenshot 2026-08-20 at 7 16 55 PM Screenshot 2026-08-20 at 7 17 19 PM Screenshot 2026-08-20 at 7 17 28 PM Screenshot 2026-08-20 at 7 17 37 PM Screenshot 2026-08-20 at 7 17 48 PM Screenshot 2026-08-20 at 7 17 55 PM Screenshot 2026-08-20 at 7 18 04 PM

🤖 Generated with Claude Code

@jbouder
jbouder requested a review from dcmcand as a code owner August 20, 2026 23:18
@github-actions

Copy link
Copy Markdown

📄 Docs preview for feat/lightweight-frame-body:
https://feat-lightweight-frame-body.nebari-frames.pages.dev

@jbouder

jbouder commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

@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.

@dcmcand dcmcand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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 deleted slots.go.
  2. Duplicate goose migration version 005, which stops every migration from running and prevents the server from booting. See the comment on the migration.
  3. A --- line inside a frontmatter block scalar truncates the document, including documents the exporter itself produces. See the comment on framemd.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.md on main documents the deleted design as current: :82 (Doc/Slots types), :84-86 (slots.go as single source of truth for slot keys), :87-88 ("one ## section per slot"), :91 ("the child's own slots win last"), :143 (reflective guards walking frames.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,224 still describes per-slot typed editors and slot rendering, and :171-172 names web/src/lib/slot-sections.ts, which this PR deletes. Same treatment as the MCP design doc (see the compose.go comment).
  • Body size is unbounded and Resolve appends every ancestor's body, so resolved size grows with chain depth. There is dedup by ref@version for diamonds, but no content dedup and no size or depth cap, and there is no connect.WithReadMaxBytes anywhere in backend/. 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: Resolve never populates acc.Extends, so compose.go's > Inherits from: line is dead on every production path (resources.go:107 and :171 are the only callers). compose_test.go covers it with a hand-built Doc literal, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Rebased onto main (which also brought #76 and #77), and ported write.go to the free-form body.

The reflective guard asked for a decision on body and template, and the answer is yes to both. body is what a Frame's content is now. template because you were right that create_frame had no way to set it at all — it is a *bool, so omitted still means keep, and update_frame cannot de-list a template by accident. The guard now walks frames.Doc alone and lists each field with the decision recorded next to it, so a new field fails with a question rather than defaulting to no.

go build, go vet, and go test ./... are green on the rebased tree.

-- 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — renumbered to 006_frame_is_template.sql, and added the missing -- +goose Down.

One thing worth flagging that fell out of it: migrate_legacy_test.go then failed with no such table: frames. Its fixture builds the schema as of 004 but only carried the tables 005 needed, so 006's ALTER had nothing to alter. It now carries the frames table too, seeds a row, and asserts a pre-006 frame comes forward with is_template = 0 rather than NULL. Also added a comment saying the fixture must carry every table a later migration touches — the failure mode reads like a broken migration when it is actually a stale test.

Comment thread backend/internal/frames/framemd.go Outdated

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. isFence now compares at column 0 (TrimRight(line, " \t") == "---"), which an indented block scalar can never produce. Trailing whitespace still closes, since editors add it and it cannot occur inside a scalar without the indentation that already disqualifies the line.

Table-driven, per your suggestion: the block scalar both after and before the required fields (the silent variant you found), a --- in the body, the trailing-whitespace fence, and a round trip through MarshalMarkdown of a document with a --- in its description — which is the case where the product destroyed its own output. I checked the test fails against the old comparison before fixing it; the reordered-frontmatter case is the one that was silent, and it now reports the lost version explicitly.

Also softened the lossless claims. The file header now names the two normalizations that actually happen — empty visibility becoming internal, and blank lines around the body being dropped — and states that nothing between the body's first and last non-blank line is touched. You were right that the unqualified claim is what stops someone looking.

Comment thread web/src/lib/frame-yaml.ts Outdated
];
for (const [heading, items] of lists) {
if (items && items.length > 0) {
sections.push(`## ${heading}\n\n${items.map((i) => `- ${i}`).join("\n")}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, taking the second option — kept the mirror and pinned it.

legacySlotsToMarkdown is now a faithful port of renderMarkdown, built the same way: a writeBullet that indents continuation lines two spaces and leaves a blank line bare, and replace(/^\n+|\n+$/g, "") in the prose branch instead of .trim().

The fixture is testdata/legacy-slots/, read by both TestLegacySlots_SharedFixture and a describe block in frame-yaml.test.ts, compared on the whole string. expected.md is generated from the Go side, so Go stays canonical. The input carries the cases you named plus the ones that would drift the same way — the multi-line item whose continuation is a nested list, an item with a blank line in the middle, prose with leading spaces that .trim() would eat, and a multi-line terminology definition (kept, even though you are right it cannot come from the form). The fixture's header comment says not to simplify the awkward values.

Your point about why neither side caught it — single-line values and substring assertions — is written into both test comments, since that is the part that would otherwise be repeated.

const restore = () => {
setRestoreError(null);
const content = new TextEncoder().encode(
serializeFrameDoc({ ...doc, version: nextVersion }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept the mirror rather than routing through the server, so this path stays as you describe it — but the two implementations are now pinned to one shared fixture (testdata/legacy-slots/), compared byte for byte on both sides. Details in the frame-yaml.ts thread.

Going through ConvertFrame would remove the class of bug entirely, and it is the better answer long-term. I did not take it here because it puts a network round trip in front of rendering a stored version, including on the detail page's first paint. Worth revisiting if the mirror drifts again despite the 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=?`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. TestSQLite_IsTemplateRoundTripsThroughEveryReadPath covers GetFrameByID, GetFrameBySlugName, ListFramesByOrg, FrameChildren, and the UPDATE arm on republish, which is a separate statement from the INSERT and would fail independently.

Two things from your framing made it into the test. It asserts the false case next to the true one, since a scan landing on the wrong column still satisfies a true-only test whenever the neighbour happens to be truthy. And it asserts Id and LatestVersion either side of the flag, so an off-by-one column reports as such rather than as a mysteriously wrong boolean. I verified it by swapping is_template and latest_version in the SELECT lists — four subtests fail, naming the column.

The comment records why the gap existed: the service tests use store.NewMemory(), which round-trips the proto and executes none of this SQL.

Comment thread web/src/app/routes.tsx
<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 />} />

@dcmcand dcmcand Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, intentional. Your read is right on both counts: the delete affordance moved to the permission-gated catalog table, and /admin no longer dead-ends, so no capability is lost.

Adding the removals to the PR description as you suggest — the truncated file list hiding three deletions is a bad way for a future reader to find out.

}
fmt.Fprintf(&b, "## %s\n\n%s\n\n", d.Heading, strings.Trim(body, "\n"))
}
if body := strings.Trim(doc.Body, "\n"); body != "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed both.

§3.4 gets the same treatment #73 used on this file: a superseded block naming the issue, the actual current output (including the Version: line, which is load-bearing since it is what update_frame needs as base_version), and the ten-slot format struck through and retained, since versions published under it still exist in storage. The comment at :11 now says it supersedes that section rather than citing it as current.

The empty-version nit is fixed too — it builds the ref bare and appends @version only when there is one, matching MarshalMarkdown. Agreed it is unreachable today; it was inconsistent with its sibling, which is enough.

The other two doc-drift items from your summary are done as well: docs/design/2026-05-21-web-app-design.md §168/§224 and the slot-sections.ts reference, and the SlotTable sections in the migration doc.

return nil, fmt.Errorf("parse frame yaml: %w", err)
}
return &d, nil
if d.Slots != nil && d.Body == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fixed.

Precedence is pinned by TestParse_BodyWinsOverLegacySlots, table-driven with three cases: slots alone folding into the body, an explicit body winning with the legacy block dropped, and an explicitly empty body still falling back to slots. The code comment says why the explicit body wins — slots: is a fallback rendering of content nothing writes any more, so preferring it would discard a deliberate edit in favour of a reconstruction.

The docYAML leak is fixed. Parse now rewrites the yaml.v3 message so the error names the offending key and lists the recognized fields instead of an unexported Go type, which is what a client can act on. TestParse_UnknownKeyErrorNamesTheSchema asserts the type name is absent, since that is the part that regresses silently.

Comment thread web/src/pages/FrameDetailPage.tsx Outdated
>
<ArrowLeft />
</Button>
<h1 className="text-2xl font-semibold">{frame.name}</h1>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with the two classes rather than a refactor, for the reason you gave — PageHeader stacks the description under the title and this header runs badges inline with it. Added a comment saying why it is not PageHeader and that the classes are deliberately identical to PageHeader's own h1, so the next person does not re-derive it.

Checked the rest of the app for the same drift: grep for text-2xl font-semibold without tracking-tight now returns nothing.

jbouder and others added 3 commits August 24, 2026 07:26
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>
…truncation

Rebased onto main, which brought #72, #73, #76, and #77. The rebase produced no
git conflicts but two packages that did not compile, plus a migration collision
that stopped the server booting. Fixes those, the codec data-loss bug, and the
smaller items from the same review.

Blockers:

- Port the MCP write surface to the free-form body. `write.go` declared ten
  slot-named input fields against a `frames.Doc` that no longer has them. It now
  carries `body` and `template`: `body` because that is what a Frame's content
  is, and `template` because the reflective guard in `resources_test.go` asked
  for a decision and the answer is yes — without it `create_frame` cannot make a
  template at all, and an omitted-means-keep pointer stops `update_frame`
  de-listing one by accident. The guard now walks `frames.Doc` alone, since
  `SlotTable` is gone.

- Renumber `005_frame_is_template.sql` to 006. main's `005_canonical_membership_
  email.sql` claimed the same version, and goose rejects duplicates at provider
  construction, so no migration ran at all. Adds the missing `-- +goose Down`.
  `migrate_legacy_test.go` builds a schema as of 004 and did not carry the frames
  table, so 006's ALTER failed there as "no such table"; the fixture now carries
  every table a later migration touches, and asserts a pre-006 frame comes
  forward as not-a-template.

- Match the frontmatter delimiter only at column 0. `TrimSpace(line) == "---"`
  let an indented `---` inside a YAML block scalar close the frontmatter, which
  truncated the document and dropped every field after it — including documents
  the exporter itself produced, where the error named a field the author never
  touched. Table-driven cases cover the block scalar in both positions, a `---`
  in the body, and a full round trip through the codec's own output. The
  unqualified "lossless" claims are replaced with the two normalizations that
  actually happen.

Should-fix:

- `frame-yaml.ts` claimed to mirror `legacy.go` and did not: no two-space
  continuation indent, and `.trim()` where Go trims newlines only. That reaches
  storage, since restoring a legacy version re-serializes the TypeScript render
  as canonical content — one rule with a nested list became three flat rules,
  permanently. The port is now faithful, and both sides are pinned to one shared
  fixture, `testdata/legacy-slots/`, compared whole rather than by substring.
  Substring assertions over single-line values are what let the drift through.

Also:

- Pin the legacy-vs-`body:` precedence, which was silent and untested, and stop
  `Parse` naming the unexported `frames.docYAML` in unknown-key errors that reach
  API clients unwrapped. The error now lists the recognized keys.
- Cover `is_template` at the sqlite level: four SELECT lists and four Scan calls,
  none of them exercised by the service tests, which use `store.NewMemory()`.
  Asserts both the true and false case and the columns either side, so a
  scan-order mismatch fails.
- Guard the empty version in `compose.go`'s `Inherits from:` line, matching
  `MarshalMarkdown`.
- Give the one page-level heading that cannot use `PageHeader` the classes
  `PageHeader` exists to hold steady.
- Rewrite the parts of AGENTS.md and the three design docs that described the
  deleted slot design as current. The MCP doc's §3.4 gets the struck-through
  supersede treatment #73 set the precedent for on that same file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Concatenating ancestor bodies drops two overrides the ten-slot resolver had:
terminology merged by key, and prose slots replaced outright. Neither is
recoverable without giving the body addressable sections again, which would
rebuild the retired schema under another name.

Records the trade in the migration design doc alongside the resolution
algorithm it supersedes, names the two things that did *not* change (removal of
a parent's rules never existed; excludes still works at ancestor granularity),
and states the bet explicitly - that a reader honors ordering - along with the
narrower fixes available if it turns out badly. resolver.go's doc comment points
at it, since the comment asserted the bet without recording that it is one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jbouder
jbouder force-pushed the feat/lightweight-frame-body branch from 2fb6817 to e920e45 Compare August 24, 2026 11:56
@jbouder

jbouder commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks — this was a genuinely useful review, particularly reproducing the codec bug through the product's own output and running the TS/Go comparison on both sides rather than translating by hand. Both are what made those two actionable rather than arguable.

Pushed as a rebase onto main plus two commits. Replies are on each thread; the shape of it:

Blockers — all three fixed.

  1. Rebased. It also picked up ci: fix e2e-sandbox by using the nebari-apps ArgoCD project, migrate to sandbox v3 #76 and feat(chart): add trustBundle, and exercise real auth in the e2e sandbox job #77 beyond the three you listed. write.go is ported to the free-form body, and the reflective guard's question is answered in both directions: body and template are now MCP write fields, which incidentally closes the create_frame gap you found. go build, go vet, go test ./... green.
  2. Renumbered to 006, with the missing -- +goose Down. That surfaced a second one: migrate_legacy_test.go built a schema as of 004 that had no frames table, so 006's ALTER failed there as no such table. The fixture now carries every table a later migration touches and asserts a pre-006 frame comes forward as not-a-template.
  3. The frontmatter fence matches at column 0. Table-driven, including the reordered-frontmatter case that was fully silent and a round trip through MarshalMarkdown's own output. The unqualified "lossless" claims are replaced with the two normalizations that actually occur.

frame-yaml.ts — kept the mirror and pinned it, per your second option. testdata/legacy-slots/ is read by both suites and compared whole; expected.md is generated from Go, so Go stays canonical. The fixture carries the multi-line item with nested markup, an item with an interior blank line, and prose with leading spaces that .trim() would eat.

The three questions — all three were intentional, and I have answered each on its thread.

  • The resolver trade is now recorded in the migration design doc §3.4, since you were right that the comment asserted the bet without saying it was one. It names what was lost (per-term and per-prose override), what was never lost (removal, which mergeStrings never supported), why per-section override needs addressable sections the spec deliberately does not define, and the narrower fixes available if the bet goes badly.
  • is_template is document-derived on purpose — a frame's state is what its current document says.
  • The /admin removals are now in the PR description, as you suggested.

Also done: the is_template sqlite coverage (all four read paths plus the UPDATE arm, asserting the false case and the neighbouring columns — verified by swapping columns in the SELECT), the schema.go precedence pin and the docYAML leak, compose.go's empty-version guard, the FrameDetailPage heading classes, and the AGENTS.md and design-doc rewrites.

Left as follow-ups, as you suggested: unbounded resolved-body growth with chain depth and the missing connect.WithReadMaxBytes, and the dead > Inherits from: branch in compose.go — both pre-existing and identical at merge-base. Happy to file both; say the word if you would rather they were in this PR.

@jbouder
jbouder requested a review from dcmcand August 24, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Frame templates: start a new Frame from a starter template Simplify the Frame authoring form to the lightweight Frame body

2 participants