Skip to content

Docs site built on Flatbread, reading the repo's own Markdown - #247

Open
tonyketcham wants to merge 13 commits into
mainfrom
toeknee/docs-site-ascii-c1d8
Open

Docs site built on Flatbread, reading the repo's own Markdown#247
tonyketcham wants to merge 13 commits into
mainfrom
toeknee/docs-site-ascii-c1d8

Conversation

@tonyketcham

@tonyketcham tonyketcham commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

docs time

The repository's guides now live at apps/docs/content/docs and the site
reads those exact files. Package README symlinks under content/reference
do the same for the eleven package docs, so nothing is copied.

- Add apps/* to the pnpm workspace, and exclude apps/* from the root
  build, dev, and build:types filters. Without that, pnpm build would
  boot a GraphQL server and run next build inside the library build,
  which every CI job and pnpm test depend on.
- Add frontmatter (id, title, section, order, summary, related) to the
  seven guides so Flatbread can validate ids and order the navigation.
- Repair every link that pointed at the old docs/ path.
- Add five markdown plugins the site runs through Flatbread's transformer:
  strip the leading H1, carry code fence titles, rewrite repo links to
  site routes, add heading anchors, and colour code with Shiki.

Change-Id: I472065d89b0b2b3ecd7752eebe81877d4151c992
The site at apps/docs renders every guide and package README through
Flatbread's GraphQL server while it builds, then exports 22 static files.
Nothing calls a server once the build is done.

Interface: a terminal document. One monospaced family, widths counted in
character cells, every vertical step one line high, two tones and a
single accent taken from the Flatbread mark. Panels use one-pixel borders
with box-drawing characters at the corners so they hold their shape at
any width; the navigation is a real directory tree, where the connectors
never have to stretch.

Motion: the hero resolves out of punctuation one cell at a time, page
titles assemble character by character, the caret marking the current
page slides between rows, and the content column repaints on navigation.
Every one of those checks useReducedMotion first and renders its final
state instead.

Motion's splitText belongs to Motion+, which installs from a private
registry with a secret token, so the site splits text itself. The whole
string stays in aria-label and each piece is inline-block.

Also adds search (scored in the browser, since Flatbread ranks nothing),
a table of contents read from the rendered HTML, copy buttons added to
code blocks after mount, a link checker that fails the build on a broken
cross-reference, and an Ubuntu-only CI job.

Change-Id: Ic1a52e34ab701be06bceeff57c5b3ea5cc6df1d9
On a phone the tree pushed the page eighteen links down. It now sits
behind a 'pages' toggle below 900px and stays open above it. The open
state follows a media query, because the reliable way to force a
<details> open across browsers is still the open attribute.

Also records what this exercise found in the Proof: one retrospective
Finding listing six limits in the content model, two Issues for the real
defects (a content path cannot climb above the project directory, and
initial load disagrees with watch mode about capture patterns), and one
Constraint explaining why the site cannot depend on Motion+.

Change-Id: I8804bdab70b945675e0fe70cac889f8478883863
@cursor cursor Bot changed the title Toeknee/docs site ascii c1d8 Docs site built on Flatbread, reading the repo's own Markdown Aug 12, 2026
svimg 3.1 and 3.2 pull sharp 0.30 and 0.31. Those versions download a
prebuilt binary, and on a socket hang-up they fall back to node-gyp.
windows-latest ships Visual Studio 18, which node-gyp 11 does not
recognise, so the fallback always fails.

sharp 0.34 ships platform packages as optional npm dependencies and has
no compile path. One workspace override replaces every copy. svimg only
calls metadata, resize, jpeg/png/webp/avif quality, toFile, and
toBuffer, which still work.

Change-Id: I14e5b03cea0c3a49b5c40789128f0c0e0ff1ec78
sqlite3, pulled in by @cursor/sdk, still uses prebuild-install. A
socket hang-up on Windows Node 20 falls through to node-gyp, which
cannot use Visual Studio 18. Three attempts with a short pause cover
that remaining flake without changing the package.

Change-Id: Iff3853a4e6a4153ff39f8bac186e711d0de6826e
@mergify

mergify Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Review verdict

REQUEST_CHANGES — Consensus HIGH on two docs-app contracts: module-level once() freezes nav/search under the documented pnpm docs / watch path, and remark-repo-links rewrites relative links without the source file (first filesystem hit wins). GraphQL↔page wiring from #246 is fixed; merge still needs the watch memo fix, source-aware link rewrite (+ matrix test), and a thin unit net for the pure logic this app ships.

Chunk-bound feedback (priority)

  1. HIGH apps/docs/lib/content.tsonce() keeps the first GraphQL list forever; empty allDocs / allSections / allPackages still “succeed.” Skip/invalidate once outside production; fail (or assert vs disk) when collections are empty but content exists on disk.
  2. HIGH apps/docs/plugins/remark-repo-links.mjs — Source-blind first-hit resolution can map README.md to the wrong /reference/*. Pass the Markdown base (or require a unique match) and add a rewrite matrix test.
  3. MED apps/docs/scripts/check-links.mjs — Scalar related iterates characters; nav YAML dual-id / required fields unchecked.
  4. MED apps/docs/lib/toc.ts / lib/search.ts — Attribute-order TOC regex and ranking logic are pure and untested.
  5. MED Root pnpm docs / verify — No predocs build; verify omits docs:build while CI docs-site catches it.
  6. MED DX/docs paths — CONTRIBUTING omits the docs site; @flatbread/core README relative link breaks on npm; README should warn play vs docs port clash + symlink need.

Coverage plan (must-have first)

  1. remark-repo-links.test.mjs — root/./README.md/reference/flatbread/ (not first package); package README → matching /reference/<pkg>/
  2. lib/content.test.tsonce rereads in non-production after mocked corpus change; graph counts match on-disk collections
  3. check-links.test.mjs — scalar related clear type error; required frontmatter / nav YAML
  4. lib/toc.test.ts / lib/search.test.ts — attr-order TOC; short-query / AND / title-prefix ranking

Reviewer scoreboard

Perspective Verdict Signal
test-coverage-robustness REQUEST_CHANGES HIGH
docs-site-contracts REQUEST_CHANGES HIGH
dx-and-examples COMMENT MED
docs-and-positioning COMMENT MED
release-discipline COMMENT MED

Models: cursor-grok-4.5-high (HIGH), composer-2.5 (MED/LOW). Oven missing → Task fallback. Judge: /tmp/review-judge-final.md. Not dependency-only (93 non-lock paths).

Open in Web View Automation 

Sent by Cursor Automation: Flatbread PR Review

Comment thread apps/docs/lib/content.ts
Comment thread apps/docs/lib/content.ts
Comment thread apps/docs/plugins/remark-repo-links.mjs Outdated
Comment thread apps/docs/scripts/check-links.mjs Outdated
Comment thread apps/docs/lib/toc.ts
Comment thread package.json Outdated
Comment thread CONTRIBUTING.md
Comment thread packages/core/README.md Outdated
The docs app shipped pure logic with no tests and no place to put
them. Vitest now runs in `apps/docs`, and the root `test` script
includes it beside the codegen and utils suites.

Two gaps in the root scripts close with it. `pnpm docs` on a fresh
clone failed with an opaque `ERR_MODULE_NOT_FOUND` because nothing
built the packages first; `predocs` now runs `pnpm build`. And
`pnpm verify` never looked at the docs pages, so a broken link or a
missing frontmatter key only showed up in the `docs-site` CI job;
`verify` now ends with `pnpm docs:check`.

Change-Id: Ie1d41e9fae2dba2f80d600359c6393938432b1b1
`once()` held the first answer to every list query for the life of
the process. Under `flatbread start --watch -- next dev` that process
outlives every file save, so the sidebar, the home page, and search
kept showing the corpus as it stood when the server started, while a
guide page read through `getDoc` refreshed. The memo now applies only
to a production build, which is the case it was written for.

An empty collection also used to build green. `allDocs` coming back
empty flattened to `[]`, and the site shipped a blank home page and an
empty sidebar; `check-links` could not catch it, because it reads the
filesystem rather than the graph. Each list reader now fails with the
collection name and the directory to look at.

Change-Id: I3f30efd46aa6f684d63e8666e49bf9a555960931
Flatbread hands the markdown processor a string, not a file, so the
rewriter cannot see which page a link came from. It used to resolve
the link against the guides folder and every package folder in turn
and take the first hit. That is a guess. A link to `./README.md`
inside `packages/core/README.md` landed on `/reference/codegen/`,
the first package with a README, and any `./`-relative path that
exists under two packages had the same fault.

The rewriter now collects every distinct file the bases point at and
rewrites only when they agree on one. Bases often do agree: every
package sits at the same depth, so `../../CONTRIBUTING.md` names the
same file from any of them. An ambiguous link is left alone, and
`check-links` reports it with the candidates and a way out, so it
fails the build instead of shipping a link that only works on GitHub.

Two smaller faults go with it. A scalar `related:` in frontmatter used
to iterate characters and report a missing page per letter; it now
reports once that `related` must be a list. And the checker's work
moved into an exported `collectProblems()` so tests can call it,
leaving the command-line behaviour unchanged.

Change-Id: I48ecb9c80a7c904b909a7da07a8f04826fc6d8a8
The contents list read the rendered HTML with a pattern that demanded
`id` be the first attribute. A heading serialized as
`<h2 class="..." id="...">` produced no entry, so the page lost its
contents list while the anchors beside each heading still worked.
The pattern now matches the tag and looks for `id` among whatever
attributes it carries.

Two comments in `rehype-heading-anchors.mjs` claimed the plugin kept
its slug rule in step with one in `lib/toc.ts`. There is no such rule
there: the plugin is the only place a heading id is made, and
`lib/toc.ts` reads those ids back out of the HTML. The comments now
say that.

Tests cover both files and the search ranking, which had none.

Change-Id: I740c764eb050959eb2b29d791c011544ac198ddc
`CONTRIBUTING.md` sent every newcomer to `pnpm play` and
`examples/nextjs`, so a contributor editing a guide had no way to
learn that `pnpm docs`, `pnpm docs:build`, and `pnpm docs:check`
exist. It now names them and says plainly that `pnpm verify` checks
the docs pages but leaves the full docs build to CI.

`packages/core/README.md` linked to the export guide with a path that
climbs out of the package. That works in the monorepo and on the site,
but the npm page for `@flatbread/core` ships only the package
directory, so the reader most likely to follow it got a 404. It is now
an absolute GitHub URL, as in `packages/flatbread/README.md`.

`apps/docs/README.md` gains the two things that catch people out:
`pnpm play` and `pnpm docs` both want ports 5057 and 3000, so only one
may run at a time, and the `content/reference` symlinks come out as
plain text on a Windows clone without symlink support.

Change-Id: Idb68b48b703f33e235abdca57d05cb468d27f622

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Review verdict

REQUEST_CHANGES — Prior eaf20ad blockers are fixed at c3383dc (once no longer freezes watch chrome; empty collections throw; remark is unanimous not first-hit; related scalar guarded; TOC id order-independent; predocs / CONTRIBUTING / core README links landed). Two independent HIGHs remain, plus a non-empty coverage plan over non-test sources.

Blocking / high

  1. apps/docs/lib/content.test.ts — GraphQL mocks never assert collection counts against disk (CONTENT_DIRS). Under-count still passes unit tests.
  2. package.json pnpm.overrides.sharp — Workspace-wide native pin in a docs PR without release callout; affects examples/nextjs and other consumers.

Medium (selected)

  • Post-flatMap silent empty when all GraphQL rows are invalid (content.ts).
  • Hero “flat files” framing vs positioning (app/page.tsx).
  • apps/docs/README.md root vs package-local run instructions.
  • Untested graphql.ts failure paths.
  • CONTRIBUTING omits port collision with pnpm play.

Coverage plan

  1. content.test.ts — graph↔disk parity; all-invalid-row throw; getPackages/getSearchEntries empty + prod once; missing html reject.
  2. graphql.ts — non-OK / errors / no data.
  3. remark-repo-links.test.mjs — missing target unchanged; hash on blob URL.
  4. check-links.test.mjs — dangling reference symlink; ignored schemes.
  5. Plugin units — strip-first-H1, code-meta, rehype duplicate slug.

Reviewer scoreboard

Perspective Signal
test-coverage-robustness HIGH — graph↔disk
docs-site-contracts HIGH on prior resolution; residual MED post-flatMap
release-discipline HIGH — sharp override
dx-and-examples MED — README/CONTRIBUTING
docs-and-positioning MED — hero copy

Prior automation threads from eaf20ad marked fully_addressed. Oven unavailable → Task fallback (models: cursor-grok-4.5-high / composer-2.5). Judge: /tmp/review-judge-final.md.

Open in Web View Automation 

Sent by Cursor Automation: Flatbread PR Review

Comment on lines +26 to +39
it('re-queries getDocs outside production when the corpus changes', async () => {
vi.mocked(query)
.mockResolvedValueOnce(docsPayload('Old title') as never)
.mockResolvedValueOnce(docsPayload('New title') as never);

const first = await getDocs();
const second = await getDocs();

expect(query).toHaveBeenCalledTimes(2);
expect(first[0]?.title).toBe('Old title');
expect(second[0]?.title).toBe('New title');
});

it('rejects when allDocs is empty', async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH — These suites mock GraphQL and prove empty-array reject / once re-query, but they never assert allDocs / allSections / allPackages lengths against files under CONTENT_DIRS. A silent under-count (one dropped guide) still passes unit tests; CI only fails on fully empty collections.

Minimal fix: Add graph↔disk count parity (readdir vs reader, or a live flatbread start fixture). Also cover getPackages / getSearchEntries empty + prod once, and all-invalid rows after flatMap.

Comment thread package.json
Comment on lines +95 to +97
"overrides": {
"sharp": "0.34.3"
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGHpnpm.overrides.sharp: "0.34.3" is workspace-wide, not docs-local. It rewrites native resolution for examples/nextjs and every other transitive sharp consumer, inside a docs PR, with no migration/release callout.

Minimal fix: Call this out in the PR/release notes and confirm integration-nextjs stays green on the pin (or scope the override if possible).

Comment thread apps/docs/lib/content.ts
Comment on lines +104 to +110
export const getSections = once(async (): Promise<Section[]> => {
const data = await query(AllSectionsDocument);
return expectRecords('allSections', data.allSections ?? []).flatMap(
(section) =>
section?.id && section.title
? [
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED (consensus)expectRecords then flatMap checks GraphQL array length before narrowing. If every row fails id/title filters, callers get [] with no throw — silent empty sidebar/home/search. Missing _content.html on a found page still ships blank prose.

Minimal fix: Throw when narrowed length is 0; treat missing html as an error when id/title exist; add a test for all-invalid rows.

Comment thread apps/docs/app/page.tsx
by="word"
stagger={0.02}
delay={0.5}
text="Flatbread reads flat files and hands your app a typed graph. Collections hold records, refs link them, and GraphQL is one way to read the result — not the whole product."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Hero lede leads with “reads flat files…”, which undercuts the canonical framing in apps/docs/content/docs/positioning.md and layout.tsx metadata (files in Git → typed relational graph; GraphQL is one read path).

Minimal fix: Align this sentence with positioning/layout copy.

Comment thread apps/docs/README.md
Comment thread apps/docs/lib/graphql.ts
Comment on lines +43 to +57
if (!response.ok) {
throw new Error(
`Flatbread answered ${response.status} for a query. Is \`flatbread start\` running on ${endpoint}?`
);
}

const result = (await response.json()) as GraphQLResponse<TData>;

if (result.errors?.length) {
throw new Error(result.errors.map((error) => error.message).join('\n'));
}

if (!result.data) {
throw new Error('Flatbread returned no data.');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Throw paths for non-OK HTTP, GraphQL errors, and missing data have no unit tests.

Minimal fix: Mock fetch for those three negatives.

Comment thread CONTRIBUTING.md Outdated
- Build all packages: `pnpm build`
- **Workspace libraries (watch-only):** `pnpm dev` — runs package `dev` scripts (e.g. `tsup --watch`) for `packages/*`; it does **not** start the Next.js example.
- **Next.js example:** prefer the flow under [Recommended onboarding](#recommended-onboarding-try-flatbread-in-the-nextjs-example); or `pnpm play` as a convenience alias.
- **Documentation site:** from the repo root, `pnpm docs` builds the packages (`predocs` runs `pnpm build` first, so a fresh clone works), then starts Flatbread on **5057** and Next on **3000**. `pnpm docs:build` builds the packages and then the static site. `pnpm docs:check` checks frontmatter and links without building. The content model is in `apps/docs/README.md`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Docs bullet correctly covers predocs / docs:build / docs:check, but omits that pnpm play and pnpm docs both bind 5057 and 3000. Onboarding ends on pnpm play, so the collision is easy to hit.

Minimal fix: One sentence here, or a cross-link to the warning in apps/docs/README.md.

The site exports to plain files, so every GraphQL read must be
cacheable: a `no-store` fetch makes the route dynamic, and a dynamic
route cannot be exported. Next keeps those cached answers in
`.next/cache/fetch-cache`, and that directory outlives the build.
Since each build sent the same URL, method, body, and headers, the
second build read the first build's answers.

Edit a guide, run `pnpm docs:build`, and the page kept its old title.
CI never saw it, because a fresh checkout has no `.next`, so the fault
only showed up after the stale page reached a reader.

The request URL now carries a stamp taken once when the module loads.
A new build gets a new key; repeated queries inside one build still
hit the cache, which is what `force-cache` is there for.

Change-Id: Ia963689e5930d1a764174c7c63f0f564a3e5d970
A package README that must read well on npm cannot use a relative
path, so it writes the absolute GitHub URL instead. The rewriter only
looked at relative links, so `/reference/flatbread/` — the repository
README, and the busiest reference page here — threw the reader out to
GitHub for the glossary, the export guide, and a dozen other guides
sitting one route away.

An absolute link into this repository names its file outright, so
there is nothing to guess: when the site serves that file, the link
now becomes the site route. A link to a file with no page here, to a
file that is gone, or to another branch is left as it stands. That
halves the GitHub links in the exported site, from 31 to 14.

Change-Id: I72bc0d562b09f68385202650eb6c7e8f7abf2afd

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Review verdict

BLOCK — workspace-wide pnpm.overrides.sharp (package.json:95-97) still has no PR body / CHANGELOG Unreleased callout (release-discipline blocker). Independently: consensus HIGH graph↔disk parity and GraphQL failure-path coverage remain open after c3383dc…013c43c (stamp + blob only).

Not dependency-only (102 non-lock paths). Oven missing → Task fallback. Models: cursor-grok-4.5-high (HIGH), composer-2.5 (MED/LOW). Perspectives: test-coverage-robustness, docs-site-contracts, dx-and-examples, docs-and-positioning, release-discipline.

Prior threads (sync c3383dc)

All seven remain not_addressed: graph↔disk; sharp override; post-flatMap silent empty; hero “flat files”; README predocs/pnpm dev; GraphQL throw tests; CONTRIBUTING port collision.

Chunk-bound feedback (priority)

  1. BLOCKER package.json:95-97 — document or scope the sharp@0.34.3 override.
  2. HIGH content.test.ts / content.ts — graph↔disk count/id parity still missing.
  3. HIGH graphql.test.ts / graphql.ts!ok / errors / no-data throws untested (stamp-only delta).
  4. MED content.tsexpectRecords then flatMap can still yield silent [].
  5. MED hero / README / CONTRIBUTING — same copy and DX gaps as last sync.

Coverage plan

  1. content.test.ts — disk glob vs getDocs/getSections/getPackages/getSearchEntries size+ids; getDoc/getPackage happy + miss.
  2. content.test.ts — post-flatMap all-invalid rows must fail (not silent empty).
  3. graphql.test.ts!response.ok, GraphQL errors, missing data.
  4. check-links.test.mjs — dangling symlink branch.
  5. remark-repo-links.test.mjs — bad percent-encoding / .. escape (LOW).

Reviewer scoreboard

  • test-coverage-robustness: 7 findings, 8 coverage gaps, signal:HIGH
  • docs-site-contracts: 6 findings, 5 coverage gaps, signal:HIGH
  • dx-and-examples: 3 findings, signal:MED
  • docs-and-positioning: 3 findings, signal:MED
  • release-discipline: 3 findings, signal:HIGH

Delta since last review closed cache-stamp freshness only; it did not clear the open HIGHs.

Open in Web View Automation 

Sent by Cursor Automation: Flatbread PR Review

Comment thread package.json
Comment on lines +95 to +97
"overrides": {
"sharp": "0.34.3"
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

BLOCKERpnpm.overrides.sharp: "0.34.3" is still workspace-wide with no PR-body or CHANGELOG Unreleased callout. This rewrites native resolution for every consumer (including examples/nextjs / svimg), inside a docs PR.

Minimal fix: Document why the pin exists, who it affects, and when it can drop — or scope/remove it.

expect(first[0]?.title).toBe('Frozen');
expect(second[0]?.title).toBe('Frozen');
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH (consensus) — Suites still mock GraphQL and prove empty-array reject / once re-query / one flatMap drop, but never assert allDocs / allSections / allPackages lengths against files under CONTENT_DIRS. A silent under-count still passes unit tests; CI only fails on fully empty collections. Delta c3383dc…013c43c did not touch this.

Minimal fix: Graph↔disk count/id parity (readdir vs reader, or live flatbread start fixture). Also cover getPackages / getSearchEntries empty + all-invalid rows after flatMap.

Comment thread apps/docs/lib/content.ts
Comment on lines +104 to +118
export const getSections = once(async (): Promise<Section[]> => {
const data = await query(AllSectionsDocument);
return expectRecords('allSections', data.allSections ?? []).flatMap(
(section) =>
section?.id && section.title
? [
{
id: section.id,
title: section.title,
order: section.order ?? 0,
blurb: section.blurb ?? '',
},
]
: []
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED (consensus)expectRecords then flatMap checks GraphQL array length before narrowing. If every row fails id/title filters, callers get [] with no throw — silent empty sidebar/home/search.

Minimal fix: Throw when narrowed length is 0; add a test for all-invalid rows.

new URL(first).searchParams.get('build')
);
});
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH — Delta added build-stamp stickiness only. The three live throw paths in graphql.ts (!response.ok, GraphQL errors, missing data) still have no unit tests.

Minimal fix: Mock fetch for those three negatives beside the stamp case.

Comment thread apps/docs/app/page.tsx
Comment on lines +52 to +58
<p className="fb-hero__lede">
<SplitText
by="word"
stagger={0.02}
delay={0.5}
text="Flatbread reads flat files and hands your app a typed graph. Collections hold records, refs link them, and GraphQL is one way to read the result — not the whole product."
/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Hero lede still opens with “reads flat files…”, which undercuts positioning.md and layout.tsx metadata (files in Git → typed relational graph; GraphQL is one read path).

Minimal fix: Align this sentence with positioning/layout copy.

Comment thread apps/docs/README.md
Comment thread CONTRIBUTING.md Outdated
- Build all packages: `pnpm build`
- **Workspace libraries (watch-only):** `pnpm dev` — runs package `dev` scripts (e.g. `tsup --watch`) for `packages/*`; it does **not** start the Next.js example.
- **Next.js example:** prefer the flow under [Recommended onboarding](#recommended-onboarding-try-flatbread-in-the-nextjs-example); or `pnpm play` as a convenience alias.
- **Documentation site:** from the repo root, `pnpm docs` builds the packages (`predocs` runs `pnpm build` first, so a fresh clone works), then starts Flatbread on **5057** and Next on **3000**. `pnpm docs:build` builds the packages and then the static site. `pnpm docs:check` checks frontmatter and links without building. The content model is in `apps/docs/README.md`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Docs bullet correctly covers predocs / docs:build / docs:check, but still omits that pnpm play and pnpm docs both bind 5057 and 3000. Onboarding ends on pnpm play, so the collision is easy to hit.

Minimal fix: One sentence here, or a cross-link to the warning in apps/docs/README.md.

`pnpm docs` never started the site. pnpm has a built-in `docs`
command — the same one npm has — which opens the package homepage in
a web browser, and it wins over a script of that name. Following the
instructions in `CONTRIBUTING.md` opened a browser tab, left ports
3000 and 5057 empty, and printed no error to explain it.

The script is now `docs:dev`, with `predocs:dev` building the packages
before it. A colon cannot collide with a pnpm command, so the three
docs commands — `docs:dev`, `docs:build`, `docs:check` — are safe as a
set. Nothing is left behind under the old name, since a shadowed
script is a trap rather than a convenience.

Change-Id: I196ad5591ae111a56d2aa2978683f19e8202c7da

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review verdict

BLOCK — Workspace-wide pnpm.overrides.sharp: "0.34.3" still has no CHANGELOG/CONTRIBUTING/migration note. Consensus HIGH holes also remain: post-flatMap silent [], missing graph↔disk parity tests, and untested GraphQL !ok / errors / missing-data throws.

Not dependency-only (99 non-dep paths of 103). Delta since 013c43c: rename docsdocs:dev only. That closes the README redundant manual build and the pnpm script-name collision. It does not close the open BLOCKER/HIGH items.

Blocking / consensus (priority)

  1. BLOCKER — Document or scope the workspace sharp override (package.json).
  2. HIGH — Re-guard after flatMap in content.ts; add all-invalid + graph↔disk tests.
  3. HIGH — Three GraphQL throw-path unit tests.

Coverage plan

  1. content.test.ts — all-invalid rows after flatMap must fail closed.
  2. content.test.ts — graph↔disk count/id parity.
  3. content.test.ts — empty/all-invalid getPackages / getSearchEntries.
  4. graphql.test.ts — negatives for !ok, errors[], missing data.
  5. Remark findTargets — reject paths outside repoRoot.
  6. CHANGELOG + CONTRIBUTING note for pnpm.overrides.sharp.

Delta closed this sync

  • apps/docs/README.md / CONTRIBUTING.md / root scripts: pnpm docs:dev + predocs:dev (no redundant root pnpm build in the quick start).

Reviewer scoreboard

  • test-coverage-robustness: HIGH findings (content + GraphQL)
  • docs-site-contracts: HIGH content/sharp; MED remark/check-links
  • release-discipline: BLOCKER sharp; MED pipeline/CONTRIBUTING
  • dx-and-examples: APPROVE (LOW CONTRIBUTING port note)
  • docs-and-positioning: APPROVE (LOW hero/glossary; MED dual front door)

Oven CLI unavailable → Task-tool fallback. Models: cursor-grok-4.5-high (HIGH), composer-2.5 (MED/LOW).

Open in Web View Automation 

Sent by Cursor Automation: Flatbread PR Review

Comment thread package.json
},
"pnpm": {
"overrides": {
"sharp": "0.34.3"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

BLOCKERpnpm.overrides.sharp: "0.34.3" is still workspace-wide with no CHANGELOG/CONTRIBUTING/migration note. It collapses older sharp pins for svimg / Next consumers inside a docs PR.

Minimal fix: Document why the pin exists, who it affects, and when it can drop — or scope/remove it.

Comment thread apps/docs/lib/content.ts

export const getSections = once(async (): Promise<Section[]> => {
const data = await query(AllSectionsDocument);
return expectRecords('allSections', data.allSections ?? []).flatMap(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH (consensus)expectRecords checks GraphQL array length before flatMap. If every row fails id/title filters, callers get [] with no throw — blank home/sidebar/search despite a non-empty GraphQL answer.

Minimal fix: Re-expectRecords (or throw) on the narrowed array; add an all-invalid-rows test.

await expect(getSections()).rejects.toThrow(/allSections/);
});

it('drops a doc that is missing a required field', async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH (consensus) — Suites still mock GraphQL and prove empty-array reject / drop-one / once re-query, but never assert allDocs / allSections / allPackages lengths against files under CONTENT_DIRS, and never cover all-invalid rows after flatMap.

Minimal fix: Graph↔disk count/id parity plus all-invalid / empty getPackages / getSearchEntries cases.

Comment thread apps/docs/lib/graphql.ts
cache,
});

if (!response.ok) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HIGH — Throw paths for !response.ok, GraphQL errors, and missing data still have no unit tests (stamp stickiness alone does not cover them).

Minimal fix: Three mock-fetch negatives in graphql.test.ts.

return { targets: [target], target, route, href: route + hash };
}

function findTargets(path, { repoRoot, bases }) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDfindTargets resolves under repoRoot/bases without a .. escape guard (unlike resolveBlobUrl).

Minimal fix: Reject resolved paths that leave repoRoot.

Comment thread apps/docs/app/page.tsx
by="word"
stagger={0.02}
delay={0.5}
text="Flatbread reads flat files and hands your app a typed graph. Collections hold records, refs link them, and GraphQL is one way to read the result — not the whole product."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW — Hero lede still opens with “reads flat files…”, which undercuts positioning.md / layout.tsx (“files in Git” → typed relational graph).

Minimal fix: Align this sentence with positioning/layout copy.

Comment thread CONTRIBUTING.md
- Build all packages: `pnpm build`
- **Workspace libraries (watch-only):** `pnpm dev` — runs package `dev` scripts (e.g. `tsup --watch`) for `packages/*`; it does **not** start the Next.js example.
- **Next.js example:** prefer the flow under [Recommended onboarding](#recommended-onboarding-try-flatbread-in-the-nextjs-example); or `pnpm play` as a convenience alias.
- **Documentation site:** from the repo root, `pnpm docs:dev` builds the packages (`predocs:dev` runs `pnpm build` first, so a fresh clone works), then starts Flatbread on **5057** and Next on **3000**. `pnpm docs:build` builds the packages and then the static site. `pnpm docs:check` checks frontmatter and links without building. The content model is in `apps/docs/README.md`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MED — Docs bullet correctly covers docs:dev / docs:build / docs:check, but still omits that pnpm play and pnpm docs:dev both bind 5057 and 3000. apps/docs/README.md already warns; onboarding here does not.

Minimal fix: One sentence here, or a cross-link to the README warning. Also land the sharp override note with the BLOCKER fix.

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.

2 participants