Resolve the dragged window by name when reordering a dock - #261
Conversation
Fixes DESKTOP-3J (IndexOutOfBoundsException: Index: 2, Size: 1): a same-dock drag crashed when the dock changed under the gesture. The drop's indices are snapshots - sourceIndex is frozen at drag start and insertionIndex comes from item bounds cached at the dock's last recomposition - while the dock list is live, and the game closing a panel mid-drag (a transient closeDialog) shrinks it. removeAt/add on stale indices then throw inside the drag-gesture coroutine, which has no handler, taking the whole app down; a stale-but-in-bounds sourceIndex was quieter and worse, reordering whatever window now sat at that index instead of the dragged one. changeWindowPositions now takes the dragged window's name, resolves its current index inside the update (dropping the gesture when the window is gone), and clamps the insertion index to the shrunken list, mirroring the clamp moveWindowToPosition already had. DropResult's sourceIndex - and the drag-start index snapshot feeding it - are removed so the trap cannot be reintroduced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesWindow reordering
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.kt`:
- Line 1338: Update the persistence flow containing the names collection and
setPositions so it returns immediately when the filtered names list is empty,
before performing any database query or transaction; preserve the existing
return behavior when reordered is null.
In
`@compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/window/DesktopWindowsAtLocation.kt`:
- Line 319: Remove index from the pointerInput keys in both
DesktopWindowsAtLocation.kt (lines 319-319) and WindowsAtLocation.kt (lines
311-311). Use uiState.name and location as the keys in both implementations
while leaving the dragDropState.startDrag behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f789526f-ee29-4712-a8b1-b536816d45a3
📒 Files selected for processing (6)
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.ktcompose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/DragDropState.ktcompose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/game/DesktopGameView.ktcompose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/window/DesktopWindowsAtLocation.ktcompose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/game/GameView.ktcompose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowsAtLocation.kt
💤 Files with no reviewable changes (1)
- compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/DragDropState.kt
Review feedback on #261: - The header's pointerInput was keyed on the window's index, so a window leaving the dock ahead of the dragged one restarted the block and cancelled the drag in flight - the same mid-drag dock change the crash fix is about. Each window is already wrapped in key(uiState.name), so identity survives that shift; keying the handler on name and location lets the gesture finish. The index was the block's last use of the parameter, so WindowViewSlot no longer takes one. - changeWindowPositions skips the persistence transaction when the dock holds nothing savable, instead of asking the DAO to rewrite an order that has no rows behind it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both review comments addressed in 69028fd.
This is also the flip side of the crash: with the index keyed, a window removed before the dragged one silently cancelled the gesture, while one removed after left the index untouched, let the gesture finish, and fed stale bounds into Since that key was the last use of the parameter, Skip persistence when the savable order is empty (minor) — fixed;
|
Problem
Fixes crash DESKTOP-3J (
IndexOutOfBoundsException: Index: 2, Size: 1inchangeWindowPositions, fatal on desktop, seen in production on 3.1.0-beta.27).A same-dock drag crashed when the dock changed under the gesture. The drop's indices are snapshots:
sourceIndexis frozen at drag start, andinsertionIndexcomes from item bounds cached at the dock's last recomposition - while the dock list is live. The game closing a panel mid-drag (a transient'scloseDialog) shrinks the list, and the unguardedremoveAt/addthen throw inside the drag-gesture coroutine, which has no handler, taking the whole app down. The crash math matched exactly: the drag layer believed the dock had 3 items while the live list had 2.There was also a quieter sibling: a stale-but-still-in-bounds
sourceIndexwould reorder whatever window now sat at that index instead of the one the user dragged.Fix
changeWindowPositionstakes the dragged window's name (whichDropResultalready carried), resolves its current index inside theupdate {}- dropping the gesture when the window is gone - and clamps the insertion index to the shrunken list, mirroring the clampmoveWindowToPositionalready had.result.name.DropResult.sourceIndex,DragDropState.sourceIndex, andstartDrag's index parameter are deleted so nothing can consume a drag-start index snapshot again.Testing
./gradlew jvmTestpasses and compose compiles for Android. The crash path is gesture-timing dependent (drop landing in the same frame a window leaves the dock), so it has no unit harness; the fix is defensive resolution against the live list at the single choke point.🤖 Generated with Claude Code
Summary by CodeRabbit