Conversation
added pair visualiser button and a private pairCreationMode boolean f…
first attempt at making it pair only
Aligned all stream and list pairs into a line in pair mode Removed 2nd arrow from function objects in pair mode Added an array to track all pairs that should show up in pair mode
Pair toggle fetches streamsPointSteps[] from context
Added dotted arrow
Temp fix for pair creation toggle button
Added arrows that point from stream nullary fn to pair created
Dotted arrow
added dotted line arrow for ArrowFn
added straight dotted lines from nullary function to created pair
Added a pairid to streamid map which helps to set the y position of t…
![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' mode to the CSE Machine, enabling stream-based visualization by tracking stream lineages and adding streamsPointSteps to the workspace state. The changes involve extensive refactoring of js-slang imports and updates to the CSE Machine's layout and component logic. Feedback highlights critical issues with non-portable local file paths in package.json, an improper import from node_modules, and a potential NaN bug in layout calculations. Several suggestions were also provided to fix typos, style violations, and remove debug logs.
| "java-slang": "^1.0.13", | ||
| "js-cookie": "^3.0.5", | ||
| "js-slang": "^1.0.85", | ||
| "js-slang": "file:../js-slang", |
There was a problem hiding this comment.
The dependency js-slang is pointing to a local file path (file:../js-slang). This will cause build failures in CI/CD environments and for other developers who do not have the repository cloned in the same relative location. Please use a versioned package from the registry.
| ] | ||
| }, | ||
| "resolutions": { | ||
| "js-slang": "portal:/C:/Users/triss/shared-source-academy-frontend-repo/js-slang" |
There was a problem hiding this comment.
The resolutions field contains a hardcoded absolute path to a local directory (portal:/C:/Users/...). This is not portable and will break the build on any other machine. This should be removed before merging.
| @@ -1,5 +1,6 @@ | |||
| import { KonvaEventObject } from 'konva/lib/Node'; | |||
| import { Label } from 'konva/lib/shapes/Label'; | |||
| import { _isEventFromThisInstance } from 'node_modules/ag-grid-community/dist/types/src/agStack/utils/event'; | |||
There was a problem hiding this comment.
Importing directly from node_modules is highly discouraged as it relies on the internal file structure of the package, which is not part of its public API and may change. Furthermore, this specific import _isEventFromThisInstance does not appear to be used anywhere in this file and should be removed.
| // streamHeights[0] is always initialised to 0, so this shouldnt ever cause | ||
| // an arrayindexexception | ||
| if (Layout.streamHeights[this.streamId] == undefined) { | ||
| Layout.streamHeights[this.streamId] = Layout.streamHeights[this.streamId - 1] + 1; |
There was a problem hiding this comment.
If this.streamId is greater than 0 but the previous index in Layout.streamHeights has not been initialized, Layout.streamHeights[this.streamId - 1] will be undefined. Adding 1 to undefined results in NaN, which can cause layout issues. Consider adding a nullish coalescing check.
const prevHeight = this.streamId > 0 ? (Layout.streamHeights[this.streamId - 1] ?? 0) : 0;
Layout.streamHeights[this.streamId] = prevHeight + 1;
| // console.log(this.streamId); | ||
| // console.log(this.data.id); |
There was a problem hiding this comment.
Remove commented-out debug logs to maintain code cleanliness.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| private static stash: Stash | undefined; | ||
| private static streamLineage: Map<string, string[]>; | ||
| private static streamPairIdToStreamId: Map<string, string>; | ||
| private static streamPairIdToParentCount: Map<string, number>; | ||
| public static togglePrintableMode(): void { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| this.paramsText = `params: ${getParamsText(this.data)}`; | ||
| this.bodyText = `body: ${getBodyText(this.data)}`; | ||
| this.bodyText = `id: ${(this.data as any).id}`; |
There was a problem hiding this comment.
Bug: The FnValue component's tooltip incorrectly displays a debug ID instead of the function's body text due to leftover temporary code.
Severity: MEDIUM
Suggested Fix
Restore the original code to display the function body in the tooltip. Replace this.bodyText = id: ${(this.data as any).id} with the commented-out line: `this.bodyText = `body: ${getBodyText(this.data)}. This will show the correct information to the user.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/cseMachine/components/values/FnValue.tsx#L77-L78
Potential issue: In the `FnValue` component, the function body's display text is being
set to a debug ID (e.g., `id: 42`) instead of the actual function body content. The
original code to display the function body has been commented out, and a developer
comment `// remember to delete this if its fixed` indicates this was a temporary change
for debugging. As a result, when a user hovers over a non-global function in the CSE
machine visualization, they will see an internal ID instead of the function's code,
degrading the tool's educational value.
Did we get this right? 👍 / 👎 to inform future reviews.
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: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| Layout.streamLengthMap.clear(); | ||
| Layout.streamCoords = []; | ||
| // Layout.streamHeights = [0]; |
There was a problem hiding this comment.
Bug: The draw() method fails to reset Layout.streamHeights because the reset line is commented out. This causes stale data to persist across redraws, leading to incorrect stream rendering.
Severity: MEDIUM
Suggested Fix
Uncomment the line Layout.streamHeights = [0]; within the draw() method in CseMachineLayout.tsx. This will ensure that all stream-related layout states are consistently reset before each redraw, preventing stale data from corrupting the new layout.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/cseMachine/CseMachineLayout.tsx#L546-L548
Potential issue: In the `draw()` method, `Layout.streamLengthMap` and
`Layout.streamCoords` are cleared, but the reset for `Layout.streamHeights` is commented
out. When a redraw is triggered, such as by toggling pair creation mode, `ArrayValue`
instances are recreated. However, because `streamHeights` retains stale data from the
previous render, the logic for calculating new stream heights is skipped. This results
in new streams being rendered at incorrect Y-coordinates, reusing positions from the
previous visualization session.
| originFnValue.addArrow(this); | ||
| } else { | ||
| Layout.pendingFnLink = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: A race condition with the Layout.pendingFnLink flag causes deferred links between functions and arrays to be missed if multiple links are pending, resulting in an incomplete visualization.
Severity: MEDIUM
Suggested Fix
Replace the single boolean flag Layout.pendingFnLink with a queue-based system. When an ArrayValue needs to defer linking, it should add its ID or the required function ID to a queue. The FnValue constructor should then process all relevant items from this queue instead of relying on a single flag.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/cseMachine/components/values/ArrayValue.tsx#L133-L137
Potential issue: A race condition exists due to the use of a single boolean flag,
`Layout.pendingFnLink`, to manage deferred linking between functions and arrays. If
multiple `ArrayValue` instances are created before their corresponding `FnValue`s, each
will set `Layout.pendingFnLink` to true. However, when the first `FnValue` is created,
it processes its links and then resets the flag to false. Subsequent `FnValue`s will see
the flag as false and skip their linking process, resulting in missing stream arrows in
the visualization.
|
Superseded by #3747 |
5 participants
Description
Type of change
How to test
Checklist