Use custom attack ratio slider for consistent cross-browser rendering#3422
Closed
hkio120 wants to merge 5 commits intoopenfrontio:mainfrom
Closed
Use custom attack ratio slider for consistent cross-browser rendering#3422hkio120 wants to merge 5 commits intoopenfrontio:mainfrom
hkio120 wants to merge 5 commits intoopenfrontio:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced native range input sliders with fluent-slider controls in the ControlPanel, introducing a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorPipeline reports formatting issues.
CI flagged code style issues. Run
npx prettier --write src/client/graphics/layers/ControlPanel.tsto fix before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` at line 1, CI flagged formatting issues in ControlPanel.ts; run the formatter and commit the result: run `npx prettier --write src/client/graphics/layers/ControlPanel.ts` (or your repo-wide prettier command), review the formatted changes in the ControlPanel class (the file that imports LitElement and html and likely declares the ControlPanel component), stage and push the updated file so the import/formatting problems are resolved for the PR.
🤖 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/ControlPanel.ts`:
- Around line 176-188: The custom slider div (used with handlers like
handleRatioSliderPointerDown, handleRatioSliderPointerMove,
handleRatioSliderPointerUp and state attackRatio/setAttackRatio) lacks keyboard
accessibility—add tabindex="0" to the slider element and implement a keydown
handler (e.g., handleRatioSliderKeyDown) wired via `@keydown` that handles
ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Home/End to call setAttackRatio with
appropriate steps (use Shift for larger steps), preventDefault on handled keys,
and ensure aria-valuenow updates when attackRatio changes.
---
Outside diff comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Line 1: CI flagged formatting issues in ControlPanel.ts; run the formatter and
commit the result: run `npx prettier --write
src/client/graphics/layers/ControlPanel.ts` (or your repo-wide prettier
command), review the formatted changes in the ControlPanel class (the file that
imports LitElement and html and likely declares the ControlPanel component),
stage and push the updated file so the import/formatting problems are resolved
for the PR.
🪄 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: 45878400-dc03-4fbe-b240-5ca97cdd3856
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.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: 2
🤖 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/ControlPanel.ts`:
- Line 207: Replace the hardcoded aria-label "Attack ratio" with a localized
string via translateText(), i.e. call translateText with a new key (e.g.
"attackRatio") where the aria-label is set (the Slider/element in
ControlPanel.ts that currently has aria-label="Attack ratio"), and add that key
with the English value "Attack ratio" into en.json so the i18n system picks it
up.
- Around line 127-133: The setAttackRatio method must reject non-finite inputs
and avoid redundant side effects: first check Number.isFinite(newRatio) and
return early if not to prevent persisting "NaN"; compute clamped = Math.min(1,
Math.max(0.01, newRatio)) and compare to this.attackRatio, and if unchanged
return early to avoid unnecessary localStorage.setItem("settings.attackRatio",
...), this.onAttackRatioChange(...) and this.requestUpdate(); only when the
clamped value is finite and different from the current this.attackRatio, assign
it, persist the stringified value, call onAttackRatioChange(this.attackRatio)
and requestUpdate().
🪄 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: f460ae9c-789f-4998-ad6a-d1d4d8f7a78f
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts
Comment on lines
+127
to
+133
| private setAttackRatio(newRatio: number) { | ||
| const clamped = Math.min(1, Math.max(0.01, newRatio)); | ||
| this.attackRatio = clamped; | ||
| localStorage.setItem("settings.attackRatio", String(this.attackRatio)); | ||
| this.onAttackRatioChange(this.attackRatio); | ||
| this.requestUpdate(); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard setAttackRatio against non-finite input and redundant writes.
Line 128 clamps range, but NaN still passes through as NaN, then Line 130 persists "NaN" and UI math can break. Also, unchanged values still trigger storage/event/update churn.
Suggested hardening
private setAttackRatio(newRatio: number) {
- const clamped = Math.min(1, Math.max(0.01, newRatio));
- this.attackRatio = clamped;
- localStorage.setItem("settings.attackRatio", String(this.attackRatio));
- this.onAttackRatioChange(this.attackRatio);
- this.requestUpdate();
+ const safeRatio = Number.isFinite(newRatio) ? newRatio : 0.2;
+ const clamped = Math.min(1, Math.max(0.01, safeRatio));
+ if (clamped === this.attackRatio) return;
+ this.attackRatio = clamped;
+ localStorage.setItem("settings.attackRatio", String(clamped));
+ this.onAttackRatioChange(clamped);
+ this.requestUpdate();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` around lines 127 - 133, The
setAttackRatio method must reject non-finite inputs and avoid redundant side
effects: first check Number.isFinite(newRatio) and return early if not to
prevent persisting "NaN"; compute clamped = Math.min(1, Math.max(0.01,
newRatio)) and compare to this.attackRatio, and if unchanged return early to
avoid unnecessary localStorage.setItem("settings.attackRatio", ...),
this.onAttackRatioChange(...) and this.requestUpdate(); only when the clamped
value is finite and different from the current this.attackRatio, assign it,
persist the stringified value, call onAttackRatioChange(this.attackRatio) and
requestUpdate().
| class="relative w-full ${heightClass} rounded-full bg-slate-700/70 border border-slate-300/25 shadow-inner cursor-pointer touch-none select-none" | ||
| role="slider" | ||
| tabindex="0" | ||
| aria-label="Attack ratio" |
Contributor
There was a problem hiding this comment.
Localize the slider accessibility label.
Line 207 uses a hardcoded English string ("Attack ratio"). This should go through translateText() and be added to en.json to match the repo i18n pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/ControlPanel.ts` at line 207, Replace the
hardcoded aria-label "Attack ratio" with a localized string via translateText(),
i.e. call translateText with a new key (e.g. "attackRatio") where the aria-label
is set (the Slider/element in ControlPanel.ts that currently has
aria-label="Attack ratio"), and add that key with the English value "Attack
ratio" into en.json so the i18n system picks it up.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/ControlPanel.ts (1)
53-57:⚠️ Potential issue | 🟠 MajorInitialize attack ratio via the validated setter.
Line 53-Line 56 reads from
localStorageand writes directly touiStatewithout validation. If storage containsNaNor out-of-range values, attack calculations can start invalid.Suggested fix
init() { - this.attackRatio = Number( - localStorage.getItem("settings.attackRatio") ?? "0.2", - ); - this.uiState.attackRatio = this.attackRatio; + const storedRatio = Number( + localStorage.getItem("settings.attackRatio") ?? "0.2", + ); + this.setAttackRatio(Number.isFinite(storedRatio) ? storedRatio : 0.2); this.eventBus.on(AttackRatioEvent, (event) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 53 - 57, The code reads settings.attackRatio from localStorage and writes it directly to uiState.attackRatio without validation; instead parse the stored value (fallback "0.2") and assign it to the validated property this.attackRatio so the class setter can clamp/validate and update uiState appropriately (i.e., replace direct assignment this.uiState.attackRatio = ... with this.attackRatio = parsedValue and keep the existing eventBus.on(AttackRatioEvent, ...) hookup unchanged).
♻️ Duplicate comments (3)
src/client/graphics/layers/ControlPanel.ts (3)
127-133:⚠️ Potential issue | 🟠 MajorHarden
setAttackRatiofor non-finite input and no-op updates.Line 128 clamp does not protect against
NaN(clampedstaysNaN), and unchanged values still trigger storage/event/render churn.Suggested hardening
private setAttackRatio(newRatio: number) { + if (!Number.isFinite(newRatio)) return; const clamped = Math.min(1, Math.max(0.01, newRatio)); + if (clamped === this.attackRatio) return; this.attackRatio = clamped; localStorage.setItem("settings.attackRatio", String(this.attackRatio)); this.onAttackRatioChange(this.attackRatio); this.requestUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 127 - 133, Harden setAttackRatio to handle non-finite inputs and avoid no-op work: first coerce newRatio into a finite clamped value (e.g., if !isFinite(newRatio) or isNaN(newRatio) treat as the min 0.01), then clamp between 0.01 and 1; if the resulting clamped value equals this.attackRatio, return early to skip localStorage.setItem("settings.attackRatio"), this.onAttackRatioChange, and this.requestUpdate(); otherwise assign this.attackRatio, persist the setting, call this.onAttackRatioChange(this.attackRatio) and this.requestUpdate().
177-188:⚠️ Potential issue | 🟠 MajorAdd keyboard support to keep slider accessible.
With
role="slider"but notabindexand no keyboard handler, keyboard-only users cannot adjust this control.Suggested keyboard wiring
<div class="relative w-full ${heightClass} rounded-full bg-slate-700/70 border border-slate-300/25 shadow-inner cursor-pointer touch-none select-none" role="slider" + tabindex="0" aria-label="Attack ratio" aria-valuemin="1" aria-valuemax="100" aria-valuenow=${Math.round(ratioPercent)} `@pointerdown`=${this.handleRatioSliderPointerDown} `@pointermove`=${this.handleRatioSliderPointerMove} `@pointerup`=${this.handleRatioSliderPointerUp} `@pointercancel`=${this.handleRatioSliderPointerUp} + `@keydown`=${this.handleRatioSliderKeyDown} >private handleRatioSliderKeyDown = (e: KeyboardEvent) => { const step = e.shiftKey ? 0.1 : 0.01; if (e.key === "ArrowRight" || e.key === "ArrowUp") { this.setAttackRatio(this.attackRatio + step); e.preventDefault(); } else if (e.key === "ArrowLeft" || e.key === "ArrowDown") { this.setAttackRatio(this.attackRatio - step); e.preventDefault(); } else if (e.key === "Home") { this.setAttackRatio(0.01); e.preventDefault(); } else if (e.key === "End") { this.setAttackRatio(1); e.preventDefault(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` around lines 177 - 188, The slider markup lacks keyboard focus and handlers: make the slider element focusable by adding tabindex="0" and wire a keydown handler (e.g., `@keydown`=${this.handleRatioSliderKeyDown}); implement a private method handleRatioSliderKeyDown that reads e.key (ArrowRight/ArrowUp to increase, ArrowLeft/ArrowDown to decrease, Home to set 0.01, End to set 1), uses a larger step when e.shiftKey is true, calls the existing setAttackRatio(this.attackRatio ± step) accordingly, and calls e.preventDefault() for handled keys so keyboard users can adjust the control while keeping the existing pointer handlers (handleRatioSliderPointerDown/Move/Up) unchanged.
180-180:⚠️ Potential issue | 🟡 MinorLocalize the slider ARIA label.
Line 180 uses a hardcoded English label. Please route this through
translateText()and add the key toen.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/ControlPanel.ts` at line 180, Replace the hardcoded aria-label "Attack ratio" in ControlPanel.ts with a call to translateText(), e.g. translateText('control.attackRatio'), and add the corresponding "control.attackRatio": "Attack ratio" entry to en.json so the label is localized; ensure the same translation key is used where aria-label is set and that translateText is imported/available in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 53-57: The code reads settings.attackRatio from localStorage and
writes it directly to uiState.attackRatio without validation; instead parse the
stored value (fallback "0.2") and assign it to the validated property
this.attackRatio so the class setter can clamp/validate and update uiState
appropriately (i.e., replace direct assignment this.uiState.attackRatio = ...
with this.attackRatio = parsedValue and keep the existing
eventBus.on(AttackRatioEvent, ...) hookup unchanged).
---
Duplicate comments:
In `@src/client/graphics/layers/ControlPanel.ts`:
- Around line 127-133: Harden setAttackRatio to handle non-finite inputs and
avoid no-op work: first coerce newRatio into a finite clamped value (e.g., if
!isFinite(newRatio) or isNaN(newRatio) treat as the min 0.01), then clamp
between 0.01 and 1; if the resulting clamped value equals this.attackRatio,
return early to skip localStorage.setItem("settings.attackRatio"),
this.onAttackRatioChange, and this.requestUpdate(); otherwise assign
this.attackRatio, persist the setting, call
this.onAttackRatioChange(this.attackRatio) and this.requestUpdate().
- Around line 177-188: The slider markup lacks keyboard focus and handlers: make
the slider element focusable by adding tabindex="0" and wire a keydown handler
(e.g., `@keydown`=${this.handleRatioSliderKeyDown}); implement a private method
handleRatioSliderKeyDown that reads e.key (ArrowRight/ArrowUp to increase,
ArrowLeft/ArrowDown to decrease, Home to set 0.01, End to set 1), uses a larger
step when e.shiftKey is true, calls the existing setAttackRatio(this.attackRatio
± step) accordingly, and calls e.preventDefault() for handled keys so keyboard
users can adjust the control while keeping the existing pointer handlers
(handleRatioSliderPointerDown/Move/Up) unchanged.
- Line 180: Replace the hardcoded aria-label "Attack ratio" in ControlPanel.ts
with a call to translateText(), e.g. translateText('control.attackRatio'), and
add the corresponding "control.attackRatio": "Attack ratio" entry to en.json so
the label is localized; ensure the same translation key is used where aria-label
is set and that translateText is imported/available in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6d8c44e-5ebb-4baf-b73d-7dc9e88e9bf8
📒 Files selected for processing (1)
src/client/graphics/layers/ControlPanel.ts
evanpelle
reviewed
Mar 19, 2026
| } | ||
| }; | ||
|
|
||
| private renderCustomAttackRatioSlider(compact: boolean = false) { |
Collaborator
There was a problem hiding this comment.
Can you make the slider its own lit element? Then we can reuse it for other things
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/components/InlineSlider.ts (1)
100-108: Consider a separate “commit” event for expensive side effectsRight now
value-changedis emitted during drag. Insrc/client/graphics/layers/ControlPanel.ts:127-133, each change is persisted tolocalStorage, which is synchronous. A dedicated commit event on pointer-up can reduce UI-thread churn.Possible pattern
+ private emitValueCommitted(value: number) { + this.dispatchEvent( + new CustomEvent("value-committed", { + detail: { value }, + bubbles: true, + composed: true, + }), + ); + } private handlePointerUp = (e: PointerEvent) => { if (this.draggingPointerId !== e.pointerId) return; this.draggingPointerId = null; const sliderTrack = e.currentTarget as HTMLElement; if (sliderTrack.hasPointerCapture(e.pointerId)) { sliderTrack.releasePointerCapture(e.pointerId); } + this.emitValueCommitted(this.clampedValue); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/InlineSlider.ts` around lines 100 - 108, The slider currently emits value changes during dragging which causes synchronous expensive side effects (e.g., localStorage writes); modify the slider component so that handlePointerUp (the method named handlePointerUp) emits a distinct commit event (e.g., "value-commit" or "value-change-commit") when dragging ends, while keeping the existing lightweight "value-changed" event for continuous UI updates during handlePointerMove/drag; update any consumers (such as the handler in ControlPanel.ts that currently reacts to "value-changed") to perform synchronous persistence only in the new commit event handler so heavy work runs once on pointer-up rather than on every drag event.
🤖 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/components/InlineSlider.ts`:
- Around line 82-90: The pointer-down handler currently calls
e.preventDefault(), which can block the element from receiving focus and break
keyboard interactions; in handlePointerDown replace the preventDefault call with
an explicit focus on the slider element: after
sliderTrack.setPointerCapture(e.pointerId) and this.updateValueFromPointer(e,
sliderTrack) call sliderTrack.focus() (optionally
sliderTrack.focus({preventScroll: true})) and remove e.preventDefault(); keep
the pointer capture and draggingPointerId logic as-is so pointer tracking works
while restoring keyboard focus for updateValueFromPointer-driven interactions.
- Around line 39-54: normalizeValue can still return max when max <= min,
causing inconsistent state with clampedValue; modify normalizeValue in the
InlineSlider class to detect an invalid range (this.max <= this.min) early and
return the same canonical value as clampedValue (or this.min) instead of
performing the clamp/snapping logic. Specifically, at the top of normalizeValue
(after the Number.isFinite check) check if this.max <= this.min and return
this.clampedValue (or this.min) so snapping/step logic (steps, snapped, toFixed
using this.stepPrecision) is never applied for an invalid range, ensuring
emitted values match clampedValue.
---
Nitpick comments:
In `@src/client/components/InlineSlider.ts`:
- Around line 100-108: The slider currently emits value changes during dragging
which causes synchronous expensive side effects (e.g., localStorage writes);
modify the slider component so that handlePointerUp (the method named
handlePointerUp) emits a distinct commit event (e.g., "value-commit" or
"value-change-commit") when dragging ends, while keeping the existing
lightweight "value-changed" event for continuous UI updates during
handlePointerMove/drag; update any consumers (such as the handler in
ControlPanel.ts that currently reacts to "value-changed") to perform synchronous
persistence only in the new commit event handler so heavy work runs once on
pointer-up rather than on every drag event.
🪄 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: 25d86cf5-1034-4a96-863b-738e811d5b7a
📒 Files selected for processing (2)
src/client/components/InlineSlider.tssrc/client/graphics/layers/ControlPanel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/ControlPanel.ts
Comment on lines
+39
to
+54
| private normalizeValue(rawValue: number) { | ||
| if (!Number.isFinite(rawValue)) { | ||
| return this.clampedValue; | ||
| } | ||
|
|
||
| const clamped = Math.min(this.max, Math.max(this.min, rawValue)); | ||
| if (this.step <= 0) return clamped; | ||
|
|
||
| const steps = Math.round((clamped - this.min) / this.step); | ||
| const snapped = this.min + steps * this.step; | ||
| return Number( | ||
| Math.min(this.max, Math.max(this.min, snapped)).toFixed( | ||
| this.stepPrecision, | ||
| ), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle invalid range config consistently in normalization
When max <= min, clampedValue returns min (Line 22), but normalizeValue can still produce max (Line 44). This creates inconsistent internal state and emitted values.
Suggested fix
private normalizeValue(rawValue: number) {
+ if (this.max <= this.min) {
+ return this.min;
+ }
if (!Number.isFinite(rawValue)) {
return this.clampedValue;
}
const clamped = Math.min(this.max, Math.max(this.min, rawValue));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private normalizeValue(rawValue: number) { | |
| if (!Number.isFinite(rawValue)) { | |
| return this.clampedValue; | |
| } | |
| const clamped = Math.min(this.max, Math.max(this.min, rawValue)); | |
| if (this.step <= 0) return clamped; | |
| const steps = Math.round((clamped - this.min) / this.step); | |
| const snapped = this.min + steps * this.step; | |
| return Number( | |
| Math.min(this.max, Math.max(this.min, snapped)).toFixed( | |
| this.stepPrecision, | |
| ), | |
| ); | |
| } | |
| private normalizeValue(rawValue: number) { | |
| if (this.max <= this.min) { | |
| return this.min; | |
| } | |
| if (!Number.isFinite(rawValue)) { | |
| return this.clampedValue; | |
| } | |
| const clamped = Math.min(this.max, Math.max(this.min, rawValue)); | |
| if (this.step <= 0) return clamped; | |
| const steps = Math.round((clamped - this.min) / this.step); | |
| const snapped = this.min + steps * this.step; | |
| return Number( | |
| Math.min(this.max, Math.max(this.min, snapped)).toFixed( | |
| this.stepPrecision, | |
| ), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/InlineSlider.ts` around lines 39 - 54, normalizeValue
can still return max when max <= min, causing inconsistent state with
clampedValue; modify normalizeValue in the InlineSlider class to detect an
invalid range (this.max <= this.min) early and return the same canonical value
as clampedValue (or this.min) instead of performing the clamp/snapping logic.
Specifically, at the top of normalizeValue (after the Number.isFinite check)
check if this.max <= this.min and return this.clampedValue (or this.min) so
snapping/step logic (steps, snapped, toFixed using this.stepPrecision) is never
applied for an invalid range, ensuring emitted values match clampedValue.
Comment on lines
+82
to
+90
| private handlePointerDown = (e: PointerEvent) => { | ||
| const sliderTrack = e.currentTarget as HTMLElement; | ||
| if (e.button !== 0 && e.pointerType === "mouse") return; | ||
|
|
||
| this.draggingPointerId = e.pointerId; | ||
| sliderTrack.setPointerCapture(e.pointerId); | ||
| this.updateValueFromPointer(e, sliderTrack); | ||
| e.preventDefault(); | ||
| }; |
Contributor
There was a problem hiding this comment.
Focus slider on pointer interaction
preventDefault() on pointer down can block normal focus behavior in some browsers. After clicking/dragging, keyboard arrows may not work until the user tabs back.
Suggested fix
private handlePointerDown = (e: PointerEvent) => {
const sliderTrack = e.currentTarget as HTMLElement;
if (e.button !== 0 && e.pointerType === "mouse") return;
+ sliderTrack.focus({ preventScroll: true });
this.draggingPointerId = e.pointerId;
sliderTrack.setPointerCapture(e.pointerId);
this.updateValueFromPointer(e, sliderTrack);
e.preventDefault();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private handlePointerDown = (e: PointerEvent) => { | |
| const sliderTrack = e.currentTarget as HTMLElement; | |
| if (e.button !== 0 && e.pointerType === "mouse") return; | |
| this.draggingPointerId = e.pointerId; | |
| sliderTrack.setPointerCapture(e.pointerId); | |
| this.updateValueFromPointer(e, sliderTrack); | |
| e.preventDefault(); | |
| }; | |
| private handlePointerDown = (e: PointerEvent) => { | |
| const sliderTrack = e.currentTarget as HTMLElement; | |
| if (e.button !== 0 && e.pointerType === "mouse") return; | |
| sliderTrack.focus({ preventScroll: true }); | |
| this.draggingPointerId = e.pointerId; | |
| sliderTrack.setPointerCapture(e.pointerId); | |
| this.updateValueFromPointer(e, sliderTrack); | |
| e.preventDefault(); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/components/InlineSlider.ts` around lines 82 - 90, The pointer-down
handler currently calls e.preventDefault(), which can block the element from
receiving focus and break keyboard interactions; in handlePointerDown replace
the preventDefault call with an explicit focus on the slider element: after
sliderTrack.setPointerCapture(e.pointerId) and this.updateValueFromPointer(e,
sliderTrack) call sliderTrack.focus() (optionally
sliderTrack.focus({preventScroll: true})) and remove e.preventDefault(); keep
the pointer capture and draggingPointerId logic as-is so pointer tracking works
while restoring keyboard focus for updateValueFromPointer-driven interactions.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/client/components/FluentSlider.ts (2)
98-102: Long class strings hurt readability.These Tailwind class strings are very long (lines 99-102). Consider extracting them into named constants or a helper method for easier maintenance.
♻️ Example: Extract class strings
// At module level or as static properties: const SLIDER_CLASS_INLINE_COMPACT = `w-full h-4 p-0 m-0 bg-transparent appearance-none cursor-pointer...`; const SLIDER_CLASS_INLINE_REGULAR = `w-full h-5 p-0 m-0 bg-transparent appearance-none cursor-pointer...`; const SLIDER_CLASS_DEFAULT = `w-full h-6 p-0 m-0 bg-transparent appearance-none cursor-pointer...`; // In renderSliderInput(): const sliderClass = this.inline ? this.compact ? SLIDER_CLASS_INLINE_COMPACT : SLIDER_CLASS_INLINE_REGULAR : SLIDER_CLASS_DEFAULT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FluentSlider.ts` around lines 98 - 102, The long inline Tailwind class strings assigned to sliderClass in renderSliderInput make the code hard to read and maintain; extract each variant into clearly named module-level constants (e.g., SLIDER_CLASS_INLINE_COMPACT, SLIDER_CLASS_INLINE_REGULAR, SLIDER_CLASS_DEFAULT) and replace the current ternary expression that sets sliderClass with references to those constants, keeping the existing condition logic (this.inline, this.compact) and leaving renderSliderInput and sliderClass usage unchanged.
85-89: Consider localizing the fallback "Slider" string.The fallback value
"Slider"is hardcoded in English. For i18n consistency, consider using a translation key liketranslateText("slider.default_label").🌐 Proposed localization fix
private get sliderAriaLabel() { if (this.ariaLabel) return this.ariaLabel; if (this.labelKey) return translateText(this.labelKey); - return "Slider"; + return translateText("slider.default_label"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/components/FluentSlider.ts` around lines 85 - 89, The getter sliderAriaLabel in FluentSlider currently hardcodes the English fallback "Slider"; change the fallback to use the translation helper by returning translateText("slider.default_label") instead of the literal string. Update the sliderAriaLabel getter (which checks this.ariaLabel and this.labelKey) to call translateText for the default case so all three branches use translateText consistently; ensure the translation key "slider.default_label" is used so i18n tooling can pick it up.tests/client/components/FluentSlider.test.ts (2)
282-305: Consider adding symmetric test:changeshould NOT dispatch whenlive=true.The test verifies
inputdispatches whenlive=true, but does not verify thatchangedoes NOT dispatch in that mode. Adding this would make the behavior contract clearer.🧪 Proposed additional test
it("should NOT dispatch value-changed on change when live mode is enabled", async () => { slider.live = true; await slider.updateComplete; const eventSpy = vi.fn(); slider.addEventListener("value-changed", eventSpy); const rangeInput = slider.querySelector( 'input[type="range"]', ) as HTMLInputElement; rangeInput.valueAsNumber = 0.5; rangeInput.dispatchEvent(new Event("change", { bubbles: true })); await slider.updateComplete; expect(eventSpy).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/components/FluentSlider.test.ts` around lines 282 - 305, Add a symmetric test that verifies the "change" event does NOT trigger "value-changed" when live mode is enabled: create a new it block named something like "should NOT dispatch value-changed on change when live mode is enabled", set slider.live = true and await slider.updateComplete, attach an eventSpy via slider.addEventListener("value-changed", eventSpy), find the range input (rangeInput) and set rangeInput.valueAsNumber then dispatch a "change" event with bubbles, await slider.updateComplete again and assert eventSpy was not called; reference the existing variables/event names (slider, eventSpy, rangeInput) and mirror the patterns used in the "input" test for consistency.
323-324: Minor: Missing blank line between describe blocks.For consistency with the rest of the file, add a blank line after the closing
});of "Inline and Live Modes".}); + describe("Edge Cases", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/client/components/FluentSlider.test.ts` around lines 323 - 324, Add a single blank line between the closing "});" of the "Inline and Live Modes" describe block and the next describe("Edge Cases", ...) block to match file consistency; locate the end of the Inline and Live Modes block (the closing });) in FluentSlider.test.ts and insert one empty line before the describe("Edge Cases") declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/components/FluentSlider.ts`:
- Around line 98-102: The long inline Tailwind class strings assigned to
sliderClass in renderSliderInput make the code hard to read and maintain;
extract each variant into clearly named module-level constants (e.g.,
SLIDER_CLASS_INLINE_COMPACT, SLIDER_CLASS_INLINE_REGULAR, SLIDER_CLASS_DEFAULT)
and replace the current ternary expression that sets sliderClass with references
to those constants, keeping the existing condition logic (this.inline,
this.compact) and leaving renderSliderInput and sliderClass usage unchanged.
- Around line 85-89: The getter sliderAriaLabel in FluentSlider currently
hardcodes the English fallback "Slider"; change the fallback to use the
translation helper by returning translateText("slider.default_label") instead of
the literal string. Update the sliderAriaLabel getter (which checks
this.ariaLabel and this.labelKey) to call translateText for the default case so
all three branches use translateText consistently; ensure the translation key
"slider.default_label" is used so i18n tooling can pick it up.
In `@tests/client/components/FluentSlider.test.ts`:
- Around line 282-305: Add a symmetric test that verifies the "change" event
does NOT trigger "value-changed" when live mode is enabled: create a new it
block named something like "should NOT dispatch value-changed on change when
live mode is enabled", set slider.live = true and await slider.updateComplete,
attach an eventSpy via slider.addEventListener("value-changed", eventSpy), find
the range input (rangeInput) and set rangeInput.valueAsNumber then dispatch a
"change" event with bubbles, await slider.updateComplete again and assert
eventSpy was not called; reference the existing variables/event names (slider,
eventSpy, rangeInput) and mirror the patterns used in the "input" test for
consistency.
- Around line 323-324: Add a single blank line between the closing "});" of the
"Inline and Live Modes" describe block and the next describe("Edge Cases", ...)
block to match file consistency; locate the end of the Inline and Live Modes
block (the closing });) in FluentSlider.test.ts and insert one empty line before
the describe("Edge Cases") declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b0c5173-024e-43c1-af97-9a531059651c
📒 Files selected for processing (3)
src/client/components/FluentSlider.tssrc/client/graphics/layers/ControlPanel.tstests/client/components/FluentSlider.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/ControlPanel.ts
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
|
This pull request has been closed because fourteen days have passed without activity. If you want to continue work on the code, comment or create a new PR. |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
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
2 participants
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)
It was an issue but Idk the number
Bug: the attack-ratio slider looked and behaved differently across browsers (Firefox vs Chrome/Opera GX) due to native input[type=range] rendering differences.
Fix: replaced the native range input with a custom slider (track + thumb + pointer handling), using a single shared style and interaction model to ensure consistent visuals and behavior across browsers.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
HulKiora
How it looked in Opera GX:
How it look after the fix :