Conversation
…ability to toggle all charts, align actions right
There was a problem hiding this comment.
Pull request overview
Merges current dev work into main, introducing a new “latest stats” storage path for faster stats retrieval, plus substantial UI refresh and client-side caching improvements for the dashboard experience.
Changes:
- Add a
LatestStatsMongo model and update the ping job to upsert latest/daily peak/record stats per server. - Switch
/api/stats/latestto read fromLatestStatsinstead of aggregating from historicalPings. - Refresh UI styling and UX (new global shell styling, landing page redesign, dashboard loading/caching states, prediction chart refactor).
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/models/Pings.ts | Adds an index on timestamp for more efficient range queries. |
| backend/src/models/LatestStats.ts | Introduces a new collection to store per-server latest/daily/record stats. |
| backend/src/jobs/PingServerJob.ts | Writes/upserts LatestStats during ping collection. |
| backend/src/api/routes/Stats.ts | Updates /latest endpoint to use LatestStats instead of aggregation. |
| app/styles/globals.css | Adds global font + improved base styling and motion reduction handling. |
| app/pages/index.tsx | Redesigns the landing/server selection UI and improves empty state + accessibility. |
| app/pages/dashboard/index.tsx | Adds caching via Preferences, loading states, auth error UI, and range UX improvements. |
| app/pages/_document.tsx | Updates body classes to align with new app shell styling. |
| app/pages/_app.tsx | Wraps app in new .app-shell layout styling. |
| app/components/server/AddServer.tsx | Updates modal layout/inputs for improved UX consistency. |
| app/components/charts/ServerTable.tsx | Adds toggle-all chart control, cached-state behavior, prediction chart refactor, pagination UX updates. |
| app/components/charts/PredictionChart.tsx | Adds a Chart.js-based prediction chart with zoom/reset support. |
| app/components/charts/OnlinePlayersChart.tsx | Adds 15s bucketing + tooltip refinements and interaction tweaks. |
| .gitignore | Adds ignore rule for .next/. |
| .claude/settings.local.json | Adds a local Claude settings file (should not be committed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a25249232
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
backend/src/api/routes/Stats.ts:173
- The refactored /latest endpoint is much more efficient by querying the new LatestStats collection instead of performing complex aggregations on the Pings collection. However, there's no validation that the LatestStats collection is properly populated. If the PingServerJob hasn't run yet or fails, this endpoint will return empty data without indicating why. Consider adding a health check or fallback logic.
const latestStats = await LatestStats.find().lean();
const data = [] as any;
for (const entry of latestStats) {
if (serverNames[entry.serverId] == null) continue;
data.push({
internalId: entry.serverId,
server: serverNames[entry.serverId] || entry.serverId,
playerCount: entry.latestCount,
dailyPeak: entry.dailyPeak,
dailyPeakTimestamp: entry.dailyPeakTimestamp,
record: entry.record,
recordTimestamp: entry.recordTimestamp,
outdated: (currentMillis - entry.latestTimestamp) > (parseInt(process.env.ping_rate as string) * 2)
});
}
res.json(data);
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const value = await fn(id); | ||
| results.set(id, value); |
There was a problem hiding this comment.
If fn(id) throws inside runWithConcurrencyLimit, the worker rejects and Promise.all(workers) aborts pingAll(), preventing LatestStats updates for all servers. Consider wrapping fn(id) in try/catch (or using Promise.allSettled) so one transient aggregation error doesn’t break the whole job.
| const value = await fn(id); | |
| results.set(id, value); | |
| try { | |
| const value = await fn(id); | |
| results.set(id, value); | |
| } catch (e) { | |
| // Swallow errors for individual ids so one failure | |
| // does not abort the entire batch. | |
| continue; | |
| } |
| const serverIds = Object.keys(data); | ||
| if (serverIds.length === 0) return; | ||
|
|
||
| const existing = await LatestStats.find({ serverId: { $in: serverIds } }).lean(); |
There was a problem hiding this comment.
dayKey is persisted into LatestStats, but it isn’t read anywhere else in the backend (only written in this job and defined on the model). If it’s not needed yet, consider removing it to avoid storing unused state; if it’s intended for daily resets, add logic that actually uses dayKey so the field stays meaningful.
| const existing = await LatestStats.find({ serverId: { $in: serverIds } }).lean(); | |
| const existing = await LatestStats.find({ serverId: { $in: serverIds }, dayKey }).lean(); |
| setBackendError(""); | ||
| if (response.status === 401 || response.status === 403) { | ||
| setAuthError("Authentication failed for this server. Please re-authenticate."); | ||
| } else if (response.ok) { |
There was a problem hiding this comment.
requestBackend sets authError on 401/403 and only clears it when response.ok is true. If a request returns another error status (e.g., 500) after a previous auth failure, the stale authError can keep the UI stuck on the auth error screen. Consider clearing authError for any response that is not 401/403 (or clearing it before issuing the request).
| } else if (response.ok) { | |
| } else { |
| document.addEventListener("contextmenu", block); | ||
| document.addEventListener("dragstart", block); | ||
| return () => { | ||
| document.removeEventListener("contextmenu", block); | ||
| document.removeEventListener("dragstart", block); |
There was a problem hiding this comment.
This globally prevents the browser context menu and drag start, which breaks expected interactions (copy/paste via context menu, opening links in new tab, drag-and-drop, and some assistive tech workflows). Consider scoping this behavior to specific elements (e.g., images) or removing it entirely.
| document.addEventListener("contextmenu", block); | |
| document.addEventListener("dragstart", block); | |
| return () => { | |
| document.removeEventListener("contextmenu", block); | |
| document.removeEventListener("dragstart", block); | |
| const images = document.querySelectorAll("img"); | |
| images.forEach((img) => { | |
| img.addEventListener("contextmenu", block); | |
| img.addEventListener("dragstart", block); | |
| }); | |
| return () => { | |
| images.forEach((img) => { | |
| img.removeEventListener("contextmenu", block); | |
| img.removeEventListener("dragstart", block); | |
| }); |
| -webkit-user-select: none; | ||
| -moz-user-select: none; | ||
| -ms-user-select: none; | ||
| user-select: none; |
There was a problem hiding this comment.
Setting user-select: none on body prevents users from selecting/copying any text (including error messages, URLs, etc.) and can negatively impact accessibility. Consider limiting this to specific decorative elements instead of applying it globally.
| -webkit-user-select: none; | |
| -moz-user-select: none; | |
| -ms-user-select: none; | |
| user-select: none; |
| serverId: { | ||
| type: String, | ||
| required: true, | ||
| unique: true | ||
| }, |
There was a problem hiding this comment.
serverId is marked unique: true here, and the schema also adds a separate .index({ serverId: 1 }) below. That results in duplicate index definitions for the same key pattern and can cause index build conflicts at startup. Keep only one definition (either rely on unique: true, or define a single { serverId: 1 } index with unique: true).
| const latestStats = await LatestStats.find().lean(); | ||
|
|
There was a problem hiding this comment.
/latest currently loads the entire latest_stats collection (LatestStats.find()), then filters in memory. This can become a full-collection scan and grow unbounded over time. Prefer querying only the serverIds that exist in Server (e.g., find({ serverId: { $in: Object.keys(serverNames) } })) and/or projecting only the fields needed for the response.
| const latestStats = await LatestStats.find().lean(); | |
| const serverIds = Object.keys(serverNames); | |
| const latestStats = await LatestStats.find( | |
| { serverId: { $in: serverIds } }, | |
| { | |
| serverId: 1, | |
| latestCount: 1, | |
| dailyPeak: 1, | |
| dailyPeakTimestamp: 1, | |
| record: 1, | |
| recordTimestamp: 1, | |
| latestTimestamp: 1 | |
| } | |
| ).lean(); |
Brings all current dev changes into main.