Conversation
Member
tomusdrw
commented
Feb 5, 2026
tomusdrw
commented
- load ecalli trace file
- fixes
- more fixes
✅ Deploy Preview for pvm-debugger ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces comprehensive host-call trace support to the PVM debugger, including a new host-call trace parsing library (JIP-6 format), trace configuration UI, trace-aware program loading, and enhanced memory editing with pending writes visualization. It refactors host-call state management, adds an application-level error boundary, removes service-ID tracking from web-worker infrastructure, and updates all host-call handlers to support restart actions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Fix all issues with AI agents
In `@src/components/HostCallDialog/handlers/HostCallActionButtons.tsx`:
- Around line 15-20: The restart button in HostCallActionButtons lacks an
explicit type, which can cause unintended form submission when this component is
used inside a form; update the <button> element that calls onRestart (the
"restart" control) to include type="button" so it does not act as a submit
button, keeping the existing className, onClick handler, and disabled prop
unchanged.
In `@src/components/HostCallDialog/index.tsx`:
- Around line 106-108: currentState may be undefined so accessing
currentState.regs and currentState.gas can throw; update the expressions that
compute finalRegs and currentGas/finalGas (the lines assigning finalRegs,
currentGas, finalGas in HostCallDialog/index.tsx) to use optional chaining or
nullish coalescing when reading currentState (e.g., currentState?.regs and
currentState?.gas) so you fall back to DEFAULT_REGS and DEFAULT_GAS safely, and
keep the pendingRegs/pendingGas precedence logic intact.
In `@src/components/ProgramLoader/Links.tsx`:
- Around line 68-81: The new external links in ProgramLoader's Links.tsx (the
<a> elements that use target="_blank" around the ExternalLink icon and the inner
JIP-6 text) are missing rel attributes; update both anchor tags to include
rel="noopener noreferrer" to prevent reverse-tabnabbing and ensure safe external
navigation. Locate the anchors in the Links.tsx component (the <a
href="https://...JIP-6.md" target="_blank"> wrapping ExternalLink and the inner
<a className="text-brand-dark..." href="https://...JIP-6.md">) and add
rel="noopener noreferrer" to each.
In `@src/components/ProgramLoader/Loader.tsx`:
- Around line 64-74: The handleFileUpload callback currently calls parseTrace
and getTraceSummary without protection; wrap the parseTrace/getTraceSummary
calls in a try-catch inside handleFileUpload so malformed trace data doesn't
crash the component — on success keep the existing setTraceContent(trace) and
setTraceSummary(getTraceSummary(...)), and on error call setTraceContent(null)
and setTraceSummary(null) (and optionally log the error or surface a
user-friendly message) so the component remains stable; reference
handleFileUpload, parseTrace, getTraceSummary, setTraceContent, setTraceSummary,
and setProgramLoad when applying the change.
In `@src/components/ProgramLoader/loadingUtils.test.ts`:
- Line 5: Rename the test file from loadingUtils.test.ts to
loading-utils.test.ts and update all references/imports to that file throughout
the repo (e.g., any import statements, test index files, or CI/test-glob
patterns that explicitly reference the old name); ensure the test suite name
("describe('loadingUtils', ...)") can remain unchanged or be adjusted for
clarity but is not required to match the filename.
In `@src/components/ProgramLoader/loadingUtils.ts`:
- Around line 48-66: The buildSpiArgsFromMemoryWrites function can allocate a
huge Uint8Array for sparse far-apart addresses; add a hard cap (e.g. const
MAX_SPI_ARG_SIZE = 10 * 1024 * 1024) and check totalSize after computing
lastAddr - firstAddr, and if totalSize > MAX_SPI_ARG_SIZE throw a clear
RangeError/ValidationError (include totalSize and MAX in the message) to fail
fast instead of allocating; apply the same guard to the other similar routine
handling memoryWrites (the other block around the 83-90 logic) and use the same
MAX constant and error handling so both places consistently refuse oversized
spans.
- Around line 68-73: The function tryParseTraceFile uses parseTrace but doesn't
check for parse errors before reading prelude fields; update tryParseTraceFile
to detect parse failures from parseTrace (e.g., an errors or success flag on the
returned trace) and short‑circuit (return null or a LoadFileResult error) when
parseTrace indicates errors so you never access trace.prelude.program with an
invalid parse; locate tryParseTraceFile and parseTrace usages and add a guard
that rejects traces with parse errors before any further processing of
ExpectedState or building the LoadFileResult.
- Around line 134-142: The current mapping uses memory.map to create one
PageMapItem per chunk which misses pages when a chunk spans multiple 4KiB pages;
change the construction of pageMap so that for each chunk (from the memory
array) you compute the firstPage = Math.floor(chunk.address / 4096) and lastPage
= Math.floor((chunk.address + (chunk.length ?? 0) - 1) / 4096) and push a
PageMapItem for every page index in that range with address = (pageIndex << 12)
(or pageIndex*4096) & ~0xfff, length = 4096 and "is-writable" = true; then keep
the existing dedupe into uniquePageMap (filter by address) to remove duplicates.
In `@src/hooks/useDebuggerActions.ts`:
- Around line 81-83: The code reads workers[0]?.memoryRanges into memoryRanges
and calls syncMemoryRangeAllWorkers which will throw if undefined; change to
default to an empty array and only call syncMemoryRangeAllWorkers when the array
is non-empty (e.g., const memoryRanges = workers[0]?.memoryRanges ?? []; if
(memoryRanges.length) syncMemoryRangeAllWorkers(...)). Apply the same
guard/default pattern wherever memoryRanges is read and passed to
syncMemoryRangeAllWorkers in this module (the other occurrences near the other
syncMemoryRangeAllWorkers calls).
- Around line 84-90: The current loop calls destroyWorker for each WorkerState
which broadcasts Commands.UNLOAD to all workers repeatedly; change this by
adding a single "destroyAllWorkers" flow or modify destroyWorker so it targets
only the specific worker id: update destroyWorker (or add destroyAllWorkers) to
accept a flag or separate path that sends Commands.UNLOAD only to the intended
worker socket instead of broadcasting to all, then replace the
Promise.all(workers.map(...dispatch(destroyWorker...))) calls with a single call
to the new destroy-all helper or call destroyWorker per-id in its non-broadcast
mode; ensure recreateWorker usage remains unchanged and keep references to
WorkerState, recreateWorker, destroyWorker and Commands.UNLOAD when making the
change.
In `@src/lib/hostCallTrace.test.ts`:
- Line 6: The test file is named in camelCase; rename the file from
src/lib/hostCallTrace.test.ts to kebab-case host-call-trace.test.ts and update
any imports or references (e.g., test runner configs or other files that import
this test) to point to host-call-trace.test.ts; also update any describe/title
strings if you want consistency (the test's describe("hostCallTrace", ...) can
remain or be changed to "host-call-trace" if desired) and run the test suite to
ensure no broken references remain.
In `@src/lib/hostCallTrace.ts`:
- Around line 1-9: The file name hostCallTrace.ts uses camelCase but the repo
requires kebab-case; rename the file to host-call-trace.ts and update all
imports referencing hostCallTrace (e.g., any import statements that import the
module providing "ecalli / PVM IO Trace Parser and Types" or types from that
module) to use the new kebab-case name; ensure build and tests are updated to
reference host-call-trace.ts and run a full repo search/replace for
"hostCallTrace" to catch any remaining references.
- Around line 205-235: The parseMemreadLine and parseMemwriteLine functions
currently ignore mismatches between the declared len and the actual decoded byte
count; update both functions to call parseHexBytes(dataHex), verify that the
returned byte array length equals parseInt(lenStr, 10), and throw a descriptive
Error (including the parsed address, declared len, and actual data length) if
they differ so callers cannot silently proceed with inconsistent memory sizes;
keep the existing parseAddress/parseHexBytes usage and ensure the thrown error
is clear for debugging.
- Around line 618-636: findHostCallEntry currently picks the first host call
with a matching pc (from trace.hostCalls.slice(indexInTrace)) which can return a
false mismatch when a later entry has the correct hostCallIndex and acceptable
gas; change the selection logic so after building matchingEntries (pc match and
hc.gas <= gas) you first try to find an entry whose hostCallIndex equals the
provided hostCallIndex and gas <= gas (e.g. matchingEntries.find(hc =>
hc.hostCallIndex === hostCallIndex && hc.gas <= gas)), use that as entry if
found, otherwise fall back to matchingEntries[0]; keep the rest of the
mismatches handling unchanged but reference the chosen entry variable (entry) as
before.
- Around line 337-468: parseTrace currently consumes raw trace text without Zod
validation; add a Zod schema for the overall ParsedTrace shape (including
prelude.program, prelude.start, initialMemoryWrites, hostCalls entries with
memoryReads/writes/registerWrites/gasAfter, termination, and errors) and
validate the input structure before doing line-by-line parsing. Implement the
schema near the top of this module, call schema.safeParse or parse inside
parseTrace at the start (or validate a preliminary parsed object after minimal
parsing) and on validation failure return or populate result.errors with the zod
error messages (or throw a clear error) so downstream code gets a typed,
validated ParsedTrace; reference parseTrace, ParsedTrace, and the
result.hostCalls/currentHostCall shapes when adding the schema and handling
failures.
In `@src/store/debugger/debuggerSlice.ts`:
- Around line 225-237: The reducer setHostCallsTrace currently clears
state.hostCallsTrace when action.payload is null/empty but leaves
state.nextHostCallIndex and state.pendingHostCall unchanged, causing stale UI
state; update setHostCallsTrace to also reset state.nextHostCallIndex to 0 and
state.pendingHostCall to null whenever action.payload is null or an
empty/whitespace string so that hostCallsTrace, nextHostCallIndex, and
pendingHostCall are consistent.
- Around line 225-233: The reducer setHostCallsTrace currently stores parsed
traces without validating them; call the existing validateTrace(action.payload)
(or run the payload through the shared Zod schema) before calling parseTrace,
and if validation reports errors throw an Error (or reject the payload) so
invalid trace content is not persisted; only after validateTrace succeeds
proceed to parseTrace and set state.hostCallsTrace with rawContent and parsed.
Ensure this change applies to the setHostCallsTrace reducer so Loader.tsx
dispatches cannot write invalid traces, keeping TraceConfigDialog.tsx validation
redundant but harmless.
In `@src/store/workers/workersSlice.ts`:
- Around line 296-342: In the auto-continue path you need to advance each
worker's host-call index and actually await resuming execution instead of
fire-and-forget; after computing newRegs/newGas/memoryEdits and after
successfully applying SET_STATE/SET_MEMORY via asyncWorkerPostMessage, increment
the host-call index on the state you pass to
dispatch(setWorkerCurrentState(...)) (or dispatch a dedicated advance-host-call
action) so the worker's currentState reflects the progressed host-call, and then
await runAllWorkers() (use await dispatch(runAllWorkers()) if it's a thunk)
rather than calling it without awaiting, so trace matching and resume errors are
not lost.
In `@src/utils/loggerService.tsx`:
- Around line 19-20: The startup log uses a template literal with embedded
newline/indentation and awkward phrasing; replace the template literal in the
console.info call in src/utils/loggerService.tsx with a plain single-line string
and fix grammar (e.g., "Logger initialized — watch console for logs."), removing
the extra whitespace/newline so the message is concise and properly punctuated.
In `@tests/trace-loading.spec.ts`:
- Around line 13-16: The test uses a hard-coded tracePath and directly calls
fileInput.setInputFiles, which bypasses shared Playwright fixtures; replace the
direct path usage with the shared fixture from tests/utils (the fixture that
supplies the deterministic file or FilePayload) and pass that fixture into the
test instead of constructing tracePath, then use that fixture value with the
existing fileInput locator and setInputFiles call (update the test signature to
accept the fixture and substitute tracePath in the setInputFiles invocation);
apply the same change to the other occurrence around lines 45-48.
🧹 Nitpick comments (12)
tests/trace-loading.spec.ts (1)
18-27: Replace fixed sleeps with condition-based waits.
waitForTimeoutadds flakiness; theexpect(...).toBeVisible/Enabledcalls already include waits. At lines 18–27, remove the 2000ms sleep before checking button visibility. At lines 49–65, remove both the 2000ms and 3000ms sleeps, and replace the 10000ms sleep after Run with a condition-based wait for either the host-call dialog or program-status visibility.Additionally, reuse the
openDebuggerfixture fromtests/utils/actions.tsinstead of duplicating thepage.gotoandlocalStorage.clearcalls.♻️ Suggested cleanup
- await page.waitForTimeout(2000); ... - await page.waitForTimeout(3000); ... - await page.waitForTimeout(10000);Replace the 10000ms sleep with:
await Promise.race([ page.locator('[role="dialog"]').waitFor({ state: 'visible' }), expect(page.getByTestId("program-status")).toBeVisible() ]).catch(() => {});src/lib/hostCallTrace.test.ts (2)
1-4: Avoid__dirnameif Vitest runs in ESM mode.If tests execute as ESM,
__dirnamecan be undefined. Consider usingimport.meta.url+fileURLToPathinstead.🔧 Safer ESM path resolution
-import * as path from "path"; +import { fileURLToPath } from "url"; @@ - const tracePath = path.join(__dirname, "../../io-trace-output.log"); + const tracePath = fileURLToPath(new URL("../../io-trace-output.log", import.meta.url));Also applies to: 67-68
69-72: Use a committed fixture to keep this test deterministic.Right now the test silently skips when the file is missing. Consider adding a small fixture trace under version control so the test always exercises the parsing path.
src/components/ProgramLoader/loadingUtils.test.ts (2)
1-3: Avoid__dirnameif Vitest runs in ESM mode.If tests execute as ESM,
__dirnamecan be undefined. Preferimport.meta.url+fileURLToPath.🔧 Safer ESM path resolution
-import * as path from "path"; +import { fileURLToPath } from "url"; @@ - const tracePath = path.join(__dirname, "../../../io-trace-output.log"); + const tracePath = fileURLToPath(new URL("../../../io-trace-output.log", import.meta.url));Also applies to: 8-9
10-13: Use a committed fixture to keep this test deterministic.Right now the test silently skips when the file is missing. Consider checking in a small fixture trace instead so the test always exercises the parsing path.
src/components/ErrorBoundary.tsx (2)
60-65: Unnecessary state reset before page reload.The
setStatecall on line 62 has no effect sincewindow.location.reload()immediately follows on line 64, discarding any in-memory state changes.🔧 Suggested simplification
<button onClick={() => { - this.setState({ hasError: false, error: null, errorInfo: null }); window.location.hash = "#/load"; window.location.reload(); }}
24-28: Consider using the existing logger service for consistency.The codebase has a
logger.errorservice (seesrc/utils/loggerService.tsx) that provides centralized error handling. Using it here would maintain consistency across the application.src/components/ProgramLoader/Loader.tsx (1)
195-195: Misleading variable name.
hasTraceWithEntrypointsimply checks iftraceContent !== null, but the name suggests it verifies that the trace contains entrypoint information. Consider renaming tohasTracefor clarity.✏️ Suggested rename
- const hasTraceWithEntrypoint = traceContent !== null; + const hasTrace = traceContent !== null;Then update usages on lines 198 and 311.
src/components/DebuggerSettings/TraceConfigDialog.tsx (2)
176-192: Consider using dropzone'sopen()method instead of creating a manual input element.The dropzone hook provides an
open()function that can trigger the file picker. This would be more consistent and avoid duplicating the file accept logic.♻️ Suggested refactor using dropzone open()
- const { getRootProps, getInputProps, isDragActive } = useDropzone({ + const { getRootProps, getInputProps, isDragActive, open } = useDropzone({ onDrop: handleFileDrop, accept: { "text/plain": [".txt", ".trace", ".log"] }, noClick: true, multiple: false, });Then update the button:
<Button variant="outline" size="sm" - onClick={() => { - const input = document.createElement("input"); - input.type = "file"; - input.accept = ".txt,.trace,.log"; - input.onchange = (e) => { - const file = (e.target as HTMLInputElement).files?.[0]; - if (file) handleFileDrop([file]); - }; - input.click(); - }} + onClick={open} >
128-129: Unsafe type assertion on FileReader result.The
e.target?.resultis cast tostringwithout verification. WhilereadAsTextshould return a string, defensive coding would check the type.🛡️ Suggested defensive check
reader.onload = (e) => { - const content = e.target?.result as string; + const content = e.target?.result; + if (typeof content !== "string") return; const { stripped, programLine: pl } = stripProgramLine(content);src/components/HostCallDialog/MemoryEditor.tsx (1)
130-130: Potential unnecessary memory reloads whenpendingWritesreference changes.Adding
pendingWritesto the dependency array causesloadMemoryto re-run whenever the array reference changes, even if the content is identical. For large memory regions, this could cause noticeable re-fetching.Consider memoizing
pendingWritesin the parent component or using a stable reference comparison if this becomes a performance concern.src/components/HostCallDialog/DefaultHostCallContent.tsx (1)
36-53: Cap log memory reads to avoid oversized allocations.
Lengths come from registers; a defensive maximum prevents large reads or UI stalls if the target/message lengths are unexpectedly huge.Suggested guard
const decoder = new TextDecoder("utf8"); +const MAX_LOG_BYTES = 16_384; ... - const targetLength = Number(currentRegs[9] ?? 0n); + const targetLength = Math.max(0, Math.min(Number(currentRegs[9] ?? 0n), MAX_LOG_BYTES)); ... - const msgLength = Number(currentRegs[11] ?? 0n); + const msgLength = Math.max(0, Math.min(Number(currentRegs[11] ?? 0n), MAX_LOG_BYTES));Also applies to: 111-154, 251-269
Fixes per CodeRabbit review: - Rename hostCallTrace files to kebab-case - Rename loadingUtils files to kebab-case - Add type="button" to restart button - Add optional chaining for currentState access - Add rel="noopener noreferrer" to external links - Add try-catch for trace parsing - Add SPI args size guard (16MB limit) - Reject traces with parse errors - Fix pageMap to handle multi-page memory chunks - Guard against undefined memoryRanges - Fix destroyWorker broadcast issue - Validate memread/memwrite length - Improve findHostCallEntry matching logic - Reset state when clearing trace - Validate trace before parsing - Await auto-continue execution - Fix logger startup message - Replace fixed waits in tests with condition-based waits
- Fix destroyWorker to always return ID for cleanup - Add retry loop to initAllWorkers for race condition robustness - Disable UI (Loader, Examples, DebuggerControls) when PVM is loading - Improve selectPVM test helper to avoid unnecessary toggling - Reload page in openDebugger to ensure clean state
Member
Author
|
@coderabbitai please review whether your previous comments are addressed and resolve if that's the case. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 21839 Let me check for unresolved conversations more thoroughly: 🏁 Script executed: Length of output: 91091
🟠 Major Issues (10 unresolved)
🟡 Minor Issues (8 unresolved)
✅ Outdated (2 comments - code changed)
Would you like me to help address any of these issues, or would you prefer to tackle them yourself first before I resolve the comments? |
Major fixes: - Links.tsx: Add rel="noopener noreferrer" to external links for security - host-call-trace.ts: Add Zod validation for trace structure - debuggerSlice.ts: Add input validation in setHostCallsTrace reducer - workersSlice.ts: Fix auto-continue to advance host-call index and await resume - useDebuggerActions.ts: Add guard for undefined memoryRanges - findHostCallEntry: Prefer exact host call index matches with early return Minor fixes: - trace-loading.spec.ts: Use shared Playwright fixtures - Create tests/utils/fixtures.ts for shared test utilities
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DebuggerControlls/index.tsx (1)
163-174:⚠️ Potential issue | 🟡 MinorAdd
noWorkerscheck to Restart button disabled condition.The Restart button at line 170 is missing the
noWorkersguard that other action buttons (Play, Run, Step) include. WhilerestartProgramhas defensive coding with optional chaining onworkers[0]?.memoryRanges, it accesses the workers array directly at line 86. For consistency across the component and to prevent edge cases, disable the button when no workers are available.🔧 Suggested change
- disabled={!pvmInitialized || isProgramEditMode} + disabled={!pvmInitialized || isProgramEditMode || noWorkers}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DebuggerControlls/index.tsx` around lines 163 - 174, The Restart button's disabled condition needs to include the existing noWorkers guard like the other controls; update the Button that calls debuggerActions.restartProgram(initialState) (in DebuggerControlls/index.tsx) to add the noWorkers boolean to its disabled expression so the button is disabled when noWorkers is true, keeping the existing pvmInitialized and isProgramEditMode checks intact.
🧹 Nitpick comments (5)
src/utils/loggerService.tsx (1)
1-58: Consider renaming file to kebab-case for consistency.The filename
loggerService.tsxuses camelCase, but the coding guidelines specify kebab-case for TypeScript files (e.g.,logger-service.tsx). This is a pre-existing issue, so it can be deferred.As per coding guidelines: "Use descriptive filenames in kebab-case (e.g.,
memory-range-panel.tsx)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/loggerService.tsx` around lines 1 - 58, Rename the file from loggerService.tsx to kebab-case (logger-service.tsx) to match naming guidelines; perform a git mv to preserve history, update all imports that reference the module (look for usages of the exported symbols Logger and logger and any imports that reference "loggerService") to the new filename, run type-check/build to ensure no broken imports remain, and adjust any tooling/exports that assume the old name.src/components/ProgramLoader/Links.tsx (1)
73-98: New JIP-6 trace file link section looks good overall.The structure follows the existing pattern for other list items, and the main external link (lines 75-79) correctly includes security attributes.
One minor inconsistency: the inner text link at line 87-91 has
rel="noopener noreferrer"withouttarget="_blank", while other inner text links (lines 21, 41-46, 62-67) have neither attribute. Therelwithouttarget="_blank"is harmless but unnecessary.🔧 Optional: Remove unnecessary rel or add target="_blank" for consistency
Either remove the unnecessary
relto match other inner links:<a className="text-brand-dark dark:text-brand underline" href="https://github.com/polkadot-fellows/JIPs/blob/main/JIP-6.md" - rel="noopener noreferrer" >Or add
target="_blank"if you want this link to also open in a new tab:<a className="text-brand-dark dark:text-brand underline" href="https://github.com/polkadot-fellows/JIPs/blob/main/JIP-6.md" + target="_blank" rel="noopener noreferrer" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ProgramLoader/Links.tsx` around lines 73 - 98, In Links.tsx the inner anchor element wrapping the "specification" text includes rel="noopener noreferrer" but lacks target="_blank", creating an inconsistent pattern with other inner links; update that anchor (the <a> around "specification") to either remove the rel attribute to match the other internal-style links or add target="_blank" alongside rel="noopener noreferrer" if you want it to open in a new tab—pick the approach consistent with other anchors in this component and apply the same change to keep behavior uniform.package.json (1)
41-42: Consider moving build-time dependencies todevDependencies.
caniuse-liteandbaseline-browser-mappingare build-time tools used byautoprefixerandbrowserslistfor browser compatibility checks. They are not imported anywhere in the runtime code. Placing them independenciesrather thandevDependencieswill include them in production installs.Move them to
devDependencies:♻️ Suggested change
"dependencies": { ... - "baseline-browser-mapping": "^2.9.19", - "caniuse-lite": "^1.0.30001769", ... }, "devDependencies": { ... + "baseline-browser-mapping": "^2.9.19", + "caniuse-lite": "^1.0.30001769", ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 41 - 42, Move the build-only packages "caniuse-lite" and "baseline-browser-mapping" out of runtime dependencies and into devDependencies in package.json: remove their entries from the dependencies object and add equivalent entries under devDependencies so they are installed only for development/build environments (keep the same version specifiers). Ensure package.json remains valid JSON and run a quick install (npm/yarn) to update lockfiles.tests/utils/actions.ts (1)
25-44: Useincludes()for simpler, more readable substring matching.
Case-insensitive substring matching is better expressed withincludes()than constructing a regex pattern. This is clearer, faster, and more appropriate for text matching in UI automation.🔧 Suggested change
- const text = await option.textContent(); - const isTarget = text?.match(new RegExp(pvmType, "i")); + const text = (await option.textContent()) ?? ""; + const isTarget = text.toLowerCase().includes(pvmType.toLowerCase());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/actions.ts` around lines 25 - 44, Replace the regex-based substring check with a case-insensitive includes() check: when iterating the locator allPvmOptions and computing isTarget from option.textContent(), convert the text and the pvmType to the same case (e.g., toLowerCase()) and use textLower.includes(pvmTypeLower) instead of text?.match(new RegExp(pvmType, "i")); update the isTarget computation and leave selection logic (isSelected/option.click()) unchanged so behavior is identical but simpler and faster.src/lib/host-call-trace.test.ts (1)
1-2: Rename this test file to use the*.spec.tssuffix.The repository learnings call for unit specs to use
*.spec.ts(x); please rename tohost-call-trace.spec.tsand update any references.Based on learnings: Unit specs should use the
*.spec.ts(x)suffix and stay beside their modules; Match test file names to the unit under test (e.g.,store/pvmSlice.spec.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/host-call-trace.test.ts` around lines 1 - 2, Rename the test file from host-call-trace.test.ts to host-call-trace.spec.ts to follow the repository convention; update any imports or references that import this test filename (e.g., test runner configs or CI patterns) and ensure the tests still import parseTrace and getTraceSummary from ./host-call-trace; keep the spec colocated with the module and run the test suite to confirm there are no import or path breakages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 106-108: The trustedDependencies array currently lists "@swc/core"
but that package is not present in dependencies or devDependencies and is not in
the active dependency tree; either remove the "@swc/core" entry from the
trustedDependencies array or add a short comment justifying why
trustedDependencies contains "@swc/core" (e.g., required for an external build
step or CI hook) and ensure a corresponding dependency entry exists in
dependencies/devDependencies if it truly is needed; update the package.json by
editing the "trustedDependencies" entry to reflect the chosen action.
In `@src/components/DebuggerSettings/TraceConfigDialog.tsx`:
- Around line 73-88: The current validateCurrentContent callback uses
validateTrace (which only checks parse errors) causing mismatches with the
store's setHostCallsTrace that uses validateTraceContent; update
validateCurrentContent to import and call validateTraceContent instead of
validateTrace, map its returned errors into setValidationErrors the same way
(e.g., `errors.map(e => \`Line ${e.lineNumber}: ${e.message}\`)`), and only call
parseTrace and setSummary(getTraceSummary(...)) when validateTraceContent
returns no errors so the dialog's preview validation matches the store
validation.
In `@src/components/ErrorBoundary.tsx`:
- Line 1: Rename the component file from ErrorBoundary.tsx to kebab-case (e.g.,
error-boundary.tsx) and update all imports that reference the module (e.g., any
import of ErrorBoundary or from "src/components/ErrorBoundary" to
"src/components/error-boundary"); inside the file ensure the exported symbol
(class or component named ErrorBoundary) remains unchanged, update any barrel
exports or index.ts under src/components to re-export from "./error-boundary",
and run a quick global search for "ErrorBoundary" and "components/ErrorBoundary"
to update remaining import paths accordingly.
In `@src/components/HostCallDialog/DefaultHostCallContent.tsx`:
- Around line 184-188: pendingMemoryWrites is recreated each render causing
MemoryEditor (which depends on pendingWrites and calls loadMemory) to reload
memory unnecessarily; import useMemo and wrap the pendingMemoryWrites assignment
in useMemo using traceEntry?.memoryWrites as the dependency so the array
reference only changes when the underlying memoryWrites change, e.g., memoize
the mapping that builds pendingMemoryWrites (referencing pendingMemoryWrites,
traceEntry?.memoryWrites, MemoryEditor, and loadMemory) to prevent redundant I/O
and avoid overwriting in-progress edits.
- Around line 111-145: The loadLogMessage async in the useEffect reads untrusted
targetLength and msgLength directly from currentState.regs; clamp both lengths
to a safe maximum (e.g., 4096 bytes) and ensure they are non-negative before
calling readMemory, then use the clamped values for readMemory calls and any
length checks so setLogTarget/setLogMessage only receive at-most 4KB strings;
update the variables targetLength and msgLength (or introduce
clampedTargetLength/clampedMsgLength) inside loadLogMessage and use those in the
existing readMemory and decode code paths.
In `@src/components/HostCallDialog/index.tsx`:
- Around line 70-99: When traceEntry changes in HostCallDialog, rebase the
pending state onto the current Redux state to avoid leaking manual edits: in the
useEffect that depends on traceEntry, first initialize pendingRegs from
currentState?.regs (or a copy of it) via setPendingRegs and pendingGas from
currentState?.gas via setPendingGas (and clear pendingMemoryEdits via
setPendingMemoryEdits([])) before applying traceEntry.registerWrites and
traceEntry.gasAfter and mapping traceEntry.memoryWrites; use the existing
setters (setPendingRegs, setPendingGas, setPendingMemoryEdits) and update the
code that currently applies registerWrites/memoryWrites so it mutates a fresh
copy based on currentState instead of the previous pending state.
In `@src/components/HostCallDialog/MemoryEditor.tsx`:
- Around line 57-67: The useEffect that syncs initialAddress/initialLength
doesn't handle when those props become undefined, leaving stale values; update
the effect in MemoryEditor (the useEffect that calls setAddressInput,
setAddress, setLengthInput, setLength) to explicitly reset to your default
values when initialAddress or initialLength are undefined (e.g., setAddressInput
to an empty string or default hex, setAddress to null/0 as appropriate, and
setLengthInput/ setLength to their defaults) so the editor clears when the trace
is cleared.
In `@src/components/ProgramLoader/loading-utils.test.ts`:
- Around line 8-13: The test currently no-ops if tracePath (constructed with
path.join and checked via fs.existsSync) is missing; either add and commit a
stable fixture file io-trace-output.log into the test resources so the
assertions that exercise loadFileFromUint8Array always run, or change the test
to throw/assert failure when fs.existsSync(tracePath) is false (e.g., replace
the early return with a failing assertion) so CI fails and alerts you that the
fixture is absent; update references in loading-utils.test.ts around tracePath,
fs.existsSync, and the code that calls loadFileFromUint8Array accordingly.
In `@src/components/ProgramLoader/loading-utils.ts`:
- Around line 74-138: Add Zod validation for the output of parseTrace at the
start of tryParseTraceFile: define a Zod schema that requires trace.prelude with
program (string), optional start with pc and gas (numbers), and
initialMemoryWrites (array or expected shape) and validate parseTrace(content)
against it before accessing trace.prelude.*; in tryParseTraceFile call
schema.parse or safeParse on the parse result, log or return null on validation
failure, and then continue using trace.prelude.program,
trace.prelude.initialMemoryWrites and trace.prelude.start as typed values
(update any subsequent error messages to reference the validated structure).
In `@tests/trace-loading.spec.ts`:
- Around line 2-12: The failing test occurs because the fixture file referenced
by FIXTURES.IO_TRACE_OUTPUT (resolved via getFixturePath) does not exist when
setInputFiles(tracePath) is called; add the missing tests/io-trace-output.log
fixture into the repo (or generate it during test setup) and ensure it is not
ignored by git — either commit it with git add -f, update .gitignore to allow
this specific fixture, or implement fixture creation in the test harness so
getFixturePath and page.locator('input[type="file"]') / setInputFiles(tracePath)
receive a valid path at runtime.
---
Outside diff comments:
In `@src/components/DebuggerControlls/index.tsx`:
- Around line 163-174: The Restart button's disabled condition needs to include
the existing noWorkers guard like the other controls; update the Button that
calls debuggerActions.restartProgram(initialState) (in
DebuggerControlls/index.tsx) to add the noWorkers boolean to its disabled
expression so the button is disabled when noWorkers is true, keeping the
existing pvmInitialized and isProgramEditMode checks intact.
---
Duplicate comments:
In `@src/hooks/useDebuggerActions.ts`:
- Around line 85-90: The looped destroy path causes N× UNLOAD broadcasts—modify
the worker teardown so UNLOAD targets only the specific worker or provide a
non-broadcast option and centralize full teardown: update destroyWorker in
workersSlice (and any caller usages like restartProgram and handlePvmTypeChange)
so it sends UNLOAD only to the worker id passed (or accept a flag
broadcast:boolean default false), then implement a single destroyAllWorkers
routine that iterates workers and calls destroyWorker for each (or toggles
broadcast=true once) so restartProgram and handlePvmTypeChange call that single
destroy-all path instead of destroying twice; ensure recreateWorker usage
remains unchanged.
---
Nitpick comments:
In `@package.json`:
- Around line 41-42: Move the build-only packages "caniuse-lite" and
"baseline-browser-mapping" out of runtime dependencies and into devDependencies
in package.json: remove their entries from the dependencies object and add
equivalent entries under devDependencies so they are installed only for
development/build environments (keep the same version specifiers). Ensure
package.json remains valid JSON and run a quick install (npm/yarn) to update
lockfiles.
In `@src/components/ProgramLoader/Links.tsx`:
- Around line 73-98: In Links.tsx the inner anchor element wrapping the
"specification" text includes rel="noopener noreferrer" but lacks
target="_blank", creating an inconsistent pattern with other inner links; update
that anchor (the <a> around "specification") to either remove the rel attribute
to match the other internal-style links or add target="_blank" alongside
rel="noopener noreferrer" if you want it to open in a new tab—pick the approach
consistent with other anchors in this component and apply the same change to
keep behavior uniform.
In `@src/lib/host-call-trace.test.ts`:
- Around line 1-2: Rename the test file from host-call-trace.test.ts to
host-call-trace.spec.ts to follow the repository convention; update any imports
or references that import this test filename (e.g., test runner configs or CI
patterns) and ensure the tests still import parseTrace and getTraceSummary from
./host-call-trace; keep the spec colocated with the module and run the test
suite to confirm there are no import or path breakages.
In `@src/utils/loggerService.tsx`:
- Around line 1-58: Rename the file from loggerService.tsx to kebab-case
(logger-service.tsx) to match naming guidelines; perform a git mv to preserve
history, update all imports that reference the module (look for usages of the
exported symbols Logger and logger and any imports that reference
"loggerService") to the new filename, run type-check/build to ensure no broken
imports remain, and adjust any tooling/exports that assume the old name.
In `@tests/utils/actions.ts`:
- Around line 25-44: Replace the regex-based substring check with a
case-insensitive includes() check: when iterating the locator allPvmOptions and
computing isTarget from option.textContent(), convert the text and the pvmType
to the same case (e.g., toLowerCase()) and use textLower.includes(pvmTypeLower)
instead of text?.match(new RegExp(pvmType, "i")); update the isTarget
computation and leave selection logic (isSelected/option.click()) unchanged so
behavior is identical but simpler and faster.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.