feat: Graph Filtering and Node Controls#226
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request adds filtering and search capabilities to the graph view component along with task lifecycle management. It introduces a new filtering hook ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SearchUI as Search/Filter UI
participant GraphCanvas
participant useGraphFilter as useGraphFilter Hook
participant GraphNodes as Graph Node Generation
participant GraphViz as Visual Render
User->>SearchUI: Input search query or filter
SearchUI->>GraphCanvas: onSearchQueryChange / filterState update
GraphCanvas->>useGraphFilter: features, filterState, runningAutoTasks
useGraphFilter->>useGraphFilter: Compute matched nodes<br/>Traverse ancestors/descendants<br/>Apply category/status filters
useGraphFilter-->>GraphCanvas: GraphFilterResult<br/>(matchedNodeIds, highlightedNodeIds,<br/>highlightedEdgeIds, hasActiveFilter)
GraphCanvas->>GraphNodes: filterResult, actionCallbacks
GraphNodes->>GraphNodes: Build nodes/edges with<br/>isMatched/isHighlighted/isDimmed
GraphNodes-->>GraphCanvas: Enhanced TaskNodes<br/>& DependencyEdges
GraphCanvas->>GraphViz: Render with highlighting classes<br/>& action handlers
GraphViz-->>User: Visual feedback:<br/>matched glow, dimmed grayed,<br/>highlighted path emphasis
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @JBotwina, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the graph visualization component by introducing robust filtering capabilities and direct action controls for individual task nodes. Users can now easily narrow down their view of the graph using category, status, and search filters, and choose to either highlight or dim matching elements. Additionally, task nodes are more interactive with new options to manage their lifecycle directly from the graph. These changes aim to improve user experience by providing better control and clarity when navigating complex dependency graphs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
JBotwina
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive filtering and control system for the graph view, which is a great enhancement. The implementation of filtering logic, node actions, and associated UI components is well-structured. However, I've found a critical issue in package.json that will break dependency installation, and a couple of medium-severity issues related to code clarity and a potential bug in the UI. Addressing these will improve the robustness and maintainability of the new features.
apps/ui/src/components/views/graph-view/components/graph-filter-controls.tsx
Outdated
Show resolved
Hide resolved
…ve onViewBranch - Added onViewDetails callback to handle feature detail viewing. - Removed onViewBranch functionality and associated UI elements for a cleaner interface.
JBotwina
force-pushed
the
graph-filtering-and-node-controls
branch
from
6f3c05c to
cb07206
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/ui/src/components/views/graph-view/components/graph-filter-controls.tsx (1)
263-295: Add explicit button type attribute.The button element should have an explicit
type="button"attribute to prevent potential issues if this component is ever rendered within a form context. While not critical in the current usage, it's a best practice for semantic clarity.🔎 Suggested fix
<button + type="button" onClick={() => onNegativeFilterChange(!isNegativeFilter)} aria-label={apps/ui/src/components/views/graph-view/graph-view.tsx (1)
49-49: Type cast is reasonable but consider narrowing the Feature type instead.The cast
f.branchName as string | undefinedsuggestsbranchNamemay have a broader type in theFeatureinterface. IfFeature.branchNameis actually typed asstring | undefined | null, consider updating the interface to match expected runtime values, or add a null check here for safety.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
apps/ui/package.jsonapps/ui/src/components/views/board-view.tsxapps/ui/src/components/views/graph-view/components/dependency-edge.tsxapps/ui/src/components/views/graph-view/components/graph-controls.tsxapps/ui/src/components/views/graph-view/components/graph-filter-controls.tsxapps/ui/src/components/views/graph-view/components/graph-legend.tsxapps/ui/src/components/views/graph-view/components/index.tsapps/ui/src/components/views/graph-view/components/task-node.tsxapps/ui/src/components/views/graph-view/graph-canvas.tsxapps/ui/src/components/views/graph-view/graph-view.tsxapps/ui/src/components/views/graph-view/hooks/index.tsapps/ui/src/components/views/graph-view/hooks/use-graph-filter.tsapps/ui/src/components/views/graph-view/hooks/use-graph-nodes.tsapps/ui/src/components/views/graph-view/index.tsapps/ui/src/styles/global.css
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/components/views/graph-view/components/graph-filter-controls.tsx (4)
apps/ui/src/components/views/graph-view/hooks/use-graph-filter.ts (3)
StatusFilterValue(20-20)GraphFilterState(4-9)STATUS_FILTER_OPTIONS(12-18)apps/ui/src/components/views/graph-view/hooks/index.ts (1)
GraphFilterState(9-9)apps/ui/src/components/views/graph-view/components/index.ts (1)
GraphFilterControls(5-5)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/graph-view/graph-view.tsx (3)
apps/ui/src/store/app-store.ts (1)
Feature(256-272)apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts (1)
NodeActionCallbacks(37-43)apps/ui/src/components/views/graph-view/graph-canvas.tsx (1)
GraphCanvas(237-243)
🔇 Additional comments (27)
apps/ui/src/components/views/graph-view/components/graph-controls.tsx (2)
34-34: LGTM! Good styling consistency.Adding
text-popover-foregroundensures proper text color contrast within the popover container, aligning with the design system.
123-129: LGTM! Clean simplification of the lock toggle.The consolidation of the className using
cn(), inline ternary for the icon, and inline tooltip text improve code consistency and readability while preserving behavior.apps/ui/package.json (1)
81-81: No action required. Version 3.1.1 is the latest version, and no direct vulnerabilities have been found for this package in Snyk's vulnerability database. The dependency is well-established with 1,998,347 weekly downloads and is scored as a key ecosystem project.apps/ui/src/components/views/graph-view/index.ts (1)
4-9: LGTM!The multi-line export formatting improves readability with no functional changes.
apps/ui/src/styles/global.css (1)
1052-1117: LGTM!The new graph filter highlight states are well-structured with proper animation keyframes, hover states, and accessibility support through
prefers-reduced-motion. The CSS classes align with the component usage in the filtering feature.apps/ui/src/components/views/graph-view/components/graph-legend.tsx (2)
2-2: LGTM!The consolidated import improves code conciseness with no behavioral changes.
47-47: LGTM!The added
text-popover-foregroundclass ensures consistent text color theming for the legend container.apps/ui/src/components/views/graph-view/components/index.ts (1)
5-5: LGTM!The new export properly exposes the GraphFilterControls component as part of the graph-view public API.
apps/ui/src/components/views/graph-view/hooks/index.ts (1)
1-9: LGTM!The expanded exports properly expose the new filtering hook (
useGraphFilter) and node action callbacks (NodeActionCallbacks) needed for the graph filtering and task control features.apps/ui/src/components/views/board-view.tsx (1)
1039-1045: LGTM!The new props properly integrate search state and task lifecycle controls into GraphView, maintaining consistency with the existing KanbanBoard implementation. The callback mappings to existing action handlers are correct.
apps/ui/src/components/views/graph-view/components/graph-filter-controls.tsx (2)
63-93: LGTM!The toggle handlers correctly manage multi-select state for both categories and statuses. The select-all logic properly toggles between empty and fully selected states.
95-108: LGTM!The label generation logic provides clear, user-friendly button text that adapts based on selection state (none, one, or multiple items).
apps/ui/src/components/views/graph-view/components/dependency-edge.tsx (2)
10-11: LGTM!The new optional props extend the edge data interface to support filter-driven highlighting and dimming states.
57-104: LGTM!The highlighting and dimming logic is well-implemented with proper priority handling:
- Color prioritizes highlighted state over status-based colors
- Stroke width scales appropriately (highlighted > selected > default > dimmed)
- Visual effects (shadows, opacity) reinforce the state hierarchy
- CSS classes align with the global styles for consistent theming
apps/ui/src/components/views/graph-view/graph-view.tsx (1)
77-112: LGTM! Well-structured action callback memoization.The
nodeActionCallbacksobject is correctly memoized with all dependencies included. The pattern of looking up features by ID and calling the appropriate handler is clean and maintains a proper separation between ID-based callbacks (used by nodes) and Feature-based callbacks (used by parent components).apps/ui/src/components/views/graph-view/hooks/use-graph-filter.ts (2)
118-121: Consider:isNegativeFilteralone triggershasActiveFilterbut results in matching all nodes.When only
isNegativeFilteris true (no search, category, or status filters),hasActiveFilterbecomes true, butmatchedNodeIdswill be empty, so the negative inversion at lines 169-175 will select all features. This may be the intended behavior (show everything dimmed except... nothing), but it could be confusing for users. Consider whetherisNegativeFiltershould only contribute tohasActiveFilterwhen combined with other filters.
30-67: Well-implemented graph traversal helpers.The
getAncestorsandgetDescendantsfunctions correctly handle cycles via thevisitedset, preventing infinite recursion in graphs with circular dependencies. The algorithms are clear and efficient.apps/ui/src/components/views/graph-view/components/task-node.tsx (3)
213-232: Previous issue resolved: View Logs and View Details now call correct handlers.The past review flagged that both menu items were calling
onViewLogs. This has been fixed - "View Agent Logs" correctly callsdata.onViewLogs?.()(line 217) and "View Details" correctly callsdata.onViewDetails?.()(line 227).
81-87: LGTM! Clean state derivation with sensible defaults.Filter highlight states use nullish coalescing for safe defaults, and the
isStoppedcomputation correctly identifies paused tasks (in_progress but not actively running).
296-304: Good UX addition for paused task indicator.The visual feedback for paused tasks (progress bar at 50% + "Paused" label) clearly differentiates from running tasks. The warning color styling is consistent with the paused indicator in the header.
apps/ui/src/components/views/graph-view/graph-canvas.tsx (3)
73-90: LGTM! Well-structured filter state management with debouncing.The filter state is cleanly organized with local state for categories/statuses/negative mode and a debounced search query (200ms) for performance. The
filterStateobject correctly aggregates all filter criteria for theuseGraphFilterhook.
214-230: Good empty state UX with clear call-to-action.The empty state panel provides helpful feedback when filters yield no results, with a clear "Clear Filters" button to help users recover. The styling is consistent with the rest of the UI.
202-210: GraphFilterControls does not contain a search input and does not need to receive searchQuery.The
GraphFilterControlscomponent is responsible only for category filtering, status filtering, and the negative filter toggle. It does not display or manage a search input field. ThesearchQueryis handled separately at theGraphCanvaslevel, where it is debounced and included infilterStatefor use by theuseGraphFilterhook. This is the correct architecture—debouncing is applied before filter calculations, and no immediate UI feedback is lost since the search input that exists elsewhere (controlled externally) can still display user input in real time.apps/ui/src/components/views/graph-view/hooks/use-graph-nodes.ts (4)
100-114: Non-null assertions are safe here due to conditional checks.The pattern
actionCallbacks?.onViewLogs ? () => actionCallbacks.onViewLogs!(feature.id) : undefinedis correct - the non-null assertion is guarded by the truthiness check in the ternary condition.
56-61: VerifyactionCallbacksis memoized in parent to prevent unnecessary recalculations.The
useMemodependency onactionCallbacks(line 153) will cause recalculation if a new object reference is passed on each render. Confirm thatGraphViewmemoizesnodeActionCallbacks(which it does at lines 78-112 in graph-view.tsx). This is correctly implemented.Also applies to: 153-153
30-35: LGTM! DependencyEdge type correctly extended for filter states.The edge data type now includes
isHighlightedandisDimmedoptional booleans, aligning with the node highlight system.
127-127: Edge ID format consistency is critical - verified to match.The edge ID format
${depId}->${feature.id}matches the format used inuse-graph-filter.ts(line 82), ensuring edge highlighting works correctly.
2 participants
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.