Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplace Webpack with Vite and Vite plugins, add a centralized reactive persisted app state and defaults, rewire editors/UI/runner to use signals and lazy-load Prettier, update CI workflows for single-file and Pages, and refresh docs and index bootstrap. ChangesBuild System & Architecture Modernization
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant index_html as index.html
participant main_ts as main.ts
participant appState
participant editor_ts as editor.ts
participant runner_ts as runner.ts
participant console_ts as console.ts
User->>Browser: Open app
Browser->>index_html: load
index_html->>main_ts: import module
main_ts->>appState: loadState()
appState->>localStorage: read snapshot
localStorage-->>appState: snapshot/null
appState-->>main_ts: signals hydrated
main_ts->>editor_ts: initializeEditors(html/css/js)
User->>editor_ts: edit code
editor_ts->>appState: update contentState
alt autoRun enabled
editor_ts->>runner_ts: debounced runCode()
runner_ts->>console_ts: post messages
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 8
♻️ Duplicate comments (1)
.github/workflows/single-file.yml (1)
13-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit permissions block to the workflow.
The workflow does not specify permissions for
GITHUB_TOKEN. As a security best practice, explicitly declare the minimum required permissions.🔒 Proposed fix to add permissions
jobs: deploy: runs-on: ubuntu-latest + permissions: + contents: read steps:As per coding guidelines, the CodeQL static analysis tool recommends setting an explicit permissions block with
contents: readas a minimal starting point.🤖 Prompt for 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. In @.github/workflows/single-file.yml around lines 13 - 41, The workflow lacks an explicit permissions block for GITHUB_TOKEN; add a minimal permissions declaration (e.g., permissions: contents: read) either at the workflow root or scoped to the deploy job to limit token access; update the top-level or the deploy job (job name "deploy") to include this permissions entry so the GITHUB_TOKEN has only the required read access.
🧹 Nitpick comments (1)
.github/workflows/single-file.yml (1)
22-22: 💤 Low valueInconsistent Node.js version across workflows.
This workflow uses Node.js 22 while
.github/workflows/static.ymluses Node.js 20. Unless there's a specific reason for this difference, consider standardizing on a single Node.js version for consistency and easier maintenance.🤖 Prompt for 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. In @.github/workflows/single-file.yml at line 22, The workflows are using different Node.js versions (node-version: "22" in this workflow vs node-version: "20" in the other workflow); pick one Node.js version to standardize on (e.g., "20" or "22") and update the node-version key in this workflow to match the version used in the other workflow (change node-version: "22" to the chosen version), and then run CI locally or via GitHub Actions to confirm no compatibility issues; also search for any other occurrences of node-version in workflow files and align them to the same value.
🤖 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 @.github/workflows/single-file.yml:
- Around line 8-10: The workflow's concurrency stanza currently uses group:
"pages", which collides with the static.yml workflow; update the concurrency
group value (the concurrency block and its group key) in the single-file
workflow to a unique name such as "single-file-build" (keep cancel-in-progress
as-is) so this workflow no longer blocks or is blocked by the Pages workflow.
In `@Build/pwa-assets.config.js`:
- Around line 1-27: The dependency constraint for `@vite-pwa/assets-generator` is
incorrect (1.0.1 does not exist); update the package version in your dependency
manifest so installs resolve deterministically — change the version specifier
from ^1.0.1 to ^1.0.2 (or to ^1.0.0 if you intend compatibility with the initial
1.0.0 release). Ensure the updated version is saved in package.json and any
lockfile is refreshed so the build uses the published release compatible with
the existing usage of defineConfig and minimal2023Preset.
In `@Build/src/appState.ts`:
- Around line 82-88: The code currently only type-checks
snapshot.activeTab/activeOutput before calling
activeTabState.set/activeOutputState.set, which lets invalid persisted strings
propagate; update this to whitelist values: define or import the canonical
allowed sets (e.g., TAB_KEYS or an enum used by the UI/editor and OUTPUT_KEYS)
and verify snapshot.activeTab and snapshot.activeOutput are members (e.g.,
allowedTabs.includes(snapshot.activeTab)) before calling
activeTabState.set/activeOutputState.set; if a value is invalid, skip setting
(or set a safe default) and optionally log a warning so downstream lookups
remain stable.
- Around line 48-54: The effect currently calls
localStorage.setItem("htmlRunnerState", JSON.stringify(createStateSnapshot()))
on every content mutation, causing sync hot-path writes; wrap this persistence
in a debounce/throttle so writes occur at a limited frequency (e.g., debounce
250–1000ms) or use requestIdleCallback to perform the write when idle, and
optionally skip writes if the new snapshot equals the last persisted snapshot.
Update the effect that references stateHydrated.get(), createStateSnapshot(),
and the localStorage.setItem call to schedule the serialized write via the
chosen debounce/throttle/idle mechanism and cancel any pending timer on cleanup.
In `@Build/src/defaultContent.ts`:
- Line 8: The default HTML in Build/src/defaultContent.ts references <script
src="main.js"></script> but the build/export emits "script.js", so update the
script tag in the default export to <script src="script.js"></script> (i.e.,
replace "main.js" with "script.js" in the default content string) so the
exported project loads the correct JS file out of the box.
In `@Build/src/runner.ts`:
- Around line 17-33: The cached promise prettierBundlePromise currently holds a
rejected promise if the dynamic imports fail, causing all future format attempts
to fail; modify the initialization in the import block so the
Promise.all(...).then(...) chain also has a .catch(err => {
prettierBundlePromise = undefined; throw err; }) (or similar reset) so that on
load failure the cached prettierBundlePromise is cleared and subsequent calls
retry the dynamic imports; reference the prettierBundlePromise variable and the
Promise.all([... import(...) ...]).then(...) chain in runner.ts when applying
this change.
In `@Build/vite.config.mjs`:
- Around line 7-25: The plugin inlineSvgFaviconPlugin is treating the file at
options.svg as UTF-8 SVG (transformIndexHtml reads it as text and runs
XML/whitespace regex) but the project supplies a PNG, corrupting the favicon;
fix by either (A) replacing the PNG with a real SVG and updating the favicon
reference to public/favicon.svg so inlineSvgFaviconPlugin can continue to read
UTF-8 SVG, or (B) update inlineSvgFaviconPlugin/transformIndexHtml to detect the
file type from options.svg (e.g., by extension or mime sniff), read binary for
non-SVG files (fs.readFileSync(options.svg) without "utf8"), skip the
XML/whitespace regex for binary images, base64-encode the raw buffer, and set
the correct type in faviconTag (image/png vs image/svg+xml) before inserting
into the head.
In `@TODO.md`:
- Line 12: Update the Auto Completion checklist item text to hyphenate
“built-in” in the phrase inside the markdown list item (the line containing "- [
] **Auto Completion** (using the built in CodeMirror tools)"); change "built in"
to "built-in" so the item reads "(using the built-in CodeMirror tools)".
---
Duplicate comments:
In @.github/workflows/single-file.yml:
- Around line 13-41: The workflow lacks an explicit permissions block for
GITHUB_TOKEN; add a minimal permissions declaration (e.g., permissions:
contents: read) either at the workflow root or scoped to the deploy job to limit
token access; update the top-level or the deploy job (job name "deploy") to
include this permissions entry so the GITHUB_TOKEN has only the required read
access.
---
Nitpick comments:
In @.github/workflows/single-file.yml:
- Line 22: The workflows are using different Node.js versions (node-version:
"22" in this workflow vs node-version: "20" in the other workflow); pick one
Node.js version to standardize on (e.g., "20" or "22") and update the
node-version key in this workflow to match the version used in the other
workflow (change node-version: "22" to the chosen version), and then run CI
locally or via GitHub Actions to confirm no compatibility issues; also search
for any other occurrences of node-version in workflow files and align them to
the same value.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d116896b-483e-43ba-b136-24f76242b42e
⛔ Files ignored due to path filters (2)
Build/package-lock.jsonis excluded by!**/package-lock.jsonBuild/public/favicon.pngis excluded by!**/*.png
📒 Files selected for processing (28)
.github/workflows/single-file.yml.github/workflows/static.ymlBuild/Buildscripts/build-all.shBuild/Buildscripts/build-inline.shBuild/Buildscripts/build.shBuild/index.htmlBuild/package.jsonBuild/pwa-assets.config.jsBuild/src/appState.tsBuild/src/defaultContent.tsBuild/src/editor.tsBuild/src/global.d.tsBuild/src/index.htmlBuild/src/main.tsBuild/src/runner.tsBuild/src/state.tsBuild/src/types.tsBuild/src/ui.tsBuild/styles/styles.cssBuild/tsconfig.jsonBuild/vite.config.mjsBuild/webpack.config.jsREADME.mdTODO.mdmain.jsmanifest.jsonservice-worker.jsstyles.css
💤 Files with no reviewable changes (10)
- Build/webpack.config.js
- service-worker.js
- Build/Buildscripts/build-all.sh
- manifest.json
- Build/src/index.html
- Build/Buildscripts/build.sh
- styles.css
- Build/src/state.ts
- Build/Buildscripts/build-inline.sh
- Build/tsconfig.json
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Build/src/runner.ts (1)
107-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
loadlistener accumulates on the preview iframe across runs.Every
runCode()attaches a newloadlistener that closes over the currenturl. The listener is never removed, so each subsequent run adds another one (each invoked on its own matching load). Over a long session this leaks listeners and closures. Use{ once: true }(or remove explicitly) so each listener is detached after firing.🔒️ Proposed fix
preview.src = url; - preview.addEventListener("load", () => URL.revokeObjectURL(url)); + preview.addEventListener( + "load", + () => URL.revokeObjectURL(url), + { once: true } + ); switchOutput("preview");🤖 Prompt for 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. In `@Build/src/runner.ts` around lines 107 - 109, The preview iframe's "load" listener attached in runCode() captures the current url and is never removed, causing accumulated listeners and closures; update the addEventListener call on preview (the listener that calls URL.revokeObjectURL(url) and then switchOutput("preview")) to be registered with the { once: true } option (or alternatively store and remove the handler after it runs) so the handler automatically detaches after firing and prevents leaks.
♻️ Duplicate comments (1)
.github/workflows/single-file.yml (1)
11-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConcurrency group still collides with
static.yml.
group: "pages"continues to overlap with the Pages deploy workflow, so this artifact build can be queued/blocked unnecessarily. A workflow-specific group like"single-file-build"would isolate it.🤖 Prompt for 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. In @.github/workflows/single-file.yml around lines 11 - 13, The concurrency group "pages" in the workflow's concurrency block collides with the Pages deploy workflow; change the concurrency group to a workflow-specific name (e.g., "single-file-build") by updating the concurrency.group value in .github/workflows/single-file.yml so this workflow uses a unique group and no longer blocks or is blocked by the Pages workflow.
🧹 Nitpick comments (8)
.github/workflows/single-file.yml (2)
16-16: 💤 Low valueJob name
deployis misleading.This job builds and uploads an artifact; it does not deploy. Renaming to e.g.
buildimproves log readability and avoids confusion with the Pages workflow.🤖 Prompt for 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. In @.github/workflows/single-file.yml at line 16, The GitHub Actions job key "deploy" is misleading because it only builds and uploads an artifact; rename the job key from deploy to build to reflect its purpose (update the top-level job identifier "deploy" to "build" in the workflow), and search/update any references to that job key (e.g., other jobs' needs:, outputs, or step uses) so they point to "build" instead of "deploy" to keep logs and dependencies consistent.
22-29: ⚡ Quick winPrefer
npm ciand enable setup-node’s built-in cache.In CI,
npm ciis faster and deterministic againstpackage-lock.json, andactions/setup-node@v4can cache~/.npmautomatically. Both reduce build time and avoid surprise resolutions.♻️ Proposed change
- name: Use Node.js uses: actions/setup-node@v4 with: node-version: "20" + cache: "npm" + cache-dependency-path: Build/package-lock.json - name: Install dependencies working-directory: Build - run: npm i + run: npm ci🤖 Prompt for 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. In @.github/workflows/single-file.yml around lines 22 - 29, Replace the non-deterministic npm install step with a clean CI install and enable setup-node’s npm cache: in the GitHub Actions job that uses actions/setup-node@v4 (with node-version: "20"), add the cache: "npm" option to its with block and change the Install dependencies run step (currently using working-directory: Build and run: npm i) to run: npm ci (keeping the working-directory: Build). This ensures deterministic installs via package-lock.json and activates setup-node’s built-in npm caching to speed subsequent runs.Build/src/defaultContent.ts (1)
8-13: 💤 Low valueUnnecessary forward-slash escapes inside a JS template literal.
<\/script>,<\/body>,<\/html>are only needed when an HTML string is embedded inside a<script>block in HTML source. Inside a.tstemplate literal they have no effect and just hurt readability. Safe to drop.♻️ Proposed cleanup
-<script src="script.js"><\/script> +<script src="script.js"></script> <h1>Hello, HTMLRunner!</h1> <p>This is a demo page.</p> <button onclick="testFunction()">Click me!</button> -<\/body> -<\/html>`; +</body> +</html>`;🤖 Prompt for 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. In `@Build/src/defaultContent.ts` around lines 8 - 13, The template literal in defaultContent.ts contains unnecessary escaped forward slashes (e.g., "<\/script>", "<\/body>", "<\/html>") that were only needed inside an HTML <script> block; remove the backslashes so the strings become "</script>", "</body>", "</html>" in the template (locate the template literal that includes the lines with "<\/script>" and replace those escaped sequences). Ensure any variables or functions that reference this content still receive the corrected unescaped HTML string.Build/src/editor.ts (3)
3-16: 💤 Low valueDuplicate import from
@codemirror/commands.
defaultKeymap, standardKeymapandtoggleCommentare imported in two separate statements from the same module. Merge them into a single import for cleanliness.♻️ Proposed change
-import { defaultKeymap, standardKeymap } from "@codemirror/commands"; +import { defaultKeymap, standardKeymap, toggleComment } from "@codemirror/commands"; @@ -import { toggleComment } from "@codemirror/commands"; // Ensure toggleComment is imported🤖 Prompt for 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. In `@Build/src/editor.ts` around lines 3 - 16, There are duplicate imports from "@codemirror/commands" (defaultKeymap, standardKeymap and toggleComment); consolidate them into a single import statement that includes defaultKeymap, standardKeymap and toggleComment so only one import from "@codemirror/commands" remains (update the import near the top of the file where defaultKeymap/standardKeymap are currently declared).
69-75: 💤 Low valueNo-op
linteradds machinery for nothing.The lint source unconditionally returns
[], solinter(...)+lintGutter()only render an empty gutter and burn a 100ms timer per edit. Either drop both extensions or plug in a real linter for the language.🤖 Prompt for 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. In `@Build/src/editor.ts` around lines 69 - 75, The current integration of linter(...) and lintGutter() is a no-op because the lint source passed to linter always returns an empty array, wasting a 100ms timer and rendering an empty gutter; either remove both linter(...) and lintGutter() calls from the editor extensions list or replace the stub lint source with a real diagnostic provider (implement a function used by linter that returns proper Diagnostic objects for the language and handles debouncing instead of relying on the unused 100ms delay), updating the references to linter and lintGutter accordingly.
76-80: 💤 Low valueRemove
standardKeymapfrom the spread;defaultKeymapalready includes it.
defaultKeymapfrom@codemirror/commandsconcatenates additional bindings withstandardKeymap, so spreading both creates duplicate key bindings.🤖 Prompt for 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. In `@Build/src/editor.ts` around lines 76 - 80, The keymap configuration currently spreads both defaultKeymap and standardKeymap, causing duplicate bindings; remove standardKeymap from the array passed to keymap.of and only include defaultKeymap and the custom binding (the object with key "Mod-/" and run: toggleComment) so use keymap.of([...defaultKeymap, { key: "Mod-/", run: toggleComment }]); update the array in the keymap.of call that currently references defaultKeymap, standardKeymap, and the toggleComment binding.Build/src/runner.ts (1)
60-60: 💤 Low valueFull-HTML detection regex misses a bare
<html>with no attributes/whitespace before EOF.
/<html[\s>]|<!doctype html/irequires a whitespace or>after<html. That covers<html>and<html lang="…">, but not pathological cases like<htmlat EOF or<html/>(self-closed). Not a real-world concern for typical user input, but worth a glance if you want to be strict; otherwise safe to keep as-is.🤖 Prompt for 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. In `@Build/src/runner.ts` at line 60, The isFullHtml detection currently using the regex assigned to isFullHtml misses cases like a bare "<html" at EOF or self-closed "<html/>"; update the regex used where isFullHtml is declared so it also matches "<html" followed by end-of-string or a "/" before ">" (for example use a lookahead or alternation such as matching "<html" when followed by whitespace, ">", "/", or end-of-string) and keep the existing doctype check (the variable name isFullHtml in Build/src/runner.ts identifies the spot to change).Build/src/ui.ts (1)
112-135: 💤 Low value
toggleDarkModeandsetPageDarkModeduplicate the same three-step sequence.Both functions call
setDarkMode(...), toggle thedark-modebody class, and refresh the theme icon.toggleDarkModecan just delegate tosetPageDarkModeto keep the logic in one place.♻️ Proposed change
export function toggleDarkMode(): void { - const newDarkMode = !darkModeState.get(); - setDarkMode(newDarkMode); - document.body.classList.toggle("dark-mode", newDarkMode); - updateThemeIcon(); + setPageDarkMode(!darkModeState.get()); }🤖 Prompt for 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. In `@Build/src/ui.ts` around lines 112 - 135, toggleDarkMode duplicates the same three-step sequence as setPageDarkMode; change toggleDarkMode to compute the new value (const newDarkMode = !darkModeState.get()) and delegate to setPageDarkMode(newDarkMode) instead of calling setDarkMode, toggling document.body, and calling updateThemeIcon itself so all state/class/icon logic lives in setPageDarkMode.
🤖 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 @.github/workflows/single-file.yml:
- Around line 4-5: The workflow's push.branches glob currently uses branches:
["*"] which does not match branch names containing "/" so feature or dependabot
branches are skipped; update the push.branches setting (the push -> branches
entry) to use branches: ["**"] instead of ["*"] so all branches, including those
with slashes, trigger the workflow.
In `@Build/src/editor.ts`:
- Around line 35-43: The setAutoRun function only updates autoRunState but must
also reconfigure every editor's autoRunCompartment like setDarkMode does: update
autoRunState.set(value) then iterate all editors (the same collection used by
setDarkMode) and call editor.view.dispatch({effects:
autoRunCompartment.reconfigure(value ? autoRunOnExtension :
autoRunOffExtension)}) or the equivalent reconfigure call using
autoRunCompartment to apply the new setting; update setAutoRun to be the single
source of truth and remove the manual reconfiguration loops from the
toggleAutoRun implementations in ui.ts and main.ts so callers don’t need to
reconfigure editors themselves.
---
Outside diff comments:
In `@Build/src/runner.ts`:
- Around line 107-109: The preview iframe's "load" listener attached in
runCode() captures the current url and is never removed, causing accumulated
listeners and closures; update the addEventListener call on preview (the
listener that calls URL.revokeObjectURL(url) and then switchOutput("preview"))
to be registered with the { once: true } option (or alternatively store and
remove the handler after it runs) so the handler automatically detaches after
firing and prevents leaks.
---
Duplicate comments:
In @.github/workflows/single-file.yml:
- Around line 11-13: The concurrency group "pages" in the workflow's concurrency
block collides with the Pages deploy workflow; change the concurrency group to a
workflow-specific name (e.g., "single-file-build") by updating the
concurrency.group value in .github/workflows/single-file.yml so this workflow
uses a unique group and no longer blocks or is blocked by the Pages workflow.
---
Nitpick comments:
In @.github/workflows/single-file.yml:
- Line 16: The GitHub Actions job key "deploy" is misleading because it only
builds and uploads an artifact; rename the job key from deploy to build to
reflect its purpose (update the top-level job identifier "deploy" to "build" in
the workflow), and search/update any references to that job key (e.g., other
jobs' needs:, outputs, or step uses) so they point to "build" instead of
"deploy" to keep logs and dependencies consistent.
- Around line 22-29: Replace the non-deterministic npm install step with a clean
CI install and enable setup-node’s npm cache: in the GitHub Actions job that
uses actions/setup-node@v4 (with node-version: "20"), add the cache: "npm"
option to its with block and change the Install dependencies run step (currently
using working-directory: Build and run: npm i) to run: npm ci (keeping the
working-directory: Build). This ensures deterministic installs via
package-lock.json and activates setup-node’s built-in npm caching to speed
subsequent runs.
In `@Build/src/defaultContent.ts`:
- Around line 8-13: The template literal in defaultContent.ts contains
unnecessary escaped forward slashes (e.g., "<\/script>", "<\/body>", "<\/html>")
that were only needed inside an HTML <script> block; remove the backslashes so
the strings become "</script>", "</body>", "</html>" in the template (locate the
template literal that includes the lines with "<\/script>" and replace those
escaped sequences). Ensure any variables or functions that reference this
content still receive the corrected unescaped HTML string.
In `@Build/src/editor.ts`:
- Around line 3-16: There are duplicate imports from "@codemirror/commands"
(defaultKeymap, standardKeymap and toggleComment); consolidate them into a
single import statement that includes defaultKeymap, standardKeymap and
toggleComment so only one import from "@codemirror/commands" remains (update the
import near the top of the file where defaultKeymap/standardKeymap are currently
declared).
- Around line 69-75: The current integration of linter(...) and lintGutter() is
a no-op because the lint source passed to linter always returns an empty array,
wasting a 100ms timer and rendering an empty gutter; either remove both
linter(...) and lintGutter() calls from the editor extensions list or replace
the stub lint source with a real diagnostic provider (implement a function used
by linter that returns proper Diagnostic objects for the language and handles
debouncing instead of relying on the unused 100ms delay), updating the
references to linter and lintGutter accordingly.
- Around line 76-80: The keymap configuration currently spreads both
defaultKeymap and standardKeymap, causing duplicate bindings; remove
standardKeymap from the array passed to keymap.of and only include defaultKeymap
and the custom binding (the object with key "Mod-/" and run: toggleComment) so
use keymap.of([...defaultKeymap, { key: "Mod-/", run: toggleComment }]); update
the array in the keymap.of call that currently references defaultKeymap,
standardKeymap, and the toggleComment binding.
In `@Build/src/runner.ts`:
- Line 60: The isFullHtml detection currently using the regex assigned to
isFullHtml misses cases like a bare "<html" at EOF or self-closed "<html/>";
update the regex used where isFullHtml is declared so it also matches "<html"
followed by end-of-string or a "/" before ">" (for example use a lookahead or
alternation such as matching "<html" when followed by whitespace, ">", "/", or
end-of-string) and keep the existing doctype check (the variable name isFullHtml
in Build/src/runner.ts identifies the spot to change).
In `@Build/src/ui.ts`:
- Around line 112-135: toggleDarkMode duplicates the same three-step sequence as
setPageDarkMode; change toggleDarkMode to compute the new value (const
newDarkMode = !darkModeState.get()) and delegate to setPageDarkMode(newDarkMode)
instead of calling setDarkMode, toggling document.body, and calling
updateThemeIcon itself so all state/class/icon logic lives in setPageDarkMode.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dda8132d-35b6-4148-84bc-a2e4a28c9698
📒 Files selected for processing (9)
.github/workflows/single-file.ymlBuild/src/appState.tsBuild/src/defaultContent.tsBuild/src/editor.tsBuild/src/runner.tsBuild/src/ui.tsBuild/vite.config.mjsREADME.mdTODO.md
✅ Files skipped from review due to trivial changes (2)
- TODO.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Build/vite.config.mjs
- Build/src/appState.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Build/src/main.ts (1)
292-301: ⚡ Quick winAdd error handling for dynamic import failures.
If
file-saverorjszipfail to load (e.g., network issues, bundler misconfiguration), the unhandled rejection will crash the export flow without user feedback.🛡️ Proposed fix to add try/catch
async function exportAsZip() { const html = editors.html.view.state.doc.toString().trim(); const css = editors.css.view.state.doc.toString().trim(); const js = editors.js.view.state.doc.toString().trim(); const files: { name: string; content: string }[] = []; if (html) files.push({ name: "index.html", content: html }); if (css) files.push({ name: "styles.css", content: css }); if (js) files.push({ name: "script.js", content: js }); if (files.length === 0) { alert("Nothing to export!"); return; } + try { if (files.length === 1) { const { saveAs } = await import("file-saver"); const blob = new Blob([files[0].content], { type: "text/plain" }); saveAs(blob, files[0].name); return; } const [{ default: JSZip }, { saveAs }] = await Promise.all([ import("jszip"), import("file-saver"), ]); const zip = new JSZip(); for (const file of files) { zip.file(file.name, file.content); } const content = await zip.generateAsync({ type: "blob" }); saveAs(content, "htmlrunner-export.zip"); + } catch (error) { + console.error("Export failed:", error); + alert("Failed to export files. Please try again."); + } }🤖 Prompt for 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. In `@Build/src/main.ts` around lines 292 - 301, The dynamic imports for "file-saver" and "jszip" (the destructuring that produces JSZip and saveAs via Promise.all and the earlier single import of saveAs) need try/catch around them to prevent unhandled rejections; wrap the await import(...) calls (both the single import branch that sets saveAs and the Promise.all branch that yields { default: JSZip } and { saveAs }) in try/catch, log the error (console.error or processLogger), show a user-facing error/notification, and return early if the imports fail so the export flow does not proceed with undefined JSZip/saveAs.
🤖 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 `@Build/src/console.ts`:
- Line 60: The current regex used in the line.match call (/:(\d+):(\d+)\)?$/) is
too narrow and misses common stack formats (e.g. "at fn (file.js:123:45)",
"fn@file.js:123:45", "file.js:123"). Replace the single regex with a more
flexible approach: try matching multiple patterns in order (e.g.,
/\(([^)]+):(\d+):(\d+)\)$/, /([^@()\s]+):(\d+):(\d+)$/, and
/([^@()\s]+):(\d+)$/) or use one combined regex that captures file, line, and
optional column; map captured groups to file, line, col (allowing col to be
undefined) and update the existing link-building logic that uses m so all common
trace formats become clickable.
In `@Build/src/main.ts`:
- Line 68: The sizes fallback uses the truthy || check so empty arrays or [0,0]
won't trigger the default; change the assignment that sets sizes (where you call
splitSizesState.get()) to validate the returned value: call
splitSizesState.get(), ensure it's a non-empty array of positive numbers (and
acceptable length), and if that validation fails use [50,50]; alternatively use
nullish coalescing (??) only if you also guard against empty arrays—update the
sizes property assignment accordingly.
---
Nitpick comments:
In `@Build/src/main.ts`:
- Around line 292-301: The dynamic imports for "file-saver" and "jszip" (the
destructuring that produces JSZip and saveAs via Promise.all and the earlier
single import of saveAs) need try/catch around them to prevent unhandled
rejections; wrap the await import(...) calls (both the single import branch that
sets saveAs and the Promise.all branch that yields { default: JSZip } and {
saveAs }) in try/catch, log the error (console.error or processLogger), show a
user-facing error/notification, and return early if the imports fail so the
export flow does not proceed with undefined JSZip/saveAs.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2388e993-a0e1-4d8a-8478-bf6af3c30a28
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/single-file.ymlBuild/package.jsonBuild/src/console.tsBuild/src/defaultContent.tsBuild/src/editor.tsBuild/src/main.tsBuild/src/runner.tsBuild/src/ui.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- Build/src/defaultContent.ts
- .github/workflows/single-file.yml
- Build/src/ui.ts
- Build/package.json
- Build/src/editor.ts
- Build/src/runner.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Build/src/main.ts (1)
522-538:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad persisted state before initializing Split.js.
initializeSplit()runs beforeloadState(), so the first render always uses the default panel sizes and ignores the savedsplitSizesStateuntil a later resize recreates the split instance.Suggested fix
document.addEventListener("DOMContentLoaded", async () => { + loadState(); initializeEditors(); initializeCopyButtons(); initializeSplit(); updateAutoRunStatus(); initializeLogFilters(); addGlobalSearchShortcuts(); @@ - loadState(); setPageDarkMode(darkModeState.get()); updateAutoRunStatus(); formatCode().catch((error) => {🤖 Prompt for 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. In `@Build/src/main.ts` around lines 522 - 538, The persisted UI state is loaded after initializeSplit(), so Split.js gets default sizes; move loadState() to run before initializeSplit() so initializeSplit() can read the persisted splitSizesState and create the split with saved sizes (adjust the DOMContentLoaded sequence to call loadState() — and any dependent calls like setPageDarkMode(darkModeState.get()) — before calling initializeSplit()); ensure initializeSplit() reads splitSizesState on creation so the saved sizes are applied immediately.
🤖 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 `@Build/eslint.config.mts`:
- Line 10: The ESLint block that sets { files: ["**/*.{js,mjs,cjs,ts,mts,cts}"],
languageOptions: { globals: globals.browser } } applies browser globals to
Node-side files (e.g., Build/vite.config.mjs), causing incorrect linting; fix by
scoping globals.browser to only client/browser patterns (e.g., src/**, web/**,
or specific extensions) and add a separate config entry for Node-side patterns
(e.g., Build/**, scripts/**) that uses languageOptions: { globals: globals.node
} so Node scripts like Build/vite.config.mjs use node globals instead of
globals.browser.
In `@Build/src/console.ts`:
- Around line 29-30: The new console message handler (initializeConsole ->
handleConsoleMessage) is not being used because the app's entrypoint still
registers its own window.message listener; call initializeConsole() from the
application's startup (e.g., the main entrypoint initialization function) so the
clickable stack-line logic runs, and remove the duplicate
window.addEventListener("message", ...) registration in the entrypoint
(Build/src/main.ts) so initializeConsole/handleConsoleMessage is the single
source of truth and events aren't rendered twice.
In `@Build/src/main.ts`:
- Around line 482-489: The search input lookup currently uses
document.querySelector(...) inside window.requestAnimationFrame which can match
a hidden .cm-search from another tab; instead scope the query to the active
editor/search panel element you just opened (e.g., obtain the search panel or
active editor container node and call root.querySelector(selector) rather than
document.querySelector). Update the focus/select logic to use that scoped node
(replace the document.querySelector call that assigns field) so mode, selector
and field behavior stays the same but targets the visible panel for the active
editor.
---
Outside diff comments:
In `@Build/src/main.ts`:
- Around line 522-538: The persisted UI state is loaded after initializeSplit(),
so Split.js gets default sizes; move loadState() to run before initializeSplit()
so initializeSplit() can read the persisted splitSizesState and create the split
with saved sizes (adjust the DOMContentLoaded sequence to call loadState() — and
any dependent calls like setPageDarkMode(darkModeState.get()) — before calling
initializeSplit()); ensure initializeSplit() reads splitSizesState on creation
so the saved sizes are applied immediately.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af0ecc02-950f-49e0-b593-9f8b16a44d1a
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
Build/eslint.config.mtsBuild/package.jsonBuild/src/appState.tsBuild/src/console.tsBuild/src/defaultContent.tsBuild/src/editor.tsBuild/src/global.d.tsBuild/src/main.tsBuild/src/runner.tsBuild/src/ui.tsBuild/src/utils.ts
✅ Files skipped from review due to trivial changes (2)
- Build/src/utils.ts
- Build/src/defaultContent.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- Build/src/global.d.ts
- Build/package.json
- Build/src/editor.ts
- Build/src/appState.ts
- Build/src/ui.ts
- Build/src/runner.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Build/src/appState.ts`:
- Around line 23-41: The initial signal values (darkModeInitial, autoRunInitial,
htmlInitial, cssInitial, jsInitial, activeTabInitial, activeOutputInitial,
splitSizesInitial, logFiltersInitial) are being seeded directly from
persistedState/localStorage without validation; update these initializers to
first safely parse localStorage (try/catch JSON.parse) and then validate each
value’s type and allowed values (e.g., html/css/js must be strings,
activeTab/activeOutput must be one of the accepted keys, splitSizes must be an
array of two numbers, logFilters must be string[]), falling back to the default
constants (defaultHtml/defaultCss/defaultJs, "html"/"preview", [50,50],
["log","error","warn","info"]) if validation fails; ensure loadState() and
applyStateSnapshot() are not relied on to fix invalid initial signals by
preventing bad values from being assigned in the darkModeInitial/... variables
in the module top-level code.
In `@Build/src/console.ts`:
- Around line 151-178: renderObject currently passes non-object nested
primitives (e.g., undefined, bigint, symbol) into the WeakSet logic and throws
TypeError; update renderObject to detect non-object/non-function values before
the visited.has/visited.add block (inside renderObject) and return a text node
for those primitives (use String(obj) or explicit "undefined"/bigint conversion)
so only actual objects/functions are checked/added to visited; reference the
renderObject function and the visited WeakSet usage around the visited.has(...)
and visited.add(...) calls.
In `@Build/src/ui.ts`:
- Around line 19-22: The showError function currently sets a new timeout every
call which lets an earlier timer clear a later error; fix this by tracking the
timeout id in a module-scoped variable (e.g., errorTimeoutId) and call
clearTimeout(errorTimeoutId) before scheduling a new setTimeout; then save the
returned id back to errorTimeoutId so the most recent message isn't prematurely
cleared. Reference: showError and errorMessage.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90e68f91-2ba0-42d6-9599-0f88cdfb65ad
📒 Files selected for processing (11)
Build/index.htmlBuild/src/appState.tsBuild/src/console.tsBuild/src/editor.tsBuild/src/main.tsBuild/src/runner.tsBuild/src/types.tsBuild/src/ui.tsBuild/src/utils.tsBuild/styles/styles.cssBuild/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- Build/styles/styles.css
🚧 Files skipped from review as they are similar to previous changes (4)
- Build/src/utils.ts
- Build/index.html
- Build/src/editor.ts
- Build/src/runner.ts
Comment on lines
+151
to
+178
| function renderObject( | ||
| obj: ConsoleData, | ||
| level = 0, | ||
| visited = new WeakSet<object>(), | ||
| ): Node { | ||
| if (obj === null) return document.createTextNode("null"); | ||
| if ( | ||
| typeof obj === "string" || | ||
| typeof obj === "number" || | ||
| typeof obj === "boolean" | ||
| ) | ||
| return document.createTextNode(String(obj)); | ||
| if (obj instanceof Error) { | ||
| const errorEl = document.createElement("span"); | ||
| errorEl.className = "console-error"; | ||
| errorEl.textContent = `${obj.name}: ${obj.message}`; | ||
| if ((obj as Error).stack) | ||
| errorEl.textContent += `\n${(obj as Error).stack!.split("\n").slice(1).join("\n")}`; | ||
| const container = document.createElement("span"); | ||
| container.appendChild(errorEl); | ||
| return container; | ||
| } | ||
| // At this point obj is either an array-like or object with console-able entries | ||
| const asObj = obj as Record<string, ConsoleData> | ConsoleData[]; | ||
| if (visited.has(asObj as object)) | ||
| return document.createTextNode("[Circular]"); | ||
| if (level > 3) return document.createTextNode("[...]"); | ||
| visited.add(asObj as object); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
for (const value of [undefined, 1n]) {
try {
new WeakSet().add(value);
console.log(`${typeof value}: accepted`);
} catch (err) {
console.log(`${typeof value}: ${err.name}`);
}
}
NODERepository: HTMLToolkit/HTMLRunner
Length of output: 105
🏁 Script executed:
# Find and read the full renderObject function
find . -name "console.ts" -type f | head -5Repository: HTMLToolkit/HTMLRunner
Length of output: 89
🏁 Script executed:
# Read the file to understand the full context
wc -l Build/src/console.tsRepository: HTMLToolkit/HTMLRunner
Length of output: 91
🏁 Script executed:
cat -n Build/src/console.tsRepository: HTMLToolkit/HTMLRunner
Length of output: 10277
🏁 Script executed:
# Find and read the types file to understand ConsoleData
find . -name "types.ts" -type fRepository: HTMLToolkit/HTMLRunner
Length of output: 87
🏁 Script executed:
cat -n Build/src/types.tsRepository: HTMLToolkit/HTMLRunner
Length of output: 2867
Handle nested primitive edge cases before the WeakSet branch.
renderObject() only short-circuits null, string, number, and boolean. Nested values like undefined or bigint inside objects (e.g., from iterating { foo: undefined }) fall through to visited.has() and visited.add() at lines 175–178, which throw TypeError because WeakSet does not accept non-object values. This breaks console rendering for otherwise valid logged data.
Proposed fix
function renderObject(
obj: ConsoleData,
level = 0,
visited = new WeakSet<object>(),
): Node {
if (obj === null) return document.createTextNode("null");
if (
typeof obj === "string" ||
typeof obj === "number" ||
- typeof obj === "boolean"
+ typeof obj === "boolean" ||
+ typeof obj === "undefined" ||
+ typeof obj === "bigint"
)
return document.createTextNode(String(obj));
+ if (typeof obj !== "object") {
+ return document.createTextNode(String(obj));
+ }
if (obj instanceof Error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function renderObject( | |
| obj: ConsoleData, | |
| level = 0, | |
| visited = new WeakSet<object>(), | |
| ): Node { | |
| if (obj === null) return document.createTextNode("null"); | |
| if ( | |
| typeof obj === "string" || | |
| typeof obj === "number" || | |
| typeof obj === "boolean" | |
| ) | |
| return document.createTextNode(String(obj)); | |
| if (obj instanceof Error) { | |
| const errorEl = document.createElement("span"); | |
| errorEl.className = "console-error"; | |
| errorEl.textContent = `${obj.name}: ${obj.message}`; | |
| if ((obj as Error).stack) | |
| errorEl.textContent += `\n${(obj as Error).stack!.split("\n").slice(1).join("\n")}`; | |
| const container = document.createElement("span"); | |
| container.appendChild(errorEl); | |
| return container; | |
| } | |
| // At this point obj is either an array-like or object with console-able entries | |
| const asObj = obj as Record<string, ConsoleData> | ConsoleData[]; | |
| if (visited.has(asObj as object)) | |
| return document.createTextNode("[Circular]"); | |
| if (level > 3) return document.createTextNode("[...]"); | |
| visited.add(asObj as object); | |
| function renderObject( | |
| obj: ConsoleData, | |
| level = 0, | |
| visited = new WeakSet<object>(), | |
| ): Node { | |
| if (obj === null) return document.createTextNode("null"); | |
| if ( | |
| typeof obj === "string" || | |
| typeof obj === "number" || | |
| typeof obj === "boolean" || | |
| typeof obj === "undefined" || | |
| typeof obj === "bigint" | |
| ) | |
| return document.createTextNode(String(obj)); | |
| if (typeof obj !== "object") { | |
| return document.createTextNode(String(obj)); | |
| } | |
| if (obj instanceof Error) { | |
| const errorEl = document.createElement("span"); | |
| errorEl.className = "console-error"; | |
| errorEl.textContent = `${obj.name}: ${obj.message}`; | |
| if ((obj as Error).stack) | |
| errorEl.textContent += `\n${(obj as Error).stack!.split("\n").slice(1).join("\n")}`; | |
| const container = document.createElement("span"); | |
| container.appendChild(errorEl); | |
| return container; | |
| } | |
| // At this point obj is either an array-like or object with console-able entries | |
| const asObj = obj as Record<string, ConsoleData> | ConsoleData[]; | |
| if (visited.has(asObj as object)) | |
| return document.createTextNode("[Circular]"); | |
| if (level > 3) return document.createTextNode("[...]"); | |
| visited.add(asObj as object); |
🤖 Prompt for 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.
In `@Build/src/console.ts` around lines 151 - 178, renderObject currently passes
non-object nested primitives (e.g., undefined, bigint, symbol) into the WeakSet
logic and throws TypeError; update renderObject to detect
non-object/non-function values before the visited.has/visited.add block (inside
renderObject) and return a text node for those primitives (use String(obj) or
explicit "undefined"/bigint conversion) so only actual objects/functions are
checked/added to visited; reference the renderObject function and the visited
WeakSet usage around the visited.has(...) and visited.add(...) calls.
Comment on lines
19
to
+22
| export function showError(message: string): void { | ||
| errorEl.textContent = message; | ||
| errorEl.style.display = "block"; | ||
| setTimeout(() => (errorEl.style.display = "none"), 5000); | ||
| errorMessage.set(message); | ||
| // clear after a short timeout | ||
| setTimeout(() => errorMessage.set(""), 5000); |
There was a problem hiding this comment.
Clear the previous dismiss timer before scheduling a new one.
Back-to-back showError() calls race here: the first timer can clear the second message almost immediately, so the latest error does not stay visible for the intended 5 seconds.
💡 Proposed fix
+let clearErrorTimeout: number | undefined;
+
export function showError(message: string): void {
+ if (clearErrorTimeout) {
+ window.clearTimeout(clearErrorTimeout);
+ }
errorMessage.set(message);
- // clear after a short timeout
- setTimeout(() => errorMessage.set(""), 5000);
+ clearErrorTimeout = window.setTimeout(() => {
+ errorMessage.set("");
+ clearErrorTimeout = undefined;
+ }, 5000);
}🤖 Prompt for 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.
In `@Build/src/ui.ts` around lines 19 - 22, The showError function currently sets
a new timeout every call which lets an earlier timer clear a later error; fix
this by tracking the timeout id in a module-scoped variable (e.g.,
errorTimeoutId) and call clearTimeout(errorTimeoutId) before scheduling a new
setTimeout; then save the returned id back to errorTimeoutId so the most recent
message isn't prematurely cleared. Reference: showError and errorMessage.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
Build/src/appState.ts (1)
22-44:⚠️ Potential issue | 🟠 MajorSanitize persisted values before using them as initial signal state.
These initializers still copy raw
localStorageJSON intofilesState,activeOutputState,splitSizesState, and the other first-load values. If storage is corrupted, bad shapes/types are present beforeapplyStateSnapshot()gets a chance to validate them, which can break the initial render and editor bootstrap.🤖 Prompt for 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. In `@Build/src/appState.ts` around lines 22 - 44, Persisted values are used directly into initial signals (filesInitial, activeFileInitial, darkModeInitial, autoRunInitial, activeOutputInitial, splitSizesInitial, logFiltersInitial) which can break startup if storage is corrupted; add small sanitizer/validator helpers (e.g., sanitizeFiles, sanitizeActiveFile, sanitizeBooleans, sanitizeSplitSizes, sanitizeLogFilters, sanitizeActiveOutput) that confirm types/shape (files is an array of objects with required id/content/title, splitSizes is two numbers in 0-100, logFilters is array of allowed strings, activeOutput is one of allowed values, darkMode/autoRun are booleans) and return safe defaults (defaultFiles, ["log","error","warn","info"], [50,50], "preview", etc.) then wire the initializers to use these sanitizers instead of directly using persistedState so the app always gets validated shapes before applyStateSnapshot runs.
🤖 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 `@Build/index.html`:
- Line 59: The preview iframe (<iframe id="preview" class="preview active">)
lacks a sandbox attribute allowing previewed scripts to access the parent; add a
sandbox attribute to harden isolation (e.g., sandbox="allow-scripts" for running
user scripts but preventing access to window.parent/storage) on the iframe with
id="preview" and avoid including permissions like allow-same-origin or
allow-top-navigation unless explicitly required.
In `@Build/package.json`:
- Line 12: The lint script in package.json currently silences ESLint failures by
appending "|| true" to the "lint" script; remove the "|| true" from the "lint"
script so ESLint errors cause a non-zero exit (i.e., change the "lint" script
value to "eslint \"src/**/*.{ts,js}\" --ext .ts,.js"), and if you need a
non-blocking helper add a separate script like "lint:ci-safe" or "lint:soft"
that includes "|| true".
In `@Build/src/biome.ts`:
- Around line 32-55: Concurrent calls to initBiome can race, so introduce an
in-flight promise (e.g. initBiomePromise: Promise<void> | null) and use it to
ensure single-flight initialization: at the start of initBiome check initialized
and return, then if initBiomePromise exists await and return; otherwise assign
initBiomePromise = (async () => { await initBiomeWasm(); workspace = new
Workspace(); const openRes = workspace.openProject({ path: "",
openUninitialized: true }); projectKey = openRes.projectKey;
workspace.updateSettings({ projectKey, configuration: { linter: { enabled: true,
rules: { recommended: true } } }, workspaceDirectory: "./", }); initialized =
true; })(); await initBiomePromise; finally clear initBiomePromise if you prefer
(or leave until process end) so all callers await the same promise and avoid
racing workspace/projectKey assignment.
In `@Build/src/editor.ts`:
- Around line 189-207: The effect currently only reconfigures
_languageCompartment when the file content changes, so renaming a file (e.g.,
script.js → styles.css) with identical content won't update the editor language;
fix by tracking the active filename (e.g., add a closure variable
lastActiveFileName) and, inside effect() along with the existing content check
on editor.view, also compare file.name to lastActiveFileName and dispatch the
_languageCompartment.reconfigure(getLanguageExtension(file.name)) when the name
changed (then set lastActiveFileName = file.name); ensure this runs regardless
of content equality but still respects the _suppressContentUpdate flow and only
triggers the reconfigure when the filename actually changed.
In `@Build/src/main.ts`:
- Around line 188-194: The map that builds console text uses JSON.stringify(d)
directly and can throw on circular or DOM objects; update the inner mapping for
ev.data inside the entries.map callback so each value (d) is serialized safely:
wrap JSON.stringify(d) in a try/catch (or use a safe replacer approach) and fall
back to String(d) or a non-throwing representation when stringify fails; ensure
the change is applied to the message construction inside the entries.map(ev => {
... }) block so a single bad value cannot break the whole "Copy All" output.
- Around line 323-349: renderFileTabs renders file tabs as non-interactive divs
so keyboard users cannot switch files; update renderFileTabs to make each tab
and the add/close controls keyboard-accessible by using focusable elements or
adding tabindex="0", proper ARIA roles/labels (e.g., role="tab" or role="button"
on the tab div, aria-selected when file.id === activeId, aria-label on closeBtn
and addBtn), and attach keydown handlers that activate the same logic as click
for Enter and Space (use the tab.dataset.file to identify the file and call the
same file-switch/close handlers). Also ensure the closeBtn and addBtn are
reachable via keyboard (tabindex or real <button>), and apply the same fixes to
the analogous rendering code referenced around lines 368-389 (the other tab/tree
render function) so keyboard activation and ARIA state are consistent.
- Around line 427-447: addFileAction is currently creating a FileTab with
id/name == fileName and unconditionally appending it to filesState, allowing
duplicate ids; update addFileAction to check filesState.get() for an existing
file with the same id or name before adding (compare against existing FileTab.id
and FileTab.name), and if a duplicate exists either reject the creation (abort
and show a prompt/alert) or generate a unique id (e.g., by appending a suffix)
before calling filesState.set(...) and activeFileState.set(...); ensure all
references to newFile use the final unique id so tab selection/close and VFS
sync remain unambiguous.
- Around line 696-705: The rename path only calls renameFileInVFS(fileName,
newName) and never updates the in-memory UI state (filesState and
activeFileState), causing the UI to show the old name; after
import("./vfs").then(vfs => vfs.renameFileInVFS(...)) also update the app state
by finding and replacing the filename in filesState (update the file object
key/name and any references) and, if activeFileState matches the old name, set
activeFileState to the trimmed newName; perform these updates after the VFS
promise resolves and handle failures by not mutating state if renameFileInVFS
throws or returns an error.
In `@Build/src/runner.ts`:
- Around line 82-98: The docContent assembly embeds raw css and js into <style>
and <script> tags (in the non-document fallback branch) which allows user
content containing literal "</style>" or "</script>" to break the HTML; update
the non-document fallback path that builds docContent to escape closing tags in
the css and js variables (same escaping used elsewhere in the codebase) before
inlining so that any occurrences of "</style>" and "</script>" are neutralized;
apply the same fix to the other fallback block referenced around the 118-123
range and ensure you update the variables used here (css, js,
consoleInterceptor, assembleFullHtml, hasDocTag) so both branches behave
consistently.
- Around line 51-57: getFileByExt only returns the first matching file, so
runCode's three lookups (for HTML, CSS, JS/MJS) drop additional files; change
the flow to collect and include all files of a given extension instead of one.
Update getFileByExt to return an array (e.g., getFilesByExt or similar) or add a
new getFilesByExt(files, ext) utility that finds all files where
f.name.split(".").pop() matches ext (optionally sort by name to preserve
deterministic order), then update runCode where it currently calls getFileByExt
(and the similar logic at the other referenced spots) to join/concatenate all
returned contents (with appropriate separators) for HTML, CSS and JS/MJS so
every source file is injected into the preview/export.
In `@Build/src/vfs.ts`:
- Around line 92-96: The catch block currently marks the VFS as ready even on
failure by calling _readyResolve() and setting _ready = true; instead, do not
resolve readiness on error—propagate the failure: remove the _ready = true and
_readyResolve() calls and reject the ready promise with the caught error (e.g.,
call an existing _readyReject(err) or create one when the readiness promise is
constructed), or rethrow the error so ready() returns a rejected promise; ensure
any consumers of ready() observe the failure rather than proceeding against a
broken VFS.
- Around line 138-148: The createFileInVFS function currently interpolates raw
names into `/sandbox/${name}` and appends entries blindly to filesState, so
validate and normalize the incoming name: reject or sanitize names containing
path separators (`/` or `\`) or parent segments (`..`) and use only the basename
(e.g., via path.basename-equivalent) to prevent sandbox escape; resolve the
target path and assert it is within `/sandbox` before writing. Also prevent
duplicate tabs by checking filesState for an existing FileTab.id/name and update
the entry instead of pushing a second one (keep existing content/language merge
logic), and apply the same validation/sanitization and duplicate-checking
pattern to the other VFS functions that write/read `/sandbox` (the same code
paths referenced in the review). Ensure you reference createFileInVFS,
filesState, activeFileState, FileTab, and getLanguageForExt when making the
changes.
- Around line 241-259: gitAddAll only stages files present in filesState so
deletions (removed by deleteFileInVFS which updates filesState) are never staged
and isomorphic-git requires git.remove for deletions; modify gitCommit (or
gitAddAll) to detect deleted paths and call git.remove for each before
committing. Specifically, after calling filesState.get() (or before commit
inside gitCommit) use isomorphic-git's statusMatrix() or git.status() to find
paths that are missing/deleted compared to HEAD, or track deletions from
deleteFileInVFS, then call git.remove({fs, dir: GIT_DIR, filepath:
<deletedPath>}) for each deleted file and only then proceed to call git.add for
existing files and git.commit; ensure functions referenced are gitAddAll,
gitCommit, deleteFileInVFS, filesState and use git.remove() to stage deletions.
---
Duplicate comments:
In `@Build/src/appState.ts`:
- Around line 22-44: Persisted values are used directly into initial signals
(filesInitial, activeFileInitial, darkModeInitial, autoRunInitial,
activeOutputInitial, splitSizesInitial, logFiltersInitial) which can break
startup if storage is corrupted; add small sanitizer/validator helpers (e.g.,
sanitizeFiles, sanitizeActiveFile, sanitizeBooleans, sanitizeSplitSizes,
sanitizeLogFilters, sanitizeActiveOutput) that confirm types/shape (files is an
array of objects with required id/content/title, splitSizes is two numbers in
0-100, logFilters is array of allowed strings, activeOutput is one of allowed
values, darkMode/autoRun are booleans) and return safe defaults (defaultFiles,
["log","error","warn","info"], [50,50], "preview", etc.) then wire the
initializers to use these sanitizers instead of directly using persistedState so
the app always gets validated shapes before applyStateSnapshot runs.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a846325-348b-45eb-88b0-11585272ccd8
⛔ Files ignored due to path filters (1)
Build/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
Build/eslint.config.mtsBuild/index.htmlBuild/package.jsonBuild/src/appState.tsBuild/src/biome.tsBuild/src/console.tsBuild/src/defaultContent.tsBuild/src/editor.tsBuild/src/git.worker.tsBuild/src/global.d.tsBuild/src/main.tsBuild/src/runner.tsBuild/src/types.tsBuild/src/ui.tsBuild/src/utils.tsBuild/src/vfs.tsBuild/styles/styles.cssBuild/tsconfig.jsonBuild/vite.config.mjs
✅ Files skipped from review due to trivial changes (1)
- Build/src/git.worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- Build/src/global.d.ts
- Build/tsconfig.json
- Build/src/utils.ts
- Build/src/console.ts
- Build/vite.config.mjs
| <div class="tab" data-output="console">Console</div> | ||
| </div> | ||
| <div class="output-content"> | ||
| <iframe id="preview" class="preview active"></iframe> |
There was a problem hiding this comment.
Sandbox the preview iframe.
This runner executes user code inside #preview. Without a sandbox attribute, previewed scripts can reach the parent app and its storage through window.parent, which breaks isolation.
🔒 Minimal hardening
- <iframe id="preview" class="preview active"></iframe>
+ <iframe id="preview" class="preview active" sandbox="allow-scripts"></iframe>🤖 Prompt for 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.
In `@Build/index.html` at line 59, The preview iframe (<iframe id="preview"
class="preview active">) lacks a sandbox attribute allowing previewed scripts to
access the parent; add a sandbox attribute to harden isolation (e.g.,
sandbox="allow-scripts" for running user scripts but preventing access to
window.parent/storage) on the iframe with id="preview" and avoid including
permissions like allow-same-origin or allow-top-navigation unless explicitly
required.
| "build": "vite build", | ||
| "build:single": "cross-env SINGLE_FILE=true npm run build", | ||
| "preview": "vite preview", | ||
| "lint": "eslint \"src/**/*.{ts,js}\" --ext .ts,.js || true", |
There was a problem hiding this comment.
Let the lint script fail normally.
|| true makes npm run lint exit successfully even when ESLint reports errors, so CI/local checks can't block regressions anymore. If you want a non-blocking helper, keep that as a separate script instead of weakening the main lint target.
Suggested fix
- "lint": "eslint \"src/**/*.{ts,js}\" --ext .ts,.js || true",
+ "lint": "eslint \"src/**/*.{ts,js}\" --ext .ts,.js",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "lint": "eslint \"src/**/*.{ts,js}\" --ext .ts,.js || true", | |
| "lint": "eslint \"src/**/*.{ts,js}\" --ext .ts,.js", |
🤖 Prompt for 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.
In `@Build/package.json` at line 12, The lint script in package.json currently
silences ESLint failures by appending "|| true" to the "lint" script; remove the
"|| true" from the "lint" script so ESLint errors cause a non-zero exit (i.e.,
change the "lint" script value to "eslint \"src/**/*.{ts,js}\" --ext .ts,.js"),
and if you need a non-blocking helper add a separate script like "lint:ci-safe"
or "lint:soft" that includes "|| true".
Comment on lines
+32
to
+55
| export async function initBiome(): Promise<void> { | ||
| if (initialized) return; | ||
|
|
||
| await initBiomeWasm(); | ||
|
|
||
| workspace = new Workspace(); | ||
|
|
||
| const openRes = workspace.openProject({ path: "", openUninitialized: true }); | ||
| projectKey = openRes.projectKey; | ||
|
|
||
| workspace.updateSettings({ | ||
| projectKey, | ||
| configuration: { | ||
| linter: { | ||
| enabled: true, | ||
| rules: { | ||
| recommended: true, | ||
| }, | ||
| }, | ||
| }, | ||
| workspaceDirectory: "./", | ||
| }); | ||
|
|
||
| initialized = true; |
There was a problem hiding this comment.
Make Biome initialization single-flight.
Concurrent lintWithBiome() calls can both enter initBiome() before initialized becomes true, which races workspace and projectKey assignment. Cache the in-flight promise and await it from all callers.
🛠️ Minimal pattern
let workspace: Workspace | null = null;
let projectKey: number | null = null;
let initialized = false;
+let initPromise: Promise<void> | null = null;
export async function initBiome(): Promise<void> {
if (initialized) return;
+ if (initPromise) return initPromise;
-
- await initBiomeWasm();
-
- workspace = new Workspace();
-
- const openRes = workspace.openProject({ path: "", openUninitialized: true });
- projectKey = openRes.projectKey;
-
- workspace.updateSettings({
- projectKey,
- configuration: {
- linter: {
- enabled: true,
- rules: {
- recommended: true,
- },
- },
- },
- workspaceDirectory: "./",
- });
-
- initialized = true;
+ initPromise = (async () => {
+ await initBiomeWasm();
+
+ workspace = new Workspace();
+
+ const openRes = workspace.openProject({ path: "", openUninitialized: true });
+ projectKey = openRes.projectKey;
+
+ workspace.updateSettings({
+ projectKey,
+ configuration: {
+ linter: {
+ enabled: true,
+ rules: {
+ recommended: true,
+ },
+ },
+ },
+ workspaceDirectory: "./",
+ });
+
+ initialized = true;
+ })();
+
+ try {
+ await initPromise;
+ } catch (err) {
+ initPromise = null;
+ throw err;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function initBiome(): Promise<void> { | |
| if (initialized) return; | |
| await initBiomeWasm(); | |
| workspace = new Workspace(); | |
| const openRes = workspace.openProject({ path: "", openUninitialized: true }); | |
| projectKey = openRes.projectKey; | |
| workspace.updateSettings({ | |
| projectKey, | |
| configuration: { | |
| linter: { | |
| enabled: true, | |
| rules: { | |
| recommended: true, | |
| }, | |
| }, | |
| }, | |
| workspaceDirectory: "./", | |
| }); | |
| initialized = true; | |
| let workspace: Workspace | null = null; | |
| let projectKey: number | null = null; | |
| let initialized = false; | |
| let initPromise: Promise<void> | null = null; | |
| export async function initBiome(): Promise<void> { | |
| if (initialized) return; | |
| if (initPromise) return initPromise; | |
| initPromise = (async () => { | |
| await initBiomeWasm(); | |
| workspace = new Workspace(); | |
| const openRes = workspace.openProject({ path: "", openUninitialized: true }); | |
| projectKey = openRes.projectKey; | |
| workspace.updateSettings({ | |
| projectKey, | |
| configuration: { | |
| linter: { | |
| enabled: true, | |
| rules: { | |
| recommended: true, | |
| }, | |
| }, | |
| }, | |
| workspaceDirectory: "./", | |
| }); | |
| initialized = true; | |
| })(); | |
| try { | |
| await initPromise; | |
| } catch (err) { | |
| initPromise = null; | |
| throw err; | |
| } | |
| } |
🤖 Prompt for 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.
In `@Build/src/biome.ts` around lines 32 - 55, Concurrent calls to initBiome can
race, so introduce an in-flight promise (e.g. initBiomePromise: Promise<void> |
null) and use it to ensure single-flight initialization: at the start of
initBiome check initialized and return, then if initBiomePromise exists await
and return; otherwise assign initBiomePromise = (async () => { await
initBiomeWasm(); workspace = new Workspace(); const openRes =
workspace.openProject({ path: "", openUninitialized: true }); projectKey =
openRes.projectKey; workspace.updateSettings({ projectKey, configuration: {
linter: { enabled: true, rules: { recommended: true } } }, workspaceDirectory:
"./", }); initialized = true; })(); await initBiomePromise; finally clear
initBiomePromise if you prefer (or leave until process end) so all callers await
the same promise and avoid racing workspace/projectKey assignment.
Comment on lines
+189
to
+207
| effect(() => { | ||
| const files = filesState.get(); | ||
| const activeId = activeFileState.get(); | ||
| const file = files.find((f) => f.id === activeId); | ||
| if (file && editor.view) { | ||
| const currentDoc = editor.view.state.doc.toString(); | ||
| if (currentDoc !== file.content && !_suppressContentUpdate) { | ||
| _suppressContentUpdate = true; | ||
| editor.view.dispatch({ | ||
| changes: { | ||
| from: 0, | ||
| to: editor.view.state.doc.length, | ||
| insert: file.content, | ||
| }, | ||
| }); | ||
| editor.view.dispatch({ | ||
| effects: _languageCompartment.reconfigure(getLanguageExtension(file.name)), | ||
| }); | ||
| _suppressContentUpdate = false; |
There was a problem hiding this comment.
Renaming the active file can leave the editor in the wrong language mode.
The _languageCompartment reconfigure is gated by currentDoc !== file.content. If the active file is renamed from script.js to styles.css without changing content, the mode never updates until the next edit.
♻️ Minimal fix
effect(() => {
const files = filesState.get();
const activeId = activeFileState.get();
const file = files.find((f) => f.id === activeId);
if (file && editor.view) {
+ editor.view.dispatch({
+ effects: _languageCompartment.reconfigure(getLanguageExtension(file.name)),
+ });
+
const currentDoc = editor.view.state.doc.toString();
if (currentDoc !== file.content && !_suppressContentUpdate) {
_suppressContentUpdate = true;
editor.view.dispatch({
changes: {
from: 0,
to: editor.view.state.doc.length,
insert: file.content,
},
});
- editor.view.dispatch({
- effects: _languageCompartment.reconfigure(getLanguageExtension(file.name)),
- });
_suppressContentUpdate = false;
}
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| effect(() => { | |
| const files = filesState.get(); | |
| const activeId = activeFileState.get(); | |
| const file = files.find((f) => f.id === activeId); | |
| if (file && editor.view) { | |
| const currentDoc = editor.view.state.doc.toString(); | |
| if (currentDoc !== file.content && !_suppressContentUpdate) { | |
| _suppressContentUpdate = true; | |
| editor.view.dispatch({ | |
| changes: { | |
| from: 0, | |
| to: editor.view.state.doc.length, | |
| insert: file.content, | |
| }, | |
| }); | |
| editor.view.dispatch({ | |
| effects: _languageCompartment.reconfigure(getLanguageExtension(file.name)), | |
| }); | |
| _suppressContentUpdate = false; | |
| effect(() => { | |
| const files = filesState.get(); | |
| const activeId = activeFileState.get(); | |
| const file = files.find((f) => f.id === activeId); | |
| if (file && editor.view) { | |
| editor.view.dispatch({ | |
| effects: _languageCompartment.reconfigure(getLanguageExtension(file.name)), | |
| }); | |
| const currentDoc = editor.view.state.doc.toString(); | |
| if (currentDoc !== file.content && !_suppressContentUpdate) { | |
| _suppressContentUpdate = true; | |
| editor.view.dispatch({ | |
| changes: { | |
| from: 0, | |
| to: editor.view.state.doc.length, | |
| insert: file.content, | |
| }, | |
| }); | |
| _suppressContentUpdate = false; | |
| } | |
| } | |
| }); |
🤖 Prompt for 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.
In `@Build/src/editor.ts` around lines 189 - 207, The effect currently only
reconfigures _languageCompartment when the file content changes, so renaming a
file (e.g., script.js → styles.css) with identical content won't update the
editor language; fix by tracking the active filename (e.g., add a closure
variable lastActiveFileName) and, inside effect() along with the existing
content check on editor.view, also compare file.name to lastActiveFileName and
dispatch the _languageCompartment.reconfigure(getLanguageExtension(file.name))
when the name changed (then set lastActiveFileName = file.name); ensure this
runs regardless of content equality but still respects the
_suppressContentUpdate flow and only triggers the reconfigure when the filename
actually changed.
Comment on lines
+188
to
+194
| const text = entries | ||
| .map((ev) => { | ||
| const timestamp = new Date(ev.timestamp).toLocaleTimeString(); | ||
| const message = ev.data | ||
| .map((d) => (typeof d === "object" ? JSON.stringify(d) : String(d))) | ||
| .join(" "); | ||
| return `${timestamp} ${message}`; |
There was a problem hiding this comment.
Guard console-copy serialization per value.
JSON.stringify(d) throws on circular objects and some DOM payloads. One such log entry will make “Copy All” fail for the entire console history.
🤖 Prompt for 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.
In `@Build/src/main.ts` around lines 188 - 194, The map that builds console text
uses JSON.stringify(d) directly and can throw on circular or DOM objects; update
the inner mapping for ev.data inside the entries.map callback so each value (d)
is serialized safely: wrap JSON.stringify(d) in a try/catch (or use a safe
replacer approach) and fall back to String(d) or a non-throwing representation
when stringify fails; ensure the change is applied to the message construction
inside the entries.map(ev => { ... }) block so a single bad value cannot break
the whole "Copy All" output.
Comment on lines
+51
to
+57
| function getFileByExt(files: FileTab[], ext: string): string { | ||
| const file = files.find((f) => { | ||
| const fext = f.name.split(".").pop()?.toLowerCase(); | ||
| return fext === ext; | ||
| }); | ||
| return file?.content ?? ""; | ||
| } |
There was a problem hiding this comment.
The runner drops every file after the first matching extension.
getFileByExt() plus the three lookups in runCode() only inject one HTML, one CSS, and one JS/MJS file. With the new multi-file tabs/VFS flow, any additional source files are silently ignored, so the preview no longer reflects the project the user is editing/exporting.
Also applies to: 63-66
🤖 Prompt for 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.
In `@Build/src/runner.ts` around lines 51 - 57, getFileByExt only returns the
first matching file, so runCode's three lookups (for HTML, CSS, JS/MJS) drop
additional files; change the flow to collect and include all files of a given
extension instead of one. Update getFileByExt to return an array (e.g.,
getFilesByExt or similar) or add a new getFilesByExt(files, ext) utility that
finds all files where f.name.split(".").pop() matches ext (optionally sort by
name to preserve deterministic order), then update runCode where it currently
calls getFileByExt (and the similar logic at the other referenced spots) to
join/concatenate all returned contents (with appropriate separators) for HTML,
CSS and JS/MJS so every source file is injected into the preview/export.
Comment on lines
+82
to
+98
| const docContent = hasDocTag | ||
| ? assembleFullHtml(html, css, js) | ||
| : [ | ||
| '<!DOCTYPE html><html><head><meta charset="UTF-8">', | ||
| "<style>", | ||
| css, | ||
| "</style>", | ||
| "<script>", | ||
| consoleInterceptor, | ||
| "</script>", | ||
| "</head><body>", | ||
| html, | ||
| "</body>", | ||
| "<script>", | ||
| js, | ||
| "</script></html>", | ||
| ].join(""); |
There was a problem hiding this comment.
Escape closing tags before inlining user CSS and JS.
Raw css and js are embedded inside <style> / <script> tags. If user code contains a literal </style> or </script>, the browser closes the tag early and the preview HTML becomes invalid.
Suggested fix
function assembleFullHtml(html: string, css: string, js: string): string {
+ const safeCss = css.replace(/<\/style/gi, "<\\/style");
+ const safeJs = js.replace(/<\/script/gi, "<\\/script");
const interceptorScript = `<script>${consoleInterceptor}<\/script>`;
- const styleTag = css.trim() ? `<style>${css}<\/style>` : "";
- const scriptTag = js.trim() ? `<script>${js}<\/script>` : "";
+ const styleTag = safeCss.trim() ? `<style>${safeCss}<\/style>` : "";
+ const scriptTag = safeJs.trim() ? `<script>${safeJs}<\/script>` : "";Apply the same escaped values in the non-document fallback path as well.
Also applies to: 118-123
🤖 Prompt for 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.
In `@Build/src/runner.ts` around lines 82 - 98, The docContent assembly embeds raw
css and js into <style> and <script> tags (in the non-document fallback branch)
which allows user content containing literal "</style>" or "</script>" to break
the HTML; update the non-document fallback path that builds docContent to escape
closing tags in the css and js variables (same escaping used elsewhere in the
codebase) before inlining so that any occurrences of "</style>" and "</script>"
are neutralized; apply the same fix to the other fallback block referenced
around the 118-123 range and ensure you update the variables used here (css, js,
consoleInterceptor, assembleFullHtml, hasDocTag) so both branches behave
consistently.
Comment on lines
+92
to
+96
| } catch (err) { | ||
| console.error("VFS init error:", err); | ||
| _ready = true; | ||
| _readyResolve(); | ||
| } |
There was a problem hiding this comment.
Don't report the VFS as ready after init failure.
This catch resolves ready() as success even when mount or git initialization failed. Callers then proceed against a broken filesystem instead of surfacing the boot error.
🤖 Prompt for 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.
In `@Build/src/vfs.ts` around lines 92 - 96, The catch block currently marks the
VFS as ready even on failure by calling _readyResolve() and setting _ready =
true; instead, do not resolve readiness on error—propagate the failure: remove
the _ready = true and _readyResolve() calls and reject the ready promise with
the caught error (e.g., call an existing _readyReject(err) or create one when
the readiness promise is constructed), or rethrow the error so ready() returns a
rejected promise; ensure any consumers of ready() observe the failure rather
than proceeding against a broken VFS.
Comment on lines
+138
to
+148
| export async function createFileInVFS( | ||
| name: string, | ||
| content = "", | ||
| ): Promise<void> { | ||
| const ext = name.split(".").pop()?.toLowerCase() || ""; | ||
| const language = getLanguageForExt(ext); | ||
| await fs.promises.writeFile(`/sandbox/${name}`, content, "utf-8"); | ||
| const files = filesState.get(); | ||
| const newFile: FileTab = { id: name, name, content, language }; | ||
| filesState.set([...files, newFile]); | ||
| activeFileState.set(name); |
There was a problem hiding this comment.
Validate file names before using them as VFS paths.
These APIs interpolate raw names directly into /sandbox/${...}. Inputs containing /, \, or .. can escape the sandbox path, and duplicate names can desynchronize state from disk by appending a second tab entry for the same file.
🧱 Minimal guard
+function assertValidFileName(name: string, existingNames: string[] = []): void {
+ if (!name || name.includes("/") || name.includes("\\") || name.includes("..")) {
+ throw new Error("Invalid file name");
+ }
+ if (existingNames.includes(name)) {
+ throw new Error("File already exists");
+ }
+}
+
export async function createFileInVFS(
name: string,
content = "",
): Promise<void> {
+ assertValidFileName(name, filesState.get().map((f) => f.name));
const ext = name.split(".").pop()?.toLowerCase() || "";
const language = getLanguageForExt(ext);
await fs.promises.writeFile(`/sandbox/${name}`, content, "utf-8");
const files = filesState.get();
const newFile: FileTab = { id: name, name, content, language };
filesState.set([...files, newFile]);
activeFileState.set(name);
}Also applies to: 151-155, 166-167
🤖 Prompt for 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.
In `@Build/src/vfs.ts` around lines 138 - 148, The createFileInVFS function
currently interpolates raw names into `/sandbox/${name}` and appends entries
blindly to filesState, so validate and normalize the incoming name: reject or
sanitize names containing path separators (`/` or `\`) or parent segments (`..`)
and use only the basename (e.g., via path.basename-equivalent) to prevent
sandbox escape; resolve the target path and assert it is within `/sandbox`
before writing. Also prevent duplicate tabs by checking filesState for an
existing FileTab.id/name and update the entry instead of pushing a second one
(keep existing content/language merge logic), and apply the same
validation/sanitization and duplicate-checking pattern to the other VFS
functions that write/read `/sandbox` (the same code paths referenced in the
review). Ensure you reference createFileInVFS, filesState, activeFileState,
FileTab, and getLanguageForExt when making the changes.
Comment on lines
+241
to
+259
| export async function gitAddAll(): Promise<void> { | ||
| const files = filesState.get(); | ||
| for (const file of files) { | ||
| try { | ||
| await git.add({ fs, dir: GIT_DIR, filepath: file.name }); | ||
| } catch (err) { | ||
| console.warn(`git add ${file.name} failed:`, err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export async function gitCommit(message: string): Promise<string> { | ||
| await gitAddAll(); | ||
| const sha = await git.commit({ | ||
| fs, | ||
| dir: GIT_DIR, | ||
| message, | ||
| author: GIT_AUTHOR, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For isomorphic-git 1.38.x, does git.add({ fs, dir, filepath })stage deletions, or do deleted files needgit.remove()/statusMatrix()handling beforecommit?
💡 Result:
For isomorphic-git 1.38.x, git.add({ fs, dir, filepath }) does NOT reliably “stage deletions” the way native git add does. If the file is deleted in the working tree, you should handle it explicitly with git.remove() (and typically decide based on git.status / statusMatrix). Evidence from isomorphic-git issues and docs: - A reported bug/behavior shows that running git.add on a removed file can throw an error, and the suggested workaround is to use git.remove for deleted files instead of git.add [1]. - The isomorphic-git add docs describe adding a file to the git index (staging area) but do not state that it stages removals automatically; the existence of a separate remove command plus the workaround indicates deletions are not handled by add alone in this library version [2]. - A common implementation pattern is: check git.status for *deleted, and if so call git.remove({ fs, dir, filepath }) rather than git.add [1]. So, before commit, you generally need deletion-aware staging logic: - For added/modified paths: call git.add(...). - For deleted paths: call git.remove(...) (decided via git.status or statusMatrix). Practical decision snippet (from the issue’s workaround): - compute status; if status indicates deleted, call git.remove, else call git.add [1]. If you skip the explicit deletion handling and only call git.add for deleted paths, you risk either errors or the deletion not being staged, leading to incorrect commit results.
Citations:
- 1: Running
git.addon removed file throws an error isomorphic-git/isomorphic-git#1099 - 2: https://isomorphic-git.org/docs/en/add
🏁 Script executed:
# First, locate and examine the vfs.ts file
find . -name "vfs.ts" -type fRepository: HTMLToolkit/HTMLRunner
Length of output: 85
🏁 Script executed:
# Read the specified lines and surrounding context to verify the code
sed -n '230,270p' Build/src/vfs.tsRepository: HTMLToolkit/HTMLRunner
Length of output: 983
🏁 Script executed:
# Search for deleteFileInVFS implementation
rg "deleteFileInVFS" -A 10 -B 2Repository: HTMLToolkit/HTMLRunner
Length of output: 1883
🏁 Script executed:
# Search for git.remove usage in the codebase
rg "git\.remove" -B 2 -A 2Repository: HTMLToolkit/HTMLRunner
Length of output: 48
🏁 Script executed:
# Check how filesState is defined and if it tracks deleted files
rg "filesState" -B 3 -A 3 | head -80Repository: HTMLToolkit/HTMLRunner
Length of output: 4350
Deleted files are never staged before commit.
gitAddAll() only processes files currently in filesState. When deleteFileInVFS() removes a file from disk, it also removes it from filesState with filesState.set(updated). Since the deleted file is no longer tracked in filesState, it never reaches git.add(), and the deletion is not staged.
Additionally, isomorphic-git 1.38.x does not handle deletions via git.add()—deleted files must be explicitly staged with git.remove(). This is never called in the codebase, so file deletions are completely ignored by gitCommit().
Handle deletion tracking explicitly, either by detecting file removals via git.status() / statusMatrix() and calling git.remove() for deleted paths before gitCommit(), or by maintaining a separate record of deleted files.
🤖 Prompt for 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.
In `@Build/src/vfs.ts` around lines 241 - 259, gitAddAll only stages files present
in filesState so deletions (removed by deleteFileInVFS which updates filesState)
are never staged and isomorphic-git requires git.remove for deletions; modify
gitCommit (or gitAddAll) to detect deleted paths and call git.remove for each
before committing. Specifically, after calling filesState.get() (or before
commit inside gitCommit) use isomorphic-git's statusMatrix() or git.status() to
find paths that are missing/deleted compared to HEAD, or track deletions from
deleteFileInVFS, then call git.remove({fs, dir: GIT_DIR, filepath:
<deletedPath>}) for each deleted file and only then proceed to call git.add for
existing files and git.commit; ensure functions referenced are gitAddAll,
gitCommit, deleteFileInVFS, filesState and use git.remove() to stage deletions.
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
2 participants
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.
Summary by CodeRabbit
New Features
Refactor
Chores / Removed
Documentation