Conversation
📝 WalkthroughWalkthroughThis PR adds a browser-based Python test execution system: new Web Workers and Pyodide integration, a modular AnswerBrowserExercise UI with hooks and adapters, utilities for building in-browser test scripts, state/interface updates for browser tests, and grading/public-spec adaptations. Also includes build/script changes for injecting Pyodide version and localization strings. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Main as Main Thread / UI
participant Worker as Web Worker
participant Pyodide as Pyodide (WASM)
User->>Main: Trigger Run/Test with user code
Main->>Main: Build/prepare test script (adapter)
Main->>Worker: postMessage({ script })
Worker->>Worker: ensure Pyodide loaded
Worker->>Pyodide: load/indexURL & configure stdio
Worker->>Pyodide: runPythonAsync(script)
loop during execution
Pyodide->>Worker: stdout/stderr chunk
Worker->>Main: postMessage(stdout/stderr)
alt needs input
Worker->>Main: postMessage(stdin_request)
Main->>User: show input prompt
User->>Main: provide input
Main->>Worker: postMessage(stdin_line)
Worker->>Pyodide: deliver stdin
end
end
Pyodide-->>Worker: execution finished
Worker->>Main: postMessage({ runResult } or { error })
Main->>User: display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Uhh need to fix some "translations" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/tmc/next.config.js (2)
43-58:⚠️ Potential issue | 🟠 MajorMissing Turbopack equivalent for the
pyodidefallback — will breaknext dev --turbo.In webpack, the
resolve.fallbackoption is typically used to silence module-not-found errors; Turbopack offers a similar option viaturbopack.resolveAlias. At the moment only thebrowsercondition is supported for conditional aliasing. Because you need to tell Turbopack to load an empty module when the package is requested in the browser, the alias must point to an actual empty stub file — unlike webpack'sfallback: false, you cannot alias directly to a boolean.The fix requires two steps:
- Create a minimal empty stub (e.g.,
src/pyodideStub.js):// Pyodide is loaded from CDN at runtime; this stub silences Turbopack resolution. export default {}
- Add
resolveAliasto theturbopackblock:⚙️ Proposed fix for Turbopack pyodide resolution
turbopack: { rules: { "*.svg": { loaders: [ { loader: "@svgr/webpack", options: { svgoConfig: svgoConfig, svgProps: { role: "presentation" }, }, }, ], as: "*.js", }, }, + resolveAlias: { + // Pyodide is loaded from CDN at runtime; prevent Turbopack from bundling it. + pyodide: { browser: "./src/pyodideStub.js" }, + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/next.config.js` around lines 43 - 58, Add a Turbopack alias to handle the Pyodide fallback and create a real empty stub so `next dev --turbo` won't error: add a minimal stub file (e.g., src/pyodideStub.js) exporting an empty object, then update the turbopack block in next.config.js to include resolveAlias mapping the pyodide package name to that stub for the browser condition (use turbopack.resolveAlias) so the bundler resolves imports to the stub instead of failing; reference turbopack.resolveAlias and the new src/pyodideStub.js when making the change.
43-58:⚠️ Potential issue | 🟠 MajorTurbopack does not inherit webpack's
resolve.fallback— add aresolveAliasforpyodidewith a stub module.With Next.js 15,
next dev --turbois stable andnext build --turbopackis available. Turbopack processes its own module graph and ignores theresolve.fallbackset in thewebpack()function. If any client-side file that Turbopack traces importspyodide(which is not installed as a package), the dev server or build will fail.The Turbopack equivalent is
resolveAliaswith a conditionalbrowsertarget. Create an empty stub file and aliaspyodideto it:Create `empty.js` in the project root:
module.exports = {}Then update
next.config.js:Add `resolveAlias` to the turbopack config:
turbopack: { rules: { "*.svg": { loaders: [ { loader: "@svgr/webpack", options: { svgoConfig: svgoConfig, svgProps: { role: "presentation" }, }, }, ], as: "*.js", }, }, + resolveAlias: { + // Pyodide is loaded from CDN at runtime; prevent Turbopack from trying to bundle it. + pyodide: { browser: "./empty.js" }, + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/next.config.js` around lines 43 - 58, Turbopack ignores webpack resolve.fallback so create a tiny stub module named empty.js exporting an empty object and then add a resolveAlias entry inside the existing turbopack config block (the turbopack.rules section shown) to alias "pyodide" to that stub for the browser target; specifically add a resolveAlias mapping (browser conditional) that points "pyodide" -> "./empty.js" so any client-side import of pyodide resolves to the stub during dev/build.services/tmc/src/app/api/public-spec/route.ts (1)
203-204:⚠️ Potential issue | 🟡 Minor
makeLogdrops variadic arguments and leaksundefinedinto logs.The inner function uses
(message, optionalParams)instead of(message, ...optionalParams). Because the return type annotation uses rest params, TypeScript collapses all extra arguments into a single array foroptionalParams. Passing that array as a single positional arg tologmeans it gets printed as[ 'a', 'b' ]instead of being spread. When called with no extra args (e.g., from lines 101/105),optionalParamsis[]andlogprints an empty array.Proposed fix
const makeLog = (requestId: string): ((message: string, ...optionalParams: unknown[]) => void) => { - return (message, optionalParams) => log(requestId, message, optionalParams) + return (message, ...optionalParams) => log(requestId, message, ...optionalParams) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/app/api/public-spec/route.ts` around lines 203 - 204, The helper makeLog currently defines the inner function as (message, optionalParams) which collapses variadic args into a single array and passes that array as one positional arg to log, causing arrays or undefined to appear in logs; change the inner function to accept rest parameters (message: string, ...optionalParams: unknown[]) and call log(requestId, message, ...optionalParams) so arguments are forwarded correctly; update the inner function signature to match the declared return type and preserve types for requestId and log forwarding.
🟡 Minor comments (10)
services/tmc/SUBMISSION-PLAN.md-113-113 (1)
113-113:⚠️ Potential issue | 🟡 Minor
Math.max(1, points.length)silently masks an empty or malformed test suite.If
pointsis empty because the pod output was malformed, a JSON parse error occurred, or the exercise genuinely has no test points, this floorsscore_maximumat1and produces a valid-looking0/1grading result rather than surfacing the underlying problem.The plan should instead specify that
points.length === 0(after normalization and error handling) should result in an explicit error grading result or at minimum a logged warning, not a silent1. Silently adjustingscore_maximummakes broken exercises indistinguishable from correctly-graded ones in the data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/SUBMISSION-PLAN.md` at line 113, The current use of Math.max(1, points.length) hides malformed or empty test suites; change the logic that computes score_maximum from using Math.max to an explicit check of points.length === 0 (after your points normalization/error handling) and in that branch produce an explicit error grading result (or at minimum emit a clear warning log) instead of silently setting score_maximum to 1; update the code paths that compute score_maximum and the grading/result construction so they detect empty/malformed points, log the problem (include normalized input/context), and return a failure/error grading object rather than a 0/1-looking score.shared-module/packages/common/src/locales/ar/tmc.json-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorNew entries are untranslated — English strings in the Arabic locale file.
Both
submit-result-failedandsubmit-result-passedcarry English values while every other key in this file is properly translated into Arabic. Arabic-locale users will see raw English for submission results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/locales/ar/tmc.json` around lines 21 - 22, The Arabic locale file contains untranslated English strings for the keys "submit-result-failed" and "submit-result-passed"; update the values for these keys in shared-module/packages/common/src/locales/ar/tmc.json to their Arabic translations, preserving the interpolation placeholders ({{score}} and {{max}}) and the intended meaning/format so Arabic users see localized messages for submission results.shared-module/packages/common/src/locales/fi/tmc.json-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorLocalization strings are in English instead of Finnish.
All other entries in this Finnish locale file are properly translated, but these two new keys contain English text. They should be translated to Finnish, e.g.:
"submit-result-failed": something like "Testit epäonnistuivat. Pisteet: {{score}}/{{max}}. Voit muokata koodiasi ja palauttaa uudelleen.""submit-result-passed": something like "Kaikki testit läpäisty! Pisteet: {{score}}/{{max}}."The same issue applies to the Swedish (
sv/tmc.json) and Ukrainian (uk/tmc.json) locale files reviewed in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/locales/fi/tmc.json` around lines 21 - 22, Two localization entries ("submit-result-failed" and "submit-result-passed") in the Finnish locale (tmc.json) are still in English; update their values to Finnish (e.g. translate to "Testit epäonnistuivat. Pisteet: {{score}}/{{max}}. Voit muokata koodiasi ja palauttaa uudelleen." and "Kaikki testit läpäisty! Pisteet: {{score}}/{{max}}.") and apply equivalent translations to the same keys in the Swedish (sv/tmc.json) and Ukrainian (uk/tmc.json) locale files so all three locale files contain proper translations for submit-result-failed and submit-result-passed.services/tmc/src/app/api/grade/route.ts-119-125 (1)
119-125:⚠️ Potential issue | 🟡 MinorError message references
exercise_spec.typebut the validation failure is aboutsubmission_data.When
normalizeSubmissionDatareturns null, the error message on line 123 saysunexpected submission type '${exercise_spec.type}', which is misleading — the issue is thatsubmission_datais missing or has an unrecognized shape, not thatexercise_spec.typeis wrong. Consider referencing the actual submission data in the message.Proposed fix
if (!submission_data) { return badRequest( - `unexpected submission type '${exercise_spec.type}' (missing or invalid submission_data)`, + `missing or invalid submission_data (exercise_spec.type='${exercise_spec.type}')`, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/app/api/grade/route.ts` around lines 119 - 125, The error message is misleading when normalizeSubmissionData(rawSubmissionData) returns null; update the badRequest call in route.ts to reference the problematic submission data rather than exercise_spec.type. Specifically, in the block using rawSubmissionData / submission_data and calling normalizeSubmissionData, change the error text in the badRequest to mention missing/invalid submission_data and include identifying info (e.g., JSON.stringify(rawSubmissionData) or a short descriptor) so the message reflects the actual validation failure in normalizeSubmissionData.services/tmc/src/components/AnswerBrowserExercise.refactor-plan.md-28-51 (1)
28-51:⚠️ Potential issue | 🟡 MinorAdd language specifiers to fenced code blocks.
The code blocks on lines 28 and 182 lack language identifiers, which triggers markdownlint MD040. Adding a language (e.g.,
```textor```plaintextfor directory trees) improves rendering and accessibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise.refactor-plan.md` around lines 28 - 51, The markdown file AnswerBrowserExercise.refactor-plan.md contains fenced code blocks (the directory tree blocks near the top and another at line ~182) without language specifiers which triggers markdownlint MD040; update those fences to include a plain-text language identifier (e.g., change ``` to ```text or ```plaintext) so the directory trees render correctly and lint passes, ensuring you modify both fenced blocks referenced in this file.services/tmc/src/components/AnswerBrowserExercise.refactor-plan.md-214-216 (1)
214-216:⚠️ Potential issue | 🟡 MinorComment uses
#instead of//in a TypeScript code example.Line 215 uses a Python/shell-style comment (
# or ...) inside what's meant to be a TypeScript import example. This would be a syntax error in TS. Use//instead.Proposed fix
-import AnswerBrowserExercise from "./AnswerBrowserExercise" # or "./AnswerBrowserExercise/index" +import AnswerBrowserExercise from "./AnswerBrowserExercise" // or "./AnswerBrowserExercise/index"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise.refactor-plan.md` around lines 214 - 216, The TypeScript import example uses a Python-style comment symbol (#) which is invalid in TS; update the import line in AnswerBrowserExercise.refactor-plan.md to use a TypeScript/JS single-line comment (//) after the import (referencing the import statement `import AnswerBrowserExercise from "./AnswerBrowserExercise"` or the alternative `"./AnswerBrowserExercise/index"`) so the example is syntactically correct.services/tmc/src/components/ViewSubmission.tsx-38-43 (1)
38-43:⚠️ Potential issue | 🟡 Minor
<code>without<pre>will collapse whitespace and lose indentation.Python code (or any indented code) rendered inside
<code>alone will have its whitespace collapsed by the browser. Wrap it in<pre>to preserve formatting.Proposed fix
<div key={f.filepath}> <div>{f.filepath}</div> - <code>{f.contents}</code> + <pre><code>{f.contents}</code></pre> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/ViewSubmission.tsx` around lines 38 - 43, The rendered code blocks in ViewSubmission.tsx use <code>{f.contents}</code> which collapses whitespace and loses indentation; update the filesToShow.map rendering so the file contents are wrapped in a <pre> (e.g., <pre><code>...</code></pre>) or otherwise use a preformatted element around f.contents so Python/indented code preserves spacing and line breaks while keeping the existing keys (f.filepath) and structure intact.services/tmc/public/browserTestWorker.js-37-41 (1)
37-41:⚠️ Potential issue | 🟡 MinorNo validation that
lastLineis parseable JSON — cryptic error on malformed output.If the test script produces no stdout or the last line isn't valid JSON,
JSON.parse(lastLine)throws a generic syntax error. Consider wrapping in a try/catch with a descriptive message, and possibly validating the shape of the parsed result.Proposed fix
var lines = stdout.trim().split("\n") var lastLine = lines[lines.length - 1] ?? "" - var runResult = JSON.parse(lastLine) - self.postMessage({ runResult: runResult }) + var runResult + try { + runResult = JSON.parse(lastLine) + } catch (parseErr) { + self.postMessage({ error: "Test output was not valid JSON: " + lastLine.slice(0, 200) }) + return + } + self.postMessage({ runResult: runResult })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/browserTestWorker.js` around lines 37 - 41, The code assumes the Python script's last stdout line is valid JSON and directly does JSON.parse(lastLine); update the promise continuation after pyodide.runPythonAsync to guard parsing: check stdout/lastLine isn't empty, wrap JSON.parse(lastLine) in a try/catch, and on error post a descriptive error object via self.postMessage (e.g., { error: "Invalid JSON from test script", details: err.message, raw: lastLine }) instead of throwing; after successful parse validate the expected shape of runResult before posting { runResult } to avoid downstream cryptic syntax errors (referencing pyodide.runPythonAsync, lastLine, runResult, self.postMessage).services/tmc/src/components/AnswerBrowserExercise/styles.ts-145-145 (1)
145-145:⚠️ Potential issue | 🟡 MinorInvalid CSS:
bordershorthand does not accept four values.
border: 4px 4px 0 0;is invalid — thebordershorthand expects<width> <style> <color>. This likely should beborder-radiusto round the top corners of the output container.Proposed fix
- border: 4px 4px 0 0; + border-radius: 4px 4px 0 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/styles.ts` at line 145, Replace the invalid CSS shorthand "border: 4px 4px 0 0;" with a border-radius declaration: locate the offending rule in services/tmc/src/components/AnswerBrowserExercise/styles.ts (the styled element containing that line) and change it to use "border-radius" with explicit units (e.g. 4px 4px 0 0) so the top corners are rounded correctly.services/tmc/src/components/AnswerBrowserExercise/AnswerBrowserExercise.tsx-47-49 (1)
47-49:⚠️ Potential issue | 🟡 MinorAdd a fallback string to
t("no-exercise-files").Every other
t()call in this PR includes a fallback string. Without one here, a missing or not-yet-loaded translation key renders the raw key"no-exercise-files"to the user.🐛 Proposed fix
- return <div>{t("no-exercise-files")}</div> + return <div>{t("no-exercise-files", "No exercise files found.")}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/AnswerBrowserExercise.tsx` around lines 47 - 49, The UI returns t("no-exercise-files") when editorFiles is empty; add a fallback string to that translation call in the AnswerBrowserExercise component so a human-readable message is shown if the key is missing or not loaded (update the t invocation inside the editorFiles.length === 0 return to include a second argument like "No exercise files available"). Ensure you modify the t(...) call used in the AnswerBrowserExercise component (where editorFiles is checked) to include the fallback string.
🧹 Nitpick comments (22)
services/tmc/SUBMISSION-PLAN.md (1)
20-20: Remove---section dividers — the##headings already provide structure.The document uses seven
---horizontal rules purely as visual separators between sections. The existing##/###heading hierarchy already communicates section boundaries, making these dividers redundant noise.♻️ Example removal (pattern repeats at lines 34, 56, 74, 86, 103, 117)
### Routes we must not break ... So we can change **internal** behavior of the grade route (e.g. how we prepare or score), but not the API contract. - ---- - ## Why "submission still successful" when tests failAs per coding guidelines, avoid banner-style section divider comments — "long separator bars, repeated dashes" — and prefer clearer naming and smaller focused sections instead.
Also applies to: 34-34, 56-56, 74-74, 86-86, 103-103, 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/SUBMISSION-PLAN.md` at line 20, Remove all standalone '---' horizontal-rule lines from services/tmc/SUBMISSION-PLAN.md (they appear after the headings at lines referenced in the review) so the document relies on the existing '##' and '###' headings for structure; edit the file to delete each '---' divider instance (the repeated three-dash lines) and ensure no additional whitespace or leftover separators remain between heading sections.services/tmc/src/components/AnswerBrowserExercise/utils/formatLogsForDisplay.ts (1)
1-6: Minor: spurious blank line in method chain and terse destructuring names.The blank line between
Object.entries(logs)and.map(...)is unusual and likely accidental.k/vcould bekey/valuefor a marginal readability gain.♻️ Proposed fix
export function formatLogsForDisplay(logs: Record<string, string>): string { - return Object.entries(logs) - - .map(([k, v]) => `[${k}]\n${v}`) + return Object.entries(logs) + .map(([key, value]) => `[${key}]\n${value}`) .join("\n\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/utils/formatLogsForDisplay.ts` around lines 1 - 6, In formatLogsForDisplay, remove the spurious blank line in the method chain after Object.entries(logs) and rename the destructured variables in the map from k and v to more descriptive names (e.g., key and value) so the chain reads Object.entries(logs).map(([key, value]) => `[${key}]\n${value}`).join("\n\n") for improved readability.services/tmc/src/components/AnswerBrowserExercise/utils/extensionToLanguage.ts (1)
3-3: Prefer strict equality===over==.
lastIndexOfalways returns anumber, so this is functionally safe, but using===is the idiomatic TypeScript convention and avoids implicit type coercion.♻️ Proposed fix
- if (separator == -1) { + if (separator === -1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/utils/extensionToLanguage.ts` at line 3, Update the equality check in extensionToLanguage.ts to use strict equality: replace the loose comparison "if (separator == -1)" with a strict comparison "if (separator === -1)" so the code follows TypeScript/JavaScript idioms and avoids implicit coercion; locate the check referencing the variable "separator" in the function that parses the extension.services/tmc/src/components/AnswerBrowserExercise/components/ActionButtons.tsx (1)
60-60:?? undefinedis redundant.
testUnavailableReasonis already typedstring | undefined, so?? undefinedis a no-op.Proposed fix
- title={p.testUnavailableReason ?? undefined} + title={p.testUnavailableReason}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/ActionButtons.tsx` at line 60, The title prop in ActionButtons (where you set title={p.testUnavailableReason ?? undefined}) uses a redundant nullish coalescing to undefined; remove the "?? undefined" and pass the value directly (title={p.testUnavailableReason}) in the component so the prop retains its existing string | undefined type without the no-op operator.services/tmc/src/components/AnswerBrowserExercise/hooks/useEditorState.ts (1)
3-3: Prefer a targeted lodash import for better tree-shaking.
import _ from "lodash"pulls in the entire lodash bundle. Since onlycloneDeepis used, import it directly.Proposed fix
-import _ from "lodash" +import cloneDeep from "lodash/cloneDeep"Then replace
_.cloneDeep(...)withcloneDeep(...)throughout the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/hooks/useEditorState.ts` at line 3, The file currently imports the entire lodash bundle via "import _ from 'lodash'"; replace that with a targeted import of cloneDeep (e.g., import { cloneDeep } from 'lodash' or import cloneDeep from 'lodash/cloneDeep') and then update all usages of _.cloneDeep(...) in useEditorState (the hook in useEditorState.ts) to call cloneDeep(...) instead so tree-shaking can remove unused lodash code.services/tmc/src/components/ViewSubmission.tsx (1)
8-20:normalizeSubmissionrelies on an unsafe double cast — add a runtime guard.Line 12 casts through
unknown, which bypasses type safety entirely. The subsequent check onraw.private_spec.typeis good, but if the input is something truly unexpected (e.g.,null), accessingraw?.private_speccould still produce surprising results. Consider adding an explicittypeof submission === "object"guard before the cast, or documenting that this is intentionally defensive.That said, the function is clearly a compatibility shim and the intent is well-documented by the comment on line 7 — this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/ViewSubmission.tsx` around lines 8 - 20, normalizeSubmission uses an unsafe double cast through unknown; add a runtime guard to avoid accessing properties on non-objects: first check that submission is an object and not null (e.g., typeof submission === "object" && submission !== null) before treating it as having private_spec, then verify 'private_spec' in submission and that private_spec is an object before checking private_spec.type; update the logic inside normalizeSubmission (referencing the normalizeSubmission function and the raw/private_spec checks) to perform these guards and only then return raw.private_spec when its type is "browser" or "editor", otherwise return the original submission.services/tmc/src/components/StateRenderer.tsx (1)
66-84: The fake-state adapter is fragile and hard to follow — consider documenting the invariant.
setStateForViewSubmissionsynthesizes an"answer-exercise"state so thatAnswerBrowserExerciseand its hooks can operate transparently in a"view-submission"context. This is clever but creates a coupling where any hook that inspectsview_type(likeuseEditorState) must produce a result that this adapter knows how to reverse-translate.A brief comment at the declaration site explaining the contract (e.g., "The inner updater must return an answer-exercise state or null; other view_types are not expected") would help future maintainers avoid subtle breakage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/StateRenderer.tsx` around lines 66 - 84, The adapter setStateForViewSubmission synthesizes a fake ExerciseIframeState with view_type "answer-exercise" so AnswerBrowserExercise and hooks like useEditorState can run inside a "view-submission" state; document this invariant at the setStateForViewSubmission declaration: explain that the updater will receive a fake "answer-exercise" state (public_spec/user_answer/previous_submission) and must return either an "answer-exercise" state (so the adapter will copy result.user_answer back into prev.submission), null/undefined (no-op), or a non-"answer-exercise" state (which will replace the state directly), and note that other view_type values are not expected—add this concise comment so future maintainers know the contract between setStateForViewSubmission, AnswerBrowserExercise, and useEditorState.services/tmc/src/components/AnswerBrowserExercise/components/EditorSection.tsx (1)
23-33: Deep-cloning the entire file array on every keystroke is costly.
_.cloneDeep(editorFiles)runs on eachonChangeevent (i.e., every character typed). For exercises with many or large files, this could cause noticeable input lag. Consider a shallow clone with a targeted content update instead, since only one file'scontentsstring changes.Proposed fix
const onChange = (newContents: string | undefined) => { if (readOnly || newContents === undefined) { return } - const newState = _.cloneDeep(editorFiles) - const changed = newState.find((ef) => ef.filepath === filepath) - if (changed) { - changed.contents = newContents - } + const newState = editorFiles.map((ef) => + ef.filepath === filepath ? { ...ef, contents: newContents } : ef, + ) setEditorState(newState) }This avoids the lodash dependency in this file entirely and is O(n) shallow rather than O(n·m) deep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/EditorSection.tsx` around lines 23 - 33, onChange currently deep-clones editorFiles on every keystroke (via _.cloneDeep(editorFiles)), which is expensive; instead perform a shallow update: create a new array from editorFiles (e.g., [...editorFiles]), find the target entry by filepath, and replace only that entry (or copy that object and set its contents) before calling setEditorState; update the onChange function to remove the _.cloneDeep call and avoid modifying existing objects in-place so only the single file's contents changes while preserving referential identity for other entries.services/tmc/src/util/pyodideLoader.ts (1)
33-36: Existing<script>tag found does not guarantee the script has finished loading.If
loadScriptis ever called outside thegetPyodidesingleton guard (or the guard is refactored), finding an existing<script>tag and resolving immediately can race against a still-loading script. Currently safe becausegetPyodideensures a single call, but the helper is fragile if reused. Consider listening for theloadevent on the existing element before resolving.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/util/pyodideLoader.ts` around lines 33 - 36, The current loadScript helper in pyodideLoader.ts finds an existing script element via document.querySelector(`script[src="${src}"]`) and calls resolve immediately, which can race if that script is still loading; change the logic in loadScript so that when an existing element is found (the existing variable) you check its load state and if it hasn't finished attach 'load' and 'error' listeners to that element and only call resolve() on the load event (or reject on error); if the element is already complete (use readyState/complete flag or a data-loaded attribute), resolve immediately. Ensure you update any code paths in getPyodide that rely on loadScript to still work with the new listener-based behavior.services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts (1)
5-8:getRunWorkerUrlsilently falls back to an empty base path.If
NEXT_PUBLIC_BASE_PATHis not set and the app is deployed under a subpath, the worker URL will be incorrect. This is likely fine for the common case, but consider logging a warning in development when the env var is absent so misconfigurations are caught early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts` around lines 5 - 8, getRunWorkerUrl currently silently falls back to an empty base when NEXT_PUBLIC_BASE_PATH is missing; change getRunWorkerUrl to detect when process.env.NEXT_PUBLIC_BASE_PATH is undefined (and process.env.NODE_ENV !== "production" / running in development) and emit a clear warning (e.g., console.warn) that the base path is missing so deployments under a subpath are caught early, while preserving the existing return value `${base ?? ""}/runWorker.js`.services/tmc/src/tmc/browserTestScript.ts (1)
339-380: Generated Python code is dense — consider breaking the test-runner scaffold into a.pytemplate file.The inline Python in
inlineAndPatchTestSources(particularly the ~40-line test code template and the long list comprehension on line 376) is difficult to read, lint, or unit-test as a single template string. Extracting it into a separate.pyresource file loaded at build time would:
- Allow Python linters/formatters to validate the template
- Make the logic easier to maintain
- Reduce risk of escaping bugs
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/tmc/browserTestScript.ts` around lines 339 - 380, The inline Python template assigned to testCode inside inlineAndPatchTestSources is too large and should be moved into a standalone .py template file; update the code so testCode is loaded from that resource (e.g., readFileSync of a template like tmc_test_runner_template.py) and then programmatically inject dynamic fragments using the existing helpers (stringifyPythonCode, patchTmcFile, patchTestSource) before sending to the runner. Specifically: create the .py template containing the scaffold (imports, __wrap_import, tmc_webeditor setup, TMCTestRunner invocation and the results list comprehension) with placeholders for __webeditor_user_code_b64 and the patched module/test blocks, replace the inline template in browserTestScript.ts (the testCode string) to read the file and perform placeholder substitutions for the outputs of stringifyPythonCode(patchTmcFile(...)) and patchTestSource(test,...), and ensure behavior around inspect.getsource and final JSON print remains identical.services/tmc/src/util/pyodideConfig.ts (1)
1-8: Version duplication across workers undermines the "single source of truth" claim.Both
public/browserTestWorker.jsandpublic/runWorker.jshardcodePYODIDE_CDN_VERSION = "0.29.3"independently. The comment in each file acknowledges the need to keep them in sync withutil/pyodideConfig.ts, but this manual synchronization is error-prone and will eventually drift. Consider generating the worker files at build time (e.g., via a template or environment-variable injection) so the version is truly defined once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/util/pyodideConfig.ts` around lines 1 - 8, The worker files duplicate the Pyodide version (PYODIDE_CDN_VERSION) instead of using the single source PYODIDE_VERSION from pyodideConfig.ts; fix by removing the hardcoded PYODIDE_CDN_VERSION in the workers and make them consume the canonical PYODIDE_VERSION at build time (either by injecting process.env.PYODIDE_VERSION into the worker bundle via your build tool or by generating the worker sources from a template that substitutes PYODIDE_VERSION from util/pyodideConfig.ts), update public worker code to construct PYODIDE_INDEX_URL/PYODIDE_SCRIPT_URL from that injected PYODIDE_VERSION (or import the config if your bundler supports it), and remove the manual sync comment so the version truly lives only in PYODIDE_VERSION.services/tmc/src/components/AnswerBrowserExercise/browserTest/pythonAdapter.ts (2)
23-23: Hardcoded English string ingetCannotRunMessage.
"Only Python files can be tested."is a user-visible string. Since this is a plain function (not a React component),useTranslationcan't be used directly here. Consider having the adapter return a translation key (e.g.,"browser-test-python-only") and translating at the call site.As per coding guidelines, "Make sure to use the
useTranslationhook for all text that is visible to the user."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/browserTest/pythonAdapter.ts` at line 23, Replace the hardcoded English message returned by getCannotRunMessage with a translation key string (e.g., "browser-test-python-only") so the adapter returns a key rather than user-facing text; update getCannotRunMessage in pythonAdapter.ts to return that key, and ensure callers translate it with useTranslation (t("browser-test-python-only")) at the UI/render site where the message is shown.
12-16:unescape()is deprecated; consider a modern replacement for UTF-8 to base64 conversion.
unescape(encodeURIComponent(…))works but uses a deprecated function. However, the suggestedTextEncoder+ spread operator approach has a limitation:String.fromCharCode(...bytes)fails on large inputs (call stack exceeded). Use one of these instead:Option 1: Chunk the byte array (works in all browsers):
const bytes = new TextEncoder().encode(userCode) let base64Code = "" const chunk = 0x8000 for (let i = 0; i < bytes.length; i += chunk) { base64Code += String.fromCharCode(...bytes.subarray(i, i + chunk)) } const safeBase64 = btoa(base64Code).replace(/\+/g, "-").replace(/\//g, "_")Option 2: Use
Uint8Array.toBase64()(modern browsers only):const base64Code = btoa(String.fromCharCode(...new TextEncoder().encode(userCode)))Also, the escaping on line 14 (
replace(/\\/g, …).replace(/"/g, …)) is a no-op since base64 output only containsA-Z,a-z,0-9,+,/, and=. It can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/browserTest/pythonAdapter.ts` around lines 12 - 16, The buildScript function uses deprecated unescape(encodeURIComponent(...)) and unnecessary escaping of base64; replace that with a UTF-8 -> base64 conversion using TextEncoder and chunking to avoid call stack issues (encode userCode to a Uint8Array, convert in chunks to a string with String.fromCharCode on subarrays, then btoa that result) and remove the replace(/\\/g, ...) and replace(/"/g, ...) since base64 won't contain those characters; update buildScript to produce __webeditor_user_code_b64 = "<base64>"\n${specScript} using the new base64 output and ensure any URL-safe replacement (e.g., '+' -> '-', '/' -> '_') only if required by downstream consumers.services/tmc/src/components/AnswerBrowserExercise/styles.ts (2)
83-97:ResetButtonduplicatesStyledButtonbase styles.
ResetButtonre-declares the same margin, cursor, border, border-radius, padding, font-size, and font-weight thatStyledButtonalready defines. Consider extendingStyledButtonto reduce duplication.Proposed fix
-export const ResetButton = styled.button` - margin: 0.5em; - margin-left: 10px; - cursor: pointer; - border: none; - border-radius: 4px; - padding: 8px 16px; - font-size: 0.875rem; - font-weight: 500; - background-color: `#ebebeb`; - color: `#252525`; +export const ResetButton = styled(StyledButton)` + margin-left: 10px; + background-color: `#ebebeb`; + color: `#252525`; &:hover { background-color: `#d5d5d5`; } `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/styles.ts` around lines 83 - 97, ResetButton duplicates base styles already defined on StyledButton; change ResetButton to extend StyledButton (e.g., styled(StyledButton)) and only override the differing properties (background-color, color, margin-left and :hover background-color) so you remove duplicated margin/cursor/border/border-radius/padding/font-size/font-weight declarations while keeping the same visual behavior; update the declaration that currently exports ResetButton to use StyledButton as the base.
1-259: Prefer theme colors and fonts over hardcoded values.This file has numerous hardcoded color hex values (
#0275d8,#ff7518,#4caf50,#f44336,#fff,#252525,#e8e8e8, etc.) and repeated monospace font stacks. The coding guidelines recommend usingbaseTheme.colorsandprimaryFontfrom the theme where possible. Consider extracting at least the primary palette values (brand blue, warning orange, success green, error red, surface/text grays) into theme constants so they're consistent project-wide and easier to maintain.As per coding guidelines, "If a component uses colors, use colors from the theme if possible." and "If a component uses fonts, use fonts from the theme if possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/styles.ts` around lines 1 - 259, The file hardcodes many colors and font stacks; update styled components to use the theme's color palette and primary font instead of literals: import/use the theme (p.theme.baseTheme.colors and p.theme.primaryFont) and replace hex values in components like RunButton, StopButton, TestButton, ResetButton, OutputHeader, StdinWaitingBanner, TestResultCard, TestResultHeader, OutputBody, OutputPre, StdinSubmittedLine, StdinInput, etc., mapping each hardcoded semantic role (brand/primary, warning, success, error, surface/text/gray) to the appropriate baseTheme.colors key and replace all repeated monospace font-family strings with p.theme.primaryFont (or a theme font stack) so components use theme colors/fonts consistently.services/tmc/src/components/AnswerBrowserExercise/components/TestResultsContent.tsx (1)
16-28:testResults.statusis rendered as raw text—consider translating it.When
testResults.testResultsis empty, the rawstatusstring (e.g.,"COMPILE_FAILED","TESTRUN_INTERRUPTED") is shown directly to the user. Consider mapping these status values to translated, human-readable labels viauseTranslation.As per coding guidelines, "Make sure to use the
useTranslationhook for all text that is visible to the user."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/TestResultsContent.tsx` around lines 16 - 28, The component TestResultsContent currently renders raw machine status strings (testResults.status) when testResults.testResults is empty; update it to use the i18n hook by importing and calling useTranslation and map status values to human-readable labels (e.g., via a small STATUS_LABELS map or a switch inside TestResultsContent) and render t(STATUS_LABELS[status] || status) instead of the raw testResults.status; keep the existing logs rendering (formatLogsForDisplay) unchanged and ensure the translation keys exist for values like "COMPILE_FAILED" and "TESTRUN_INTERRUPTED".services/tmc/src/components/AnswerBrowserExercise/components/TestResultCard.tsx (1)
14-18: User-visible "PASS"/"FAIL" strings should useuseTranslation.Line 17 renders hardcoded English text
"PASS"/"FAIL"directly in the UI. Per coding guidelines, all user-visible text should go through theuseTranslationhook.Proposed fix
+import { useTranslation } from "react-i18next" + -export const TestResultCard: React.FC<TestResultCardProps> = (p) => ( - <StyledCard passed={p.passed} data-cy="test-result"> - <TestResultHeader passed={p.passed}> - {p.passed ? "PASS" : "FAIL"}: {p.name} - </TestResultHeader> +export const TestResultCard: React.FC<TestResultCardProps> = (p) => { + const { t } = useTranslation() + return ( + <StyledCard passed={p.passed} data-cy="test-result"> + <TestResultHeader passed={p.passed}> + {p.passed ? t("test-pass") : t("test-fail")}: {p.name} + </TestResultHeader>As per coding guidelines, "Make sure to use the
useTranslationhook for all text that is visible to the user."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/TestResultCard.tsx` around lines 14 - 18, The TestResultCard currently renders hardcoded "PASS"/"FAIL"; import and use the useTranslation hook inside TestResultCard (e.g., const { t } = useTranslation()) and replace the hardcoded strings in TestResultHeader with t(...) calls (e.g., t('testResult.pass') and t('testResult.fail') or whichever translation keys you choose), ensuring you reference p.passed when selecting the key; update any related tests or add translation entries for the chosen keys.services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx (2)
84-86: Remove the redundantscrollIntoViewoptions comment.The
/* scrollIntoView options: ... */comment only labels the parameter types already visible in the code. It conveys no non-obvious intent, rationale, or constraint. Theeslint-disablecomment on line 85 already provides the needed context.♻️ Proposed fix
useEffect(() => { if (waitingForInput && inputRef.current) { - /* scrollIntoView options: ScrollBehavior and ScrollLogicalPosition (not user-facing) */ // eslint-disable-next-line i18next/no-literal-string inputRef.current.scrollIntoView({ behavior: "smooth", block: "nearest" }) } }, [waitingForInput])As per coding guidelines, "Do not add comments that simply restate what the code already says."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx` around lines 84 - 86, Remove the redundant comment above the scrollIntoView call: delete the line "/* scrollIntoView options: ScrollBehavior and ScrollLogicalPosition (not user-facing) */" and keep the existing eslint-disable comment and the call inputRef.current.scrollIntoView({ behavior: "smooth", block: "nearest" }) unchanged so the code remains clear and lint-suppressed where intended.
67-73: Consider refocusing the input after stdin submission and on waiting state.
handleSubmitclearsstdinValuebut leaves focus elsewhere;useEffectonly scrolls without calling.focus(). For programs with multiple sequentialinput()calls, the user must manually re-click the field each time.✨ Proposed improvement
const handleSubmit = () => { if (submitStdinLine == null) { return } submitStdinLine(stdinValue) setStdinValue("") + inputRef.current?.focus() }useEffect(() => { if (waitingForInput && inputRef.current) { inputRef.current.scrollIntoView({ behavior: "smooth", block: "nearest" }) + inputRef.current.focus() } }, [waitingForInput])Also applies to: 82-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx` around lines 67 - 73, The input is not refocused after submission and while waiting, making sequential input calls require manual clicks; update handleSubmit to, after calling submitStdinLine and clearing via setStdinValue(""), call focus on the input ref (e.g., stdinInputRef.current?.focus()) and ensure stdinInputRef is attached to the input element, and also modify the existing useEffect that currently only scrolls (and the analogous logic around the waiting state between lines ~82-88) to call stdinInputRef.current?.focus() when the component is waiting for the next stdin or when stdinValue is cleared so the field regains focus automatically; guard all ref accesses for null to avoid runtime errors.services/tmc/src/components/AnswerBrowserExercise/browserTest/index.ts (1)
10-12: Redundant explicit type annotations when function is already typed asGetBrowserTestAdapter.Since
getBrowserTestAdapteris typed viaGetBrowserTestAdapter, the parameter type and return type annotations on lines 11–12 are inferred automatically.♻️ Proposed simplification
- export const getBrowserTestAdapter: GetBrowserTestAdapter = ( - runtime: BrowserTestSpec["runtime"], - ): BrowserTestRunnerAdapter | null => { + export const getBrowserTestAdapter: GetBrowserTestAdapter = (runtime) => { switch (runtime) { case "python": return getPythonBrowserTestAdapter() default: return null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/browserTest/index.ts` around lines 10 - 12, The function getBrowserTestAdapter already has the GetBrowserTestAdapter type, so remove the redundant explicit parameter and return type annotations: drop the ": BrowserTestSpec[\"runtime\"]" on the runtime parameter and the ": BrowserTestRunnerAdapter | null" return annotation so the compiler can infer them from GetBrowserTestAdapter; keep the function name getBrowserTestAdapter and the runtime parameter identifier unchanged to preserve usage sites.services/tmc/src/components/AnswerBrowserExercise/AnswerBrowserExercise.tsx (1)
82-112: Replace hardcoded hex colors in the grading banner with theme tokens.Lines 88–90 use raw hex values (
#dcfce7,#fef3c7,#166534,#92400e) instead of colours drawn frombaseTheme. Importing the theme palette keeps colours consistent with the rest of the UI and makes future theming straightforward.♻️ Proposed fix (example — adjust to actual token names)
+ import { baseTheme } from "@/shared-module/common/styles"className={css` margin-bottom: 1rem; padding: 0.75rem 1rem; border-radius: 0.5rem; - background-color: ${passed ? "#dcfce7" : "#fef3c7"}; - color: ${passed ? "#166534" : "#92400e"}; + background-color: ${passed ? baseTheme.colors.green[100] : baseTheme.colors.yellow[100]}; + color: ${passed ? baseTheme.colors.green[700] : baseTheme.colors.yellow[800]}; font-size: 0.9375rem; `}As per coding guidelines, "If a component uses colors, use colors from the theme if possible. Example:
import { baseTheme } from "@/shared-module/common/styles"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/AnswerBrowserExercise.tsx` around lines 82 - 112, The grading banner in AnswerBrowserExercise uses hardcoded hex colors (background-color: `#dcfce7` / `#fef3c7` and color: `#166534` / `#92400e`); import the theme (e.g. baseTheme from "@/shared-module/common/styles") and replace those raw hex values with the appropriate theme tokens (use semantic success/warning or green/yellow palette tokens) in the background-color and color expressions so the banner uses baseTheme color tokens instead of literals, updating the component where grading and passed are computed.
services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx
Outdated
Show resolved
Hide resolved
services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx
Outdated
Show resolved
Hide resolved
services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (4)
services/tmc/src/util/pyodideLoader.ts (1)
47-65: Rejected-promise caching addressed — LGTM.The previous concern about
pyodidePromisepermanently holding a rejected promise has been resolved. Thecatchblock correctly resetspyodidePromise = nullbefore re-throwing, allowing callers to retry after a transient failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/util/pyodideLoader.ts` around lines 47 - 65, The getPyodide function correctly avoids caching a permanently rejected promise by resetting the module-level pyodidePromise to null inside the catch block before re-throwing; no code changes are required—leave getPyodide, the pyodidePromise handling, and the catch/reset logic as implemented.services/tmc/public/runWorker.js (2)
118-134: Template-based injection applied — LGTM.The user script is now delivered via base64 (
self.userScriptB64) and the Python template is fetched as a separate file rather than built through string concatenation. This resolves the fragility and double-escaping concerns from the prior review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/runWorker.js` around lines 118 - 134, The current post-run flush incorrectly treats stderr as stdout: in the promise handler after pyodide.runPythonAsync, when handling flushErr from stderrDecoder you append it to stdout and post it with type "stdout"; change this to append flushErr to the stderr accumulator (or a new stderr variable) and post it with type "stderr" using the same self.postMessage call pattern; update references around stdoutDecoder, stderrDecoder, flushOut, flushErr, stdout, and self.postMessage to reflect the correct stream handling.
99-116:TextDecoderfix applied — LGTM.Multi-byte UTF-8 is now decoded correctly via
TextDecoderwith{ stream: true }, and the decoder flush is called after the run completes. This resolves the previous mojibake concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/runWorker.js` around lines 99 - 116, The stderr raw handler currently decodes into stderrDecoder but incorrectly appends to the stdout buffer and emits messages as type "stdout"; update the pyodide.setStderr raw function (the raw callback that uses stderrDecoder) to append decoded chunks to the stderr variable (not stdout) and postMessage with type "stderr" (preserving the same chunk payload), mirroring the logic used in pyodide.setStdout.services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts (1)
96-105: Buffered stdout flushing applied — LGTM.The per-byte re-render concern from the prior review is resolved. Chunks are accumulated in
runOutputBufferRefand flushed at most every 50 ms (or eagerly on newlines and stdin requests), greatly reducing React update frequency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts` around lines 96 - 105, The buffering implementation in useRunOutput's handleMessage correctly accumulates stdout chunks into runOutputBufferRef and only triggers flushStdout on newlines or the periodic timer, so no code change is required; keep the current logic in handleMessage (referencing runOutputBufferRef and flushStdout) to maintain reduced React re-render frequency and ensure eager flushing on newline or stdin requests.
🧹 Nitpick comments (1)
services/tmc/public/runWorker.js (1)
9-10:PYODIDE_CDN_VERSIONis duplicated frompyodideConfig.tswith no enforcement.The comment on line 6 manually instructs keeping these in sync. In a plain Web Worker script that cannot
import, this is understandable, but the risk of version drift between this file andpyodideConfig.tsis real (e.g., the TS module gets bumped but this file is overlooked).Consider adding a build-time check (e.g., a lint rule, a CI script, or a simple
grep-based assertion) that asserts both constants share the same version string, surfacing mismatches before they reach production.Would you like me to generate a CI/lint script that verifies the two version constants are kept in sync?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/runWorker.js` around lines 9 - 10, PYODIDE_CDN_VERSION and INDEX_URL in runWorker.js are duplicated from pyodideConfig.ts and can drift; add a build-time consistency check that fails CI if versions diverge by extracting the PYODIDE_CDN_VERSION value from both runWorker.js and pyodideConfig.ts (or the INDEX_URL in runWorker.js) and comparing them. Implement this as a small script or CI step (e.g., grep/awk/node) that reads the PYODIDE_CDN_VERSION constant in pyodideConfig.ts and the PYODIDE_CDN_VERSION/INDEX_URL in services/tmc/public/runWorker.js and exits non‑zero on mismatch, so the pipeline prevents merging when the two values differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/tmc/public/runWorker.js`:
- Around line 38-44: The getPyodide function caches pyodidePromise even when
loadPyodide rejects, preventing recovery; update getPyodide so the Promise
assigned to pyodidePromise handles rejection by resetting pyodidePromise to null
(e.g., attach a .catch that sets pyodidePromise = null and rethrows the error)
before returning it, ensuring subsequent calls to getPyodide will retry
loadPyodide; refer to pyodidePromise, getPyodide, and loadPyodide in your
change.
- Around line 27-36: The getTemplate function currently assigns templatePromise
to the fetch chain and never clears it on failure, so a rejected fetch
permanently poisons templatePromise; modify getTemplate (and the fetch chain for
templatePromise) to check response.ok and, on any failure or thrown error, reset
templatePromise back to null before rethrowing the error so subsequent run calls
can retry; specifically update the fetch promise returned by getTemplate
(referencing templatePromise and getTemplate) to attach a .then that verifies
r.ok -> r.text() and a .catch (or finally on rejection) that sets
templatePromise = null and rethrows the original error.
In `@services/tmc/public/runWorkerTemplate.py`:
- Line 64: The local variable `t` from the tuple returned by sys.exc_info() is
unused in the exception handling in runWorkerTemplate.py; update both
occurrences where you unpack as `t, v, tb = sys.exc_info()` (the one at the
current spot and the identical pattern near line 40) to prefix the unused name
with an underscore (e.g., `_t, v, tb = sys.exc_info()`), leaving `v` and `tb`
unchanged so linters stop complaining while the code behavior remains the same.
- Around line 47-49: Inside execute(), setting __name__ = "__main__" only
creates a local variable and doesn't affect exec; change exec to run user_code
with a globals dict where '__name__' is set to "__main__" (e.g., build or copy a
globals dict, set globals_dict['__name__']="__main__", then call exec(user_code,
globals_dict)), so code guarded by if __name__ == "__main__": will execute;
update the call site in execute and ensure user_code is passed to exec with that
modified globals dict rather than relying on a local variable.
- Line 72: The create_task call currently discards the Task returned by
asyncio.create_task(wrap_execution()), which lets the task be garbage-collected
and cancelled; capture and retain that Task (e.g., assign it to a variable like
task = asyncio.create_task(wrap_execution()) or store it on a long-lived object)
so the Task returned by asyncio.create_task is strongly referenced until
wrap_execution completes; update the code around the
asyncio.create_task(wrap_execution()) invocation to keep that reference
(referencing the wrap_execution() call and the created Task variable).
- Around line 16-21: The generic_visit method in runWorkerTemplate.py is
inappropriately mutating all AST string constants via re.sub(r"[\n] ", "\n",
node.value), which corrupts legitimate user strings; remove the substitution and
the replacement ast.Constant creation/return so generic_visit simply delegates
to super() (i.e., stop altering ast.Constant string nodes in generic_visit), and
if the re import becomes unused after removing this code, delete the re import
as well; reference the generic_visit method and the ast.Constant handling in
your changes.
- Around line 32-35: The compiled AST can contain top-level await (PatchCode
rewrites input() calls to await input_impl()), so change the compile(...) call
that builds user_code to pass the ast.PyCF_ALLOW_TOP_LEVEL_AWAIT flag (i.e., use
compile(tree, " ", "exec", flags=ast.PyCF_ALLOW_TOP_LEVEL_AWAIT)) so top-level
await expressions in the module are accepted; update the compile invocation near
the PatchCode().visit(tree) usage to include this flag.
In `@services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts`:
- Around line 160-167: The cleanup in the useEffect only clears
flushTimerRef.current but doesn't terminate workerRef.current, causing the
worker to keep running and handleMessage to reference stale closures; update the
effect cleanup to call the existing stopRun function (which properly terminates
workerRef.current, clears timers, and removes handlers) instead of manually
clearing flushTimerRef, or have the cleanup delegate to stopRun so teardown is
consistent and idempotent; reference stopRun, workerRef.current,
flushTimerRef.current, and handleMessage to locate and replace the current
cleanup logic.
- Around line 53-79: The runPython function can start a new run while a previous
one is still executing, causing the same worker to be reused and its message
handlers to be replaced which interleaves stdout/errors; fix by checking
runExecuting at the top of runPython and returning early (guard) OR explicitly
terminate and recreate the worker before starting a new run: if runExecuting is
true return (and ensure UI disables run trigger), or call
workerRef.current.terminate(), set workerRef.current = undefined, then new
Worker(getRunWorkerUrl()) and reassign handlers; ensure setRunExecuting is set
appropriately and existing finish() is called/used to clean up the previous run
state when terminating.
In `@services/tmc/src/util/pyodideLoader.ts`:
- Around line 27-45: The loadScript helper currently resolves immediately if it
finds an existing script tag, which breaks retries in getPyodide because the
existing tag may still be loading or previously errored; update loadScript to
check the existing element's load state and, if it isn't already successfully
loaded, attach onload and onerror handlers to that element (or inspect
script.readyState) so the returned Promise only resolves/rejects when that
script actually finishes; also ensure that when a previous load errored you
remove or replace the tag so subsequent calls can retry and that this behavior
coordinates with pyodidePromise and window.loadPyodide.
---
Duplicate comments:
In `@services/tmc/public/runWorker.js`:
- Around line 118-134: The current post-run flush incorrectly treats stderr as
stdout: in the promise handler after pyodide.runPythonAsync, when handling
flushErr from stderrDecoder you append it to stdout and post it with type
"stdout"; change this to append flushErr to the stderr accumulator (or a new
stderr variable) and post it with type "stderr" using the same self.postMessage
call pattern; update references around stdoutDecoder, stderrDecoder, flushOut,
flushErr, stdout, and self.postMessage to reflect the correct stream handling.
- Around line 99-116: The stderr raw handler currently decodes into
stderrDecoder but incorrectly appends to the stdout buffer and emits messages as
type "stdout"; update the pyodide.setStderr raw function (the raw callback that
uses stderrDecoder) to append decoded chunks to the stderr variable (not stdout)
and postMessage with type "stderr" (preserving the same chunk payload),
mirroring the logic used in pyodide.setStdout.
In `@services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts`:
- Around line 96-105: The buffering implementation in useRunOutput's
handleMessage correctly accumulates stdout chunks into runOutputBufferRef and
only triggers flushStdout on newlines or the periodic timer, so no code change
is required; keep the current logic in handleMessage (referencing
runOutputBufferRef and flushStdout) to maintain reduced React re-render
frequency and ensure eager flushing on newline or stdin requests.
In `@services/tmc/src/util/pyodideLoader.ts`:
- Around line 47-65: The getPyodide function correctly avoids caching a
permanently rejected promise by resetting the module-level pyodidePromise to
null inside the catch block before re-throwing; no code changes are
required—leave getPyodide, the pyodidePromise handling, and the catch/reset
logic as implemented.
---
Nitpick comments:
In `@services/tmc/public/runWorker.js`:
- Around line 9-10: PYODIDE_CDN_VERSION and INDEX_URL in runWorker.js are
duplicated from pyodideConfig.ts and can drift; add a build-time consistency
check that fails CI if versions diverge by extracting the PYODIDE_CDN_VERSION
value from both runWorker.js and pyodideConfig.ts (or the INDEX_URL in
runWorker.js) and comparing them. Implement this as a small script or CI step
(e.g., grep/awk/node) that reads the PYODIDE_CDN_VERSION constant in
pyodideConfig.ts and the PYODIDE_CDN_VERSION/INDEX_URL in
services/tmc/public/runWorker.js and exits non‑zero on mismatch, so the pipeline
prevents merging when the two values differ.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
services/tmc/public/browserTestWorker.jsservices/tmc/public/runWorker.jsservices/tmc/public/runWorkerTemplate.pyservices/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsxservices/tmc/src/components/AnswerBrowserExercise/hooks/useEditorState.tsservices/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.tsservices/tmc/src/components/AnswerBrowserExercise/hooks/useTestRun.tsservices/tmc/src/util/pyodideLoader.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- services/tmc/src/components/AnswerBrowserExercise/hooks/useTestRun.ts
- services/tmc/public/browserTestWorker.js
- services/tmc/src/components/AnswerBrowserExercise/components/RunOutputContent.tsx
- services/tmc/src/components/AnswerBrowserExercise/hooks/useEditorState.ts
services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts
Outdated
Show resolved
Hide resolved
services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
dependencies
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
services/tmc/public/browserTestWorker.js (1)
26-35:⚠️ Potential issue | 🟡 MinorPipeline warning:
stderris captured but never forwarded or read.
stderris accumulated in thesetStderrbatched handler but never posted to the main thread and never included in any message. Either forward it (likerunWorker.jsdoes with{ type: "stderr" }) or remove the buffer to silence the warning and avoid misleading future readers.💡 Proposed fix (forward stderr as a warning, consistent with runWorker.js)
pyodide.setStderr({ batched: function (msg) { - stderr += msg + "\n" + console.warn("[browserTestWorker stderr]", msg) }, })And remove
var stderr = ""on line 26.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/browserTestWorker.js` around lines 26 - 35, The stderr buffer is never used; remove the unused var stderr declaration and change the pyodide.setStderr batched handler to forward messages to the main thread (e.g., postMessage({ type: "stderr" or "warning", message: msg })) instead of accumulating them — update the pyodide.setStderr batched function in browserTestWorker.js to mirror the runWorker.js pattern so stderr is emitted to the main thread.
🧹 Nitpick comments (4)
shared-module/packages/common/src/utils/responseHeaders.js (1)
74-74: Remove the commented-out legacy CSP from Line 74.Keeping the old CSP as an inline comment is noisy and can mislead future edits. Keep the active policy only, and retain the rationale comments above.
Suggested cleanup
- value: "default-src * 'self' data: 'unsafe-inline' 'unsafe-eval'; worker-src 'self' blob:", // "default-src 'self'; connect-src 'self'; font-src 'self'; frame-src *; img-src 'self' data: blob: https://storage.googleapis.com; script-src 'self' 'unsafe-eval' 'unsafe-inline' blob:; style-src 'self' 'unsafe-inline'", + value: "default-src * 'self' data: 'unsafe-inline' 'unsafe-eval'; worker-src 'self' blob:",As per coding guidelines, "Prefer comment quality over comment quantity. Do not add comments that simply restate what the code already says..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/utils/responseHeaders.js` at line 74, Remove the commented-out legacy CSP from the 'value' property in responseHeaders.js so only the active policy remains; locate the object with the value: "default-src * 'self' data: 'unsafe-inline' 'unsafe-eval'; worker-src 'self' blob:" and delete the trailing commented legacy string (the old "default-src 'self'..." content) while keeping any explanatory rationale comments above intact.services/tmc/public/runWorkerTemplate.py (2)
56-56: Use a descriptive filename incompile()for clearer tracebacks.
" "(a space) as the filename argument appears in user-facing tracebacks and error messages. A descriptive string like"<user-script>"improves debuggability.💡 Proposed fix
- user_code = compile(wrapped, " ", "exec") + user_code = compile(wrapped, "<user-script>", "exec")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/runWorkerTemplate.py` at line 56, Replace the non-descriptive filename used when compiling user code so tracebacks show a meaningful source: change the compile call that creates user_code (compile(wrapped, " ", "exec")) to use a descriptive filename like "<user-script>" or "<user-code>" (referencing the variables wrapped and user_code) so exceptions and tracebacks reference the user script name instead of a blank space.
60-65: Broadexcept Exceptionis intentional here — acceptable for user-code sandboxing.The catch is necessary to report any parse/transform error back through
printErrorrather than crashing the worker. The Ruff BLE001 warning can be suppressed inline.💡 Suppress the Ruff warning without changing behavior
-except Exception: +except Exception: # noqa: BLE001🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/public/runWorkerTemplate.py` around lines 60 - 65, Update the broad exception suppression to silence Ruff BLE001 without changing behavior: leave the try/except and the existing error reporting (sys.exc_info, traceback.extract_tb, printError, exit()) intact but add an inline ruff/noqa comment to the except line (e.g., append a "# noqa: BLE001" or equivalent ruff suppression) so the linter is suppressed while keeping the broad catch for sandboxed user code.services/tmc/package.json (1)
62-63:pyodidebelongs indevDependencies, notdependencies.Per
next.config.js, Pyodide is explicitly excluded from the client/server bundle and loaded from a CDN at runtime. The npm package is used only at build time for the version JSON and TypeScript types. Listing it as a runtime dependency inflates the production install and may cause confusion.💡 Proposed fix
- "zstddec": "^0.1.0", - "pyodide": "^0.29.3" + "zstddec": "^0.1.0" }, "devDependencies": { + "pyodide": "^0.29.3",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tmc/package.json` around lines 62 - 63, Move the "pyodide" entry out of dependencies into devDependencies in package.json: remove "pyodide": "^0.29.3" from the dependencies block and add it under devDependencies so it's only present at build time (per next.config.js which excludes Pyodide from the runtime bundle); ensure package.json still contains "zstddec" in dependencies and update any lockfile by running the appropriate install command afterwards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/tmc/public/browserTestWorker.js`:
- Around line 13-19: getPyodide() currently caches a rejected promise in the
module-level variable pyodidePromise so any load failure blocks retries; change
the load logic so that when calling loadPyodide({ indexURL: PYODIDE_INDEX_URL })
you attach a .catch handler that sets pyodidePromise = null (so subsequent calls
retry) and then rethrows the error; update getPyodide (and the pyodidePromise
assignment) to use this fault-tolerant promise handling while keeping the
cached-success behavior.
In `@services/tmc/public/runWorker.js`:
- Around line 67-71: The parameter `tb` in the `self.printError` function is
unused; rename it to `_tb` (or prefix it with an underscore) in the
`self.printError = function (message, kind, line, tb)` signature to silence the
linter and signal intent, and ensure no other code relies on the original `tb`
name (update any callers if necessary); do not otherwise change the function
body that constructs `msg`, posts the `run_error` message, and sets
`runHadError`.
- Around line 73-75: The local accumulation buffer stderr is declared and
appended to but never read; remove the unused variable declaration "var stderr =
\"\"" and any assignments/reset of stderr (and the code that concatenates into
it) so only real-time postMessage({ type: "stderr", ... }) is relied upon;
search for the symbol stderr in runWorker.js and delete its declaration and any
writes (but do not change the postMessage calls or self.exit() behavior).
In `@services/tmc/public/runWorkerTemplate.py`:
- Around line 29-48: The ast.Module construction for variable wrapped is missing
the required type_ignores argument; update the ast.Module(...) call that wraps
the AsyncFunctionDef named "__run__" to include type_ignores=[] (i.e.,
ast.Module(body=[...], type_ignores=[])) so it conforms to Python 3.8+ AST API
before calling ast.fix_missing_locations(wrapped).
In `@services/tmc/scripts/inject-pyodide-version.cjs`:
- Line 27: The file ends without a trailing newline causing prettier to fail;
open the file containing the console.log statement console.log("Injected Pyodide
index URL:", indexUrl) and add a single newline character at the end of the file
(after that console.log line) so the file terminates with a newline.
- Around line 16-23: The injectIndexUrl function currently overwrites the file
even when the regex doesn't match; update it to first test for a match (e.g.,
use the same RegExp or compare content before/after) and only perform
fs.writeFileSync when a replacement actually occurred, and if no match is found
raise/throw an explicit error or log and exit so the build fails instead of
silently continuing; reference injectIndexUrl and indexUrlStr and the /var
PYODIDE_INDEX_URL = "[^"]*"/ pattern when making the change.
- Around line 4-5: The ESLint rule `@typescript-eslint/no-require-imports` is
incorrectly flagging require("fs") and require("path") in this CommonJS script;
add an override in the ESLint config for the *.cjs glob to disable that rule
(turn off `@typescript-eslint/no-require-imports` for .cjs files) so require() is
allowed in inject-pyodide-version.cjs rather than adding inline disables in the
file.
---
Duplicate comments:
In `@services/tmc/public/browserTestWorker.js`:
- Around line 26-35: The stderr buffer is never used; remove the unused var
stderr declaration and change the pyodide.setStderr batched handler to forward
messages to the main thread (e.g., postMessage({ type: "stderr" or "warning",
message: msg })) instead of accumulating them — update the pyodide.setStderr
batched function in browserTestWorker.js to mirror the runWorker.js pattern so
stderr is emitted to the main thread.
---
Nitpick comments:
In `@services/tmc/package.json`:
- Around line 62-63: Move the "pyodide" entry out of dependencies into
devDependencies in package.json: remove "pyodide": "^0.29.3" from the
dependencies block and add it under devDependencies so it's only present at
build time (per next.config.js which excludes Pyodide from the runtime bundle);
ensure package.json still contains "zstddec" in dependencies and update any
lockfile by running the appropriate install command afterwards.
In `@services/tmc/public/runWorkerTemplate.py`:
- Line 56: Replace the non-descriptive filename used when compiling user code so
tracebacks show a meaningful source: change the compile call that creates
user_code (compile(wrapped, " ", "exec")) to use a descriptive filename like
"<user-script>" or "<user-code>" (referencing the variables wrapped and
user_code) so exceptions and tracebacks reference the user script name instead
of a blank space.
- Around line 60-65: Update the broad exception suppression to silence Ruff
BLE001 without changing behavior: leave the try/except and the existing error
reporting (sys.exc_info, traceback.extract_tb, printError, exit()) intact but
add an inline ruff/noqa comment to the except line (e.g., append a "# noqa:
BLE001" or equivalent ruff suppression) so the linter is suppressed while
keeping the broad catch for sandboxed user code.
In `@shared-module/packages/common/src/utils/responseHeaders.js`:
- Line 74: Remove the commented-out legacy CSP from the 'value' property in
responseHeaders.js so only the active policy remains; locate the object with the
value: "default-src * 'self' data: 'unsafe-inline' 'unsafe-eval'; worker-src
'self' blob:" and delete the trailing commented legacy string (the old
"default-src 'self'..." content) while keeping any explanatory rationale
comments above intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/tmc/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
services/tmc/package.jsonservices/tmc/public/browserTestWorker.jsservices/tmc/public/runWorker.jsservices/tmc/public/runWorkerTemplate.pyservices/tmc/scripts/inject-pyodide-version.cjsservices/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.tsservices/tmc/src/util/pyodide-version.jsonservices/tmc/src/util/pyodideConfig.tsservices/tmc/src/util/pyodideLoader.tsshared-module/packages/common/src/utils/responseHeaders.js
✅ Files skipped from review due to trivial changes (1)
- services/tmc/src/util/pyodide-version.json
🚧 Files skipped from review as they are similar to previous changes (2)
- services/tmc/src/components/AnswerBrowserExercise/hooks/useRunOutput.ts
- services/tmc/src/util/pyodideLoader.ts
| function getPyodide() { | ||
| if (pyodidePromise !== null) { | ||
| return pyodidePromise | ||
| } | ||
| pyodidePromise = loadPyodide({ indexURL: PYODIDE_INDEX_URL }) | ||
| return pyodidePromise | ||
| } |
There was a problem hiding this comment.
pyodidePromise is permanently cached on rejection — worker cannot recover.
Unlike runWorker.js (which resets pyodidePromise = null in a .catch), this getPyodide() permanently holds a rejected promise. Any CDN or load failure makes all subsequent test runs fail immediately with no retry.
🐛 Proposed fix
function getPyodide() {
if (pyodidePromise !== null) {
return pyodidePromise
}
- pyodidePromise = loadPyodide({ indexURL: PYODIDE_INDEX_URL })
- return pyodidePromise
+ pyodidePromise = loadPyodide({ indexURL: PYODIDE_INDEX_URL }).catch(function (err) {
+ pyodidePromise = null
+ throw err
+ })
+ return pyodidePromise
}📝 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.
| function getPyodide() { | |
| if (pyodidePromise !== null) { | |
| return pyodidePromise | |
| } | |
| pyodidePromise = loadPyodide({ indexURL: PYODIDE_INDEX_URL }) | |
| return pyodidePromise | |
| } | |
| function getPyodide() { | |
| if (pyodidePromise !== null) { | |
| return pyodidePromise | |
| } | |
| pyodidePromise = loadPyodide({ indexURL: PYODIDE_INDEX_URL }).catch(function (err) { | |
| pyodidePromise = null | |
| throw err | |
| }) | |
| return pyodidePromise | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/public/browserTestWorker.js` around lines 13 - 19, getPyodide()
currently caches a rejected promise in the module-level variable pyodidePromise
so any load failure blocks retries; change the load logic so that when calling
loadPyodide({ indexURL: PYODIDE_INDEX_URL }) you attach a .catch handler that
sets pyodidePromise = null (so subsequent calls retry) and then rethrows the
error; update getPyodide (and the pyodidePromise assignment) to use this
fault-tolerant promise handling while keeping the cached-success behavior.
| self.printError = function (message, kind, line, tb) { | ||
| var msg = kind + " on line " + line + ": " + message | ||
| self.postMessage({ type: "run_error", message: msg, output: stdout }) | ||
| runHadError = true | ||
| } |
There was a problem hiding this comment.
Pipeline warning: unused tb parameter in printError.
tb (traceback frames) is accepted but never used. Prefix it with _ to silence the linter and signal intent.
💡 Proposed fix
-self.printError = function (message, kind, line, tb) {
+self.printError = function (message, kind, line, _tb) {🧰 Tools
🪛 GitHub Actions: Code Style
[warning] 67-67: eslint: warning: 'tb' is defined but never used. (no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/public/runWorker.js` around lines 67 - 71, The parameter `tb` in
the `self.printError` function is unused; rename it to `_tb` (or prefix it with
an underscore) in the `self.printError = function (message, kind, line, tb)`
signature to silence the linter and signal intent, and ensure no other code
relies on the original `tb` name (update any callers if necessary); do not
otherwise change the function body that constructs `msg`, posts the `run_error`
message, and sets `runHadError`.
| var runHadError = false | ||
| var stdout = "" | ||
| var stderr = "" |
There was a problem hiding this comment.
Pipeline warning: stderr accumulation buffer is never read.
stderr is collected byte-by-byte (line 125) but the accumulated string is never consumed — neither in self.exit() nor in error messages. The individual chunks are already forwarded in real-time via postMessage({ type: "stderr" }), so the buffer is redundant. Remove the variable and its reset assignments to fix the linter warning and reduce confusion.
💡 Proposed fix
var runHadError = false
var stdout = ""
-var stderr = "" stdout = ""
- stderr = ""
var stdoutDecoder = new TextDecoder("utf-8", { fatal: false })
var stderrDecoder = new TextDecoder("utf-8", { fatal: false }) if (chunk.length > 0) {
- stderr += chunk
self.postMessage({ type: "stderr", chunk: chunk })
}🧰 Tools
🪛 GitHub Actions: Code Style
[warning] 75-75: eslint: warning: 'stderr' is assigned a value but never used. (no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/public/runWorker.js` around lines 73 - 75, The local
accumulation buffer stderr is declared and appended to but never read; remove
the unused variable declaration "var stderr = \"\"" and any assignments/reset of
stderr (and the code that concatenates into it) so only real-time postMessage({
type: "stderr", ... }) is relied upon; search for the symbol stderr in
runWorker.js and delete its declaration and any writes (but do not change the
postMessage calls or self.exit() behavior).
| wrapped = ast.Module( | ||
| body=[ | ||
| ast.AsyncFunctionDef( | ||
| name="__run__", | ||
| args=ast.arguments( | ||
| posonlyargs=[], | ||
| args=[], | ||
| vararg=None, | ||
| kwonlyargs=[], | ||
| kw_defaults=[], | ||
| kwarg=None, | ||
| defaults=[], | ||
| ), | ||
| body=tree.body, | ||
| decorator_list=[], | ||
| returns=None, | ||
| ), | ||
| ], | ||
| ) | ||
| ast.fix_missing_locations(wrapped) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3.13 ast.Module type_ignores required argument
💡 Result:
In Python 3.13, ast.Module’s constructor signature is ast.Module(body, type_ignores) (no default shown), so creating it with only body can raise:
TypeError: Module() missing required argument 'type_ignores'(docs.python.org)
Fix
Pass an explicit empty list (or your collected TypeIgnore nodes):
import ast
mod = ast.Module(body=[ast.Pass()], type_ignores=[])type_ignores is the list of # type: ignore comments in the module (otherwise it’s typically an empty list). (docs.python.org)
Citations:
- 1: https://docs.python.org/zh-cn/3.13/library/ast.html?utm_source=openai
- 2: https://docs.python.org/3.10/library/ast.html?utm_source=openai
🌐 Web query:
Python 3.8 ast.Module constructor type_ignores parameter required
💡 Result:
In Python 3.8, ast.Module gained a new, required field: type_ignores. Its constructor signature effectively became:
ast.Module(body, type_ignores)(wheretype_ignoresis a list ofast.TypeIgnorenodes) [1].
So code that worked in 3.7 like:
ast.Module(body=[...])can fail in 3.8+ with:
TypeError: required field "type_ignores" missing from Module[2][3].
Fix when constructing AST manually
Pass an explicit list (usually empty):
import ast
mod = ast.Module(body=[...], type_ignores=[])What type_ignores is for
When parsing with type_comments=True, the parser records # type: ignore comment locations into Module.type_ignores; otherwise it’s an empty list [1]. (So ast.parse(...) will produce a Module(..., type_ignores=...) automatically; the issue is mainly with manual construction.)
Sources: Python 3.8 AST grammar / docs showing Module(stmt* body, type_ignore* type_ignores) and how # type: ignore becomes Module.type_ignores [1]; common error reports and fixes in real projects (e.g., Werkzeug/Flask, IPython/Jupyter contexts) [2][3].
[1] https://docs.activestate.com/activepython/3.8/python/library/ast.html
[2] https://stackoverflow.com/questions/63609384/typeerror-required-field-type-ignores-missing-from-module
[3] https://lightrun.com/answers/pallets-werkzeug-astmodule-signature-change-in-python-380a3-causes-typeerror
🌐 Web query:
ast.fix_missing_locations Python ast.Module type_ignores
💡 Result:
ast.fix_missing_locations(node) only fills in missing source-location attributes (lineno, col_offset, and optionally end_lineno, end_col_offset) so the tree can be passed to compile(). It does not add/repair required structural fields on nodes. [1]
In Python 3.8+, ast.Module’s shape is:
ast.Module(body, type_ignores)wheretype_ignoresis a list ofTypeIgnorenodes (i.e.,# type: ignorecomments). [2]
So if you manually construct a module and then call fix_missing_locations, you still must provide type_ignores yourself (usually []), e.g.:
m = ast.Module(body=[...], type_ignores=[])
m = ast.fix_missing_locations(m)
code = compile(m, filename="<ast>", mode="exec")type_ignores is populated by parsing when type comments are enabled; otherwise it’s an empty list. [3]
Sources: Python ast docs on fix_missing_locations [1] and ast.Module(..., type_ignores) / type_ignores meaning [2][3].
[1] https://docs.python.org/3.12/library/ast.html#ast.fix_missing_locations
[2] https://docs.python.org/3.12/library/ast.html#ast.Module
[3] https://docs.python.org/3.12/library/ast.html#ast.parse
ast.Module is missing the required type_ignores=[] argument.
In Python 3.8+, including Python 3.13 used by Pyodide, ast.Module requires a type_ignores parameter. Without it, the code will immediately raise TypeError: Module() missing required argument 'type_ignores' when executed.
Proposed fix
wrapped = ast.Module(
body=[
ast.AsyncFunctionDef(
name="__run__",
args=ast.arguments(
posonlyargs=[],
args=[],
vararg=None,
kwonlyargs=[],
kw_defaults=[],
kwarg=None,
defaults=[],
),
body=tree.body,
decorator_list=[],
returns=None,
),
],
+ type_ignores=[],
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/public/runWorkerTemplate.py` around lines 29 - 48, The
ast.Module construction for variable wrapped is missing the required
type_ignores argument; update the ast.Module(...) call that wraps the
AsyncFunctionDef named "__run__" to include type_ignores=[] (i.e.,
ast.Module(body=[...], type_ignores=[])) so it conforms to Python 3.8+ AST API
before calling ast.fix_missing_locations(wrapped).
| const fs = require("fs") | ||
| const path = require("path") |
There was a problem hiding this comment.
Pipeline failure: require() is forbidden by ESLint — fix by overriding the rule for .cjs files.
The @typescript-eslint/no-require-imports rule fires on both require("fs") and require("path") because the ESLint config applies globally. Since this is a CommonJS script (.cjs), require() is the only correct import mechanism — the rule should be disabled for this file or for the *.cjs glob in the ESLint config.
💡 Suggested fix (add override to ESLint config, not in this file)
// eslint.config.js (or .eslintrc equivalent)
+ {
+ files: ["**/*.cjs"],
+ rules: {
+ "@typescript-eslint/no-require-imports": "off",
+ },
+ },Alternatively, add a file-level disable comment as a quick fix:
+/* eslint-disable `@typescript-eslint/no-require-imports` */
const fs = require("fs")
const path = require("path")📝 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 fs = require("fs") | |
| const path = require("path") | |
| /* eslint-disable `@typescript-eslint/no-require-imports` */ | |
| const fs = require("fs") | |
| const path = require("path") |
🧰 Tools
🪛 ESLint
[error] 4-4: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
[error] 5-5: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
🪛 GitHub Actions: Code Style
[error] 4-4: ESLint: A require() style import is forbidden. (no-require-imports)
[error] 5-5: ESLint: A require() style import is forbidden. (no-require-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/scripts/inject-pyodide-version.cjs` around lines 4 - 5, The
ESLint rule `@typescript-eslint/no-require-imports` is incorrectly flagging
require("fs") and require("path") in this CommonJS script; add an override in
the ESLint config for the *.cjs glob to disable that rule (turn off
`@typescript-eslint/no-require-imports` for .cjs files) so require() is allowed in
inject-pyodide-version.cjs rather than adding inline disables in the file.
| function injectIndexUrl(filePath) { | ||
| let content = fs.readFileSync(filePath, "utf8") | ||
| content = content.replace( | ||
| /var PYODIDE_INDEX_URL = "[^"]*"/, | ||
| `var PYODIDE_INDEX_URL = ${indexUrlStr}`, | ||
| ) | ||
| fs.writeFileSync(filePath, content) | ||
| } |
There was a problem hiding this comment.
Silent no-op when the regex doesn't match — add a guard to detect stale/missing placeholder.
String.prototype.replace returns the original string unchanged when the pattern has no match. If var PYODIDE_INDEX_URL = "..." is absent or already substituted with a different quote style, the worker file is overwritten with its original content and the build log still prints "Injected…", making the failure invisible.
🛡️ Proposed fix
function injectIndexUrl(filePath) {
let content = fs.readFileSync(filePath, "utf8")
- content = content.replace(
- /var PYODIDE_INDEX_URL = "[^"]*"/,
- `var PYODIDE_INDEX_URL = ${indexUrlStr}`,
- )
+ const pattern = /var PYODIDE_INDEX_URL = "[^"]*"/
+ if (!pattern.test(content)) {
+ throw new Error(`PYODIDE_INDEX_URL placeholder not found in ${filePath}`)
+ }
+ content = content.replace(pattern, `var PYODIDE_INDEX_URL = ${indexUrlStr}`)
fs.writeFileSync(filePath, content)
}📝 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.
| function injectIndexUrl(filePath) { | |
| let content = fs.readFileSync(filePath, "utf8") | |
| content = content.replace( | |
| /var PYODIDE_INDEX_URL = "[^"]*"/, | |
| `var PYODIDE_INDEX_URL = ${indexUrlStr}`, | |
| ) | |
| fs.writeFileSync(filePath, content) | |
| } | |
| function injectIndexUrl(filePath) { | |
| let content = fs.readFileSync(filePath, "utf8") | |
| const pattern = /var PYODIDE_INDEX_URL = "[^"]*"/ | |
| if (!pattern.test(content)) { | |
| throw new Error(`PYODIDE_INDEX_URL placeholder not found in ${filePath}`) | |
| } | |
| content = content.replace(pattern, `var PYODIDE_INDEX_URL = ${indexUrlStr}`) | |
| fs.writeFileSync(filePath, content) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/scripts/inject-pyodide-version.cjs` around lines 16 - 23, The
injectIndexUrl function currently overwrites the file even when the regex
doesn't match; update it to first test for a match (e.g., use the same RegExp or
compare content before/after) and only perform fs.writeFileSync when a
replacement actually occurred, and if no match is found raise/throw an explicit
error or log and exit so the build fails instead of silently continuing;
reference injectIndexUrl and indexUrlStr and the /var PYODIDE_INDEX_URL =
"[^"]*"/ pattern when making the change.
|
|
||
| injectIndexUrl(runWorkerPath) | ||
| injectIndexUrl(browserTestWorkerPath) | ||
| console.log("Injected Pyodide index URL:", indexUrl) No newline at end of file |
There was a problem hiding this comment.
Pipeline failure: prettier requires a trailing newline at end of file.
Add a newline character after line 27.
🧰 Tools
🪛 ESLint
[error] 27-27: Insert ⏎
(prettier/prettier)
🪛 GitHub Actions: Code Style
[error] 27-27: prettier/prettier: Insert ⏎
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/tmc/scripts/inject-pyodide-version.cjs` at line 27, The file ends
without a trailing newline causing prettier to fail; open the file containing
the console.log statement console.log("Injected Pyodide index URL:", indexUrl)
and add a single newline character at the end of the file (after that
console.log line) so the file terminates with a newline.
1 participant
Summary by CodeRabbit
New Features
Localization