Performance Overlay rework/redesign#3274
Merged
evanpelle merged 4 commits intoopenfrontio:mainfrom Feb 23, 2026
Merged
Conversation
Contributor
WalkthroughAdds gated frame profiling, per-frame and per-tick layer metrics with TPS and 60s average, a draggable/resizable performance overlay showing render/tick breakdowns and copyable snapshots, new localization keys, and emits a user-settings-changed event when settings change. Changes
Sequence DiagramsequenceDiagram
participant Renderer as GameRenderer
participant Profiler as FrameProfiler
participant Overlay as PerformanceOverlay
participant Settings as UserSettings
Renderer->>Profiler: isEnabled()?
alt profiling enabled
Profiler-->>Renderer: true
Renderer->>Renderer: accumulate per-frame render durations
Renderer->>Overlay: flush render metrics at tick start
Renderer->>Renderer: accumulate per-tick layer durations
Renderer->>Overlay: update tick-layer metrics after tick
else profiling disabled
Profiler-->>Renderer: false
Renderer->>Renderer: render/tick without profiling
end
Settings->>Settings: set(key, value)
Settings->>Settings: emitChange(key, value)
Settings->>Overlay: user-settings-changed event
Overlay->>Overlay: update UI / persisted state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (2)
src/client/graphics/layers/PerformanceOverlay.ts (2)
380-390: Global event listener is never removed.The
"user-settings-changed"listener added ininit()is never cleaned up. If the component is ever disconnected and reconnected, duplicate listeners will stack. Consider storing a reference and removing it indisconnectedCallback():Sketch
+ private onSettingsChanged = (event: Event) => { + const customEvent = event as CustomEvent<{ key?: string; value?: unknown }>; + if (customEvent.detail?.key !== "settings.performanceOverlay") return; + const nextVisible = customEvent.detail.value === true; + if (this.isVisible === nextVisible) return; + this.setVisible(nextVisible); + }; init() { ... - globalThis.addEventListener("user-settings-changed", (event: Event) => { - const customEvent = event as CustomEvent<{ key?: string; value?: unknown }>; - if (customEvent.detail?.key !== "settings.performanceOverlay") return; - const nextVisible = customEvent.detail.value === true; - if (this.isVisible === nextVisible) return; - this.setVisible(nextVisible); - }); + globalThis.addEventListener("user-settings-changed", this.onSettingsChanged); } + disconnectedCallback() { + super.disconnectedCallback(); + globalThis.removeEventListener("user-settings-changed", this.onSettingsChanged); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 380 - 390, The global "user-settings-changed" listener added in PerformanceOverlay.init() never gets removed, causing duplicate handlers when the component is reconnected; fix by capturing the listener callback in a property (e.g., this._onUserSettingsChanged or similar) when calling globalThis.addEventListener in init(), use that stored function instead of an inline lambda, and remove it in PerformanceOverlay.disconnectedCallback() via globalThis.removeEventListener("user-settings-changed", this._onUserSettingsChanged) so the handler is registered once and cleaned up on disconnect.
649-760: Three copies of the same EMA update pattern.
updateLayerStats,updateRenderPerTickMetrics, andupdateTickLayerMetricsall do the same work: iterate entries, compute EMA avg, track running max, accumulate total. A shared helper would reduce duplication:Sketch of shared helper
private updateEmaMap( statsMap: Map<string, { avg: number; max: number; last: number; total: number }>, durations: Record<string, number>, alpha: number = 0.2, ): { name: string; avg: number; max: number; total: number }[] { for (const [name, duration] of Object.entries(durations)) { const existing = statsMap.get(name); if (!existing) { statsMap.set(name, { avg: duration, max: duration, last: duration, total: duration }); } else { const avg = existing.avg + alpha * (duration - existing.avg); const max = Math.max(existing.max, duration); const total = existing.total + duration; statsMap.set(name, { avg, max, last: duration, total }); } } return Array.from(statsMap.entries()) .map(([name, s]) => ({ name, avg: s.avg, max: s.max, total: s.total })) .sort((a, b) => b.total - a.total); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 649 - 760, Multiple methods duplicate the EMA/max/total update logic; extract a private helper (e.g., updateEmaMap) that accepts a stats Map (this.layerStats / this.renderPerTickLayerStats / this.tickLayerStats), a durations Record, and optional alpha and performs the per-entry upsert (avg EMA, max, last, total) and returns the sorted breakdown array; then replace the bodies of updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics to call updateEmaMap (assigning its return to this.layerBreakdown or this.tickLayerBreakdown where those methods previously computed breakdowns) and keep the other per-method bookkeeping (renderLastTickFrameCount, renderLastTickLayerTotalMs, tickLayerLastCount, tickLayerLastTotalMs, etc.) intact.
🤖 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/PerformanceOverlay.ts`:
- Line 1038: The hardcoded tooltip strings for the collapse/expand button in
PerformanceOverlay.ts need i18n: replace the raw "Collapse" / "Expand" used in
the button title (driven by this.renderLayersExpanded) with calls to the
translation helper (translateText) so the title reads something like
translateText("Collapse") or translateText("Expand") based on
this.renderLayersExpanded; update both occurrences in the PerformanceOverlay
component where the title attribute is set and ensure any nearby tooltip usage
(same property) follows the same pattern.
- Around line 928-941: The render clamping allows clampedY to reach
viewportHeight (pushing the overlay fully off-screen) while the drag handler
caps Y at viewportHeight - 100; fix the clamping in PerformanceOverlay by using
the same upper bound as the drag handler: change the clamp for this.position.y
so it uses Math.min(this.position.y, viewportHeight - 100) instead of
Math.min(this.position.y, viewportHeight), keeping the existing Math.max(margin,
...) logic and referencing this.position.y and clampedY.
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 380-390: The global "user-settings-changed" listener added in
PerformanceOverlay.init() never gets removed, causing duplicate handlers when
the component is reconnected; fix by capturing the listener callback in a
property (e.g., this._onUserSettingsChanged or similar) when calling
globalThis.addEventListener in init(), use that stored function instead of an
inline lambda, and remove it in PerformanceOverlay.disconnectedCallback() via
globalThis.removeEventListener("user-settings-changed",
this._onUserSettingsChanged) so the handler is registered once and cleaned up on
disconnect.
- Around line 649-760: Multiple methods duplicate the EMA/max/total update
logic; extract a private helper (e.g., updateEmaMap) that accepts a stats Map
(this.layerStats / this.renderPerTickLayerStats / this.tickLayerStats), a
durations Record, and optional alpha and performs the per-entry upsert (avg EMA,
max, last, total) and returns the sorted breakdown array; then replace the
bodies of updateLayerStats, updateRenderPerTickMetrics, and
updateTickLayerMetrics to call updateEmaMap (assigning its return to
this.layerBreakdown or this.tickLayerBreakdown where those methods previously
computed breakdowns) and keep the other per-method bookkeeping
(renderLastTickFrameCount, renderLastTickLayerTotalMs, tickLayerLastCount,
tickLayerLastTotalMs, etc.) intact.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
fix the clamping in PerformanceOverlay by using the same upper bound as the drag handler Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/graphics/layers/PerformanceOverlay.ts (1)
1038-1038:⚠️ Potential issue | 🟡 MinorHardcoded
"Collapse"/"Expand"tooltip strings still bypass i18n.Despite the prior fix confirmation, both the render-layers collapse button (Line 1038) and the tick-layers collapse button (Line 1100) still use raw English literals instead of
translateText. Both need the same treatment:🌐 Proposed fix
- title=${this.renderLayersExpanded ? "Collapse" : "Expand"} + title=${this.renderLayersExpanded + ? translateText("performance_overlay.collapse") + : translateText("performance_overlay.expand")}- title=${this.tickLayersExpanded ? "Collapse" : "Expand"} + title=${this.tickLayersExpanded + ? translateText("performance_overlay.collapse") + : translateText("performance_overlay.expand")}Also applies to: 1100-1100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` at line 1038, The collapse/expand button tooltips in PerformanceOverlay.ts are using hardcoded English strings; update both occurrences (the render-layers button using this.renderLayersExpanded and the tick-layers button using the similar expanded flag) to call translateText and pass the appropriate key/text (e.g., translateText("Collapse") or translateText("Expand")) instead of raw literals so the title attribute becomes title={this.renderLayersExpanded ? translateText("Collapse") : translateText("Expand")} (and the analogous expression for the tick-layers control).
🧹 Nitpick comments (1)
src/client/graphics/layers/PerformanceOverlay.ts (1)
649-760: Identical EMA update pattern duplicated across three methods.
updateLayerStats,updateRenderPerTickMetrics, andupdateTickLayerMetricsall share the same Map-based EMA logic (init on first entry, thenavg = prev + α*(new - prev), running max and total). Extracting a small helper keeps the smoothing factor and logic in one place:♻️ Proposed refactor
+ private static updateEmaMap( + map: Map<string, { avg: number; max: number; last: number; total: number }>, + durations: Record<string, number>, + alpha = 0.2, + ): void { + for (const [name, duration] of Object.entries(durations)) { + const e = map.get(name); + if (!e) { + map.set(name, { avg: duration, max: duration, last: duration, total: duration }); + } else { + map.set(name, { + avg: e.avg + alpha * (duration - e.avg), + max: Math.max(e.max, duration), + last: duration, + total: e.total + duration, + }); + } + } + }Then each method becomes a single call:
- Object.entries(layerDurations).forEach(([name, duration]) => { - const existing = this.layerStats.get(name); - if (!existing) { ... } else { ... } - }); + PerformanceOverlay.updateEmaMap(this.layerStats, layerDurations);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 649 - 760, The EMA/map-update logic is duplicated in updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics; extract a single helper (e.g., updateStatsEntry or applyEmaToMap) that accepts the target Map (layerStats, renderPerTickLayerStats, tickLayerStats), the entry name, duration, and alpha and performs the init-or-update (avg, max, last, total) logic, then replace the repeated blocks in those three methods with calls to that helper to keep the smoothing factor and update behavior centralized.
🤖 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/PerformanceOverlay.ts`:
- Around line 380-390: The global "user-settings-changed" listener added inside
PerformanceOverlay.init() is created as an anonymous function and never removed,
causing leaks and duplicate listeners on repeated init(); fix by defining the
handler as a class field (e.g., this._onUserSettingsChanged = (event: Event) =>
{ ... } or a bound method) and use that field when calling
globalThis.addEventListener in init(); then remove it in
PerformanceOverlay.disconnectedCallback() via
globalThis.removeEventListener("user-settings-changed",
this._onUserSettingsChanged), and ensure init() avoids re-registering the same
handler (e.g., check if this._onUserSettingsChanged is already set or remove
before add).
---
Duplicate comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Line 1038: The collapse/expand button tooltips in PerformanceOverlay.ts are
using hardcoded English strings; update both occurrences (the render-layers
button using this.renderLayersExpanded and the tick-layers button using the
similar expanded flag) to call translateText and pass the appropriate key/text
(e.g., translateText("Collapse") or translateText("Expand")) instead of raw
literals so the title attribute becomes title={this.renderLayersExpanded ?
translateText("Collapse") : translateText("Expand")} (and the analogous
expression for the tick-layers control).
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 649-760: The EMA/map-update logic is duplicated in
updateLayerStats, updateRenderPerTickMetrics, and updateTickLayerMetrics;
extract a single helper (e.g., updateStatsEntry or applyEmaToMap) that accepts
the target Map (layerStats, renderPerTickLayerStats, tickLayerStats), the entry
name, duration, and alpha and performs the init-or-update (avg, max, last,
total) logic, then replace the repeated blocks in those three methods with calls
to that helper to keep the smoothing factor and update behavior centralized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/graphics/layers/PerformanceOverlay.ts (1)
1108-1115: Hardcoded"Collapse"/"Expand"tooltip strings still present.Both collapse buttons still use raw English strings for their
titleattributes despite a previous confirmed fix. These should usetranslateText(...).Also applies to: 1170-1177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 1108 - 1115, Replace the hardcoded English tooltip strings used in the collapse buttons with the translation helper: update the title binding on the button that uses renderLayersExpanded and toggleRenderLayersExpanded so it uses translateText("Collapse") when this.renderLayersExpanded is true and translateText("Expand") when false (same change for the duplicate button at the other occurrence); locate the button element (class "collapse-button", click handler toggleRenderLayersExpanded, and property renderLayersExpanded) and swap the raw "Collapse"/"Expand" strings for translateText(...) accordingly.
🧹 Nitpick comments (1)
src/client/graphics/layers/PerformanceOverlay.ts (1)
550-580: Direct DOM mutation of--overlay-widthis unsafe when position changes mid-resize.Lit compares against the previously bound value, not the live DOM value — so "if the DOM value changes, but the value set through lit-html bindings hasn't, lit-html won't know to update the DOM value and will leave it alone."
That means
style.setProperty("--overlay-width", …)survives Lit re-renders only whilethis.positionis unchanged. If a simultaneous touch-drag fires and updatesthis.position, Lit commits a new style string (with emptyoverlayWidthStylebecauseoverlayWidthPxis stillnull) viasetAttribute, wiping the CSS variable and snapping the overlay back to its default width mid-resize.Prefer updating
this.overlayWidthPxdirectly and drop the direct DOM write:♻️ Proposed refactor
private onResizePointerMove = (e: PointerEvent) => { if (!this.resizeState || e.pointerId !== this.resizeState.pointerId) return; const margin = 8; const viewportWidth = window.innerWidth; const left = Math.max(margin, Math.min(this.position.x, viewportWidth)); const maxWidthPx = Math.max(120, viewportWidth - left - margin); const minWidthPx = Math.min(260, maxWidthPx); const delta = e.clientX - this.resizeState.startClientX; const nextWidth = this.resizeState.startWidthPx + delta; const clamped = Math.max(minWidthPx, Math.min(maxWidthPx, nextWidth)); this.resizeState.pendingWidthPx = clamped; - const overlay = this.renderRoot.querySelector<HTMLElement>( - ".performance-overlay", - ); - overlay?.style.setProperty("--overlay-width", `${clamped}px`); + this.overlayWidthPx = clamped; }; private onResizePointerUp = (e: PointerEvent) => { if (!this.resizeState || e.pointerId !== this.resizeState.pointerId) return; globalThis.removeEventListener("pointermove", this.onResizePointerMove); globalThis.removeEventListener("pointerup", this.onResizePointerUp); globalThis.removeEventListener("pointercancel", this.onResizePointerUp); - this.overlayWidthPx = this.resizeState.pendingWidthPx; this.resizeState = null; this.requestUpdate(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 550 - 580, The direct DOM write in onResizePointerMove (overlay?.style.setProperty("--overlay-width", …)) is unsafe; instead of mutating the element, update the component state so Lit's bindings stay authoritative: set this.overlayWidthPx = clamped (in addition to this.resizeState.pendingWidthPx) and call this.requestUpdate() so the bound overlayWidthStyle is updated during the drag; remove the renderRoot.querySelector style.setProperty call entirely so onResizePointerMove and onResizePointerUp consistently rely on overlayWidthPx and resizeState.
🤖 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/PerformanceOverlay.ts`:
- Around line 1078-1083: The color calculation uses 1000 / this.frameTime which
is Infinity when frameTime === 0, causing getPerformanceColor(Infinity) to
return "performance-good"; update the render logic in PerformanceOverlay so you
compute a safe fps value (e.g. const fps = this.frameTime > 0 ? 1000 /
this.frameTime : 0) or choose a neutral class when this.frameTime === 0, then
pass that safe value to getPerformanceColor and render the ms label unchanged
(this.frameTime) so the initial "0ms" shows a neutral style instead of green;
reference getPerformanceColor and the frameTime property in the component.
---
Duplicate comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 1108-1115: Replace the hardcoded English tooltip strings used in
the collapse buttons with the translation helper: update the title binding on
the button that uses renderLayersExpanded and toggleRenderLayersExpanded so it
uses translateText("Collapse") when this.renderLayersExpanded is true and
translateText("Expand") when false (same change for the duplicate button at the
other occurrence); locate the button element (class "collapse-button", click
handler toggleRenderLayersExpanded, and property renderLayersExpanded) and swap
the raw "Collapse"/"Expand" strings for translateText(...) accordingly.
---
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 550-580: The direct DOM write in onResizePointerMove
(overlay?.style.setProperty("--overlay-width", …)) is unsafe; instead of
mutating the element, update the component state so Lit's bindings stay
authoritative: set this.overlayWidthPx = clamped (in addition to
this.resizeState.pendingWidthPx) and call this.requestUpdate() so the bound
overlayWidthStyle is updated during the drag; remove the
renderRoot.querySelector style.setProperty call entirely so onResizePointerMove
and onResizePointerUp consistently rely on overlayWidthPx and resizeState.
evanpelle
approved these changes
Feb 23, 2026
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
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
3 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.
Description:
updates the Performance Overlay to be more usable (draggable/resizable/scrollable), adds tick-level metrics (TPS + per-layer tick timings), and reduces overhead when the overlay is hidden.
UI/UX
New metrics
Performance / overhead
GameRendereronly callsFrameProfiler.clear()/consume()and per-layerstart/endwhen profiling is enabled.Settings plumbing
UserSettingsnow dispatches auser-settings-changedCustomEventonset()/setFloat().settings.performanceOverlaychanges so visibility stays in sync even when toggled outside the overlay.Implementation notes (by file)
src/client/graphics/layers/PerformanceOverlay.tssrc/client/graphics/GameRenderer.tsFrameProfiler.isEnabled().updateRenderPerTickMetrics(...).tick()call) and publishes them viaupdateTickLayerMetrics(...).src/core/game/UserSettings.tsemitChange(key, value)to dispatchuser-settings-changedtoglobalThis(best-effort).resources/lang/en.jsonperformance_overlay.*strings for TPS and the new render/tick layer sections.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME