agent: @U0AJM7X8FBR Admin + Docs + API - we want a new admin page to view analy#330
agent: @U0AJM7X8FBR Admin + Docs + API - we want a new admin page to view analy#330sweetmantech wants to merge 1 commit intotestfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA new admin API endpoint ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Route Handler
participant Validator as Validator
participant Handler as GET Handler
participant Slack as Slack Web API
participant Auth as Auth System
Client->>Route: GET /api/admins/coding-agent/slack-tags?period=weekly
Route->>Handler: getSlackTagsHandler(request)
Handler->>Validator: validateGetSlackTagsQuery(request)
Validator->>Auth: validateAdminAuth(request)
alt Auth fails
Auth-->>Validator: NextResponse (error)
Validator-->>Handler: NextResponse (error)
Handler-->>Route: NextResponse (error)
Route-->>Client: 401/403 Response
else Auth succeeds
Auth-->>Validator: ✓
Validator->>Validator: Extract & validate period
Validator-->>Handler: { period: "weekly" }
Handler->>Slack: auth.test()
Slack-->>Handler: Bot user_id
Handler->>Slack: conversations.list() + pagination
Slack-->>Handler: Channel list
loop For each channel
Handler->>Slack: conversations.history(channel, oldest cutoff)
Slack-->>Handler: Messages (paginated)
loop For matching messages
Handler->>Slack: users.info(user_id)
Slack-->>Handler: User details (cached)
Handler->>Handler: Build SlackTag
end
end
Handler->>Handler: Sort by timestamp (newest-first)
Handler-->>Route: NextResponse { status, total, tags, CORS headers }
Route-->>Client: 200 JSON Response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The changes introduce new functionality across four coordinated files totaling 281 lines. While individual files are straightforward (route handler, validators, orchestrator), the Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/admins/coding-agent/slack-tags/route.ts`:
- Around line 17-18: Add a JSDoc block for the exported route handler OPTIONS
explaining its purpose (responds to preflight CORS requests), mention it returns
a 204 No Content with CORS headers, and annotate the return type
(Promise<NextResponse>); place the comment immediately above the export async
function OPTIONS() declaration and reference getCorsHeaders() in the description
to clarify where headers come from.
- Around line 13-18: The PR lacks automated tests for the new Slack tags
endpoint in route.ts (GET -> getSlackTagsHandler and OPTIONS preflight); add a
small test matrix that covers: successful GET (mock auth to return a valid user
and mock the Slack upstream to return tags, assert JSON and status 200), invalid
period parameter (call GET with bad period, assert 400), auth rejection (mock
auth to fail, assert 401/403 as implemented), Slack upstream failure (mock the
upstream call used by getSlackTagsHandler to return an error/timeout and assert
the handler returns the expected error/status), and the OPTIONS preflight (call
OPTIONS and assert status 204 and CORS headers via getCorsHeaders()). Use unit
tests on getSlackTagsHandler for orchestration logic and a lightweight
route-level test for OPTIONS; mock external deps (auth, Slack client,
getCorsHeaders) and keep tests minimal and focused.
In `@lib/admins/coding-agent/fetchSlackMentions.ts`:
- Around line 98-103: The code currently treats Slack API failures in
conversations.list and conversations.history by breaking out of loops, allowing
partial results to be returned; update the logic in fetchSlackMentions (around
the calls to slackGet for "conversations.list" and "conversations.history") to
throw a descriptive Error instead of using break when resp.ok is false (include
resp.error or status info in the message), matching the auth.test error
behavior; ensure the thrown error propagates up so callers receive a failure
rather than incomplete tag data.
- Around line 67-71: The default period value of "all" causes getCutoffTs to
return null and triggers an unbounded full-workspace crawl (function getCutoffTs
and the code path that iterates channels/messages), which will exhaust Slack API
limits; fix by changing the default period from "all" to a bounded window such
as "weekly" in the request validation (where the default is set) and/or enforce
a maximum allowed window in validateGetSlackTagsQuery so getCutoffTs never
receives "all" for unauthenticated/public requests; alternatively implement
caching/persistence (store previously fetched mentions and perform incremental
updates) and add server-side protection (rate limiting or requiring auth) to
avoid walking all channels.
In `@lib/admins/coding-agent/validateGetSlackTagsQuery.ts`:
- Around line 36-40: The error response in the failure branch of
validateGetSlackTagsQuery.ts returns { status, error } but other branches use {
status, message }; change the NextResponse.json payload in the result.success
false branch (where firstError is extracted) to return { status: "error",
message: firstError.message } while preserving the 400 status and
getCorsHeaders() usage so the client sees a consistent error envelope; update
any local variable names if needed to match this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d09486ec-3d3e-45a3-9ffe-e34e37997dee
⛔ Files ignored due to path filters (2)
lib/admins/coding-agent/__tests__/getSlackTagsHandler.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**lib/admins/coding-agent/__tests__/validateGetSlackTagsQuery.test.tsis excluded by!**/*.test.*,!**/__tests__/**and included bylib/**
📒 Files selected for processing (4)
app/api/admins/coding-agent/slack-tags/route.tslib/admins/coding-agent/fetchSlackMentions.tslib/admins/coding-agent/getSlackTagsHandler.tslib/admins/coding-agent/validateGetSlackTagsQuery.ts
| export async function GET(request: NextRequest): Promise<NextResponse> { | ||
| return getSlackTagsHandler(request); | ||
| } | ||
|
|
||
| export async function OPTIONS(): Promise<NextResponse> { | ||
| return new NextResponse(null, { status: 204, headers: getCorsHeaders() }); |
There was a problem hiding this comment.
Please add endpoint coverage before merging.
I don't see tests in this change set for the success path, invalid period, auth rejection, Slack upstream failure, or the OPTIONS preflight. This endpoint is mostly orchestration, so a small route/handler test matrix will catch regressions quickly.
Based on learnings: "Write tests for new API endpoints covering all success and error paths."
If helpful, I can sketch the minimal test matrix for the route/handler split.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/admins/coding-agent/slack-tags/route.ts` around lines 13 - 18, The PR
lacks automated tests for the new Slack tags endpoint in route.ts (GET ->
getSlackTagsHandler and OPTIONS preflight); add a small test matrix that covers:
successful GET (mock auth to return a valid user and mock the Slack upstream to
return tags, assert JSON and status 200), invalid period parameter (call GET
with bad period, assert 400), auth rejection (mock auth to fail, assert 401/403
as implemented), Slack upstream failure (mock the upstream call used by
getSlackTagsHandler to return an error/timeout and assert the handler returns
the expected error/status), and the OPTIONS preflight (call OPTIONS and assert
status 204 and CORS headers via getCorsHeaders()). Use unit tests on
getSlackTagsHandler for orchestration logic and a lightweight route-level test
for OPTIONS; mock external deps (auth, Slack client, getCorsHeaders) and keep
tests minimal and focused.
| export async function OPTIONS(): Promise<NextResponse> { | ||
| return new NextResponse(null, { status: 204, headers: getCorsHeaders() }); |
There was a problem hiding this comment.
Add JSDoc for OPTIONS.
Line 17 is an exported route handler and currently has no docblock.
Based on learnings: "All API routes should have JSDoc comments."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/admins/coding-agent/slack-tags/route.ts` around lines 17 - 18, Add a
JSDoc block for the exported route handler OPTIONS explaining its purpose
(responds to preflight CORS requests), mention it returns a 204 No Content with
CORS headers, and annotate the return type (Promise<NextResponse>); place the
comment immediately above the export async function OPTIONS() declaration and
reference getCorsHeaders() in the description to clarify where headers come
from.
| function getCutoffTs(period: SlackTagsPeriod): number | null { | ||
| if (period === "all") return null; | ||
| const days = period === "daily" ? 1 : period === "weekly" ? 7 : 30; | ||
| return (Date.now() - days * 24 * 60 * 60 * 1000) / 1000; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
fd "fetchSlackMentions.ts" -t fRepository: recoupable/api
Length of output: 103
🏁 Script executed:
# Check file size to determine best way to read
wc -l lib/admins/coding-agent/fetchSlackMentions.tsRepository: recoupable/api
Length of output: 107
🏁 Script executed:
# Read the full file
cat -n lib/admins/coding-agent/fetchSlackMentions.tsRepository: recoupable/api
Length of output: 6692
🏁 Script executed:
# Find and read validateGetSlackTagsQuery to verify the default period
fd "validateGetSlackTagsQuery*" -t fRepository: recoupable/api
Length of output: 178
🏁 Script executed:
# Read validateGetSlackTagsQuery to see the default period
cat -n lib/admins/coding-agent/validateGetSlackTagsQuery.tsRepository: recoupable/api
Length of output: 1832
🏁 Script executed:
# Check if there's a route handler that calls this function
fd "slack-tags" -t fRepository: recoupable/api
Length of output: 40
🏁 Script executed:
# Check for any caching layer or memoization
rg "fetchSlackMentions" -A 3 -B 3Repository: recoupable/api
Length of output: 7053
🏁 Script executed:
# Read the handler to see if there's any caching or timeout handling
cat -n lib/admins/coding-agent/getSlackTagsHandler.tsRepository: recoupable/api
Length of output: 1601
Address the full workspace crawl risk in the default period="all" mode.
When getCutoffTs() returns null, the function walks through every accessible channel and all messages in each channel without time filtering (lines 115–174). Since period defaults to "all" (validateGetSlackTagsQuery.ts line 7), an unauthenticated request hits this expensive path immediately. On larger workspaces, this generates excessive Slack API calls that will exceed rate limits (Slack's web API allows ~20 requests/minute) or exceed request timeouts.
Before enabling this endpoint in production, implement caching/persistence (e.g., store mentions in the database and refresh incrementally) or change the default period to a bounded option like "weekly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/admins/coding-agent/fetchSlackMentions.ts` around lines 67 - 71, The
default period value of "all" causes getCutoffTs to return null and triggers an
unbounded full-workspace crawl (function getCutoffTs and the code path that
iterates channels/messages), which will exhaust Slack API limits; fix by
changing the default period from "all" to a bounded window such as "weekly" in
the request validation (where the default is set) and/or enforce a maximum
allowed window in validateGetSlackTagsQuery so getCutoffTs never receives "all"
for unauthenticated/public requests; alternatively implement caching/persistence
(store previously fetched mentions and perform incremental updates) and add
server-side protection (rate limiting or requiring auth) to avoid walking all
channels.
| do { | ||
| const params: Record<string, string> = { types: "public_channel,private_channel", limit: "200" }; | ||
| if (channelCursor) params.cursor = channelCursor; | ||
|
|
||
| const resp = await slackGet<ConversationsListResponse>("conversations.list", token, params); | ||
| if (!resp.ok) break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/admins/coding-agent/fetchSlackMentions.tsRepository: recoupable/api
Length of output: 6692
Replace silent partial failures with explicit error handling to prevent incomplete analytics.
Lines 103 and 128 break on Slack API failures, allowing the function to return whatever tags were collected before the failure. This means incomplete data silently passes through, undercounting mentions for an analytics endpoint.
The error handling is also inconsistent—line 89 throws on auth.test failure, but conversations.list (103) and conversations.history (128) silently break. To maintain data integrity, propagate these failures as errors instead.
Suggested fix
- if (!resp.ok) break;
+ if (!resp.ok) {
+ throw new Error(`Slack conversations.list failed: ${resp.error ?? "unknown error"}`);
+ }
…
- if (!history.ok) break;
+ if (!history.ok) {
+ throw new Error(
+ `Slack conversations.history failed for channel ${channel.id}: ${history.error ?? "unknown error"}`,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/admins/coding-agent/fetchSlackMentions.ts` around lines 98 - 103, The
code currently treats Slack API failures in conversations.list and
conversations.history by breaking out of loops, allowing partial results to be
returned; update the logic in fetchSlackMentions (around the calls to slackGet
for "conversations.list" and "conversations.history") to throw a descriptive
Error instead of using break when resp.ok is false (include resp.error or status
info in the message), matching the auth.test error behavior; ensure the thrown
error propagates up so callers receive a failure rather than incomplete tag
data.
| if (!result.success) { | ||
| const firstError = result.error.issues[0]; | ||
| return NextResponse.json( | ||
| { status: "error", error: firstError.message }, | ||
| { status: 400, headers: getCorsHeaders() }, |
There was a problem hiding this comment.
Keep the error envelope consistent with the other branches.
Line 39 returns { status, error }, but the 403 branch in lib/admins/validateAdminAuth.ts and the 500 branch in lib/admins/coding-agent/getSlackTagsHandler.ts both use { status, message }. Reusing message here keeps client-side error handling uniform.
As per coding guidelines: "Return consistent response formats."
Suggested fix
- { status: "error", error: firstError.message },
+ { status: "error", message: firstError.message },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/admins/coding-agent/validateGetSlackTagsQuery.ts` around lines 36 - 40,
The error response in the failure branch of validateGetSlackTagsQuery.ts returns
{ status, error } but other branches use { status, message }; change the
NextResponse.json payload in the result.success false branch (where firstError
is extracted) to return { status: "error", message: firstError.message } while
preserving the 400 status and getCorsHeaders() usage so the client sees a
consistent error envelope; update any local variable names if needed to match
this change.
1 participant
Automated PR from coding agent.
Summary by CodeRabbit
New Features