feat: add drag-and-drop sorting for API provider cards

[taiiiyang]

Type of Changes

• ✨ New feature (feat)

Description

Add drag-and-drop functionality to reorder API provider cards in the settings page.

• Support drag to reorder provider cards
• Visual feedback: card floats up with shadow when dragging
• Auto-scroll when dragging to list edges

How Has This Been Tested?

• Verified through manual testing

Checklist

• I have tested these changes locally
• My code follows the code style of this project
• My changes do not break existing functionality
• If my code was generated by AI, I have proofread and improved it as necessary.

---

Summary by cubic

Added drag-and-drop sorting to API provider cards in Settings using dnd-kit. Users can reorder providers with clear drag visuals, stable scroll, and the selected card stays selected.

• New Features

  • Drag to reorder provider cards.
  • Keyboard and mouse drag support.

• Bug Fixes

  • Optimistic, serialized config writes to prevent flicker or lost updates across tabs.
  • Keep scroll position unchanged after reorder.

^{Written for commit 857f241. Summary will update on new commits.}

[changeset-bot]

🦋 Changeset detected

Latest commit: 857f241

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@read-frog/extension	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[claude]

PR Review: Add drag-and-drop sorting for API provider cards

【Taste Score】

🟡 Acceptable

Overall the implementation works, but there are a few issues to address.

---

【Key Issues】

1. Type mismatch in setList callback ⚠️

<ReactSortable
  list={apiProvidersConfig}
  setList={setProvidersConfig}  // ❌ Type mismatch

apiProvidersConfig is APIProviderConfig[] (filtered), but setProvidersConfig expects ProvidersConfig (full array including non-API providers like google-translate, microsoft-translate).

When the user drags to reorder, setList will be called with only the API providers, dropping all non-API providers from the config. This is a data loss bug.

Fix: Create a handler that merges the reordered API providers back with the non-API providers:

const handleReorder = (newList: APIProviderConfig[]) => {
  const nonApiProviders = providersConfig.filter(p => !isAPIProviderConfig(p))
  setProvidersConfig([...newList, ...nonApiProviders])
}

2. Ineffective useEffect for selection preservation

useEffect(() => {
  if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
    setSelectedProviderId(apiProvidersConfig[0].id)
  }
  // eslint-disable-next-line react-hooks/exhaustive-deps
}, [])

This effect:

• Runs only once on mount (empty deps)
• Only sets selection if it already equals the first item
• Essentially does nothing useful

The comment says "Lock in initial selection to prevent it from jumping after reorder" but the logic doesn't achieve this. If this is meant to preserve selection after reorder, it needs to track the previous selection and restore it, not check against the first item.

Suggestion: Either remove this if not needed, or implement proper selection preservation logic (e.g., using onEnd callback from Sortable).

3. Nested scrollable container may cause issues

<div ref={scrollContainerRef} className="overflow-y-auto ...">
  <ReactSortable scroll={scrollContainerRef.current ?? true} ...>

Passing scrollContainerRef.current ?? true is problematic:

• On first render, scrollContainerRef.current is null, so it falls back to true
• The scroll option expects an HTMLElement or boolean
• If true, SortableJS will use auto-scroll detection which may not work correctly with the hidden scrollbar styles

---

【Minor Issues】

4. Removed cursor-pointer from ProviderCard

The original card had cursor-pointer to indicate clickability. Now only the wrapper has cursor-grab. Consider keeping both behaviors clear:

<div className="... cursor-grab active:cursor-grabbing ...">
  <ProviderCard ... /> // Inner card could retain cursor-pointer for click indication
</div>

5. Cleaned up commented code ✅

Good removal of the commented-out onSubmit in ProviderConfigForm. Clean code is better.

---

【Recommendations】

1. Critical: Fix the type mismatch that causes non-API providers to be dropped
2. Important: Either fix or remove the ineffective useEffect
3. Consider: Add keyboard accessibility for reordering (a11y)
4. Consider: Add visual feedback when order is saved (toast notification)

---

【Testing Suggestions】

• Add a non-API provider (google-translate) and verify it persists after reordering API providers
• Test that the selected provider remains selected after drag
• Test auto-scroll behavior with many providers
• Test on touch devices (the delayOnTouchOnly option)

[cubic-dev-ai]

2 issues found across 5 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name=".changeset/lemon-icons-burn.md">

<violation number="1" location=".changeset/lemon-icons-burn.md:2">
P2: This changeset declares a `patch` version bump for a new feature. Per semantic versioning, new features should use `minor` (backwards-compatible additions), while `patch` is for bug fixes. Consider changing to `minor` since drag-and-drop sorting is new functionality.</violation>
</file>

<file name="src/entrypoints/options/pages/api-providers/providers-config.tsx">

<violation number="1" location="src/entrypoints/options/pages/api-providers/providers-config.tsx:140">
P0: Critical data loss: `setList={setProvidersConfig}` will replace the entire providers config with only API providers after reorder, deleting any non-API providers. Create a handler that preserves non-API providers when updating the list.</violation>
</file>

_{Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.}

[mengxi-ream]

dnd 包是不是好点 https://dndkit.com/

[taiiiyang]

dnd 包是不是好点 https://dndkit.com/

可以，不过昨天我试了下，dndkit的代码量会偏多点

[ananaBMaster]

dnd 包是不是好点 dndkit.com

可以，不过昨天我试了下，dndkit的代码量会偏多点

或许我们可以比较一下？把两个东西的优劣点拿出来看看

[claude]

PR Review: feat: add drag-and-drop sorting for API provider cards

Overall Assessment

🟢 Good taste - This is a well-implemented feature using the established @dnd-kit library. The component is reusable and follows the project's React patterns.

---

👍 What's Done Well

1. Good library choice: @dnd-kit is a mature, accessible drag-and-drop library with keyboard support built-in
2. Reusable component: SortableList is generic and can be reused elsewhere in the codebase
3. Proper type constraints: T extends { id: string } ensures type safety
4. Accessibility: Keyboard sensor included for non-mouse users
5. Sensible defaults: 8px distance activation prevents accidental drags on click

---

⚠️ Potential Issues to Consider

1. Click Handler Conflict (Medium Priority)

In ProviderCard (providers-config.tsx:178):

onClick={() => setSelectedProviderId(id)}

The card has both click-to-select and drag behavior. While the 8px distance constraint helps, this could cause UX confusion - users might accidentally start dragging when trying to select.

Suggestion: Consider adding a dedicated drag handle icon instead of making the entire card draggable, or add visual affordance (grip icon) to indicate drag capability.

2. Ineffective useEffect in ProviderCardList (Low Priority)

// Lock in initial selection to prevent it from jumping after reorder
useEffect(() => {
  if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
    setSelectedProviderId(apiProvidersConfig[0].id)
  }
  // eslint-disable-next-line react-hooks/exhaustive-deps
}, [])

This effect sets selectedProviderId to itself when the condition is met - it's a no-op. The comment says "lock in initial selection" but the logic doesn't achieve this. Either:

• Remove this useEffect if not needed
• Or implement the intended behavior (perhaps storing the initial selection in a ref)

3. Local State Synchronization (Low Priority)

The SortableList component maintains local state that syncs with props:

const [localList, setLocalList] = useState(list)

useEffect(() => {
  setLocalList(list)
}, [list])

This pattern works but could cause a flash of stale content if the external list changes while a drag is in progress. The eslint-disable comment suggests this was considered. This is acceptable for this use case.

4. Missing key in renderItem (Minor)

The ProviderCard in renderItem callback doesn't need a key since SortableItemWrapper handles it, but double-check React warnings in dev mode.

---

🔒 Security

No security concerns identified. This is purely a UI reordering feature with no external data handling.

---

📊 Performance

• ✅ Uses animateLayoutChanges: () => false to prevent unnecessary layout animations
• ✅ restrictToVerticalAxis modifier prevents unnecessary horizontal calculations
• ⚠️ Consider memoizing handleReorder with useCallback to prevent recreation on each render

---

🧪 Test Coverage

No tests were added for the new SortableList component. Consider adding:

1. Unit tests for the reorder logic
2. Integration tests verifying drag-and-drop behavior

---

📝 Minor Suggestions

1. cursor styling: cursor-grab active:cursor-grabbing is applied to all items - this is good UX feedback

2. autoScroll disabled: autoScroll={false} means long lists won't auto-scroll when dragging to edges. Was this intentional? The PR description mentions "auto-scroll when dragging to list edges" but the code disables it.

---

Summary

This is a solid implementation that adds useful functionality. The main concerns are:

1. Clarify the interaction between click-to-select and drag
2. The no-op useEffect should be fixed or removed
3. Verify if autoScroll should be enabled (contradicts PR description)

Verdict: Approve with minor fixes suggested above.

[cubic-dev-ai]

2 issues found across 4 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/components/sortable-list.tsx">

<violation number="1" location="src/components/sortable-list.tsx:69">
P2: The PR description claims &quot;Auto-scroll when dragging to list edges&quot; but `autoScroll` is explicitly set to `false`, which disables this functionality. Either update the code to enable auto-scroll or correct the PR description to reflect the actual behavior.</violation>

<violation number="2" location="src/components/sortable-list.tsx:92">
P2: Missing `transition` property from `useSortable` hook. Without including `transition` in the style object, drag animations will be jerky/instant instead of smooth. Add `transition` to the destructured values and include it in the style.</violation>
</file>

_{Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.}

[claude]

PR Review: feat: add drag-and-drop sorting for API provider cards

【Taste Score】

🟢 Good taste

Overall this is a well-implemented feature that follows the project's patterns. The component abstraction is clean and reusable.

---

✅ What's Done Well

1. Good library choice: @dnd-kit is a modern, accessible, well-maintained library for React drag-and-drop
2. Reusable component: SortableList is properly abstracted with generics, making it reusable for other parts of the app
3. Accessibility support: Keyboard navigation with KeyboardSensor is included
4. Visual feedback: Proper drag overlay and opacity states provide clear UX
5. Modifiers: restrictToVerticalAxis and restrictToFirstScrollableAncestor are appropriate constraints
6. Local state pattern: Using local state for immediate UI updates while syncing async storage is a good pattern (lines 34-42 of sortable-list.tsx)

---

⚠️ Issues & Suggestions

1. Bug: initSelectProvider logic appears to be a no-op (Medium Priority)

// providers-config.tsx:55-58
const initSelectProvider = useEffectEvent(() => {
  if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
    setSelectedProviderId(apiProvidersConfig[0].id)
  }
})

This condition only sets selectedProviderId when it's already equal to apiProvidersConfig[0].id - essentially setting it to itself. The comment says "Lock in initial selection to prevent it from jumping after reorder" but this logic doesn't achieve that goal.

Possible fixes:

• If the intent is to set the initial selection when none exists, the condition should be if (apiProvidersConfig.length > 0 && !selectedProviderId)
• If the intent is to preserve selection, consider using a ref to track the initially selected ID

2. Missing key prop warning risk (Low Priority)

// providers-config.tsx:150-152
renderItem={providerConfig => (
  <ProviderCard providerConfig={providerConfig} />
)}

The ProviderCard is rendered without a key prop directly, but since the parent SortableItemWrapper uses key={item.id} this should work. However, for React reconciliation during drag operations, consider adding a key to the inner component as well.

3. Consider adding handleDragCancel (Low Priority)

The DndContext doesn't handle the onDragCancel event. While not critical, adding this would ensure activeItem is cleared if a drag is cancelled (e.g., pressing Escape):

const handleDragCancel = () => {
  setActiveItem(null)
}

4. Potential stale closure in effect dependency (Low Priority)

// providers-config.tsx:61-63
useEffect(() => {
  initSelectProvider()
}, [])

The empty dependency array is fine with useEffectEvent, but eslint-disable comment might be needed for the rule if it complains.

---

🔒 Security

No security concerns. This is purely UI state management with no external data handling.

---

⚡ Performance

• Good: The distance: 8 activation constraint prevents accidental drags
• Good: Using local state prevents re-renders from async storage operations
• Consider: Memoizing renderItem callback in the parent if performance issues arise with many providers

---

🧪 Test Coverage

No new tests added for the SortableList component. Consider adding:

• Unit tests for the SortableList component behavior
• Integration tests for the reorder functionality persistence

---

📋 Summary

The implementation is solid and follows project conventions well. The main concern is the initSelectProvider logic which appears to be ineffective. The drag-and-drop experience looks polished with proper visual feedback.

Recommended actions before merge:

1. Review/fix the initSelectProvider logic - verify it achieves the intended behavior
2. Consider adding onDragCancel handler for completeness

Good work on the feature! 🐸

[claude]

Code Review for PR #844: feat: add drag-and-drop sorting for API provider cards

【Taste Score】

🟢 Good taste

Overall, this is a well-implemented feature with good UX considerations.

---

✅ Strengths

1. Good library choice: @dnd-kit is a mature, accessible drag-and-drop library with excellent React 18+ support.

2. Reusable component design: The SortableList component is generic and can be reused for other sortable lists in the codebase.

3. Smart local state handling: Using local state for immediate UI updates while syncing async is a good pattern (lines 34-42 in sortable-list.tsx).

4. Accessibility support: Keyboard navigation via KeyboardSensor and sortableKeyboardCoordinates is included.

5. Visual feedback: Drag overlay with scale and shadow provides clear visual indication.

6. Proper modifiers: restrictToVerticalAxis and restrictToFirstScrollableAncestor prevent erratic drag behavior.

---

🔍 Issues & Suggestions

1. Potential bug: initSelectProvider logic is unclear

providers-config.tsx:55-58:

const initSelectProvider = useEffectEvent(() => {
  if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
    setSelectedProviderId(apiProvidersConfig[0].id)
  }
})

This condition sets selectedProviderId to itself if it already equals apiProvidersConfig[0].id. The comment says "Lock in initial selection to prevent it from jumping after reorder" but if the goal is to preserve selection, this logic doesn't achieve that. If the selected provider is reordered to a different position, it won't retain selection. Consider storing the selected ID in a ref or using a different approach.

2. Missing key stability concern

sortable-list.tsx:86: The key is item.id, which is correct. However, the ProviderCard component inside renderItem doesn't receive key - this is fine since it's wrapped, but worth noting.

3. Click handler conflict with drag

providers-config.tsx:181: onClick={() => setSelectedProviderId(id)} on ProviderCard may conflict with drag gestures. While PointerSensor has a distance constraint of 8px, clicks still fire after short drags. Consider using onPointerDown with additional logic or handling selection in a dedicated area (like a select icon).

4. No drag handle indicator

The entire card is draggable (cursor-grab), but there's no visual drag handle icon. Users may not discover this feature. Consider adding a grip/drag icon to the card.

5. Missing test coverage

No tests for the new SortableList component or the reordering logic. Given the complexity of drag-and-drop state management, unit tests for:

• handleReorder function
• Array reordering with arrayMove
• Selection preservation after reorder

would be valuable.

---

🔐 Security

No security concerns - this is purely UI logic with no external data handling.

⚡ Performance

Good. The component uses proper memoization patterns and the useEffect for syncing localList is appropriate.

---

📝 Minor Suggestions

1. Consider adding onDragCancel handler in SortableList to reset activeItem if drag is cancelled (e.g., pressing Escape).

2. Dead code cleanup - The commented-out onSubmit handler was removed in provider-config-form/index.tsx. Good cleanup! ✓

3. Overflow handling - The scroll container already hides scrollbars. Ensure the drag overlay doesn't get clipped when dragging at container edges.

---

Summary

Solid implementation of a nice UX feature. The main areas for improvement are the selection preservation logic and adding visual affordances for discoverability. Consider adding tests for the reordering logic.

[claude]

PR Review: feat: add drag-and-drop sorting for API provider cards

【Taste Score】

🟢 Good taste - Clean implementation using a well-established library (@dnd-kit) with appropriate abstraction.

【Key Insights】

Data structure: ✅ Correctly preserves non-API providers during reorder by filtering and re-concatenating.

Complexity: ✅ Reasonable - the SortableList component is a clean, reusable abstraction with proper separation of concerns.

Risk points: Low - changes are well-contained to the options page.

---

Code Quality & Best Practices

Strengths:

1. Good abstraction - SortableList is generic and reusable (<T extends { id: string }>)
2. Proper library choice - @dnd-kit is the recommended modern React DnD solution
3. Accessibility support - Keyboard sensor included for a11y compliance
4. Smart local state pattern - Using local state for immediate UI feedback while async storage syncs is correct

Suggestions:

1. initSelectProvider logic appears incomplete (providers-config.tsx:55-58):

   const initSelectProvider = useEffectEvent(() => {
     if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
       setSelectedProviderId(apiProvidersConfig[0].id)
     }
   })

   This condition sets the same value that's already selected - effectively a no-op. The intent seems to be to preserve selection, but the condition selectedProviderId === apiProvidersConfig[0].id means it only runs when already selected. Consider if this logic is actually needed or if it should handle the case where selectedProviderId is null/invalid.

2. Missing key prop warning potential (sortable-list.tsx:151):
   The ProviderCard inside renderItem doesn't receive an explicit key. While the parent SortableItemWrapper has the key, React may still warn in some scenarios. Consider documenting that the rendered item doesn't need a key since it's wrapped.

3. useEffectEvent is experimental (providers-config.tsx:5):
   useEffectEvent is currently an experimental React API. While it works, consider:

   • Adding a comment noting this is experimental
   • Or using useCallback with proper deps as a stable alternative

---

Potential Bugs

1. Race condition edge case (sortable-list.tsx:39-42):

   useEffect(() => {
     setLocalList(list)
   }, [list])

   If the user drags rapidly while storage is syncing, the external list prop update could overwrite in-progress drag state. This is mitigated by the comment acknowledging async storage, but worth noting for edge cases.

2. Click event propagation (providers-config.tsx:181):
   The onClick handler on ProviderCard may fire after drag operations. The distance: 8 activation constraint in PointerSensor helps, but very short drags (<8px) that don't trigger drag could still fire click. This seems acceptable for this UI.

---

Performance Considerations

✅ Good choices:

• restrictToVerticalAxis and restrictToFirstScrollableAncestor modifiers reduce unnecessary calculations
• verticalListSortingStrategy is appropriate for this use case
• Sensors properly configured with activation constraints

⚠️ Minor consideration:

• DragOverlay re-renders renderItem(activeItem) which creates a new React tree. For this simple card UI, this is fine, but worth noting if cards become complex.

---

Security Concerns

✅ No security concerns identified. The changes are purely UI-focused and don't involve:

• User input sanitization issues
• Storage of sensitive data
• External API calls
• Dynamic code execution

---

Test Coverage

⚠️ No tests added for SortableList component

Recommend adding at minimum:

1. Unit test for SortableList verifying items render in order
2. Unit test verifying setList is called with reordered array after drag
3. Integration test for handleReorder in ProviderCardList verifying non-API providers are preserved

---

Summary

Overall: Good implementation. The PR introduces a clean, reusable drag-and-drop component using industry-standard libraries. The code follows project patterns and is appropriately scoped.

Recommended actions before merge:

1. Clarify the initSelectProvider logic - it appears to be a no-op in its current form
2. Consider adding basic unit tests for the new SortableList component
3. (Optional) Add comment about useEffectEvent being experimental

---

🟢 Verdict: Approve with minor suggestions

[claude]

PR Review: feat: add drag-and-drop sorting for API provider cards

Overall, this is a well-structured implementation that adds a useful feature. The code follows the project's conventions and uses appropriate libraries. Here's my detailed feedback:

---

✅ What's Good

1. Solid library choice: @dnd-kit is well-maintained, accessible (keyboard support), and feature-rich
2. Reusable component: SortableList is generic and can be reused elsewhere
3. Good UX touches:
   • Drag distance constraint (8px) prevents accidental drags
   • Visual feedback with opacity and shadow on drag
   • Restricts to vertical axis and scroll container

---

🟡 Suggestions

1. initSelectProvider logic seems redundant (providers-config.tsx:55-63)

const initSelectProvider = useEffectEvent(() => {
  if (apiProvidersConfig.length > 0 && selectedProviderId === apiProvidersConfig[0].id) {
    setSelectedProviderId(apiProvidersConfig[0].id)
  }
})

This sets selectedProviderId to the same value it already has. The condition checks if selectedProviderId === apiProvidersConfig[0].id then sets it to apiProvidersConfig[0].id - a no-op. Consider clarifying the intent or removing if not needed.

2. Consider memoizing renderItem callback (providers-config.tsx:150-152)

renderItem={providerConfig => (
  <ProviderCard providerConfig={providerConfig} />
)}

This creates a new function on every render. While SortableList uses the list items as keys, wrapping with useCallback could prevent unnecessary re-renders:

const renderProviderCard = useCallback(
  (providerConfig: APIProviderConfig) => <ProviderCard providerConfig={providerConfig} />,
  []
)

3. Missing key in DragOverlay render (sortable-list.tsx:93-95)

When rendering activeItem in DragOverlay, consider if renderItem expects a key. Currently it's fine since it's a single item, but worth noting.

4. Accessibility: Consider adding aria-label for screen readers (sortable-list.tsx:117-127)

The sortable items could benefit from ARIA labels describing their position and drag state:

aria-label={`Item ${index + 1} of ${list.length}. ${isDragging ? 'Currently being dragged' : 'Drag to reorder'}`}

---

🔴 Potential Issues

1. Race condition potential in handleDragEnd (sortable-list.tsx:60-69)

const handleDragEnd = (event: DragEndEvent) => {
  setActiveItem(null)
  const { active, over } = event
  if (over && active.id !== over.id) {
    const oldIndex = localList.findIndex(item => item.id === active.id)
    const newIndex = localList.findIndex(item => item.id === over.id)
    const newList = arrayMove(localList, oldIndex, newIndex)
    setLocalList(newList)  // Local state update
    setList(newList)       // Async storage update
  }
}

If setList is async and triggers a config update that propagates back via useEffect, there could be a brief flicker. The current implementation handles this with local state, but consider if setList rejects/fails - the local state would be out of sync with actual storage.

Suggestion: Handle errors in setList or consider optimistic updates with rollback.

2. cursor-grab applied to entire wrapper may conflict with inner click handlers (sortable-list.tsx:121)

The ProviderCard has an onClick handler and a Switch component. The grab cursor and drag listeners are on the wrapper. This works due to the 8px distance constraint, but could cause subtle UX issues if users expect to click immediately.

---

📊 Test Coverage

No tests were added for the new SortableList component. Consider adding tests for:

• Reordering logic (arrayMove)
• Keyboard accessibility
• State synchronization between local and external list

---

🔐 Security

No security concerns identified. The changes are purely UI/UX related with no data handling beyond existing config patterns.

---

📦 Bundle Impact

Adding 4 @dnd-kit packages:

• @dnd-kit/core: ~20KB minified
• @dnd-kit/sortable: ~7KB minified
• @dnd-kit/modifiers: ~2KB minified
• @dnd-kit/utilities: ~1KB minified

This is reasonable for the functionality provided.

---

Summary

Aspect	Rating
Code Quality	🟢 Good
Best Practices	🟢 Good
Performance	🟡 Minor optimization possible
Security	🟢 No concerns
Test Coverage	🔴 Missing tests
Accessibility	🟡 Could be improved

Recommendation: Approve with minor suggestions. The implementation is solid and the feature works well. Consider adding tests in a follow-up.

[claude]

PR Review: feat: add drag-and-drop sorting for API provider cards

【Taste Score】

🟢 Good taste

This is a well-implemented feature that adds user-requested functionality. The architecture follows existing patterns, and the concurrent write handling is thoughtfully designed.

---

【Key Insights】

Data structure: Correctly separates API providers from non-API providers during reorder, preserving stable ordering for the full config.

Complexity: The write queue + version counter pattern in config.ts is the right approach for handling optimistic updates with serialized persistence.

Risk points: Module-level mutable state (writeQueue, writeVersion) - works fine for browser extension single-instance context.

---

🟢 Strengths

1. Reusable SortableList component - Generic, well-typed, handles accessibility (keyboard navigation) out of the box via dnd-kit.

2. Optimistic UI with proper reconciliation - The writeConfigAtom changes correctly implement:

   • Immediate UI update (line 45)
   • Serialized storage writes via promise queue (lines 56-88)
   • Stale-write detection via version counter (lines 74, 82)

3. Scroll position preservation - findVerticalScrollContainer + requestAnimationFrame keeps scroll stable after reorder.

4. Event propagation fix - onPointerDown={e => e.stopPropagation()} on Switch prevents drag activation when toggling enabled state.

5. Edge case handling - handleReorder preserves providers that appear during drag (e.g., from config sync in another tab).

---

🟡 Minor Suggestions

1. Unused effect in ProviderCardList (lines 76-83):

   useEffect(() => {
     if (didLockInitialSelectionRef.current)
       return
     if (selectedProviderId) {
       setSelectedProviderId(selectedProviderId)  // Sets value to itself
       didLockInitialSelectionRef.current = true
     }
   }, [selectedProviderId, setSelectedProviderId])

   This effect sets selectedProviderId to its own current value. If the intent is to persist the initial selection, the effect body could be simplified or removed. If it's meant to trigger a side effect in the atom, consider adding a comment explaining the purpose.

2. Potential CSS class typo - Gradient classes changed from bg-gradient-to-b to bg-linear-to-b (lines 158, 177). Verify this is intentional for your Tailwind version - standard Tailwind uses bg-gradient-to-*.

3. Missing tests for new functionality:

   • The SortableList component has no unit tests
   • The new writeConfigAtom queue/version logic isn't covered by config.test.ts
   • Consider adding tests for:
     • Reorder preserves non-API providers
     • Concurrent writes don't lose data
     • Version counter prevents stale overwrites

4. findVerticalScrollContainer could be memoized - It's called on every drag end, but the scroll container rarely changes. Not critical given the low frequency.

---

🔒 Security

No concerns. The changes don't introduce any user input handling, external data fetching, or privilege escalation paths.

---

⚡ Performance

Good. The optimistic update pattern ensures UI remains responsive. The write queue prevents storage thrashing. useSensor with distance: 8 activation constraint prevents accidental drags.

---

Summary

This is a clean, well-architected PR that adds a frequently-requested feature. The concurrent write handling is sophisticated and correctly implemented. The minor suggestions above are polish items rather than blockers.

Recommendation: ✅ Approve after addressing the unused effect (item 1) and verifying the gradient class names (item 2).

[dosubot]

Related Documentation

Checked 14 published document(s) in 1 knowledge base(s). No updates required.

^{How did I do? Any feedback?}