Open
Conversation
Contributor
WalkthroughAdds a draggable panel system: new DraggableController and DraggableManager handle pointer-driven dragging, viewport clamping, overlap avoidance, and localStorage persistence. Multiple UI components were updated to expose Changes
Sequence DiagramsequenceDiagram
participant User as User / Pointer
participant Controller as DraggableController
participant Manager as DraggableManager
participant DOM as DOM & Viewport
participant Storage as localStorage
User->>Controller: pointerdown on element
Controller->>Controller: check locked, record pointer & start coords
Controller->>DOM: setPointerCapture()
User->>Controller: pointermove
Controller->>Controller: check commit threshold
alt committed
Controller->>Manager: snapshotObstacles(exclude)
Manager-->>Controller: obstacle rects
Controller->>Controller: compute candidate offsets
Controller->>Controller: clamp to viewport & resolve overlaps
Controller->>DOM: apply CSS transform (translate)
Controller->>Storage: persist x,y,locked
end
User->>Controller: pointerup / lostpointercapture
Controller->>Controller: finalize move, restore z-index
Controller->>DOM: release pointer capture
Controller->>Manager: notify resize / reclamp if needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
522-522: Hardcoded offset couples positioning to panel width.The
calc(50%-250px)offset assumes a 500px panel width (from line 534). If the panel width changes, this value must be updated manually.Consider using CSS custom properties or deriving the offset from the actual width for easier maintenance:
/* Example: Use transform for true centering without hardcoded values */ sm:left-1/2 sm:-translate-x-1/2However, if transform conflicts with the draggable system's
translate()usage, the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PlayerInfoOverlay.ts` at line 522, The hardcoded centering in PlayerInfoOverlay (class attribute containing "sm:left-[calc(50%-250px)]") ties the offset to a fixed 500px panel width; replace this with a maintenance-friendly approach by using true centering (e.g., set the small-screen left to 50% and apply a negative 50% translate) or use a CSS custom property for the panel width and compute the calc from that property so the position updates automatically if the panel size changes; if the component's draggable system relies on transform-based translations, prefer the CSS custom property approach to avoid conflicts with the draggable translate() behavior and update the class on the element referenced in PlayerInfoOverlay accordingly.src/client/graphics/DraggableController.ts (1)
286-293: Consider a brief comment for the silent catch.The empty catch block is acceptable here since
releasePointerCapturecan throw if already released. A short inline comment would clarify intent for future readers.📝 Suggested clarification
try { this.el.releasePointerCapture(this._pointerId); - } catch { - // already released + } catch { + // Pointer capture may already be released; safe to ignore }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/DraggableController.ts` around lines 286 - 293, Add a brief inline comment inside the empty catch in DraggableController where releasePointerCapture is called (the block that checks this._pointerId and calls this.el.releasePointerCapture(this._pointerId)) explaining that the catch is intentionally silent because releasePointerCapture can throw when the pointer is already released; keep the catch empty otherwise so behavior stays the same and reference this._pointerId and el.releasePointerCapture in the comment for clarity.
🤖 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/client/graphics/DraggableController.ts`:
- Around line 333-345: The load() method currently assigns data.x/data.y and
data.locked directly from parsed localStorage which can be non-number or
non-boolean; update load() to defensively validate and coerce these fields
before assigning to this._offsetX, this._offsetY and this._locked: parse numeric
values with Number(...) or parseFloat and check Number.isFinite(...) (falling
back to 0 on invalid input), and ensure locked is a boolean (use === true or
Boolean(...) with a safe default); keep the existing try/catch and
localStorage.removeItem(this.storageKey) behavior but replace direct assignments
with validated/coerced values to prevent NaN or string values from propagating
into transforms.
---
Nitpick comments:
In `@src/client/graphics/DraggableController.ts`:
- Around line 286-293: Add a brief inline comment inside the empty catch in
DraggableController where releasePointerCapture is called (the block that checks
this._pointerId and calls this.el.releasePointerCapture(this._pointerId))
explaining that the catch is intentionally silent because releasePointerCapture
can throw when the pointer is already released; keep the catch empty otherwise
so behavior stays the same and reference this._pointerId and
el.releasePointerCapture in the comment for clarity.
In `@src/client/graphics/layers/PlayerInfoOverlay.ts`:
- Line 522: The hardcoded centering in PlayerInfoOverlay (class attribute
containing "sm:left-[calc(50%-250px)]") ties the offset to a fixed 500px panel
width; replace this with a maintenance-friendly approach by using true centering
(e.g., set the small-screen left to 50% and apply a negative 50% translate) or
use a CSS custom property for the panel width and compute the calc from that
property so the position updates automatically if the panel size changes; if the
component's draggable system relies on transform-based translations, prefer the
CSS custom property approach to avoid conflicts with the draggable translate()
behavior and update the class on the element referenced in PlayerInfoOverlay
accordingly.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78625b5a-57c2-42a9-a96d-2a4b1b996a99
📒 Files selected for processing (13)
index.htmlresources/lang/en.jsonsrc/client/graphics/DraggableController.tssrc/client/graphics/DraggableManager.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/DraggablePanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/InGamePromo.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/UnitDisplay.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/DraggableController.ts (1)
231-263: Collision resolution can fail to converge with clustered obstacles.When multiple obstacles overlap the candidate rect, resolving one collision can push the element into another. The single-pass approach may leave residual overlaps. For the current use case with few panels, this is tolerable.
Consider adding a bounded iteration loop (e.g., max 3 passes) if future layouts add more draggable panels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/DraggableController.ts` around lines 231 - 263, The collision resolution in resolveCollisions(nr: DOMRect) does a single-pass over this._obstacleRects and can leave residual overlaps; modify resolveCollisions to loop up to a small bounded number of passes (e.g., max 3) re-checking all obstacles each pass and breaking early if this._offsetX and this._offsetY do not change between passes, so each iteration applies the same overlap logic but will converge against clustered obstacles.
🤖 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/client/graphics/layers/DraggablePanel.ts`:
- Around line 97-124: The two toolbar <button> elements in DraggablePanel's
template (the reset button that calls resetPosition() and uses
DraggablePanel.resetSvg, and the lock toggle button that calls toggleLock() and
switches between DraggablePanel.lockSvg / DraggablePanel.unlockSvg based on
this._locked) lack ARIA labels; add aria-label attributes to both buttons using
the same translated strings as their title props (e.g.
aria-label=${translateText("draggable_panel.reset_position")} for the reset
button and aria-label=${this._locked ?
translateText("draggable_panel.unlock_to_move") :
translateText("draggable_panel.lock_position")} for the lock button) so screen
readers will announce them.
---
Nitpick comments:
In `@src/client/graphics/DraggableController.ts`:
- Around line 231-263: The collision resolution in resolveCollisions(nr:
DOMRect) does a single-pass over this._obstacleRects and can leave residual
overlaps; modify resolveCollisions to loop up to a small bounded number of
passes (e.g., max 3) re-checking all obstacles each pass and breaking early if
this._offsetX and this._offsetY do not change between passes, so each iteration
applies the same overlap logic but will converge against clustered obstacles.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6036c1ba-43c1-4a41-8c37-1457d0acab2e
📒 Files selected for processing (4)
index.htmlsrc/client/graphics/DraggableController.tssrc/client/graphics/layers/DraggablePanel.tssrc/client/graphics/layers/PlayerInfoOverlay.ts
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME