Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018tqpxGBVBUCe7nACwttGag
Preview deploymentsHost Test Results 1 files 1 suites 20m 47s ⏱️ Results for commit 18842f5. Realm Server Test Results 1 files ±0 319 suites ±0 1h 51m 15s ⏱️ + 2m 43s Results for commit 18842f5. ± Comparison against earlier commit 2e2a622. |
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review checked the package READMEs' API claims against packages/*/src, probed the new check-realm.mjs checks with deliberately broken docs, and swept the tree for references to the old docs/ and notes/ locations. It didn't review the prose of the moved docs beyond what the lint covers.
No blocking issues. The import check is weaker than the gallery README says: it skips every @cardstack/choreo/film import, and fixing that surfaces one import in a design sketch, which needs a call on whether design/ is in scope. The rest are small README corrections.
- Follow
export *inexportsOf, and decide whether design sketches are import-checked (check-realm.mjs,exportsOf). - glimmer-motion README, Testing:
bounds().xand thevelocityOfsignature. - glimmer-motion README, rule 1: show that the modifier's
styletakes an object. - choreo README,
<Film>example: the required@menuTitleis missing. - Question: do in-realm
#headinglinks land on choreo.boxel.site, given the host'suser-content-id prefix? (headingIds)
Follow-up, not this PR: the repo-link and site-URL checks drop the fragment, so cross-README anchors such as packages/glimmer-motion#participant-hosts and #testing aren't validated (both resolve today). headingIds already exists, so a fragment on a blob/main/….md link, or on a tree/main/<dir> link against that directory's README.md, could be checked the same way.
| const names = new Set(); | ||
| for (const node of tree.statements) { | ||
| if (ts.isExportDeclaration(node) && !node.exportClause) { | ||
| return undefined; // `export *`: anything may come through |
There was a problem hiding this comment.
[Claude Code 🤖] exportsOf gives up on export *, and @cardstack/choreo/film resolves to src/film.ts, which is a single export * from './film/index.ts'. So none of the 17 @cardstack/choreo/film imports in the docs, guides and READMEs is checked: appending import { NoSuchThing } from '@cardstack/choreo/film' to realm/docs/guide.md still passes pnpm lint:realm. Following the re-export closes it:
if (ts.isExportDeclaration(node) && !node.exportClause) {
const inner = exportsOf(
path.resolve(path.dirname(file), node.moduleSpecifier.text),
);
if (!inner) {
return undefined;
}
inner.forEach((name) => names.add(name));
continue;
}With that in place the check flags one import: type ShotArgs in realm/design/film-graph/sylva.gts.md, which no package exports. That file sketches syntax that doesn't exist, and the rest of the design history is kept as written. So the decision is whether the import loop should read design/ at all. I'd drop design/ from the files the import loop reads, and keep it for the link, site-URL and repo-link loops.
Regression in the new check. Not blocking, but the gallery README states this check as a guarantee, so I'd fix it here.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 18842f5. exportsOf now follows a relative export *, and returns undefined only for a re-export from another package. It also records export * as ns. The imports in design/ are now skipped, as you suggested, because those notes sketch APIs that were never built. The link, site-URL and repo-link loops still read design/. A planted import { NoSuchThing } from '@cardstack/choreo/film' in guide.md now fails pnpm lint:realm. The header comment and the gallery README both say design/ imports aren't checked.
| const withoutFences = (markdown) => | ||
| markdown.replace(/^(```|~~~)[^\n]*\n[\s\S]*?^\1[^\n]*$/gm, ''); | ||
|
|
||
| /** the ids GitHub (and the host's renderer) give a document's headings */ |
There was a problem hiding this comment.
[Claude Code 🤖] The slugs computed here match the host's, but the host renders heading ids with a user-content- prefix (gfmHeadingId({ prefix: 'user-content-' }) in runtime-common/marked-sync.ts). I can't find anything in host or base that maps a bare #goal fragment onto user-content-goal, the way GitHub's page script does. Do the 44 in-realm #heading links in docs/ and design/ actually land on choreo.boxel.site?
If they don't, that's a follow-up for the host's markdown rendering, not this PR. Either way, this comment should say these are GitHub's slugs and that the host prefixes them. Question; non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] No, as far as the code shows. The only mention of user-content in host, base and runtime-common is that gfmHeadingId call, and nothing reads location.hash or maps a bare #… link onto the prefixed id. I haven't tried it in a browser. That's now CS-13777, and the PR description lists it as a follow-up. The comment in 18842f5 says these are GitHub's slugs, and that the host renders them with the user-content- prefix.
| test('it lands', async function (assert) { | ||
| await render(<template><Card /></template>); | ||
| await animationsSettled(); | ||
| assert.strictEqual(bounds(find('.card')!).x, 120); |
There was a problem hiding this comment.
[Claude Code 🤖] bounds() returns { top, left, width, height } (Box in src/test-support/index.ts). So .x doesn't type-check, and at runtime it's undefined, so this assertion can never pass.
| assert.strictEqual(bounds(find('.card')!).x, 120); | |
| assert.strictEqual(bounds(find('.card')!).left, 120); |
Regression (new in this README); non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 18842f5: the example asserts .left now.
| | `setupMotion(hooks)` | resets what outlives an owner (the projection root, the layout-loop guard, motion speed, and every registered reset) around each test, and fails a test that ran a layout loop | | ||
| | `animationsSettled(opts?)` | resolves when every motion element, layout animation and registered busy probe has stopped, and names what was still moving when it times out | | ||
| | `bounds(el)` / `shape(el)` / `boundsAndShape(el)` | rects relative to the test root; the cumulative 2×2 transform | | ||
| | `velocityOf(el, prop)` | how fast a value is moving right now | |
There was a problem hiding this comment.
[Claude Code 🤖] velocityOf takes only the element and is async: velocityOf(el): Promise<{ x, y }>, in px/s sampled across two frames. There is no prop argument, so this row should read velocityOf(el), and say that it's async and returns x/y. The row came over from the source README.
Regression (new in this README); non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 18842f5. The row now reads velocityOf(el) and says it's async and returns { x, y } in px/s, sampled across two frames. Nothing else in the docs, guides or skills uses the two-argument form.
| <div class='card' style={{this.accentStyle}} {{motion drag=true}}> | ||
|
|
||
| {{! ✓ Motion applies these itself, custom properties included }} | ||
| <div class='card' {{motion style=this.accentStyle drag=true}}> |
There was a problem hiding this comment.
[Claude Code 🤖] The modifier's style is a Record<string, unknown> (MotionProps.style in src/node.ts). But the ✗ line binds the same this.accentStyle to the style attribute, which takes a string. Read together, the pair says "move the same value into the modifier", and a string there won't type-check. A distinct name with its shape spelled out would fix it, e.g. {{motion style=this.accentVars drag=true}} with {{! accentVars = { '--accent': … } }}.
Regression (new in this README); non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 18842f5. The ✗ line says accentStyle is a string, and the ✓ line binds a separate accentVars, with its object shape ({ '--accent': … }) given in the comment above it.
| ```gts | ||
| import { Film, IframePicture } from '@cardstack/choreo/film'; | ||
|
|
||
| <Film @name='sagrada' @seek='exact' @clock={{CLOCK}} @over='everything'> |
There was a problem hiding this comment.
[Claude Code 🤖] @menuTitle is a required argument of <Film> (FilmSignature in src/film/film.gts), and this example leaves it out, so the snippet won't type-check if copied. Add @menuTitle='Sagrada Família', or a … in the tag like the <f.Shot … > line has.
Pre-existing in the source README, now in the published reference; non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 18842f5: the example passes @menuTitle='Sagrada Família'. The two <Film> examples in realm/docs/film.md already mark the elided arguments with …, so I left them alone.
check-realm follows `export *`, so imports from @cardstack/choreo/film are checked; skips the design records' imports, which sketch APIs never built; and checks the heading a choreo.boxel.site or monorepo URL's fragment names. The glimmer-motion README's testing example, velocityOf row and first rule, and the choreo README's <Film> example now match the source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018tqpxGBVBUCe7nACwttGag
|
[Claude Code 🤖] The follow-up in the review summary (cross-README fragments) is done in this PR rather than deferred, in 18842f5. The site-URL and repo-link checks now check the |
This branch has not been deployed
1 participant
Background and Goal
CS-13305. The Choreo import (CS-13281) left the source repo's
docs/,notes/, README and AGENTS.md inpackages/choreo-gallery/as a holding location. This PR gives each of them a home:It also points every reference to those files at the new locations. The holding
packages/choreo-gallery/docs/andnotes/directories are gone.Where to start
packages/choreo/README.mdis now the main Choreo reference, built from the source README's Choreo half. Its API section was checked againstChoreoContextand<Choreo>'s signature. The source README listed ac.claimedquery that doesn't exist (it'sc.counterpart) and left out six steps.packages/glimmer-motion/README.mdgains "Three rules React does not need" (from the source AGENTS.md), a Testing section and "How it was made". Its helper list no longer names astarthelper, which isn't exported.packages/choreo-player/README.mdtakes indocs/choreo-player.md, except that doc's release checklist.packages/choreo-gallery/realm/docs/holds the eleven reader docs, plus aREADME.mdindex.realm/design/holdsnotes/and the rest of the design history.packages/choreo-gallery/tools/check-realm.mjs(run bypnpm lint:realm) gains checks over the realm docs, the test-app guides and the package READMEs:#headinganchors resolve. The heading ids are worked out GitHub-style, the way the host'smarkedrenderer makes them.export *re-exports. The imports indesign/aren't checked, because those notes sketch APIs that were never built.choreo.boxel.siteURL names a file in the realm.github.com/cardstack/boxel/{blob,tree}/main/…link names a path that exists.#fragmentnames a heading in that file, or in the directory's README.design/links into the archivedcardstack/choreorepo.The rest of the diff is mechanical: moves, and references rewritten to the new paths. That covers about 75 code comments, the choreo skills and the lint configs. In the test-app guides it covers about 130 links that pointed into the archived repo. The API-inventory guide's 351 source links were rebuilt from
api-inventory.json, because they still placed Choreo and Film underglimmer-motion/src.Key decisions and non-obvious mechanics
https://choreo.boxel.site/docs/<x>.md, source links to the monorepo on GitHub. The header images come fromchoreo.boxel.siteand won't render until CS-13304 publishes the site. CS-13305 stays open until they're confirmed. Until then, the lint check confirms that each URL names a file in the realm.notes/*.mdfor their reasoning, and these links stay resolvable both in the repo and on the site.realm/design/README.mdsays the notes were written in Choreo's own repo, so the paths they mention are that repo's layout. Historical paths inside the notes are left as they were written..gts.mdfiles. They don't compile, and the realm would index a.gtsfile as a module. Each sketch is fenced verbatim, and.prettierignorekeeps prettier from reformatting it.api-inventory.json→choreo-gallery/tools/, as the realm check's input.guide-topics.json→ beside the test-app guides it indexes, so the guides port (CS-13298 PR C) takes it along with them.tograph.mjs→choreo-film-app/tools/, the app whose scores it generated. It got a lint pass there; its output is byte-identical for both films.realm/licenses/, so it ships with the site.documentation-site.mdis kept inrealm/design/, not retired. It describes how the test-app guides are registered and tuned, and the guides port (CS-13298 PR C) needs that.source-repo/README.md,source-repo/AGENTS.mdand the old gallery deploy doc are retired; the READMEs and the.claude/skillscover their content.Follow-up
user-content-prefix, and nothing maps a bare#headinglink onto them. So in-realm heading links probably don't scroll on choreo.boxel.site. The realm check confirms each fragment names a real heading. Whether the host scrolls to it is the host's job.