-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(ui): add new findings table #9699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
✅ All necessary |
|
✅ Conflict Markers Resolved All conflict markers have been successfully resolved in this pull request. |
🔒 Container Security ScanImage: ✅ No Vulnerabilities DetectedThe container image passed all security checks. No known CVEs were found.📋 Resources:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
solid work! Clean component architecture, proper type safety with as const, and the ExpandableSection using CSS grid for animations is the right approach. 👍
🟡 Request: Add TODO with ticket reference for commented code
In ui/components/findings/table/column-findings.tsx, the commented out ImpactedResourcesCell code (lines 21 and 265-277) should have a ticket reference so it doesn't become mystery code later.
Please update to:
// TODO: PROWLER-379 - Enable ImpactedResourcesCell when backend supports grouped findings
// import { ImpactedResourcesCell } from "./impacted-resources-cell";And same for the column definition block.
🟢 Minor: Dead export
ImpactedResourcesCell is exported in index.ts but never used. Not blocking, just noting it adds to bundle size. Can clean up now or leave for when the feature is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the previous two requests 👍
🔒 Container Security ScanImage: 📊 Vulnerability Summary
10 package(s) affected
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9699 +/- ##
==========================================
- Coverage 92.70% 92.51% -0.19%
==========================================
Files 129 163 +34
Lines 3153 23285 +20132
==========================================
+ Hits 2923 21543 +18620
- Misses 230 1742 +1512
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
demo.mov
Context
Redesign of the Findings table UI with new design system components, improved filtering UX, and enhanced table interactions.
Description
Steps to review
/findingsand verify:pnpm run typecheck- should passChecklist
UI
API
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.