Repository navigation
Preserve pnpm resolved dependencies when peer ranges overlap - #22
Conversation
|
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:
📝 WalkthroughWalkthroughCentralizes hover/cursor state and boundary-aware hit-testing in report-ui graph view; prevents treating pnpm peerDependencies as resolved child refs when building lockfile nodes; adds a test ensuring pnpm-resolved deps are preserved when peer ranges declare the same package. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (pointer)
participant Canvas as Canvas DOM
participant Graph as GraphView
participant Renderer as Renderer/Popover
User->>Canvas: pointer move / down / up
Canvas->>Graph: updateHoverFromClientPosition(clientX, clientY)
Graph->>Graph: check canvas bounding rect -> findNode or null
Graph-->>Canvas: setCanvasClickableCursor(true/false)
Graph->>Renderer: show/hide popover or focus/clear node
Renderer-->>User: UI update (cursor / hover / popover)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JosephMaynard. The following files were modified: * `report-ui/graphView.ts` These files were kept as they were: * `src/runners/lockfileGraph.ts` These files were ignored: * `src/runners/npmLs.test.ts` These file types are not supported: * `report-ui/style.css`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
report-ui/graphView.ts (1)
1753-1753: Consider consolidating cursor/reset calls into one helper.
setCanvasClickableCursor(false)is repeated across multiple lifecycle handlers; a smallresetInteractionVisualState()helper would reduce drift risk in future edits.Also applies to: 1767-1767, 1831-1831, 1946-1946, 1994-1994
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` at line 1753, Create a small helper named resetInteractionVisualState() that encapsulates the repeated reset behavior (at minimum calling setCanvasClickableCursor(false) and any related cursor/reset calls), then replace the multiple inline setCanvasClickableCursor(false) occurrences with a single call to resetInteractionVisualState() from the lifecycle handlers where they appear; update references to use the new helper to avoid drift and keep visual-reset logic centralized.
🤖 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`:
- Line 1753: Create a small helper named resetInteractionVisualState() that
encapsulates the repeated reset behavior (at minimum calling
setCanvasClickableCursor(false) and any related cursor/reset calls), then
replace the multiple inline setCanvasClickableCursor(false) occurrences with a
single call to resetInteractionVisualState() from the lifecycle handlers where
they appear; update references to use the new helper to avoid drift and keep
visual-reset logic centralized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1801-1804: After a pan (when moved is true) you only update the
cursor via setCanvasClickableCursor(Boolean(findNode(event.clientX,
event.clientY))) which leaves hover/selection highlights stale; call the same
hover-update logic that your mousemove handler uses (or explicitly update hover
state at the current coordinates) after setting the cursor so nodes/edges under
the cursor are re-evaluated. Locate the mousemove handler or hover updater used
elsewhere (the function that computes hover from event.clientX/event.clientY)
and invoke it here (or call it indirectly by dispatching the same handler)
immediately after setCanvasClickableCursor to refresh hover highlights.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
report-ui/graphView.ts (1)
1814-1818:⚠️ Potential issue | 🟡 MinorClear hover when focus is dismissed on empty click/tap.
When focused,
updateHover(...)is intentionally skipped. In the empty-target paths,clearFocus()runs but hover can remain stale until the next move event. Explicitly clearing hover here avoids stale highlight.💡 Suggested patch
const node = updateHoverFromClientPosition(event.clientX, event.clientY); if (!node) { clearFocus(); hidePopover(); + updateHover(null); return; }if (!panState.moved && event.changedTouches.length === 1) { const node = findNode( event.changedTouches[0].clientX, event.changedTouches[0].clientY, ); if (!node) { clearFocus(); hidePopover(); + updateHover(null); } else { applyFocus(node.slug); showPopover(node.slug); } }Also applies to: 1935-1943
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 1814 - 1818, The empty-click path leaves a stale hover because updateHoverFromClientPosition(...) is skipped when focused; modify the branches where you do "const node = updateHoverFromClientPosition(event.clientX, event.clientY); if (!node) { clearFocus(); hidePopover(); return; }" to also call clearHover() (or the project's hover-clear helper) before returning so hover state is cleared; apply the same change for the other analogous block around the updateHover call (the similar branch later in the file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@report-ui/graphView.ts`:
- Around line 1814-1818: The empty-click path leaves a stale hover because
updateHoverFromClientPosition(...) is skipped when focused; modify the branches
where you do "const node = updateHoverFromClientPosition(event.clientX,
event.clientY); if (!node) { clearFocus(); hidePopover(); return; }" to also
call clearHover() (or the project's hover-clear helper) before returning so
hover state is cleared; apply the same change for the other analogous block
around the updateHover call (the similar branch later in the file).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
report-ui/graphView.ts (1)
1315-1319: Consider addingrequestRender()for standalone use.Unlike
updateHover(null)which triggers a render,clearHover()silently resets state without marking dirty or requesting a render. Currently this works because callers (handleWindowMouseUp,handleTouchEnd) immediately callclearFocus()which handles rendering. However, this creates a subtle coupling - ifclearHover()is called standalone in the future, the UI won't update.💡 Suggested improvement for robustness
function clearHover(): void { hoverSlug = null; hoverNodes = new Set(); hoverEdges = new Set(); + dirty = true; + requestRender(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/graphView.ts` around lines 1315 - 1319, The clearHover function currently resets hover state but does not trigger a render, creating a hidden coupling to callers; modify clearHover() to mark the view dirty and request a render by calling the same render path used in updateHover (e.g., invoke requestRender() or markDirty()/requestRender() as appropriate) so that it behaves standalone, and ensure this change remains consistent with clearFocus and updateHover(null) behavior to avoid duplicate renders.
🤖 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 1315-1319: The clearHover function currently resets hover state
but does not trigger a render, creating a hidden coupling to callers; modify
clearHover() to mark the view dirty and request a render by calling the same
render path used in updateHover (e.g., invoke requestRender() or
markDirty()/requestRender() as appropriate) so that it behaves standalone, and
ensure this change remains consistent with clearFocus and updateHover(null)
behavior to avoid duplicate renders.
1 participant
Summary
peerDependenciesranges when building the pnpm dependency graph so resolved nodes are retainedTesting
Summary by CodeRabbit
Bug Fixes
Improvements
Tests