fix(chat): keep the eval-based dynamic import out of client bundles - #3421
Merged
Conversation
Project pages are served with `script-src 'self' 'nonce-...' https://esm.sh`. That has no 'unsafe-eval', so the `new Function("specifier", ...)` in platform/compat/dynamic-import.ts throws EvalError in the browser. Hydration dies before first paint and the page sits on its skeleton loaders forever. Two barrel imports were pulling that server-only helper into the `veryfront/chat` client entry. Both need only `getHostEnv`, but importing the `platform/compat/process.ts` barrel also drags in its `runCommand` re-export from process/command.ts, which imports dynamic-import.ts: chat/index.ts -> chat/stream-watchdog.ts -> agent/streaming/lifecycle/watchdog-compat-adapter.ts -> platform/compat/process.ts -> process/command.ts chat/index.ts -> react/components/chat/agent-card.tsx -> ... -> react/components/chat/missing-renderer-warning.ts -> platform/environment.ts -> platform/compat/process.ts -> process/command.ts Both now import `platform/compat/process/env.ts` directly, which is where getHostEnv is defined and which is already the convention elsewhere (see security/sandbox/deno-sandbox.ts, utils/logger/logger.ts). The `new Function` itself is deliberately left alone. It is load-bearing on the server: it keeps the import non-literal so neither `deno compile` nor the release-asset rewriter traces into the specifier. Removing it is not an option — the release-asset builder then rejects the module outright with "Release module contains a non-literal dynamic import". The fix is to keep the eval and keep it off the client. Two regression guards, both confirmed red before the fix and green after: - scaffolded-project-build asserts no *published* JS asset contains `new Function`. This is the ground truth: before the fix, 5 of 7 scaffolds published chunk bfbed417dd16 — byte-identical to the asset failing in production — and every existing assertion in that file still passed. - dynamic-import.test walks value imports (not `import type`) from the six client entries in PLATFORM_UTILITY_PATHS and fails with the offending chain. This one found the second chain above, which manual tracing had missed. Verified: uploads per scaffold drop 269 -> 263, so the whole server-only subtree leaves the client bundle rather than just the one chunk.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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 (2)
📝 WalkthroughWalkthroughThe change replaces two barrel imports with direct environment imports. It adds client-bundle reachability tests and scaffold build assertions that reject JavaScript assets containing ChangesCSP-safe client bundles
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/platform/compat/dynamic-import.test.ts`:
- Around line 68-72: Update the candidate list in resolveSpecifier to include
the directory fallback `${base}/index.tsx` alongside the existing index.ts
option, so extensionless imports resolving to index.tsx return their resolved
candidate.
🪄 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: 5e51b032-7c62-4d57-a597-dd6224a98de0
📒 Files selected for processing (4)
src/agent/streaming/lifecycle/watchdog-compat-adapter.tssrc/platform/compat/dynamic-import.test.tssrc/platform/environment.tssrc/release-assets/scaffolded-project-build.test.ts
…rence Two `ci (lint)` tasks, both caught locally against the same chain CI runs: - lint:test-typecheck — regex capture groups are `string | undefined` under the repo's strict indexing, so guard the pushes instead of indexing raw. - docs:api-reference:check — the explanatory comment added to watchdog-compat-adapter.ts shifts its line numbers, and the generated reference pins source links by line. Regenerated with `deno task docs`; the chat.md diff is line-number movement only, no API surface change.
…ty guard
resolveSpecifier checked `${base}/index.ts` but not `${base}/index.tsx`, so an
extensionless value import resolving through a .tsx directory module returned
null and the traversal stopped there — a silent pass.
That matters here specifically: the guard walks React subtrees, and
src/react/components/chat/chat/index.tsx is a directory module inside
chat/index.ts, the one entry that actually leaked.
No live blind spot today (nothing currently reaches those three .tsx directory
modules through a relative or #veryfront extensionless import), so this closes
a false-negative class rather than fixing a miss. Guard still fails on the
pre-fix imports.
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
Project pages are served with
script-src 'self' 'nonce-...' https://esm.sh. That has no'unsafe-eval', so thenew Function("specifier", ...)inplatform/compat/dynamic-import.tsthrowsEvalErrorin the browser. Hydration dies before first paint and the page sits on its skeleton loaders forever.Observed on
support-agent-vkmo7d.production.veryfront.com:document.querySelectorAll('*').lengthis 61 and the page text isLoading messages....This is independent of #3420. That PR fixed release-asset 503 shedding, which was also breaking this page. Page loads that shed zero assets still died on this
EvalError, so both fixes are needed for the page to render.What changed
Two barrel imports pulled a server-only helper into the
veryfront/chatclient entry. Both need onlygetHostEnv, but importing theplatform/compat/process.tsbarrel also drags in itsrunCommandre-export fromprocess/command.ts, which importsdynamic-import.ts:Both now import
#veryfront/platform/compat/process/env.tsdirectly, wheregetHostEnvis defined. That is already the convention — seesecurity/sandbox/deno-sandbox.tsandutils/logger/logger.ts.The
new Functionis deliberately left aloneIt is load-bearing on the server: it keeps the import non-literal so neither
deno compilenor the release-asset rewriter traces into the specifier.I tried removing it first.
deno compiletolerates the bare form, but the release-asset builder does not:That broke
scaffolded-project-build.test.tsacross 5 scaffolds. The fix is to keep the eval and keep it off the client. The doc comment on that helper mentions onlydeno compile, which is what made this look safe — worth a follow-up to correct.Type of Change
Checklist
Two guards, both confirmed red before the fix and green after:
1. Shipped bytes (
scaffolded-project-build.test.ts) — asserts no published JS asset containsnew Function. This is ground truth, and it is the assertion that matters: the build succeeds either way, and every pre-existing assertion in that file passed while the eval was shipping.Measured by probing actual uploads:
bfbed417dd16)bfbed417dd16)bfbed417dd16)bfbed417dd16)bfbed417dd16)bfbed417dd16is byte-identical to the chunk failing in production. Uploads per scaffold drop 269 → 263, so the whole server-only subtree leaves the client bundle rather than just the one chunk.2. Import reachability (
dynamic-import.test.ts) — walks value imports (notimport type, which is erased) from the six client entries inPLATFORM_UTILITY_PATHSand fails with the offending chain.This guard earned its place immediately: it found the second chain above, which manual tracing missed because a BFS only reports the shortest path. Without it I would have shipped a fix that cut one chain and left the other.
Verification:
src/release-assets/15 passed / 220 steps;src/platform/ src/chat/ src/agent/streaming/307 passed / 3245 steps;deno check,deno lint,deno fmt --checkclean. Rebased onto540509c5(#3420 merged).Notes for review
chat/index.tsever leaked. The other five client entries are clean, andmdx/workflowonly appeared to leak under a regex that ignoredimport type.platform/compat/process.tsis a recurring hazard: any client-reachable module importing it inheritsrunCommand. The reachability test now catches that class of mistake rather than this one instance.Summary by CodeRabbit
Bug Fixes
Tests
Documentation