feat(web): implement analytics dashboard for issue #30#75
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
7 findings across backend, frontend, and documentation.
🟡 Warnings (4)
- Incomplete member cache for
newMembers/onlineMembers(src/api/routes/guilds.js:677-691) —guild.members.cacheonly contains members Discord has sent to the bot during the session. These KPIs will undercount unless members are explicitly fetched. - Locale/timezone-dependent bucket labels (
src/api/routes/guilds.js:140-153) —toLocaleStringoutput varies by server environment. Consider fixed formatting or moving label generation to the frontend. - Cross-browser date parsing ambiguity (
web/src/components/dashboard/analytics-dashboard.tsx:63-69) —new Date("...T00:00:00.000")without aZsuffix is parsed as local time in some engines and UTC in others. - Open query param forwarding in analytics proxy (
web/src/app/api/guilds/[guildId]/analytics/route.ts:50-53) — All client query params are forwarded to the bot API. Should allowlist expected params only.
🟡 Warning (documentation)
- AGENTS.md Key Files table not updated — New files (
analytics-dashboard.tsx,bot-api.ts,guild-selection.ts,analytics/route.ts,analytics.ts) and the new analytics endpoint are not documented per project conventions.
🔵 Nitpicks (2)
- Stale
nowreference (analytics-dashboard.tsx:85) —new Date()captured at render time becomes stale if the user stays on the page. - Silent $0 cost for unknown models (
src/modules/ai.js:121-127) —MODEL_PRICING_PER_MILLIONreturns 0 for unlisted models with no warning. Also verifyclaude-haiku-4-5model ID format.
See inline comments for details and suggested fixes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Use Intl.DateTimeFormat with explicit UTC timezone in formatBucketLabel - Document guild.members.cache limitation for newMembers/onlineMemberCount - Use UTC-consistent ISO date strings (append Z) in analytics dashboard - Move 'now' into useState for freshness on re-mount - Log warning for unknown models in cost estimation, fix haiku model ID - Allowlist query params in analytics proxy route instead of forwarding all - Update AGENTS.md Key Files table with new analytics files
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end analytics: a backend GET Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (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 |
There was a problem hiding this comment.
Review Summary
1 remaining finding after the fix commit addressed all 7 prior findings.
The fix commit (eacbc6e) correctly addressed: locale-dependent bucket labels, cross-browser date parsing, open query param forwarding, stale now reference, silent $0 for unknown models, Haiku model ID, AGENTS.md updates, and member cache documentation.
Remaining issue
🟡 Warning: Stale fetch responses in analytics dashboard (web/src/components/dashboard/analytics-dashboard.tsx:160-215) — fetchAnalytics doesn't use AbortController to cancel previous in-flight requests when parameters change. Rapid filter/range changes cause race conditions where a slow earlier response overwrites newer data. The existing server-selector.tsx in this codebase already implements this pattern correctly — apply the same approach here.
See inline comment for details and suggested fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
AGENTS.mdREADME.mdsrc/api/routes/guilds.jssrc/modules/ai.jstests/api/routes/guilds.test.jstests/modules/ai.test.jsweb/package.jsonweb/src/app/api/guilds/[guildId]/analytics/route.tsweb/src/app/dashboard/page.tsxweb/src/components/dashboard/analytics-dashboard.tsxweb/src/components/layout/server-selector.tsxweb/src/lib/bot-api.tsweb/src/lib/discord.server.tsweb/src/lib/guild-selection.tsweb/src/types/analytics.tsweb/tests/api/guild-analytics.test.tsweb/tests/app/dashboard.test.tsxweb/tests/components/dashboard/analytics-dashboard.test.tsxweb/tests/lib/discord.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
AGENTS.md
📄 CodeRabbit inference engine (AGENTS.md)
Update
AGENTS.mdKey Files table when adding new commands or modules
Files:
AGENTS.md
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must be in the
tests/directory and use Vitest framework
Files:
tests/modules/ai.test.jstests/api/routes/guilds.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix for Node.js built-in imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging (import { info, warn, error } from '../logger.js') — NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read configuration at runtime
UsesetConfigValue(path, value, guildId?)to update configuration at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
All new code must include tests with 80% coverage threshold on statements, branches, functions, and lines
Files:
src/modules/ai.jssrc/api/routes/guilds.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Checkconfig.yourModule.enabledbefore processing in module handlers
Prefer the per-requestgetConfig()pattern for new modules instead of reactiveonConfigChangewiring, which is only for stateful resources
Files:
src/modules/ai.js
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Update
README.mdwhen adding new commands, modules, or changing environment variables
Files:
README.md
🧠 Learnings (5)
📚 Learning: 2025-10-10T15:05:26.145Z
Learnt from: CR
Repo: BillChirico/LUA-Obfuscator PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T15:05:26.145Z
Learning: Applies to **/*.{jsx,tsx} : Use JSX with Tailwind CSS for the web interface
Applied to files:
web/package.json
📚 Learning: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to src/app/layout.tsx : Use `vercel/analytics` and `vercel/speed-insights` packages for user analytics and performance monitoring in production
Applied to files:
web/tests/components/dashboard/analytics-dashboard.test.tsxweb/src/components/dashboard/analytics-dashboard.tsxweb/tests/app/dashboard.test.tsxweb/src/app/dashboard/page.tsxweb/src/types/analytics.ts
📚 Learning: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to src/**/*.{ts,tsx} : Use `date-fns` for date manipulation and formatting throughout the application
Applied to files:
web/src/components/dashboard/analytics-dashboard.tsxsrc/api/routes/guilds.js
📚 Learning: 2026-02-18T00:10:37.278Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.278Z
Learning: Applies to src/modules/events.js : Register new modules in `src/modules/events.js` via `registerEventHandlers()`
Applied to files:
AGENTS.md
📚 Learning: 2025-11-26T01:57:34.920Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:57:34.920Z
Learning: Applies to src/app/**/*.{ts,tsx} : Use Server Components by default in `src/app` for data fetching; mark interactive components with `"use client"` directive
Applied to files:
web/src/components/layout/server-selector.tsx
🧬 Code graph analysis (9)
web/src/app/api/guilds/[guildId]/analytics/route.ts (2)
web/src/lib/bot-api.ts (1)
getBotApiBaseUrl(8-18)src/logger.js (1)
error(231-233)
web/tests/components/dashboard/analytics-dashboard.test.tsx (2)
web/src/components/dashboard/analytics-dashboard.tsx (1)
AnalyticsDashboard(84-685)web/src/lib/guild-selection.ts (1)
SELECTED_GUILD_KEY(1-1)
web/src/components/dashboard/analytics-dashboard.tsx (2)
web/src/types/analytics.ts (2)
AnalyticsRangePreset(1-1)DashboardAnalytics(40-63)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)GUILD_SELECTED_EVENT(2-2)
web/src/lib/discord.server.ts (1)
web/src/lib/bot-api.ts (1)
getBotApiBaseUrl(8-18)
web/tests/app/dashboard.test.tsx (1)
web/src/app/dashboard/page.tsx (1)
DashboardPage(3-5)
tests/modules/ai.test.js (2)
src/modules/ai.js (1)
generateResponse(447-568)src/logger.js (1)
info(217-219)
web/src/components/layout/server-selector.tsx (1)
web/src/lib/guild-selection.ts (1)
broadcastSelectedGuild(7-14)
src/api/routes/guilds.js (2)
src/index.js (1)
dbPool(310-310)src/logger.js (2)
warn(224-226)error(231-233)
web/src/app/dashboard/page.tsx (1)
web/src/components/dashboard/analytics-dashboard.tsx (1)
AnalyticsDashboard(84-685)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (26)
web/src/lib/bot-api.ts (1)
1-18: Clean utility — LGTM.Simple, well-documented normalization logic. The trailing-slash stripping and suffix detection handle the common configurations correctly.
web/src/lib/discord.server.ts (1)
4-4: URL normalization and auth header changes look correct.The switch from direct
BOT_API_URLtogetBotApiBaseUrl()centralizes URL construction, and${botApiBaseUrl}/guildscorrectly resolves to/api/v1/guilds. Thex-api-secretheader aligns with the analytics proxy route and backend expectations.Also applies to: 144-166
web/src/types/analytics.ts (1)
1-63: Well-structured types that align with the backend response shape — LGTM.The interfaces accurately model the analytics payload from the
GET /:id/analyticsendpoint, including nullableonlineMembers, string-based ISO date fields, and the nestedaiUsagestructure. Clean separation of concerns across the type definitions.src/api/routes/guilds.js (2)
58-155: Helper functions are well-structured — LGTM.
parseDateParam,parseAnalyticsRange,parseAnalyticsInterval, andformatBucketLabelare clean, well-documented utilities with sensible validation and defaults. The auto-interval selection (hour for ≤48h, day otherwise) is a nice UX touch.
630-745: Post-processing and response assembly look solid.Good defensive defaults for KPI rows, proper Number() conversions for DB results, documented cache limitations for
members.cache, and the graceful.catch()on the logs query. The response shape aligns well with the TypeScript types inweb/src/types/analytics.ts.README.md (1)
259-259: README update accurately describes the new analytics feature — LGTM.As per coding guidelines, the README is updated for the new analytics dashboard module. The description concisely captures the key capabilities.
web/tests/lib/discord.test.ts (2)
400-427: Test expectations correctly updated for the new URL scheme and auth header — LGTM.The URL expectations (
/api/v1/guilds) and header assertions (x-api-secret) are consistent with the changes indiscord.server.tsandbot-api.ts.
470-506: Mutual guild tests properly updated to match new API path — LGTM.Both the success-path and failure-path
getMutualGuildstests now check for/api/v1/guildsin the fetch URL, consistent with thegetBotApiBaseUrl()normalization.web/package.json (1)
27-27:rechartsversion 3.5.0 is valid and stable.The specified version
^3.5.0is an official Recharts release published on November 23, 2025, with a subsequent patch release available (v3.5.1). The dependency specification is correct.web/tests/app/dashboard.test.tsx (1)
30-41: LGTM — mock and assertion correctly track the new page structure.The
vi.mockstub and updated assertion accurately reflect thatDashboardPagenow delegates entirely toAnalyticsDashboard, keeping this a lightweight smoke test for the page wrapper while the heavier behavioral coverage lives inanalytics-dashboard.test.tsx.web/src/app/dashboard/page.tsx (1)
1-5: LGTM — clean simplification.tests/modules/ai.test.js (1)
19-19: Import placement is fine —vi.mockhoisting applies.
infois imported at Line 19 while the logger mock is declared at Lines 36–41. In Vitest, allvi.mock()calls are hoisted before module imports, soinforeceives the mockedvi.fn(). No TDZ issue here.web/tests/components/dashboard/analytics-dashboard.test.tsx (1)
116-119: Locale is already explicitly specified as "en-US" in the formatter—no action required.The
formatNumber()function at lines 80-82 ofanalytics-dashboard.tsxalready callstoLocaleString("en-US")with an explicit locale parameter. This guarantees US-style number formatting regardless of the CI runner's system locale, making the test assertions safe from locale-dependent flakiness.Likely an incorrect or invalid review comment.
tests/api/routes/guilds.test.js (3)
62-67: LGTM — good additions to the mock member fixture.Adding
bot: false,joinedTimestamp, andpresencetomockMembercorrectly supports the new analytics route's realtime and KPI calculations (online members, new members).
606-613: LGTM — concise validation of the custom-range error path.
636-652: LGTM — graceful degradation test is well-structured.The test correctly verifies the fallback when the logs query fails: status 200 with empty
byModeland zero cost. This aligns with the resilience requirement in the PR objectives.src/modules/ai.js (2)
128-160: LGTM —toNonNegativeNumberandestimateAiCostUsdare well-implemented.
toNonNegativeNumbercorrectly handles edge cases (NaN, Infinity, negatives, non-numeric input).estimateAiCostUsdgracefully degrades to $0 for unknown models with a structured warning log. ThetoFixed(6)rounding is appropriate for cost estimation precision.
517-538: LGTM — solid defensive extraction and structured usage logging.Good use of optional chaining for
data?.choices?.[0]?.message?.contentand the fallback chain formodelUsed. The structured log at line 530 includes all fields needed for downstream analytics aggregation and follows the Winston structured-metadata guideline.web/src/components/dashboard/analytics-dashboard.tsx (8)
84-101: LGTM — well-structured state initialization.Capturing
nowviauseState(() => new Date())ensures date-dependent defaults remain consistent across re-renders. The draft/applied pattern for custom dates cleanly separates user input from committed query parameters.
102-140: LGTM — guild selection synchronization is clean.Listening to both
CustomEvent(same-window) andStorageEvent(cross-tab) with proper cleanup is the right approach. ResettingchannelFilteron guild change avoids stale filters.
237-268: LGTM — good use ofuseMemofor derived chart data.The
heatmapLookuppre-computes a Map for O(1) cell lookups and tracks the max for alpha normalization.modelUsageDataandtokenBreakdownDatacleanly transform API data for recharts consumption.
496-533: LGTM — message volume and AI usage charts.The line chart and combined pie/bar layout are well-structured with appropriate recharts configurations.
dot={false}on lines with many data points is a good performance and UX choice.
639-679: LGTM — heatmap implementation is solid.The table-based approach is semantically correct for a grid visualization. The alpha calculation (
0.2 + (value / max) * 0.8) provides good visual contrast, and the division-by-zero guard onheatmapLookup.max === 0is correct.titleattributes on cells provide basic tooltip accessibility.
359-429: LGTM — KPI cards are well-structured and responsive.The conditional rendering with
kpis ? formatNumber(...) : "—"gracefully handles the loading/null state. The 5-column grid withsm:grid-cols-2 xl:grid-cols-5breakpoints provides good responsive behavior.
56-69: 🧹 Nitpick | 🔵 TrivialConsider using
date-fnsfor date manipulation instead of manual formatting.The
formatDateInput,startOfDayIso, andendOfDayIsohelpers manually construct date strings. The project guidelines recommenddate-fnsfor date operations, which would provideformat,startOfDay, andendOfDaywith better timezone and edge-case handling.That said, the current implementation is correct for UTC-anchored ISO strings and is lightweight.
Based on learnings: "Use
date-fnsfor date manipulation and formatting throughout the application" (from CLAUDE.md, applies tosrc/**/*.{ts,tsx}).⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxCommunity/Volvox.Website PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:57:34.920Z Learning: Applies to src/**/*.{ts,tsx} : Use `date-fns` for date manipulation and formatting throughout the applicationLearnt from: CR Repo: VolvoxCommunity/Volvox.Website PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:57:34.920Z Learning: Applies to src/app/layout.tsx : Use `vercel/analytics` and `vercel/speed-insights` packages for user analytics and performance monitoring in productionLearnt from: CR Repo: VolvoxCommunity/Volvox.Website PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-26T01:57:34.920Z Learning: Applies to src/app/page.tsx : Separate data fetching (Server Components) from UI interactivity (Client Components), passing data as props from `src/app/page.tsx` to `homepage-client.tsx`
602-612: The code is correct for recharts 3.5.0. The Bar component's onClick signature in recharts 3.x remains(data, index, event), with index as the second numeric parameter. The current implementation(_value, index)properly extracts the channel index and will work as expected without risk of silent failure.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 37-39: AGENTS.md needs two new entries for the added utility
modules: add rows for web/src/lib/guild-selection.ts (purpose: Guild selection
constants and the broadcastSelectedGuild broadcaster) and web/src/lib/bot-api.ts
(purpose: getBotApiBaseUrl — normalises BOT_API_URL to a stable /api/v1 base) in
the "Key Files" table so contributors can find these utilities; reference the
exact filenames and the exported symbols broadcastSelectedGuild and
getBotApiBaseUrl in the brief purpose column text.
In `@src/api/routes/guilds.js`:
- Around line 492-503: Do not return err.message directly to clients from the
analytics route; instead detect validation vs unexpected errors in the
router.get('/:id/analytics', requireGuildAdmin, validateGuild, async (req, res)
=> { ... }) block around parseAnalyticsRange and return a safe client-facing
message (e.g., "Invalid range parameter") for known validation errors while
logging the full error internally (use console/error logger) for unexpected
exceptions; alternatively, have parseAnalyticsRange throw a specific
ValidationError type and check for that type before returning the specific
message, otherwise respond with a generic 400/500 message and record err.message
in server logs.
- Around line 540-629: Add the recommended DB indexes to improve the six
concurrent queries: create an index on conversations(guild_id, created_at) to
support the KPI/volume/heatmap/channel queries, create conversations(guild_id,
role, created_at) to support the active-conversation realtime query (used with
ACTIVE_CONVERSATION_WINDOW_MINUTES), and add an expression/functional index on
logs to cover the model-usage query such as an index on ((metadata->>'guildId'),
message, timestamp) or appropriate JSONB expression index to support the
logsWhere filter; ensure these indexes are created in your migrations so the
dbPool.query calls (the Promise.all producing kpiResult, volumeResult,
channelResult, heatmapResult, activeResult, modelUsageResult) use them.
- Around line 103-113: The "today" branch in src/api/routes/guilds.js uses
from.setHours(0,0,0,0) which sets server-local midnight and causes mismatch with
UTC-based created_at timestamps; update that call to from.setUTCHours(0,0,0,0)
in the function that builds the { from, to, range } result so the returned
`from` is midnight UTC (leave the month/week branches unchanged) and ensure any
tests/uses of that returned `from` (e.g., SQL params or formatBucketLabel) now
operate against the UTC-aligned Date.
In `@src/modules/ai.js`:
- Around line 117-126: The MODEL_PRICING_PER_MILLION hardcoded table will
silently return $0 for unknown Claude models; update the file by adding an
explanatory comment above MODEL_PRICING_PER_MILLION that documents that this is
manual/approximate pricing, includes a link to the authoritative upstream
pricing page (or vendor pricing API) and instructs maintainers to add new model
entries when released, and also mention the existing logWarn call (logWarn) for
unknown models so maintainers know where runtime warnings are emitted; keep the
comment concise and include the vendor pricing URL and a note about updating
both input/output fields for new models.
In `@tests/api/routes/guilds.test.js`:
- Around line 533-604: The analytics success test in the "GET /:id/analytics" it
block is missing an assertion for kpis.newMembers; add an assertion (e.g.
expect(res.body.kpis.newMembers).toBe(1)) after the existing KPI checks so the
test verifies the newMembers calculation based on the mocked member with
joinedTimestamp; update the assertions inside the same it('should return
analytics payload...') test to include this check.
In `@tests/modules/ai.test.js`:
- Around line 252-280: The test omits asserting the estimated cost produced by
generateResponse; update the test in tests/modules/ai.test.js (the "should log
structured AI usage metadata for analytics" case) to include an assertion for
estimatedCostUsd in the expect.objectContaining for the 'AI usage' info call,
computing the expected cost for model 'claude-sonnet-4-20250514' from the same
cost logic used by generateResponse and use a numeric matcher (e.g., toBeCloseTo
or exact value) to validate estimatedCostUsd alongside
guildId/channelId/model/promptTokens/completionTokens/totalTokens. Ensure you
reference the generateResponse/AI usage logging behavior when deriving the
expected cost so the test matches the implementation.
In `@web/src/app/api/guilds/`[guildId]/analytics/route.ts:
- Around line 47-49: The URL construction using new URL(...) for upstreamUrl can
throw if botApiBaseUrl is malformed; move the creation of upstreamUrl (and any
use of encodeURIComponent(guildId) in that construction) inside the existing
try-catch block (or wrap it with its own try-catch) so any thrown error is
caught and results in the same structured JSON error response; update the
handler where upstreamUrl is defined so errors from new URL(...) are handled
consistently with the rest of the route logic.
- Around line 12-32: The GET handler in function GET currently only validates a
session token (getToken) but does not verify the user is a member/admin of the
requested guild; update GET to enforce guild-level authorization before
proceeding by either (A) calling your mutual-guilds checker (e.g.,
getMutualGuilds or equivalent) with token.accessToken and confirm the returned
list includes guildId with admin permissions, or (B) forward the user's OAuth
access token to the bot API instead of using the shared x-api-secret so the
bot's requireGuildAdmin middleware can run; if the check fails return 403 and do
not continue to the proxy call. Ensure you reference GET, getToken, and params
when locating where to add this logic.
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Around line 231-235: The applyCustomRange function currently applies
customFromDraft and customToDraft without validating their order; add a guard in
applyCustomRange that checks whether customFromDraft is before or equal to
customToDraft and only applies when valid, otherwise prevent applying and
surface a user-visible error or corrective action (e.g., swap the dates or call
a UI notifier). Specifically, update applyCustomRange to compare customFromDraft
and customToDraft before calling setCustomFromApplied and setCustomToApplied,
and if invalid set a visible error state (e.g., setCustomRangeError or trigger
the existing toast/notification) so the user knows the range is incorrect.
- Around line 160-215: fetchAnalytics can leave multiple in-flight requests that
race and can update state after unmount; add an AbortController to the fetch
flow: create a new AbortController for each invocation of fetchAnalytics, pass
controller.signal into fetch, store the controller in a ref (or closure) so you
can call previousController.abort() before starting a new request, and ensure
the effect that calls fetchAnalytics aborts the controller in its cleanup; also
update the catch block in fetchAnalytics to ignore AbortError (do not call
setAnalytics/setError when aborted) while still handling real errors, and keep
the existing setLoading/setLastUpdatedAt logic intact.
In `@web/src/components/layout/server-selector.tsx`:
- Around line 74-78: During the restore path in ServerSelector, remove the
broadcast of the selection so AnalyticsDashboard (which bootstraps from
SELECTED_GUILD_KEY on mount) doesn't get a redundant GUILD_SELECTED_EVENT and
refetch; specifically, in the restore branch where you call
setSelectedGuild(saved) and set restored = true, skip calling
broadcastSelectedGuild(saved.id) (or guard it with if (!restored) so only
user-initiated selection still broadcasts). Ensure all other code paths that
represent explicit user selection still call broadcastSelectedGuild so sibling
components that don't read localStorage continue to work.
In `@web/src/lib/guild-selection.ts`:
- Around line 7-14: The function broadcastSelectedGuild should guard against an
empty guildId to avoid downstream requests like /api/guilds//analytics; in
broadcastSelectedGuild add a one-line check (e.g. if (!guildId) return) before
dispatching the CustomEvent (referencing broadcastSelectedGuild and
GUILD_SELECTED_EVENT) so empty strings are ignored and not broadcast.
In `@web/tests/api/guild-analytics.test.ts`:
- Around line 54-65: Add a second test case mirroring the existing "returns 500
when bot API env vars are missing" spec that specifically exercises the
BOT_API_SECRET-missing branch: set process.env.BOT_API_URL to a valid value,
delete process.env.BOT_API_SECRET, keep mockGetToken.mockResolvedValue({
accessToken: "discord-token" }), call GET(createRequest(), { params:
Promise.resolve({ guildId: "guild-1" }) }) and assert response.status is 500 and
the JSON body.error matches /not configured/i; this will cover the code path
checked by the guard (!botApiBaseUrl || !botApiSecret) and use the same helpers
(mockGetToken and GET) as the original test.
- Around line 4-7: Replace the lazy-closure mock for getToken with Vitest's
explicit hoisted mock to match project convention: declare mockGetToken using
vi.hoisted(() => vi.fn()), then update the vi.mock("next-auth/jwt", ...) factory
to call mockGetToken from that hoisted variable (preserving the current factory
shape) so the test uses the hoisted mockGetToken instead of the inline (...args)
=> mockGetToken(...args) wrapper.
In `@web/tests/components/dashboard/analytics-dashboard.test.tsx`:
- Around line 89-146: Add two tests for AnalyticsDashboard to cover the API
error and 401 branches: 1) set SELECTED_GUILD_KEY in localStorage, mock
global.fetch to resolve to a non-ok response (ok: false, status: 500) and render
<AnalyticsDashboard />; waitFor the error card to appear and assert there is a
retry button visible (and that clicking it triggers another fetch). 2) set
SELECTED_GUILD_KEY, mock global.fetch to resolve with status 401 and render
<AnalyticsDashboard />; assert that the app redirects to /login (or that the
router navigation function used by AnalyticsDashboard is called with '/login').
Use the same vi.spyOn(global, "fetch") pattern as other tests and restore mocks
between tests.
…tch 1/5) - Add AbortController pattern to fetchAnalytics to prevent stale responses from overwriting current state when parameters change rapidly - Match the implementation in server-selector.tsx exactly: - abortControllerRef for tracking current request - Abort previous in-flight request before starting new one - Pass signal to fetch call - Ignore AbortError in catch block - Only reset loading if this request is still current - Cleanup on unmount - Add missing Key Files entries to AGENTS.md: - web/src/lib/guild-selection.ts (broadcastSelectedGuild) - web/src/lib/bot-api.ts (getBotApiBaseUrl) Addresses review threads: - PRRT_kwDORICdSM5vMpLB: race condition in fetchAnalytics - PRRT_kwDORICdSM5vMsXh: same race condition - PRRT_kwDORICdSM5vMufZ: missing AGENTS.md entries Note: Thread PRRT_kwDORICdSM5vMpK0 (member cache limitation) already documented in src/api/routes/guilds.js lines 680-685.
There was a problem hiding this comment.
Review Summary — 6 findings
The prior review findings (AbortController, UTC formatting, date parsing Z suffix, query param allowlisting, stale now, unknown model warning, AGENTS.md) have all been addressed in commit eacbc6e.
Remaining issues:
🔴 Critical (1)
- Broken access control in analytics proxy (
web/src/app/api/guilds/[guildId]/analytics/route.ts:12-32) — Any authenticated user can query analytics for any guild. The proxy usesx-api-secretwhich bypasses the bot'srequireGuildAdminmiddleware. Must add guild-level authorization before proxying.
🟡 Warnings (3)
- "today" range uses server-local midnight (
src/api/routes/guilds.js:105) —setHours(0,0,0,0)should besetUTCHours(0,0,0,0)to align with UTC-based DB timestamps. - Client hardcodes
interval, bypassing server auto-detection (analytics-dashboard.tsx:152) — Custom ranges of 1-2 days get forced todayinterval (1-2 data points) instead of letting the server pickhour. new URL()outside try-catch (analytics/route.ts:47-49) — Can throw unhandled on malformedBOT_API_URL.
🔵 Nitpicks (2)
- Redundant broadcast on restore (
server-selector.tsx:74-78) — Causes a duplicate analytics fetch on page load. - No empty string guard in
broadcastSelectedGuild(guild-selection.ts:7-9) — Could cause requests to/api/guilds//analytics.
See inline comments for details and suggested fixes.
- Use setUTCHours for 'today' range to align with UTC timestamps - Add pricing table maintenance comment with Anthropic pricing reference - Add newMembers assertion to analytics success test
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
AGENTS.mdweb/src/components/dashboard/analytics-dashboard.tsx
🧰 Additional context used
📓 Path-based instructions (1)
AGENTS.md
📄 CodeRabbit inference engine (AGENTS.md)
Update
AGENTS.mdKey Files table when adding new commands or modules
Files:
AGENTS.md
🧠 Learnings (4)
📚 Learning: 2026-02-16T05:48:12.653Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T05:48:12.653Z
Learning: Applies to src/app/layout.tsx : Use `vercel/analytics` and `vercel/speed-insights` packages for user analytics and performance monitoring in production
Applied to files:
web/src/components/dashboard/analytics-dashboard.tsx
📚 Learning: 2026-02-12T06:25:33.802Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T06:25:33.802Z
Learning: Applies to src/**/*.{ts,tsx} : Use `date-fns` for date manipulation and formatting throughout the application
Applied to files:
web/src/components/dashboard/analytics-dashboard.tsx
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to AGENTS.md : Update `AGENTS.md` Key Files table when adding new commands or modules
Applied to files:
AGENTS.md
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to src/modules/events.js : Register new modules in `src/modules/events.js` via `registerEventHandlers()`
Applied to files:
AGENTS.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 37-41: The Key Files table in AGENTS.md is missing the new shared
types module; add an entry for web/src/types/analytics.ts describing it as the
canonical analytics type definitions (include exported type names like
AnalyticsRange, DashboardAnalytics, DashboardKpis) so contributors can find the
shared types used by web/src/components/dashboard/analytics-dashboard.tsx,
web/src/app/api/guilds/[guildId]/analytics/route.ts, and tests; update the table
row list (alongside web/src/lib/guild-selection.ts and web/src/lib/bot-api.ts)
to include this new file and a concise description.
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Around line 570-573: The code currently maps modelUsageData to deprecated
Recharts <Cell> components inside <Pie> (and similarly uses <Cell> in the
channel-activity <BarChart>), so replace those <Cell> usages by providing a
custom renderer via the Pie's shape or content prop (and the Bar's shape/content
prop) that reads each datum's fill and applies it when rendering slices/bars;
update the PieChart code that references modelUsageData and the channel-activity
BarChart code to remove Cell and implement a custom shape/content function that
uses the datum.fill (and datum.model or datum.key) to set fill and any other
styling, ensuring the renderer respects Recharts' render args for compatibility
with v4.
- Around line 462-466: The online members card shows "0" before initial load
because it compares analytics?.realtime.onlineMembers to null instead of
checking whether analytics exists; change the rendering to first check analytics
(the analytics prop/state) and if missing return "—", otherwise if
analytics.realtime.onlineMembers === null show "N/A", else call formatNumber
with analytics.realtime.onlineMembers (fallback to 0 only when analytics
exists). Update the expression around analytics, realtime, onlineMembers and
formatNumber in analytics-dashboard.tsx accordingly.
- Around line 355-371: The onClick handlers for the Refresh and "Try again"
buttons in the analytics-dashboard component are returning the Promise from
fetchAnalytics(), causing potential floating-promise lint errors; change both
handlers from onClick={() => fetchAnalytics()} to onClick={() => void
fetchAnalytics()} so the promise is intentionally ignored (apply to the Refresh
button and the Button inside the error Card that call fetchAnalytics()).
- Around line 56-69: The helpers use literal "Z" boundaries (UTC) which
misinterpret local dates; replace formatDateInput/startOfDayIso/endOfDayIso with
date-fns-based logic: use format(date, 'yyyy-MM-dd') in place of
formatDateInput, and compute local start/end with startOfDay(date) and
endOfDay(date) and then format to ISO (e.g., formatISO) or convert the local
start/end to UTC using date-fns-tz (zonedTimeToUtc) before serializing; update
callers to pass Date objects or parse the local "yyyy-MM-dd" string into a local
Date (new Date(`${dateStr}T00:00:00`) without "Z") before applying
startOfDay/endOfDay so the resulting ISO strings represent the true UTC bounds
of the user's local day.
- Around line 657-696: Update the table headers for accessibility: in the thead
change the static "Day" header th to include scope="col"; for the HOURS.map
header cells add scope="col" to each <th> (the one using key={hour} and
rendering {hour % 3 === 0 ? hour : ""}); and in the tbody change the row label
cell that renders {day} (currently a <td> with className "pr-2
text-muted-foreground") to a <th scope="row"> while keeping its classes and key
usage; leave the HOURS.map cells that render the colored divs as <td> but ensure
their keys remain (e.g., key={`${day}-${hour}`}) so screen readers can correctly
associate data cells with their column and row headers (references: HOURS, DAYS,
heatmapLookup).
---
Duplicate comments:
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Around line 248-252: applyCustomRange currently applies whatever drafts exist
without validating that customFromDraft is <= customToDraft; add a guard in
applyCustomRange to check the date order (e.g., if customFromDraft >
customToDraft) and handle it by either setting a visible validation error state
(so the UI can show a message) or swapping the values before calling
setCustomFromApplied and setCustomToApplied; reference the applyCustomRange
function and the setters setCustomFromApplied/setCustomToApplied (and any
existing validation/error state like customRangeError) when implementing this
fix.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/api/routes/guilds.jssrc/modules/ai.jstests/api/routes/guilds.test.jstests/modules/ai.test.jsweb/src/components/dashboard/analytics-dashboard.tsxweb/tests/api/guild-analytics.test.tsweb/tests/components/dashboard/analytics-dashboard.test.tsx
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must be in the
tests/directory and use Vitest framework
Files:
tests/api/routes/guilds.test.jstests/modules/ai.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix for Node.js built-in imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging (import { info, warn, error } from '../logger.js') — NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read configuration at runtime
UsesetConfigValue(path, value, guildId?)to update configuration at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
All new code must include tests with 80% coverage threshold on statements, branches, functions, and lines
Files:
src/modules/ai.jssrc/api/routes/guilds.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Checkconfig.yourModule.enabledbefore processing in module handlers
Prefer the per-requestgetConfig()pattern for new modules instead of reactiveonConfigChangewiring, which is only for stateful resources
Files:
src/modules/ai.js
🧠 Learnings (4)
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to src/**/*.js : All new code must include tests with 80% coverage threshold on statements, branches, functions, and lines
Applied to files:
tests/api/routes/guilds.test.js
📚 Learning: 2026-02-16T05:48:12.653Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T05:48:12.653Z
Learning: Applies to src/app/layout.tsx : Use `vercel/analytics` and `vercel/speed-insights` packages for user analytics and performance monitoring in production
Applied to files:
web/tests/components/dashboard/analytics-dashboard.test.tsxweb/src/components/dashboard/analytics-dashboard.tsx
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to src/**/*.js : Always use Winston logging (`import { info, warn, error } from '../logger.js'`) — NEVER use `console.log`, `console.warn`, `console.error`, or any `console.*` method in src/ files
Applied to files:
tests/modules/ai.test.js
📚 Learning: 2026-02-12T06:25:33.802Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T06:25:33.802Z
Learning: Applies to src/**/*.{ts,tsx} : Use `date-fns` for date manipulation and formatting throughout the application
Applied to files:
web/src/components/dashboard/analytics-dashboard.tsxsrc/api/routes/guilds.js
🧬 Code graph analysis (6)
tests/api/routes/guilds.test.js (1)
src/api/server.js (2)
app(27-27)app(95-95)
src/modules/ai.js (3)
src/modules/chimeIn.js (2)
model(89-89)model(147-147)src/index.js (2)
guildConfig(188-188)guildId(157-157)src/logger.js (1)
info(217-219)
web/tests/api/guild-analytics.test.ts (1)
src/modules/ai.js (1)
response(511-522)
tests/modules/ai.test.js (2)
src/modules/ai.js (1)
generateResponse(462-585)src/logger.js (2)
info(217-219)warn(224-226)
web/src/components/dashboard/analytics-dashboard.tsx (4)
web/src/types/analytics.ts (2)
AnalyticsRangePreset(1-1)DashboardAnalytics(40-63)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)GUILD_SELECTED_EVENT(2-2)web/src/components/ui/card.tsx (5)
Card(76-76)CardHeader(77-77)CardTitle(79-79)CardDescription(80-80)CardContent(81-81)web/src/components/ui/button.tsx (1)
Button(58-58)
src/api/routes/guilds.js (2)
src/index.js (1)
dbPool(310-310)src/logger.js (2)
warn(224-226)error(231-233)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: claude-review
🔇 Additional comments (4)
src/modules/ai.js (1)
532-555: Analytics integration looks correct.
modelUsedfallback chain, token sanitization,totalTokensderived fallback, cost estimation, and structuredinfo('AI usage', …)logging are all sound. The log fires only on the success path and contains no PII.tests/modules/ai.test.js (1)
252-281: LGTM — analytics test correctly asserts all structured log fields includingestimatedCostUsd.web/tests/api/guild-analytics.test.ts (1)
1-224: Well-structured and comprehensive — all previously flagged gaps are addressed.The test suite now covers all critical branches: unauthenticated, missing admin, guild not in mutual list, owner bypass, guild-check 502, both missing-env-var 500 paths, malformed URL 500, the happy-path proxy (including
x-api-secretheader assertion), and upstream error forwarding.vi.hoistedis used correctly for both mocks.src/api/routes/guilds.js (1)
84-98:parseAnalyticsRangethrows plainErrorinstead of a custom error class.The coding guidelines require using custom error classes from
src/utils/errors.jsfor error handling. All threethrow new Error(...)calls inparseAnalyticsRange(lines 84–98) should use a custom error class instead.However, note that
ValidationErrordoes not exist insrc/utils/errors.js. Either add aValidationErrorclass tosrc/utils/errors.js, or use an existing pattern for validation failures. The error handling in the route's catch block (lines 510–513) safely sanitizes messages before returning a 400 response regardless of error type.
🤖 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/modules/ai.js`:
- Around line 136-137: The module-level Set warnedUnknownModels retains state
across tests; add and export a small test-only reset function (e.g. export
function _resetWarnedUnknownModels() or resetWarnedUnknownModels()) that calls
warnedUnknownModels.clear() so tests can reset this hidden state between runs;
reference the existing symbols warnedUnknownModels, _setPoolGetter and setPool
to keep naming consistent and mark the helper as intended for testing only in
the export comment.
- Around line 125-134: Remove the incorrect entry 'claude-opus-4-20250514' from
the MODEL_PRICING_PER_MILLION object to avoid referencing a non-existent
Anthropic model ID; update the pricing map by deleting that key so only valid
model IDs remain (e.g., keep 'claude-opus-4-1-20250805' and any other verified
IDs) and run tests that use MODEL_PRICING_PER_MILLION to ensure pricing lookups
no longer expect the removed snapshot ID.
In `@tests/modules/ai.test.js`:
- Around line 283-323: Add a test that exercises the warnedUnknownModels
deduplication by calling generateResponse twice with a mocked fetch response
that returns an unknown model (e.g., 'unknown-model-xyz'), assert that process
warn was called exactly once for that model (filter warn.mock.calls for the
message 'Unknown model for cost estimation, returning $0' and meta.model ===
'unknown-model-xyz' and expect length === 1), and then call the exported
_clearWarnedModels() to reset state for other tests; mock global fetch and
ensure other expectations (like info not being called with estimatedCostUsd) as
needed to keep assertions deterministic.
In `@web/tests/components/dashboard/analytics-dashboard.test.tsx`:
- Around line 132-135: Replace fragile DOM traversal in tests by adding stable
data-testid attributes to the card containers in analytics-dashboard.tsx (e.g.,
data-testid="realtime-indicator-card" and
data-testid="active-ai-conversations-card") and update the test assertions in
analytics-dashboard.test.tsx to query those containers with getByTestId and then
use within(...) to assert the em-dash ("—") is present; specifically stop using
label.closest("div")?.parentElement and instead locate the card via
screen.getByTestId("realtime-indicator-card") (and the corresponding
"active-ai-conversations-card") before calling within(...).getByText("—").
---
Duplicate comments:
In `@src/api/routes/guilds.js`:
- Around line 548-649: The analytics route issues many concurrent DB queries
(see Promise.all invoking dbPool.query for the conversations and logs queries
using conversationWhere, logsWhere, and ACTIVE_CONVERSATION_WINDOW_MINUTES),
which will slow under load; add the recommended PostgreSQL indexes to cover
these queries: create a composite index on conversations(guild_id, created_at)
to speed KPI/volume/channel/heatmap queries, a composite index on
conversations(guild_id, role, created_at) to support the active AI conversations
query (used with ACTIVE_CONVERSATION_WINDOW_MINUTES), and add an
expression/covering index on logs for the metadata guild id and
message/timestamp lookup (e.g., index on ((metadata->>'guildId')) plus needed
columns) so the dbPool.query for model usage (using logsWhere) can use the
index; apply these indexes in a migration and verify query plans for
dbPool.query calls.
In `@tests/api/routes/guilds.test.js`:
- Around line 533-674: Add two tests for the analytics endpoint to cover the
!dbPool 503 branch and the outer catch 500 branch: create one test that sets
dbPool (or the module's dbPool variable used by the /api/v1/guilds/:id/analytics
handler) to null/undefined, calls GET /api/v1/guilds/guild1/analytics and
asserts status 503; create a second test that forces the primary Promise.all
used in the analytics handler to reject (e.g.,
mockPool.query.mockResolvedValueOnce for preliminary queries but
mockRejectedValueOnce for the first Promise that Promise.all awaits) then call
the same endpoint and assert status 500 and appropriate error shape; reference
the analytics route handler, the dbPool guard, and the Promise.all call to
locate where to hook the mocks.
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Line 17: The code currently imports and uses the deprecated Recharts <Cell>
for the Pie and Bar charts; update the Pie (rendering modelUsageData entries)
and the Bar (rendering channel activity using channelFilter) to remove <Cell>
usage and instead pass a custom renderer via the respective component's shape
(or content) prop: implement a custom Pie slice renderer that reads entry.fill
from modelUsageData and applies it, and implement a custom Bar rectangle
renderer that resolves the fill color from channelFilter and draws the bar;
replace the <Cell> elements in the Pie and Bar JSX with these shape/content
props and ensure the custom render functions receive and use the same props
Recharts passes (entry, x/y/width/height, etc.) so styling remains identical.
There was a problem hiding this comment.
5 findings (2 warnings, 3 nitpicks). CI passes. Prior critical issues (access control, AbortController, UTC handling, query param allowlisting) have all been addressed across the 17 fix commits.
🟡 Warnings (2)
guildIdinterpolated into fetch URL withoutencodeURIComponent(analytics-dashboard.tsx:224) —guildIdoriginates from user-writablelocalStorage. While Discord IDs are numeric, encoding defends against URL injection from crafted storage values.- Unchecked
as DashboardAnalyticscast on untrusted API response (analytics-dashboard.tsx:254) — If the upstream response shape doesn't match the TypeScript interface, the component will throw at render time with an unhelpful error instead of showing the error card.
🔵 Nitpicks (3)
lastUpdatedAt.toLocaleTimeString()is locale-dependent (analytics-dashboard.tsx:367) — Inconsistent with the explicit"en-US"locale used informatNumber.broadcastSelectedGuilddispatches event but doesn't writelocalStorage(guild-selection.ts:7-18) — Future callers may forget the separatelocalStoragewrite needed for cross-tab sync.- Error message length check is an unusual guard pattern (
guilds.js:506-512) — Theerr.message.length < 200check doesn't distinguish validation from runtime errors. Consider tagging validation errors explicitly.
See inline comments for details.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
web/src/components/layout/server-selector.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-16T05:48:12.653Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T05:48:12.653Z
Learning: Applies to src/app/**/*.{ts,tsx} : Use Server Components by default in `src/app` for data fetching; mark interactive components with `"use client"` directive
Applied to files:
web/src/components/layout/server-selector.tsx
🧬 Code graph analysis (1)
web/src/components/layout/server-selector.tsx (2)
web/src/types/discord.ts (1)
MutualGuild(16-18)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)broadcastSelectedGuild(7-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
web/src/components/layout/server-selector.tsx (2)
17-20: Clean import consolidation — LGTM.Extracting
SELECTED_GUILD_KEYandbroadcastSelectedGuildinto the sharedguild-selectionmodule and importing them here removes the previously inline constant and centralises the broadcast logic correctly.
83-85: Auto-selection fallback andloadGuildsdependency chain are correct — LGTM.
selectGuild(data[0])is only reached whenrestoredisfalse, so it always represents a genuine first-time selection and the broadcast is appropriate. SinceselectGuildcarries an empty dependency array its identity is stable, which keepsloadGuildsstable and prevents theuseEffectfrom re-running on subsequent renders.Also applies to: 100-100
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/layout/server-selector.tsx`:
- Around line 35-43: Add a guard so clicking the currently-selected guild does
not re-broadcast and reset analytics: create a ref (e.g., selectedGuildRef) that
mirrors the selectedGuild id, update it whenever you setSelectedGuild (in
selectGuild) and when you restore from localStorage, and change selectGuild to
compare guild.id against selectedGuildRef.current and only call
broadcastSelectedGuild(guild.id) when they differ; ensure the restore path also
sets selectedGuildRef to the restored id so the first manual re-click is
suppressed.
---
Duplicate comments:
In `@web/src/components/layout/server-selector.tsx`:
- Around line 74-77: The restoration logic currently calls
setSelectedGuild(saved) directly (instead of selectGuild) which intentionally
avoids triggering broadcastSelectedGuild and the AnalyticsDashboard refetch; add
an inline comment next to the setSelectedGuild(saved) call explaining this
intent and referencing selectGuild, broadcastSelectedGuild, and
AnalyticsDashboard to make the reason explicit for future readers and prevent
accidental regressions.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/api/routes/guilds.jssrc/modules/ai.jstests/modules/ai.test.jsweb/src/components/dashboard/analytics-dashboard.tsxweb/src/components/layout/server-selector.tsxweb/src/lib/guild-selection.tsweb/tests/components/dashboard/analytics-dashboard.test.tsxweb/tests/components/layout/server-selector.test.tsxweb/tests/lib/guild-selection.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must be in the
tests/directory and use Vitest framework
Files:
tests/modules/ai.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix for Node.js built-in imports (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston logging (import { info, warn, error } from '../logger.js') — NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read configuration at runtime
UsesetConfigValue(path, value, guildId?)to update configuration at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
All new code must include tests with 80% coverage threshold on statements, branches, functions, and lines
Files:
src/modules/ai.jssrc/api/routes/guilds.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Checkconfig.yourModule.enabledbefore processing in module handlers
Prefer the per-requestgetConfig()pattern for new modules instead of reactiveonConfigChangewiring, which is only for stateful resources
Files:
src/modules/ai.js
🧠 Learnings (5)
📚 Learning: 2026-02-16T05:48:12.653Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T05:48:12.653Z
Learning: Applies to src/app/layout.tsx : Use `vercel/analytics` and `vercel/speed-insights` packages for user analytics and performance monitoring in production
Applied to files:
web/tests/components/dashboard/analytics-dashboard.test.tsxweb/src/components/dashboard/analytics-dashboard.tsx
📚 Learning: 2026-02-12T06:25:33.802Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-12T06:25:33.802Z
Learning: Applies to src/**/*.{ts,tsx} : Use `date-fns` for date manipulation and formatting throughout the application
Applied to files:
web/src/components/dashboard/analytics-dashboard.tsxsrc/api/routes/guilds.js
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to src/**/*.js : All new code must include tests with 80% coverage threshold on statements, branches, functions, and lines
Applied to files:
tests/modules/ai.test.js
📚 Learning: 2026-02-18T00:10:37.289Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-18T00:10:37.289Z
Learning: Applies to src/**/*.js : Always use Winston logging (`import { info, warn, error } from '../logger.js'`) — NEVER use `console.log`, `console.warn`, `console.error`, or any `console.*` method in src/ files
Applied to files:
tests/modules/ai.test.js
📚 Learning: 2026-02-16T05:48:12.653Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-16T05:48:12.653Z
Learning: Applies to src/app/**/*.{ts,tsx} : Use Server Components by default in `src/app` for data fetching; mark interactive components with `"use client"` directive
Applied to files:
web/src/components/layout/server-selector.tsx
🧬 Code graph analysis (5)
web/tests/components/dashboard/analytics-dashboard.test.tsx (2)
web/src/components/dashboard/analytics-dashboard.tsx (1)
AnalyticsDashboard(251-919)web/src/lib/guild-selection.ts (1)
SELECTED_GUILD_KEY(1-1)
web/src/components/dashboard/analytics-dashboard.tsx (2)
web/src/types/analytics.ts (2)
AnalyticsRangePreset(1-1)DashboardAnalytics(40-63)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)GUILD_SELECTED_EVENT(2-2)
src/modules/ai.js (4)
src/modules/chimeIn.js (2)
model(89-89)model(147-147)src/index.js (2)
guildConfig(188-188)guildId(157-157)src/logger.js (1)
info(217-219)src/deploy-commands.js (1)
guildId(26-26)
web/tests/lib/guild-selection.test.ts (1)
web/src/lib/guild-selection.ts (3)
broadcastSelectedGuild(9-26)SELECTED_GUILD_KEY(1-1)GUILD_SELECTED_EVENT(2-2)
web/src/components/layout/server-selector.tsx (2)
web/src/types/discord.ts (1)
MutualGuild(16-18)web/src/lib/guild-selection.ts (2)
SELECTED_GUILD_KEY(1-1)broadcastSelectedGuild(9-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (18)
src/modules/ai.js (2)
537-560: Solid integration of structured AI usage logging into the response flow.The optional chaining for reply extraction, the
modelUsedfallback, token derivation viatoNonNegativeNumber, thetotalTokensfallback with||, and the structuredinfo('AI usage', …)log are all well-implemented and align with the analytics aggregation requirements.
144-180: Cost estimation utilities look correct.
toNonNegativeNumbercorrectly guards againstNaN,Infinity, and negative values.estimateAiCostUsdproperly deduplicates unknown-model warnings and stabilises precision withtoFixed(6).tests/modules/ai.test.js (2)
254-351: Comprehensive analytics test coverage.The structured logging assertion, parameterized pricing validation (math checks out:
0.0007and0.00056), and the unknown-model warning deduplication test are well-constructed and correctly exercise the newestimateAiCostUsdandwarnedUnknownModelspaths.
45-53: Good test hygiene — resetting_resetWarnedUnknownModelsinbeforeEach.This prevents state leakage of the
warnedUnknownModelsset between tests.src/api/routes/guilds.js (3)
58-169: Analytics helpers are well-structured and robust.
parseAnalyticsRangecorrectly validates custom bounds, enforces max span, and uses UTC arithmetic throughout (addressing prior feedback).parseAnalyticsIntervalauto-derives a sensible bucket size from the time window.formatBucketLabelusesIntl.DateTimeFormatwith explicit UTC timezone, consistent with the stored timestamp semantics.AnalyticsRangeValidationErrorprovides clean error differentiation in the route handler.
562-662: Six parallel DB queries with graceful degradation on logs failure — good resilience pattern.The
.catch()on the model-usage query returning{ rows: [] }ensures the analytics endpoint still serves partial data when thelogstable is unavailable or the query fails. The parameterized SQL construction with dynamic$Nindexing for the optional channel filter is correct.
710-724:newMembersrange check uses inclusive upper bound (joinedAt <= toMs).This is consistent with the SQL
created_at <= $3used in other queries. The Discord cache limitation is clearly documented — good practice.web/tests/lib/guild-selection.test.ts (1)
1-35: Clean, focused tests covering both happy and guard paths ofbroadcastSelectedGuild.The whitespace-trimming assertion (line 17) and the empty/whitespace guard test (lines 29–30) directly mirror the implementation logic. Good coverage.
web/src/components/layout/server-selector.tsx (1)
194-199: Guard against re-selecting the same guild — addresses prior feedback.This prevents redundant
broadcastSelectedGuildcalls and avoids resetting the analytics channel filter when clicking the already-active guild.web/src/lib/guild-selection.ts (1)
1-26: Clean single-responsibility module for guild selection broadcasting.The SSR guard, input trimming, empty-check, silent
localStoragecatch, andCustomEventdispatch are all well-handled. The ordering (persist first, then dispatch) is correct — even if persistence fails, the in-tab event still fires so the UI reacts.web/tests/components/layout/server-selector.test.tsx (2)
12-22: Good mock pattern — preserves actual constants while replacingbroadcastSelectedGuild.Using
vi.importActualto spread the real exports and only overriding the function under test keepsSELECTED_GUILD_KEYandGUILD_SELECTED_EVENTconsistent with the production module.
82-175: Three well-targeted tests covering the restore/default/re-click broadcast behaviors.
- Restore path: no broadcast when selection comes from localStorage ✅
- Default path: broadcasts when auto-selecting first guild ✅
- Re-click guard: no redundant broadcast or fetch when clicking the active guild ✅
web/tests/components/dashboard/analytics-dashboard.test.tsx (4)
1-87: Well-structured test fixture and recharts mock.The
analyticsPayloadfixture provides a complete, realistic response shape. The recharts mock is appropriately minimal — renders wrapper divs to preserve children while avoiding canvas/SVG rendering issues in JSDOM.
122-147: Good distinction between "no data" and "zero" for realtime indicators.Lines 132 and 145–146 correctly verify that placeholder state shows an em-dash (
—) rather than0, ensuring the UI communicates "data not loaded" vs "zero activity." The aria-label queries ("Online members value","Active AI conversations value") are more resilient than the DOM traversal pattern flagged in a prior review.
273-319: Validation test correctly verifies no fetch fires for an invalid custom range.The pattern of recording
callCountBeforeApply, clicking Apply with from > to, and then asserting no additional fetch was made is a solid approach to test the client-side validation guard.
339-362: 401 redirect test properly mocks and restoreswindow.location.The
@ts-expect-errorannotations for location mocking and the restoration in the cleanup are standard practice for testing redirect behavior in JSDOM.web/src/components/dashboard/analytics-dashboard.tsx (2)
56-110: LGTM — timezone fix correctly applied.
parseLocalDateInput+new Date(year, monthIndex, day, ...)constructor (local time) before.toISOString()properly resolves the UTC-boundary issue from the previous review.
332-412: LGTM — AbortController implementation is solid.The ref-based abort-before-start pattern, the
finallyguard (abortControllerRef.current === controller), and theAbortErrorsuppression are all correctly implemented.
🤖 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/api/routes/guilds.js`:
- Around line 564-572: The analytics query is counting distinct usernames which
can change; update the schema to add a user_id column to the conversations
table, update the addToHistory function to accept and persist the userId
(propagate the userId parameter from generateResponse() into addToHistory), and
change the aggregation in the query inside routes/guilds.js to COUNT(DISTINCT
CASE WHEN role = 'user' THEN user_id END)::int AS active_users; also add a DB
migration to backfill or set user_id for existing rows and update any
INSERT/UPDATE code paths that write to conversations to include the new user_id
column.
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Around line 894-907: The per-cell message count is only provided via a
hover-only title on the inner <div> (the element with className "h-4 rounded-sm
border" inside the <td key={`${day}-${hour}`}>), which is not accessible to
keyboard/screen-reader users; replace the title with an accessible label by
moving it to an aria-label on the cell element (either the <td
key={`${day}-${hour}`}> or the inner div) and make the element
keyboard-focusable if needed (e.g., add tabIndex={0}) so screen readers and
keyboard users can read the message count programmatically; ensure you remove
the title attribute once aria-label is added.
- Around line 503-518: The range-preset and channel-filter buttons do not expose
their selected state to assistive tech; update the Button elements (the ones
rendered from RANGE_PRESETS and the channel filter map) to include
aria-pressed={rangePreset === preset.value} (or the equivalent check for channel
filter, e.g., aria-pressed={selectedChannel === channel}) so the active
selection is announced; keep the existing variant logic and onClick handlers
(setRangePreset, setCustomRangeError, and the channel selection handlers) but
add the aria-pressed boolean prop to each Button to reflect its selected state.
- Around line 664-686: Make the loading/placeholder behavior consistent and
remove the unreachable null branch: add the same loading guard used for
activeAiConversations to the onlineMembers rendering (i.e., check `loading ||
analytics == null` before showing `analytics.realtime.onlineMembers`) so both
realtime tiles show the placeholder during foreground refreshes, and remove the
`activeAiConversations === null ? "N/A" :` branch (and its "N/A" case) since
DashboardRealtime defines `activeAiConversations: number` and validation via
`isFiniteNumber(realtime.activeAiConversations)` guarantees it is never null.
In `@web/src/components/layout/server-selector.tsx`:
- Around line 35-43: The manual localStorage write in selectGuild is redundant
because broadcastSelectedGuild already persists and broadcasts the selected
guild; remove the localStorage.setItem(SELECTED_GUILD_KEY, guild.id) call from
selectGuild, keep setSelectedGuild(guild) and the
broadcastSelectedGuild(guild.id) invocation so state updates and persistence
happen through the single broadcastSelectedGuild path; leave the restore/read
logic (which intentionally avoids broadcast) unchanged.
---
Duplicate comments:
In `@src/modules/ai.js`:
- Around line 125-134: Remove the invalid model key 'claude-opus-4-20250514'
from the MODEL_PRICING_PER_MILLION map in src/modules/ai.js (or replace it with
a real model id such as 'claude-opus-4-6' and the correct input/output pricing),
ensuring only valid Anthropic model IDs like 'claude-opus-4-1-20250805' remain;
update the MODEL_PRICING_PER_MILLION object accordingly so lookups for
MODEL_PRICING_PER_MILLION['claude-opus-4-20250514'] no longer occur.
In `@web/src/components/dashboard/analytics-dashboard.tsx`:
- Around line 781-784: The code still maps Cell components inside the Pie
(modelUsageData.map(... <Cell />)) which is deprecated; remove the per-slice
Cell usage and instead put the fill/color into each modelUsageData entry (e.g.,
an explicit fill property) and let the Pie read it via its data items or provide
a custom shape prop on the Pie to render slices with the entry.fill; similarly
for the channel-activity BarChart remove per-bar Cell usage and implement a
custom shape renderer (pass shape prop to Bar) or use a data-driven fill key on
the Bar's data entries (e.g., a selected flag or fill color) so coloring logic
lives in the data and the Pie/Bar use shape or data-key for fill rather than
Cell.
- Around line 358-361: The code currently uses a hard redirect via
window.location.href in the 401 handler which forces a full page reload and
bypasses Next.js App Router/React state; in the component/function
(analytics-dashboard.tsx) replace window.location.href = "/login" with
router.push("/login") from next/navigation (import and get router via
useRouter() or the App Router hook used in this file) to perform an idiomatic
client-side transition, or if a full reload is intentionally required add a
clarifying comment above the window.location.href line describing why a hard
navigation is necessary.
| dbPool.query( | ||
| `SELECT | ||
| COUNT(*)::int AS total_messages, | ||
| COUNT(*) FILTER (WHERE role = 'assistant')::int AS ai_requests, | ||
| COUNT(DISTINCT CASE WHEN role = 'user' THEN username END)::int AS active_users | ||
| FROM conversations | ||
| WHERE ${conversationWhere}`, | ||
| conversationValues, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the conversations table schema definition
rg -n 'CREATE TABLE.*conversations' -A 20Repository: BillChirico/bills-bot
Length of output: 2537
🏁 Script executed:
# Also search for table schema in migration files or initialization scripts
fd -e sql -e js | xargs rg -l 'conversations' | head -20Repository: BillChirico/bills-bot
Length of output: 261
🏁 Script executed:
# Check addToHistory call sites and what's passed as username
ast-grep --pattern 'addToHistory($$$)'Repository: BillChirico/bills-bot
Length of output: 1035
🏁 Script executed:
# Also search for addToHistory function definition
rg -n 'function addToHistory|const addToHistory|export.*addToHistory' -A 10Repository: BillChirico/bills-bot
Length of output: 698
🏁 Script executed:
# Check how username is obtained and if user_id is available
rg -n 'username.*=' src/modules/ai.js -B 3 -A 1Repository: BillChirico/bills-bot
Length of output: 348
🏁 Script executed:
# Search for Discord user ID references
rg -n 'user\.id|userId|user_id' src/modules/ai.js | head -20Repository: BillChirico/bills-bot
Length of output: 530
🏁 Script executed:
# Check the full context around the addToHistory call
sed -n '560,580p' src/api/routes/guilds.jsRepository: BillChirico/bills-bot
Length of output: 849
🏁 Script executed:
# Get broader context around lines 564-572 in guilds.js to see function signature and available data
sed -n '520,600p' src/api/routes/guilds.jsRepository: BillChirico/bills-bot
Length of output: 2936
🏁 Script executed:
# Check if userId is ever stored or could be stored in conversations table
rg -n 'INSERT INTO conversations|conversations' src/modules/ai.js -B 2 -A 2Repository: BillChirico/bills-bot
Length of output: 951
🏁 Script executed:
# Check database persistence of conversations - find where data is actually stored
rg -n 'INSERT|UPDATE.*conversations' src/ -A 3Repository: BillChirico/bills-bot
Length of output: 3630
🏁 Script executed:
# Check if userId is passed through the entire flow to addToHistory
rg -n 'addToHistory.*userId|processUserMessage' src/modules/ai.js -B 5 -A 5Repository: BillChirico/bills-bot
Length of output: 47
🏁 Script executed:
# Check if there are any tests for the analytics endpoint to understand expectations
rg -n 'active_users|activeUsers' tests/Repository: BillChirico/bills-bot
Length of output: 623
🏁 Script executed:
# Check the function signature where username comes from in the ai module
sed -n '460,480p' src/modules/ai.jsRepository: BillChirico/bills-bot
Length of output: 805
🏁 Script executed:
# Verify userId is available at the point of addToHistory call
sed -n '570,575p' src/modules/ai.jsRepository: BillChirico/bills-bot
Length of output: 351
Store Discord user IDs instead of usernames for stable user identification in analytics.
COUNT(DISTINCT CASE WHEN role = 'user' THEN username END) incorrectly counts distinct usernames as distinct users. Since Discord users can change their usernames, this will miscount users who update their display name. The userId parameter is already available in generateResponse() but isn't passed to addToHistory(). Update the schema to include user_id, pass userId through addToHistory(), and change the query to count distinct user_id instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/routes/guilds.js` around lines 564 - 572, The analytics query is
counting distinct usernames which can change; update the schema to add a user_id
column to the conversations table, update the addToHistory function to accept
and persist the userId (propagate the userId parameter from generateResponse()
into addToHistory), and change the aggregation in the query inside
routes/guilds.js to COUNT(DISTINCT CASE WHEN role = 'user' THEN user_id
END)::int AS active_users; also add a DB migration to backfill or set user_id
for existing rows and update any INSERT/UPDATE code paths that write to
conversations to include the new user_id column.
| <div className="flex flex-wrap items-center gap-2"> | ||
| {RANGE_PRESETS.map((preset) => ( | ||
| <Button | ||
| key={preset.value} | ||
| variant={rangePreset === preset.value ? "default" : "outline"} | ||
| size="sm" | ||
| onClick={() => { | ||
| setRangePreset(preset.value); | ||
| if (preset.value !== "custom") { | ||
| setCustomRangeError(null); | ||
| } | ||
| }} | ||
| > | ||
| {preset.label} | ||
| </Button> | ||
| ))} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Button component file(s)
fd -type f \( -name "*button*" -o -name "*Button*" \) | grep -E "\.(tsx|ts|jsx|js)$" | head -20Repository: BillChirico/bills-bot
Length of output: 235
🏁 Script executed:
# Look for Button component definition in common locations
find . -path ./node_modules -prune -o -type f \( -name "*.tsx" -o -name "*.ts" \) -print | xargs grep -l "export.*Button\|const Button" | head -10Repository: BillChirico/bills-bot
Length of output: 99
🏁 Script executed:
# Check the specific file and lines mentioned in the review
wc -l web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 121
🏁 Script executed:
# View the code at lines 503-518 and context
sed -n '500,520p' web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 683
🏁 Script executed:
# View the code at lines 699-719
sed -n '695,725p' web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 1116
🏁 Script executed:
# Examine the Button component implementation
cat -n web/src/components/ui/button.tsxRepository: BillChirico/bills-bot
Length of output: 2399
🏁 Script executed:
# Also check for any aria-pressed usage in the analytics-dashboard file
rg "aria-pressed" web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 47
🏁 Script executed:
# Check if aria-pressed is used elsewhere in the codebase
rg "aria-pressed" web/src/components/ --type tsx --type tsRepository: BillChirico/bills-bot
Length of output: 92
🏁 Script executed:
# Check for aria-pressed usage in web directory with correct syntax
rg "aria-pressed" web/src/ -A 2 -B 2Repository: BillChirico/bills-bot
Length of output: 47
🏁 Script executed:
# Also check for other aria-* attributes to see accessibility patterns in codebase
rg "aria-\w+" web/src/ --only-matching | sort | uniq -c | sort -rnRepository: BillChirico/bills-bot
Length of output: 247
Toggle buttons missing aria-pressed — AT users cannot determine the active selection.
Neither the range-preset buttons (lines 503–518) nor the channel-filter buttons (lines 699–719) communicate their selected state to assistive technologies. The visual variant="default" / variant="outline" distinction is invisible to screen readers.
♿ Proposed fix (range presets; apply the same pattern to channel filter buttons)
<Button
key={preset.value}
variant={rangePreset === preset.value ? "default" : "outline"}
size="sm"
+ aria-pressed={rangePreset === preset.value}
onClick={() => {
setRangePreset(preset.value);
if (preset.value !== "custom") {
setCustomRangeError(null);
}
}}
>For channel-filter buttons:
<Button
size="sm"
variant={channelFilter === null ? "default" : "outline"}
+ aria-pressed={channelFilter === null}
onClick={() => setChannelFilter(null)}
>
All channels
</Button>
{(analytics?.channelActivity ?? []).map((channel) => (
<Button
key={channel.channelId}
size="sm"
variant={channelFilter === channel.channelId ? "default" : "outline"}
+ aria-pressed={channelFilter === channel.channelId}
onClick={...}
>Also applies to: 699-719
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/analytics-dashboard.tsx` around lines 503 - 518,
The range-preset and channel-filter buttons do not expose their selected state
to assistive tech; update the Button elements (the ones rendered from
RANGE_PRESETS and the channel filter map) to include aria-pressed={rangePreset
=== preset.value} (or the equivalent check for channel filter, e.g.,
aria-pressed={selectedChannel === channel}) so the active selection is
announced; keep the existing variant logic and onClick handlers (setRangePreset,
setCustomRangeError, and the channel selection handlers) but add the
aria-pressed boolean prop to each Button to reflect its selected state.
| <p aria-label="Online members value" className="mt-2 text-2xl font-semibold"> | ||
| {analytics == null | ||
| ? "—" | ||
| : analytics.realtime.onlineMembers === null | ||
| ? "N/A" | ||
| : formatNumber(analytics.realtime.onlineMembers)} | ||
| </p> | ||
| </div> | ||
| <div className="rounded-lg border p-4"> | ||
| <div className="flex items-center gap-2 text-sm text-muted-foreground"> | ||
| <Bot className="h-4 w-4" /> | ||
| Active AI conversations | ||
| </div> | ||
| <p | ||
| aria-label="Active AI conversations value" | ||
| className="mt-2 text-2xl font-semibold" | ||
| > | ||
| {loading || analytics == null | ||
| ? "—" | ||
| : analytics.realtime.activeAiConversations === null | ||
| ? "N/A" | ||
| : formatNumber(analytics.realtime.activeAiConversations)} | ||
| </p> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find analytics-related type definitions
fd -e ts -e tsx | xargs grep -l "DashboardRealtime\|isDashboardAnalyticsPayload" | head -20Repository: BillChirico/bills-bot
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Check the analytics type file
fd analytics -e ts -e tsx | head -20Repository: BillChirico/bills-bot
Length of output: 242
🏁 Script executed:
cat web/src/types/analytics.tsRepository: BillChirico/bills-bot
Length of output: 1331
🏁 Script executed:
#!/bin/bash
# Check the isDashboardAnalyticsPayload function around line 172 in analytics-dashboard.tsx
sed -n '165,185p' web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check the complete condition for both fields
sed -n '664,690p' web/src/components/dashboard/analytics-dashboard.tsxRepository: BillChirico/bills-bot
Length of output: 1130
Two issues in the realtime indicator values.
1. Inconsistent loading guard: onlineMembers (line 665) only checks analytics == null, while activeAiConversations (line 681) also gates on loading. During a foreground refresh (loading=true, analytics=<previous data>), online members shows the stale value while active AI conversations shows "—". Either remove the loading || from line 681, or add it to the online-members check for consistency.
2. activeAiConversations === null is dead code: The DashboardRealtime type defines activeAiConversations: number (not number | null), so the field cannot be null. The validation at line 172 enforces this by requiring isFiniteNumber(realtime.activeAiConversations). The ? "N/A" branch at line 684 is unreachable at runtime—either remove the null check or update the type and validation to allow nullability if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/analytics-dashboard.tsx` around lines 664 - 686,
Make the loading/placeholder behavior consistent and remove the unreachable null
branch: add the same loading guard used for activeAiConversations to the
onlineMembers rendering (i.e., check `loading || analytics == null` before
showing `analytics.realtime.onlineMembers`) so both realtime tiles show the
placeholder during foreground refreshes, and remove the `activeAiConversations
=== null ? "N/A" :` branch (and its "N/A" case) since DashboardRealtime defines
`activeAiConversations: number` and validation via
`isFiniteNumber(realtime.activeAiConversations)` guarantees it is never null.
|
|
||
| return ( | ||
| <td key={`${day}-${hour}`}> | ||
| <div | ||
| title={`${day} ${hour}:00 — ${value} messages`} | ||
| className="h-4 rounded-sm border" | ||
| style={{ | ||
| backgroundColor: | ||
| value === 0 | ||
| ? "transparent" | ||
| : `rgba(88, 101, 242, ${alpha.toFixed(3)})`, | ||
| }} | ||
| /> | ||
| </td> |
There was a problem hiding this comment.
Heatmap cell data is inaccessible to keyboard/screen-reader users.
The title attribute on the inner <div> (line 898) is only surfaced on mouse hover; keyboard users and screen readers cannot access it. Replacing title with aria-label on the <td> (or its inner <div>) makes the per-cell message count programmatically accessible.
♿ Proposed fix
- <td key={`${day}-${hour}`}>
+ <td key={`${day}-${hour}`} aria-label={`${day} ${hour}:00 — ${value} messages`}>
<div
- title={`${day} ${hour}:00 — ${value} messages`}
className="h-4 rounded-sm border"
style={{...}}
/>
</td>📝 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.
| return ( | |
| <td key={`${day}-${hour}`}> | |
| <div | |
| title={`${day} ${hour}:00 — ${value} messages`} | |
| className="h-4 rounded-sm border" | |
| style={{ | |
| backgroundColor: | |
| value === 0 | |
| ? "transparent" | |
| : `rgba(88, 101, 242, ${alpha.toFixed(3)})`, | |
| }} | |
| /> | |
| </td> | |
| return ( | |
| <td key={`${day}-${hour}`} aria-label={`${day} ${hour}:00 — ${value} messages`}> | |
| <div | |
| className="h-4 rounded-sm border" | |
| style={{ | |
| backgroundColor: | |
| value === 0 | |
| ? "transparent" | |
| : `rgba(88, 101, 242, ${alpha.toFixed(3)})`, | |
| }} | |
| /> | |
| </td> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/dashboard/analytics-dashboard.tsx` around lines 894 - 907,
The per-cell message count is only provided via a hover-only title on the inner
<div> (the element with className "h-4 rounded-sm border" inside the <td
key={`${day}-${hour}`}>), which is not accessible to keyboard/screen-reader
users; replace the title with an accessible label by moving it to an aria-label
on the cell element (either the <td key={`${day}-${hour}`}> or the inner div)
and make the element keyboard-focusable if needed (e.g., add tabIndex={0}) so
screen readers and keyboard users can read the message count programmatically;
ensure you remove the title attribute once aria-label is added.
| const selectGuild = useCallback((guild: MutualGuild) => { | ||
| setSelectedGuild(guild); | ||
| try { | ||
| localStorage.setItem(SELECTED_GUILD_KEY, guild.id); | ||
| } catch { | ||
| // localStorage may be unavailable (e.g. incognito) | ||
| } | ||
| }; | ||
| broadcastSelectedGuild(guild.id); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant localStorage write — broadcastSelectedGuild already persists.
selectGuild writes to localStorage on lines 38, then broadcastSelectedGuild (line 42) writes the same key again. Since broadcastSelectedGuild was designed as the single persistence + broadcast entry point, the manual write is unnecessary.
♻️ Suggested simplification
const selectGuild = useCallback((guild: MutualGuild) => {
setSelectedGuild(guild);
- try {
- localStorage.setItem(SELECTED_GUILD_KEY, guild.id);
- } catch {
- // localStorage may be unavailable (e.g. incognito)
- }
broadcastSelectedGuild(guild.id);
}, []);Note: the restore path (lines 74–76) correctly avoids broadcasting, so it still needs its own read-from-storage logic but not a write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/layout/server-selector.tsx` around lines 35 - 43, The
manual localStorage write in selectGuild is redundant because
broadcastSelectedGuild already persists and broadcasts the selected guild;
remove the localStorage.setItem(SELECTED_GUILD_KEY, guild.id) call from
selectGuild, keep setSelectedGuild(guild) and the
broadcastSelectedGuild(guild.id) invocation so state updates and persistence
happen through the single broadcastSelectedGuild path; leave the restore/read
logic (which intentionally avoids broadcast) unchanged.
SQL integer overflow risk in token SUM aggregation; ::int has max ~2.1B and token counts may exceed this in high-volume guilds. Changed to ::bigint for COUNT(*), prompt_tokens, and completion_tokens in the AI usage query. Resolves PR review comment on line 641.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
| }; | ||
| broadcastSelectedGuild(guild.id); | ||
| }, []); |
There was a problem hiding this comment.
Redundant localStorage write in selectGuild callback
Low Severity
The selectGuild callback writes guild.id to localStorage directly, then immediately calls broadcastSelectedGuild(guild.id) which also writes the same key to localStorage. The manual localStorage.setItem call in selectGuild is now redundant since broadcastSelectedGuild handles both persistence and event dispatch. Additionally, broadcastSelectedGuild trims the guild ID before writing, creating a subtle inconsistency where the raw and trimmed values could differ on the first vs. second write.
Additional Locations (1)
| if (!backgroundRefresh) { | ||
| setLoading(true); | ||
| } | ||
| setError(null); |
There was a problem hiding this comment.
Background refresh clears error causing repeated visual flash
Medium Severity
setError(null) is called unconditionally at the start of every fetch, including silent background refreshes (the 30-second auto-refresh). Unlike foreground fetches, background refreshes don't set loading to true, so there's no visual replacement for the cleared error. When the server is unreachable, the error card disappears and reappears every 30 seconds, creating a distracting flash. The setError(null) call here needs to be conditional on !backgroundRefresh, mirroring the setLoading(true) guard on the previous line.
| : analytics.realtime.onlineMembers === null | ||
| ? "N/A" | ||
| : formatNumber(analytics.realtime.onlineMembers)} | ||
| </p> |
There was a problem hiding this comment.
Inconsistent loading check between real-time indicator displays
Low Severity
The "Online members" indicator checks only analytics == null to decide whether to show "—", while the adjacent "Active AI conversations" indicator checks loading || analytics == null. During a foreground re-fetch (e.g., clicking Refresh or changing the date range), loading is true but analytics still holds the previous data — so "Online members" shows a stale count while "Active AI conversations" shows "—". Both live in the same "Real-time indicators" card, making the inconsistency visible.
Additional Locations (1)
1 participant
Summary
Backend / API
GET /api/v1/guilds/:id/analyticswith guild-scoped analytics payloadstotalMessages,aiRequests,aiCostUsd,activeUsers,newMembers)onlineMembers,activeAiConversations)range,from,to,interval,channelIdAI usagelogging insrc/modules/ai.js(model/tokens/estimated cost) so analytics can aggregate model/token/cost metrics from persistent logsWeb integration
GET /api/guilds/[guildId]/analyticsto securely forward analytics requests to bot API v1x-api-secret)Docs
Testing
Bot tests
pnpm testWeb tests (targeted)
pnpm --filter bills-bot-web test tests/app/dashboard.test.tsx tests/components/dashboard/analytics-dashboard.test.tsx tests/api/guild-analytics.test.ts tests/lib/discord.test.ts tests/components/layout/server-selector.test.tsxpnpm --filter bills-bot-web typecheckNotes
pnpm outdatedreports existing available updates for@biomejs/biomeandmem0ai(not changed in this PR).web/tests/lib/auth.test.tscurrently fails in this branch due pre-existing expectations around auth module import-time validation (not touched by this change).Closes #30