chore(templates): retire the unreachable feature system - #3939
Conversation
The feature system was live in the type system and dead everywhere else,
in two independent ways.
**Nothing could request a feature.** `--with`/`-w` was registered as an
array flag in `cli/shared/args.ts` and read by no code:
`handleInitCommand` never touched `args.with`, and `InitOptions.features`
was populated by no non-test caller. Every remaining caller passed
`features: []` literally -- the type checker found three more once the
field was removed. `templates/integrations/_base/files/SETUP.md` shipped
`veryfront init my-app --with ai …` into every integration scaffold, so
the one place a user was told to use the flag was advertising something
inert.
**Nothing could load a feature's files.** All six features shipped a
`files/` tree -- 29 files -- that has never been scaffolded.
`feature-loader.ts` read them through `loadTemplateFromDirectory` with an
absolute path, but that function resolves its argument as a key into the
compressed manifest, and the generator emits no feature keys. Templates
moved to the manifest; features did not move with them.
Deleting rather than finishing, because the files are not merely
unwired:
- `ai` overwrites `agents/assistant.ts` on the ai-agent template, swapping
the calculator assistant for a weather one while the shipped
`evals/assistant.eval.ts` still gates on `calledTool("calculator")`.
- `auth` overwrites `app/dashboard/page.tsx`, which is saas-starter's
product surface.
- `redis` is not Redis: `lib/redis.ts` has no client and no network I/O,
and its tips instruct the user to start a server it never contacts.
- `workflows` references six tool ids that are defined nowhere.
- `blob` is superseded by `veryfront/workflow/blob`.
What survived was worth less than the plumbing that carried it. No
feature declared dependencies, `configMerge` was read by nothing, and
with `files/` gone the whole system contributed tips plus six envVars --
for features no user could select. So the plumbing goes with them:
`feature-loader.ts`, `FeatureName`/`FeatureConfig`/`ResolvedFeature`, the
`--with` flag, `InitOptions.features`, and the #3786 detector that
existed only to report this condition.
Kept, because they are load-bearing beyond features:
- `mergeFiles` moves to `templates/loader.ts` -- integrations and the
generated AGENTS.md/.env.example overlay use it.
- `withMdxExtension` stays and is now purely file-driven. The `mdx`
feature was the path that motivated it, but the rule it enforces
outlives that path: a template that ships `.mdx` gets the extension
declared whether or not its config remembers to say so. Its test is
retargeted at `minimal`, which is that case today.
`featureTips` is renamed `setupTips`, which is what it now carries.
`validateOrThrow`'s kind/callback indirection collapses into
`validateIntegrationsOrThrow` now that only one kind remains.
Closes #3797
Closes #3800
|
Warning Review limit reached
Next review available in: 26 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (61)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dec789267
ℹ️ 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".
Removing the feature plumbing from cli/shared/project-creation.ts shifts the source line numbers the generated reference links to. Regenerated with the pinned toolchain (deno 2.7.7, per .tool-versions) rather than hand-edited, so the pins match what CI produces.
`test:scripts` enumerates its test files by hand, so deleting feature-files-detection.test.ts without editing the task left `deno task test:scripts` (and `verify`, which calls it) failing with a missing-module error before any script test ran. Caught in review on #3939.
|
Both review findings addressed at P1 ( P2 ( @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Blast-radius check on the breaking change, before merging. I swept the platform monorepo for consumers of the public
Net: the break is real for a hypothetical external JSR consumer that passed CI is green across the board on |
Closes #3797. Closes #3800.
Both issues asked for a decision, not a half-state. This takes the delete branch of each, and the two turn out to be the same decision.
The feature system was dead in two independent ways
Nothing could request a feature.
--with/-wwas registered as an array flag incli/shared/args.tsand read by no code.handleInitCommandnever touchedargs.with;InitOptions.featureswas populated by no non-test caller. Every remaining caller passedfeatures: []literally — the type checker surfaced three more (cli/app/operations/project-creation.ts,cli/commands/demo/demo.ts,cli/mcp/tools/catalog-tools.ts) the moment the field was removed. Meanwhiletemplates/integrations/_base/files/SETUP.mdshippedveryfront init my-app --with ai …into every integration scaffold, so the one place a user was told to use the flag was advertising something inert.Nothing could load a feature's files. All six features shipped a
files/tree — 29 files — never scaffolded.feature-loader.tsread them vialoadTemplateFromDirectorywith an absolute path, but that function resolves its argument as a key into the compressed manifest, and the generator emits no feature keys. Templates migrated to the manifest; features never moved with them.Why delete rather than finish
The files are not merely unwired — five of six are wrong (per the per-feature assessment in #3797):
aiagents/assistant.tson ai-agent, swapping the calculator assistant for a weather one while the shippedevals/assistant.eval.tsstill gates oncalledTool("calculator")authapp/dashboard/page.tsx— saas-starter's product surfaceredislib/redis.tshas no client and no network I/O; its tips tell the user to start a server it never contactsworkflowsblobveryfront/workflow/blobmdxAnd what survived was worth less than the plumbing carrying it. Verified on this branch's base: no feature declares dependencies,
configMergeis read by nothing, and withfiles/gone the entire system contributed tips plus six envVars — for features no user could select. Wiring--withto reach that would also have newly exposedredis's andblob's tips, which are false.So the plumbing goes with the files:
feature-loader.ts,FeatureName/FeatureConfig/ResolvedFeature,--with/-w,InitOptions.features, and the #3786 detector that existed only to report this condition.MaterializeScaffoldRequest.featuresveryfront/scaffoldis a public export, so removing the optionalfeaturesfield fromMaterializeScaffoldRequestis a deliberate breaking change. Raised in review; recording it rather than leaving it implicit.Who is affected: JSR/Deno consumers of
materializeScaffoldonly. npm consumers were never affected —build-npm-dnt.ts:306embeds templates through the compressed manifest and copies nofeature.json, soloadFeatureConfigread a path absent from the package. JSR shipped it because the top-levelexcludeliststemplates/files/andtemplates/integrations/but nottemplates/features/.What they lose, probed through the public entrypoint on the base commit rather than inferred from types:
envVars, the
.env/.env.examplepair those envVars cause to be emitted (that is the+2), and tips. No featurefiles/were ever scaffolded — consistent with the manifest-key analysis above.Why it is not preserved behind a deprecated field. What survived is actively misleading, not merely inert. The
authtips it emitted:None of those routes are scaffolded, and it wrote
JWT_SECRETinto.env.examplefor auth code that does not exist.redisis the same shape. A deprecated field would keep telling consumers their project has a login page it does not have — strictly worse than the silent no-op #3800 was filed about, and both issues close on "not a half-state".AGENTS.md permits this: "Preserve public API compatibility unless the task explicitly asks for a breaking change." #3797's acceptance is "the directories deleted and the detector removed"; #3800's is "
InitOptions.featuresis removed".Migration: drop the field. It has no replacement because it had no working behaviour.
Kept, because they outlive features
mergeFilesmoves totemplates/loader.ts. Integrations and the generatedAGENTS.md/.env.exampleoverlay depend on it.withMdxExtensionstays, now purely file-driven. Themdxfeature was the path that motivated it (fix(npm): make ext-content-mdx an optional peer, not a runtime dependency #3783 review), but the rule outlives that path: a template that ships.mdxgets@veryfront/ext-content-mdxdeclared whether or not its config remembers to. Its test is retargeted atminimal, the case that exists today, with a guard asserting the template still ships an.mdxfile so the test can't go vacuous silently.Incidental cleanups the deletion forced
featureTips→setupTips, which is what it now carries (integration tips only).validateOrThrow's kind/callback indirection collapses intovalidateIntegrationsOrThrow— it existed to serve two kinds, and one is gone.generated-artifact-checks.test.tsno longer creates atemplates/featuresfixture dir the generator no longer reads.deno.json'stest:scriptsno longer lists the deletedfeature-files-detection.test.ts. That task enumerates its files by hand, so the stale path madedeno task test:scripts(andverify) fail with a missing-module error before any script test ran. Caught in review.Verification
All with pinned Deno 2.7.7 (
.tool-versions):deno test templates/ cli/shared/ cli/commands/init/— 69 passed, 0 failed (652 steps), including the scaffold-quality, scaffold-parity and init integration suites.deno task test:scripts— 142 passed (470 steps), 1 failed. That one (npm supply-chain policy … DNT Deno shim) reproduces identically on an untouched baseline worktree, so it is pre-existing.deno task fmt:check,deno lint(5068 files),deno check src/index.ts cli/main.ts— all clean.generate-templates-manifest.ts --check— current. The manifest diff is exactly one line: the corrected SETUP.md command.deno test cli/ tests/docs/guide-content.test.ts— 329 passed, 5 step failures. None are regressions: 3 (deploy,schedule,up) reproduce identically on an unmodified worktree and are network-dependent; the other 2 (demo --auto help,initializeGitRepo) pass in isolation on this branch and are parallel-run cwd interference. Thedemo.tsdiff is a single removed line inside acreateProjectcall and cannot affect--helpoutput.Deliberately not included
templates/features/ai/files/prompts/assistant.tsis deleted with the rest. #3797 flagged it as the onlyprompts/+promptRegistryexample anywhere intemplates/, worth relocating totemplates/files/ai-agent/prompts/. That is a separate, additive change — recovering it from history is a one-liner if wanted, and it should land where the scaffold-quality gate covers it rather than riding along with a deletion.