refactor(fork): the design-props policy gets one home - #81
Conversation
The primitive-props validation was implemented twice, byte-identical: once in the desktop resolver and once in the guest engine. Twenty lines of caps, JSX-shaped name pattern, children exclusion and control-character rules would drift the first time one side was fixed. Moved the policy to normalizeForkDesignProps in packages/shared, the only module graph the desktop preload and the web engine island both reach. The dual CALLS stay - the resolver cannot vouch for what the page later writes into data-t3-props, and the engine cannot vouch for the fiber - but the rules now have one implementation. The five-line predicate twins beside it (normalizeFilePath / readSourceFile, normalizeComponentName / readComponentName) stay duplicated on purpose: at that size a copy is cheaper than a dependency. Importing a workspace package into the sandboxed preload reintroduces the failure mode #79 just fixed, so alwaysBundle now covers @t3tools/* for that entry. Confirmed against the artifact: with the rule the only external is Electron's own synthesized require, and without it the preload gains require("@t3tools/shared/forkDesignProps"). The guard test pins both the bundling rule and the single implementation.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.github.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thermo-nuclear code quality review — do not approve.
The policy extraction is the right idea, but this PR stops halfway through the judo move. It adds a third “canonical” policy test suite while leaving the two old ones intact, then keeps identity MAX_* re-exports that exist only to feed those leftovers. The single-implementation guard is a brittle string tell that does not even match the new implementation’s cap check (MAX_DESIGN_PROPS). Fork placement into packages/shared may be defensible for dual module graphs, but not while the dedupe is incomplete and the already-over-1k guard file grows again.
Minimum to reconsider: delete the two leftover policy suites, drop dead MAX_* re-exports, replace the brittle guard tell, and stop growing forkDesignMode.test.ts past 1k (extract instead).
Sent by Cursor Automation: Thermo-nuclear PR review
…orts Review follow-up. Moving the policy to packages/shared without deleting the two suites that tested the copies left three assertions of the same rules — and the shared test's own docblock claimed the opposite. The desktop and web policy suites are gone; each boundary keeps only what is actually its own (describeResolvedSource props integration, parsePropsAttr and formatDesignProps). With those suites gone the MAX_PROPS / MAX_PROP_VALUE_LENGTH re-exports had zero consumers on either side, so they go too; the caps live with the policy that enforces them. The ownership guard no longer greps for one historical spelling of the loop condition, which the shared implementation had already renamed past. It asserts the outcome instead: callers import the shared module and declare no name pattern of their own, and a repo-wide walk pins normalizeForkDesignProps to exactly one definition. Verified the walk fails when a second copy is pasted back in.
NoahHendrickson
left a comment
NoahHendrickson
left a comment
There was a problem hiding this comment.
Automated review of the consolidation. The core of the PR checks out clean: the shared implementation is logic-identical to both removed twins (caps 12/64, same name and control-character patterns, children exclusion, null-on-empty), the removed MAX_PROPS/MAX_PROP_VALUE_LENGTH exports have zero remaining references, resolution of the new subpath was verified across web Vite, the engine-island esbuild bundle, the desktop pack entries, and all three vitest projects, and the new shared suite covers everything the deleted suites did plus DEL/NUL/tab and aria- cases.
Six findings, ranked, as inline comments — three substantive:
- The new
alwaysBundleguard assertion is vacuous: the identical string already exists in thesrc/main.tspack entry, so the preview-pick entry it was added to protect is not actually pinned. - The
packages/shared/package.jsonexports edit is an inline edit of an upstream-owned file that is in neitherfiles:norwatch:of.fork/customizations.yaml, and no guard asserts the export entry — invisible to every drift layer. - The desktop boundary lost its only runtime test that hostile props get filtered; the web boundary kept one, and the replacement enforcement on this side is textual only.
Plus three minor: a Windows path-separator break in the new walk guard, the walk's coverage overclaim (three roots, not repo-wide), and dead alias re-exports on the desktop side.
One note without a diff anchor: the watch: comment for apps/desktop/vite.config.ts in .fork/customizations.yaml (~line 2194) still records the invariant as "must keep listing every package the resolver imports (react-grab AND bippy)". This PR extends that exact list to @t3tools/*, so the recorded intent a sync-conflict resolver would port is now incomplete — worth extending the comment while in there.
Two of the guards added here did not actually guard. The alwaysBundle assertion ran against the whole vite config, and the src/main.ts pack entry already carries an identical `@t3tools/*` clause — so the preview-pick entry could lose its own and the guard would stay green, with the regression surfacing only in packaged builds. It now slices the config to the preview-pick entry before asserting, which future-proofs the react-grab and bippy pins too. The single-definition walk covered three hand-picked roots, so a third copy in packages/client-runtime, apps/mobile or apps/server would pass unnoticed despite the test claiming repo-wide. It also read gitignored scratch files and hard-coded a path separator that breaks on Windows. Replaced with the git grep idiom this directory already uses: genuinely repo-wide, tracked files only, no walk. The exports map in packages/shared/package.json was an unfenced edit of an upstream-owned JSON file with no manifest entry — an upstream sync taking their version of that block would drop the subpath with no drift signal. Added under watch: with the strict-JSON note, plus a guard that parses the exports map. Deleting the desktop policy suite also took with it that boundary's only runtime proof that hostile props get filtered; what survived only passed valid bags through. Added one boundary test that feeds a hostile bag through describeResolvedSource - not the policy rules again, just that this side still routes props through them. Dropped the desktop-local ResolvedProps / normalizeResolvedProps aliases: nothing outside that file consumed either. The web side keeps its DesignProps alias because the vendored request.ts genuinely imports it. Each guard verified by reverting what it protects: the sliced bundling pin, the exports-map pin, the repo-wide walk (via a copy planted in client-runtime) and the hostile-props test all fail as intended.
1 participant
Follow-up to the review on #79. The primitive-props validation was implemented twice, byte-identical — once in the desktop resolver (
normalizeResolvedProps) and once in the guest engine (readDesignProps). Twenty lines of caps, JSX-shaped name pattern,childrenexclusion and control-character rules, in two places, would drift the first time one side was "fixed".What changed
The policy moved to
normalizeForkDesignPropsinpackages/shared/src/forkDesignProps.ts. Both call sites are now thin aliases that keep their local names, so nothing downstream moved.The dual calls stay — that was never the redundancy worth removing. The resolver cannot vouch for what the page later writes into
data-t3-props, and the engine cannot vouch for the fiber, so each boundary still validates independently. What is now singular is the rules.The five-line predicate twins beside it (
normalizeFilePath/readSourceFile,normalizeComponentName/readComponentName) stay duplicated on purpose. That is not an inconsistency — at five lines a copy is cheaper than a dependency; at twenty lines of policy it is not.The bundling trap this walked into
packages/sharedis the only module graph the desktop preload and the web engine island both reach — but importing a workspace package into the sandboxed preload reintroduces exactly the failure #79 just fixed. Verified against the artifact rather than assumed:preview-pick-preload.cjsrequire("@t3tools/shared/forkDesignProps"),require("electron")require("electron")So
alwaysBundlefor that entry now covers@t3tools/*. The guard test pins the bundling rule and asserts neither call site re-declares the policy. Bundle size went slightly down (131.01 → 130.92 kB) from the dedup.Tradeoff worth flagging
This puts one fork-owned file inside an upstream package, which the fork architecture otherwise avoids. There is no precedent for a fork-created file in
packages/shared— the two entries already in the manifest are upstream files the fork edits. It is namedforkDesignPropsso upstream cannot collide with it, and the manifest records why it lives there instead ofapps/web/src/custom/**. If you would rather keep fork code fully contained, the alternative is leaving the duplication in place; say the word and I will revert.I also did not take the other declined finding (isolating bippy behind a preload-local adapter) — it relocates the DevTools-hook side effect without removing it, and the real defect there was bundling, already fixed.
Verification
vp test run src/custom/designMode src/__fork_guards__fromapps/web→ 46 files / 395 tests pass (includes the engine-bundle guard, proving esbuild inlines the shared module into the injected IIFE)vp test run src/forkDesignProps.test.tsfrompackages/shared→ 6 passvp test run src/previewfromapps/desktop→ 9 files / 107 tests passtypecheckclean on shared, web and desktop;vp fmt --checkcleanvp pack+ artifact grep, both directions, as aboveModel: Claude Opus 5 (1M context), harness: Claude Code.