Update Python textbook layout to load from json_py/ and preserve chatbot section/text behavior#4085
Open
Isha-Sovasaria wants to merge 80 commits into
Open
Update Python textbook layout to load from json_py/ and preserve chatbot section/text behavior#4085Isha-Sovasaria wants to merge 80 commits into
Isha-Sovasaria wants to merge 80 commits into
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe SicpPy layout now obtains login state, derives the current section and visible paragraph text, and conditionally renders the Chatbot when the user is logged in and the SICp chatbot feature flag is enabled. ChangesSicpPy chatbot integration
Estimated code review effort: 2 (Simple) | ~10 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.
Comment on lines
+51
to
+52
| const text = div.textContent; | ||
| visibleParagraphs += text + '\n'; |
Contributor
There was a problem hiding this comment.
If div.textContent is null, appending it directly to visibleParagraphs will result in the string "null\n" being added, which can pollute the text sent to the chatbot. It is safer to check if textContent is non-null before appending.
const text = div.textContent;
if (text) {
visibleParagraphs += text + '\n';
}
Coverage Report for CI Build 29793920176Coverage increased (+0.02%) to 42.32%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>
…out symbols; add host plugin callbacks; relax CseMachineHostPlugin signature
…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>
# Conflicts: # src/new_routes/sicpjs/_layout.tsx # src/new_routes/sicpy/_layout.tsx
RichDom2185
requested changes
Jul 19, 2026
| {isLoggedIn && Constants.featureFlags.enableSicpChatbot && ( | ||
| <Chatbot getSection={getSection} getText={getText} /> | ||
| )} | ||
| {/* Chatbot moved to Python layout */} |
Author
There was a problem hiding this comment.
It is removed because Prof Martin said to keep the chatbot visible only for python versions of the books and it shouldnt be visible on the js side
Member
There was a problem hiding this comment.
I see, in that case, hiding it for JS does not necessarily mean removing the feature. We should put it under a feature flag instead, similar to the Louis chatbot, but gated by language?
Author
There was a problem hiding this comment.
I haven't removed the feature.The original frontend had this React Component in the layout for the js textbook.Now I added that to the python textbook's layout.However, if I choose to do a feature flag-i feel its unnecessary because there is no Louis support for js -the RAG pipeline is purely for python book in the backend.And then we would have the same code in two layouts (code duplication) with a guarantee that it never will be switched on for js -just to keep this bit of code in this place.I think best to delete from here and let it exist In the correct place-python layout
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update the Python textbook layout to use the Python JSON backend and ensure the chatbot section/text extraction logic remains aligned with the existing SICP JS flow.
Include selected playground language in chatbot requests so backend receives language context.
[continueChat] now accept an optional [language] and send a serialized {chapter, variant, displayName, mainLanguage} in the request body.