Cytoscape refactor#1358
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Cytoscape into core/init/handlers modules, adds CytoscapeCore with a public API and course-focus/tooltip handlers, filters prerequisite graphs using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SideControls
participant CytoscapeCore
participant Handlers as CytoscapeHandlers
participant CourseDrawer
participant Store as takenCoursesStore
User->>SideControls: trigger zoom/layout/draggable/label
SideControls->>CytoscapeCore: zoom / runLayout / setElementsAreDraggable / setShowCodeLabels
Store-->>CytoscapeCore: takenCourses update
CytoscapeCore->>CytoscapeCore: filterElementsByRootCourse(root, takenIds)
CytoscapeCore->>Handlers: register events / apply styling
User->>CytoscapeCore: click course node
CytoscapeCore->>Handlers: emit courseClick(courseId)
Handlers->>CourseDrawer: request open with courseId
CourseDrawer-->>User: render course details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/lib/mediaStore.ts (1)
6-9: Consider storing the MediaQueryList reference for potential cleanup.While module-level listeners typically persist for the app lifetime, storing the reference enables cleanup if needed in the future (e.g., for testing or HMR scenarios).
🔎 Optional improvement
if (browser) { const mediaQuery = window.matchMedia("(min-width: 768px)"); isDesktop.set(mediaQuery.matches); - mediaQuery.addEventListener("change", (e) => isDesktop.set(e.matches)); + const handleChange = (e: MediaQueryListEvent) => isDesktop.set(e.matches); + mediaQuery.addEventListener("change", handleChange); + // Export for cleanup if needed: export const cleanupMediaQuery = () => mediaQuery.removeEventListener("change", handleChange); }src/lib/components/cytoscape/graph-layout.ts (1)
77-81: Minor: shorthand property available.🔎 Use shorthand property
- animate: animate, // Whether to animate the layout + animate, // Whether to animate the layoutsrc/lib/components/cytoscape/cytoscape.svelte (1)
27-30: Inconsistent ref initialization patterns.
cytoscapeCoreRefuses$state()withundefined, butcourseDrawerRefandsideControlsRefare declared without initialization. For consistency and to avoid potential TypeScript issues, initialize all refs similarly.🔎 Consistent initialization
// Component refs let cytoscapeCoreRef: CytoscapeCore | undefined = $state(); - let courseDrawerRef: CourseDrawer; - let sideControlsRef: SideControls; + let courseDrawerRef: CourseDrawer | undefined = $state(); + let sideControlsRef: SideControls | undefined = $state();src/lib/components/cytoscape/cytoscape-core.svelte (1)
84-86: Consider consistent early-return guards.The
runLayoutmethod uses optional chaining, while other methods likezoomandfocusOnCourseuse explicit guards with early returns. For consistency and clarity, consider using the same pattern:🔎 Suggested refactor for consistency
export function runLayout(layoutOptions: LayoutOptions) { + if (!cy) return; - cy?.layout(layoutOptions).run(); + cy.layout(layoutOptions).run(); }src/lib/components/cytoscape/course-drawer.svelte (1)
32-52: Good refactor to public API pattern.The shift from prop-based state management to a ref-based public API is a solid architectural improvement. The component now owns its state and exposes control methods, which is more aligned with component composition patterns.
💡 Optional: Consider arrow functions for consistency
For consistency with modern JavaScript patterns, you could use arrow functions:
-export const closeDrawer = function () { +export const closeDrawer = () => { sheetOpen = false; -} +}; -export const openDrawer = function () { +export const openDrawer = () => { sheetOpen = true; -} +}; -export const setSelectedCourse = function (course: Course | undefined) { +export const setSelectedCourse = (course: Course | undefined) => { selectedCourse = course; -} +};
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/color-dot.sveltesrc/lib/components/cytoscape/course-drawer.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/graph-layout.tssrc/lib/components/cytoscape/help-control.sveltesrc/lib/components/cytoscape/legend.sveltesrc/lib/components/cytoscape/side-controls.sveltesrc/lib/mediaStore.ts
💤 Files with no reviewable changes (1)
- src/lib/components/cytoscape/color-dot.svelte
🧰 Additional context used
📓 Path-based instructions (3)
**/*.svelte
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.svelte: Use Svelte 5 runes ($state, $derived, $effect, $props) for reactivity in all Svelte components
Destructure component props from the $props() rune in Svelte 5 components
Avoid legacy createEventDispatcher; use modern Svelte 5 event patterns
Files:
src/lib/components/cytoscape/legend.sveltesrc/lib/components/cytoscape/help-control.sveltesrc/lib/components/cytoscape/cytoscape-core.sveltesrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/side-controls.svelte
src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Group feature components by functionality under course/, charts/, cytoscape/, and map/
Files:
src/lib/components/cytoscape/legend.sveltesrc/lib/components/cytoscape/help-control.sveltesrc/lib/components/cytoscape/cytoscape-init.tssrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/cytoscape-core.sveltesrc/lib/components/cytoscape/graph-layout.tssrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/side-controls.svelte
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with comprehensive typing throughout the codebase
Files:
src/lib/components/cytoscape/cytoscape-init.tssrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/graph-layout.tssrc/lib/mediaStore.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
Applied to files:
src/lib/components/cytoscape/cytoscape-init.tssrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/cytoscape-core.sveltesrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/course/tabs/course-prerequisites.svelte
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/routes/**/+page.svelte : Define pages using SvelteKit file-based routing with +page.svelte
Applied to files:
src/lib/components/cytoscape/course-drawer.svelte
🧬 Code graph analysis (1)
src/lib/components/cytoscape/cytoscape-init.ts (3)
src/lib/components/cytoscape/graph-layout.ts (2)
generateFcoseLayout(77-121)generateLayeredLayout(15-75)src/lib/types/course.ts (2)
CourseReference(5-8)CourseUtils(35-105)src/lib/components/cytoscape/paths.ts (1)
getPredecessorsNotTaken(7-19)
🪛 ast-grep (0.40.3)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
[warning] 185-190: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML = <div class="bg-black text-white p-2 rounded-lg"> <h1 class="text-lg font-semibold">${targetNode.id()}</h1> <p class="text-sm">${targetNode.data("description")}</p> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 185-190: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML = <div class="bg-black text-white p-2 rounded-lg"> <h1 class="text-lg font-semibold">${targetNode.id()}</h1> <p class="text-sm">${targetNode.data("description")}</p> </div>
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-web
🔇 Additional comments (19)
src/lib/components/cytoscape/help-control.svelte (1)
2-15: LGTM!The refactor from prop-based
isDesktopto a centralized store is a good pattern for shared responsive state. The import structure is clean, and the store subscription via$isDesktopintegrates correctly with the component.src/lib/components/cytoscape/legend.svelte (1)
14-30: LGTM!The inline rendering is clean and maintains the same toggling behavior. Good use of
$bindablefor two-way binding and thearia-labelfor accessibility.src/lib/components/cytoscape/graph-layout.ts (1)
15-19: LGTM!The signature change from
focus: string | nulltoanimate: booleanprovides clearer intent and explicit control over animation behavior, aligning well with the broader refactor.src/lib/components/cytoscape/side-controls.svelte (2)
18-32: LGTM!The shift to callback props (
onzoomin,onzoomout, etc.) is a clean pattern that decouples the UI controls from the Cytoscape instance. Proper use of Svelte 5$props()destructuring with optional callbacks.
76-87: Refactor to use Svelte 5 patterns instead of exported getter functions.The exported functions (
getShowCodeLabels,getLayoutType,getElementsAreDraggable) are not idiomatic Svelte 5 and are unused by the parent component. Since the component already uses$state()for reactive state and$props()for props, expose the internal state directly as reactive exports or use$derivedfor computed values instead of getter functions.src/lib/components/cytoscape/cytoscape.svelte (1)
76-98: LGTM on the composition structure.The component composition with
CytoscapeCore,Legend,SideControls,HelpControl, andCourseDraweris well-organized and follows the refactored architecture. The event wiring through callbacks is clean.src/lib/components/cytoscape/cytoscape-core.svelte (3)
1-28: Excellent Svelte 5 patterns and TypeScript usage.The component properly uses Svelte 5 runes ($props, $state) and comprehensive TypeScript typing. The use of
ReturnType<typeof setupCytoscapeHandlers>for the handler type is a good pattern.
29-61: Well-structured initialization and cleanup.The onMount lifecycle properly handles initialization, error cases, and cleanup. The async layout computation with an explanatory comment is clear.
131-131: Clean and minimal template.The template properly provides the container for Cytoscape with appropriate styling classes.
src/lib/components/cytoscape/course-drawer.svelte (1)
73-111: Well-implemented reactive effects.The
$effectblocks properly track dependencies and guard against undefined references. The focus management and URL parameter synchronization logic is clear and correct.src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (4)
10-24: Good use of scratch namespace for state management.Using Cytoscape's scratch space with a namespaced key and type-safe interface is a solid pattern for storing application-specific state on the graph instance.
33-51: Proper reactive mode subscription with cleanup.The use of
$effect.rootto manage the mode subscription is correct and the cleanup function is properly returned in the public API. This prevents memory leaks when the handlers are torn down.
52-127: Well-organized internal helper functions.The helper functions are focused, well-named, and handle their specific responsibilities cleanly. The use of Cytoscape's styling API and class-based styling is appropriate.
200-238: Well-designed public API with proper cleanup.The public API provides all necessary control methods and the
cleanupfunction properly removes all event handlers and terminates reactive effects, preventing memory leaks.src/lib/components/cytoscape/cytoscape-init.ts (5)
1-21: Clean imports and well-defined interfaces.The module imports are organized and the
ComputeLayoutOptionsinterface provides clear typing for the layout computation function.
23-53: Appropriate plugin registration pattern.The plugin registration is properly guarded with a module-level flag to prevent duplicate registration. The
tippyFactoryworkaround for Cytoscape Popper integration is a reasonable approach.
55-101: Well-designed initialization function.The
initializeCytoscapefunction provides a clean API with sensible defaults and proper TypeScript typing. The automatic plugin registration on first call is elegant.
103-120: Clean layout computation with proper async handling.The function provides a simple abstraction for layout computation and properly handles async operations for the layered layout.
131-171: Verify fallback behavior when root course is not found.When the root node is not found (lines 147-150), the function returns all
elementDefinitionsunchanged. This could lead to unexpected behavior if the root course ID is invalid or mistyped. Consider whether logging a warning or returning an empty array would be more appropriate.Current behavior:
if (!rootNode) { tempCy.destroy(); return elementDefinitions; // Returns ALL elements }Alternative approaches to consider:
- Return an empty array if the root is invalid
- Log a warning to help debug invalid root courses
- Throw an error if the root is required
What is the intended behavior when a course isn't found in the graph?
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/course-drawer.svelte (1)
90-111: Invalid assignment to $derived variable.Line 94 attempts to assign
focus = null, butfocusis defined as a$derivedvalue on Line 53. In Svelte 5, derived values are read-only and cannot be assigned to directly. This will cause a compilation or runtime error.The logic at lines 105-106 shows the correct approach: modifying
page.url.searchParamsdirectly, which will then update the derivedfocusvalue. The assignment at line 94 should be removed, as the parameter deletion at line 105 already handles clearing the focus.🔎 Proposed fix
$effect(() => { if (!cytoscapeCoreRef || !allowFocusing) return; - if (!sheetOpen) { - focus = null; - } - if (sheetOpen) { if (selectedCourse) { let courseId = CourseUtils.courseReferenceToSanitizedString( selectedCourse.course_reference, ); page.url.searchParams.set("focus", courseId); } } else { page.url.searchParams.delete("focus"); } tick().then(() => { pushState(page.url, page.state); }); });The
page.url.searchParams.delete("focus")already handles clearing the focus parameter when the drawer closes.
♻️ Duplicate comments (4)
src/lib/components/course/tabs/course-prerequisites.svelte (1)
35-43: Previous reactivity issue has been resolved.The takenCourses and eleDefs values are now properly reactive using Svelte 5's
$derivedrune, addressing the concern raised in the previous review. WhentakenCoursesStoreupdates, both values will re-compute automatically.src/lib/components/cytoscape/cytoscape.svelte (1)
33-47: Previous issues have been resolved.The race condition from the setTimeout has been fixed by using an
$effectthat properly checks forcytoscapeCoreRefavailability, and error handling has been added for thefetchCoursepromise. The implementation now follows Svelte 5 best practices.src/lib/components/cytoscape/course-drawer.svelte (1)
67-71: Previous memory leak issue has been resolved.The direct
.subscribe()call has been replaced with a proper$effectusing$searchModalOpen, which automatically manages subscription cleanup. This follows Svelte 5 best practices.src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (1)
183-206: Previous XSS vulnerability has been resolved.The tooltip creation now properly uses
textContentinstead ofinnerHTMLand builds the DOM usingcreateElementandappendChild. This prevents injection of malicious HTML/JavaScript from course IDs or descriptions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/course/tabs/course-prerequisites.sveltesrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/cytoscape.svelte
🧰 Additional context used
📓 Path-based instructions (3)
**/*.svelte
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.svelte: Use Svelte 5 runes ($state, $derived, $effect, $props) for reactivity in all Svelte components
Destructure component props from the $props() rune in Svelte 5 components
Avoid legacy createEventDispatcher; use modern Svelte 5 event patterns
Files:
src/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/course/tabs/course-prerequisites.svelte
src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Group feature components by functionality under course/, charts/, cytoscape/, and map/
Files:
src/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/course/tabs/course-prerequisites.svelte
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with comprehensive typing throughout the codebase
Files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
Applied to files:
src/lib/components/cytoscape/cytoscape.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.tssrc/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/course/tabs/course-prerequisites.svelte
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to **/*.svelte : Use Svelte 5 runes ($state, $derived, $effect, $props) for reactivity in all Svelte components
Applied to files:
src/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/course/tabs/course-prerequisites.svelte
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to **/*.svelte : Avoid legacy createEventDispatcher; use modern Svelte 5 event patterns
Applied to files:
src/lib/components/cytoscape/course-drawer.svelte
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/routes/**/+page.svelte : Define pages using SvelteKit file-based routing with +page.svelte
Applied to files:
src/lib/components/cytoscape/course-drawer.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-web
- GitHub Check: build-web
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (3)
37-37: Consider typingcurrentTipmore specifically.The
currentTipvariable is typed asany. Consider using a more specific type based on the Cytoscape popper API return type for better type safety.
208-210: Consider reordering tooltip lifecycle for clarity.The current order (show new tip → destroy old tip → store new tip) works correctly but is less clear than: destroy old tip → create new tip → show new tip → store new tip.
🔎 Suggested refactor
+ destroyTip(); const tip = targetNode.popper({ content: () => { const div = document.createElement("div"); const container = document.createElement("div"); container.className = "bg-black text-white p-2 rounded-lg"; 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); div.appendChild(container); return div; }, }); tip.show(); - destroyTip(); currentTip = tip;
1-249: Suggest verification of key behaviors.As noted in the PR objectives, please verify that the refactored behavior remains correct, particularly:
- The
isDesktopstore correctly switches between mobile tooltip and desktop drawer behavior- The
mode.currentreactive updates apply theme changes properly- The
takenCoursesStoresubscription updates graph styling when courses are marked as takenRun the following verification steps manually:
- Toggle between mobile and desktop viewports and verify tap behavior switches between tooltip and drawer
- Switch between light/dark mode and verify graph colors update
- Mark courses as taken and verify graph styling updates immediately
If you'd like, I can help create automated tests for these behaviors in a future PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/cytoscape/course-drawer.sveltesrc/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/cytoscape/course-drawer.svelte
🧰 Additional context used
📓 Path-based instructions (2)
src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Group feature components by functionality under course/, charts/, cytoscape/, and map/
Files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with comprehensive typing throughout the codebase
Files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
Applied to files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧬 Code graph analysis (1)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (5)
src/lib/takenCoursesStore.ts (1)
takenCoursesStore(6-6)src/lib/theme.ts (2)
getTextColor(12-21)getTextOutlineColor(23-32)src/lib/types/course.ts (1)
CourseUtils(35-105)src/lib/components/cytoscape/paths.ts (2)
markNextCourses(37-69)highlightPath(71-97)src/lib/mediaStore.ts (1)
isDesktop(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (8)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (8)
10-24: LGTM: Clean scratch namespace pattern.The scratch namespace implementation follows best practices for storing app-specific state on the Cytoscape instance with proper type safety and lazy initialization.
33-55: Previous reactivity issue has been resolved.The takenCoursesStore subscription (lines 53-55) and cleanup (line 246) have been properly implemented. The store will now trigger
applyTakenCoursesStylereactively whenever courses are marked as taken.
59-94: LGTM: Theme and styling helpers are well-implemented.The
destroyTipandapplyThemeAndLabelStylefunctions correctly handle cleanup and theme-based styling with appropriate color selections for light/dark modes.
96-119: LGTM: Taken courses styling logic is correct.The function correctly applies styling based on the taken courses store. Using
get()for a one-time read is appropriate here since the function is called reactively via the store subscription (line 54).
121-131: LGTM: Subject hiding logic is correct.The function properly manages node visibility by restoring previously hidden nodes before hiding new ones, preventing state accumulation issues.
135-173: LGTM: Event handlers implement correct interaction behavior.The mouse event handlers properly manage highlighting, dragging, and tooltip lifecycle. The integration with path visualization is appropriate.
175-211: Previous XSS vulnerability has been resolved.The tooltip creation now correctly uses
textContent(lines 196, 200) instead ofinnerHTML, which prevents XSS attacks from malicious course IDs or descriptions.
213-248: LGTM: Clean public API with proper cleanup.The event registration and public API provide a well-structured interface for controlling the Cytoscape instance. The cleanup method properly unregisters all handlers and unsubscribes from stores (line 246).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (3)
37-37: Consider typing currentTip more specifically.The
currentTipvariable is typed asany. Based on line 190 where it's assigned fromtargetNode.popper(), it could be typed asReturnType<cytoscape.NodeSingular['popper']>or a Cytoscape popper type if available.🔎 Suggested improvement
- let currentTip: any = undefined; + let currentTip: ReturnType<cytoscape.NodeSingular['popper']> | undefined = undefined;
137-137: Consider typing event parameters more specifically.The event handler parameters are typed as
any. They could be typed ascytoscape.EventObjectfor better type safety and IDE support.🔎 Suggested improvement
- const mouseoverHandler = (event: any) => { + const mouseoverHandler = (event: cytoscape.EventObject) => { const targetNode = event.target; // ... }; - const dbltapHandler = (event: any) => { + const dbltapHandler = (event: cytoscape.EventObject) => { const targetNode = event.target; // ... }; - const onetapHandler = async (event: any) => { + const onetapHandler = async (event: cytoscape.EventObject) => { const targetNode = event.target; // ... };Also applies to: 165-165, 177-177
177-177: Remove unnecessary async keyword.The
onetapHandlerfunction is declared asasyncbut doesn't useawaitanywhere in its body. Theasynckeyword adds unnecessary overhead.🔎 Suggested improvement
- const onetapHandler = async (event: any) => { + const onetapHandler = (event: cytoscape.EventObject) => { const targetNode = event.target;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Group feature components by functionality under course/, charts/, cytoscape/, and map/
Files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with comprehensive typing throughout the codebase
Files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
📚 Learning: 2025-08-24T08:36:25.768Z
Learnt from: CR
Repo: twangodev/uw-coursemap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T08:36:25.768Z
Learning: Applies to src/lib/components/{course,charts,cytoscape,map}/**/*.{svelte,ts,js} : Group feature components by functionality under course/, charts/, cytoscape/, and map/
Applied to files:
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts
🧬 Code graph analysis (1)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (5)
src/lib/theme.ts (2)
getTextColor(12-21)getTextOutlineColor(23-32)src/lib/takenCoursesStore.ts (1)
takenCoursesStore(6-6)src/lib/types/course.ts (1)
CourseUtils(35-105)src/lib/components/cytoscape/paths.ts (3)
markNextCourses(37-69)clearPath(99-107)highlightPath(71-97)src/lib/mediaStore.ts (1)
isDesktop(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (7)
src/lib/components/cytoscape/cytoscape-handlers.svelte.ts (7)
1-24: LGTM!The imports are appropriate and the scratch namespace pattern is a clean way to store app-specific state on the Cytoscape instance. The type definitions and helper function are well-structured.
50-105: LGTM!The styling functions correctly apply theme-based colors and manage taken/next course states. The previous reactivity concern has been properly addressed by the subscription on lines 131-133, ensuring styling updates whenever
takenCoursesStorechanges.
107-117: LGTM!The subject visibility logic correctly manages node removal and restoration using the scratch namespace. The optional chaining safely handles null cases.
122-133: LGTM!The reactive subscriptions are properly implemented using Svelte 5's
$effect.rootfor mode tracking and standard store subscription for taken courses. Both cleanup functions are correctly captured and called in the public cleanup method.
190-207: Excellent fix for the XSS vulnerability!The tooltip creation now correctly uses
textContentinstead ofinnerHTMLand builds the DOM structure usingdocument.createElement. This prevents XSS attacks that could occur if course IDs or descriptions contained malicious HTML/JavaScript.
215-219: LGTM!All event handlers are properly registered with appropriate event types and element selectors.
223-250: LGTM!The public API is well-designed with a clean separation of concerns. The
cleanupmethod comprehensively removes all event listeners, destroys the tooltip, and unsubscribes from reactive dependencies, preventing memory leaks. The closure-based state management provides good encapsulation.
2 participants
please make sure everything on this branch still works as intended (we should probably make tests at some point)
I split cytoscape into 4 different files
I added back the legend but rn its not actually functional, purely only there visually. In a future pr we can add back the functionality.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Refactor