chore: gate formatting and close inert checks in remix_cli and remix_agent - #208
Open
leoafarias wants to merge 14 commits into
Open
chore: gate formatting and close inert checks in remix_cli and remix_agent#208leoafarias wants to merge 14 commits into
leoafarias wants to merge 14 commits into
Conversation
Nothing verified Dart formatting. The shared dart-actions workflow runs an analyzer step but no formatter, and no melos script checked it, so two committed test files had drifted. Adds `format:check`, plus an `analyze` script so a local `melos run ci` reports what a pull request would. Seven default registry templates had drifted too, and are reformatted here. That gate cannot see them: they are `.tmpl` files, so `dart format` never reads them, and `add` formats the tree it writes, so an unformatted template cannot reach a consumer either. Nothing was left that could notice. Fortal's templates are derived from formatted source and were already clean. Extends check_dependency_constraints.dart to the registry's own pub constraints. `melos bootstrap` never rewrites them and the pubspec walk could not see them, leaving 196 declarations that nothing compared to the workspace. check_version_alignment.dart keeps sole ownership of the `remix` floor. Replaces the remix_agent barrel assertion, which listed six paths that no longer exist and therefore could not fail, with a set comparison that also rejects an added export. Pins bundledPresets to the preset trees on disk, requires a worksheet per component, and corrects the transcript worksheet's followThreshold to the 48 the code ships. Co-authored-by: Cursor <cursoragent@cursor.com>
…jects AgentExecution rendered its single child inside a whole AgentTranscript, which installs Arrow/Page/Home/End shortcuts and a Semantics container. Nested in a host transcript the inner list shrink-wraps to a zero scroll extent, but its action still consumed those intents, so focus landing in an execution card silently stopped the outer transcript from scrolling; the inner `busy` value also announced the status a second time. The output now uses AgentLiveEdgeScrollView, the primitive plan and activity already use. The composer disposed a superseded owned controller or focus node inside didUpdateWidget, while the child RemixTextArea still held it, so detaching touched a disposed object. Both swaps now defer to a post-frame callback, matching the transcript's scroll-controller swap. Co-authored-by: Cursor <cursoragent@cursor.com>
…lean `format:check` covers Dart sources, but registry templates are `.dart.tmpl` and escape it. No consumer is affected either way, since `add` formats the tree it writes, so installed source is formatted regardless of what the template looked like. What this buys is that the committed templates stay diffable against each other instead of differing by pure whitespace. Every template was already clean when this was written, so it guards against future drift rather than fixing anything observed. It is a test rather than a tool under `tool/` so that it uses the real TemplateRenderer and takes both prefixes from ProjectConfig, rather than carrying a third private copy of the substitution rules that could drift from what `add` actually writes. The barrel stays untouched: it documents that `lib/src` is deliberately unexported, and `test/registry_test.dart` already reaches internals this way. Scoped to the hand-authored preset. `fortal` is generated from remix_fortal's formatted source through a byte-identical round-trip assertion, so it is formatter-clean by construction and `open-code:fortal:check` is its real gate; a wider version confirmed all 48 Fortal templates were already clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Nothing outside `lib/ui/` imports this app's installed mirror -- the playground's own previews render remix_fortal -- so the source `remix add` copies into every consumer app was only ever analyzed, never built. It was also the only workspace app with no test directory, while apps/dashboard has seven files. melos `test:flutter` selects on `dirExists: test`, so adding one wires it into ci with no configuration. Covers the theme contract and the sidebar layout, the one default component that opens a route, owns a breakpoint, and has a controlled/uncontrolled contract. The theme test deliberately asserts no token count: the open_code fixture owns that pin for a freshly generated consumer, and a second copy would mean two places to edit per added token. It does pin the indigo customization, because check_open_code_dogfood.dart records that as a deliberate edit and therefore tolerates any divergence -- it cannot notice a reinstall reverting it. The sheet is matched by its barrier label, not by ModalBarrier: MaterialApp's own home route always has one, so the bare type finder reports a sheet that is not there. Changing the breakpoint default fails three of the four. Co-authored-by: Cursor <cursoragent@cursor.com>
…ases `@ui/` was spelled four times across two files, twice as a bare `substring(4)` whose 4 had to be read back to the literal to be understood. One `uiTargetPrefix` constant now carries the spelling and its length, next to `bundledPresets` where the target format is validated. `Installer.add` was 247 lines that mixed resolving an install with performing one, so the preflight ordering -- the property that makes `--dry-run` and `--diff` trustworthy -- was something a reader had to reconstruct. It is now `_planAdd`, which reads and validates and writes nothing, and `_install`, which carries out the resulting `_InstallPlan`. `add` is 42 lines and its control flow is unchanged: print the plan, return on dry-run, print the diff, otherwise install. `_InstallPlan` is deliberately one value handed forward, so a later step cannot re-read the project and act on a different answer than the one already printed. `_install` names its fields as locals so the moved steps read as they did before, which keeps this reviewable as a move rather than a rewrite. All four load-bearing comments are preserved, including the build-filter note explaining why already-installed adapters stay in the focused build. Co-authored-by: Cursor <cursoragent@cursor.com>
…cator AgentStyleBuilder duplicated RemixStyleSpecBuilder: the same either/or between a fluent style and a resolved styleSpec, down to forwarding the same optional controller. Rather than keep the copy in step, it is deleted and RemixStyleSpecBuilder is exported from packages/remix, which is what all 20-odd Remix components already use. Separately, the transcript never passed a controller to it, and `focused` has no other source: Agent's slots resolve above any Naked control, so the `focus-visible` state the transcript worksheet documents could never activate. The transcript now publishes its focus through a WidgetStatesController, and the new test fails without it. The pointer-driven states still do not resolve on that slot and did not before either -- a host's `onHovered` on AgentTranscriptSpec.viewport has never had an effect -- so nothing there changed. Also collapses `_indicator`, which was the same body in five components differing only by the spec type: 67 lines to 35. The builder parameter stays per-component on purpose, since a permission card discloses details and an answer discloses sources. The related latched-flag duplication is deliberately left alone: those six surfaces expose different public names and activity's getter is not even the same expression, so sharing it would need an adapter per component to save nothing. Co-authored-by: Cursor <cursoragent@cursor.com>
Each of these asserted something the code does not do, which is worse than saying nothing: a reader trusts them over the code. The transcript claimed Mix keeps tracking pointer states while it publishes focus. Mix does the opposite — `StyleBuilder` gates its pointer detector on `controller == null`, so supplying one suppresses it. But the conclusion that this cost the viewport its hover is also wrong: a host `onHovered` on `AgentTranscriptSpec.viewport` resolves no differently before this branch than after. Checked by running an `onHovered` expectation against the pre-branch transcript, which passed no controller; it fails there identically. So there is no regression to fix, and the comment now records what the controller does and does not carry, plus who would own hover if anything ever asked for it. The playground theme test claimed `check_open_code_dogfood.dart` tolerates any divergence and cannot notice a reinstall. It can: `!edited && reason != null` fails when a declared customization matches the template again. What it cannot see is *which* edit, since any custom color satisfies it, so what this test adds is the values themselves and the tie between the ring and the primary. The transcript focus test repeated the first claim and loses that clause. Co-authored-by: Cursor <cursoragent@cursor.com>
Extracting `agentDisclosureIndicator` put it between the glyph-set dartdoc and the enum that dartdoc described, so the paragraph about a set of glyphs became the documentation for a widget factory and `AgentFunctionalGlyphKind` was left with none. The function moves above the paragraph, which restores both. `public_api_test.dart` names the files under `src/style/` that stay out of the barrel by omission, and still listed `style_builder.dart` after it was deleted. A pin that names a file the tree no longer has cannot be trusted to be checking what it says. `registry_test.dart` held the last `substring(4)` on a registry target, which `uiTargetPrefix` was introduced to retire; the production sites already use it. Two comments narrated this branch rather than the code. `_install`'s said its locals read "as they did when this was one method", which tells a reader nothing once the branch is merged, so it now states the invariant that earns them: nothing past that point re-reads the project, so no step can act on a different answer than the one already printed. `format:check`'s counted the files that had drifted when it was added, which stops being true immediately. Co-authored-by: Cursor <cursoragent@cursor.com>
leoafarias
changed the title
`theme_data.dart.tmpl` requires twenty tokens: fourteen colors, the five `chart*` entries the chart item added in #207, and `radius`. Eleven sentences across the repo still said fifteen, the size before chart. Two of them are in `card.dart.tmpl` and `textfield.dart.tmpl`, so the registry was shipping the false claim into every consumer that runs `remix add card` or `remix add textfield` -- six installed files here carried it. Those mirrors are enforced copies, so correcting the templates and rerunning `--overwrite` moves them. Two sentences in CLEAN_SHEET.md needed more than the number. One said the growing catalog needed "no addition to the fifteen theme tokens", which chart falsifies; it now names the one time the vocabulary did move. The other said chart "resolves only the existing fifteen theme tokens", offering that as proof `remix add chart` stays compatible with a theme installed before chart existed. The opposite is true: chart resolves `chart1`-`chart5`, an older theme does not carry them, and authored files are preserved on normal runs, so such a theme needs them added by hand. That is now what it says. Nothing compared the prose to the constructor -- grepping `packages/`, `tool/`, and `test/` for any assertion of a token count returned nothing, so the claim went stale the moment #207 landed and nothing could notice. `registry_test.dart` now counts `required this.` in the theme template and requires both shipping templates to spell that number, and only that number. Reverted against the old `card.dart.tmpl` it fails with the count it expected. Also corrects `open-code:dogfood:check`'s description, which named only the playground; `_consumers` has held two entries since the Agent example joined. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`specs/components/transcript.yaml` recorded `followThreshold default 56` while every constructor shipped 48. It was caught by reading, not by CI: `public_api_test.dart` validates worksheets by filename set only, so their contents assert whatever they like. A general validator for worksheet measurements would be a parser built for one line -- across all eight worksheets this is the only measurement that names a code identifier with a value; the rest is prose like "stop square 12, fill inset 3/24". So the constant is pinned instead and the worksheet stays prose. The plan named two declaration sites; there are five. `AgentTranscript` has it on both constructors, and it is `AgentTranscript` the worksheet is about, so pinning only `live_edge.dart` and `activity.dart` would have left the drifting site unpinned. All five are asserted. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…itself The check decided divergence with `!stdout.contains(_clean)`, so anything the CLI printed that was not the exact sentence `No authored-source differences.` counted as an edited file. Rewording that sentence in `installer.dart` made every item report "has diverged from the template. Rerun `--overwrite`" -- 39 of the 40 here -- which is both false and useless: `--overwrite` cannot fix a change in the CLI's own output. `--diff` prints a preamble and then exactly one verdict, the sentinel or a real `git diff`, so both are recognizable. Matching a line rather than a substring also stops a diff hunk that happens to contain the sentence from reading as clean. When neither or both appear, the check now names that instead of guessing, and points at `installer.dart`. Verified by rewording the sentinel: 39 items report the new message and none report divergence, while `apps/playground/theme` -- the one item that really does diverge -- still classifies correctly against its `_customized` entry. A shared constant was the obvious fix and is not available. The tool lives in the root `remix_workspace` package, which declares no dependency on `remix_cli`; `remix_cli.dart` is a marker library that documents `lib/src/` as deliberately not re-exported; and `installer_test.dart`'s copy of the literal is a pin of the output contract, so importing the constant there would make the assertion tautological. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g Fortal The builder derived one bundled registry tree from analyzer-checked Dart source, and ten separate things in it were the literal word Fortal: the source and output paths, the `Fortal`/`fortal` substitution pair, the rejected path segment, the legal source placements, a hardcoded `theme` item keyed off `theme/theme.dart`, a hardcoded `icons` item copied from the default preset, the `componentNames.length + 2` collision guard, the directories import inference recognizes, the six packages it demands floors for, and the `sidebar_layout -> sidebar` dependency nothing imports. All ten move into `PresetSpec`, with `fortalPreset` as the single instance and the entrypoint unchanged. `FortalPresetBuilder` becomes `PresetBuilder` and now contains no Fortal knowledge: what remains of the word is `main`, which really is Fortal's entrypoint, the spec itself, doc comments using it as the example, and the header naming the tool inside generated output. `theme` and `icons` were two shapes hiding behind one name. A shared item is derived from a source directory and may be imported by components; a copied item is taken verbatim from the default preset and derived from nothing. Making them separate kinds is what lets a source package have several of the first and none of the second. Behavior is unchanged, and that is checkable rather than asserted: running the writer, not just `--check`, leaves `registry/fortal/` byte-identical -- 48 templates and registry.yaml, no diff. A second spec with a different word, different directories, no copied items, and its own forbidden self-import now derives in the tests, so "general" is verified rather than claimed; without it every test here would still be exercising the values the builder used to hardcode. The file keeps its name. It is still Fortal's entrypoint, and renaming it is worth doing when a second caller exists, not before. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ctional glyphs The Lucide catalog has 27,874 `IconData` constants and no `@staticIconProvider`, so importing it defeats icon tree shaking; that is what forced the hand-rolled codepoints in `_LucideGlyphs` and the exact pin on `lucide_icons_flutter`. `RemixIcons` carries the annotation, a preserved codepoint lock with per-glyph source hashes, and this repository's own notices file, so the 17 glyphs become plain `RemixIcons` references and the dependency is removed outright, with no compatibility shim. Four marks change shape while keeping their meaning, and are accepted as such: `error` -> `exclamationTriangle`, `tool` -> `code`, `loading` -> `update` (circular arrows rotate cleanly under `_LoadingGlyph`), and `permission` -> `lockClosed`. The other 13 map directly. The glyph test now asserts `RemixIcons` identities instead of raw codepoints, so it names the glyph it expects rather than a number that happened to match. The glyph migration was executed by a delegated Gemini 3.1 Pro agent from a written task; the README and provenance prose it produced was corrected here. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…writes
Track C derives registry templates from this package by substituting `Agent` ->
`{{typePrefix}}` and `agent` -> `{{valuePrefix}}`, and the builder's round-trip
assertion cannot catch a wrong substitution because it is symmetric: rewriting
`a long-running agent turn` into `a long-running ui turn` round-trips exactly
and still ships nonsense to the consumer.
Two doc comments used `agent` as a domain noun rather than as the family name.
They are reworded so every remaining occurrence in `lib/src` is an identifier,
a `ValueKey` string, or prose naming the component family - each of which is
meant to take the consumer's prefix. This is the authoring discipline
`remix_fortal` already follows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Review follow-ups for the recent
remix_cliandremix_agentwork. Most of this is verification that either did not exist or could not fail.Formatting was unverified. The shared
dart-actionsworkflow runs an analyzer step but no formatter, and no melos script checked it, so two committed test files had drifted. This addsformat:check, plus ananalyzescript so a localmelos run cireports what a pull request would. Seven default registry templates had drifted too, and nothing could have noticed:dart formatnever reads.tmplfiles, andaddformats the tree it writes, so an unformatted template cannot reach a consumer either. A new test renders every default template and compares the result to the formatter.Three guards could not fail. The
remix_agentbarrel assertion listed six paths that no longer exist, so it passed unconditionally; it is now a set comparison, which also rejects an added export.check_dependency_constraints.dartcould not reach the registry's own pub constraints, leaving 196 declarations that nothing compared against the workspace. The transcript worksheet documented afollowThresholdof 56 while the code ships 48.Two
remix_agentdefects. A transcript could nest inside itself, and a component disposed objects it did not own. Separately,focus-visibleon the transcript viewport had no source at all — Agent's slots resolve above any Naked control, so no ancestor state scope existed — and the transcript now publishes its focus through aWidgetStatesController. The pointer-driven states still do not resolve on that slot and did not before, so nothing there changed.The default preset had no test coverage. Nothing outside
apps/playground/lib/ui/imported the generated mirror, so the preset that consumers actually receive was only ever analyzed, never run.Two refactors round it out: the
@ui/target prefix is named rather than spelled as a baresubstring(4),addis split into a plan phase and an install phase, andAgentStyleBuilderis replaced byRemixStyleSpecBuilder— the builder all 20-odd Remix components already use — now exported frompackages/remix.Related Issues
None.
Validation
melos run ci— passed. 16 steps: ~4,400 tests across 11 packages, 769 files formatter-clean, both open-code fixture checks, plus the dogfood and Fortal parity gates.Checklist
Note: Updating the
pubspec.yamlandCHANGELOG.mdis not required. These are handled automatically during the release process.///).Breaking Change
Does this PR require users of the package to manually update their code?
AgentStyleBuilderlived undersrc/style/and was never exported from theremix_agentbarrel, so removing it is not consumer-visible. The one public API change is additive:packages/remixnow also exportsRemixStyleSpecBuilder.