Repository navigation
Configure Louis chatbot for the Python SICP textbook - #4085
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCourse configuration now includes Louis chatbot enablement and prompting. The chatbot API sends the configured prompt during initialization and message continuation. Chatbot rendering moves from the JavaScript layout to the Python layout with session, feature-flag, section, and visible-text context. ChangesLouis chatbot configuration and integration
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request integrates a new Chatbot component into the SICP layout, conditionally rendering it based on user session status and a feature flag. It introduces helper functions to extract the current section from the URL and to retrieve visible text paragraphs from the viewport. Feedback on the changes highlights a potential issue where a null textContent could append the string 'null\n' to the aggregated text, suggesting a null check before appending.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Coverage Report for CI Build 30463511211Coverage decreased (-0.008%) to 42.998%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Fix crash when stored playground language state is invalid Validate the stored chapter+variant combination against ALL_LANGUAGES when loading from localStorage. Falls back to the default language config if the combination is invalid (e.g. chapter 1 + explicit-control). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fall back to defaultLanguageConfig for invalid stored language state When the stored chapter+variant combination is not in ALL_LANGUAGES, reset to `defaultLanguageConfig` (a known-valid pair) rather than to the individual default context fields, so the fallback is explicitly "the default language config" as the issue describes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>
* docs: add local backend setup steps to README Fills the gap between "point REACT_APP_BACKEND_URL at localhost:4000" and an actually-running backend, which otherwise just shows the "under maintenance" page. Also corrects the postgres docker command from the backend repo's own README, which has a stray -e flag before -p 5432:5432 that prevents the port from actually being published. * docs: add "Running your own py-slang" section Mirrors the existing "Running your own js-slang" section, linking out to py-slang's own README (added in a companion PR there) rather than duplicating the instructions here. * Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update README.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Shrey Jain <101659971+Shrey5132@users.noreply.github.com> Co-authored-by: Martin Henz <henz@comp.nus.edu.sg>
…4068) * feat(cseMachine): rename global/program frames and annotate globals - Rename the top two CSE Machine frame labels to match Python's LEGB terminology: "Global" -> "Built-in functions", "Program" -> "Globals" (#4042). - Thread the new optional CseSerializedEnvFrame.globalNames field (companion source-academy/plugins + source-academy/py-slang PRs) through CseSnapshotAdapter onto the fake Environment, and render it as a "globals: x, y" annotation at the top of a call frame the moment it's created, so it's clear those names resolve straight to the global frame instead of the usual enclosing-scope chain (#4041). - Add @sourceacademy/common-cse-machine as a direct dependency: it's a peerDependency of @sourceacademy/web-cse-machine whose types frontend re-exports and uses directly, but frontend never installed it itself, so those types were silently resolving to `any` under skipLibCheck. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cseMachine): stack globals annotation below frame name, fix overflow, and occlude arrows behind header text - Move the globals annotation to its own row stacked below the frame name (rather than beside it or inside the box as a binding), since frame names are arbitrary function names of unpredictable length and can't safely share a row with anything else. - Widen the frame box to fit the annotation label the same way binding text already widens it (capped at the same FrameDefaultWidth), so it no longer gets ellipsis-truncated or rendered left of the frame's own border when the label is longer than the box's other content. - Add an opaque background rect behind the frame name and the globals annotation so the parent/tail arrow passing through that header gap is fully hidden behind the text, instead of showing through the empty space between glyphs (arrows are already layered behind content, but plain Konva text has no opaque fill of its own). Closes #4069. * fix(cseMachine): fix assignment animation crash for statement-only-assignment languages InstrType.ASSIGNMENT constructed AssignmentAnimation with the *current* (post-assignment) top-of-stash item, which is only valid because a JS assignment *expression* re-pushes its value. A Python assignment *statement* pushes nothing back, so the stash is genuinely empty afterwards, the non-null assertion lied, and the constructor threw reading `stashItem.index` on undefined — silently, since the try/catch around CseAnimation.updateAnimation() had no logging. Use the *previous* stash's top item instead, matching how InstrType.POP (also a pure consumer) already does this correctly. Also add the missing console.error so future animation regressions in this path aren't invisible again. Closes #4072. * fix(cseMachine): stop the arrow-occlusion patch from being wider than the text it backs Frame.tsx's header-text backing rects used Text.width(), which floors at Config.TextMinWidth (30px) — a minimum meant for binding key/value alignment, unrelated to how wide the text visually is. For a short name like a one-letter function ("f", 9px of actual ink), that floor is wider than Config.FramePaddingX (20px, the arrow's x-offset), so the padded-out rect covered the arrow well past where the glyph itself ends — an oversized, unnecessary gap in the arrow line even when the text was nowhere near wide enough to actually cross it. Use getTextWidth(text) directly instead, sized to what's actually rendered. * chore: bump common-cse-machine/web-cse-machine, drop published-type workaround casts Now that @sourceacademy/common-cse-machine@0.2.0 (declaring globalNames) and @sourceacademy/web-cse-machine@2.0.0 are actually published, bump both dependencies and simplify the read sites that were casting around the not-yet-published type. Frame.tsx still needs one cast: `Env` (js-slang's real Environment type) has no globalNames field regardless of any package version, since it's an ad-hoc property CseSnapshotAdapter.ts stashes onto the fake Environment object — updated that comment to say so accurately. Also adds @sourceacademy/common-cse-machine to .yarnrc.yml's npmPreapprovedPackages, matching the existing entries for web-cse-machine/conductor/js-slang: yarn's npmMinimalAgeGate (3-day quarantine on freshly published versions) was blocking install of the version just published in the companion plugins release. * fix(cseMachine): scope the Python LEGB frame rename to the Conductor snapshot path only The #4042 rename ("Global" -> "Built-in functions", "Program" -> "Globals") was motivated by Python's LEGB terminology, but Frame.tsx's frameNames lookup applied unconditionally, so the legacy non-conductor path (conductor.enable off, real js-slang interpreter) picked it up too — a real regression against js-slang's own established naming, confirmed live and flagged by Gemini's review on this PR. Gate the rename on Layout.snapshotMode, which is true only while rendering a Conductor snapshot (currently Python-only) and false for the entire legacy path — evalCodeSaga never calls CseMachine.renderSnapshot() when conductor.enable is off, so this is a reliable, already-existing signal rather than a new one. Other changes in this PR are left as-is: the globalNames annotation is already inert for legacy js-slang Environments (they never carry that ad-hoc field), the arrow-occlusion background-rect fix is a general visual correctness fix unrelated to any language, and the assignment-animation and frame-transition-timing fixes don't touch frame naming at all. Adds regression coverage on both sides of the gate: a real js-slang EnvTree rendered outside snapshot mode keeps "Global"/"Program", and a Python snapshot still gets "Built-in functions"/"Globals". * fix(cseMachine): apply center-alignment offset to the globals annotation too reassignCoordinatesX applied the center-alignment textOffset to the frame name but not to _globalNamesText, leaving the annotation flush-left under a centered name whenever center-alignment is on. Flagged independently by CodeRabbit and Shrey's review on #4068. Also extracts the headerRowHeight formula (duplicated between the constructor and reassignCoordinatesY) into a single module-level constant, per CodeRabbit's nitpick. Added a regression test; confirmed it fails without the fix (30 vs 80) and passes with it. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Shrey Jain <101659971+Shrey5132@users.noreply.github.com>
…4084) * Show a popup for input() requests instead of hanging (py-slang#190) Wires up Conductor's new receiveInputRequest hook: a handleInputRequest saga listens for input requests from the runner, shows the existing PromptDialog popup, and answers via hostPlugin.sendInput. The home tab and the viz CSE Machine tab share the same evalCodeConductorSaga run, so this single fix covers both surfaces from the issue. Also adds isWaitingForInput workspace state, used to suspend the execTime watchdog while a popup is open so a user typing an answer doesn't get their run force-stopped mid-prompt. Points @sourceacademy/conductor at the local checkout (portal:) for dev while the protocol change there is unreleased; repin to a real published version once that lands upstream. * chore: deps update * Fix uncaught rejection when the input popup is dismissed via onClose showSimplePromptDialog's underlying promise rejects if the dialog is dismissed via its onClose path (e.g. clicking outside) rather than a button response. The inner try/finally in handleInputRequest reset isWaitingForInput on that path but didn't catch the rejection, so it escaped the saga's while(true) loop and killed the task permanently - leaving the runner blocked forever with no way to answer this or any future input() call. Catch it and send an empty string, same as an explicit Cancel. Caught by gemini-code-assist review on PR #4084. * Repin @sourceacademy/conductor to the published npm 0.6.0 The npm publish for conductor 0.6.0 landed, so the git-tag stopgap pin isn't needed anymore - back to the normal semver-via-npm-registry pin this repo used before. Verified: tsc --noEmit clean against the real published package.
…channel (#4083) * feat(cseMachine): wire breakpoint step indices from the CSE snapshot channel CseMachineHostPlugin.receiveSnapshots now takes breakpointSteps as a second argument (paired with the @sourceacademy/web-cse-machine protocol change), and handleCseSnapshots dispatches WorkspaceActions.updateBreakpointSteps with it alongside the existing snapshot/stepsTotal updates. The double- chevron breakpoint-navigation buttons in SideContentCseMachine already read props.breakpointSteps and are rendered unconditionally for the conductor/Python flow — they just needed real data instead of an empty array. No new UI. Depends on a py-slang change that detects breakpoint() in the CSE machine (source-academy/py-slang issue #189). package.json/yarn.lock currently point @sourceacademy/common-cse-machine and @sourceacademy/web-cse-machine at a local yarn-link portal to /home/vakshay/plugins for development; swap for a real published semver range once that plugins PR merges. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: revert local yarn-link dev scaffolding for cse-machine packages package.json/yarn.lock were pointing @sourceacademy/common-cse-machine and @sourceacademy/web-cse-machine at a local yarn-link portal (/home/vakshay/plugins) for development. That doesn't resolve for anyone else or CI, and web-cse-machine can't be pinned to the plugins branch via Yarn's git+workspace protocol either (it depends on the sibling common-cse-machine workspace via workspace:*, and Yarn's single-workspace git fetch doesn't build sibling workspaces first, so the prepack/rollup step can't find its dist). Reverting to the original published semver ranges so this PR's dependency diff is clean. This PR depends on source-academy/plugins#55 (adds breakpointSteps to the CSE snapshot protocol) merging and publishing to npm before it will typecheck/build — opening as a draft until then. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump cse-machine deps to the published versions with breakpointSteps @sourceacademy/common-cse-machine ^0.2.0 -> ^0.3.0 and @sourceacademy/web-cse-machine ^2.0.0 -> ^3.0.0, now that source-academy/plugins#55 has published (bumping the same two packages, plus runner-cse-machine, to carry breakpointSteps over the CSE snapshot channel). Unblocks this PR: tsc --noEmit passes against the real published CseMachineHostPlugin.receiveSnapshots(snapshots, breakpointSteps) signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: formattin --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: henz <henz@comp.nus.edu.sg>
adding conductor info
…out symbols; add host plugin callbacks; relax CseMachineHostPlugin signature
RichDom2185
marked this pull request as draft
July 14, 2026 13:40
Member
|
@Isha-Sovasaria I'm marking as draft since there are many merge conflicts. Please re-mark as ready when ready |
…channel (#4083) * feat(cseMachine): wire breakpoint step indices from the CSE snapshot channel CseMachineHostPlugin.receiveSnapshots now takes breakpointSteps as a second argument (paired with the @sourceacademy/web-cse-machine protocol change), and handleCseSnapshots dispatches WorkspaceActions.updateBreakpointSteps with it alongside the existing snapshot/stepsTotal updates. The double- chevron breakpoint-navigation buttons in SideContentCseMachine already read props.breakpointSteps and are rendered unconditionally for the conductor/Python flow — they just needed real data instead of an empty array. No new UI. Depends on a py-slang change that detects breakpoint() in the CSE machine (source-academy/py-slang issue #189). package.json/yarn.lock currently point @sourceacademy/common-cse-machine and @sourceacademy/web-cse-machine at a local yarn-link portal to /home/vakshay/plugins for development; swap for a real published semver range once that plugins PR merges. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: revert local yarn-link dev scaffolding for cse-machine packages package.json/yarn.lock were pointing @sourceacademy/common-cse-machine and @sourceacademy/web-cse-machine at a local yarn-link portal (/home/vakshay/plugins) for development. That doesn't resolve for anyone else or CI, and web-cse-machine can't be pinned to the plugins branch via Yarn's git+workspace protocol either (it depends on the sibling common-cse-machine workspace via workspace:*, and Yarn's single-workspace git fetch doesn't build sibling workspaces first, so the prepack/rollup step can't find its dist). Reverting to the original published semver ranges so this PR's dependency diff is clean. This PR depends on source-academy/plugins#55 (adds breakpointSteps to the CSE snapshot protocol) merging and publishing to npm before it will typecheck/build — opening as a draft until then. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: bump cse-machine deps to the published versions with breakpointSteps @sourceacademy/common-cse-machine ^0.2.0 -> ^0.3.0 and @sourceacademy/web-cse-machine ^2.0.0 -> ^3.0.0, now that source-academy/plugins#55 has published (bumping the same two packages, plus runner-cse-machine, to carry breakpointSteps over the CSE snapshot channel). Unblocks this PR: tsc --noEmit passes against the real published CseMachineHostPlugin.receiveSnapshots(snapshots, breakpointSteps) signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: formattin --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: henz <henz@comp.nus.edu.sg>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Isha-Sovasaria
changed the title
martin-henz
self-requested a review
July 29, 2026 14:59
martin-henz
approved these changes
Jul 29, 2026
martin-henz
left a comment
martin-henz
left a comment
Member
There was a problem hiding this comment.
multiple thorough review rounds
Contributor
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Some important pointers that I have noticed:
|
RichDom2185
added a commit
that referenced
this pull request
Jul 30, 2026
2 of 7 tasks
RichDom2185
added a commit
that referenced
this pull request
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed