Repository navigation
Feat: Dependency Graph Diagram - #16
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a client-side dependency graph feature: a new TypeScript graph module with public API, UI panels and controls to toggle list/graph views, lazy graph initialization, canvas-based rendering with pan/zoom/hover/focus, workspace selection, popover UI, and accompanying styles and DOM wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as View Controller
participant Graph as Graph Module
participant Canvas as Canvas Renderer
participant Popover as Popover UI
User->>UI: Click "Graph View"
UI->>Graph: initGraphView(options) / switchWorkspace(name)
Graph->>Graph: adaptDataset() / buildWorkspaceGraph()
Graph->>Graph: computeAmplification() / layoutGraph()
Graph->>Canvas: requestRender()
Canvas->>Canvas: draw nodes, edges, labels
User->>Canvas: Hover node
Canvas->>Graph: updateHover(slug)
Graph->>Canvas: requestRender()
User->>Canvas: Click node
Canvas->>Graph: applyFocus(slug)
Graph->>Canvas: requestRender()
Graph->>Popover: showPopover(slug)
User->>Popover: "Open in List"
Popover->>UI: onOpenList(slug)
UI->>UI: switch to list view and highlight item
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
report-ui/main.ts (1)
2129-2154: Redundant ID lookup pattern.The function tries two different ID formats (
dep-${slug}andgetDepDomId(slug)) butgetDepDomId(slug)returns"dep-" + slug, making them equivalent. The duplicate lookups on lines 2131-2134 and 2138-2139 are unnecessary.♻️ Suggested simplification
function openListFromGraph(slug: string): void { setActiveView("list"); - let target = document.getElementById(`dep-${slug}`); - if (!target) { - target = document.getElementById(getDepDomId(slug)); - } + let target = document.getElementById(getDepDomId(slug)); if (!target && depByKey.has(slug)) { forcedVisibleDepKeys.add(slug); renderList(); - target = document.getElementById(`dep-${slug}`); - if (!target) target = document.getElementById(getDepDomId(slug)); + target = document.getElementById(getDepDomId(slug)); } if (!target) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` around lines 2129 - 2154, The dual ID lookups in openListFromGraph are redundant because getDepDomId(slug) already returns "dep-"+slug; replace the repeated document.getElementById calls with a single lookup using getDepDomId(slug) to set target initially, remove the extra fallback checks, and keep the existing branch that, when !target and depByKey.has(slug), adds slug to forcedVisibleDepKeys, calls renderList(), then re-attempts a single getDepDomId(slug) lookup to set target; preserve the rest of the logic that handles HTMLDetailsElement (using openDepKeys and ensureDepDetailsRendered), the highlight class, scrollIntoView, and timeout removal.report-ui/style.css (1)
44-100: Consider consolidating duplicated graph CSS variables.The graph-related CSS variables are defined identically in
:rootand[data-theme="dark"](lines 44-52 vs 80-89), and similarly for:root.lightand[data-theme="light"](lines 69-78 vs 91-100). This duplication increases maintenance burden.Consider defining the graph variables once in
:rootand only overriding them in the light theme variants:♻️ Suggested simplification
:root { /* ... existing variables ... */ --graph-direct-runtime: var(--green); --graph-direct-dev: var(--amber); --graph-transitive: var(--accent); --graph-edge: var(--border-color-strong); --graph-highlight: var(--accent-hover); --graph-muted: var(--text-muted); --graph-vuln-high: var(--red); --graph-vuln-medium: var(--amber); } :root.light, [data-theme="light"] { /* ... existing overrides ... */ --graph-highlight: var(--accent); --graph-muted: var(--text-secondary); } -[data-theme="dark"] { - --graph-direct-runtime: var(--green); - --graph-direct-dev: var(--amber); - --graph-transitive: var(--accent); - --graph-edge: var(--border-color-strong); - --graph-highlight: var(--accent-hover); - --graph-muted: var(--text-muted); - --graph-vuln-high: var(--red); - --graph-vuln-medium: var(--amber); -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/style.css` around lines 44 - 100, Consolidate the duplicated graph-related CSS vars by declaring them once in :root (e.g., --graph-direct-runtime, --graph-direct-dev, --graph-transitive, --graph-edge, --graph-highlight, --graph-muted, --graph-vuln-high, --graph-vuln-medium) and remove the identical blocks from :root.light and [data-theme="dark"]; keep only the overrides that actually differ (for example --graph-highlight and --graph-muted in :root.light or [data-theme="light"]) inside the theme-specific selectors (:root.light and [data-theme="light"]) so theme-specific differences persist while eliminating duplicated declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@report-ui/graphView.ts`:
- Around line 1250-1256: The current code writes workspace names into options
via innerHTML which only escapes quotes and leaves text nodes vulnerable (e.g.,
"<script>"); replace the innerHTML construction with DOM-safe creation: for each
workspace create an HTMLOptionElement (document.createElement('option') or new
Option()), set option.value = workspace.name and option.textContent =
workspace.name (or use option.setAttribute('value', workspace.name)), then
append to options.workspaceSelect (clearing existing children first) so both the
attribute and visible text are correctly escaped and no raw HTML from
workspace.name is injected.
In `@report-ui/index.html`:
- Around line 254-263: The Graph View button (id="view-graph-btn",
class="view-switch-btn active") is marked active while the List view panel is
actually shown; remove the stray "active" class from the Graph button and ensure
the same "active" class is applied to the view-switch button that corresponds to
the visible panel (and that the panel element for the list view has the matching
active state), so the button state and displayed panel (data-view attributes /
panel classes) are synchronized.
In `@src/report.ts`:
- Around line 201-210: The Graph View button currently wrongly has the active
state; update the view-switch markup so the initially active button matches the
initially shown panel by removing the "active" class from the element with id
"view-graph-btn" (and ensure the corresponding List View button uses class
"view-switch-btn active"); locate the elements by id "view-graph-btn" and the
list-view button (same "view-switch-btn" class) and mirror the fix applied in
index.html so active state and visible panel are consistent.
---
Nitpick comments:
In `@report-ui/main.ts`:
- Around line 2129-2154: The dual ID lookups in openListFromGraph are redundant
because getDepDomId(slug) already returns "dep-"+slug; replace the repeated
document.getElementById calls with a single lookup using getDepDomId(slug) to
set target initially, remove the extra fallback checks, and keep the existing
branch that, when !target and depByKey.has(slug), adds slug to
forcedVisibleDepKeys, calls renderList(), then re-attempts a single
getDepDomId(slug) lookup to set target; preserve the rest of the logic that
handles HTMLDetailsElement (using openDepKeys and ensureDepDetailsRendered), the
highlight class, scrollIntoView, and timeout removal.
In `@report-ui/style.css`:
- Around line 44-100: Consolidate the duplicated graph-related CSS vars by
declaring them once in :root (e.g., --graph-direct-runtime, --graph-direct-dev,
--graph-transitive, --graph-edge, --graph-highlight, --graph-muted,
--graph-vuln-high, --graph-vuln-medium) and remove the identical blocks from
:root.light and [data-theme="dark"]; keep only the overrides that actually
differ (for example --graph-highlight and --graph-muted in :root.light or
[data-theme="light"]) inside the theme-specific selectors (:root.light and
[data-theme="light"]) so theme-specific differences persist while eliminating
duplicated declarations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/report-assets.jsis excluded by!**/dist/**dist/report.jsis excluded by!**/dist/**report-ui/dist/report.cssis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (6)
report-ui/graphView.tsreport-ui/index.htmlreport-ui/main.tsreport-ui/style.csssrc/report-assets.tssrc/report.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
report-ui/main.ts (1)
672-674: Consider simplifyinggetDepDomIdusing template literal.Minor style suggestion for consistency with other similar functions in the codebase.
♻️ Optional simplification
function getDepDomId(depKey: string): string { - return "dep-" + depKey; + return `dep-${depKey}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` around lines 672 - 674, The getDepDomId function currently concatenates strings using "+"; refactor it to use a template literal for consistency and readability by replacing the return "dep-" + depKey with a template: return `dep-${depKey}` inside the getDepDomId(depKey: string) function.report-ui/graphView.ts (3)
1272-1276: Event listeners are not cleaned up when the graph is deactivated.The
mousemoveandmouseuplisteners are attached towindow, but there's no cleanup when the graph view is hidden. While this won't cause functional issues sincepanState.downgates the logic, it's slightly wasteful. Consider adding cleanup insetActive(false).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 1272 - 1276, When deactivating the graph in setActive(false) remove the listeners you previously attached so they don't remain on window/canvas: call options.canvas.removeEventListener for handleCanvasMouseDown, handleWheel, and the mouseleave/updateHover handler, and call window.removeEventListener for handleWindowMouseMove and handleWindowMouseUp; in setActive(true) ensure you re-add the same listeners (using the same handler references) so behavior is unchanged. Target the handlers named handleCanvasMouseDown, handleWindowMouseMove, handleWindowMouseUp, handleWheel and the anonymous mouseleave => updateHover listener (replace the anonymous listener with a named function if needed) inside the setActive method.
657-668: Non-null assertion operators assume nodes always exist.The
graph.nodes.get(a)!andgraph.nodes.get(b)!assertions could theoretically fail if the layer contains a slug not in the nodes map. While unlikely given the build logic, adding a guard would be safer.🛡️ Defensive check
layer.sort((a, b) => { - const nodeA = graph.nodes.get(a)!; - const nodeB = graph.nodes.get(b)!; + const nodeA = graph.nodes.get(a); + const nodeB = graph.nodes.get(b); + if (!nodeA || !nodeB) return 0; if (nodeA.amplification !== nodeB.amplification) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 657 - 668, The sort callback in layer.sort uses non-null assertions graph.nodes.get(a)! and graph.nodes.get(b)! which can throw if a slug is missing; replace them with guarded lookups (const nodeA = graph.nodes.get(a); const nodeB = graph.nodes.get(b)) and handle undefined deterministically: if both undefined return 0, if nodeA undefined return 1 (place after nodeB), if nodeB undefined return -1, otherwise continue the existing comparison logic (amplification, kind special-case for 'direct-runtime', then ref.name localeCompare) so the sorter never assumes nodes exist.
116-120: Consider usingMath.minandMath.maxfor the clamp function.The current implementation is correct but could be simplified.
♻️ Optional simplification
function clamp(value: number, min: number, max: number): number { - if (value < min) return min; - if (value > max) return max; - return value; + return Math.max(min, Math.min(max, value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 116 - 120, The clamp function can be simplified: replace the current conditional body in the function clamp(value: number, min: number, max: number) with a single expression that uses Math.min and Math.max to preserve identical behavior (e.g., return Math.min(max, Math.max(min, value))); keep the same function signature and types to ensure no behavioral change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@report-ui/graphView.ts`:
- Around line 402-405: Replace the hard throw on a null 2D context with a
graceful degradation: when options.canvas.getContext('2d') returns null, log a
warning (or call a provided logger), set a flag (e.g. this.noCanvas or
options.hasCanvas = false) and return early from the drawing/initialization
routine so the rest of the report continues to render; optionally attempt a
fallback by creating an OffscreenCanvas or switching to an SVG/text placeholder.
Concretely, change the block that references context and
options.canvas.getContext to handle null by setting the flag, skipping drawing
code that uses context, and using a fallback render path instead of throwing an
Error.
---
Nitpick comments:
In `@report-ui/graphView.ts`:
- Around line 1272-1276: When deactivating the graph in setActive(false) remove
the listeners you previously attached so they don't remain on window/canvas:
call options.canvas.removeEventListener for handleCanvasMouseDown, handleWheel,
and the mouseleave/updateHover handler, and call window.removeEventListener for
handleWindowMouseMove and handleWindowMouseUp; in setActive(true) ensure you
re-add the same listeners (using the same handler references) so behavior is
unchanged. Target the handlers named handleCanvasMouseDown,
handleWindowMouseMove, handleWindowMouseUp, handleWheel and the anonymous
mouseleave => updateHover listener (replace the anonymous listener with a named
function if needed) inside the setActive method.
- Around line 657-668: The sort callback in layer.sort uses non-null assertions
graph.nodes.get(a)! and graph.nodes.get(b)! which can throw if a slug is
missing; replace them with guarded lookups (const nodeA = graph.nodes.get(a);
const nodeB = graph.nodes.get(b)) and handle undefined deterministically: if
both undefined return 0, if nodeA undefined return 1 (place after nodeB), if
nodeB undefined return -1, otherwise continue the existing comparison logic
(amplification, kind special-case for 'direct-runtime', then ref.name
localeCompare) so the sorter never assumes nodes exist.
- Around line 116-120: The clamp function can be simplified: replace the current
conditional body in the function clamp(value: number, min: number, max: number)
with a single expression that uses Math.min and Math.max to preserve identical
behavior (e.g., return Math.min(max, Math.max(min, value))); keep the same
function signature and types to ensure no behavioral change.
In `@report-ui/main.ts`:
- Around line 672-674: The getDepDomId function currently concatenates strings
using "+"; refactor it to use a template literal for consistency and readability
by replacing the return "dep-" + depKey with a template: return `dep-${depKey}`
inside the getDepDomId(depKey: string) function.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
dist/report-assets.jsis excluded by!**/dist/**dist/report.jsis excluded by!**/dist/**dist/runners/lockfileGraph.jsis excluded by!**/dist/**report-ui/dist/report.cssis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (6)
report-ui/graphView.tsreport-ui/index.htmlreport-ui/main.tsreport-ui/style.csssrc/report-assets.tssrc/report.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- report-ui/index.html
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
report-ui/graphView.ts (1)
514-526: Consider using index-based iteration for better performance.Using
queue.shift()in a while loop has O(n) complexity per shift, making the BFS O(n²) in the worst case. For large dependency graphs, this could be noticeable.♻️ Suggested optimization using index-based iteration
const included = new Set<string>(); - const queue = [...roots]; - while (queue.length > 0) { - const slug = queue.shift(); - if (!slug) continue; + const queue = [...roots]; + let queueIndex = 0; + while (queueIndex < queue.length) { + const slug = queue[queueIndex++]; if (included.has(slug)) continue; if (!dataset.dependencies[slug]) continue; included.add(slug);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 514 - 526, The BFS uses queue.shift() which is O(n) per dequeue; replace it with index-based iteration by keeping an index (e.g., let i = 0) and using queue[i++] to advance through the array so dequeues are O(1). Update the loop that currently references queue.shift() (the block that initializes const included = new Set<string>(); const queue = [...roots]; while (queue.length > 0) { const slug = queue.shift(); ... }) to use index iteration while preserving the same checks against included, dataset.dependencies and pushing children from childrenBySlug.report-ui/main.ts (2)
2168-2168: Consider if graph render on filter change is necessary.The
graphView?.requestRender()call on filter changes triggers a graph re-render, but the graph view shows the full workspace graph regardless of list filters. This call is harmless but may be unnecessary overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` at line 2168, The graphView?.requestRender() call unconditionally triggers a graph re-render on list filter changes; either remove this call from the filter-change handler or guard it so it only runs when the active filters actually affect graph visibility. Locate the handler that invokes graphView?.requestRender() (the code surrounding graphView?.requestRender()) and either delete that line or add a predicate that compares the previous and new filter state (or a boolean like filtersAffectGraph) before calling graphView.requestRender().
1683-1700: Type assertions assume DOM elements exist.The graph-related element bindings use non-null type assertions. If any of these elements are missing from the HTML, runtime errors will occur. This matches the existing pattern in the file, but consider adding null checks or optional chaining where the graph view is used if HTML structure might vary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` around lines 1683 - 1700, The current element bindings (e.g., viewGraphButton, graphBackButton, listViewPanel, graphViewPanel, graphWorkspaceSelect, graphCanvas, graphCanvasShell, graphPopover, graphPopoverName, graphPopoverVersion, graphPopoverLicense, graphPopoverVulns, graphPopoverAmplification, graphOpenList, reportFooter) use non-null assertions which will throw at runtime if any DOM node is missing; change them to nullable types (remove the "as ...Element" non-null assertions) and add guards where these symbols are used (early return or feature-check conditional) so all accesses to properties or method calls on these elements first verify the variable is not null/undefined, or provide a graceful fallback/error log when a required element is absent. Ensure every usage path of the graph view (e.g., handlers referenced by viewGraphButton, graphCanvas rendering logic, graphPopover updates) checks the element presence before DOM operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@report-ui/graphView.ts`:
- Around line 514-526: The BFS uses queue.shift() which is O(n) per dequeue;
replace it with index-based iteration by keeping an index (e.g., let i = 0) and
using queue[i++] to advance through the array so dequeues are O(1). Update the
loop that currently references queue.shift() (the block that initializes const
included = new Set<string>(); const queue = [...roots]; while (queue.length > 0)
{ const slug = queue.shift(); ... }) to use index iteration while preserving the
same checks against included, dataset.dependencies and pushing children from
childrenBySlug.
In `@report-ui/main.ts`:
- Line 2168: The graphView?.requestRender() call unconditionally triggers a
graph re-render on list filter changes; either remove this call from the
filter-change handler or guard it so it only runs when the active filters
actually affect graph visibility. Locate the handler that invokes
graphView?.requestRender() (the code surrounding graphView?.requestRender()) and
either delete that line or add a predicate that compares the previous and new
filter state (or a boolean like filtersAffectGraph) before calling
graphView.requestRender().
- Around line 1683-1700: The current element bindings (e.g., viewGraphButton,
graphBackButton, listViewPanel, graphViewPanel, graphWorkspaceSelect,
graphCanvas, graphCanvasShell, graphPopover, graphPopoverName,
graphPopoverVersion, graphPopoverLicense, graphPopoverVulns,
graphPopoverAmplification, graphOpenList, reportFooter) use non-null assertions
which will throw at runtime if any DOM node is missing; change them to nullable
types (remove the "as ...Element" non-null assertions) and add guards where
these symbols are used (early return or feature-check conditional) so all
accesses to properties or method calls on these elements first verify the
variable is not null/undefined, or provide a graceful fallback/error log when a
required element is absent. Ensure every usage path of the graph view (e.g.,
handlers referenced by viewGraphButton, graphCanvas rendering logic,
graphPopover updates) checks the element presence before DOM operations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/report-assets.jsis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
report-ui/graphView.tsreport-ui/main.tssrc/report-assets.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
report-ui/graphView.ts (3)
1277-1284: Global mousedown listener for popover dismissal lacks cleanup.This
document.addEventListener('mousedown', ...)is added duringsetupControls()but never removed. While this is generally fine for the lifetime of the app, it could cause issues if the graph view is destroyed and recreated.♻️ Track and clean up the document listener
+ let documentMouseDownHandler: ((event: MouseEvent) => void) | null = null; + function setupControls(): void { // ... existing code ... - document.addEventListener('mousedown', (event) => { + documentMouseDownHandler = (event: MouseEvent) => { if (!active) return; const target = event.target as Node; if (options.popover.hidden) return; if (options.popover.contains(target)) return; if (options.canvasHost.contains(target)) return; hidePopover(); - }); + }; + document.addEventListener('mousedown', documentMouseDownHandler); } + // In unbindInteractionListeners or a new cleanup function: + function cleanup(): void { + if (documentMouseDownHandler) { + document.removeEventListener('mousedown', documentMouseDownHandler); + documentMouseDownHandler = null; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 1277 - 1284, The global mousedown listener added in setupControls() is never removed; store the listener callback (e.g. const onDocumentMouseDown = (event) => { ... }) instead of an anonymous function and call document.removeEventListener('mousedown', onDocumentMouseDown) during cleanup/remove lifecycle (e.g. in destroy(), dispose(), or teardownControls()). Ensure the handler references the same symbols (options.popover, options.canvasHost, hidePopover()) so popover dismissal logic remains identical while preventing leaks when the graph view is destroyed and recreated.
1195-1202: Wheel event handler prevents default scrolling.The
event.preventDefault()on wheel events prevents page scrolling when the cursor is over the canvas. This is intentional for zoom behavior, but users might find it unexpected if they're trying to scroll past the graph view.Consider adding a modifier key requirement (e.g., Ctrl+wheel) for zooming, which is a common UX pattern for embedded canvas elements. This would allow normal scrolling when the modifier isn't pressed.
♻️ Optional: Add modifier key for zoom
function handleWheel(event: WheelEvent): void { + // Only zoom when Ctrl/Cmd is held, otherwise allow normal scroll + if (!event.ctrlKey && !event.metaKey) return; event.preventDefault(); const rect = options.canvas.getBoundingClientRect(); const x = event.clientX - rect.left; const y = event.clientY - rect.top; const factor = Math.exp(-event.deltaY * 0.0013); applyZoom(zoom * factor, x, y); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 1195 - 1202, The wheel handler currently always calls event.preventDefault() and zooms, blocking page scroll; update handleWheel to only preventDefault and call applyZoom when a modifier key is held (e.g., event.ctrlKey || event.metaKey to support Windows/Linux and Mac), otherwise return and allow normal scrolling; locate the function handleWheel (uses options.canvas, zoom, applyZoom) and gate the zoom logic behind the modifier key check while leaving other calculations intact.
405-417: Static innerHTML usage is safe here, but could use textContent for consistency.The static analysis flagged this innerHTML assignment. While the content is a hardcoded string (no user input), using DOM APIs would be more consistent with the XSS-safe approach used elsewhere (e.g., the workspace select fix at lines 1298-1303).
♻️ Optional: Use DOM APIs for consistency
function showCanvasFallback(): void { if (fallbackShown) return; fallbackShown = true; console.warn('Dependency Radar: unable to initialize 2D canvas; graph rendering disabled.'); options.controlsRoot.classList.add('hidden'); options.workspaceWrap.classList.add('hidden'); options.canvas.style.display = 'none'; const fallback = document.createElement('div'); fallback.className = 'empty-state'; - fallback.innerHTML = - '<div class="empty-state-text">Graph view is unavailable in this browser context.</div>'; + const text = document.createElement('div'); + text.className = 'empty-state-text'; + text.textContent = 'Graph view is unavailable in this browser context.'; + fallback.appendChild(text); options.canvasHost.appendChild(fallback); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 405 - 417, In showCanvasFallback(), replace the static fallback.innerHTML assignment with DOM API calls to create and append the child elements: create a container div (class "empty-state-text"), set its textContent to "Graph view is unavailable in this browser context.", append that to the fallback div (class "empty-state"), and then append fallback to options.canvasHost; this keeps the same structure but avoids innerHTML while using the existing function name and the fallback, options.canvasHost, and options.canvas elements referenced in the diff.report-ui/main.ts (1)
2086-2152: Consider extracting deeply nested null checks.The function handles missing DOM nodes gracefully with console warnings. However, the long chain of null checks (lines 2110-2122) could be simplified for readability.
♻️ Optional: Extract graph DOM validation
+ const graphDomElements = [ + controls.graphWorkspaceSelect, + controls.graphWorkspaceWrap, + controls.graphControls, + controls.graphCanvas, + controls.graphCanvasShell, + controls.graphPopover, + controls.graphPopoverName, + controls.graphPopoverVersion, + controls.graphPopoverLicense, + controls.graphPopoverVulns, + controls.graphPopoverAmplification, + controls.graphOpenList, + ]; + function setActiveView(view: "list" | "graph"): void { // ... existing code ... if (!graphInitialized) { - if ( - !controls.graphWorkspaceSelect || - !controls.graphWorkspaceWrap || - // ... 10 more checks ... - ) { + if (graphDomElements.some((el) => !el)) { console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled."); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` around lines 2086 - 2152, The long chain of null checks inside setActiveView makes the function hard to read; extract that validation into a helper like validateGraphDomNodes or controlsHasGraphNodes which takes the controls object and returns a boolean, then replace the big if (...) block that checks graphWorkspaceSelect, graphWorkspaceWrap, graphControls, graphCanvas, graphCanvasShell, graphPopover, graphPopoverName, graphPopoverVersion, graphPopoverLicense, graphPopoverVulns, graphPopoverAmplification, graphOpenList with a single if (!validateGraphDomNodes(controls)) { console.warn(...); return; }, keeping graphInitialized, graphView initialization, and subsequent calls (graphView.initGraphView, graphView.setActive, graphView.requestRender) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@report-ui/graphView.ts`:
- Around line 1277-1284: The global mousedown listener added in setupControls()
is never removed; store the listener callback (e.g. const onDocumentMouseDown =
(event) => { ... }) instead of an anonymous function and call
document.removeEventListener('mousedown', onDocumentMouseDown) during
cleanup/remove lifecycle (e.g. in destroy(), dispose(), or teardownControls()).
Ensure the handler references the same symbols (options.popover,
options.canvasHost, hidePopover()) so popover dismissal logic remains identical
while preventing leaks when the graph view is destroyed and recreated.
- Around line 1195-1202: The wheel handler currently always calls
event.preventDefault() and zooms, blocking page scroll; update handleWheel to
only preventDefault and call applyZoom when a modifier key is held (e.g.,
event.ctrlKey || event.metaKey to support Windows/Linux and Mac), otherwise
return and allow normal scrolling; locate the function handleWheel (uses
options.canvas, zoom, applyZoom) and gate the zoom logic behind the modifier key
check while leaving other calculations intact.
- Around line 405-417: In showCanvasFallback(), replace the static
fallback.innerHTML assignment with DOM API calls to create and append the child
elements: create a container div (class "empty-state-text"), set its textContent
to "Graph view is unavailable in this browser context.", append that to the
fallback div (class "empty-state"), and then append fallback to
options.canvasHost; this keeps the same structure but avoids innerHTML while
using the existing function name and the fallback, options.canvasHost, and
options.canvas elements referenced in the diff.
In `@report-ui/main.ts`:
- Around line 2086-2152: The long chain of null checks inside setActiveView
makes the function hard to read; extract that validation into a helper like
validateGraphDomNodes or controlsHasGraphNodes which takes the controls object
and returns a boolean, then replace the big if (...) block that checks
graphWorkspaceSelect, graphWorkspaceWrap, graphControls, graphCanvas,
graphCanvasShell, graphPopover, graphPopoverName, graphPopoverVersion,
graphPopoverLicense, graphPopoverVulns, graphPopoverAmplification, graphOpenList
with a single if (!validateGraphDomNodes(controls)) { console.warn(...); return;
}, keeping graphInitialized, graphView initialization, and subsequent calls
(graphView.initGraphView, graphView.setActive, graphView.requestRender)
unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/report-assets.jsis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
report-ui/graphView.tsreport-ui/main.tssrc/report-assets.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@report-ui/graphView.ts`:
- Around line 1109-1124: The tick() loop currently always calls
requestAnimationFrame, so update it to stop scheduling frames when the graph is
idle: inside tick() (the function named tick) after updateTargets() and const
moving = animateNodes(), only call frameId = window.requestAnimationFrame(tick)
if active && (dirty || moving) (and set frameId = 0 otherwise); ensure frameId
is cleared when exiting (when !active) and apply the same conditional scheduling
change to the other identical RAF call block referenced around lines 1126–1129
so the RAF loop stops when nothing is dirty or moving.
- Line 513: Multiple arrow callbacks used with .forEach are expression-bodied
and implicitly return the result of Set.add()/Map.set(), which violates the
Biome rule useIterableCallbackReturn; for each occurrence (e.g., the call using
roots.add at the snippet shown and the callbacks at lines 720, 772–773, 809–810,
872–873, 877–878), convert the arrow to a block body and call the mutating
method inside without returning its value — for example change .forEach((slug)
=> roots.add(slug)) to .forEach((slug) => { roots.add(slug); }); do the same for
callbacks that call map.set(...) or similar so they become { map.set(key, val);
} instead of an expression.
In `@report-ui/main.ts`:
- Around line 2103-2131: In setActiveView, avoid toggling the UI into graph mode
before confirming graph DOM prerequisites; when view === "graph" first verify
controls.listViewPanel && controls.graphViewPanel and call hasGraphDomNodes()
(and return early if missing) and only then update currentView, toggle
classes/aria, footer, body class and initialize graphView via initGraphView if
!graphInitialized; ensure graphView?.setActive(false) is only called when
switching back to list and graphView?.setActive(true) is invoked after
successful graph initialization — reference setActiveView,
controls.listViewPanel, controls.graphViewPanel, hasGraphDomNodes(),
graphInitialized, initGraphView, and graphView.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/report-assets.jsis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
report-ui/graphView.tsreport-ui/main.tssrc/report-assets.ts
| Object.keys(dataset.dependencies) | ||
| .filter((slug) => (parentsBySlug.get(slug) || []).length === 0) | ||
| .slice(0, 40) | ||
| .forEach((slug) => roots.add(slug)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists
ls -la report-ui/graphView.tsRepository: JosephMaynard/dependency-radar
Length of output: 146
🏁 Script executed:
# Get line count to ensure line numbers are valid
wc -l report-ui/graphView.tsRepository: JosephMaynard/dependency-radar
Length of output: 101
🏁 Script executed:
# Check the specific lines mentioned in the review
# Line 513
echo "=== Line 513 ===" && sed -n '513p' report-ui/graphView.ts
# Lines around 720
echo "=== Line 720 ===" && sed -n '720p' report-ui/graphView.ts
# Lines around 772-773
echo "=== Lines 772-773 ===" && sed -n '772,773p' report-ui/graphView.ts
# Lines around 809-810
echo "=== Lines 809-810 ===" && sed -n '809,810p' report-ui/graphView.ts
# Lines around 872-873
echo "=== Lines 872-873 ===" && sed -n '872,873p' report-ui/graphView.ts
# Lines around 877-878
echo "=== Lines 877-878 ===" && sed -n '877,878p' report-ui/graphView.tsRepository: JosephMaynard/dependency-radar
Length of output: 839
🏁 Script executed:
# Run the verification command from the review comment
rg -nP '\.forEach\(\([^)]*\)\s*=>\s*(?!\{)' report-ui/graphView.tsRepository: JosephMaynard/dependency-radar
Length of output: 2915
Fix expression-bodied forEach callbacks that trigger the Biome lint rule useIterableCallbackReturn.
These callbacks on lines 513, 720, 772–773, 809–810, 872–873, and 877–878 implicitly return Set.add()/Map.set() results. Convert each to a block body.
🛠️ Proposed fix
- .forEach((slug) => roots.add(slug));
+ .forEach((slug) => {
+ roots.add(slug);
+ });
- layer.forEach((slug, index) => layerOrder.set(slug, index));
+ layer.forEach((slug, index) => {
+ layerOrder.set(slug, index);
+ });
- ancestors.forEach((nodeSlug) => focusNodes.add(nodeSlug));
- descendants.forEach((nodeSlug) => focusNodes.add(nodeSlug));
+ ancestors.forEach((nodeSlug) => {
+ focusNodes.add(nodeSlug);
+ });
+ descendants.forEach((nodeSlug) => {
+ focusNodes.add(nodeSlug);
+ });
- selected.parents.forEach((nodeSlug) => focusPushNodes.add(nodeSlug));
- selected.children.forEach((nodeSlug) => focusPushNodes.add(nodeSlug));
+ selected.parents.forEach((nodeSlug) => {
+ focusPushNodes.add(nodeSlug);
+ });
+ selected.children.forEach((nodeSlug) => {
+ focusPushNodes.add(nodeSlug);
+ });
- ancestors.forEach((nodeSlug) => hoverNodes.add(nodeSlug));
- descendants.forEach((nodeSlug) => hoverNodes.add(nodeSlug));
+ ancestors.forEach((nodeSlug) => {
+ hoverNodes.add(nodeSlug);
+ });
+ descendants.forEach((nodeSlug) => {
+ hoverNodes.add(nodeSlug);
+ });
- node.parents.forEach((parent) => hoverNodes.add(parent));
- node.children.forEach((child) => hoverNodes.add(child));
+ node.parents.forEach((parent) => {
+ hoverNodes.add(parent);
+ });
+ node.children.forEach((child) => {
+ hoverNodes.add(child);
+ });🧰 Tools
🪛 Biome (2.4.4)
[error] 513-513: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@report-ui/graphView.ts` at line 513, Multiple arrow callbacks used with
.forEach are expression-bodied and implicitly return the result of
Set.add()/Map.set(), which violates the Biome rule useIterableCallbackReturn;
for each occurrence (e.g., the call using roots.add at the snippet shown and the
callbacks at lines 720, 772–773, 809–810, 872–873, 877–878), convert the arrow
to a block body and call the mutating method inside without returning its value
— for example change .forEach((slug) => roots.add(slug)) to .forEach((slug) => {
roots.add(slug); }); do the same for callbacks that call map.set(...) or similar
so they become { map.set(key, val); } instead of an expression.
Comment on lines
+1109
to
+1124
| function tick(): void { | ||
| if (!active) { | ||
| frameId = 0; | ||
| return; | ||
| } | ||
|
|
||
| updateTargets(); | ||
| const moving = animateNodes(); | ||
|
|
||
| if (dirty || moving) { | ||
| renderGraph(); | ||
| dirty = false; | ||
| } | ||
|
|
||
| frameId = window.requestAnimationFrame(tick); | ||
| } |
Contributor
There was a problem hiding this comment.
Stop the RAF loop when the graph is idle.
The current tick() keeps scheduling frames even when nothing is dirty/moving, which burns CPU while graph view is open.
♻️ Proposed fix
function tick(): void {
if (!active) {
frameId = 0;
return;
}
updateTargets();
const moving = animateNodes();
- if (dirty || moving) {
+ const needsNextFrame = dirty || moving;
+ if (needsNextFrame) {
renderGraph();
dirty = false;
+ frameId = window.requestAnimationFrame(tick);
+ return;
}
- frameId = window.requestAnimationFrame(tick);
+ frameId = 0;
}
function renderLoop(): void {
- if (frameId) return;
+ if (!active || frameId) return;
frameId = window.requestAnimationFrame(tick);
}Also applies to: 1126-1129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@report-ui/graphView.ts` around lines 1109 - 1124, The tick() loop currently
always calls requestAnimationFrame, so update it to stop scheduling frames when
the graph is idle: inside tick() (the function named tick) after updateTargets()
and const moving = animateNodes(), only call frameId =
window.requestAnimationFrame(tick) if active && (dirty || moving) (and set
frameId = 0 otherwise); ensure frameId is cleared when exiting (when !active)
and apply the same conditional scheduling change to the other identical RAF call
block referenced around lines 1126–1129 so the RAF loop stops when nothing is
dirty or moving.
Comment on lines
+2103
to
+2131
| function setActiveView(view: "list" | "graph"): void { | ||
| if (!controls.listViewPanel || !controls.graphViewPanel) { | ||
| console.warn("Dependency Radar: view panels are missing from the report DOM."); | ||
| return; | ||
| } | ||
| currentView = view; | ||
| const isList = view === "list"; | ||
| controls.listViewPanel.classList.toggle("active", isList); | ||
| controls.graphViewPanel.classList.toggle("active", !isList); | ||
| controls.listViewPanel.setAttribute("aria-hidden", String(!isList)); | ||
| controls.graphViewPanel.setAttribute("aria-hidden", String(isList)); | ||
| if (controls.viewGraphButton) { | ||
| controls.viewGraphButton.style.display = isList ? "" : "none"; | ||
| } | ||
| if (controls.graphBackButton) { | ||
| controls.graphBackButton.style.display = isList ? "none" : ""; | ||
| } | ||
| controls.reportFooter?.classList.toggle("hidden", !isList); | ||
| document.body.classList.toggle("graph-mode", !isList); | ||
| if (isList) { | ||
| graphView?.setActive(false); | ||
| return; | ||
| } | ||
| if (!graphInitialized) { | ||
| if (!hasGraphDomNodes()) { | ||
| console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled."); | ||
| return; | ||
| } | ||
| graphView = initGraphView({ |
Contributor
There was a problem hiding this comment.
Validate graph DOM prerequisites before switching the UI into graph mode.
Right now the view toggles to graph first, then bails out if required graph nodes are missing. That leaves list hidden with no usable graph.
♻️ Proposed fix
function setActiveView(view: "list" | "graph"): void {
if (!controls.listViewPanel || !controls.graphViewPanel) {
console.warn("Dependency Radar: view panels are missing from the report DOM.");
return;
}
+ if (view === "graph" && !graphInitialized && !hasGraphDomNodes()) {
+ console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled.");
+ return;
+ }
currentView = view;
const isList = view === "list";
controls.listViewPanel.classList.toggle("active", isList);
controls.graphViewPanel.classList.toggle("active", !isList);
@@
- if (!graphInitialized) {
- if (!hasGraphDomNodes()) {
- console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled.");
- return;
- }
+ if (!graphInitialized) {
graphView = initGraphView({📝 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
| function setActiveView(view: "list" | "graph"): void { | |
| if (!controls.listViewPanel || !controls.graphViewPanel) { | |
| console.warn("Dependency Radar: view panels are missing from the report DOM."); | |
| return; | |
| } | |
| currentView = view; | |
| const isList = view === "list"; | |
| controls.listViewPanel.classList.toggle("active", isList); | |
| controls.graphViewPanel.classList.toggle("active", !isList); | |
| controls.listViewPanel.setAttribute("aria-hidden", String(!isList)); | |
| controls.graphViewPanel.setAttribute("aria-hidden", String(isList)); | |
| if (controls.viewGraphButton) { | |
| controls.viewGraphButton.style.display = isList ? "" : "none"; | |
| } | |
| if (controls.graphBackButton) { | |
| controls.graphBackButton.style.display = isList ? "none" : ""; | |
| } | |
| controls.reportFooter?.classList.toggle("hidden", !isList); | |
| document.body.classList.toggle("graph-mode", !isList); | |
| if (isList) { | |
| graphView?.setActive(false); | |
| return; | |
| } | |
| if (!graphInitialized) { | |
| if (!hasGraphDomNodes()) { | |
| console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled."); | |
| return; | |
| } | |
| graphView = initGraphView({ | |
| function setActiveView(view: "list" | "graph"): void { | |
| if (!controls.listViewPanel || !controls.graphViewPanel) { | |
| console.warn("Dependency Radar: view panels are missing from the report DOM."); | |
| return; | |
| } | |
| if (view === "graph" && !graphInitialized && !hasGraphDomNodes()) { | |
| console.warn("Dependency Radar: graph view DOM nodes are missing; graph view disabled."); | |
| return; | |
| } | |
| currentView = view; | |
| const isList = view === "list"; | |
| controls.listViewPanel.classList.toggle("active", isList); | |
| controls.graphViewPanel.classList.toggle("active", !isList); | |
| controls.listViewPanel.setAttribute("aria-hidden", String(!isList)); | |
| controls.graphViewPanel.setAttribute("aria-hidden", String(isList)); | |
| if (controls.viewGraphButton) { | |
| controls.viewGraphButton.style.display = isList ? "" : "none"; | |
| } | |
| if (controls.graphBackButton) { | |
| controls.graphBackButton.style.display = isList ? "none" : ""; | |
| } | |
| controls.reportFooter?.classList.toggle("hidden", !isList); | |
| document.body.classList.toggle("graph-mode", !isList); | |
| if (isList) { | |
| graphView?.setActive(false); | |
| return; | |
| } | |
| if (!graphInitialized) { | |
| graphView = initGraphView({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@report-ui/main.ts` around lines 2103 - 2131, In setActiveView, avoid toggling
the UI into graph mode before confirming graph DOM prerequisites; when view ===
"graph" first verify controls.listViewPanel && controls.graphViewPanel and call
hasGraphDomNodes() (and return early if missing) and only then update
currentView, toggle classes/aria, footer, body class and initialize graphView
via initGraphView if !graphInitialized; ensure graphView?.setActive(false) is
only called when switching back to list and graphView?.setActive(true) is
invoked after successful graph initialization — reference setActiveView,
controls.listViewPanel, controls.graphViewPanel, hasGraphDomNodes(),
graphInitialized, initGraphView, and graphView.
This was referenced Mar 2, 2026
Merged
This was referenced Mar 17, 2026
This was referenced Apr 27, 2026
Merged
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
Style