Ast display#1406
Open
ProfessorAtomicManiac wants to merge 34 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces element-definition prerequisite graphs with an AST-driven course prerequisite system: removes Changes
Sequence DiagramsequenceDiagram
participant User
participant CoursePrereqGraph
participant ExpandState
participant CytoscapeCore as Cytoscape
participant CourseAPI
participant CourseDrawer
User->>CoursePrereqGraph: mount(ast, targetCourseId, styleEntries)
CoursePrereqGraph->>ExpandState: createExpandState(ast, targetCourseId)
ExpandState->>CytoscapeCore: astToElements(...) → initial elements
CytoscapeCore->>CytoscapeCore: generateTreeLayout() → positions
User->>CytoscapeCore: click course node
CytoscapeCore->>CoursePrereqGraph: onCourseClick(courseId)
CoursePrereqGraph->>CourseAPI: fetchCourse(courseId)
CourseAPI-->>CoursePrereqGraph: Course (with prerequisites.ast)
CoursePrereqGraph->>CourseDrawer: setSelected + open
User->>CytoscapeCore: click expand node
CytoscapeCore->>ExpandState: expandCourse(courseId)
ExpandState->>CourseAPI: fetchCourse(courseId)
CourseAPI-->>ExpandState: Course.prerequisites.ast
ExpandState->>CytoscapeCore: astToElements(newAst) → add/dedupe elements
CytoscapeCore->>CytoscapeCore: generateTreeLayout() → recompute
User->>CytoscapeCore: click collapse node
CytoscapeCore->>ExpandState: collapseCourse(courseId)
ExpandState->>CytoscapeCore: remove/release elements & restore siblings
CytoscapeCore->>CytoscapeCore: generateTreeLayout() → recompute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (1)
238-254:⚠️ Potential issue | 🟡 MinorDon’t assume AST-backed prerequisite nodes have
descriptiondata.The new prerequisite graph path only passes
abstract_syntax_treeinto the renderer, and the AST types insrc/lib/types/course.tsdon’t carry descriptions. On mobile, this tooltip body will end up blank/undefinedunless you make the paragraph conditional or hydrate descriptions separately.Suggested fix
const heading = document.createElement("h1"); heading.className = "text-lg font-semibold"; heading.textContent = targetNode.id(); - const para = document.createElement("p"); - para.className = "text-sm"; - para.textContent = targetNode.data("description"); - container.appendChild(heading); - container.appendChild(para); + const description = targetNode.data("description"); + if (description) { + const para = document.createElement("p"); + para.className = "text-sm"; + para.textContent = description; + container.appendChild(para); + } div.appendChild(container);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts` around lines 238 - 254, The tooltip creation assumes targetNode.data("description") exists; update the popper content logic in the targetNode.popper callback used to build the tip so it conditionally creates and appends the <p> element only when targetNode.data("description") is non-null/defined (or use a fallback string like empty string/“No description available”), or alternatively ensure descriptions are hydrated before rendering the prerequisite graph; specifically modify the code that reads targetNode.data("description") inside the popper content builder to guard that access and avoid inserting an undefined paragraph.
🧹 Nitpick comments (1)
src/lib/components/cytoscape/graph-layout.ts (1)
25-31: Reuse a single canvas for text measurement.
generateTreeLayout()is on the expand/collapse path, so allocating a fresh<canvas>for every node on every relayout adds avoidable DOM churn. Hoisting one shared canvas/context will make this path cheaper.Suggested refactor
+const measureCanvas = + typeof document !== "undefined" ? document.createElement("canvas") : null; +const measureContext = measureCanvas?.getContext("2d"); + function measureTextWidth(text: string, fontSize: number): number { - const canvas = document.createElement("canvas"); - const ctx = canvas.getContext("2d"); + const ctx = measureContext; if (!ctx) return text.length * fontSize * 0.6; // Fallback estimate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/graph-layout.ts` around lines 25 - 31, measureTextWidth currently creates a new <canvas> per call which causes DOM churn during generateTreeLayout's expand/collapse; instead hoist a single shared canvas and its 2D context (e.g., module-level or closure-scoped variables) and reuse them inside measureTextWidth (falling back to the current estimate if ctx is unavailable). Update measureTextWidth to reference the shared canvas/context and set ctx.font before measuring; ensure the variables are lazily initialized so callers like generateTreeLayout and any node-measure code use the same canvas instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/cytoscape/cytoscape-init.ts`:
- Around line 243-260: The course deduplication is storing
parentOperatorId/parentOperatorType on the deduped node, which loses
per-occurrence parent info; change the model so those fields live on the edge
(or create an explicit edge entry per parent occurrence) instead of on the node:
in astToElements() (the block that builds the nodes array and the corresponding
edges), stop attaching parentOperatorId/parentOperatorType to the node data and
instead attach them to the edge data for each parent→course relationship (ensure
edges reference the CourseUtils.courseReferenceToString(node) id and include
parentOperatorId/parentOperatorType), and update any consumers (e.g.,
expand-state.svelte.ts) to read parentOperatorId/parentOperatorType from the
edge rather than node; apply the same change to the other similar blocks
mentioned (around the other course-node creation sites).
In `@src/lib/components/cytoscape/expand-state.svelte.ts`:
- Around line 296-314: The collapse logic in findAllNestedExpanded currently
treats ancestry by checking ownedElementIds.includes(nestedCourseId) on the
expandedCourses map, which incorrectly collapses shared prerequisite nodes
referenced by multiple parents; change the implementation to track explicit
parent links for expansions instead of inferring from ownedElementIds. Add and
use a dedicated parent reference map (e.g., expansionParents or a parentId
property on the expandedCourses entries) and update the recursion in
findAllNestedExpanded to follow those parent pointers (use
expandedCourses.get(rootCourseId).childIds or expansionParents lookup) so only
descendants whose parent reference points to the collapsing branch are returned;
also update the code paths that create/remove expansions to maintain these
parent refs accordingly (affecting the code around findAllNestedExpanded and the
related collapse/unexpand handlers referenced near lines 349-359).
- Around line 193-199: Prevent re-entry by tracking in-flight operations: add a
Set (e.g., expandingCourses/collapsingCourses or a single pendingCourses Map)
and check it at the start of expandCourse(courseId) and
collapseCourse(courseId); if courseId is present return immediately, otherwise
add it before any await (before calling fetchCourse) and remove it in a finally
block so it is cleared on success or error. Ensure expandedCourses is only
mutated after the in-flight flag is cleared or as part of the same atomic flow
to avoid double-incremented ref counts and ownership overwrites; apply the same
guard logic to collapseCourse and any related ref-count/ownership updates.
- Around line 153-167: runLayout currently starts the layout with
cyInstance.layout(...).run() and returns immediately, so the layoutstop handler
that calls addExpandNodes() runs later and other code (e.g., retargeting that
references nodes named like expand-${courseId}) can run before those nodes
exist; fix by turning runLayout into an async function that awaits the
layoutstop event: wrap cyInstance.one("layoutstop", ...) in a Promise and
resolve the promise after calling addExpandNodes(), so runLayout only returns
once addExpandNodes() has finished; keep existing calls to removeExpandNodes(),
getValidElementsForLayout(), generateTreeLayout(), and
cyInstance.layout(...).run() but replace the immediate return with awaiting the
new Promise that resolves inside the layoutstop handler that calls
addExpandNodes().
In `@src/lib/components/cytoscape/graph-data.ts`:
- Around line 15-24: processGraphData currently assumes every element has
item.data which causes runtime crashes when a fetched element lacks a data
object; update processGraphData to guard and ensure item.data exists before
accessing it (e.g., create item.data = item.data || {} at the start of the
per-item loop or check item.data truthiness before using Object.hasOwn and "id"
in item.data), then proceed to set pannable, normalize title with
Object.hasOwn(item.data, "title"), and add the "course" class only when
item.data is present and item.data.type !== "compound"; reference the
processGraphData function and the per-item usages of item.data, Object.hasOwn,
and "id" in item.data when making the fix.
In `@src/lib/components/cytoscape/graph-layout.ts`:
- Around line 467-471: The branch that returns a preset layout when rootId is
falsy discards the ELK-computed node positions; instead of returning positions:
{}, keep and return the previously computed ELK positions (the variable holding
the computed positions) so edgeless prerequisite graphs still place the single
target node. Update the if (!rootId) return to use that computed positions
variable (and preserve animate), referencing findRootNode, rootId and the
computed positions variable used earlier in this module.
---
Outside diff comments:
In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts`:
- Around line 238-254: The tooltip creation assumes
targetNode.data("description") exists; update the popper content logic in the
targetNode.popper callback used to build the tip so it conditionally creates and
appends the <p> element only when targetNode.data("description") is
non-null/defined (or use a fallback string like empty string/“No description
available”), or alternatively ensure descriptions are hydrated before rendering
the prerequisite graph; specifically modify the code that reads
targetNode.data("description") inside the popper content builder to guard that
access and avoid inserting an undefined paragraph.
---
Nitpick comments:
In `@src/lib/components/cytoscape/graph-layout.ts`:
- Around line 25-31: measureTextWidth currently creates a new <canvas> per call
which causes DOM churn during generateTreeLayout's expand/collapse; instead
hoist a single shared canvas and its 2D context (e.g., module-level or
closure-scoped variables) and reuse them inside measureTextWidth (falling back
to the current estimate if ctx is unavailable). Update measureTextWidth to
reference the shared canvas/context and set ctx.font before measuring; ensure
the variables are lazily initialized so callers like generateTreeLayout and any
node-measure code use the same canvas instance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b80d015a-80eb-49a8-bb59-37cc33d41c96
📒 Files selected for processing (17)
src/lib/components/course/course-tabs.sveltesrc/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/course-prereq-graph.sveltesrc/lib/components/cytoscape/cytoscape-core.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/cytoscape-init.tssrc/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/cytoscape/expand-state.svelte.tssrc/lib/components/cytoscape/graph-data.tssrc/lib/components/cytoscape/graph-layout.tssrc/lib/components/cytoscape/graph-styles.tssrc/lib/components/cytoscape/paths.tssrc/lib/types/course.tssrc/routes/courses/[courseIdentifier]/+page.sveltesrc/routes/courses/[courseIdentifier]/+page.tssrc/routes/explorer/[subject]/+page.tssrc/routes/explorer/all/+page.ts
💤 Files with no reviewable changes (3)
- src/routes/courses/[courseIdentifier]/+page.svelte
- src/routes/courses/[courseIdentifier]/+page.ts
- src/lib/components/course/course-tabs.svelte
Comment on lines
+243
to
+260
| // Course reference node | ||
| if (isCourseReference(node)) { | ||
| const id = CourseUtils.courseReferenceToString(node); | ||
| // Use only first subject, truncated to 4 chars (e.g., "COMP 240" instead of "COMPSCI/MATH 240") | ||
| const firstSubject = [...node.subjects].sort()[0].slice(0, 4); | ||
| const label = `${firstSubject} ${node.course_number}`; | ||
|
|
||
| // Only add node if not already present | ||
| if (!nodes.some((n) => n.data.id === id)) { | ||
| nodes.push({ | ||
| data: { | ||
| id, | ||
| label, | ||
| type: "prereq", | ||
| parentOperatorId: parentContext.operatorId, | ||
| parentOperatorType: parentContext.operatorType, | ||
| }, | ||
| classes: "hoverable course", |
Contributor
There was a problem hiding this comment.
Shared prerequisite nodes can't keep only one parent-operator reference.
astToElements() deduplicates course nodes by id, but parentOperatorId / parentOperatorType live on the node itself. If the same course appears in multiple branches, whichever parent wins here is the one src/lib/components/cytoscape/expand-state.svelte.ts uses later for OR-sibling removal, so expanding that shared node can hide siblings from the wrong branch. This metadata needs to be per edge or per parent occurrence instead of per deduped node.
Also applies to: 287-293, 335-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/cytoscape-init.ts` around lines 243 - 260, The
course deduplication is storing parentOperatorId/parentOperatorType on the
deduped node, which loses per-occurrence parent info; change the model so those
fields live on the edge (or create an explicit edge entry per parent occurrence)
instead of on the node: in astToElements() (the block that builds the nodes
array and the corresponding edges), stop attaching
parentOperatorId/parentOperatorType to the node data and instead attach them to
the edge data for each parent→course relationship (ensure edges reference the
CourseUtils.courseReferenceToString(node) id and include
parentOperatorId/parentOperatorType), and update any consumers (e.g.,
expand-state.svelte.ts) to read parentOperatorId/parentOperatorType from the
edge rather than node; apply the same change to the other similar blocks
mentioned (around the other course-node creation sites).
Comment on lines
+153
to
+167
| async function runLayout(): Promise<void> { | ||
| if (!cyInstance) return; | ||
|
|
||
| // Remove expand nodes before layout (they'll be re-added after) | ||
| removeExpandNodes(); | ||
|
|
||
| const validElements = getValidElementsForLayout(); | ||
| const layoutOptions = await generateTreeLayout(true, validElements, true); | ||
|
|
||
| cyInstance.layout(layoutOptions).run(); | ||
|
|
||
| cyInstance.one("layoutstop", () => { | ||
| addExpandNodes(); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
runLayout() doesn't wait for the post-layout callback.
The promise resolves right after layout().run(), while the callback that calls addExpandNodes() runs later. The retargeting at Lines 272-281 can therefore point edges at expand-${courseId} before that node has been recreated.
🛠️ Proposed fix
async function runLayout(): Promise<void> {
if (!cyInstance) return;
// Remove expand nodes before layout (they'll be re-added after)
removeExpandNodes();
const validElements = getValidElementsForLayout();
const layoutOptions = await generateTreeLayout(true, validElements, true);
- cyInstance.layout(layoutOptions).run();
-
- cyInstance.one("layoutstop", () => {
- addExpandNodes();
- });
+ await new Promise<void>((resolve) => {
+ const layout = cyInstance!.layout(layoutOptions);
+ cyInstance!.one("layoutstop", () => {
+ addExpandNodes();
+ resolve();
+ });
+ layout.run();
+ });
}Also applies to: 270-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 153 - 167,
runLayout currently starts the layout with cyInstance.layout(...).run() and
returns immediately, so the layoutstop handler that calls addExpandNodes() runs
later and other code (e.g., retargeting that references nodes named like
expand-${courseId}) can run before those nodes exist; fix by turning runLayout
into an async function that awaits the layoutstop event: wrap
cyInstance.one("layoutstop", ...) in a Promise and resolve the promise after
calling addExpandNodes(), so runLayout only returns once addExpandNodes() has
finished; keep existing calls to removeExpandNodes(),
getValidElementsForLayout(), generateTreeLayout(), and
cyInstance.layout(...).run() but replace the immediate return with awaiting the
new Promise that resolves inside the layoutstop handler that calls
addExpandNodes().
Comment on lines
+193
to
+199
| async expandCourse(courseId: string): Promise<void> { | ||
| if (!cyInstance) return; | ||
| if (expandedCourses.has(courseId)) return; | ||
|
|
||
| try { | ||
| const course = await fetchCourse(courseId); | ||
| const ast = course.prerequisites?.abstract_syntax_tree; |
Contributor
There was a problem hiding this comment.
Guard expand/collapse against re-entry.
A quick double-tap can enter expandCourse() twice before Line 261 marks the course expanded. That double-increments ref counts and overwrites ownership tracking, so the next collapse can leak elements. collapseCourse() has the same race against an in-flight expand.
Also applies to: 290-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 193 - 199,
Prevent re-entry by tracking in-flight operations: add a Set (e.g.,
expandingCourses/collapsingCourses or a single pendingCourses Map) and check it
at the start of expandCourse(courseId) and collapseCourse(courseId); if courseId
is present return immediately, otherwise add it before any await (before calling
fetchCourse) and remove it in a finally block so it is cleared on success or
error. Ensure expandedCourses is only mutated after the in-flight flag is
cleared or as part of the same atomic flow to avoid double-incremented ref
counts and ownership overwrites; apply the same guard logic to collapseCourse
and any related ref-count/ownership updates.
Comment on lines
+296
to
+314
| // Recursively find all nested expanded courses | ||
| function findAllNestedExpanded(rootCourseId: string): string[] { | ||
| const info = expandedCourses.get(rootCourseId); | ||
| if (!info) return []; | ||
|
|
||
| const nested: string[] = []; | ||
| const { ownedElementIds } = info; | ||
|
|
||
| // Find direct children that are expanded | ||
| expandedCourses.forEach((_, nestedCourseId) => { | ||
| if (nestedCourseId !== rootCourseId && ownedElementIds.includes(nestedCourseId)) { | ||
| nested.push(nestedCourseId); | ||
| // Recursively find their nested expansions | ||
| nested.push(...findAllNestedExpanded(nestedCourseId)); | ||
| } | ||
| }); | ||
|
|
||
| return nested; | ||
| } |
Contributor
There was a problem hiding this comment.
Shared expanded descendants get collapsed by the wrong branch.
findAllNestedExpanded() infers ancestry from ownedElementIds.includes(nestedCourseId). A shared prerequisite node satisfies that test for every branch that references it, so collapsing one parent can tear down another branch's still-visible expansion. Track expansion parent refs separately instead of deriving them from shared element ownership.
Also applies to: 349-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 296 - 314,
The collapse logic in findAllNestedExpanded currently treats ancestry by
checking ownedElementIds.includes(nestedCourseId) on the expandedCourses map,
which incorrectly collapses shared prerequisite nodes referenced by multiple
parents; change the implementation to track explicit parent links for expansions
instead of inferring from ownedElementIds. Add and use a dedicated parent
reference map (e.g., expansionParents or a parentId property on the
expandedCourses entries) and update the recursion in findAllNestedExpanded to
follow those parent pointers (use expandedCourses.get(rootCourseId).childIds or
expansionParents lookup) so only descendants whose parent reference points to
the collapsing branch are returned; also update the code paths that
create/remove expansions to maintain these parent refs accordingly (affecting
the code around findAllNestedExpanded and the related collapse/unexpand handlers
referenced near lines 349-359).
Comment on lines
+15
to
+24
| export function processGraphData(data: ElementDefinition[]): ElementDefinition[] { | ||
| data.forEach((item: any) => { | ||
| item["pannable"] = true; | ||
| if (!Object.hasOwn(item.data, "title")) { | ||
| // to avoid the warnings in console | ||
| item.data["title"] = ""; | ||
| } | ||
| // Add "course" class to course nodes (not edges or compound nodes) | ||
| if ("id" in item.data && item.data.type !== "compound") { | ||
| item.classes = item.classes ? `${item.classes} course` : "course"; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard item.data before normalizing remote graph elements.
processGraphData() now runs directly on fetched JSON. If any element arrives without a data object, both Object.hasOwn(item.data, "title") and "id" in item.data throw and take down the entire explorer load.
Suggested fix
export function processGraphData(data: ElementDefinition[]): ElementDefinition[] {
- data.forEach((item: any) => {
- item["pannable"] = true;
- if (!Object.hasOwn(item.data, "title")) {
- item.data["title"] = "";
+ data.forEach((item) => {
+ item.pannable = true;
+ item.data ??= {};
+ if (!Object.hasOwn(item.data, "title")) {
+ item.data.title = "";
}📝 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.
Suggested change
| export function processGraphData(data: ElementDefinition[]): ElementDefinition[] { | |
| data.forEach((item: any) => { | |
| item["pannable"] = true; | |
| if (!Object.hasOwn(item.data, "title")) { | |
| // to avoid the warnings in console | |
| item.data["title"] = ""; | |
| } | |
| // Add "course" class to course nodes (not edges or compound nodes) | |
| if ("id" in item.data && item.data.type !== "compound") { | |
| item.classes = item.classes ? `${item.classes} course` : "course"; | |
| } | |
| export function processGraphData(data: ElementDefinition[]): ElementDefinition[] { | |
| data.forEach((item) => { | |
| item.pannable = true; | |
| item.data ??= {}; | |
| if (!Object.hasOwn(item.data, "title")) { | |
| item.data.title = ""; | |
| } | |
| // Add "course" class to course nodes (not edges or compound nodes) | |
| if ("id" in item.data && item.data.type !== "compound") { | |
| item.classes = item.classes ? `${item.classes} course` : "course"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/graph-data.ts` around lines 15 - 24,
processGraphData currently assumes every element has item.data which causes
runtime crashes when a fetched element lacks a data object; update
processGraphData to guard and ensure item.data exists before accessing it (e.g.,
create item.data = item.data || {} at the start of the per-item loop or check
item.data truthiness before using Object.hasOwn and "id" in item.data), then
proceed to set pannable, normalize title with Object.hasOwn(item.data, "title"),
and add the "course" class only when item.data is present and item.data.type !==
"compound"; reference the processGraphData function and the per-item usages of
item.data, Object.hasOwn, and "id" in item.data when making the fix.
Comment on lines
+467
to
+471
| const rootId = findRootNode(courseData); | ||
|
|
||
| if (!rootId) { | ||
| return { name: "preset", positions: {}, animate }; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the computed positions for edgeless prerequisite graphs.
If this graph ever contains only the target course node, findRootNode() returns undefined because there are no edges. This branch then throws away the ELK positions computed a few lines above and returns an empty preset layout.
Suggested fix
const rootId = findRootNode(courseData);
if (!rootId) {
- return { name: "preset", positions: {}, animate };
+ return {
+ name: "preset",
+ positions: Object.fromEntries(
+ convertToCenterPositions(positions, nodeDimensions),
+ ),
+ animate,
+ animationDuration: 1000,
+ animationEasing: "ease-in-out",
+ fit: true,
+ padding: 30,
+ };
}📝 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.
Suggested change
| const rootId = findRootNode(courseData); | |
| if (!rootId) { | |
| return { name: "preset", positions: {}, animate }; | |
| } | |
| const rootId = findRootNode(courseData); | |
| if (!rootId) { | |
| return { | |
| name: "preset", | |
| positions: Object.fromEntries( | |
| convertToCenterPositions(positions, nodeDimensions), | |
| ), | |
| animate, | |
| animationDuration: 1000, | |
| animationEasing: "ease-in-out", | |
| fit: true, | |
| padding: 30, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/graph-layout.ts` around lines 467 - 471, The
branch that returns a preset layout when rootId is falsy discards the
ELK-computed node positions; instead of returning positions: {}, keep and return
the previously computed ELK positions (the variable holding the computed
positions) so edgeless prerequisite graphs still place the single target node.
Update the if (!rootId) return to use that computed positions variable (and
preserve animate), referencing findRootNode, rootId and the computed positions
variable used earlier in this module.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (1)
181-200:⚠️ Potential issue | 🟠 MajorDepartment graph nodes will silently fail hover and double-tap interactions.
The
mouseoverHandler(line 185) anddbltapHandler(line 214) require thehoverableclass. However,processGraphData()ingraph-data.ts(used for department graphs) only adds thecourseclass—nothoverable. This creates a breaking change: course graphs (from AST) havehoverableand work correctly, but department graphs (from JSON) do not, causing hover and double-tap handlers to return early without executing.Only
onetapHandler(line 227) will work for department nodes since it checks for thecourseclass, whichprocessGraphData()does add.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts` around lines 181 - 200, Handlers mouseoverHandler and dbltapHandler are skipping department graph nodes because they require the "hoverable" class while processGraphData() only assigns "course"; update processGraphData() to also add the "hoverable" class for department/course nodes (the same nodes that currently get "course") so hover and dbltap behavior runs, or alternatively modify mouseoverHandler and dbltapHandler to accept nodes with class "course" in addition to "hoverable" (refer to mouseoverHandler, dbltapHandler, processGraphData(), and onetapHandler when changing class checks).
♻️ Duplicate comments (5)
src/lib/components/cytoscape/expand-state.svelte.ts (3)
193-205:⚠️ Potential issue | 🟠 MajorNo guard against concurrent expand/collapse operations.
A quick double-tap can enter
expandCourse()twice before line 261 marks the course as expanded. The check at line 195 (expandedCourses.has(courseId)) will pass the second time because the first call is awaitingfetchCourse. This can cause:
- Same elements added twice
- Reference counts double-incremented
removedNodeIdscorrupted during sibling removalSuggested fix - add in-flight tracking
+ /** Set of courses currently being expanded/collapsed */ + const pendingOperations = new Set<string>(); + async expandCourse(courseId: string): Promise<void> { if (!cyInstance) return; if (expandedCourses.has(courseId)) return; + if (pendingOperations.has(courseId)) return; + + pendingOperations.add(courseId); try { const course = await fetchCourse(courseId); // ... rest of implementation } catch (error) { console.error(`Failed to expand ${courseId}:`, error); + } finally { + pendingOperations.delete(courseId); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 193 - 205, The expandCourse function can be re-entered before fetchCourse resolves, so add an in-flight tracking Set (e.g., inFlightExpansions) and check it at the start of expandCourse alongside expandedCourses, then add the courseId to inFlightExpansions immediately before awaiting fetchCourse and remove it in a finally block; only mark expandedCourses.add(courseId) after the expansion completes successfully, and ensure in-flight is cleared on both success and error to prevent duplicate adds, double increments, or corrupted removedNodeIds (reference expandCourse, expandedCourses).
153-167:⚠️ Potential issue | 🟠 Major
runLayout()resolves before expand nodes are added.The function starts the layout with
cyInstance.layout(layoutOptions).run()and returns immediately, whileaddExpandNodes()runs asynchronously in thelayoutstopcallback. Code that runs afterawait runLayout()(like edge retargeting at lines 272-281) can referenceexpand-${courseId}nodes before they exist.Suggested fix
async function runLayout(): Promise<void> { if (!cyInstance) return; removeExpandNodes(); const validElements = getValidElementsForLayout(); const layoutOptions = await generateTreeLayout(true, validElements, true); - cyInstance.layout(layoutOptions).run(); - - cyInstance.one("layoutstop", () => { - addExpandNodes(); - }); + await new Promise<void>((resolve) => { + const layout = cyInstance!.layout(layoutOptions); + cyInstance!.one("layoutstop", () => { + addExpandNodes(); + resolve(); + }); + layout.run(); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 153 - 167, runLayout currently starts the layout and returns immediately while addExpandNodes runs later in the "layoutstop" handler, causing callers to see expand-* nodes before they exist; change runLayout to return a Promise that resolves only after the layout stops and addExpandNodes completes: inside runLayout (which already calls removeExpandNodes, getValidElementsForLayout, generateTreeLayout and cyInstance.layout(layoutOptions).run()), wrap the layout/run call in a new Promise and in the cyInstance.one("layoutstop", async () => { await addExpandNodes(); resolve(); }) handler resolve the Promise (and reject on errors or timeouts as needed) so await runLayout() only continues once addExpandNodes has finished.
296-314:⚠️ Potential issue | 🟠 MajorShared descendants collapsed by wrong branch.
findAllNestedExpanded()determines ancestry viaownedElementIds.includes(nestedCourseId). A shared prerequisite satisfies this check for every branch that references it, so collapsing one parent can tear down expansions that belong to a sibling branch still visible in the graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/expand-state.svelte.ts` around lines 296 - 314, findAllNestedExpanded incorrectly treats a course as a descendant if rootCourseId's ownedElementIds includes it, which collapses shared prerequisites used by other visible branches; instead determine true ancestry by following parent links up the chain. Update findAllNestedExpanded (and the expandedCourses data usage) to check each candidate nestedCourseId by walking its parent pointer(s) until you either reach rootCourseId (include it) or reach a different top-level node (exclude it); if no parent pointer currently exists on the expandedCourses entries, build a reverse parent map from expandedCourses' ownedElementIds -> parentId and use that to traverse parents for each nestedCourseId rather than using ownedElementIds.includes.src/lib/components/cytoscape/graph-layout.ts (1)
467-471:⚠️ Potential issue | 🟡 MinorSingle-node graphs discard computed positions.
When
findRootNode()returnsundefined(e.g., a course with no prerequisites shows only the target node with no edges), the ELK-computed positions are discarded and an empty preset layout is returned. This loses the computed position for the single node.Suggested fix
const rootId = findRootNode(courseData); if (!rootId) { - return { name: "preset", positions: {}, animate }; + return { + name: "preset", + positions: Object.fromEntries( + convertToCenterPositions(positions, nodeDimensions), + ), + animate, + animationDuration: 1000, + animationEasing: "ease-in-out", + fit: true, + padding: 30, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/graph-layout.ts` around lines 467 - 471, The code returns an empty preset layout when findRootNode(courseData) is undefined, discarding any ELK-computed positions for single-node graphs; instead, remove the early return and ensure the function returns the "preset" layout that includes the computed positions (the positions object produced by the ELK layout step) and animate flag even when rootId is falsy—update the conditional around findRootNode to fall back to using the existing computed positions (from the ELK/layout result variable) rather than returning positions: {}.src/lib/components/cytoscape/cytoscape-init.ts (1)
243-265:⚠️ Potential issue | 🟠 MajorShared prerequisite nodes retain first parent's operator reference.
When a course appears in multiple branches, the deduplication at line 251 keeps only the first occurrence. However,
parentOperatorIdandparentOperatorTypeare stored on the node itself (lines 257-258). If a later branch references the same course under a different OR parent, that relationship is lost.This means expanding a shared node may trigger sibling removal for the wrong branch in
expand-state.svelte.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/cytoscape-init.ts` around lines 243 - 265, The deduplication logic in isCourseReference handling (CourseUtils.courseReferenceToString) only keeps the first node's parentOperatorId/parentOperatorType, losing other branch relationships; change the node data shape to track parentOperatorIds and parentOperatorTypes (or a parentContexts array) instead of single values, initialize these arrays when pushing a new node, and when an existing node is found append the current parentContext if not already present so expand-state.svelte.ts can determine the correct parent(s) for sibling removal/expansion.
🧹 Nitpick comments (1)
src/lib/components/cytoscape/graph-layout.ts (1)
25-32: Consider reusing the canvas context for text measurements.A new canvas element is created for each
measureTextWidthcall. For graphs with many nodes, this could be optimized by creating the canvas once and reusing it.Optional optimization
+// Cached canvas context for text measurement +let measurementCtx: CanvasRenderingContext2D | null = null; + +function getMeasurementContext(): CanvasRenderingContext2D | null { + if (!measurementCtx) { + const canvas = document.createElement("canvas"); + measurementCtx = canvas.getContext("2d"); + } + return measurementCtx; +} + function measureTextWidth(text: string, fontSize: number): number { - const canvas = document.createElement("canvas"); - const ctx = canvas.getContext("2d"); + const ctx = getMeasurementContext(); if (!ctx) return text.length * fontSize * 0.6; // Fallback estimate ctx.font = `${fontSize}px sans-serif`; return ctx.measureText(text).width; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/components/cytoscape/graph-layout.ts` around lines 25 - 32, The measureTextWidth function creates a new canvas per call which is inefficient; change it to use a single shared canvas and 2D context (e.g., a module-level lazy-initialized canvas/ctx used by measureTextWidth) so subsequent calls reuse the same context, keep setting ctx.font before measuring, and preserve the existing fallback behavior if ctx is unavailable; update references in measureTextWidth to use the shared canvas/ctx and ensure thread-safety by initializing lazily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts`:
- Around line 223-229: The onetapHandler currently ignores clicks unless the
target has class "course" (targetNode?.hasClass("course")), so either ensure
department graph nodes get the "course" class where elements are created (update
astToElements to add "course" to the class list for department/course nodes) or
broaden the onetapHandler check to accept the department node class used in
astToElements; locate astToElements and add "course" to the class string for
department nodes (or adjust onetapHandler to check both "course" and the
department-specific class) so course-click functionality works for both graphs.
---
Outside diff comments:
In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts`:
- Around line 181-200: Handlers mouseoverHandler and dbltapHandler are skipping
department graph nodes because they require the "hoverable" class while
processGraphData() only assigns "course"; update processGraphData() to also add
the "hoverable" class for department/course nodes (the same nodes that currently
get "course") so hover and dbltap behavior runs, or alternatively modify
mouseoverHandler and dbltapHandler to accept nodes with class "course" in
addition to "hoverable" (refer to mouseoverHandler, dbltapHandler,
processGraphData(), and onetapHandler when changing class checks).
---
Duplicate comments:
In `@src/lib/components/cytoscape/cytoscape-init.ts`:
- Around line 243-265: The deduplication logic in isCourseReference handling
(CourseUtils.courseReferenceToString) only keeps the first node's
parentOperatorId/parentOperatorType, losing other branch relationships; change
the node data shape to track parentOperatorIds and parentOperatorTypes (or a
parentContexts array) instead of single values, initialize these arrays when
pushing a new node, and when an existing node is found append the current
parentContext if not already present so expand-state.svelte.ts can determine the
correct parent(s) for sibling removal/expansion.
In `@src/lib/components/cytoscape/expand-state.svelte.ts`:
- Around line 193-205: The expandCourse function can be re-entered before
fetchCourse resolves, so add an in-flight tracking Set (e.g.,
inFlightExpansions) and check it at the start of expandCourse alongside
expandedCourses, then add the courseId to inFlightExpansions immediately before
awaiting fetchCourse and remove it in a finally block; only mark
expandedCourses.add(courseId) after the expansion completes successfully, and
ensure in-flight is cleared on both success and error to prevent duplicate adds,
double increments, or corrupted removedNodeIds (reference expandCourse,
expandedCourses).
- Around line 153-167: runLayout currently starts the layout and returns
immediately while addExpandNodes runs later in the "layoutstop" handler, causing
callers to see expand-* nodes before they exist; change runLayout to return a
Promise that resolves only after the layout stops and addExpandNodes completes:
inside runLayout (which already calls removeExpandNodes,
getValidElementsForLayout, generateTreeLayout and
cyInstance.layout(layoutOptions).run()), wrap the layout/run call in a new
Promise and in the cyInstance.one("layoutstop", async () => { await
addExpandNodes(); resolve(); }) handler resolve the Promise (and reject on
errors or timeouts as needed) so await runLayout() only continues once
addExpandNodes has finished.
- Around line 296-314: findAllNestedExpanded incorrectly treats a course as a
descendant if rootCourseId's ownedElementIds includes it, which collapses shared
prerequisites used by other visible branches; instead determine true ancestry by
following parent links up the chain. Update findAllNestedExpanded (and the
expandedCourses data usage) to check each candidate nestedCourseId by walking
its parent pointer(s) until you either reach rootCourseId (include it) or reach
a different top-level node (exclude it); if no parent pointer currently exists
on the expandedCourses entries, build a reverse parent map from expandedCourses'
ownedElementIds -> parentId and use that to traverse parents for each
nestedCourseId rather than using ownedElementIds.includes.
In `@src/lib/components/cytoscape/graph-layout.ts`:
- Around line 467-471: The code returns an empty preset layout when
findRootNode(courseData) is undefined, discarding any ELK-computed positions for
single-node graphs; instead, remove the early return and ensure the function
returns the "preset" layout that includes the computed positions (the positions
object produced by the ELK layout step) and animate flag even when rootId is
falsy—update the conditional around findRootNode to fall back to using the
existing computed positions (from the ELK/layout result variable) rather than
returning positions: {}.
---
Nitpick comments:
In `@src/lib/components/cytoscape/graph-layout.ts`:
- Around line 25-32: The measureTextWidth function creates a new canvas per call
which is inefficient; change it to use a single shared canvas and 2D context
(e.g., a module-level lazy-initialized canvas/ctx used by measureTextWidth) so
subsequent calls reuse the same context, keep setting ctx.font before measuring,
and preserve the existing fallback behavior if ctx is unavailable; update
references in measureTextWidth to use the shared canvas/ctx and ensure
thread-safety by initializing lazily.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e51723e-e4ca-46d6-93ca-9808a009fc6a
📒 Files selected for processing (17)
src/lib/components/course/course-tabs.sveltesrc/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/course-prereq-graph.sveltesrc/lib/components/cytoscape/cytoscape-core.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/cytoscape-init.tssrc/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/cytoscape/expand-state.svelte.tssrc/lib/components/cytoscape/graph-data.tssrc/lib/components/cytoscape/graph-layout.tssrc/lib/components/cytoscape/graph-styles.tssrc/lib/components/cytoscape/paths.tssrc/lib/types/course.tssrc/routes/courses/[courseIdentifier]/+page.sveltesrc/routes/courses/[courseIdentifier]/+page.tssrc/routes/explorer/[subject]/+page.tssrc/routes/explorer/all/+page.ts
💤 Files with no reviewable changes (3)
- src/routes/courses/[courseIdentifier]/+page.svelte
- src/lib/components/course/course-tabs.svelte
- src/routes/courses/[courseIdentifier]/+page.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/explorer/all/+page.ts
- src/lib/components/cytoscape/paths.ts
- src/routes/explorer/[subject]/+page.ts
- src/lib/components/course/tabs/course-prerequisites.svelte
- src/lib/components/cytoscape/cytoscape-core.svelte
- src/lib/components/cytoscape/graph-data.ts
Comment on lines
223
to
229
| const onetapHandler = async (event: any) => { | ||
| const targetNode = event.target; | ||
| if (targetNode?.data("type") === "compound") { | ||
|
|
||
| // Only handle nodes with the "course" class | ||
| if (!targetNode?.hasClass("course")) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Same class-based gating applies to course clicks.
The onetap handler now requires the course class (line 227). This is consistent with the course graph where astToElements assigns "hoverable course" to course nodes, but department graph nodes need verification that they also receive this class for course-click functionality to work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/cytoscape/cytoscape-handlers.svelte.ts` around lines 223 -
229, The onetapHandler currently ignores clicks unless the target has class
"course" (targetNode?.hasClass("course")), so either ensure department graph
nodes get the "course" class where elements are created (update astToElements to
add "course" to the class list for department/course nodes) or broaden the
onetapHandler check to accept the department node class used in astToElements;
locate astToElements and add "course" to the class string for department nodes
(or adjust onetapHandler to check both "course" and the department-specific
class) so course-click functionality works for both graphs.
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
1 participant
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.
Summary by CodeRabbit
New Features
Refactoring