Create trace language interactive sandbox (React + Vite, GitHub Pages CI/CD)#1
Conversation
Co-authored-by: bluehexagons <6945365+bluehexagons@users.noreply.github.com>
Co-authored-by: bluehexagons <6945365+bluehexagons@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a React + Vite single-page “trace” language playground (targeted for GitHub Pages) and vendors the trace interpreter with browser-focused adaptations, plus CI/CD and interpreter unit tests.
Changes:
- Introduce the playground UI (editor, examples list, args input, output/logs panel) and styling.
- Vendor and adapt the trace interpreter for browser usage, plus add Vitest coverage for core language behaviors.
- Add project/tooling scaffolding (TypeScript configs, ESLint, Vite/Vitest config) and GitHub Actions CI + Pages deployment workflows.
Reviewed changes
Copilot reviewed 17 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Sets Vitest runtime environment. |
| vite.config.ts | Configures React plugin and GitHub Pages base path. |
| tsconfig.node.json | TypeScript config for Node-side tooling (Vite config). |
| tsconfig.json | TS project references setup. |
| tsconfig.app.json | TS config for app source (React + DOM). |
| src/main.tsx | React app entrypoint mounting <App />. |
| src/lib/trace.ts | Vendored trace interpreter adapted for browser + fixes. |
| src/index.css | Full dark-mode playground UI styling. |
| src/examples.ts | Curated example scripts/metadata for the sidebar. |
| src/assets/react.svg | React asset included by template. |
| src/tests/trace.test.ts | Vitest unit tests for interpreter behavior. |
| src/App.tsx | Playground UI + run/echo output wiring. |
| src/App.css | Notes that styles live in index.css. |
| public/vite.svg | Vite public asset. |
| package.json | Defines dependencies and dev scripts (build/lint/test). |
| package-lock.json | Locks dependency tree for reproducible installs. |
| index.html | App shell HTML + metadata/favicon wiring. |
| eslint.config.js | ESLint flat config for TS/React hooks/refresh. |
| README.md | Project docs, features, and CI/deploy instructions. |
| .gitignore | Updates ignores for node/Vite project artifacts. |
| .github/workflows/deploy.yml | GitHub Pages build + deploy workflow. |
| .github/workflows/ci.yml | CI workflow for lint/build/test on pushes + PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleKeyDown = useCallback( | ||
| (e: React.KeyboardEvent<HTMLTextAreaElement>) => { | ||
| if ((e.ctrlKey || e.metaKey) && e.key === 'Enter') { |
There was a problem hiding this comment.
React.KeyboardEvent is referenced but React itself isn't imported, so TypeScript will error with "Cannot find name 'React'". Import the type (e.g., import type { KeyboardEvent } from 'react') or import React and use React.KeyboardEvent.
There was a problem hiding this comment.
Fixed in 06c5182 — added import type { KeyboardEvent } from 'react' and replaced React.KeyboardEvent with KeyboardEvent.
| t.logger = (msg: string) => logs.push(msg) | ||
| // Suppress error logger to capture cleanly | ||
| t.errorLogger = (msg: string) => logs.push(`⚠ ${msg}`) |
There was a problem hiding this comment.
t.logger / t.errorLogger are invoked with multiple arguments by the Trace interpreter (e.g., this.logger('token …', varName, value)), but these overrides only accept a single msg parameter so most details are dropped. Capture/format all arguments (e.g., join them into a string) so the console panel shows meaningful output.
| t.logger = (msg: string) => logs.push(msg) | |
| // Suppress error logger to capture cleanly | |
| t.errorLogger = (msg: string) => logs.push(`⚠ ${msg}`) | |
| t.logger = (...msgs: unknown[]) => { | |
| const line = msgs.map(m => String(m)).join(' ') | |
| logs.push(line) | |
| } | |
| // Suppress error logger to capture cleanly | |
| t.errorLogger = (...msgs: unknown[]) => { | |
| const line = msgs.map(m => String(m)).join(' ') | |
| logs.push(`⚠ ${line}`) | |
| } |
There was a problem hiding this comment.
Fixed in 06c5182 — both t.logger and t.errorLogger now accept ...msgs: unknown[] and join all arguments with a space before pushing to the log array.
| if (/[0-9]/.test(t.string[1])) { | ||
| this.logger('token ' + f.i + ':', '&' + t.string.substring(1), (f.stack as Float64Array)[parseInt(t.string.substring(1), 10)]) | ||
| } else { | ||
| const v = vars.get(t.string.substring(1)) as number | ||
| this.logger('token ' + f.i + ':', '&' + v, (f.stack as Float64Array)[v]) |
There was a problem hiding this comment.
In the @...@ (beep) logging case, the code indexes into f.stack without checking for null. For scripts with stackSize <= 0 (no args) that still use @&...@, this will throw at runtime. Guard for f.stack === null (or treat missing stack as 0) before indexing.
| if (/[0-9]/.test(t.string[1])) { | |
| this.logger('token ' + f.i + ':', '&' + t.string.substring(1), (f.stack as Float64Array)[parseInt(t.string.substring(1), 10)]) | |
| } else { | |
| const v = vars.get(t.string.substring(1)) as number | |
| this.logger('token ' + f.i + ':', '&' + v, (f.stack as Float64Array)[v]) | |
| const stack = f.stack as Float64Array | null | |
| if (/[0-9]/.test(t.string[1])) { | |
| const index = parseInt(t.string.substring(1), 10) | |
| const stackValue = | |
| stack && index >= 0 && index < stack.length ? stack[index] : undefined | |
| this.logger('token ' + f.i + ':', '&' + t.string.substring(1), stackValue) | |
| } else { | |
| const v = vars.get(t.string.substring(1)) as number | |
| const stackValue = | |
| stack && v >= 0 && v < stack.length ? stack[v] : undefined | |
| this.logger('token ' + f.i + ':', '&' + v, stackValue) |
There was a problem hiding this comment.
Fixed in 06c5182 — the beep handler now holds the stack as Float64Array | null and guards both index paths (numeric and variable-based pointer) before accessing it, returning undefined when the stack is absent or the index is out of bounds.
|
@copilot apply changes based on the comments in this thread |
…beep handler Co-authored-by: bluehexagons <6945365+bluehexagons@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 18-23: The workflow currently references third-party actions with
mutable tags (actions/checkout@v4 and actions/setup-node@v4); replace those
mutable refs with the corresponding immutable 40-character commit SHAs to harden
supply-chain guarantees (e.g., change uses: actions/checkout@v4 and uses:
actions/setup-node@v4 to uses: actions/checkout@<full-commit-sha> and uses:
actions/setup-node@<full-commit-sha>), ensuring you fetch the exact commit SHAs
from the respective GitHub action repositories and update both occurrences in
the CI workflow.
In `@src/__tests__/trace.test.ts`:
- Around line 93-100: The test assigns a logger override to the Trace instance
with the first parameter typed as number, but Trace calls its logger with a
string (e.g. 'token ' + f.i + ':'), causing a type mismatch; update the test's
logger signature used on the result of Trace.parse (t.logger) to accept a string
as the first parameter (e.g., change the first parameter type from number to
string) so it matches how Trace invokes the logger.
In `@src/App.tsx`:
- Around line 118-121: Add explicit type="button" attributes to the plain
<button> elements in App.tsx to prevent accidental form submission: update the
button that uses onClick={() => loadExample(i)} (the element with
className={`example-btn${selectedExample === i ? ' active' : ''}`}) and the
other button around line 172 to include type="button". This ensures these
buttons are not treated as submit buttons by default and follows
accessibility/best-practice guidelines.
In `@src/index.css`:
- Around line 74-83: The .main CSS rule contains a duplicate gap property;
remove the redundant gap: 0 declaration so only gap: 1.25rem remains in the
.main block (locate the .main selector in the diff and delete the gap: 0 line to
satisfy declaration-block-no-duplicate-properties).
In `@src/lib/trace.ts`:
- Around line 562-570: The run method's signature lacks an explicit return type
even though it may return null in some paths; update the run declaration (the
run(...) function) to explicitly return number | null to reflect that it can
return a numeric result or null (even if value is normally assigned from f.value
after the initial frame push). Ensure the signature change propagates to any
related type annotations or overloads so callers see the correct return type.
- Around line 401-420: The cache lookup in parse(s) uses preprocessed as key but
discards the original params because the local params array is empty; update the
caching so cached entries include params (extend the CachedScript shape to store
params) and when retrieving from cachedScripts in parse use cached.params (not
the local params) to construct the Trace; alternatively, include a
params-signature in the cache key (based on the parameter list) so scripts with
the same body but different parameter declarations are distinct. Ensure changes
touch the parse method, cachedScripts population site, and the CachedScript type
to store and return params alongside tokens and stackSize.
- Around line 577-587: The local variable `stack` is incorrectly cast to
Float64Array even though `StackFrame` may return null when `stackSize` is 0;
change the assignment so `stack` is nullable (e.g., Float64Array | null) or only
obtain `f.stack` inside the `if (stackSize > 0)` block, and update uses (the
writes to stack[0] and the loop) to operate on the non-null stack; this fixes
the potential null dereference related to `stackSize`, `f` (StackFrame) and the
`stack` variable without relying on an unsafe cast.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/vite.svgis excluded by!**/*.svgsrc/assets/react.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
.github/workflows/ci.yml.github/workflows/deploy.yml.gitignoreREADME.mdeslint.config.jsindex.htmlpackage.jsonsrc/App.csssrc/App.tsxsrc/__tests__/trace.test.tssrc/examples.tssrc/index.csssrc/lib/trace.tssrc/main.tsxtsconfig.app.jsontsconfig.jsontsconfig.node.jsonvite.config.tsvitest.config.ts
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| cache: npm |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check repository structure and locate workflow files
ls -la .github/workflows/ 2>/dev/null || echo "No workflows directory found"Repository: bluehexagons/trace-sandbox
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Run the verification script from the review comment
# to check for mutable vs pinned action refs
set -euo pipefail
echo "=== Checking for mutable action refs (should be empty) ==="
rg -n '^\s*-\s*uses:\s*[^@]+@(?![0-9a-fA-F]{40}\b)\S+' .github/workflows/ || echo "No matches found"
echo ""
echo "=== Checking for pinned action refs (40-char SHAs) ==="
rg -n '^\s*-\s*uses:\s*[^@]+@[0-9a-fA-F]{40}\b' .github/workflows/ || echo "No matches found"
echo ""
echo "=== All action uses statements in workflows ==="
rg -n '^\s*-\s*uses:' .github/workflows/ || echo "No matches found"Repository: bluehexagons/trace-sandbox
Length of output: 816
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: bluehexagons/trace-sandbox
Length of output: 856
🏁 Script executed:
#!/bin/bash
# Simpler verification: find all action refs in ci.yml
echo "=== All action uses in ci.yml ==="
grep -n "uses:" .github/workflows/ci.ymlRepository: bluehexagons/trace-sandbox
Length of output: 179
Pin GitHub Actions to immutable commit SHAs.
Using mutable refs (@v4) for third-party actions can silently change behavior and weakens CI supply-chain guarantees. Pin full 40-char commit SHAs instead (e.g., actions/checkout@e2f20e631e6baf43c68e6dd3cdf1430cbf8099e7).
🔧 Proposed hardening diff
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<40-char-commit-sha>
- - uses: actions/setup-node@v4
+ - uses: actions/setup-node@<40-char-commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 18 - 23, The workflow currently
references third-party actions with mutable tags (actions/checkout@v4 and
actions/setup-node@v4); replace those mutable refs with the corresponding
immutable 40-character commit SHAs to harden supply-chain guarantees (e.g.,
change uses: actions/checkout@v4 and uses: actions/setup-node@v4 to uses:
actions/checkout@<full-commit-sha> and uses:
actions/setup-node@<full-commit-sha>), ensuring you fetch the exact commit SHAs
from the respective GitHub action repositories and update both occurrences in
the CI workflow.
| it('collects echo (@…@) output via logger override', () => { | ||
| const logs: string[] = [] | ||
| const t = Trace.parse('x = 42; @=x@; x') | ||
| t.logger = (_token: number, ...args: unknown[]) => | ||
| logs.push(args.map(String).join(' ')) | ||
| t.run() | ||
| expect(logs.some(l => l.includes('42'))).toBe(true) | ||
| }) |
There was a problem hiding this comment.
Logger override parameter type mismatch.
The logger override on line 96 types the first parameter as number:
t.logger = (_token: number, ...args: unknown[]) =>However, looking at trace.ts lines 634, 642, and 644, the logger is called with a string as the first argument (e.g., 'token ' + f.i + ':'). The parameter should be typed as string:
🔧 Proposed fix
- t.logger = (_token: number, ...args: unknown[]) =>
+ t.logger = (...args: unknown[]) =>
logs.push(args.map(String).join(' '))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/trace.test.ts` around lines 93 - 100, The test assigns a logger
override to the Trace instance with the first parameter typed as number, but
Trace calls its logger with a string (e.g. 'token ' + f.i + ':'), causing a type
mismatch; update the test's logger signature used on the result of Trace.parse
(t.logger) to accept a string as the first parameter (e.g., change the first
parameter type from number to string) so it matches how Trace invokes the
logger.
| <button | ||
| className={`example-btn${selectedExample === i ? ' active' : ''}`} | ||
| onClick={() => loadExample(i)} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file size and structure
wc -l src/App.tsx
echo "---"
# Read lines around 118-121
sed -n '115,125p' src/App.tsx | cat -n
echo "---"
# Read lines around 172
sed -n '170,175p' src/App.tsx | cat -nRepository: bluehexagons/trace-sandbox
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Check for lint configuration files
fd -e "biome.json" -e ".eslintrc" -e "eslint.config.js" -e ".eslintrc.json" -e ".eslintrc.js" .
echo "---"
# Also check package.json for lint scripts
cat package.json | grep -A 5 -B 5 "lint\|eslint\|biome" || echo "No lint config found in package.json"Repository: bluehexagons/trace-sandbox
Length of output: 828
🏁 Script executed:
#!/bin/bash
# Look for eslint config files
find . -maxdepth 2 -name "eslint.config.*" -o -name ".eslintrc*"
echo "---"
# Also check for eslint.config.js specifically
ls -la | grep -i eslintRepository: bluehexagons/trace-sandbox
Length of output: 160
🏁 Script executed:
#!/bin/bash
cat -n eslint.config.jsRepository: bluehexagons/trace-sandbox
Length of output: 846
🏁 Script executed:
#!/bin/bash
# Verify the first button (lines 118-121)
sed -n '118,125p' src/App.tsx | cat -n
echo "---"
# Verify the second button (line 172)
sed -n '170,176p' src/App.tsx | cat -nRepository: bluehexagons/trace-sandbox
Length of output: 723
Add explicit type="button" to both buttons.
These buttons should have an explicit type to prevent unintended form submission behavior and follow accessibility best practices.
Proposed fix
<button
+ type="button"
className={`example-btn${selectedExample === i ? ' active' : ''}`}
onClick={() => loadExample(i)}
>- <button className="run-btn" onClick={runCode}>
+ <button type="button" className="run-btn" onClick={runCode}>
▶ Run
</button>Also applies to: 172-172
🧰 Tools
🪛 Biome (2.4.4)
[error] 118-121: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.tsx` around lines 118 - 121, Add explicit type="button" attributes to
the plain <button> elements in App.tsx to prevent accidental form submission:
update the button that uses onClick={() => loadExample(i)} (the element with
className={`example-btn${selectedExample === i ? ' active' : ''}`}) and the
other button around line 172 to include type="button". This ensures these
buttons are not treated as submit buttons by default and follows
accessibility/best-practice guidelines.
| .main { | ||
| flex: 1; | ||
| display: flex; | ||
| gap: 0; | ||
| max-width: 1400px; | ||
| width: 100%; | ||
| margin: 0 auto; | ||
| padding: 1.5rem; | ||
| gap: 1.25rem; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
if [ -f src/index.css ]; then
echo "=== File found. Reading lines 70-85 ==="
sed -n '70,85p' src/index.css | cat -n
echo ""
echo "=== Full .main block ==="
rg -n -A12 '^\s*\.main\s*\{' src/index.css
else
echo "File not found at src/index.css"
find . -name "index.css" -type f
fiRepository: bluehexagons/trace-sandbox
Length of output: 1144
🏁 Script executed:
# Check for stylelint configuration in the project
echo "=== Looking for stylelint config ==="
find . -maxdepth 2 -name ".stylelintrc*" -o -name "stylelint.config.*" | head -20Repository: bluehexagons/trace-sandbox
Length of output: 126
🏁 Script executed:
# Check stylelint configuration to confirm the rule is active
cat .stylelintrc.jsonRepository: bluehexagons/trace-sandbox
Length of output: 1076
Remove duplicate gap declaration in .main.
The .main block has gap: 0; on line 77 which is immediately overridden by gap: 1.25rem; on line 82. This violates the declaration-block-no-duplicate-properties rule enforced by stylelint and should be removed.
Proposed fix
.main {
flex: 1;
display: flex;
- gap: 0;
max-width: 1400px;
width: 100%;
margin: 0 auto;
padding: 1.5rem;
gap: 1.25rem;
}📝 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.
| .main { | |
| flex: 1; | |
| display: flex; | |
| gap: 0; | |
| max-width: 1400px; | |
| width: 100%; | |
| margin: 0 auto; | |
| padding: 1.5rem; | |
| gap: 1.25rem; | |
| } | |
| .main { | |
| flex: 1; | |
| display: flex; | |
| max-width: 1400px; | |
| width: 100%; | |
| margin: 0 auto; | |
| padding: 1.5rem; | |
| gap: 1.25rem; | |
| } |
🧰 Tools
🪛 Stylelint (17.3.0)
[error] 77-77: Unexpected duplicate "gap" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.css` around lines 74 - 83, The .main CSS rule contains a duplicate
gap property; remove the redundant gap: 0 declaration so only gap: 1.25rem
remains in the .main block (locate the .main selector in the diff and delete the
gap: 0 line to satisfy declaration-block-no-duplicate-properties).
| static parse(s: string) { | ||
| const preprocessed = s.replace(/#[^\n]*/g, '').replace(/\s/g, '') | ||
| let stringLeft = preprocessed | ||
| const tokens: TraceToken[] = [] | ||
| let findOperator = false | ||
| const loi = [0] // last operator index was too long to keep typing | ||
| let groupLevel = 0 | ||
| let stackSize = 0 | ||
| let params: string[] = [] | ||
| const groupLevels: number[] = [] | ||
| let match: (RegExpExecArray | null) = null | ||
|
|
||
| if (cachedScripts.has(preprocessed)) { | ||
| const cached = cachedScripts.get(preprocessed) as CachedScript | ||
| return new Trace(preprocessed, cached.tokens, params, cached.stackSize) | ||
| } | ||
|
|
||
| if (stringLeft.length === 0) { | ||
| return new Trace(preprocessed, tokens, params, stackSize) | ||
| } |
There was a problem hiding this comment.
Consider cache key collision with different params.
The cache uses preprocessed (the script body without whitespace/comments) as the key, but the cached entry stores tokens and stackSize without params. When a cached script is retrieved (lines 413-416), params is always empty from the local initialization, not from the original parse.
This could cause issues if the same script body is parsed with different parameter declarations, though this seems unlikely given parameter syntax is part of the script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/trace.ts` around lines 401 - 420, The cache lookup in parse(s) uses
preprocessed as key but discards the original params because the local params
array is empty; update the caching so cached entries include params (extend the
CachedScript shape to store params) and when retrieving from cachedScripts in
parse use cached.params (not the local params) to construct the Trace;
alternatively, include a params-signature in the cache key (based on the
parameter list) so scripts with the same body but different parameter
declarations are distinct. Ensure changes touch the parse method, cachedScripts
population site, and the CachedScript type to store and return params alongside
tokens and stackSize.
| run( | ||
| args: number[] = [], | ||
| variables: ({[s: string]: number} | null) = null, | ||
| vars: (Map<string, number> | null) = null, | ||
| functions: (Map<string, Trace> | null) = null, | ||
| rand: () => number = Math.random, | ||
| executionLimit = 1000, | ||
| executionStart: number = performance.now() | ||
| ) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return type could be number | null but is unspecified.
The run() method can return null if this.tokens is empty and no frames process (though unlikely given the initial frame push). The return type should be explicitly declared to match the implementation.
However, in practice, the initial frame is always pushed (line 589) and value gets assigned from f.value (line 965), so this is a minor typing clarity issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/trace.ts` around lines 562 - 570, The run method's signature lacks an
explicit return type even though it may return null in some paths; update the
run declaration (the run(...) function) to explicitly return number | null to
reflect that it can return a numeric result or null (even if value is normally
assigned from f.value after the initial frame push). Ensure the signature change
propagates to any related type annotations or overloads so callers see the
correct return type.
| const stackSize = this.stackSize === -1 ? args.length + 1 : this.stackSize | ||
| let f: StackFrame = new StackFrame(this.tokens, stackSize) | ||
| const stack = f.stack as Float64Array | ||
|
|
||
| if (stackSize > 0) { | ||
| stack[0] = stackSize - 1 | ||
|
|
||
| for (let i = 0; i < stackSize && i < args.length; i++) { | ||
| stack[i + 1] = +args[i] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential null dereference when stackSize is 0.
When stackSize is 0, f.stack is null (from StackFrame constructor line 314), but stack is cast to Float64Array on line 579. While the access on lines 582-586 is guarded by if (stackSize > 0), the cast itself is misleading and could cause issues if future code uses stack outside this guard.
Consider using a conditional assignment or keeping the nullable type:
- const stack = f.stack as Float64Array
+ const stack = f.stack
if (stackSize > 0) {
- stack[0] = stackSize - 1
+ stack![0] = stackSize - 1
for (let i = 0; i < stackSize && i < args.length; i++) {
- stack[i + 1] = +args[i]
+ stack![i + 1] = +args[i]
}
}📝 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.
| const stackSize = this.stackSize === -1 ? args.length + 1 : this.stackSize | |
| let f: StackFrame = new StackFrame(this.tokens, stackSize) | |
| const stack = f.stack as Float64Array | |
| if (stackSize > 0) { | |
| stack[0] = stackSize - 1 | |
| for (let i = 0; i < stackSize && i < args.length; i++) { | |
| stack[i + 1] = +args[i] | |
| } | |
| } | |
| const stackSize = this.stackSize === -1 ? args.length + 1 : this.stackSize | |
| let f: StackFrame = new StackFrame(this.tokens, stackSize) | |
| const stack = f.stack | |
| if (stackSize > 0) { | |
| stack![0] = stackSize - 1 | |
| for (let i = 0; i < stackSize && i < args.length; i++) { | |
| stack![i + 1] = +args[i] | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/trace.ts` around lines 577 - 587, The local variable `stack` is
incorrectly cast to Float64Array even though `StackFrame` may return null when
`stackSize` is 0; change the assignment so `stack` is nullable (e.g.,
Float64Array | null) or only obtain `f.stack` inside the `if (stackSize > 0)`
block, and update uses (the writes to stack[0] and the loop) to operate on the
non-null stack; this fixes the potential null dereference related to
`stackSize`, `f` (StackFrame) and the `stack` variable without relying on an
unsafe cast.
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: bluehexagons <6945365+bluehexagons@users.noreply.github.com>
Applied the review-thread fixes in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
actions/setup-node uses node-version: 20, but Vite 7.x requires Node ^20.19.0 || >=22.12.0 (per package-lock). Deploy builds can fail if 20 resolves to an older 20.x on the runner. Pin Node to 20.19.0 (or use 22) to satisfy the engine constraint.
| node-version: 20 | |
| node-version: 20.19.0 |
| export default defineConfig({ | ||
| plugins: [react()], | ||
| base: '/trace-sandbox/', | ||
| }) |
There was a problem hiding this comment.
base: '/trace-sandbox/' hard-codes the GitHub Pages subpath, which typically forces local dev to be served under that subpath and makes forks/custom domains harder. Consider deriving base from the Vite command ('/' for dev, '/trace-sandbox/' for build) or from an env var so the app runs correctly in both local dev and Pages.
| export default defineConfig({ | |
| plugins: [react()], | |
| base: '/trace-sandbox/', | |
| }) | |
| export default defineConfig(({ command }) => ({ | |
| plugins: [react()], | |
| base: command === 'build' ? '/trace-sandbox/' : '/', | |
| })) |
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/trace-sandbox/vite.svg" /> |
There was a problem hiding this comment.
The favicon path is hard-coded to /trace-sandbox/vite.svg, which will 404 in local dev (and if the repo name changes). Prefer using a base-aware path (e.g., /vite.svg and let Vite rewrite with base, or a relative ./vite.svg) so it works in both dev and GitHub Pages builds.
| <link rel="icon" type="image/svg+xml" href="/trace-sandbox/vite.svg" /> | |
| <link rel="icon" type="image/svg+xml" href="./vite.svg" /> |
| "noFallthroughCasesInSwitch": true, | ||
| "noUncheckedSideEffectImports": true | ||
| }, | ||
| "include": ["vite.config.ts"] |
There was a problem hiding this comment.
tsconfig.node.json only includes vite.config.ts, but this repo also has vitest.config.ts. Since the build runs tsc -b, vitest.config.ts won’t be type-checked, so config type errors could slip through CI. Add vitest.config.ts to include (or a glob for *.config.ts).
| "include": ["vite.config.ts"] | |
| "include": ["*.config.ts"] |
| const cachedScripts = new Map<string, CachedScript>() | ||
| const stdlib = new Map<string, Trace>() | ||
|
|
There was a problem hiding this comment.
cachedScripts is a module-level Map with no size bound/eviction, so a sandbox session that runs many unique scripts can grow memory indefinitely. Consider adding a max size with simple eviction (e.g., FIFO/LRU) or making caching optional to avoid unbounded memory growth in the browser.
| const cachedScripts = new Map<string, CachedScript>() | |
| const stdlib = new Map<string, Trace>() | |
| const CACHED_SCRIPTS_MAX_SIZE = 100 | |
| const STDLIB_MAX_SIZE = 100 | |
| class BoundedMap<K, V> extends Map<K, V> { | |
| private readonly maxSize: number | |
| constructor(maxSize: number) { | |
| super() | |
| this.maxSize = maxSize | |
| } | |
| set(key: K, value: V): this { | |
| // Evict the oldest entry when adding a new key beyond the max size. | |
| if (this.maxSize > 0 && this.size >= this.maxSize && !this.has(key)) { | |
| const firstKey = this.keys().next().value as K | undefined | |
| if (firstKey !== undefined) { | |
| this.delete(firstKey) | |
| } | |
| } | |
| return super.set(key, value) | |
| } | |
| } | |
| const cachedScripts: Map<string, CachedScript> = new BoundedMap<string, CachedScript>(CACHED_SCRIPTS_MAX_SIZE) | |
| const stdlib: Map<string, Trace> = new BoundedMap<string, Trace>(STDLIB_MAX_SIZE) |
|
|
||
| - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
actions/setup-node uses node-version: 20, but Vite 7.x requires Node ^20.19.0 || >=22.12.0 (per package-lock). This workflow may fail on runners that resolve 20 to an earlier 20.x. Pin Node to 20.19.0 (or use 22) to match Vite’s engine requirement.
| node-version: 20 | |
| node-version: 20.19.0 |
|
@copilot apply changes based on the comments in this thread |
…n, tsconfig, BoundedMap Co-authored-by: bluehexagons <6945365+bluehexagons@users.noreply.github.com>
Applied all 6 fixes from review thread in
|
permissions: contents: read(CodeQL finding)React.KeyboardEventtype — importKeyboardEventfromreactdirectlyunknown[]args and join them@&...@) handler in trace.tsnode-versionto20.19.0in both CI and deploy workflows (Vite 7.x engine requirement)baseconditional:/for dev,/trace-sandbox/for build./vite.svg(base-relative) instead of hard-coded/trace-sandbox/vite.svgtsconfig.node.jsoninclude to*.config.tsto covervitest.config.tsBoundedMapwith FIFO eviction (max 100 entries) forcachedScriptsto prevent unbounded memory growthOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Documentation
Tests