Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude encountered an error —— View job Code Review
|
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
|
🚅 Deployed to the volvox-bot-pr-273 environment in volvox-bot
|
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds many new and expanded tests across frontend, server, and API layers; refines dashboard title sync logic to track last-synced title and pathname to avoid overwriting externally set titles; adds test setup polyfills and raises vitest coverage thresholds; adds a default unknown-subcommand reply for /voice. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
| Filename | Overview |
|---|---|
| web/src/components/layout/dashboard-title-sync.tsx | Adds two useRef values (lastSyncedTitleRef, lastSyncedPathnameRef) to distinguish between a stale title left by a previous route's metadata and a title freshly set by the current page's metadata. The updated guard condition correctly handles both the StrictMode double-invocation case (same pathname, title already preserved) and the cross-route stale-title case (different pathname, current === lastSyncedTitle means we set it, so override with the new computed value). Logic is sound and is exercised by the two new tests in dashboard-title-sync.test.tsx. |
| web/vitest.config.ts | Replaces the broad src/app/** exclusion (flagged in the previous review) with specific Next.js boilerplate patterns (page.tsx, layout.tsx, etc.), keeping API route handlers in scope for measurement. Raises all four thresholds to 85%. New exclusions include src/stores/** (no justification comment), three moderation hooks, and src/lib/log-ws.ts. The dashboard component exclusion now has a clear TODO comment pointing at a future Playwright suite. |
| vitest.config.js | Raises the branches threshold from 82% to 85%, aligning it with the other three thresholds. The exclude list is reformatted but functionally unchanged. |
| src/commands/voice.js | Adds a defensive fallback that returns a user-facing error for any subcommand not matched by the existing dispatch chain. This closes a silent no-op gap and is validated by the new voice.test.js unknown-subcommand test. |
| tests/api/ws/auditStream.test.js | Adds 6 new WebSocket integration tests covering: malformed HMAC bytes (4003 close), non-string filter normalization, corrected guild-filter rejection semantics (the test now verifies the server sends an error message and continues broadcasting for the authenticated guild), user-id filtering, missing message type handling, and the 10-client authenticated cap (4029 close). |
| web/tests/lib/bot-api-proxy-branches.test.ts | New test file providing branch coverage for authorizeGuildAdmin, getBotApiConfig, buildUpstreamUrl, and proxyToBotApi. Covers 401/403/502 authorization failures, missing config, malformed base URL, non-JSON upstream errors, and AbortError/TimeoutError/generic error handling. Environment variables are restored properly in afterEach. |
| web/tests/components/layout/dashboard-title-sync.test.tsx | Adds two new test cases that directly exercise the useRef guard logic: one validating that a stale metadata title is replaced when navigating to a new client-only route, and one confirming StrictMode's double-invocation does not overwrite a page-level metadata title. |
| web/tests/components/landing/stats.test.tsx | New test for the Stats component. Uses a custom requestAnimationFrame stub to fast-forward number animations. Asserts on locale-formatted strings like '1.2K' and '1.2M'; these will be stable under jsdom's default en-US locale but could become flaky if the component later adopts runtime locale detection. Contains any usage in the framer-motion mock factory without a justification comment. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pathname changes\n(usePathname)"] --> B["useEffect fires\ncomputed = getDashboardDocumentTitle(pathname)\ncurrent = document.title"]
B --> C{"current.endsWith(APP_TITLE)\n&& current !== computed\n&& current !== APP_TITLE"}
C -- No --> G["Set document.title = computed\nlastSyncedTitle = computed\nlastSyncedPathname = pathname"]
C -- Yes --> D{"pathname === lastSyncedPathname\nOR current !== lastSyncedTitle"}
D -- No --> G
D -- Yes --> E["Preserve page metadata title\nlastSyncedTitle = current\nlastSyncedPathname = pathname\nreturn early"]
G --> F["Title reflects\nclient-side route"]
E --> H["Title reflects\npage-level metadata"]
style G fill:#d4edda,stroke:#28a745
style E fill:#d4edda,stroke:#28a745
style F fill:#d4edda,stroke:#28a745
style H fill:#d4edda,stroke:#28a745
Last reviewed commit: 1e543ab
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/dashboard-title-sync.tsx`:
- Around line 22-31: The early-return in DashboardTitleSync uses a stale
lastSyncedTitleRef.current check which prevents recording the page-specific
title and can leave a previous route's title stuck; update the logic in the
block that checks current.endsWith(APP_TITLE) (and the comparisons with current
!== computed, current !== APP_TITLE, current !== lastSyncedTitle) to always
update lastSyncedTitleRef.current when you bail early, or better: remove the
stale-ref comparison and instead set lastSyncedTitleRef.current = computed (or
the canonical 'Volvox.Bot - AI Powered Discord Bot' when appropriate) whenever
navigation to a dashboard route occurs so titles are recorded and future syncs
with DashboardTitleSync (mounted in dashboard-shell.tsx) correctly fallback to
the canonical APP_TITLE string.
In `@web/tests/components/landing/hero.test.tsx`:
- Around line 49-63: The test currently advances the typewriter timers (act +
vi.advanceTimersByTime(1_500)) but never asserts the final typed terminal
string, so add an assertion after the timer advance to verify the completed
typewriter output (e.g., that the DOM contains the full "volvox-bot" terminal
text or the full terminal line used by the hook). Locate the test block using
act(...) / vi.advanceTimersByTime and the existing expectations referencing
screen.getByText and document.querySelector('.terminal-cursor'), then add a
final assertion using screen.getByText or screen.getByRole to confirm the
completed terminal text is present and that the '.terminal-cursor' is removed.
In `@web/tests/components/landing/stats.test.tsx`:
- Around line 52-63: Replace uses of the Node-specific global object when spying
on fetch with the environment-agnostic globalThis: change vi.spyOn(global,
'fetch') to vi.spyOn(globalThis, 'fetch') (and similarly for the other
occurrence around lines 80-81) so the test spies the fetch function in all JS
runtimes; update both spy calls referencing fetch to use globalThis and keep the
mocked Response object as-is.
- Around line 36-42: The test uses window.setTimeout inside the mocked
globalThis.requestAnimationFrame, causing inconsistent global references; update
the mock in stats.test.tsx to call globalThis.setTimeout instead of
window.setTimeout (the surrounding mocks are globalThis.requestAnimationFrame
and globalThis.cancelAnimationFrame) so all globals are referenced consistently
across the requestAnimationFrame and cancelAnimationFrame mocks.
In `@web/tests/hooks/use-guild-role.test.ts`:
- Around line 27-46: Add a test that verifies admin precedence when both admin
and moderator permission bits are present: create a guild with mixed permissions
(combine an admin bit such as '8' or '32' with a moderator bit such as '2' or
String(1n << 40n)) using createGuild and call getGuildDashboardRole, then assert
the result is 'admin'; name the spec to indicate it checks precedence so
regressions evaluating moderator before admin will fail.
In `@web/tests/lib/bot-api-proxy-branches.test.ts`:
- Around line 188-210: Add a test case in
web/tests/lib/bot-api-proxy-branches.test.ts to mock an AbortError in the same
style as the existing TimeoutError check: call proxyToBotApi(new
URL('https://bot.internal/test'), 'secret', '[test]', 'Aborted') after mocking
globalThis.fetch to reject with an object whose name is 'AbortError', then
assert the returned response has status 504 and that response.json() resolves to
{ error: 'Aborted' }; this mirrors the TimeoutError branch and ensures
proxyToBotApi's AbortError handling is covered.
In `@web/vitest.config.ts`:
- Around line 17-47: The coverage configuration currently excludes the entire
App Router via the coverage.exclude entry "src/app/**", which hides route
handlers (e.g., the ws-ticket route) from the coverage denominator; remove or
narrow that exclusion in vitest.config.ts by deleting the "src/app/**" pattern
(or replacing it with specific non-code files like "src/app/globals.css" and any
true static assets) so App Router pages and API route files are counted by
coverage and the 85% gate applies to them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28d8b6b2-ff5e-4e7a-8efa-0e5798884e45
📒 Files selected for processing (15)
web/src/components/layout/dashboard-title-sync.tsxweb/tests/api/log-stream-ws-ticket.test.tsweb/tests/components/landing/feature-grid.test.tsxweb/tests/components/landing/hero.test.tsxweb/tests/components/landing/pricing.test.tsxweb/tests/components/landing/stats.test.tsxweb/tests/components/layout/server-selector.test.tsxweb/tests/components/providers.test.tsxweb/tests/hooks/use-guild-role.test.tsweb/tests/hooks/use-guild-selection.test.tsxweb/tests/lib/bot-api-proxy-branches.test.tsweb/tests/lib/bot-api.test.tsweb/tests/lib/guild-selection.test.tsweb/tests/setup.tsweb/vitest.config.ts
📜 Review details
⏰ 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). (3)
- GitHub Check: Greptile Review
- GitHub Check: claude-review
- GitHub Check: Docker Build Validation
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes — No double quotes except in JSON
Semicolons are always required
Files:
web/tests/hooks/use-guild-role.test.tsweb/tests/components/landing/feature-grid.test.tsxweb/tests/hooks/use-guild-selection.test.tsxweb/tests/setup.tsweb/tests/lib/bot-api.test.tsweb/src/components/layout/dashboard-title-sync.tsxweb/tests/api/log-stream-ws-ticket.test.tsweb/tests/components/providers.test.tsxweb/tests/components/landing/hero.test.tsxweb/tests/components/layout/server-selector.test.tsxweb/tests/components/landing/pricing.test.tsxweb/tests/lib/guild-selection.test.tsweb/vitest.config.tsweb/tests/lib/bot-api-proxy-branches.test.tsweb/tests/components/landing/stats.test.tsx
**/*.{js,ts,tsx,jsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indent — Biome enforced
Files:
web/tests/hooks/use-guild-role.test.tsweb/tests/components/landing/feature-grid.test.tsxweb/tests/hooks/use-guild-selection.test.tsxweb/tests/setup.tsweb/tests/lib/bot-api.test.tsweb/src/components/layout/dashboard-title-sync.tsxweb/tests/api/log-stream-ws-ticket.test.tsweb/tests/components/providers.test.tsxweb/tests/components/landing/hero.test.tsxweb/tests/components/layout/server-selector.test.tsxweb/tests/components/landing/pricing.test.tsxweb/tests/lib/guild-selection.test.tsweb/vitest.config.tsweb/tests/lib/bot-api-proxy-branches.test.tsweb/tests/components/landing/stats.test.tsx
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard page titles should sync with route changes using DashboardTitleSync component mounted in dashboard-shell.tsx and canonical title string 'Volvox.Bot - AI Powered Discord Bot'
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/pages/dashboard/**/*.{ts,tsx} : Use shared title helpers from web/src/lib/page-titles.ts for setting browser titles in dashboard pages
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/commands/**/*.js : Guild owners should be explicitly exempted from role hierarchy restrictions in command handlers to match Discord permission model expectations
Applied to files:
web/tests/hooks/use-guild-role.test.ts
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard page titles should sync with route changes using DashboardTitleSync component mounted in dashboard-shell.tsx and canonical title string 'Volvox.Bot - AI Powered Discord Bot'
Applied to files:
web/src/components/layout/dashboard-title-sync.tsx
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/pages/dashboard/**/*.{ts,tsx} : Use shared title helpers from web/src/lib/page-titles.ts for setting browser titles in dashboard pages
Applied to files:
web/src/components/layout/dashboard-title-sync.tsx
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/app/**/*.{ts,tsx} : Apply static metadata to server-rendered dashboard entry pages and use title template format for root app metadata
Applied to files:
web/src/components/layout/dashboard-title-sync.tsxweb/vitest.config.ts
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to web/src/app/dashboard/page.{ts,tsx} : Dashboard SSR entry points must export `metadata` using `createPageMetadata()` from `web/src/lib/page-titles.ts`
Applied to files:
web/src/components/layout/dashboard-title-sync.tsx
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to web/src/app/dashboard/**/*.{ts,tsx} : New dashboard routes must add a matcher entry to `dashboardTitleMatchers` in `web/src/lib/page-titles.ts` with exact equality for leaf routes and subtree check for subtrees
Applied to files:
web/src/components/layout/dashboard-title-sync.tsx
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/api/ws/logStream.js : Implement backward-compatible WebSocket ticket parsing to accept both legacy 3-part and new 4-part tickets for maintaining existing log stream behavior
Applied to files:
web/tests/api/log-stream-ws-ticket.test.ts
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/app/api/**/route.ts : Include guildId in signed WebSocket ticket payload when issuing tickets from dashboard endpoints
Applied to files:
web/tests/api/log-stream-ws-ticket.test.ts
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/api/ws/*.js : Bind audit-log WebSocket auth tickets to guild context by including nonce.expiry.guildId.hmac in ticket validation
Applied to files:
web/tests/api/log-stream-ws-ticket.test.ts
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to src/api/ws/**/*.js : Enforce tenant scoping on WebSocket handlers: reject mismatched guildId in filters and require entry.guild_id === ws.guildId for broadcast matching
Applied to files:
web/tests/lib/guild-selection.test.ts
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Applied to files:
web/vitest.config.tsweb/tests/lib/bot-api-proxy-branches.test.tsweb/tests/components/landing/stats.test.tsx
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to **/*.{js,ts,tsx,jsx} : Use single quotes — No double quotes except in JSON
Applied to files:
web/vitest.config.ts
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to src/**/*.{js,ts,tsx} : Use Winston logger from `src/logger.js`, NEVER `console.*`
Applied to files:
web/vitest.config.ts
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to **/*.{js,ts,tsx,jsx,json,md} : Use 2-space indent — Biome enforced
Applied to files:
web/vitest.config.ts
🪛 GitHub Check: SonarCloud Code Analysis
web/tests/hooks/use-guild-selection.test.tsx
[warning] 70-70: Prefer globalThis over window.
[warning] 30-30: Prefer globalThis over window.
[warning] 63-63: Prefer globalThis over window.
[warning] 44-44: Prefer globalThis over window.
[warning] 64-64: Prefer globalThis over window.
web/tests/lib/guild-selection.test.ts
[warning] 37-37: Prefer globalThis over window.
web/tests/components/landing/stats.test.tsx
[warning] 81-81: Prefer globalThis over global.
[warning] 37-37: Prefer globalThis over window.
[warning] 52-52: Prefer globalThis over global.
🔇 Additional comments (11)
web/tests/lib/bot-api-proxy-branches.test.ts (3)
1-44: Well-structured test setup with proper mock hoisting.The use of
vi.hoisted()ensures mocks are available before module imports, which is the correct pattern for Vitest. The setup/teardown properly saves and restoresglobalThis.fetchand environment variables.
66-70: Good coverage of permission edge cases.The test correctly validates the three branches: valid admin permission ('8'), non-admin permission ('32'), and invalid input ('garbage') that throws in the BigInt constructor and is caught to return false.
72-133: Comprehensive authorization flow coverage.Tests cover all branches of
authorizeGuildAdmin: missing token (401), expired refresh token (401), guild verification failure (502), missing/non-manageable guild (403), and successful authorization for owners and administrators (null return).web/tests/components/providers.test.tsx (2)
35-56: Good coverage on both toaster theme branches.These cases exercise the explicit
'dark'path and theundefined -> 'system'fallback thatThemedToasteruses, so the provider test now covers the behavior that actually drives theToastertheme prop.
4-6:⚠️ Potential issue | 🟡 MinorApply the repo TS style rules to the new test additions.
The new mocks and assertions continue using double-quoted strings and omit semicolons, which is inconsistent with the enforced JS/TS conventions for this repository.
As per coding guidelines,
**/*.{js,ts,tsx,jsx}: Use single quotes — No double quotes except in JSONandSemicolons are always required`.Also applies to: 17-29, 35-56
⛔ Skipped due to learnings
Learnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower itLearnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to **/*.{js,ts,tsx,jsx} : Use single quotes — No double quotes except in JSONLearnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to **/*.{js,ts,tsx,jsx} : Semicolons are always requiredLearnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to **/*.{js,ts,tsx,jsx,json,md} : Use 2-space indent — Biome enforcedLearnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to src/**/*.{js,ts,tsx} : Use parameterized SQL — Never string interpolation in queriesLearnt from: CR Repo: VolvoxLLC/volvox-bot PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-03-08T04:37:50.339Z Learning: Applies to **/*.{js,ts,tsx,jsx} : Use ESM only — Use `import/export`, no CommonJSweb/tests/components/layout/server-selector.test.tsx (3)
29-43: Nice env isolation for the invite-path tests.Capturing the original
NEXT_PUBLIC_DISCORD_CLIENT_IDand restoring it inafterEachkeeps the empty-state and invite-state cases from bleeding into each other.Also applies to: 53-53
258-326: Good edge-case coverage here.These cases cover both the member-only navigation path and the runtime guild-shape filter, which are exactly the branches that tend to get missed when coverage targets go up.
67-82: No issues found.getBotInviteUrl()readsprocess.env.NEXT_PUBLIC_DISCORD_CLIENT_IDinside the function body at call time, not at module scope. The test correctly mutates the env before rendering, so the invited branch executes as intended.web/tests/components/landing/feature-grid.test.tsx (1)
1-53: LGTM!The test setup correctly uses
vi.hoisted()to establish mocks before the module import, and the framer-motion mock pattern is consistent with other landing component tests in this PR. Both test cases cover meaningful scenarios: feature card rendering and reduced-motion accessibility behavior.web/tests/components/landing/pricing.test.tsx (1)
1-79: LGTM!The test correctly mocks both framer-motion and the Discord utility module. The test cases cover important user flows: verifying the billing toggle switches between monthly/annual pricing with correct values, and verifying graceful degradation when no invite URL exists. The approach of using
closest('button')to find the parent button element is appropriate for checking disabled state.web/tests/components/landing/stats.test.tsx (1)
51-89: Test logic and assertions are well-structured.The mock data correctly matches the
BotStatsinterface, and the assertions properly verify the formatted output fromformatNumber()andformatUptime(). The error fallback test appropriately checks for the 6 placeholder dashes and trust message. The animation frame mocking with proper cleanup inafterEachis thorough.
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/api/routes/warnings.test.js`:
- Around line 62-90: The test currently depends on call order of mockPool.query
via consecutive mockResolvedValueOnce calls; change it to a mockImplementation
on mockPool.query that inspects the SQL text (e.g., using
expect.stringContaining fragments like 'SELECT COUNT(*)::integer AS total' or
the warnings SELECT with 'WHERE guild_id') and returns the appropriate mocked
result for the COUNT vs data query, or alternatively collect
mockPool.query.mock.calls after the request and assert their SQL arguments and
parameter arrays rather than relying on nth-call ordering; update the assertions
in the test (the block using authed(request(app).get(...))) to use this
SQL-branching mock or post-response call inspection so the test is resilient to
reordered queries in src/api/routes/warnings.js.
In `@tests/commands/config.test.js`:
- Around line 216-217: The test currently asserts on embed.fields by position
(e.g., embed.fields[0].value) which couples the test to ordering; update the
assertions in tests/commands/config.test.js to locate the desired field by its
name first (use embed.fields.find(f => f.name === '<FieldName>') or equivalent)
and then assert on that field's value (replace the positional references like
embed.fields[0] and embed.fields[1] with name-based lookups for the "Settings"
and other named fields), and apply the same change to the other assertions noted
around lines 380-403 to avoid brittle layout-dependent tests; keep using
mockReply and the existing embed.toJSON() extraction but resolve fields by name
before checking .value.
- Around line 143-161: The test name claims it covers empty arrays and objects
but only asserts channels are returned; update the test to also assert the
empty-object key is present or rename it to reflect only arrays. Specifically,
in the test "should include empty arrays and objects in autocomplete results"
(which mocks getConfig and calls autocomplete with the interaction and
mockRespond), either rename the test to "should include empty arrays in
autocomplete results" or add an assertion on mockRespond to expect an entry for
the empty object key (e.g., that the response includes { name: 'features',
value: 'features' }) alongside the existing channels assertion so the
empty-object case (features: {}) is actually verified.
In `@tests/commands/voice.test.js`:
- Around line 276-284: The test currently asserts that an unknown subcommand
leaves a deferred interaction unresolved; instead update the test to assert the
command handler returns an explicit error after deferring. Keep
getConfig.mockReturnValueOnce and createInteraction({ subcommand: 'mystery' })
as-is, call execute(interaction), assert interaction.deferReply was called, and
then assert interaction.editReply was called with the expected error response
(e.g., a rejection or "Unknown subcommand" message) rather than asserting
editReply was never called; this targets the behavior in the execute function in
src/commands/voice.js and the interaction.deferReply / interaction.editReply
calls.
- Around line 236-262: The test currently only asserts the attachment filename;
update the 'exports voice session data as CSV' test to also verify the CSV
payload content returned in the attachment from execute(interaction): after the
call, extract the first file object passed to interaction.editReply (files[0])
and assert its content buffer/string includes the CSV header row and the
expected data row (matching exportVoiceSessions mock values such as
id,user_id,channel_id,joined_at,left_at,duration_seconds and the specific
dates), so you validate both header serialization and the exported row in
addition to the existing filename assertion.
In `@tests/modules/welcomeOnboarding.test.js`:
- Line 427: Replace the ad-hoc safeEditReply.mockClear() call with a
beforeEach-based mock reset to guarantee isolation: add a beforeEach hook at the
top of the welcomeOnboarding describe block that calls vi.clearAllMocks() (or
vi.resetAllMocks() if you prefer restoring implementations) so mocks like
safeEditReply and safeSend are cleared before every test, then remove the manual
safeEditReply.mockClear() invocation.
- Around line 90-98: Split the combined spec into two focused tests: one that
asserts buildRulesAgreementMessage returns the expected content and components
(check message.content includes 'Read the server rules', message.components
length and the button label via message.components[0].components[0].data.label),
and a separate test for isReturningMember that verifies both true and false
outcomes by calling isReturningMember with a mocked flags.has (use
vi.fn().mockReturnValue(true) and vi.fn().mockReturnValue(false)). Ensure each
test has a clear description (e.g., "builds the rules agreement message" and
"detects returning members") so failures point directly to the responsible
function.
In `@vitest.config.js`:
- Around line 13-22: The coverage exclusions added to vitest.config.js remove
critical, branch-heavy code and undercut the 85% gate; remove
'src/modules/handlers/ticketHandler.js' and 'src/utils/discordCache.js' from the
exclude array so they count toward coverage, and instead add focused unit tests
that exercise their branching and error paths (create specs that import
ticketHandler and discordCache and cover success, failure, and edge/conditional
branches). Ensure vitest.config.js keeps only truly untestable/bootstrap files
in exclude and add targeted test files that mock dependencies and assert the
handler/cache behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0aa2d886-0290-47f9-91a4-2b1f4740c470
📒 Files selected for processing (7)
tests/api/routes/tempRoles.test.jstests/api/routes/warnings.test.jstests/api/ws/auditStream.test.jstests/commands/config.test.jstests/commands/voice.test.jstests/modules/welcomeOnboarding.test.jsvitest.config.js
📜 Review details
⏰ 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: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes — No double quotes except in JSON
Semicolons are always required
Files:
tests/commands/config.test.jstests/modules/welcomeOnboarding.test.jstests/commands/voice.test.jsvitest.config.jstests/api/routes/warnings.test.jstests/api/ws/auditStream.test.jstests/api/routes/tempRoles.test.js
**/*.{js,ts,tsx,jsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indent — Biome enforced
Files:
tests/commands/config.test.jstests/modules/welcomeOnboarding.test.jstests/commands/voice.test.jsvitest.config.jstests/api/routes/warnings.test.jstests/api/ws/auditStream.test.jstests/api/routes/tempRoles.test.js
tests/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests required with 80% coverage threshold, never lower it
Files:
tests/commands/config.test.jstests/modules/welcomeOnboarding.test.jstests/commands/voice.test.jstests/api/routes/warnings.test.jstests/api/ws/auditStream.test.jstests/api/routes/tempRoles.test.js
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard page titles should sync with route changes using DashboardTitleSync component mounted in dashboard-shell.tsx and canonical title string 'Volvox.Bot - AI Powered Discord Bot'
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to src/**/*.{js,ts,tsx} : Use safe Discord message methods — `safeReply()`/`safeSend()`/`safeEditReply()`
Applied to files:
tests/modules/welcomeOnboarding.test.js
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Applied to files:
vitest.config.js
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to src/api/ws/**/*.js : Enforce tenant scoping on WebSocket handlers: reject mismatched guildId in filters and require entry.guild_id === ws.guildId for broadcast matching
Applied to files:
tests/api/routes/warnings.test.jstests/api/ws/auditStream.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/api/ws/*.js : Bind audit-log WebSocket auth tickets to guild context by including nonce.expiry.guildId.hmac in ticket validation
Applied to files:
tests/api/ws/auditStream.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/api/ws/logStream.js : Implement backward-compatible WebSocket ticket parsing to accept both legacy 3-part and new 4-part tickets for maintaining existing log stream behavior
Applied to files:
tests/api/ws/auditStream.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to web/src/app/api/**/route.ts : Include guildId in signed WebSocket ticket payload when issuing tickets from dashboard endpoints
Applied to files:
tests/api/ws/auditStream.test.js
📚 Learning: 2026-03-10T23:21:49.730Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to src/api/routes/*.js : Add adaptDeleteGuildIdParam in API route handlers to enforce guild moderation checks on DELETE requests without losing record id
Applied to files:
tests/api/routes/tempRoles.test.js
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to src/api/routes/**/*.{js,ts,tsx} : API routes must mount in `src/api/server.js` and include auth middleware if needed
Applied to files:
tests/api/routes/tempRoles.test.js
📚 Learning: 2026-03-10T23:29:51.063Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:29:51.063Z
Learning: Applies to src/api/routes/*.js : Apply requireGuildModerator authorization check to DELETE endpoints that modify guild-scoped resources
Applied to files:
tests/api/routes/tempRoles.test.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/commands/**/*.js : Guild owners should be explicitly exempted from role hierarchy restrictions in command handlers to match Discord permission model expectations
Applied to files:
tests/api/routes/tempRoles.test.js
🪛 GitHub Check: SonarCloud Code Analysis
tests/api/routes/warnings.test.js
[warning] 50-50: Move function 'authed' to the outer scope.
🔇 Additional comments (13)
tests/api/ws/auditStream.test.js (6)
173-179: Good malformed-ticket coverage.This hits the bad-HMAC path with a structurally valid ticket, which is a solid regression check for auth failure handling.
219-235: Nice normalization assertion.Asserting the full
filter_okpayload makes thenullcoercion behavior explicit and also protects the authenticatedguildIddefault.
304-338: Strong tenant-scope regression test.Checking the mismatch error and then verifying delivery stays scoped to
guild1covers the important boundary here. Based on learnings Enforce tenant scoping on WebSocket handlers: reject mismatched guildId in filters and require entry.guild_id === ws.guildId for broadcast matching.
371-399: GooduserIdfilter coverage.Sending the non-matching entry first makes this fail if the filter leaks, so it gives real signal instead of only asserting the happy path.
436-444: Nice protocol guardrail.This keeps the malformed-message contract explicit for clients that omit
type.
468-489: Good hard-cap coverage.Filling all 10 authenticated slots before asserting
4029on the 11th lines up cleanly with the server’s max-client enforcement.tests/modules/welcomeOnboarding.test.js (7)
1-42: LGTM!Mock setup is comprehensive and correctly handles the Discord cache fallback pattern. The newly imported
buildRulesAgreementMessageandisReturningMemberfunctions are properly covered by the tests below.
56-88: LGTM!Comprehensive normalization test covering whitespace trimming, empty label filtering, null option handling, and DM step filtering. This aligns well with the production implementation.
100-103: LGTM!Concise coverage for the guard conditions in
buildRoleMenuMessage.
175-235: LGTM!Thorough validation of the error paths: unconfigured role, missing role from cache/fetch, and non-editable role. These tests strengthen confidence in the rejection logic.
237-293: LGTM!Good coverage of the already-verified early return and role assignment failure handling. The assertions correctly verify the expected error messages.
295-345: LGTM!This test validates important behavior: when
interaction.memberis undefined, the code fetches viaguild.members.fetch, and when the DM sequence fails partway through, it breaks the loop but still reports success (since the role was assigned). This aligns with the production implementation inhandleRulesAcceptButton.
448-458: LGTM!Good edge-case coverage for the no-op scenario where selected roles already match the member's current roles.
BillChirico
force-pushed
the
bill/raise-web-coverage-to-85
branch
from
7166c2e to
5c223d5
Compare
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| include: ['src/**/*.js'], | ||
| exclude: ['src/deploy-commands.js'], | ||
| exclude: [ | ||
| 'src/deploy-commands.js', |
There was a problem hiding this comment.
Excluding src/modules/events/voiceState.js from coverage reduces the meaning of the raised branches threshold (and can mask regressions in an event handler that likely runs in production). If this exclusion is temporary to unblock coverage, consider adding targeted tests for voiceState instead, or at least documenting why it’s excluded so the coverage contract is clear.
| 'src/deploy-commands.js', | |
| 'src/deploy-commands.js', | |
| // NOTE: The voiceState event handler is tightly coupled to live voice | |
| // state updates and external integrations, which makes it difficult | |
| // to exercise reliably in unit tests without extensive mocking or | |
| // end-to-end infrastructure. It is intentionally excluded from | |
| // coverage until dedicated tests or refactoring are implemented. | |
| // When adding tests for voiceState, remove this exclusion so that | |
| // coverage thresholds accurately reflect this event handler. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/modules/welcomeOnboarding.test.js (1)
423-478: 🧹 Nitpick | 🔵 TrivialConsider splitting into separate tests for clearer failure diagnosis.
This test combines two distinct scenarios: unconfigured role menu and no-op updates. Splitting them improves test isolation and makes failures easier to diagnose.
♻️ Suggested split
- it('handles unconfigured role menus and no-op updates', async () => { - const interaction = { - user: { id: 'user-3' }, - values: [], - member: { - roles: { - cache: new Map(), - add: vi.fn(async () => {}), - remove: vi.fn(async () => {}), - }, - }, - guild: { - roles: { - cache: new Map(), - fetch: vi.fn(async () => null), - }, - }, - deferReply: vi.fn(async () => {}), - }; - - await handleRoleMenuSelection(interaction, { welcome: { roleMenu: { enabled: false } } }); - expect(safeEditReply).toHaveBeenCalledWith( - interaction, - expect.objectContaining({ content: expect.stringContaining('not configured') }), - ); - - const role = { id: 'role-a', editable: true }; - const noChangeInteraction = { - ...interaction, - values: ['role-a'], - member: { - roles: { - cache: new Map([['role-a', role]]), - add: vi.fn(async () => {}), - remove: vi.fn(async () => {}), - }, - }, - guild: { - roles: { - cache: new Map([['role-a', role]]), - fetch: vi.fn(async () => role), - }, - }, - }; - - await handleRoleMenuSelection(noChangeInteraction, { - welcome: { - roleMenu: { enabled: true, options: [{ label: 'Role A', roleId: 'role-a' }] }, - }, - }); - - expect(safeEditReply).toHaveBeenCalledWith( - noChangeInteraction, - expect.objectContaining({ content: '✅ No role changes were needed.' }), - ); - }); + it('responds with not-configured when role menu is disabled', async () => { + const interaction = { + user: { id: 'user-3' }, + values: [], + member: { + roles: { + cache: new Map(), + add: vi.fn(async () => {}), + remove: vi.fn(async () => {}), + }, + }, + guild: { + roles: { + cache: new Map(), + fetch: vi.fn(async () => null), + }, + }, + deferReply: vi.fn(async () => {}), + }; + + await handleRoleMenuSelection(interaction, { welcome: { roleMenu: { enabled: false } } }); + + expect(safeEditReply).toHaveBeenCalledWith( + interaction, + expect.objectContaining({ content: expect.stringContaining('not configured') }), + ); + }); + + it('reports no changes when selected roles match current assignments', async () => { + const role = { id: 'role-a', editable: true }; + const interaction = { + user: { id: 'user-3' }, + values: ['role-a'], + member: { + roles: { + cache: new Map([['role-a', role]]), + add: vi.fn(async () => {}), + remove: vi.fn(async () => {}), + }, + }, + guild: { + roles: { + cache: new Map([['role-a', role]]), + fetch: vi.fn(async () => role), + }, + }, + deferReply: vi.fn(async () => {}), + }; + + await handleRoleMenuSelection(interaction, { + welcome: { + roleMenu: { enabled: true, options: [{ label: 'Role A', roleId: 'role-a' }] }, + }, + }); + + expect(safeEditReply).toHaveBeenCalledWith( + interaction, + expect.objectContaining({ content: '✅ No role changes were needed.' }), + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/modules/welcomeOnboarding.test.js` around lines 423 - 478, Split the combined test into two distinct tests: one that only covers the "unconfigured role menu" scenario and one that only covers the "no-op updates" scenario; to do this, create two it(...) cases that each construct only the relevant mock interaction and guild/member role caches, call handleRoleMenuSelection with the appropriate config, and assert the corresponding safeEditReply expectation (the first asserting a message containing "not configured" and the second asserting the exact '✅ No role changes were needed.' response). Keep the existing mock helpers (interaction, noChangeInteraction) but move repeated setup into a small helper or beforeEach if needed and ensure you reference handleRoleMenuSelection and safeEditReply in each new test so failure output isolates which scenario broke.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/modules/handlers/reminderHandler.test.js`:
- Around line 142-145: Add a unit test that simulates both handleReminderSnooze
failing and safeReply throwing to exercise the nested try-catch: mock
mockHandleReminderSnooze to reject, mock mockSafeReply to reject, register the
handler via registerReminderButtonHandler/getRegisteredHandler, invoke the
handler with a reminder_snooze_... interaction object and assert the promise
resolves (does not throw) and that mockSafeReply was called; this ensures the
code path where safeReply itself fails is covered.
In `@tests/modules/reactions.branches.test.js`:
- Around line 78-91: The test setup in the beforeEach uses a mix of
mockReturnValue(Promise.resolve()) and mockResolvedValue(undefined) for async
mocks—make them consistent by replacing mockReturnValue(Promise.resolve()) calls
with mockResolvedValue(undefined) for the async mocks; update mockTrackReaction,
mockRecordFeedback, and mockDeleteFeedback (and any other mocks in the
beforeEach using Promise.resolve()) to use mockResolvedValue(undefined) so all
async mock stubs (e.g., mockHandleReactionRoleAdd, mockHandleReactionRoleRemove,
mockHandleReactionAdd, mockHandleReactionRemove, mockTrackReaction,
mockRecordFeedback, mockDeleteFeedback) use the same pattern.
---
Duplicate comments:
In `@tests/modules/welcomeOnboarding.test.js`:
- Around line 423-478: Split the combined test into two distinct tests: one that
only covers the "unconfigured role menu" scenario and one that only covers the
"no-op updates" scenario; to do this, create two it(...) cases that each
construct only the relevant mock interaction and guild/member role caches, call
handleRoleMenuSelection with the appropriate config, and assert the
corresponding safeEditReply expectation (the first asserting a message
containing "not configured" and the second asserting the exact '✅ No role
changes were needed.' response). Keep the existing mock helpers (interaction,
noChangeInteraction) but move repeated setup into a small helper or beforeEach
if needed and ensure you reference handleRoleMenuSelection and safeEditReply in
each new test so failure output isolates which scenario broke.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4d1f756-a823-40fb-a2f5-497e25428baf
📒 Files selected for processing (5)
tests/modules/handlers/reminderHandler.test.jstests/modules/handlers/welcomeOnboardingHandler.test.jstests/modules/reactions.branches.test.jstests/modules/welcomeOnboarding.test.jsvitest.config.js
📜 Review details
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes — No double quotes except in JSON
Semicolons are always required
Files:
tests/modules/handlers/welcomeOnboardingHandler.test.jstests/modules/welcomeOnboarding.test.jsvitest.config.jstests/modules/handlers/reminderHandler.test.jstests/modules/reactions.branches.test.js
**/*.{js,ts,tsx,jsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indent — Biome enforced
Files:
tests/modules/handlers/welcomeOnboardingHandler.test.jstests/modules/welcomeOnboarding.test.jsvitest.config.jstests/modules/handlers/reminderHandler.test.jstests/modules/reactions.branches.test.js
tests/**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests required with 80% coverage threshold, never lower it
Files:
tests/modules/handlers/welcomeOnboardingHandler.test.jstests/modules/welcomeOnboarding.test.jstests/modules/handlers/reminderHandler.test.jstests/modules/reactions.branches.test.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard page titles should sync with route changes using DashboardTitleSync component mounted in dashboard-shell.tsx and canonical title string 'Volvox.Bot - AI Powered Discord Bot'
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Applied to files:
tests/modules/welcomeOnboarding.test.jsvitest.config.js
📚 Learning: 2026-03-10T20:36:29.483Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T20:36:29.483Z
Learning: Applies to src/**/{startup,command-register,reload}*.{js,ts} : Remove process.env.GUILD_ID runtime reads from bot startup and reload command registration
Applied to files:
vitest.config.js
📚 Learning: 2026-03-08T04:37:50.339Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to src/**/*.{js,ts,tsx} : Use safe Discord message methods — `safeReply()`/`safeSend()`/`safeEditReply()`
Applied to files:
tests/modules/handlers/reminderHandler.test.js
🔇 Additional comments (16)
vitest.config.js (1)
13-21: LGTM! Exclusions are now appropriately scoped and thresholds raised to 85%.The configuration changes are well-justified:
Exclusions are appropriate:
deploy-commands.jsis CLI bootstrap code, andvoiceState.jsis a thin 21-line event registration wrapper—its actual business logic (handleVoiceStateUpdateinvoice.js) is comprehensively tested invoice.test.js.Thresholds meet requirements: All coverage thresholds at 85% exceed the 80% minimum requirement. Based on learnings, tests are required with 80% coverage threshold and should never be lowered.
Previous issues addressed: The non-existent
reactionCreate.jstypo and inappropriate production handler exclusions from past reviews have been removed.tests/modules/reactions.branches.test.js (5)
1-44: LGTM! Proper mock setup with ESM imports.The mock declarations precede the dynamic import correctly, ensuring Vitest's hoisting behavior works as expected. All mocks are cleanly organized by module.
46-75: Well-structured test helpers.
createReactionprovides consistent test fixtures, andgetHandlerscleanly validates handler registration while returning the handlers for direct invocation. The assertions ingetHandlersensure the handlers are registered before tests proceed.
93-109: Engagement tracking assertion now present — addresses prior feedback.Line 100 adds
expect(mockTrackReaction).toHaveBeenCalled(), which resolves the previous review comment requesting explicit engagement tracking verification. The test now properly validates that reaction tracking is invoked alongside AI feedback recording.
111-154: Good coverage of conditional paths.Tests correctly verify:
- Non-feedback emojis skip
recordFeedback(lines 111-118)- Feedback deletion on supported reaction removal (lines 134-145)
- Non-feedback removal skips
deleteFeedback(lines 147-154)These match the conditional logic in the source handlers.
120-132: Error isolation tests validate try/catch scoping.Both tests confirm that reaction-role failures are caught and logged without preventing the starboard handler from executing. This matches the properly-scoped try/catch structure in
src/modules/events/reactions.js(lines 65-72 and 116-123 from context snippets).Also applies to: 156-168
tests/modules/handlers/reminderHandler.test.js (3)
1-38: LGTM - Well-structured test setup with proper mocking patterns.The mock declarations before imports and helper functions (
createClient,getRegisteredHandler) provide clean test scaffolding. The use of wrapper functions invi.mock(e.g.,(...args) => mockGetConfig(...args)) correctly allows mock behavior to be changed per test.
40-116: LGTM - Routing and filtering tests are comprehensive.The test suite covers the key routing paths:
- Non-button filtering
- Unrelated custom ID filtering
- Feature-disabled gating
- Correct dispatch to snooze vs. dismiss handlers
118-182: LGTM - Error handling edge cases well covered.Tests correctly exercise:
- Error logging with structured context
- Ephemeral fallback reply when
!replied && !deferred- Skipping fallback when
deferred: true- Skipping fallback when
replied: truetests/modules/handlers/welcomeOnboardingHandler.test.js (2)
14-35: LGTM! Partial mock pattern correctly preserves production constants.The
importOriginalapproach properly re-exports the realRULES_ACCEPT_BUTTON_IDandROLE_MENU_SELECT_IDconstants while only mocking the handler functions. This ensures the test fixtures exercise the actual routing logic in the production handler.
48-161: Test coverage is comprehensive and correctly validates handler behavior.The test suite covers all critical paths:
- Early return when
guildIdis missing- Early return when welcome is disabled
- Successful routing to
handleRulesAcceptButtonandhandleRoleMenuSelection- Error handling with logging and safe fallback replies
The error message assertions (lines 115-116, 158-159) correctly match the production handler strings from
src/modules/handlers/welcomeOnboardingHandler.js.tests/modules/welcomeOnboarding.test.js (5)
1-2: LGTM! Mock cleanup properly centralized inbeforeEach.The addition of
GuildMemberFlagsBitFieldimport enables proper flag assertions, and thebeforeEachhook withvi.clearAllMocks()ensures test isolation—addressing the previous review feedback about manualmockClear()being fragile.Also applies to: 47-49
63-95: Good coverage of normalization edge cases.This test validates trimming, filtering of junk options (empty labels, nulls), and DM step sanitization—ensuring the normalized config is safe for downstream processing.
97-137: Builder and flag detection tests properly validate production contracts.
- Lines 99, 104: Using
toJSON()and assertingcustom_idensures the button is wired to the correct handler routing- Lines 110, 114: Verifying
flags.has()is called withGuildMemberFlagsBitField.Flags.DidRejoinconfirms the actual flag check in production (seesrc/modules/welcomeOnboarding.js:76-78)- Lines 133, 135: The role menu
custom_idassertion locks the select menu to the handler's expected ID
197-315: Comprehensive error path coverage for rules acceptance.The tests validate all rejection scenarios in
handleRulesAcceptButton:
- Missing
verifiedRoleconfig → "not configured"- Role not found → "cannot find"
- Role above bot's highest role → "above my highest role"
- Member already has role → "already verified"
- Role assignment throws → "Failed to assign"
Each matches the production messages in
src/modules/welcomeOnboarding.js:125-211.
317-367: Test correctly validates member fetching and DM sequence early termination.When
interaction.memberis missing, the handler fetches viainteraction.guild.members.fetch(interaction.user.id)(per production code at line 136). The DM sequence mock setup (resolve once → reject once) with 3 steps correctly expectssendto be called exactly twice, validating thebreakbehavior on DM failure.
| expect(mockSafeReply).toHaveBeenCalledWith(interaction, { | ||
| content: '❌ Something went wrong processing your request.', | ||
| ephemeral: true, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for when safeReply itself throws.
The source code has a nested try-catch that silently swallows failures from safeReply (line 48-50 in the source). Adding a test ensures this defensive path is exercised.
🧪 Suggested additional test
it('silently handles safeReply failures when sending fallback', async () => {
mockHandleReminderSnooze.mockRejectedValueOnce(new Error('bad snooze'));
mockSafeReply.mockRejectedValueOnce(new Error('reply failed'));
const client = createClient();
registerReminderButtonHandler(client);
const handler = getRegisteredHandler(client);
// Should not throw despite safeReply failure
await expect(handler({
isButton: () => true,
customId: 'reminder_snooze_42_15m',
guildId: 'guild-1',
user: { id: 'user-1' },
replied: false,
deferred: false,
})).resolves.toBeUndefined();
expect(mockSafeReply).toHaveBeenCalled();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/handlers/reminderHandler.test.js` around lines 142 - 145, Add a
unit test that simulates both handleReminderSnooze failing and safeReply
throwing to exercise the nested try-catch: mock mockHandleReminderSnooze to
reject, mock mockSafeReply to reject, register the handler via
registerReminderButtonHandler/getRegisteredHandler, invoke the handler with a
reminder_snooze_... interaction object and assert the promise resolves (does not
throw) and that mockSafeReply was called; this ensures the code path where
safeReply itself fails is covered.
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockGetConfig.mockReturnValue({ | ||
| ai: { feedback: { enabled: true } }, | ||
| starboard: { enabled: true }, | ||
| }); | ||
| mockTrackReaction.mockReturnValue(Promise.resolve()); | ||
| mockHandleReactionRoleAdd.mockResolvedValue(undefined); | ||
| mockHandleReactionRoleRemove.mockResolvedValue(undefined); | ||
| mockHandleReactionAdd.mockResolvedValue(undefined); | ||
| mockHandleReactionRemove.mockResolvedValue(undefined); | ||
| mockRecordFeedback.mockReturnValue(Promise.resolve()); | ||
| mockDeleteFeedback.mockReturnValue(Promise.resolve()); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer consistent mockResolvedValue for async mocks.
Lines 85-88 use mockResolvedValue(undefined) while lines 84, 89, and 90 use mockReturnValue(Promise.resolve()). Both work, but consistency improves readability.
♻️ Suggested consistency fix
beforeEach(() => {
vi.clearAllMocks();
mockGetConfig.mockReturnValue({
ai: { feedback: { enabled: true } },
starboard: { enabled: true },
});
- mockTrackReaction.mockReturnValue(Promise.resolve());
+ mockTrackReaction.mockResolvedValue(undefined);
mockHandleReactionRoleAdd.mockResolvedValue(undefined);
mockHandleReactionRoleRemove.mockResolvedValue(undefined);
mockHandleReactionAdd.mockResolvedValue(undefined);
mockHandleReactionRemove.mockResolvedValue(undefined);
- mockRecordFeedback.mockReturnValue(Promise.resolve());
- mockDeleteFeedback.mockReturnValue(Promise.resolve());
+ mockRecordFeedback.mockResolvedValue(undefined);
+ mockDeleteFeedback.mockResolvedValue(undefined);
});📝 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.
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| mockGetConfig.mockReturnValue({ | |
| ai: { feedback: { enabled: true } }, | |
| starboard: { enabled: true }, | |
| }); | |
| mockTrackReaction.mockReturnValue(Promise.resolve()); | |
| mockHandleReactionRoleAdd.mockResolvedValue(undefined); | |
| mockHandleReactionRoleRemove.mockResolvedValue(undefined); | |
| mockHandleReactionAdd.mockResolvedValue(undefined); | |
| mockHandleReactionRemove.mockResolvedValue(undefined); | |
| mockRecordFeedback.mockReturnValue(Promise.resolve()); | |
| mockDeleteFeedback.mockReturnValue(Promise.resolve()); | |
| }); | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| mockGetConfig.mockReturnValue({ | |
| ai: { feedback: { enabled: true } }, | |
| starboard: { enabled: true }, | |
| }); | |
| mockTrackReaction.mockResolvedValue(undefined); | |
| mockHandleReactionRoleAdd.mockResolvedValue(undefined); | |
| mockHandleReactionRoleRemove.mockResolvedValue(undefined); | |
| mockHandleReactionAdd.mockResolvedValue(undefined); | |
| mockHandleReactionRemove.mockResolvedValue(undefined); | |
| mockRecordFeedback.mockResolvedValue(undefined); | |
| mockDeleteFeedback.mockResolvedValue(undefined); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/reactions.branches.test.js` around lines 78 - 91, The test
setup in the beforeEach uses a mix of mockReturnValue(Promise.resolve()) and
mockResolvedValue(undefined) for async mocks—make them consistent by replacing
mockReturnValue(Promise.resolve()) calls with mockResolvedValue(undefined) for
the async mocks; update mockTrackReaction, mockRecordFeedback, and
mockDeleteFeedback (and any other mocks in the beforeEach using
Promise.resolve()) to use mockResolvedValue(undefined) so all async mock stubs
(e.g., mockHandleReactionRoleAdd, mockHandleReactionRoleRemove,
mockHandleReactionAdd, mockHandleReactionRemove, mockTrackReaction,
mockRecordFeedback, mockDeleteFeedback) use the same pattern.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bill Chirico <bill@chirico.dev>
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bill Chirico <bill@chirico.dev>
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bill Chirico <bill@chirico.dev>
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bill Chirico <bill@chirico.dev>
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
Comment out the pull_request trigger in the workflow. Signed-off-by: Bill Chirico <bill@chirico.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/tests/components/landing/pricing.test.tsx`:
- Around line 72-77: The test currently finds buttons by text nodes then climbs
to the enclosing button using getByText(...).closest('button'); update the
assertions to query the actual accessible controls via role: use
screen.getByRole('button', { name: 'npm install' }) and
screen.getByRole('button', { name: 'curl | bash' }) (or iterate over an array of
those labels and call screen.getByRole for each) and then assert that the
returned element is defined and toBeDisabled(); replace the installButtons array
and loop to use screen.getByRole('button', { name }) calls instead of .closest.
- Around line 10-24: The framer-motion mock only defines motion.div and motion.p
so new motion tags break tests; update the mock (the vi.mock callback that
returns motion: { ... } and the createComponent helper) to dynamically create
any motion.* element accessed (e.g., by using a Proxy or by generating
components on demand) so calls like motion.section or motion.h2/h3 map to
createComponent(tag) instead of hardcoding div/p; ensure useInView continues to
forward to mockUseInView.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10800f38-fa64-4a30-bede-9f8780d3c72f
📒 Files selected for processing (1)
web/tests/components/landing/pricing.test.tsx
📜 Review details
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes — No double quotes except in JSON
Semicolons are always required
Files:
web/tests/components/landing/pricing.test.tsx
**/*.{js,ts,tsx,jsx,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use 2-space indent — Biome enforced
Files:
web/tests/components/landing/pricing.test.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:37:50.339Z
Learning: Applies to tests/**/*.{js,ts,tsx} : Tests required with 80% coverage threshold, never lower it
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-10T23:21:49.730Z
Learning: Applies to web/src/components/layout/dashboard-shell.tsx : Dashboard page titles should sync with route changes using DashboardTitleSync component mounted in dashboard-shell.tsx and canonical title string 'Volvox.Bot - AI Powered Discord Bot'
🔇 Additional comments (1)
web/tests/components/landing/pricing.test.tsx (1)
38-60: Nice coverage of the billing-toggle path.This locks down the switch state, price-unit swap, and annual savings rows without overspecifying markup.
| vi.mock('framer-motion', async () => { | ||
| const React = await import('react'); | ||
| const createComponent = (tag: string) => | ||
| React.forwardRef(({ animate: _animate, initial: _initial, transition: _transition, whileHover: _whileHover, ...props }: any, ref: any) => | ||
| React.createElement(tag, { ...props, ref }, props.children) | ||
| ); | ||
|
|
||
| return { | ||
| motion: { | ||
| div: createComponent('div'), | ||
| p: createComponent('p'), | ||
| }, | ||
| useInView: (...args: unknown[]) => mockUseInView(...args), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the framer-motion mock tolerant of new motion.* tags.
This stub only defines motion.div and motion.p. A harmless wrapper change in Pricing.tsx to motion.section, motion.h2, etc. will break the test harness before the pricing behavior is exercised.
♻️ Suggested change
vi.mock('framer-motion', async () => {
const React = await import('react');
const createComponent = (tag: string) =>
React.forwardRef(({ animate: _animate, initial: _initial, transition: _transition, whileHover: _whileHover, ...props }: any, ref: any) =>
React.createElement(tag, { ...props, ref }, props.children)
);
+ const motion = new Proxy(
+ {},
+ {
+ get: (_target, tag) => createComponent(String(tag)),
+ },
+ );
return {
- motion: {
- div: createComponent('div'),
- p: createComponent('p'),
- },
+ motion,
useInView: (...args: unknown[]) => mockUseInView(...args),
};
});📝 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.
| vi.mock('framer-motion', async () => { | |
| const React = await import('react'); | |
| const createComponent = (tag: string) => | |
| React.forwardRef(({ animate: _animate, initial: _initial, transition: _transition, whileHover: _whileHover, ...props }: any, ref: any) => | |
| React.createElement(tag, { ...props, ref }, props.children) | |
| ); | |
| return { | |
| motion: { | |
| div: createComponent('div'), | |
| p: createComponent('p'), | |
| }, | |
| useInView: (...args: unknown[]) => mockUseInView(...args), | |
| }; | |
| }); | |
| vi.mock('framer-motion', async () => { | |
| const React = await import('react'); | |
| const createComponent = (tag: string) => | |
| React.forwardRef(({ animate: _animate, initial: _initial, transition: _transition, whileHover: _whileHover, ...props }: any, ref: any) => | |
| React.createElement(tag, { ...props, ref }, props.children) | |
| ); | |
| const motion = new Proxy( | |
| {}, | |
| { | |
| get: (_target, tag) => createComponent(String(tag)), | |
| }, | |
| ); | |
| return { | |
| motion, | |
| useInView: (...args: unknown[]) => mockUseInView(...args), | |
| }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/tests/components/landing/pricing.test.tsx` around lines 10 - 24, The
framer-motion mock only defines motion.div and motion.p so new motion tags break
tests; update the mock (the vi.mock callback that returns motion: { ... } and
the createComponent helper) to dynamically create any motion.* element accessed
(e.g., by using a Proxy or by generating components on demand) so calls like
motion.section or motion.h2/h3 map to createComponent(tag) instead of hardcoding
div/p; ensure useInView continues to forward to mockUseInView.
| const installButtons = [screen.getByText('npm install'), screen.getByText('curl | bash')]; | ||
| for (const buttonLabel of installButtons) { | ||
| const button = buttonLabel.closest('button'); | ||
| expect(button).not.toBeNull(); | ||
| expect(button).toBeDisabled(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Query the disabled CTAs by role instead of climbing from text nodes.
getByText(...).closest('button') couples the assertion to the current wrapper structure. getByRole('button', { name }) keeps the test aligned with the actual accessible control.
♻️ Suggested change
- const installButtons = [screen.getByText('npm install'), screen.getByText('curl | bash')];
- for (const buttonLabel of installButtons) {
- const button = buttonLabel.closest('button');
- expect(button).not.toBeNull();
- expect(button).toBeDisabled();
- }
+ for (const name of ['npm install', 'curl | bash']) {
+ expect(screen.getByRole('button', { name })).toBeDisabled();
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/tests/components/landing/pricing.test.tsx` around lines 72 - 77, The test
currently finds buttons by text nodes then climbs to the enclosing button using
getByText(...).closest('button'); update the assertions to query the actual
accessible controls via role: use screen.getByRole('button', { name: 'npm
install' }) and screen.getByRole('button', { name: 'curl | bash' }) (or iterate
over an array of those labels and call screen.getByRole for each) and then
assert that the returned element is defined and toBeDisabled(); replace the
installButtons array and loop to use screen.getByRole('button', { name }) calls
instead of .closest.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| 'src/hooks/use-moderation-cases.ts', | ||
| 'src/hooks/use-moderation-stats.ts', | ||
| 'src/hooks/use-user-history.ts', |
There was a problem hiding this comment.
The coverage exclude comment claims the dashboard UI is covered via end-to-end tests, but there don’t appear to be any Playwright/Cypress/etc. specs in the repo. This is misleading documentation for why the dashboard subtree is excluded from the unit-test coverage contract—either add a reference to where those E2E tests live/run or reword the comment to avoid implying coverage that isn’t present here.
| statements: 85, | ||
| branches: 82, | ||
| branches: 85, | ||
| functions: 85, | ||
| lines: 85, | ||
| }, |
There was a problem hiding this comment.
This PR is titled/described as increasing web coverage to 85%, but this change also raises the root (bot) suite branch threshold to 85%. If the intent is to enforce 85% repo-wide, the PR title/description should be updated to reflect that; otherwise, consider keeping the root thresholds unchanged and limiting the change to web/vitest.config.ts.
| name: Claude Code Review | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] | ||
| #on: | ||
| # pull_request: | ||
| # types: [opened, synchronize] |
There was a problem hiding this comment.
Workflow trigger is fully disabled
The entire on: block is commented out, which means this workflow will never fire automatically on any future pull request. The job definition, secrets, and step configuration remain, but without a trigger the workflow is permanently inert. If the intent is to replace this with Greptile reviews, the file should either be deleted or have the trigger replaced rather than simply commented out, so the intent is explicit and reviewable in git history.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/claude-review.yml
Line: 1-5
Comment:
**Workflow trigger is fully disabled**
The entire `on:` block is commented out, which means this workflow will never fire automatically on any future pull request. The job definition, secrets, and step configuration remain, but without a trigger the workflow is permanently inert. If the intent is to replace this with Greptile reviews, the file should either be deleted or have the trigger replaced rather than simply commented out, so the intent is explicit and reviewable in git history.
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Bill Chirico <bill@chirico.dev>
railway-app
bot
temporarily deployed
to
volvox-bot / volvox-bot-pr-273
🧹 Preview Environment Cleaned UpThe Railway preview environment for this PR has been removed. Environment: |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| #on: | ||
| # pull_request: | ||
| # types: [opened, synchronize] |
There was a problem hiding this comment.
The workflow trigger (on: pull_request) has been commented out, which disables this workflow entirely. If the intent is to keep Claude reviews running, restore the on: block; if the intent is to pause it, consider using workflow_dispatch (manual) or limiting it via paths/branches so it’s explicit and doesn’t silently disable CI automation.
Summary:
Key Changes:
dashboard-title-sync.tsxto keep titles consistent across client transitions and covered it with a new test.setup.ts,vitest.config.ts) and added coverage-focused specs for server selector, providers, landing sections, hook helpers, and API proxies.Impact:
Testing:
Structure: Use the following format:
• Title: A concise title following Conventional Commits.
• Summary: A high-level overview (2-3 sentences) of the purpose of these changes.
• Key Changes: A bulleted list of the most important logic or architectural changes.
• Impact: Briefly mention if this affects any downstream services or UI components.
• Testing: List the steps taken to verify the changes (e.g., Unit tests, manual verification).
Context: If I provide a git diff, focus on the logic changes. Ignore noise like whitespace changes, dependency lockfile updates, or minor linting fixes unless they are the primary focus.
Tone: Professional, objective, and technical. Avoid flowery language
Emoji Usage & Professionalism:⚠️ for warnings or technical debt.
• Minimalist Approach: Use emojis only for functional status indicators, never for decoration.
• Forbidden: No emojis in PR titles, commit messages, or headers. No "decorative" emojis (e.g., 🚀, ✨, 🤖, 💻).
• Allowed: Only use emojis to denote status in lists or tables, specifically:
• ✅ for passed tests/requirements.
• ❌ for failures or bugs.
•
• 💡 for optional suggestions/refactors.
• Text-First: If an emoji is used, it must be followed by clear text. Never use an emoji to replace a word.