Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "Pair Visualisation" feature to the CSE Machine, enabling stream lineage visualization through a new toggleable mode. The changes include state management for streamsPointSteps, new arrow components, and logic to link array values to their originating functions. Feedback focuses on removing non-portable local file paths in package.json, deleting an accidentally committed build artifact, and cleaning up leftover debug logs, commented-out code, and redundant logic in the FnValue component.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…into streamVis Please enter a commit message to explain why this merge is necessary, fix
Removed local resolution for 'js-slang' package.
Coverage Report for CI Build 26861602081Coverage decreased (-0.1%) to 41.752%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
| // Clear arrows to prevent duplicates from multiple draw calls | ||
|
|
||
| if (pairs != undefined) { | ||
| for (const pair of pairs) { | ||
| const target = Layout.values.get(pair); | ||
| if (target) { | ||
| this.addArrow(target); | ||
| } | ||
| } | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This program seems to get stuck at step 81, now: 
|
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
I can reproduce the issue flagged by Prof @martin-henz. It's a blocker
Akshay-2007-1
left a comment
Akshay-2007-1
left a comment
There was a problem hiding this comment.
- FnValue.tsx — the main fix (root cause of the bug)
The bug: The draw() method runs on every render step. At the very start of draw(), it did this._streamArrows = [] —
wiping the arrow list. But before that wipe, it was calling getStreamLineage() and trying to use the result. So the
arrows were built, then immediately thrown away before they could be rendered. The stream visualization never actually
drew anything.
The fix: Move getStreamLineage() inside the if (getPairCreationMode()) block, after the wipe. Now arrows are built
fresh each render, in the right order.
Also removed: ~30 lines of dead constructor code that tried to draw stream arrows during object construction — but
since draw() always wiped _streamArrows = [] at the start, anything set in the constructor was always discarded before
rendering. It was completely unreachable code that had no effect.
Also fixed: centerX (the horizontal center of the function circles) was being computed after the arrows were created.
Arrows use centerX for their starting position, so they could start from x=0. Moved centerX calculation to the very
top of draw().
Also removed: A throw new Error('Closure has no main reference...') that could never actually fire (the value is
always set before draw() is called), but if it ever did, it would crash the whole visualization silently.
- ArrayValue.tsx — removed dead constructor code
The pair (array) constructor had code that tried to notify its parent function value about itself, so the function
could draw an arrow to it. This was the other half of the dead code above — it set Layout.pendingFnLink = true as a
signal. Since FnValue's constructor code reading that flag was already dead, this code had no effect either. Removed
it, along with the unused Closure import.
- CseMachineLayout.tsx — removed dead state variables
Six static fields (currentDarkPairs, currentLightPairs, etc.) were declared and reset in setContext(), but never read
Also removed: A throw new Error('Closure has no main reference...') that could never actually fire (the value is always set before draw() is called), but
if it ever did, it would crash the whole visualization silently.
- ArrayValue.tsx — removed dead constructor code
The pair (array) constructor had code that tried to notify its parent function value about itself, so the function could draw an arrow to it. This was the
other half of the dead code above — it set Layout.pendingFnLink = true as a signal. Since FnValue's constructor code reading that flag was already dead,
this code had no effect either. Removed it, along with the unused Closure import.
- CseMachineLayout.tsx — removed dead state variables
Six static fields (currentDarkPairs, currentLightPairs, etc.) were declared and reset in setContext(), but never read anywhere — they were leftovers from
an earlier implementation. Also removed pendingFnLink (the boolean flag used by the dead constructor code above) and streamNodes = null which was assigned
but never used in the JSX render.
- updateInspector.ts — stop silently swallowing errors
The catch block previously had a comment saying errors were "most likely harmless, we can pretty much ignore this." This meant if the CSE machine
visualization crashed during rendering, it would look like the diagram was just "stuck" — no error, no feedback. Changed to console.error(...) so any
crash now shows up in the browser console with a real message.
I made an AI written summary for the changes I made in the recent commit for better understanding. @DaRealTristan do look into if the actual Stream dotted arrows work or my changes are breaking!
|
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:
WalkthroughThis PR adds pair-creation-mode visualization to the CSE machine diagram. It introduces new state and accessors in ChangesPair Creation Mode Visualization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/features/cseMachine/components/values/FnValue.tsx (1)
144-152: 💤 Low valueTighten typing and drop redundant optional chaining in
addArrow.
target: anydiscards type safety even thoughcalculateStepsonly handlesArrayValue. Also_streamArrowsis always initialized at declaration, so?.pushis a no-op.♻️ Proposed refactor
- addArrow(target: any): void { + addArrow(target: ArrayValue): void { // Check how many arrows already point to this specific target const currentCount = this._streamArrows.filter(arrow => arrow.target === target).length; // Pass the count as the offsetIndex - this._streamArrows?.push( + this._streamArrows.push( new ArrowFromStreamNullaryFn(this, currentCount).to(target) as ArrowFromStreamNullaryFn, ); }(Requires importing
ArrayValue; resolves the unusedindexparam at Line 326 as well by switching toarrow => arrow.draw().)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/cseMachine/components/values/FnValue.tsx` around lines 144 - 152, The addArrow method uses target: any and redundant optional chaining; tighten its signature to target: ArrayValue (import ArrayValue), remove the ?. and call this._streamArrows.push(...) directly, and keep creating the ArrowFromStreamNullaryFn(this, currentCount).to(target) cast as before; also ensure calculateSteps still expects ArrayValue and update any consumers accordingly (this resolves the unused index warning by enabling arrow => arrow.draw() elsewhere when iterating _streamArrows).src/features/cseMachine/components/arrows/ArrowFromStreamNullaryFn.tsx (1)
20-29: ⚡ Quick winReassess union-type geometry:
centerX/radiusaccess is safe here
GlobalFnValueandContValueboth declarereadonly radiusandcenterX(they’re set during layout), soArrowFromStreamNullaryFn’sfrom.centerX/from.radiususage won’t becomeNaN. Also,ArrowFromStreamNullaryFnis currently only instantiated fromsrc/features/cseMachine/components/values/FnValue.tsx(withthisas aFnValue).Minor: update the class doc comment at
ArrowFromStreamNullaryFn.tsxline 9 (“an GenericArrow”).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/cseMachine/components/arrows/ArrowFromStreamNullaryFn.tsx` around lines 20 - 29, The access to from.centerX and from.radius in ArrowFromStreamNullaryFn is safe because GlobalFnValue and ContValue (and FnValue) declare readonly radius and centerX, so remove any unnecessary defensive guards or casts around using from.centerX/from.radius (leave startPointX/startPointY calculation using from.centerX + 2 * from.radius and from.y()); add a short inline comment in ArrowFromStreamNullaryFn noting that FnValue|GlobalFnValue|ContValue all provide centerX/radius to clarify the union typing, and update the class doc comment on ArrowFromStreamNullaryFn (around the top of the file) to the requested text “an GenericArrow”.src/features/cseMachine/CseMachine.tsx (2)
184-193: ⚡ Quick winImprove type safety for
findKeyByValueInMap.The parameter type
anyis overly broad. Based on the method's purpose (finding a nullary function ID by pair ID), the parameter should be typed asstring.🔧 Proposed type safety improvement
- public static findKeyByValueInMap(value: any) { + public static findKeyByValueInMap(value: string): string | undefined { for (const [key, array] of CseMachine.streamLineage.entries()) { if (array.includes(value)) { return key; } } return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/cseMachine/CseMachine.tsx` around lines 184 - 193, The parameter of CseMachine.findKeyByValueInMap is typed as any but should be a string since it finds a nullary function ID by pair ID; change the parameter type from any to string (e.g., findKeyByValueInMap(value: string)) and update any call sites that pass non-string values to ensure they pass strings or are converted, keeping the return type as string | undefined and using the existing CseMachine.streamLineage Map iteration logic.
430-441: 💤 Low valueConsider refactoring duplicate rendering logic.
The pair creation mode branch (lines 431-437) duplicates the same sequence as the fallback
elseblock (lines 482-488). Consider extracting this shared logic into a helper method to reduce duplication.♻️ Example refactor
private static refreshLayoutAndDraw(): void { Layout.setContext(CseMachine.environmentTree!, CseMachine.control!, CseMachine.stash!); if (!CseMachine.getMasterLayout()) { CseMachine.setMasterLayout(Layout.getLayoutPositions(this.controlStash)); } Layout.applyFixedPositions(); CseAnimation.updateAnimation(); this.setVis(Layout.draw()); }Then use it in both branches:
if (CseMachine.getPairCreationMode()) { - Layout.setContext(CseMachine.environmentTree, CseMachine.control, CseMachine.stash); - if (!CseMachine.getMasterLayout()) { - CseMachine.setMasterLayout(Layout.getLayoutPositions(this.controlStash)); - } - Layout.applyFixedPositions(); - CseAnimation.updateAnimation(); - this.setVis(Layout.draw()); + CseMachine.refreshLayoutAndDraw(); } else if (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/cseMachine/CseMachine.tsx` around lines 430 - 441, Extract the duplicated sequence (Layout.setContext(...), conditional CseMachine.getMasterLayout()/CseMachine.setMasterLayout(...), Layout.applyFixedPositions(), CseAnimation.updateAnimation(), this.setVis(Layout.draw())) into a single helper (e.g., private static refreshLayoutAndDraw or an instance method refreshLayoutAndDraw) and replace the duplicated blocks in the CseMachine.getPairCreationMode() branch and the fallback else branch with a call to that helper; ensure the helper accepts or references the same symbols used now (CseMachine.environmentTree, CseMachine.control, CseMachine.stash, this.controlStash) so the behavior remains identical.src/commons/sagas/WorkspaceSaga/helpers/updateInspector.ts (1)
50-52: ⚡ Quick winConsider displaying error feedback to users.
The console logging helps developers debug rendering failures, but users won't see this message. Given the PR context mentions programs appearing "stuck," consider dispatching an action to display an error notification in the UI so users understand why the visualization didn't update.
💡 Example approach
} catch (e) { // TODO: Hardcoded to make use of the first editor tab. Rewrite after editor tabs are added. yield put(actions.setEditorHighlightedLines(workspaceLocation, 0, [])); // Log rendering errors so they are not silently swallowed — the visualization // simply won't update when this fires, which looks like the diagram is "stuck". console.error('[updateInspector] Visualization rendering failed:', e); + // Optionally notify the user + yield put(actions.addEditorTab(workspaceLocation, { + type: 'error', + message: 'CSE Machine visualization failed to render. Check console for details.' + })); }(Adjust the action name and payload to match your existing notification system.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commons/sagas/WorkspaceSaga/helpers/updateInspector.ts` around lines 50 - 52, In updateInspector.ts, inside the catch block that currently only calls console.error('[updateInspector] Visualization rendering failed:', e), dispatch a UI notification action (e.g., showErrorNotification or notifyError) with a user-friendly message and include enough error context (error message/id) for debugging; update the error handling in the updateInspector function to call the existing notification action creator or saga/dispatcher used across the app so users see a visible error when the visualization fails instead of only a console error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/cseMachine/CseMachine.tsx`:
- Line 68: Remove the leftover commented-out assignment for
CseMachine.pairCreationMode (the line "//CseMachine.pairCreationMode = false;")
from the CseMachine component to clean up dead/commented code; simply delete
that commented line so the source contains no unused commented fragments
referencing CseMachine.pairCreationMode.
- Around line 438-440: Remove the leftover commented debug lines that call
Layout.draw(), console.log(Layout.currentDarkPairs), and
this.setVis(Layout.currentDarkPairs) in CseMachine (the commented trio around
setVis/Layout.currentDarkPairs). Edit the CseMachine.tsx file to delete those
three commented lines so only active production code remains (leave any real
calls like this.setVis elsewhere intact).
- Line 186: Remove the leftover commented debug line "// console.log(key +
array);" from the CseMachine component in CseMachine.tsx; locate the commented
statement inside the CseMachine class/function and delete that single line so no
commented console.log remains in the source.
---
Nitpick comments:
In `@src/commons/sagas/WorkspaceSaga/helpers/updateInspector.ts`:
- Around line 50-52: In updateInspector.ts, inside the catch block that
currently only calls console.error('[updateInspector] Visualization rendering
failed:', e), dispatch a UI notification action (e.g., showErrorNotification or
notifyError) with a user-friendly message and include enough error context
(error message/id) for debugging; update the error handling in the
updateInspector function to call the existing notification action creator or
saga/dispatcher used across the app so users see a visible error when the
visualization fails instead of only a console error.
In `@src/features/cseMachine/components/arrows/ArrowFromStreamNullaryFn.tsx`:
- Around line 20-29: The access to from.centerX and from.radius in
ArrowFromStreamNullaryFn is safe because GlobalFnValue and ContValue (and
FnValue) declare readonly radius and centerX, so remove any unnecessary
defensive guards or casts around using from.centerX/from.radius (leave
startPointX/startPointY calculation using from.centerX + 2 * from.radius and
from.y()); add a short inline comment in ArrowFromStreamNullaryFn noting that
FnValue|GlobalFnValue|ContValue all provide centerX/radius to clarify the union
typing, and update the class doc comment on ArrowFromStreamNullaryFn (around the
top of the file) to the requested text “an GenericArrow”.
In `@src/features/cseMachine/components/values/FnValue.tsx`:
- Around line 144-152: The addArrow method uses target: any and redundant
optional chaining; tighten its signature to target: ArrayValue (import
ArrayValue), remove the ?. and call this._streamArrows.push(...) directly, and
keep creating the ArrowFromStreamNullaryFn(this, currentCount).to(target) cast
as before; also ensure calculateSteps still expects ArrayValue and update any
consumers accordingly (this resolves the unused index warning by enabling arrow
=> arrow.draw() elsewhere when iterating _streamArrows).
In `@src/features/cseMachine/CseMachine.tsx`:
- Around line 184-193: The parameter of CseMachine.findKeyByValueInMap is typed
as any but should be a string since it finds a nullary function ID by pair ID;
change the parameter type from any to string (e.g., findKeyByValueInMap(value:
string)) and update any call sites that pass non-string values to ensure they
pass strings or are converted, keeping the return type as string | undefined and
using the existing CseMachine.streamLineage Map iteration logic.
- Around line 430-441: Extract the duplicated sequence (Layout.setContext(...),
conditional CseMachine.getMasterLayout()/CseMachine.setMasterLayout(...),
Layout.applyFixedPositions(), CseAnimation.updateAnimation(),
this.setVis(Layout.draw())) into a single helper (e.g., private static
refreshLayoutAndDraw or an instance method refreshLayoutAndDraw) and replace the
duplicated blocks in the CseMachine.getPairCreationMode() branch and the
fallback else branch with a call to that helper; ensure the helper accepts or
references the same symbols used now (CseMachine.environmentTree,
CseMachine.control, CseMachine.stash, this.controlStash) so the behavior remains
identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63d238e1-21ef-4517-a1b6-a1706382f122
📒 Files selected for processing (8)
src/commons/sagas/WorkspaceSaga/helpers/updateInspector.tssrc/commons/sideContent/content/SideContentCseMachine.tsxsrc/features/cseMachine/CseMachine.tsxsrc/features/cseMachine/CseMachineLayout.tsxsrc/features/cseMachine/components/arrows/ArrowFromStreamNullaryFn.tsxsrc/features/cseMachine/components/arrows/DottedArrow.tsxsrc/features/cseMachine/components/values/ArrayValue.tsxsrc/features/cseMachine/components/values/FnValue.tsx
Just checked with the stream examples we have and it works! Thanks for all the help @Akshay-2007-1 ! Will work on the js-slang map issue when I am back in Singapore |
7 participants
Description
Created new panel in CSE machine for Stream Visualization (pairs and nullary functions belonging to streams).
Displays results from streamViz js-slang PR (source-academy/js-slang#1939). Specifically, we use results from streamLineage from the Context object to draw dotted arrows from a nullary function to the pair(s) it returns.
The map streamLineage (Found in context object, types.ts) that maps the ID of nullary functions to the pairs it returns.
Everytime a CALL0 is called in the control, we push a tuple (nullaryFnID, controlStackHeight) to the pendingStreamFnStack[]. Within generateCSEMachineStateStream, we check the whether the height of the control stack has reached the height of i-1, where i is the controlStackHeight when we pendingStreamFnStack.peek(). If so, we pop pendingStreamFnStack.pop() and we add the pair that is at the top of the stash to the array of pairs that the nullaryFnID maps to, using streamLineage.
See also frontend issue: #3727
Type of change
How to test
Checklist