Repository navigation
Update report UI and embed refreshed report assets - #28
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a focus-aware graph layout and smooth viewport animation; converts the CTA into a structured card; restructures filter and metadata UI into accessible dropdown/popover panels with active-filter chips; and extends AggregatedData.environment with package manager and tool-version fields. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphView as Graph View
participant Layout as Layout Engine
participant Viewport as Viewport
participant Render as Render Loop
User->>GraphView: focus on node (click)
GraphView->>Layout: collectDirectedDistances (BFS)
Note over Layout: compute directed parent/child distances
Layout-->>GraphView: distance map
GraphView->>Layout: buildFocusLayoutTargets (assign columns/rows)
Layout-->>GraphView: node target coordinates
GraphView->>Viewport: focusViewportOn(target center)
Viewport->>Render: setViewportTarget / animateViewport (easing, reduced-motion aware)
Render->>Render: update camera and node interpolation each tick
Render->>GraphView: apply node positions & drawFocusedEdge for highlights
Render-->>User: animated focused layout visible
sequenceDiagram
participant User
participant UI as Filter/Metadata UI
participant Logic as Filter Logic
participant DataView as List/Graph Renderer
User->>UI: open Filters or Metadata
UI->>UI: set aria-hidden/inert, open popover
User->>UI: change filter option
UI->>Logic: update predicates (workspace/vuln)
Logic->>DataView: apply filters and update view
DataView-->>User: refreshed list/graph
User->>UI: click outside or press Escape
UI->>UI: close popover and clear pending viewport targets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 19 minutes and 12 seconds. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
report-ui/style.css (1)
773-787:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
displayproperty causes unintended visibility.The
display: noneon line 775 is immediately overridden bydisplay: flexon line 783, making the element always visible. Based on the mobile media query (line 2564-2566) also settingdisplay: none, the intent appears to be hiding this toggle by default.🐛 Proposed fix
.license-filter-toggle { display: none; padding: 6px 12px; border: 1px solid var(--border-color); border-radius: var(--radius); background: transparent; color: var(--text-secondary); font-size: 12px; cursor: pointer; - display: flex; align-items: center; gap: 6px; transition: all var(--transition); }If the element should be flex when visible, consider using a class toggle or separate selector for the visible state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/style.css` around lines 773 - 787, The .license-filter-toggle rule currently declares display: none then immediately display: flex, causing it to always be visible; remove the duplicate display property so the default state is hidden (keep display: none) and move display: flex into a separate visible state selector or class (e.g., .license-filter-toggle--visible or a media/query-specific selector) so the element is only flex when explicitly shown; update any scripts or markup that toggle visibility to add/remove that visible class (refer to .license-filter-toggle and the mobile media query that also sets display: none).
🤖 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/main.ts`:
- Around line 2197-2212: The list view dropdown and counting logic fails to
treat missing usage.origins.workspaces as ["root"], causing workspace
counts/filters to mismatch the graph; update the code that builds options (the
block using controls.workspace, workspaceNames, formatCount, and countBy) to
normalize each dependency's usage.origins.workspaces to ["root"] when undefined
or empty before counting/filtering (same normalization used in graphView.ts),
and ensure the same normalization is applied in the filtering logic that runs
when a workspace option is selected so "Workspace root" includes deps that omit
the field.
- Around line 2372-2478: getActiveFilterChips() currently omits the search input
so the UI shows zero filters when only a search is active; update
getActiveFilterChips to add an ActiveFilterChip for controls.search when
controls.search.value is non-empty (e.g., id "search", label "Search: " +
selectedOptionLabel or controls.search.value, and remove handler that clears
controls.search.value = ""), and ensure the remove handler triggers the same
update path as other controls (either call syncActiveFilterUi() or dispatch the
input/change event used elsewhere) so clearing the chip actually removes the
filter; adjust any clear-all handlers if they rely on chip-count to also clear
controls.search.
---
Outside diff comments:
In `@report-ui/style.css`:
- Around line 773-787: The .license-filter-toggle rule currently declares
display: none then immediately display: flex, causing it to always be visible;
remove the duplicate display property so the default state is hidden (keep
display: none) and move display: flex into a separate visible state selector or
class (e.g., .license-filter-toggle--visible or a media/query-specific selector)
so the element is only flex when explicitly shown; update any scripts or markup
that toggle visibility to add/remove that visible class (refer to
.license-filter-toggle and the mobile media query that also sets display: none).
🪄 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: 1d0b9f9f-15da-4dca-9ec9-b994016d15ae
⛔ Files ignored due to path filters (2)
report-ui/dist/report.cssis excluded by!**/dist/**report-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (7)
report-ui/graphView.tsreport-ui/index.htmlreport-ui/main.tsreport-ui/style.cssreport-ui/types.tssrc/report-assets.tssrc/report.ts
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)
report-ui/main.ts (1)
2744-2767:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't treat sort changes as filter resets.
controls.sortalready has its own change handler, so including it infilterControlsmakes mobile sort changes also callhandleFilterControlChange(), clearforcedVisibleDepKeys, and hide dependencies surfaced from graph/root-link navigation. Desktop column-header sorting does not do that, so the two sort paths diverge.Suggested fix
const filterControls = [ controls.search, controls.direct, controls.runtime, - controls.sort, controls.hasVulns, controls.workspace, controls.licensePermissive,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@report-ui/main.ts` around lines 2744 - 2767, The sort control (controls.sort) is incorrectly included in filterControls causing its mobile change handler to call handleFilterControlChange which clears forcedVisibleDepKeys and calls renderList; remove controls.sort from the filterControls array so that sort changes use their dedicated change handler only, leaving handleFilterControlChange, forcedVisibleDepKeys, and renderList behavior reserved for actual filter inputs.
🤖 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/index.html`:
- Around line 279-296: The search input with id "search" inside the
"search-wrapper" lacks an accessible label; update the markup to provide an
explicit label (either a visible <label for="search">Packages</label> or an
sr-only label) or add an aria-label="Search packages" to the input, and apply
the same change in the embedded report code referencing the same "search" input
in src/report.ts so both the index.html's input and the programmatic input have
a persistent accessible name for screen readers.
---
Outside diff comments:
In `@report-ui/main.ts`:
- Around line 2744-2767: The sort control (controls.sort) is incorrectly
included in filterControls causing its mobile change handler to call
handleFilterControlChange which clears forcedVisibleDepKeys and calls
renderList; remove controls.sort from the filterControls array so that sort
changes use their dedicated change handler only, leaving
handleFilterControlChange, forcedVisibleDepKeys, and renderList behavior
reserved for actual filter inputs.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 538c2395-adb3-4742-8932-7a2147f9e78c
⛔ 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 (5)
report-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/style.css
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JosephMaynard. The following files were modified: * `report-ui/graphView.ts` * `report-ui/main.ts` * `src/report.ts` These file types are not supported: * `report-ui/index.html` * `report-ui/style.css`
This was referenced Apr 30, 2026
Merged
Merged
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
src/report.tsTesting
Summary by CodeRabbit
New Features
UI Updates
Data
Accessibility