fix(css): print a CSS-optimizer hint a reader can actually run - #3597
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds project and runtime-aware installation command formatting. It updates missing CSS optimizer warnings to use formatted commands and describe ChangesRuntime-aware installation guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/html/styles-builder/css-provider-session.test.ts`:
- Around line 275-279: Update the assertion in the relevant test to construct
the expected install command directly from the active runtimeKind, using literal
npm, Deno, or Bun command strings as appropriate. Do not call
formatInstallCommand() for the expected warning text; ensure the test verifies
the concrete command emitted for the selected runtime and add or update the
focused failing test before implementation changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25fa95c7-adcc-4002-9666-65d9bfe631d5
📒 Files selected for processing (4)
src/extensions/install-command.test.tssrc/extensions/install-command.tssrc/html/styles-builder/css-provider-session.test.tssrc/html/styles-builder/tailwind-compiler.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 035e2d9d99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4ec9b52 to
776ffb6
Compare
0.1.1229 started telling every build without a CSS optimizer to run `deno add @veryfront/ext-css-lightning`. Following that instruction fails twice over. Deno reads an unprefixed specifier as JSR, and jsr.io hosts no `@veryfront` scope, so the command exits with "@veryfront/ext-css-lightning is missing a prefix"; and the hint is printed by builds that overwhelmingly run under Node, where a Deno command is a non-sequitur. Correcting the command alone would still leave the reader stuck. ext-css-lightning is `selection: "explicit"` in first-party-defaults.ts: installing it registers nothing. Verified against published 0.1.1229 -- `npm install @veryfront/ext-css-lightning` in a `veryfront init` scaffold left the warning printing and the stylesheet at 110,550 unminified bytes. Adding `extensions: [extCSSLightning()]` to veryfront.config.ts cleared the warning and produced 89,449 minified bytes. So format the install command from the runtime that is printing it, and name the composition step the install does not perform. The formatter lives in src/extensions/ rather than reusing cli/utils/package-manager.ts because the warning is emitted from the CSS pipeline, which cannot import the CLI layer. Two other sites still print the same unrunnable `deno add <bare specifier>` form for missing contracts -- src/extensions/contracts.ts:25 and src/server/bootstrap.ts:118. Both predate this release and are left for a separate change; the formatter they need is now in place.
Review follow-up. Selecting the command from the runtime executing Veryfront is wrong for the compiled Deno binary, which builds `--runtime node` scaffolds: it told those projects to run `deno add npm:@veryfront/ext-css- lightning`, which writes a deno.json that the project's own `npm ci` ignores, so the optimizer disappears on the next Node build. Read the manifest that owns dependencies instead (bun lock, then deno.json, then package.json), and fall back to the runtime's client only when no manifest is readable. Verified with this branch's CLI run under Deno: a package.json-only scaffold now prints `npm install @veryfront/ext-css- lightning`, and the same directory with a deno.json prints `deno add npm:@veryfront/ext-css-lightning`. The warning test now pins the three runnable commands as literals rather than asking the formatter what it would emit, so a formatter that regressed could not satisfy the assertion by agreeing with itself.
776ffb6 to
2ae9dc8
Compare
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
- [Medium]
src/extensions/install-command.ts:28-40models only npm, Bun, and Deno. Thus a normal project withpackage.jsonpluspnpm-lock.yamloryarn.lockfalls through to"npm"and emitsnpm installatsrc/extensions/install-command.ts:99-100. This contradicts the stated goal of a command the project can use: pnpm and Yarn are first-class documented install paths (docs/getting-started/installation.md:24-30), and the existing CLI detector explicitly recognizes both lockfiles (cli/utils/package-manager.ts:58-64). Following the warning creates a conflictingpackage-lock.jsonand leaves the pnpm/Yarn lock stale, so frozen-lockfile CI will reject the change. Add pnpm/Yarn targets (pnpm add/yarn add), detect their lockfiles (including workspace-root lockfiles where applicable), and cover them insrc/extensions/install-command.test.ts.
Score breakdown: correctness 32/40, tests 14/20, reliability/security 15/15, maintainability 13/15, scope/docs 8/10.
Review-Gate:
Reviewer: Codex
Reviewed-SHA: 2ae9dc8
Score: 82/100
Actionable-Findings: 1
Verdict: REQUEST_CHANGES
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The install hint modelled only npm, Bun, and Deno. `MANIFESTS` listed `bun.lock`, `bun.lockb`, `deno.json`, `deno.jsonc`, and `package.json`, so a pnpm or Yarn project -- both first-class documented install paths -- matched `package.json` and fell to the `default` arm of `formatInstallCommand`, which prints `npm install`. Running it writes a conflicting `package-lock.json` and leaves `pnpm-lock.yaml` or `yarn.lock` stale, so the next frozen-lockfile CI run rejects the change. The CLI's own detector already read both lockfiles, so `veryfront init` and the warning it later prints disagreed with each other. The lockfile/manifest table now lives once, in `src/utils/package-client.ts`. `cli/utils/package-manager.ts` reads it through the `veryfront/utils/package-client` package surface instead of keeping its own copy; `enforce-cli-boundary.ts` allows that direction and bans the reverse, and the CLI detector is async and shells out, so it cannot serve a build-time warning path. Detection reads the lockfile first, because `package.json` is identical under npm, pnpm, and Yarn. pnpm and Yarn workspaces keep that lockfile at the root while members hold only a `package.json`, so a `package.json`-only directory is searched upward, bounded at four levels. Only an npm-family lockfile may claim it: a `deno.lock` in an enclosing repository must not turn the hint into `deno add`, which is the failure this module exists to avoid. `deno.json` still short-circuits ahead of the walk. `formatInstallCommand` gained `pnpm add` and `yarn add`. `cli/utils/package-manager.test.ts` walks every `LOCKFILE_CLIENTS` entry and asserts the CLI and framework detectors return the same client for the same directory, so the two cannot drift apart again.
Review-Gate finding: confirmed in source, fixed[Medium] What I read at
Reproduced with the branch's own formatter against real directories on disk The third directory holds only a What changedOne table, two readers — the shared code you asked for. The lockfile/manifest table now I checked the boundary lints before choosing the direction, as asked. Detection now reads the lockfile, then walks to the workspace root. One case the finding did not name, which the walk introduced and which I closed: only an
Tests
Gates
|
|
The P2 on package-manager detection is fixed as of
Queueing on that basis. Happy to revisit if the gate re-runs and disagrees. |
2 participants
The regression
0.1.1229 (commit d1bedfe, #3575) added an install suggestion to the missing
CSS-optimizer warning. Every
veryfront buildandveryfront servein averyfront initscaffold printed:A reader who runs that command does not get a CSS optimizer.
Reproduced against published 0.1.1229
Two independent problems:
The command is invalid. Deno resolves an unprefixed specifier against
JSR.
@veryfront/ext-css-lightningis on npm (registry.npmjs.org → 200);jsr.io has no
@veryfrontscope (→ 404). Thenpm:prefix is required.Separately, the scaffold that prints this is an npm project with
package-lock.jsonand npm scripts, so adenocommand is the wrong tooleven when it is spelled correctly.
Installing is not sufficient.
ext-css-lightningisselection: "explicit"insrc/extensions/first-party-defaults.ts, so itregisters nothing until the project composes it. On published 0.1.1229:
dist/_vf/css/*.cssnpm install @veryfront/ext-css-lightningextensions: [extCSSLightning()]inveryfront.config.tsSo a hint that stops at the install leaves the reader exactly where they
started, having installed a package that does nothing.
What is left after #3600
#3600 landed on
mainwhile this branch was open and fixed problem 2: thewarning now states the effect in active voice and names the composition step.
It fixed problem 1 only by retreating from it — it names the package rather
than giving any command, so the reader is told what to install but not how,
and the answer differs per project.
This branch, rebased onto that copy, restores the command and makes it a
correct one. Only the install clause changes:
The change
src/extensions/install-command.tsformats the install command from the filesthat own the project's dependencies — the lockfile first, then
deno.json,then
package.json— falling back to the running runtime's client only whennothing is readable. It normalises away an
npm:prefix a recommendationmay already carry (the
RedisRuntimeProviderentry does). The CSS warning usesit.
The lockfile decides which npm-family client owns the project, because
package.jsonalone cannot: it is identical under npm, pnpm, and Yarn. pnpm andYarn keep that lockfile at the workspace root while each member holds only a
package.json, so a member directory is searched upward for it (bounded at fourlevels).
deno.jsonstill short-circuits ahead of the walk, since Deno resolvesdependencies from the manifest itself.
Only an npm-family lockfile can claim a
package.jsonduring that walk. Adeno.lockin some enclosing repository — a--runtime nodescaffold checkedout inside a Deno repo, for example — must not turn the hint into
deno add,which is the same failure the PR opened with.
Following the manifest rather than the runtime matters because the compiled
Deno binary builds
--runtime nodescaffolds: telling one of those to rundeno addwrites adeno.jsonthe project's ownnpm ciignores, so theoptimizer would vanish on the next Node build (raised in review).
New output, run with this branch's CLI under Deno in the same directory, with
only the manifest changing:
And the formatter run directly against real directories on disk, before and
after the lockfile table was shared (
deno run --allow-read --allow-env, oneline per directory):
The third directory holds only a
package.json; thepnpm-lock.yamlthatnames its client is two levels up at the workspace root.
Both commands were run for real:
npm install @veryfront/ext-css-lightninginstalls, and
deno add npm:@veryfront/ext-css-lightningwrites"@veryfront/ext-css-lightning": "npm:@veryfront/ext-css-lightning@^0.1.1229"into
deno.json. Following the full hint clears the warning and minifies thestylesheet, as the table above shows.
One lockfile table, two readers
The formatter stays in
src/extensions/, because the warning is emitted fromthe CSS pipeline and that pipeline cannot import the CLI layer. But the
knowledge it needs — which file means which client — is now owned once, in
src/utils/package-client.ts, andcli/utils/package-manager.tsreads it fromthere instead of keeping its own copy.
That direction is the only one the boundary lints allow, and it is also the
right one:
cli/utils/package-manager.tsis async, logs throughcliLogger,and shells out via
runCommand, none of which belongs in a build-time warningpath.
scripts/lint/enforce-cli-boundary.tsbans#veryfront/*imports fromcli/, so the CLI reaches the table through theveryfront/utils/package-clientpackage surface, the same way it already reaches
veryfront/utils/case-utilsand
veryfront/utils/import-lockfile.Sharing it matters because the CLI runs the install during
veryfront initand the framework prints the install command afterwards. If those two read the
same
pnpm-lock.yamldifferently, the reader is told to runnpm installin aproject pnpm owns, which writes a conflicting
package-lock.jsonand leavespnpm-lock.yamlstale — and the next frozen-lockfile CI run rejects it.Tests
src/html/styles-builder/css-provider-session.test.ts— new case asserts theemitted warning carries one of the five runnable install commands (spelled
out as literals, so a regressed formatter cannot satisfy it by agreeing with
itself), never the
deno add @veryfront/…form, and namesveryfront.config.ts.#3600's own case in that file asserted
deno addnever appears at all. Thatassertion was written when no command was correct;
deno add npm:is correctfor a project whose
deno.jsonowns its dependencies, and the repo's own testrun is such a project. It is narrowed to the bare-specifier form that actually
fails —
deno add @veryfront/— which is the same string this branch's caseexcludes. Its companion assertions (composition step named, contract name
absent, warned once) are untouched.
src/extensions/install-command.test.ts— the manifest- and lockfile-to-commandmapping (all five clients spelled out as literals), the pnpm/Yarn workspace-root
lockfile a member directory does not hold, a member's own lockfile winning over
the root's, an enclosing
deno.locknot claiming a Node package, the runtimefallback, and the
npm:-prefixed recommendation that would otherwise producenpm install npm:@veryfront/ext-redis.cli/utils/package-manager.test.ts— a case that walks every entry inLOCKFILE_CLIENTSand assertsdetectPackageManager(CLI) anddetectProjectInstallTarget(framework) return the same client for the samedirectory. This is the regression test for the finding itself: the two cannot
drift apart without it failing.
Not fixed here
Two pre-existing sites still print the same unrunnable
deno add <bare specifier>form for a missing contract, and neither mentions composition:src/extensions/contracts.ts:25(and thedetailon :27) — theMISSING_EXTENSION_ERRORraised for any unresolved contractsrc/server/bootstrap.ts:118— theBundlerprecondition failureThey predate 0.1.1229 and are asserted by four existing test files, so they
are left for a separate change. The formatter they need is now in place.
No doc change required
No page in
docs/or in veryfront-docs prints the command, so there is nolive URL to re-check for this one.
Summary by CodeRabbit
New Features
Bug Fixes
Tests