Skip to content

Resolve the dragged window by name when reordering a dock - #261

Merged
sproctor merged 2 commits into
masterfrom
fix-drag-reorder-crash
Aug 7, 2026
Merged

Resolve the dragged window by name when reordering a dock#261
sproctor merged 2 commits into
masterfrom
fix-drag-reorder-crash

Conversation

@sproctor

@sproctor sproctor commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Problem

Fixes crash DESKTOP-3J (IndexOutOfBoundsException: Index: 2, Size: 1 in changeWindowPositions, 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: 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. The game closing a panel mid-drag (a transient's closeDialog) shrinks the list, and the unguarded removeAt/add then 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 sourceIndex would reorder whatever window now sat at that index instead of the one the user dragged.

Fix

  • changeWindowPositions takes the dragged window's name (which DropResult already carried), 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.
  • Both drop handlers (desktop and mobile) pass result.name.
  • The stale-snapshot trap is removed from the API: DropResult.sourceIndex, DragDropState.sourceIndex, and startDrag's index parameter are deleted so nothing can consume a drag-start index snapshot again.

Testing

./gradlew jvmTest passes 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

  • Bug Fixes
    • Improved window drag-and-drop reordering for more reliable positioning.
    • Safely handles missing windows and prevents unnecessary layout updates.
    • Correctly adjusts destination positions when moving windows within the same location.

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>
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@sproctor, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6255ea92-80c5-4b42-a943-bfeeb685431e

📥 Commits

Reviewing files that changed from the base of the PR and between de5d26a and 69028fd.

📒 Files selected for processing (3)
  • compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.kt
  • compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/window/DesktopWindowsAtLocation.kt
  • compose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowsAtLocation.kt
📝 Walkthrough

Walkthrough

changeWindowPositions now identifies dragged windows by name, resolves their current index, handles missing windows, clamps destination indices, and avoids unnecessary persistence. Drag state and desktop/mobile call sites no longer pass source indices.

Changes

Window reordering

Layer / File(s) Summary
Remove source index from drag state
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/DragDropState.kt
DragDropState.startDrag, DropResult, endDrag, and clearState no longer track sourceIndex.
Resolve and persist reordered windows
compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.kt
changeWindowPositions resolves the dragged window by name, handles missing windows, clamps the destination index, and skips persistence when no reorder occurs.
Update desktop and mobile drag wiring
compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/game/DesktopGameView.kt, compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/window/DesktopWindowsAtLocation.kt, compose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/game/GameView.kt, compose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/window/WindowsAtLocation.kt
Desktop and mobile drag initiation and same-location reorder callbacks use window names instead of source indices.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: resolving the dragged window by name during dock reordering.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-drag-reorder-crash

Comment @coderabbitai help to get the list of available commands.

@sproctor sproctor closed this Aug 6, 2026
@sproctor sproctor reopened this Aug 6, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b2831a0 and de5d26a.

📒 Files selected for processing (6)
  • compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.kt
  • compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/window/DragDropState.kt
  • compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/game/DesktopGameView.kt
  • compose/src/jvmMain/kotlin/warlockfe/warlock3/compose/desktop/ui/window/DesktopWindowsAtLocation.kt
  • compose/src/mobileMain/kotlin/warlockfe/warlock3/compose/ui/game/GameView.kt
  • compose/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

Comment thread compose/src/commonMain/kotlin/warlockfe/warlock3/compose/ui/game/GameViewModel.kt Outdated
@sproctor sproctor closed this Aug 7, 2026
@sproctor sproctor reopened this Aug 7, 2026
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>
@sproctor

sproctor commented Aug 7, 2026

Copy link
Copy Markdown
Owner Author

Both review comments addressed in 69028fd.

pointerInput keyed on the index (major) — fixed, and it turned out to matter more than a restart nuisance. Each window in the dock is wrapped in key(uiState.name), so its composition identity survives a neighbor being removed; the index in the pointerInput key was therefore the only thing killing an in-flight drag when a window ahead of the dragged one left the dock. That is the same mid-drag dock change this PR exists to handle, so it was cancelling exactly the drags that most needed to complete. Both handlers now key on uiState.name and location, with a comment on why the index must stay out.

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 removeAt/add — the Index: 2, Size: 1 in the report.

Since that key was the last use of the parameter, WindowViewSlot no longer takes an index at all, in the same spirit as dropping DropResult.sourceIndex.

Skip persistence when the savable order is empty (minor) — fixed; changeWindowPositions now returns before the transaction when the dock holds nothing savable. Worth noting the DAO call was not a pure no-op in that case: with an empty names, setPositions still walked the dock and compacted any sparse positions, so a reorder of transient-only panels could rewrite saved rows (including closed windows' remembered slots) that the user never touched. Returning early avoids the round trip and that side effect.

./gradlew jvmTest passes and compose compiles for Android.

@sproctor
sproctor merged commit 54a8884 into master Aug 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant