feat(web): add group resource pickers and improve drag UX#736
Open
ksong008 wants to merge 1 commit intodaeuniverse:mainfrom
Open
feat(web): add group resource pickers and improve drag UX#736ksong008 wants to merge 1 commit intodaeuniverse:mainfrom
ksong008 wants to merge 1 commit intodaeuniverse:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR enhances drag-and-drop interactions for group resource management by isolating drag handles to dedicated wrapper elements, introducing an expand/collapse UI for groups, adding a reusable group resource picker modal, implementing automatic group expansion during drag operations with edge auto-scrolling, enabling group reordering, and refactoring group content rendering with custom collapse controls. Changes
Sequence DiagramssequenceDiagram
participant User
participant DragContext as DragDropContext
participant PointerListener as Pointer Listener<br/>(Auto-Scroll)
participant GroupCard as Group Card<br/>(Auto-Expand)
participant Store as Group State
User->>DragContext: Drag resource over group
DragContext->>PointerListener: onDragUpdate(pointerPosition)
PointerListener->>PointerListener: elementFromPoint(x, y)<br/>identify hovered group
PointerListener->>Store: setHoveredGroupId(groupId)
Store->>GroupCard: hoveredGroupId prop updated
GroupCard->>GroupCard: Detect hover state
GroupCard->>Store: setGroupExpanded(groupId, true)<br/>via onExpand callback
GroupCard->>User: Render expanded content
PointerListener->>User: Trigger edge auto-scroll<br/>if pointer near viewport edge
User->>DragContext: Drop resource on group
DragContext->>Store: Add resource to group<br/>via mutation
sequenceDiagram
participant User
participant Modal as GroupAddNodesModal
participant Dialog as SelectionDialog<br/>(Internal)
participant Filter as Filter Logic
participant API as onSubmit<br/>Mutation
User->>Modal: Open modal (opened=true)
Modal->>Dialog: Render with items list
Dialog->>Dialog: Reset selectedIds &<br/>query on open
User->>Dialog: Type in search field
Dialog->>Filter: Update query state
Filter->>Filter: Filter items by substring<br/>match (title/description/meta/keywords)
Dialog->>User: Render filtered results
User->>Dialog: Click item or press<br/>Enter/Space
Dialog->>Dialog: Toggle selection in<br/>selectedIds state
User->>Dialog: Click Submit button
Dialog->>Dialog: Validate selectedIds.length > 0
Dialog->>API: Call onSubmit(selectedIds)
API->>API: Execute async mutation
API-->>Dialog: Success
Dialog->>Dialog: Close modal &<br/>reset state
Dialog->>User: Render closed
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/components/SortableGroupContent.tsx (1)
188-193: Minor: Consider simplifying effectiveExpandedSections logic.The current logic handles
collapsedearly-return but the check!autoExpandValuecould short-circuit before checkingexpandedSections.includes(autoExpandValue).Current behavior is correct, but for clarity:
♻️ Optional simplification
const effectiveExpandedSections = useMemo(() => { - if (collapsed || !autoExpandValue || expandedSections.includes(autoExpandValue)) { + if (collapsed) { return expandedSections } + if (!autoExpandValue || expandedSections.includes(autoExpandValue)) { + return expandedSections + } return [...expandedSections, autoExpandValue] }, [autoExpandValue, collapsed, expandedSections])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/SortableGroupContent.tsx` around lines 188 - 193, The logic inside the useMemo for effectiveExpandedSections is correct but can be simplified for clarity: first short-circuit on collapsed by returning expandedSections, then check autoExpandValue presence and membership in expandedSections (i.e., if !autoExpandValue || expandedSections.includes(autoExpandValue) return expandedSections), otherwise return [...expandedSections, autoExpandValue]; update the effectiveExpandedSections useMemo (and its dependency array [autoExpandValue, collapsed, expandedSections]) to reflect this clearer conditional flow.apps/web/src/pages/Orchestrate/index.tsx (2)
194-216: Consider extracting magic numbers as named constants.The edge auto-scroll implementation uses several magic numbers (24, 176, 180, 320, 0.3) that would benefit from named constants for maintainability.
♻️ Optional: Extract constants
+const EDGE_SCROLL_BASE_SPEED = 24 +const EDGE_SCROLL_MAX_ADDITIONAL_SPEED = 176 +const EDGE_SCROLL_MIN_THRESHOLD = 180 +const EDGE_SCROLL_MAX_THRESHOLD = 320 +const EDGE_SCROLL_THRESHOLD_RATIO = 0.3 const tickEdgeAutoScroll = useCallback(() => { autoScrollFrameRef.current = null if (!draggingActiveRef.current || !edgeAutoScrollEnabledRef.current || !dragPointerRef.current) return const viewportHeight = window.innerHeight - const threshold = Math.min(320, Math.max(180, Math.round(viewportHeight * 0.3))) + const threshold = Math.min(EDGE_SCROLL_MAX_THRESHOLD, Math.max(EDGE_SCROLL_MIN_THRESHOLD, Math.round(viewportHeight * EDGE_SCROLL_THRESHOLD_RATIO))) const pointerY = dragPointerRef.current.y let delta = 0 if (pointerY < threshold) { const intensity = Math.min(1, (threshold - pointerY) / threshold) - delta = -Math.round(24 + intensity * intensity * 176) + delta = -Math.round(EDGE_SCROLL_BASE_SPEED + intensity * intensity * EDGE_SCROLL_MAX_ADDITIONAL_SPEED) } else if (pointerY > viewportHeight - threshold) { const intensity = Math.min(1, (pointerY - (viewportHeight - threshold)) / threshold) - delta = Math.round(24 + intensity * intensity * 176) + delta = Math.round(EDGE_SCROLL_BASE_SPEED + intensity * intensity * EDGE_SCROLL_MAX_ADDITIONAL_SPEED) } // ... }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/Orchestrate/index.tsx` around lines 194 - 216, The tickEdgeAutoScroll function contains magic numbers; extract 24, 176, 180, 320, and 0.3 into named constants (e.g., BASE_SCROLL_SPEED = 24, MAX_ADDITIONAL_SCROLL = 176, MIN_THRESHOLD = 180, MAX_THRESHOLD = 320, THRESHOLD_RATIO = 0.3) defined near the hook or top of the file, then replace the literals inside tickEdgeAutoScroll with those constants (referencing function name tickEdgeAutoScroll, refs like dragPointerRef.current, edgeAutoScrollEnabledRef.current, and autoScrollFrameRef.current) to improve readability and maintainability while keeping existing behavior.
255-261: Use onlypointermoveto avoid potential double-handling.Both
mousemoveandpointermoveevents are registered. On devices that support both, this causes the handler to be called twice per movement. Thepointermoveevent is the modern unified API that handles both mouse and touch input across all major browsers, including Safari.♻️ Proposed simplification
- window.addEventListener('mousemove', handlePointerMove, { passive: true }) - window.addEventListener('pointermove', handlePointerMove, { passive: true }) + window.addEventListener('pointermove', handlePointerMove, { passive: true }) return () => { - window.removeEventListener('mousemove', handlePointerMove) - window.removeEventListener('pointermove', handlePointerMove) + window.removeEventListener('pointermove', handlePointerMove) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/Orchestrate/index.tsx` around lines 255 - 261, The event handler handlePointerMove is being attached to both 'mousemove' and 'pointermove' causing duplicate calls; change the registration to only window.addEventListener('pointermove', handlePointerMove, { passive: true }) and update the cleanup to only removeEventListener('pointermove', handlePointerMove), removing all 'mousemove' additions/removals so pointer events are the single source of pointer movement handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/SortableGroupContent.tsx`:
- Around line 188-193: The logic inside the useMemo for
effectiveExpandedSections is correct but can be simplified for clarity: first
short-circuit on collapsed by returning expandedSections, then check
autoExpandValue presence and membership in expandedSections (i.e., if
!autoExpandValue || expandedSections.includes(autoExpandValue) return
expandedSections), otherwise return [...expandedSections, autoExpandValue];
update the effectiveExpandedSections useMemo (and its dependency array
[autoExpandValue, collapsed, expandedSections]) to reflect this clearer
conditional flow.
In `@apps/web/src/pages/Orchestrate/index.tsx`:
- Around line 194-216: The tickEdgeAutoScroll function contains magic numbers;
extract 24, 176, 180, 320, and 0.3 into named constants (e.g., BASE_SCROLL_SPEED
= 24, MAX_ADDITIONAL_SCROLL = 176, MIN_THRESHOLD = 180, MAX_THRESHOLD = 320,
THRESHOLD_RATIO = 0.3) defined near the hook or top of the file, then replace
the literals inside tickEdgeAutoScroll with those constants (referencing
function name tickEdgeAutoScroll, refs like dragPointerRef.current,
edgeAutoScrollEnabledRef.current, and autoScrollFrameRef.current) to improve
readability and maintainability while keeping existing behavior.
- Around line 255-261: The event handler handlePointerMove is being attached to
both 'mousemove' and 'pointermove' causing duplicate calls; change the
registration to only window.addEventListener('pointermove', handlePointerMove, {
passive: true }) and update the cleanup to only
removeEventListener('pointermove', handlePointerMove), removing all 'mousemove'
additions/removals so pointer events are the single source of pointer movement
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa7e9ee6-ec15-460e-bb07-b343b0abcd86
📒 Files selected for processing (10)
apps/web/src/components/DraggableResourceBadge.tsxapps/web/src/components/DroppableGroupCard.tsxapps/web/src/components/GroupResourcePickerModal.tsxapps/web/src/components/SortableGroupContent.tsxapps/web/src/components/SortableResourceBadge.tsxapps/web/src/i18n/locales/en.jsonapps/web/src/i18n/locales/zh-Hans.jsonapps/web/src/pages/Orchestrate/Group.tsxapps/web/src/pages/Orchestrate/Subscription.tsxapps/web/src/pages/Orchestrate/index.tsx
There was a problem hiding this comment.
Pull request overview
Adds Orchestrate UX improvements centered on group management: group-level resource pickers, sortable group cards, and more forgiving drag-and-drop (including edge auto-scroll and better interactions with collapsed groups).
Changes:
- Introduce group resource picker modals for adding nodes and subscription groups.
- Make groups sortable and adjust group card layout/interaction (collapsed-by-default, summary line, drag handle).
- Improve drag UX in Orchestrate (hover-based fallback drop, destination tracking, edge auto-scroll), plus i18n updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/components/DraggableResourceBadge.tsx | Refines draggable badge UI with an explicit drag handle. |
| apps/web/src/components/DroppableGroupCard.tsx | Adds collapse/expand UI, summary slot, and group reorder drag handle support. |
| apps/web/src/components/GroupResourcePickerModal.tsx | New reusable modal for multi-select picking (nodes/subscription groups). |
| apps/web/src/components/SortableGroupContent.tsx | Reworks group content into compact droppable zones with “add” actions. |
| apps/web/src/components/SortableResourceBadge.tsx | Refines sortable badge styling and moves drag handle to a dedicated element. |
| apps/web/src/i18n/locales/en.json | Adds strings for group picker UI and expand action. |
| apps/web/src/i18n/locales/zh-Hans.json | Adds Chinese strings for group picker UI and expand action. |
| apps/web/src/pages/Orchestrate/Group.tsx | Implements sortable/collapsible group cards and wires in picker modals + add actions. |
| apps/web/src/pages/Orchestrate/Subscription.tsx | Adjusts accordion default open behavior in the subscription resource UI. |
| apps/web/src/pages/Orchestrate/index.tsx | Adds edge auto-scroll, drag destination tracking, hovered-group fallback drop behavior, and group sorting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
318
to
+344
| const onDragEnd = (result: DropResult) => { | ||
| const { source, destination, draggableId } = result | ||
| const fallbackGroupId = hoveredGroupIdRef.current | ||
|
|
||
| setIsDragging(false) | ||
| setDraggingResource(null) | ||
|
|
||
| if (!destination) return | ||
| setDragDestinationDroppableId(null) | ||
| setHoveredGroupId(null) | ||
| hoveredGroupIdRef.current = null | ||
| edgeAutoScrollEnabledRef.current = false | ||
| stopEdgeAutoScroll() | ||
|
|
||
| const sourceDroppableId = source.droppableId | ||
| const destDroppableId = destination.droppableId | ||
| const destDroppableId = destination?.droppableId | ||
|
|
||
| if (sourceDroppableId === 'group-list' && destDroppableId === 'group-list' && destination) { | ||
| if (source.index !== destination.index) { | ||
| setGroupSortOrder(arrayMove(sortedGroupIds, source.index, destination.index)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (!destination) { | ||
| if (fallbackGroupId) { | ||
| if (sourceDroppableId === 'subscription-list') { | ||
| const subId = draggableId.replace('subscription-', '') | ||
| const targetGroup = groupsQuery?.groups.find((group: GroupsQuery['groups'][number]) => group.id === fallbackGroupId) |
There was a problem hiding this comment.
onDragEnd performs “fallback” mutations when destination is null, but @hello-pangea/dnd can also produce destination: null when the drag is cancelled (e.g. ESC), which would unintentionally add nodes/subscription groups to the hovered group. Guard this branch by checking result.reason === 'DROP' (or explicitly returning early on reason === 'CANCEL') before applying any mutations based on hoveredGroupIdRef.
Comment on lines
+255
to
+260
| window.addEventListener('mousemove', handlePointerMove, { passive: true }) | ||
| window.addEventListener('pointermove', handlePointerMove, { passive: true }) | ||
|
|
||
| return () => { | ||
| window.removeEventListener('mousemove', handlePointerMove) | ||
| window.removeEventListener('pointermove', handlePointerMove) |
There was a problem hiding this comment.
The drag pointer tracking effect registers both mousemove and pointermove handlers. On browsers that dispatch compatibility mouse events after pointer events, this can double-fire and cause unnecessary state updates / elementFromPoint calls during dragging. Consider using a single input source (prefer pointermove), or feature-detect and only register one listener.
Suggested change
| window.addEventListener('mousemove', handlePointerMove, { passive: true }) | |
| window.addEventListener('pointermove', handlePointerMove, { passive: true }) | |
| return () => { | |
| window.removeEventListener('mousemove', handlePointerMove) | |
| window.removeEventListener('pointermove', handlePointerMove) | |
| const moveEventName = 'PointerEvent' in window ? 'pointermove' : 'mousemove' | |
| window.addEventListener(moveEventName, handlePointerMove, { passive: true }) | |
| return () => { | |
| window.removeEventListener(moveEventName, handlePointerMove) |
Comment on lines
+125
to
+127
| role="button" | ||
| tabIndex={0} | ||
| aria-pressed={checked} |
There was a problem hiding this comment.
The selectable item wrapper uses role="button" with aria-pressed, but the UI is semantically a multi-select checkbox list. For better screen reader behavior, prefer role="checkbox" with aria-checked (or use an actual <button>/<label> pattern and let the checkbox be the only interactive control) so the state is announced correctly.
Suggested change
| role="button" | |
| tabIndex={0} | |
| aria-pressed={checked} | |
| role="checkbox" | |
| tabIndex={0} | |
| aria-checked={checked} |
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
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.
Upstream PR Prep for
daeuniverse/daedPrepared against upstream
mainat commit70e413c9ceb0ad40c33a3f2da55ec8e2c6be4ef5.Source reference:
1. Upstream requirements from CONTRIBUTING
main.main.pnpm testpnpm lint2. Current upstream-ready branch
Prepared branch:
feat/group-picker-drag-uxCurrent HEAD:
e09d515feat(web): add group resource pickers and improve drag UXCurrent diff vs
upstream/main:Changed files:
apps/web/src/components/DraggableResourceBadge.tsxapps/web/src/components/DroppableGroupCard.tsxapps/web/src/components/GroupResourcePickerModal.tsxapps/web/src/components/SortableGroupContent.tsxapps/web/src/components/SortableResourceBadge.tsxapps/web/src/i18n/locales/en.jsonapps/web/src/i18n/locales/zh-Hans.jsonapps/web/src/pages/Orchestrate/Group.tsxapps/web/src/pages/Orchestrate/Subscription.tsxapps/web/src/pages/Orchestrate/index.tsxNotes:
ci/linux-amd64v3-test.3. Verification results run locally
Environment used:
v22.22.210.24.0Results:
pnpm install: passedpnpm test: passedpnpm lint: failedpnpm check-types: failedFailure details
pnpm lintThis failed before reaching project lint violations.
Observed error:
Interpretation:
Relevant files:
pnpm check-typesObserved error:
Interpretation:
6.0.2.Relevant file:
pnpm testStatus:
Summary:
apps/webvitest suite passed4. PR blockers before opening upstream PR
Remaining blockers
This PR changes visible orchestrate behavior. Upstream guidance strongly suggests screenshots or a short recording.
Recommendation:
tmp/as PR attachments.Even though these failures appear to be repository toolchain issues rather than branch-specific logic regressions, they are still part of the expected pre-PR checks in CONTRIBUTING.
Recommendation:
main.No longer blockers
Those have already been cleaned up in
feat/group-picker-drag-ux.5. Recommended PR scope
Recommended upstream scope:
6. Suggested PR title
feat(orchestrate): add group resource pickers and improve group drag-and-drop UXAlternative:
feat(web): add group pickers, sortable groups, and better drag UX7. Draft PR description
pnpm check-typescurrently fails in repo TypeScript config with:These look like existing toolchain/config compatibility issues rather than failures introduced by this branch's application logic.
Manual testing
Screenshots
Suggested screenshot set:
Open PR from:
ksong008:feat/group-picker-drag-uxInto:
daeuniverse:main9. Final recommendation
Status: technically prepared, but not fully green
Meaning:
If you are okay opening the PR with explicit notes about the current lint/type-check failures, this branch is ready enough to proceed.