feat/playbooks panel #2774
feat/playbooks panel #2774
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new PlaybooksPanel and PanelWrapper, refactors multiple existing panels to use PanelWrapper, updates View to render "playbooks" panels, extends panel types with "playbooks", and adds a cross-browser hoverScrollbar CSS class. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PlaybooksPanel
participant API as useGetPlaybookSpecsDetails
participant Modal as SubmitPlaybookRunForm
User->>PlaybooksPanel: View playbook rows
User->>PlaybooksPanel: Click "Run" on a row
PlaybooksPanel->>PlaybooksPanel: set selectedRow, set loading
PlaybooksPanel->>API: fetch playbook specs (component_id, config_id, check_id)
API-->>PlaybooksPanel: return specs
PlaybooksPanel->>Modal: open with prefilled ids and params
User->>Modal: submit or close
Modal-->>PlaybooksPanel: close
PlaybooksPanel->>PlaybooksPanel: clear selectedRow, clear loading
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
adityathebe
force-pushed
the
feat/playbooks-panel
branch
from
d83eb82 to
15863d3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/mixins.module.css (1)
22-32: Class name may be misleading.The class
hoverScrollbaralways hides the scrollbar rather than showing it on hover. If the intent was to reveal the scrollbar on hover, add a:hoverpseudo-class rule. If permanent hiding is intentional, consider renaming tohiddenScrollbarfor clarity.If hover-reveal behavior is desired:
.hoverScrollbar { overflow-y: auto; scrollbar-width: none; /* Firefox */ -ms-overflow-style: none; /* IE/Edge */ } .hoverScrollbar::-webkit-scrollbar { display: none; /* Safari/Chrome */ width: 0; height: 0; } + +.hoverScrollbar:hover { + scrollbar-width: thin; /* Firefox */ + -ms-overflow-style: auto; /* IE/Edge */ +} + +.hoverScrollbar:hover::-webkit-scrollbar { + display: block; + width: 8px; +} + +.hoverScrollbar:hover::-webkit-scrollbar-thumb { + background: rgba(160, 160, 160, 0.7); + border-radius: 4px; +}src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (1)
66-74: Missing dependency in useEffect.
checkScrollPositionis referenced inside useEffect but not included in the dependency array. While this works because the function only uses the stable ref, ESLint's exhaustive-deps rule would flag this. Consider usinguseCallbackor inlining the logic.+import { useMemo, useState, useRef, useEffect, useCallback } from "react"; + - // Check scroll position to show/hide indicator - const checkScrollPosition = () => { + // Check scroll position to show/hide indicator + const checkScrollPosition = useCallback(() => { const container = scrollContainerRef.current; if (!container) return; const { scrollTop, scrollHeight, clientHeight } = container; const isScrollable = scrollHeight > clientHeight; const isAtBottom = scrollHeight - scrollTop - clientHeight < 10; setShowScrollIndicator(isScrollable && !isAtBottom); - }; + }, []); // Set up scroll listener and check initial state useEffect(() => { const container = scrollContainerRef.current; if (!container) return; checkScrollPosition(); container.addEventListener("scroll", checkScrollPosition); return () => container.removeEventListener("scroll", checkScrollPosition); - }, [rows]); + }, [rows, checkScrollPosition]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/pages/audit-report/components/View/View.tsx(3 hunks)src/pages/audit-report/components/View/panels/BarGaugePanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/DurationPanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/NumberPanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/PanelWrapper.tsx(1 hunks)src/pages/audit-report/components/View/panels/PieChartPanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx(1 hunks)src/pages/audit-report/components/View/panels/TablePanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/TextPanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/index.ts(1 hunks)src/pages/audit-report/types/index.ts(1 hunks)src/utils/mixins.module.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/audit-report/components/View/panels/TablePanel.tsx (1)
src/pages/audit-report/types/index.ts (1)
PanelResult(478-496)
src/pages/audit-report/components/View/View.tsx (1)
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (1)
PlaybooksPanel(33-172)
🔇 Additional comments (12)
src/pages/audit-report/components/View/panels/PanelWrapper.tsx (1)
3-36: PanelWrapper abstraction looks consistent and reusableProps cover current usage (title/description plus overrideable class names), and the container styling provides a sensible, uniform panel shell for the other components.
src/pages/audit-report/components/View/panels/TextPanel.tsx (1)
3-25: TextPanel correctly delegates header/description to PanelWrapperEach row is now wrapped in PanelWrapper with consistent title/description handling, while the value rendering and keys remain unchanged, so behavior should be preserved.
src/pages/audit-report/components/View/panels/TablePanel.tsx (1)
11-35: TablePanel now uses PanelWrapper without affecting table renderingWrapping the table body with PanelWrapper centralizes the header/description styling while leaving row iteration and cell content unchanged.
src/pages/audit-report/components/View/panels/NumberPanel.tsx (1)
4-41: NumberPanel’s PanelWrapper usage preserves existing value behaviorPer-row PanelWrapper titles correctly use
label || summary.name, and the numeric formatting path is unchanged, so this is a safe structural improvement.src/pages/audit-report/components/View/panels/BarGaugePanel.tsx (1)
4-126: BarGaugePanel wrapping with PanelWrapper is purely structuralThe gauge configuration and rendering logic are untouched; using PanelWrapper standardizes the header/description styling around the existing bar gauge UI.
src/pages/audit-report/components/View/panels/DurationPanel.tsx (1)
4-38: DurationPanel cleanly migrated to per-row PanelWrapperEach duration row now lives inside a PanelWrapper with appropriate title/description, while the nanoseconds → milliseconds conversion and formatDuration call remain as before.
src/pages/audit-report/components/View/panels/PieChartPanel.tsx (1)
12-130: PieChartPanel’s adoption of PanelWrapper keeps chart logic intactOnly the outer container is swapped for PanelWrapper with a min-height; the pie data generation and Recharts setup are unchanged, so functionality should be preserved with more consistent panel styling.
src/pages/audit-report/types/index.ts (1)
478-495: PanelResult type extension to include"properties"and"playbooks"looks correctExtending the
typeunion ensures the compiler is aware of these panel variants while keeping the rest of the shape unchanged.src/pages/audit-report/components/View/panels/index.ts (1)
9-9: LGTM!The export follows the established pattern for panel barrel exports.
src/pages/audit-report/components/View/View.tsx (2)
28-30: LGTM!The import addition follows the existing pattern for panel imports.
431-432: LGTM!The new case follows the established pattern for rendering panel types.
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (1)
76-157: LGTM on the UI implementation.The component structure is well-organized with proper empty state handling, accessible button states during loading, and a thoughtful scroll indicator UX pattern.
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx
Outdated
Show resolved
Hide resolved
adityathebe
force-pushed
the
feat/playbooks-panel
branch
from
15863d3 to
11104fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (1)
49-54: Consider adding error handling for failed playbook fetches.The
useGetPlaybookSpecsDetailshook can return anerrorstate, but it's not currently destructured or handled. If the fetch fails, the button simply returns to the "Run" state with no user feedback, which could be confusing.Consider destructuring the error and showing feedback:
- const { data: playbookSpec, isLoading } = useGetPlaybookSpecsDetails( + const { data: playbookSpec, isLoading, error } = useGetPlaybookSpecsDetails( selected?.id ?? "", { enabled: !!selected?.id } );Then display an error state or toast notification when
erroris truthy, so users understand why the modal didn't open.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/pages/audit-report/components/View/View.tsx(3 hunks)src/pages/audit-report/components/View/panels/BarGaugePanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/DurationPanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/NumberPanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/PanelWrapper.tsx(1 hunks)src/pages/audit-report/components/View/panels/PieChartPanel.tsx(3 hunks)src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx(1 hunks)src/pages/audit-report/components/View/panels/TablePanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/TextPanel.tsx(2 hunks)src/pages/audit-report/components/View/panels/index.ts(1 hunks)src/pages/audit-report/types/index.ts(1 hunks)src/utils/mixins.module.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/pages/audit-report/components/View/panels/BarGaugePanel.tsx
- src/utils/mixins.module.css
- src/pages/audit-report/components/View/panels/TextPanel.tsx
- src/pages/audit-report/types/index.ts
- src/pages/audit-report/components/View/panels/NumberPanel.tsx
- src/pages/audit-report/components/View/panels/PanelWrapper.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/pages/audit-report/components/View/panels/TablePanel.tsx (1)
src/pages/audit-report/types/index.ts (1)
PanelResult(478-496)
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (3)
src/components/Playbooks/Runs/Submit/SubmitPlaybookRunForm.tsx (2)
SubmitPlaybookRunFormValues(25-32)SubmitPlaybookRunForm(45-161)src/pages/audit-report/types/index.ts (1)
PanelResult(478-496)src/api/query-hooks/playbooks.tsx (1)
useGetPlaybookSpecsDetails(96-111)
src/pages/audit-report/components/View/View.tsx (1)
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (1)
PlaybooksPanel(33-179)
🔇 Additional comments (7)
src/pages/audit-report/components/View/panels/TablePanel.tsx (1)
3-3: LGTM! Clean refactor to PanelWrapper.The refactoring to use
PanelWrapperstandardizes the panel layout and is consistent with other panels in the PR.Also applies to: 11-15, 34-34
src/pages/audit-report/components/View/panels/DurationPanel.tsx (1)
4-4: LGTM! Consistent with the PanelWrapper refactor pattern.The refactor aligns with the broader PR changes to standardize panel layout using
PanelWrapper.Also applies to: 26-31, 37-37
src/pages/audit-report/components/View/panels/index.ts (1)
9-9: LGTM! Export completes the public API surface.The export makes
PlaybooksPanelavailable for use in the view rendering flow.src/pages/audit-report/components/View/panels/PieChartPanel.tsx (1)
12-12: LGTM! PanelWrapper refactor with appropriate styling.The addition of
min-h-[300px]ensures the chart has adequate display space. Refactor is consistent with other panels.Also applies to: 102-106, 130-130
src/pages/audit-report/components/View/View.tsx (1)
28-29: LGTM! PlaybooksPanel integration is correct.The import and rendering case for
PlaybooksPanelproperly integrates the new panel type into the view rendering flow.Also applies to: 431-432
src/pages/audit-report/components/View/panels/PlaybooksPanel.tsx (2)
34-44: Excellent fix! Runtime validation now properly guards against invalid data.The type guard correctly validates that each row is an object with a non-empty
idstring before treating it as aPlaybookRunRow. This addresses the previous concern about unsafe casting.
166-166: Good fix for the stale data race condition.The added check
playbookSpec.id === selected.idensures the modal only renders when the fetched spec matches the currently selected row, preventing mismatched data from appearing if the user rapidly switches selections.
❌ Deploy Preview for clerk-saas-ui failed. Why did it fail? →
|
❌ Deploy Preview for flanksource-demo-stable failed. Why did it fail? →
|
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.