refactor: decompose KanbanBoard God Component into focused hooks#147
refactor: decompose KanbanBoard God Component into focused hooks#147ryota-murakami merged 6 commits intomainfrom
Conversation
…antic actions (#143) - Replace setSearchQuery with updateSearch - Replace setSelectedRepos([]) with clearSelection - Replace setIsAdding(true/false) with startAdding/finishAdding - Replace setAddError with setAddingError/clearError - All actions wrapped in useCallback for referential stability - startAdding() combines setIsAdding(true) + clearError() atomically
- Extract ~195 lines of DnD logic into src/hooks/board/useKanbanDnD.ts - Move forgivingCollisionDetection, sensors, drag handlers, grid calculations - Column sub-handlers (handleNewRowDrop, handleColumnInsertDrop, handleColumnSwap) included - handleDragEnd split into column/card branches per #145 - History state stays in KanbanBoard via pushCardHistory/pushColumnHistory callbacks - All handlers wrapped in useCallback with correct dependency arrays
- Extract history/columnHistory state, handleUndo, Z-key useEffect - Hook is self-contained: keyboard shortcut registered internally - Returns pushCardHistory/pushColumnHistory for useKanbanDnD consumption - KanbanBoard no longer imports setStatusLists/setRepoCards directly - Removes ~73 lines from KanbanBoard
- Extract comments state + 3 CRUD handlers into useCommentState hook - Preserve optimistic updates, Sentry error tracking, and toast feedback - KanbanBoard now imports zero direct server actions - Removed unused imports: useState, useCallback, Sentry, toast, CommentColor - KanbanBoard reduced from 825 → 295 lines (completes decomposition)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR extracts DnD, undo/history, and comment-management logic from KanbanBoard into three new hooks (useKanbanDnD, useKanbanUndo, useCommentState), updates exports, and seals useRepositorySearch by replacing raw setters with semantic action handlers; KanbanBoard is reduced by several hundred lines and wired to the new hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (drag)"
participant Board as "KanbanBoard"
participant Hook as "useKanbanDnD"
participant Redux as "Redux (dispatch)"
participant API as "Server/API"
participant Sentry as "Sentry / Toasts"
Note over User,Hook: Card drag & drop flow
User->>Board: drag end event
Board->>Hook: handleDragEnd(active, over)
Hook->>Redux: pushCardHistory(snapshot)
Hook->>Redux: dispatch local reorder/move
Hook->>API: persist change (batchUpdate / updateRepoCardPosition)
API-->>Hook: success / failure
alt success
Hook->>Board: finish (clear active)
else failure
Hook->>Redux: rollback previous snapshot
Hook->>Sentry: report error
Hook->>Board: show toast error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 69.27% 69.29% +0.01%
==========================================
Files 146 149 +3
Lines 4554 4602 +48
Branches 1216 1226 +10
==========================================
+ Hits 3155 3189 +34
- Misses 1377 1391 +14
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/hooks/board/useKanbanUndo.ts (1)
75-121:handleUndorecreated on every history push causes event listener churn.
handleUndocloses overhistoryandcolumnHistorystate, so it's recreated on every push. TheuseEffecton line 124 then tears down and re-registers thekeydownlistener each time. Using refs for the stacks (read inhandleUndo, written in push callbacks) would give a stablehandleUndoidentity and avoid this re-registration cycle.Also applies to: 124-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useKanbanUndo.ts` around lines 75 - 121, The handleUndo callback closes over history and columnHistory so its identity changes whenever those arrays change; replace the collapsing dependency by keeping stable refs for the stacks (e.g., historyRef and columnHistoryRef) that mirror state and are written to inside the push/update logic, then have handleUndo read from historyRef.current and columnHistoryRef.current so it can be memoized without history/columnHistory in its dependency array; update the push callbacks that call setHistory/setColumnHistory to also update the corresponding ref, and ensure the useEffect that registers the keydown listener uses the stable handleUndo (so it no longer tears down/re-registers on each stack push).src/hooks/board/useKanbanDnD.ts (1)
222-224: AlignupdateStatusListPositionimport with other board actions.The function is already exported from
@/lib/actions/board(line 254), but it's dynamically imported here while all other board actions (updateRepoCardPosition,batchUpdateRepoCardOrders,swapStatusListPositions,batchUpdateStatusListPositions) are statically imported at lines 42–46. Move it to the static imports for consistency.Proposed fix
Add to the static imports at lines 42–46:
import { updateRepoCardPosition, batchUpdateRepoCardOrders, swapStatusListPositions, batchUpdateStatusListPositions, + updateStatusListPosition, } from '@/lib/actions/board'Then simplify the handler:
- try { - const { updateStatusListPosition } = await import('@/lib/actions/board') - await updateStatusListPosition(activeStatus.id, targetRow, targetCol) + try { + await updateStatusListPosition(activeStatus.id, targetRow, targetCol)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useKanbanDnD.ts` around lines 222 - 224, The dynamic import of updateStatusListPosition inside the try block should be made consistent with other board actions by moving updateStatusListPosition to the static imports alongside updateRepoCardPosition, batchUpdateRepoCardOrders, swapStatusListPositions, and batchUpdateStatusListPositions, and then remove the inline await import and call updateStatusListPosition(activeStatus.id, targetRow, targetCol) directly; update the import list (where those other actions are imported) and simplify the try block to call updateStatusListPosition by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/board/useCommentState.ts`:
- Around line 72-88: handleCommentChange currently applies optimistic state via
setComments then ignores failures from updateComment when result.success is
false; capture the previous comment for cardId before calling setComments, and
on failure revert the state using setComments(prev => ({...prev, [cardId]:
previous})) and surface an error to the user (e.g., trigger a
toast/notification) so they see the save failed; keep references to the
functions/variables updateComment, setComments, result.success and ensure the
rollback runs inside the failure branch and the user-visible notification is
invoked.
- Around line 97-117: The handlers handleCommentColorChange and
handleCommentDelete currently use try/catch but the underlying functions
updateCommentColor and deleteComment return ActionResult<void> and never throw,
so change both to follow the handleCommentChange pattern: call
updateCommentColor/deleteComment, inspect the returned result.success and handle
result.success === false by logging (avoid double-reporting to Sentry if server
already reports) and showing toast.error; only use Sentry.captureException here
if the result contains an error payload and server did not already log it.
Ensure you still optimistically update state (setComments) before the call and
revert or show an error when result.success is false.
In `@src/hooks/board/useKanbanDnD.ts`:
- Around line 436-457: Cross-column moves ignore the visual drop target and
instead compute newOrder by finding activeCard in the filtered array; update
this to respect over.id like the same-column path: in the else branch (where
activeCard, cards, targetStatusId are used and dispatch(setRepoCards(...)) is
called) compute targetColumnCards then determine newOrder using over.id — if
over.id corresponds to a card in targetColumnCards use findIndex(c => c.id ===
over.id) (and insert before/after as your same-column logic expects), and if
over.id is a column/zone treat it as append (newOrder =
targetColumnCards.length); then call updateRepoCardPosition(activeCard.id,
targetStatusId, newOrder) and keep the existing Sentry + rollback behavior.
In `@src/hooks/board/useKanbanUndo.ts`:
- Around line 136-140: The Z-key handler in useKanbanUndo currently triggers on
any 'z'/'Z' press and prevents default, causing conflicts with Ctrl/Cmd+Z
browser undo; update the key check in the keyboard listener (the block that
currently tests event.key === 'z' || event.key === 'Z') to only trigger when no
modifier keys are pressed (e.g. require !event.ctrlKey && !event.metaKey, and
optionally !event.altKey if desired) before calling handleUndo() and calling
event.preventDefault(); ensure you keep the existing uppercase/lowercase
handling and update the conditional in the same listener so bare-Z triggers
board undo but Ctrl/Cmd+Z does not.
---
Nitpick comments:
In `@src/hooks/board/useKanbanDnD.ts`:
- Around line 222-224: The dynamic import of updateStatusListPosition inside the
try block should be made consistent with other board actions by moving
updateStatusListPosition to the static imports alongside updateRepoCardPosition,
batchUpdateRepoCardOrders, swapStatusListPositions, and
batchUpdateStatusListPositions, and then remove the inline await import and call
updateStatusListPosition(activeStatus.id, targetRow, targetCol) directly; update
the import list (where those other actions are imported) and simplify the try
block to call updateStatusListPosition by name.
In `@src/hooks/board/useKanbanUndo.ts`:
- Around line 75-121: The handleUndo callback closes over history and
columnHistory so its identity changes whenever those arrays change; replace the
collapsing dependency by keeping stable refs for the stacks (e.g., historyRef
and columnHistoryRef) that mirror state and are written to inside the
push/update logic, then have handleUndo read from historyRef.current and
columnHistoryRef.current so it can be memoized without history/columnHistory in
its dependency array; update the push callbacks that call
setHistory/setColumnHistory to also update the corresponding ref, and ensure the
useEffect that registers the keydown listener uses the stable handleUndo (so it
no longer tears down/re-registers on each stack push).
| } else { | ||
| // Move to different column | ||
| const updatedCards = cards.map((c) => | ||
| c.id === activeCard.id ? { ...c, statusId: targetStatusId } : c, | ||
| ) | ||
| dispatch(setRepoCards(updatedCards)) | ||
|
|
||
| try { | ||
| const targetColumnCards = updatedCards.filter( | ||
| (c) => c.statusId === targetStatusId, | ||
| ) | ||
| const newOrder = targetColumnCards.findIndex( | ||
| (c) => c.id === activeCard.id, | ||
| ) | ||
| await updateRepoCardPosition(activeCard.id, targetStatusId, newOrder) | ||
| } catch (error) { | ||
| Sentry.captureException(error, { | ||
| tags: { action: 'syncCardPosition' }, | ||
| }) | ||
| dispatch(setRepoCards(cards)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the full context of useKanbanDnD.ts around the mentioned lines
wc -l src/hooks/board/useKanbanDnD.tsRepository: laststance/gitbox
Length of output: 96
🏁 Script executed:
# Read the relevant section with broader context to understand the handler
sed -n '400,480p' src/hooks/board/useKanbanDnD.tsRepository: laststance/gitbox
Length of output: 2518
🏁 Script executed:
# Search for updateRepoCardPosition usage across the codebase
rg -n 'updateRepoCardPosition' --type=tsRepository: laststance/gitbox
Length of output: 793
🏁 Script executed:
# Search for how over.id is used in this file
rg -n 'over\.id' src/hooks/board/useKanbanDnD.ts -B2 -A2Repository: laststance/gitbox
Length of output: 1203
🏁 Script executed:
# Look for any similar order calculation patterns with drop position
rg -n 'findIndex.*statusId' --type=ts -C3Repository: laststance/gitbox
Length of output: 43
🏁 Script executed:
# Get the full handleDragEnd function to see the complete logic
sed -n '333,483p' src/hooks/board/useKanbanDnD.tsRepository: laststance/gitbox
Length of output: 4588
🏁 Script executed:
# Check if there are any comments or TODOs about cross-column ordering
rg -n 'cross.column|different column' src/hooks/board/useKanbanDnD.ts -B2 -A2Repository: laststance/gitbox
Length of output: 262
🏁 Script executed:
# Check the updateRepoCardPosition function signature and implementation
sed -n '382,410p' src/lib/actions/board.tsRepository: laststance/gitbox
Length of output: 657
Cross-column card move does not respect visual drop position — card lands at array order instead of where dropped.
When moving a card to a different column (lines 437-451), the code ignores over.id and instead places the card at its natural position in the filtered array. Compare this to same-column reordering (lines 413-428), which explicitly uses over.id to calculate the visual drop position via findIndex.
For cross-column moves, pass over.id to determine the drop position, similar to the same-column logic. If over.id is a card in the target column, use it to calculate the insertion point. If over.id is a column/zone, append to the end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/board/useKanbanDnD.ts` around lines 436 - 457, Cross-column moves
ignore the visual drop target and instead compute newOrder by finding activeCard
in the filtered array; update this to respect over.id like the same-column path:
in the else branch (where activeCard, cards, targetStatusId are used and
dispatch(setRepoCards(...)) is called) compute targetColumnCards then determine
newOrder using over.id — if over.id corresponds to a card in targetColumnCards
use findIndex(c => c.id === over.id) (and insert before/after as your
same-column logic expects), and if over.id is a column/zone treat it as append
(newOrder = targetColumnCards.length); then call
updateRepoCardPosition(activeCard.id, targetStatusId, newOrder) and keep the
existing Sentry + rollback behavior.
There was a problem hiding this comment.
This is pre-existing behavior that was moved verbatim from KanbanBoard into the hook — not introduced by this refactoring PR. The cross-column ordering logic was written this way intentionally: when moving a card to a different column, over.id may refer to a column (not a card), so findIndex against cards would return -1. The current approach appends to the natural position in the target column. Changing DnD behavior in a pure refactoring PR carries regression risk. Will track as a separate enhancement issue if needed.
There was a problem hiding this comment.
@ryota-murakami You're absolutely right — preserving exact behavior is the correct approach for a refactoring PR. Mixing behavior changes with structural changes would make it harder to identify the source of any regressions.
If you'd like to track this as a future enhancement, I can help you create a follow-up issue with the context and suggested approach. Just let me know!
🧠 Learnings used
Learnt from: CR
Repo: laststance/gitbox PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T14:45:15.429Z
Learning: Applies to **/*.{tsx,ts} : Use React 19.2 features: `useOptimistic`, `useActionState`, `use` API, and Form Actions for state management and async operations
Learnt from: CR
Repo: laststance/gitbox PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T14:50:11.012Z
Learning: Applies to **/{components,app}/**/*.{ts,tsx} : Use React 19.2 APIs: `useOptimistic`, `useActionState`, `use` API, and Form Actions instead of traditional state management patterns
- useCommentState: convert unreachable try/catch to result.success pattern (server actions return ActionResult, never throw) - useCommentState: add rollback + toast on all 3 handlers failure path - useCommentState: remove duplicate Sentry reporting (server already reports) - useKanbanUndo: guard Z-key handler with !ctrlKey && !metaKey && !altKey to avoid conflict with browser Ctrl+Z undo
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
…tion Address CodeRabbit nitpick: align updateStatusListPosition import with other board actions that are already statically imported.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/hooks/board/useKanbanDnD.ts (1)
208-293: ConsideruseOptimisticfor the optimistic-dispatch/DB-sync pattern per project guidelines.The sub-handlers (
handleNewRowDrop,handleColumnInsertDrop,handleColumnSwap, and the card paths inhandleDragEnd) all share the same pattern: optimistically update Redux state → fire async DB call → rollback on error. This is the exact use-caseuseOptimisticwas designed for.Per project guidelines,
useOptimisticis the preferred approach for async state management with optimistic updates. Given the Redux dependency, one practical option is to keep Redux as the source of truth for persistence but useuseOptimisticto manage the transient "in-flight" visual state, reducing the need for manual rollback boilerplate.This is a broader architectural change that doesn't need to block this PR, but would be good to track as a follow-up.
Based on learnings: "Use React 19.2 features:
useOptimistic,useActionState,useAPI, and Form Actions for state management and async operations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/board/useKanbanDnD.ts` around lines 208 - 293, The handlers handleNewRowDrop and handleColumnInsertDrop (and similarly handleColumnSwap and the card-related branches in handleDragEnd) duplicate an optimistic-update → async DB call → manual rollback pattern; replace this boilerplate by using React's useOptimistic to manage the transient optimistic state while keeping Redux as the persistent source of truth: create a local optimistic state keyed by status IDs with useOptimistic, update that optimistic state immediately in the handlers (instead of directly dispatching setStatusLists), call updateStatusListPosition / batchUpdateStatusListPositions, then on success dispatch the Redux commit (setStatusLists) and on failure revert the optimistic state and capture the error (Sentry.captureException) — this centralizes rollback logic, removes manual dispatch rollback in the catch blocks, and apply the same pattern for handleColumnSwap and the card move branches in handleDragEnd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/board/useKanbanUndo.ts`:
- Around line 75-120: handleUndo currently performs optimistic Redux updates and
pops history before the DB write completes, so failures leave Redux and DB out
of sync; capture the pre-undo state and restore it on DB failure. Modify
useKanbanUndo to accept current board state via UseKanbanUndoParams (e.g.,
currentStatuses and currentCards), then in handleUndo save the preDispatchState
(for columns: currentStatuses or the current setStatusLists state; for cards:
currentCards or current setRepoCards state) before calling dispatch and before
slicing history; if batchUpdateStatusListPositions or batchUpdateRepoCardOrders
rejects, in the .catch() restore Redux by dispatching
setStatusLists(preDispatchState) or setRepoCards(preDispatchState) and restore
the history stack (push the popped item back via setColumnHistory / setHistory)
in addition to the existing Sentry/toast handling. Ensure you reference the
existing symbols: handleUndo, batchUpdateStatusListPositions,
batchUpdateRepoCardOrders, dispatch(setStatusLists), dispatch(setRepoCards),
setColumnHistory, setHistory.
---
Duplicate comments:
In `@src/hooks/board/useKanbanUndo.ts`:
- Around line 136-145: The key handler in useKanbanUndo correctly restricts undo
to bare Z (no Ctrl/Meta/Alt) to avoid browser conflicts; keep the existing guard
(the conditional checking event.key === 'z'/'Z' plus !event.ctrlKey,
!event.metaKey, !event.altKey), keep event.preventDefault() and the handleUndo()
call as-is in the keydown handler so no changes are needed to the useKanbanUndo
logic.
---
Nitpick comments:
In `@src/hooks/board/useKanbanDnD.ts`:
- Around line 208-293: The handlers handleNewRowDrop and handleColumnInsertDrop
(and similarly handleColumnSwap and the card-related branches in handleDragEnd)
duplicate an optimistic-update → async DB call → manual rollback pattern;
replace this boilerplate by using React's useOptimistic to manage the transient
optimistic state while keeping Redux as the persistent source of truth: create a
local optimistic state keyed by status IDs with useOptimistic, update that
optimistic state immediately in the handlers (instead of directly dispatching
setStatusLists), call updateStatusListPosition / batchUpdateStatusListPositions,
then on success dispatch the Redux commit (setStatusLists) and on failure revert
the optimistic state and capture the error (Sentry.captureException) — this
centralizes rollback logic, removes manual dispatch rollback in the catch
blocks, and apply the same pattern for handleColumnSwap and the card move
branches in handleDragEnd.
| // Save card history for undo | ||
| pushCardHistory(cards) | ||
|
|
||
| if (activeCard.statusId === targetStatusId) { | ||
| // Reorder within same column | ||
| const columnCards = cardsByStatus[targetStatusId] ?? EMPTY_CARDS | ||
| const oldIndex = columnCards.findIndex((c) => c.id === active.id) | ||
| const newIndex = columnCards.findIndex((c) => c.id === over.id) | ||
|
|
||
| if (oldIndex !== -1 && newIndex !== -1 && oldIndex !== newIndex) { | ||
| const reordered = arrayMove(columnCards, oldIndex, newIndex) | ||
| const otherCards = cards.filter((c) => c.statusId !== targetStatusId) | ||
| dispatch(setRepoCards([...otherCards, ...reordered])) | ||
|
|
||
| const updates = reordered.map((card, index) => ({ | ||
| id: card.id, | ||
| statusId: targetStatusId, | ||
| order: index, | ||
| })) | ||
| try { | ||
| await batchUpdateRepoCardOrders(updates) | ||
| } catch (error) { | ||
| Sentry.captureException(error, { | ||
| tags: { action: 'syncCardOrder' }, | ||
| }) | ||
| dispatch(setRepoCards(cards)) | ||
| } | ||
| } |
There was a problem hiding this comment.
pushCardHistory fires before the no-op guard, creating phantom undo states.
pushCardHistory(cards) is called at line 409 unconditionally, before the oldIndex !== newIndex check at line 417. When a card is dropped on its current position (or not found in columnCards), no board state changes — but a snapshot is pushed. The user then needs two Z presses to undo one real drag: the first silently consumes the phantom entry.
🔧 Proposed fix — move `pushCardHistory` inside actual-move branches
- // Save card history for undo
- pushCardHistory(cards)
-
if (activeCard.statusId === targetStatusId) {
// Reorder within same column
const columnCards = cardsByStatus[targetStatusId] ?? EMPTY_CARDS
const oldIndex = columnCards.findIndex((c) => c.id === active.id)
const newIndex = columnCards.findIndex((c) => c.id === over.id)
if (oldIndex !== -1 && newIndex !== -1 && oldIndex !== newIndex) {
+ pushCardHistory(cards)
const reordered = arrayMove(columnCards, oldIndex, newIndex)
...
}
} else {
// Move to different column
+ pushCardHistory(cards)
const updatedCards = cards.map(...)
...
}| const handleUndo = useCallback(() => { | ||
| // Try column undo first, then card undo | ||
| if (columnHistory.length > 0) { | ||
| const previousState = columnHistory[columnHistory.length - 1] | ||
| if (previousState) { | ||
| dispatch(setStatusLists(previousState)) | ||
| setColumnHistory((prev) => prev.slice(0, -1)) | ||
| toast.success('Column order restored') | ||
|
|
||
| // P1-8: Sync reverted column positions to DB | ||
| const updates = previousState.map((s) => ({ | ||
| id: s.id, | ||
| gridRow: s.gridRow, | ||
| gridCol: s.gridCol, | ||
| })) | ||
| batchUpdateStatusListPositions(updates).catch((error) => { | ||
| Sentry.captureException(error, { | ||
| tags: { action: 'undoColumnPositions' }, | ||
| }) | ||
| toast.error('Failed to sync undo to database') | ||
| }) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if (history.length > 0) { | ||
| const previousState = history[history.length - 1] | ||
| if (previousState) { | ||
| dispatch(setRepoCards(previousState)) | ||
| setHistory((prev) => prev.slice(0, -1)) | ||
| toast.success('Card operation undone') | ||
|
|
||
| // P1-8: Sync reverted card positions to DB | ||
| const updates = previousState.map((c, index) => ({ | ||
| id: c.id, | ||
| statusId: c.statusId, | ||
| order: c.order ?? index, | ||
| })) | ||
| batchUpdateRepoCardOrders(updates).catch((error) => { | ||
| Sentry.captureException(error, { | ||
| tags: { action: 'undoCardPositions' }, | ||
| }) | ||
| toast.error('Failed to sync undo to database') | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Undo DB-sync failure leaves Redux and database permanently out of sync.
Both undo paths optimistically update Redux state and pop the history stack before the DB write completes. If batchUpdateStatusListPositions or batchUpdateRepoCardOrders throws, the .catch() only fires Sentry + toast — there is no dispatch(setStatusLists(...)) / dispatch(setRepoCards(...)) call to roll back the optimistic update. On the next page load the database wins and the board silently reverts, which is confusing.
Compare with handleColumnSwap in useKanbanDnD.ts (line 319–326), which captures statuses before dispatch and restores it in the catch.
The fix requires capturing the current state before dispatching, then restoring it on failure. The cleanest approach is to extend UseKanbanUndoParams with the current board state:
🔧 Proposed fix
interface UseKanbanUndoParams {
/** Redux dispatch function */
dispatch: AppDispatch
+ /** Current status lists for rollback on undo failure */
+ statuses: StatusListDomain[]
+ /** Current repo cards for rollback on undo failure */
+ cards: RepoCardForRedux[]
} export function useKanbanUndo(
params: UseKanbanUndoParams,
): UseKanbanUndoReturn {
- const { dispatch } = params
+ const { dispatch, statuses, cards } = params if (previousState) {
+ const preUndoState = statuses
dispatch(setStatusLists(previousState))
setColumnHistory((prev) => prev.slice(0, -1))
toast.success('Column order restored')
batchUpdateStatusListPositions(updates).catch((error) => {
Sentry.captureException(error, { tags: { action: 'undoColumnPositions' } })
+ dispatch(setStatusLists(preUndoState))
+ setColumnHistory((prev) => [...prev, previousState])
toast.error('Failed to sync undo to database')
}) if (previousState) {
+ const preUndoCards = cards
dispatch(setRepoCards(previousState))
setHistory((prev) => prev.slice(0, -1))
toast.success('Card operation undone')
batchUpdateRepoCardOrders(updates).catch((error) => {
Sentry.captureException(error, { tags: { action: 'undoCardPositions' } })
+ dispatch(setRepoCards(preUndoCards))
+ setHistory((prev) => [...prev, previousState])
toast.error('Failed to sync undo to database')
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/board/useKanbanUndo.ts` around lines 75 - 120, handleUndo currently
performs optimistic Redux updates and pops history before the DB write
completes, so failures leave Redux and DB out of sync; capture the pre-undo
state and restore it on DB failure. Modify useKanbanUndo to accept current board
state via UseKanbanUndoParams (e.g., currentStatuses and currentCards), then in
handleUndo save the preDispatchState (for columns: currentStatuses or the
current setStatusLists state; for cards: currentCards or current setRepoCards
state) before calling dispatch and before slicing history; if
batchUpdateStatusListPositions or batchUpdateRepoCardOrders rejects, in the
.catch() restore Redux by dispatching setStatusLists(preDispatchState) or
setRepoCards(preDispatchState) and restore the history stack (push the popped
item back via setColumnHistory / setHistory) in addition to the existing
Sentry/toast handling. Ensure you reference the existing symbols: handleUndo,
batchUpdateStatusListPositions, batchUpdateRepoCardOrders,
dispatch(setStatusLists), dispatch(setRepoCards), setColumnHistory, setHistory.
2 participants
Summary
KanbanBoard was 825 lines with 3+ mixed concerns (DnD, undo, comments, search API). This PR decomposes it into focused, testable hooks following established patterns.
useRepositorySearchAPI — replace 4 rawsetStatesetters with semantic actions (startAdding(),finishAdding(),clearSelection(), etc.)useKanbanDnDhook — DnD sensors, collision detection, drag handlers, grid calculations (~195 lines)useKanbanUndohook — history stacks,handleUndo, Z-key keyboard shortcut (~73 lines)useCommentStatehook — comment CRUD with optimistic updates, Sentry tracking, toast feedback (~75 lines)Result: KanbanBoard reduced from ~825 → ~295 lines. Zero behavior changes.
Closes #141, Closes #142, Closes #143, Closes #144, Closes #145
Architecture
Key Decisions
handleDragEnd, no meaningful intermediate stateuseKanbanUndoprovidespushCardHistory/pushColumnHistorycallbacks passed intouseKanbanDnDforgivingCollisionDetectionexported from hook file<DndContext>Test plan
pnpm lint— cleanpnpm typecheck— cleanpnpm test— 1282 passedpnpm build— successpnpm e2e:parallel— 11/12 shards passed (shard 1 failure is pre-existing infrastructure issue, confirmed identical onmain)Summary by CodeRabbit
New Features
Refactor