Repository navigation
fix(webapp): server logs pagination and twitch not-configured ux - #613
Merged
Merged
Conversation
- Add total count to logs API response to fix pagination bug - Add countRecentLogs and countLogsByType methods to ServerLogService - Update frontend to use API-returned total instead of page length - Add GET /api/twitch/status endpoint to check configuration - Show clear banner when Twitch is not configured - Disable Add button when Twitch API credentials are missing - Update tests to expect total field in logs response
📝 WalkthroughWalkthroughThe PR adds server-side total count calculation for server logs endpoints and introduces a new Twitch configuration status check. Backend routes now compute and return log totals; the frontend pagination uses these provided counts. A new Twitch status endpoint checks API configuration availability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Size Change: +162 B (+0.05%) Total Size: 326 kB 📦 View Changed
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/backend/src/routes/management.ts (1)
221-237: Parallelize log fetch + count to reduce response timeBoth calls are independent in each branch; running them sequentially adds avoidable latency.
Suggested refactor
if (type) { - const logs = await serverLogService.getLogsByType( - guildId, - type as LogType, - limit, - ) - const total = await serverLogService.countLogsByType( - guildId, - type as LogType, - ) + const [logs, total] = await Promise.all([ + serverLogService.getLogsByType( + guildId, + type as LogType, + limit, + ), + serverLogService.countLogsByType( + guildId, + type as LogType, + ), + ]) res.json({ logs, total }) return } - const logs = await serverLogService.getRecentLogs(guildId, limit) - const total = await serverLogService.countRecentLogs(guildId) + const [logs, total] = await Promise.all([ + serverLogService.getRecentLogs(guildId, limit), + serverLogService.countRecentLogs(guildId), + ]) res.json({ logs, total })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/routes/management.ts` around lines 221 - 237, The two independent DB calls in management route are done sequentially and should be parallelized to reduce latency: for the type branch, call serverLogService.getLogsByType(guildId, type as LogType, limit) and serverLogService.countLogsByType(guildId, type as LogType) in parallel with Promise.all and then res.json({ logs, total }); likewise for the fallback branch use Promise.all with serverLogService.getRecentLogs(guildId, limit) and serverLogService.countRecentLogs(guildId) so both values are fetched concurrently before sending the response.packages/backend/tests/integration/routes/twitch.test.ts (1)
31-52: RestoreTWITCH_CLIENT_IDbetween tests to avoid env leakageThese tests mutate global process env but don’t restore the prior value, which can make neighboring tests flaky.
Suggested test hardening
describe('GET /api/twitch/status', () => { let app: express.Express + let previousClientId: string | undefined beforeEach(() => { + previousClientId = process.env.TWITCH_CLIENT_ID app = express() app.use(express.json()) setupTwitchRoutes(app) app.use(errorHandler) }) + + afterEach(() => { + if (previousClientId === undefined) { + delete process.env.TWITCH_CLIENT_ID + } else { + process.env.TWITCH_CLIENT_ID = previousClientId + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/integration/routes/twitch.test.ts` around lines 31 - 52, The tests mutate process.env.TWITCH_CLIENT_ID without restoring it; capture the original value before each test and restore it after each test (or use afterEach) so neighboring tests don't leak state — for example, save the current process.env.TWITCH_CLIENT_ID in beforeEach or at test start, run the test cases that set/delete process.env.TWITCH_CLIENT_ID (the tests named 'returns configured=true when TWITCH_CLIENT_ID is set' and 'returns configured=false when TWITCH_CLIENT_ID is not set'), then restore the saved value in afterEach to ensure process.env.TWITCH_CLIENT_ID is returned to its prior state for other tests.packages/frontend/src/pages/TwitchNotifications.test.tsx (1)
378-394: Nice UX test—consider one more assertion for short-circuit behaviorTo fully enforce the pre-check contract, assert that notifications fetch is skipped when
configuredis false.Optional assertion
expect( screen.getByText(/Twitch API is not configured/), ).toBeInTheDocument() expect(screen.queryByText('Add')).not.toBeInTheDocument() + expect(api.twitch.list).not.toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/pages/TwitchNotifications.test.tsx` around lines 378 - 394, The test should also assert that the notifications fetch is skipped when Twitch is unconfigured: after mocking api.twitch.status to return { configured: false } (as already done) and calling renderPage()/waitFor, add an assertion that vi.mocked(api.twitch.notifications) (or the actual notifications fetch function on the api.twitch object used in the component) was not called (e.g., expect(vi.mocked(api.twitch.notifications)).not.toHaveBeenCalled()); this enforces the short-circuit pre-check behavior alongside mockGuildSelection and existing DOM assertions.packages/backend/tests/integration/routes/management.test.ts (1)
725-763: Consider asserting count method calls in logs route testsYou already assert the response includes
total; adding call assertions would better lock route behavior to the new contract.Example assertions to add
expect(mockServerLogService.getRecentLogs).toHaveBeenCalledWith( '111111111111111111', 50, ) + expect(mockServerLogService.countRecentLogs).toHaveBeenCalledWith( + '111111111111111111', + )expect(mockServerLogService.getLogsByType).toHaveBeenCalledWith( '111111111111111111', 'error', 50, ) + expect(mockServerLogService.countLogsByType).toHaveBeenCalledWith( + '111111111111111111', + 'error', + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/integration/routes/management.test.ts` around lines 725 - 763, Tests for the logs route assert the response.total but don't assert that the corresponding count methods were called; add expectations that mockServerLogService.countRecentLogs was called with '111111111111111111' in the first test (after getRecentLogs assertion) and that mockServerLogService.countLogsByType was called with '111111111111111111' and 'error' in the second test (after getLogsByType assertion) to lock the route to the new contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/routes/twitch.ts`:
- Around line 113-114: The /api/twitch/status endpoint currently sets configured
= !!process.env.TWITCH_CLIENT_ID which can be a false positive; update the check
so configured is true only if TWITCH_CLIENT_ID is present AND at least one of
TWITCH_ACCESS_TOKEN or TWITCH_CLIENT_SECRET is present (i.e. configured =
!!TWITCH_CLIENT_ID && (!!TWITCH_ACCESS_TOKEN || !!TWITCH_CLIENT_SECRET)), then
return that value in res.json({ configured }); locate this in
packages/backend/src/routes/twitch.ts where configured is defined and used.
In `@packages/frontend/src/pages/TwitchNotifications.tsx`:
- Around line 55-62: The current checkTwitchStatus async handler maps any
exception from api.twitch.status() to setTwitchConfigured(false), which treats
network/timeouts/5xx as “not configured”; update checkTwitchStatus to
distinguish a real 404/unsuccessful configuration response from transient errors
by inspecting the error/response: call api.twitch.status() and on success
setTwitchConfigured(res.data.configured); on error, if the error indicates a
definitive “not configured” (e.g., response.status === 404 or error payload
explicitly says not configured) setTwitchConfigured(false), otherwise do not
change the existing twitchConfigured state and optionally set or log a
transientError flag; reference the checkTwitchStatus function, useEffect hook,
setTwitchConfigured, and api.twitch.status to locate where to implement this
logic.
In `@packages/frontend/src/services/logsApi.ts`:
- Around line 6-19: The current getRecent and getByType calls drop limit when it
is 0 because they use truthy checks; change the param handling to test for
undefined explicitly (e.g., limit !== undefined) so a numeric 0 is passed
through; update the params objects in getRecent and getByType (the apiClient.get
calls) to include limit when limit !== undefined while keeping other params
(like type) intact.
---
Nitpick comments:
In `@packages/backend/src/routes/management.ts`:
- Around line 221-237: The two independent DB calls in management route are done
sequentially and should be parallelized to reduce latency: for the type branch,
call serverLogService.getLogsByType(guildId, type as LogType, limit) and
serverLogService.countLogsByType(guildId, type as LogType) in parallel with
Promise.all and then res.json({ logs, total }); likewise for the fallback branch
use Promise.all with serverLogService.getRecentLogs(guildId, limit) and
serverLogService.countRecentLogs(guildId) so both values are fetched
concurrently before sending the response.
In `@packages/backend/tests/integration/routes/management.test.ts`:
- Around line 725-763: Tests for the logs route assert the response.total but
don't assert that the corresponding count methods were called; add expectations
that mockServerLogService.countRecentLogs was called with '111111111111111111'
in the first test (after getRecentLogs assertion) and that
mockServerLogService.countLogsByType was called with '111111111111111111' and
'error' in the second test (after getLogsByType assertion) to lock the route to
the new contract.
In `@packages/backend/tests/integration/routes/twitch.test.ts`:
- Around line 31-52: The tests mutate process.env.TWITCH_CLIENT_ID without
restoring it; capture the original value before each test and restore it after
each test (or use afterEach) so neighboring tests don't leak state — for
example, save the current process.env.TWITCH_CLIENT_ID in beforeEach or at test
start, run the test cases that set/delete process.env.TWITCH_CLIENT_ID (the
tests named 'returns configured=true when TWITCH_CLIENT_ID is set' and 'returns
configured=false when TWITCH_CLIENT_ID is not set'), then restore the saved
value in afterEach to ensure process.env.TWITCH_CLIENT_ID is returned to its
prior state for other tests.
In `@packages/frontend/src/pages/TwitchNotifications.test.tsx`:
- Around line 378-394: The test should also assert that the notifications fetch
is skipped when Twitch is unconfigured: after mocking api.twitch.status to
return { configured: false } (as already done) and calling renderPage()/waitFor,
add an assertion that vi.mocked(api.twitch.notifications) (or the actual
notifications fetch function on the api.twitch object used in the component) was
not called (e.g.,
expect(vi.mocked(api.twitch.notifications)).not.toHaveBeenCalled()); this
enforces the short-circuit pre-check behavior alongside mockGuildSelection and
existing DOM assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b650077b-31e6-4e37-8bde-24830df0c399
📒 Files selected for processing (10)
packages/backend/src/routes/management.tspackages/backend/src/routes/twitch.tspackages/backend/tests/integration/routes/management.test.tspackages/backend/tests/integration/routes/twitch.test.tspackages/frontend/src/pages/ServerLogs.tsxpackages/frontend/src/pages/TwitchNotifications.test.tsxpackages/frontend/src/pages/TwitchNotifications.tsxpackages/frontend/src/services/api.tspackages/frontend/src/services/logsApi.tspackages/shared/src/services/ServerLogService.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: SonarCloud Scan
- GitHub Check: compressed-size
- GitHub Check: Quality Gates
🔇 Additional comments (7)
packages/backend/tests/integration/routes/management.test.ts (1)
342-344: No concerns on this hunk (format-only split call)packages/shared/src/services/ServerLogService.ts (1)
84-92: Count helpers look correct and consistentThese methods match the same
wherefilters as the corresponding list queries, so backend pagination totals should stay accurate.packages/frontend/src/services/api.ts (1)
346-346: Status API client addition looks goodThis method cleanly matches the new backend status endpoint and keeps the typed response shape consistent.
packages/frontend/src/pages/ServerLogs.tsx (1)
175-181: Pagination total source fix is correctUsing
res.data.totalon Line 181 properly decouples pagination metadata from the current page’s sliced list.packages/frontend/src/pages/TwitchNotifications.test.tsx (1)
97-99: Defaultingapi.twitch.statusto configured in setup is solidGood baseline to keep existing tests focused unless a test explicitly overrides status.
packages/frontend/src/pages/TwitchNotifications.tsx (2)
290-308: Clear not-configured UX branch.The dedicated “Not Configured” state is straightforward and removes ambiguity for users.
324-325: Good defensive disable on Add action.Disabling the Add button when configuration isn’t confirmed prevents invalid attempts.
Comment on lines
+113
to
+114
| const configured = !!process.env.TWITCH_CLIENT_ID | ||
| res.json({ configured }) |
There was a problem hiding this comment.
/api/twitch/status can report a false positive configured state
Line 113 only checks TWITCH_CLIENT_ID, but lookup logic in this same file also needs either TWITCH_ACCESS_TOKEN or TWITCH_CLIENT_SECRET. This can re-enable the broken UX you’re fixing (button enabled, then lookup fails).
Suggested fix
- const configured = !!process.env.TWITCH_CLIENT_ID
+ const clientId = process.env.TWITCH_CLIENT_ID
+ const hasAccessToken = !!process.env.TWITCH_ACCESS_TOKEN
+ const hasClientSecret = !!process.env.TWITCH_CLIENT_SECRET
+ const configured = Boolean(clientId && (hasAccessToken || hasClientSecret))
res.json({ configured })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/routes/twitch.ts` around lines 113 - 114, The
/api/twitch/status endpoint currently sets configured =
!!process.env.TWITCH_CLIENT_ID which can be a false positive; update the check
so configured is true only if TWITCH_CLIENT_ID is present AND at least one of
TWITCH_ACCESS_TOKEN or TWITCH_CLIENT_SECRET is present (i.e. configured =
!!TWITCH_CLIENT_ID && (!!TWITCH_ACCESS_TOKEN || !!TWITCH_CLIENT_SECRET)), then
return that value in res.json({ configured }); locate this in
packages/backend/src/routes/twitch.ts where configured is defined and used.
Comment on lines
+55
to
+62
| useEffect(() => { | ||
| const checkTwitchStatus = async () => { | ||
| try { | ||
| const res = await api.twitch.status() | ||
| setTwitchConfigured(res.data.configured) | ||
| } catch { | ||
| setTwitchConfigured(false) | ||
| } |
There was a problem hiding this comment.
Don’t map all status-check failures to “not configured.”
Line 61 currently sets twitchConfigured to false for any exception (network failure, timeout, 5xx), which can show an incorrect “Not Configured” banner and block actions for a temporary outage.
Suggested fix
+ const [twitchStatusError, setTwitchStatusError] = useState<string | null>(null)
useEffect(() => {
const checkTwitchStatus = async () => {
try {
const res = await api.twitch.status()
setTwitchConfigured(res.data.configured)
+ setTwitchStatusError(null)
} catch {
- setTwitchConfigured(false)
+ setTwitchConfigured(null)
+ setTwitchStatusError('Unable to verify Twitch configuration status')
}
}
checkTwitchStatus()
}, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/pages/TwitchNotifications.tsx` around lines 55 - 62,
The current checkTwitchStatus async handler maps any exception from
api.twitch.status() to setTwitchConfigured(false), which treats
network/timeouts/5xx as “not configured”; update checkTwitchStatus to
distinguish a real 404/unsuccessful configuration response from transient errors
by inspecting the error/response: call api.twitch.status() and on success
setTwitchConfigured(res.data.configured); on error, if the error indicates a
definitive “not configured” (e.g., response.status === 404 or error payload
explicitly says not configured) setTwitchConfigured(false), otherwise do not
change the existing twitchConfigured state and optionally set or log a
transientError flag; reference the checkTwitchStatus function, useEffect hook,
setTwitchConfigured, and api.twitch.status to locate where to implement this
logic.
Comment on lines
6
to
+19
| getRecent: (guildId: string, limit?: number) => | ||
| apiClient.get<{ logs: ServerLog[] }>(`/guilds/${guildId}/logs`, { | ||
| params: limit ? { limit } : {}, | ||
| }), | ||
| apiClient.get<{ logs: ServerLog[]; total: number }>( | ||
| `/guilds/${guildId}/logs`, | ||
| { | ||
| params: limit ? { limit } : {}, | ||
| }, | ||
| ), | ||
| getByType: (guildId: string, type: string, limit?: number) => | ||
| apiClient.get<{ logs: ServerLog[] }>(`/guilds/${guildId}/logs`, { | ||
| params: { type, ...(limit ? { limit } : {}) }, | ||
| }), | ||
| apiClient.get<{ logs: ServerLog[]; total: number }>( | ||
| `/guilds/${guildId}/logs`, | ||
| { | ||
| params: { type, ...(limit ? { limit } : {}) }, | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Use explicit undefined checks for limit query params.
Current truthy checks omit limit when it is 0 (Line 10 and Line 17), which can silently change request behavior.
Suggested fix
getRecent: (guildId: string, limit?: number) =>
apiClient.get<{ logs: ServerLog[]; total: number }>(
`/guilds/${guildId}/logs`,
{
- params: limit ? { limit } : {},
+ params: limit !== undefined ? { limit } : {},
},
),
getByType: (guildId: string, type: string, limit?: number) =>
apiClient.get<{ logs: ServerLog[]; total: number }>(
`/guilds/${guildId}/logs`,
{
- params: { type, ...(limit ? { limit } : {}) },
+ params: { type, ...(limit !== undefined ? { limit } : {}) },
},
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/services/logsApi.ts` around lines 6 - 19, The current
getRecent and getByType calls drop limit when it is 0 because they use truthy
checks; change the param handling to test for undefined explicitly (e.g., limit
!== undefined) so a numeric 0 is passed through; update the params objects in
getRecent and getByType (the apiClient.get calls) to include limit when limit
!== undefined while keeping other params (like type) intact.
|
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…ts (#613) * fix(webapp): server logs pagination and twitch not-configured ux - Add total count to logs API response to fix pagination bug - Add countRecentLogs and countLogsByType methods to ServerLogService - Update frontend to use API-returned total instead of page length - Add GET /api/twitch/status endpoint to check configuration - Show clear banner when Twitch is not configured - Disable Add button when Twitch API credentials are missing - Update tests to expect total field in logs response * test(twitch): mock twitch status and add not-configured test * test(twitch): add status endpoint coverage tests
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server Logs Pagination Fix
countRecentLogsandcountLogsByTypemethods to ServerLogService; API now returns the actual total count/api/guilds/:guildId/logsendpoint to return{ logs, total }instead of just{ logs }res.data.totalfrom the API responseTwitch Notifications Not-Configured UX
GET /api/twitch/statusendpoint that returns{ configured: boolean }twitch.getStatus()method to the frontend API service🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements