Repository navigation
fix(api): wrap Slack JSON.parse calls in try-catch - #4144
vaishcodescape wants to merge 3 commits into
Conversation
- Introduced `batchGetGitHubStatuses` procedure to efficiently retrieve statuses for multiple workspaces. - Enhanced GitHub utilities with `batchFetchGitHubPRStatuses` for improved performance. - Updated workspace-related components to utilize the new batch fetching mechanism. - Adjusted query policies to align with the new batch polling interval for workspace list items. - Refactored related types and functions to support the new batch processing capabilities.
… handling Unhandled JSON.parse exceptions in the Slack events and interactions endpoints caused 500 responses on malformed payloads, triggering Slack's retry loop. Now returns 400 consistently, matching the existing pattern in link/route.ts.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces two independent changes: hardening Slack API route handlers with guarded JSON parsing to return 400 errors on invalid payloads, and implementing a new batch GitHub PR status fetching system for desktop workspaces that groups queries by repository, executes a single GraphQL call, and integrates results via a React hook and updated query policies. ChangesSlack JSON Payload Validation
GitHub Batch PR Status System
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related Issues
Possibly Related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Greptile SummaryThis PR has two distinct scopes: the stated fix wraps
Confidence Score: 3/5The Slack route changes are safe at runtime but likely break the TypeScript build in strict mode; the desktop batch feature is additive but the hover-gate removal may not fully deliver the N+1 fix it intends to. The Record<string, unknown> annotation in both Slack route files changes downstream property accesses from any (which TypeScript allows freely) to unknown (which TypeScript forbids without narrowing). Sites like payload.team?.id, iterating payload.actions, and reading event.type after destructuring would all be compiler errors in a strict-mode project — the same code compiled cleanly before because JSON.parse() returns any. If the CI pipeline runs tsc, this blocks the build. The batch desktop feature is architecturally sound but the hover-gate removal may cause all N workspace queries to fire simultaneously on initial mount before the batch result arrives. Both Slack route files (events/route.ts and interactions/route.ts) need the type annotation corrected before merging. batch-pr-status.ts and git-status.ts are worth a second look for the headSha omission and escape function.
|
| Filename | Overview |
|---|---|
| apps/api/src/app/api/integrations/slack/events/route.ts | Adds try-catch around JSON.parse. The Record<string, unknown> annotation makes all downstream accesses (event.type, event.channel_type, etc.) TypeScript errors in strict mode. |
| apps/api/src/app/api/integrations/slack/interactions/route.ts | Adds try-catch around JSON.parse. Same type-annotation issue: payload.team?.id, payload.user?.id, payload.actions iteration are all TypeScript errors with Record<string, unknown>. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts | New batch GraphQL PR-status fetcher. Logic is mostly solid; escapeGraphQLString is incomplete (missing newline/CR); cwd is taken from entries[0] which may misapply GitHub auth when entries span multiple hosts. |
| apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts | Adds batchGetGitHubStatuses procedure. Correct overall; headSha is not populated in batch entries, reducing PR-match precision compared to per-workspace path. |
| apps/desktop/src/renderer/hooks/useBatchGitHubStatus.ts | New hook that polls batch status and seeds per-workspace cache only when empty. Seeding logic is safe; races with per-workspace queries on initial load. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx | isActive no longer gated on hover; enables getGitHubStatus for all worktrees immediately, causing N individual queries on initial mount before batch seeds cache. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts | Adds GraphQL PR node schemas and normalizeGraphQLPR converter. Schema design with discriminated unions is correct; normalisation of statusCheckRollup and reviewRequests is clean. |
| apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.test.ts | New test file covering schema parsing and normalizeGraphQLPR. Good coverage of edge cases including null nodes, draft PRs, cross-repo PRs, and GHPRResponseSchema compatibility. |
Sequence Diagram
sequenceDiagram
participant Slack
participant EventsRoute as slack/events
participant InteractionsRoute as slack/interactions
participant QStash
Slack->>EventsRoute: POST (raw body)
EventsRoute->>EventsRoute: verifySlackSignature()
EventsRoute->>EventsRoute: JSON.parse(body) [try-catch]
alt malformed JSON
EventsRoute-->>Slack: 400 Invalid JSON payload
else valid JSON
EventsRoute->>QStash: publishJSON (mention/DM job)
EventsRoute-->>Slack: 200 ok
end
Slack->>InteractionsRoute: POST (form-encoded body)
InteractionsRoute->>InteractionsRoute: verifySlackSignature()
InteractionsRoute->>InteractionsRoute: JSON.parse(payloadRaw) [try-catch]
alt malformed JSON
InteractionsRoute-->>Slack: 400 Invalid JSON payload
else valid JSON
InteractionsRoute->>InteractionsRoute: handleModelSelect / handleDisconnectAccount
InteractionsRoute-->>Slack: 200 ok
end
Comments Outside Diff (2)
-
apps/api/src/app/api/integrations/slack/interactions/route.ts, line 40-51 (link)Record<string, unknown>annotation breaks downstream TypeScriptChanging
payloadfrom implicitany(returned byJSON.parse) tolet payload: Record<string, unknown>makes every indexed value typed asunknown. Accessingpayload.team?.id,payload.user?.id, and iteratingpayload.actionsare all TypeScript errors because you cannot call property-access or optional-chaining onunknownwithout first narrowing — the TypeScript compiler will rejectObject is of type 'unknown'for each of these sites. The same issue exists inevents/route.tswhereevent.type,event.channel_type, etc. are accessed after destructuring aRecord<string, unknown>. Usingas anycast on the assignment (payload = JSON.parse(payloadRaw) as any) or widening toRecord<string, any>would preserve runtime behavior without breaking the type checker.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/api/src/app/api/integrations/slack/interactions/route.ts Line: 40-51 Comment: **`Record<string, unknown>` annotation breaks downstream TypeScript** Changing `payload` from implicit `any` (returned by `JSON.parse`) to `let payload: Record<string, unknown>` makes every indexed value typed as `unknown`. Accessing `payload.team?.id`, `payload.user?.id`, and iterating `payload.actions` are all TypeScript errors because you cannot call property-access or optional-chaining on `unknown` without first narrowing — the TypeScript compiler will reject `Object is of type 'unknown'` for each of these sites. The same issue exists in `events/route.ts` where `event.type`, `event.channel_type`, etc. are accessed after destructuring a `Record<string, unknown>`. Using `as any` cast on the assignment (`payload = JSON.parse(payloadRaw) as any`) or widening to `Record<string, any>` would preserve runtime behavior without breaking the type checker. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx, line 148-151 (link)N+1 individual queries still fire on initial mount
Now that
isActiveis alwaystruefor worktrees (no longer gated onhasHovered), everyWorkspaceListItemimmediately enables itsgetGitHubStatusquery. React Query fires all of these the moment the sidebar renders — beforeuseBatchGitHubStatusinWorkspaceSidebarhas resolved and seeded the cache. The batch approach avoids N+1 polls, but the initial load still spawns N individualgh pr viewprocesses in parallel. Consider keepingisActive: falsein this policy and relying exclusively on the batch hook to seed the per-workspace cache, then only enabling individual queries when the batch result is stale or missing for that workspace.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx Line: 148-151 Comment: **N+1 individual queries still fire on initial mount** Now that `isActive` is always `true` for worktrees (no longer gated on `hasHovered`), every `WorkspaceListItem` immediately enables its `getGitHubStatus` query. React Query fires all of these the moment the sidebar renders — before `useBatchGitHubStatus` in `WorkspaceSidebar` has resolved and seeded the cache. The batch approach avoids N+1 polls, but the initial load still spawns N individual `gh pr view` processes in parallel. Consider keeping `isActive: false` in this policy and relying exclusively on the batch hook to seed the per-workspace cache, then only enabling individual queries when the batch result is stale or missing for that workspace. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/api/src/app/api/integrations/slack/interactions/route.ts:40-51
**`Record<string, unknown>` annotation breaks downstream TypeScript**
Changing `payload` from implicit `any` (returned by `JSON.parse`) to `let payload: Record<string, unknown>` makes every indexed value typed as `unknown`. Accessing `payload.team?.id`, `payload.user?.id`, and iterating `payload.actions` are all TypeScript errors because you cannot call property-access or optional-chaining on `unknown` without first narrowing — the TypeScript compiler will reject `Object is of type 'unknown'` for each of these sites. The same issue exists in `events/route.ts` where `event.type`, `event.channel_type`, etc. are accessed after destructuring a `Record<string, unknown>`. Using `as any` cast on the assignment (`payload = JSON.parse(payloadRaw) as any`) or widening to `Record<string, any>` would preserve runtime behavior without breaking the type checker.
### Issue 2 of 4
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts:317-319
**`escapeGraphQLString` does not escape control characters**
The function only replaces `\` and `"`, but GraphQL string literals forbid unescaped newline (`\n`), carriage-return (`\r`), and other control characters. While git refs and GitHub owner/repo names cannot contain newlines today, a more complete escape protects against unexpected inputs and matches GraphQL spec requirements.
```suggestion
function escapeGraphQLString(value: string): string {
return value
.replace(/\\/g, "\\\\")
.replace(/"/g, '\\"')
.replace(/\n/g, "\\n")
.replace(/\r/g, "\\r")
.replace(/\t/g, "\\t");
}
```
### Issue 3 of 4
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:148-151
**N+1 individual queries still fire on initial mount**
Now that `isActive` is always `true` for worktrees (no longer gated on `hasHovered`), every `WorkspaceListItem` immediately enables its `getGitHubStatus` query. React Query fires all of these the moment the sidebar renders — before `useBatchGitHubStatus` in `WorkspaceSidebar` has resolved and seeded the cache. The batch approach avoids N+1 polls, but the initial load still spawns N individual `gh pr view` processes in parallel. Consider keeping `isActive: false` in this policy and relying exclusively on the batch hook to seed the per-workspace cache, then only enabling individual queries when the batch result is stale or missing for that workspace.
### Issue 4 of 4
apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts:223-235
**`headSha` is omitted from batch entries, reducing PR-match precision**
`BatchWorkspaceEntry.headSha` is optional, but it is used by both `shouldAcceptPRMatch` and `sortPRCandidates` to prefer the PR whose `headRefOid` matches the local commit. Because the procedure never reads the worktree's current HEAD sha and never populates `headSha`, the batch path uses branch-name matching only — meaning it can silently pick a stale or incorrect PR when multiple PRs share the same branch name. The per-workspace `getGitHubStatus` path reads the sha, so the two code paths can disagree on which PR to display.
Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/slack-json-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function escapeGraphQLString(value: string): string { | ||
| return value.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); | ||
| } |
There was a problem hiding this comment.
escapeGraphQLString does not escape control characters
The function only replaces \ and ", but GraphQL string literals forbid unescaped newline (\n), carriage-return (\r), and other control characters. While git refs and GitHub owner/repo names cannot contain newlines today, a more complete escape protects against unexpected inputs and matches GraphQL spec requirements.
| function escapeGraphQLString(value: string): string { | |
| return value.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); | |
| } | |
| function escapeGraphQLString(value: string): string { | |
| return value | |
| .replace(/\\/g, "\\\\") | |
| .replace(/"/g, '\\"') | |
| .replace(/\n/g, "\\n") | |
| .replace(/\r/g, "\\r") | |
| .replace(/\t/g, "\\t"); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/workspaces/utils/github/batch-pr-status.ts
Line: 317-319
Comment:
**`escapeGraphQLString` does not escape control characters**
The function only replaces `\` and `"`, but GraphQL string literals forbid unescaped newline (`\n`), carriage-return (`\r`), and other control characters. While git refs and GitHub owner/repo names cannot contain newlines today, a more complete escape protects against unexpected inputs and matches GraphQL spec requirements.
```suggestion
function escapeGraphQLString(value: string): string {
return value
.replace(/\\/g, "\\\\")
.replace(/"/g, '\\"')
.replace(/\n/g, "\\n")
.replace(/\r/g, "\\r")
.replace(/\t/g, "\\t");
}
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const allWorktreeRows = localDb.select().from(worktrees).all(); | ||
| const worktreeMap = new Map(allWorktreeRows.map((wt) => [wt.id, wt])); | ||
| const workspaceMap = new Map( | ||
| allWorkspaceRows.map((ws) => [ws.id, ws]), | ||
| ); | ||
|
|
||
| const entries = allWorkspaceRows | ||
| .filter((ws) => ws.type === "worktree" && ws.worktreeId) | ||
| .map((ws) => { | ||
| // biome-ignore lint/style/noNonNullAssertion: filtered above | ||
| const wt = worktreeMap.get(ws.worktreeId!); | ||
| if (!wt) return null; |
There was a problem hiding this comment.
headSha is omitted from batch entries, reducing PR-match precision
BatchWorkspaceEntry.headSha is optional, but it is used by both shouldAcceptPRMatch and sortPRCandidates to prefer the PR whose headRefOid matches the local commit. Because the procedure never reads the worktree's current HEAD sha and never populates headSha, the batch path uses branch-name matching only — meaning it can silently pick a stale or incorrect PR when multiple PRs share the same branch name. The per-workspace getGitHubStatus path reads the sha, so the two code paths can disagree on which PR to display.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/lib/trpc/routers/workspaces/procedures/git-status.ts
Line: 223-235
Comment:
**`headSha` is omitted from batch entries, reducing PR-match precision**
`BatchWorkspaceEntry.headSha` is optional, but it is used by both `shouldAcceptPRMatch` and `sortPRCandidates` to prefer the PR whose `headRefOid` matches the local commit. Because the procedure never reads the worktree's current HEAD sha and never populates `headSha`, the batch path uses branch-name matching only — meaning it can silently pick a stale or incorrect PR when multiple PRs share the same branch name. The per-workspace `getGitHubStatus` path reads the sha, so the two code paths can disagree on which PR to display.
How can I resolve this? If you propose a fix, please make it concise.
Summary
JSON.parse()in try-catch in the Slack events (events/route.ts) and interactions (interactions/route.ts) API route handlers400 Bad Requestwith{ error: "Invalid JSON payload" }on malformed JSON instead of crashing with an unhandled exception (500)link/route.tswhich already handles parse errors gracefullyProblem
The Slack events and interactions endpoints call
JSON.parse()without error handling. If the request body contains invalid JSON (e.g., corrupted in transit or truncated), the unhandled exception causes a 500 response. Slack then retries the event up to 3 times, each retry also failing — amplifying the issue.Affected endpoints
slack/eventsapp_mention, DM messages,link_shared,entity_details_requested,app_home_openedslack/interactionsTest plan
Summary by cubic
Batch-fetches GitHub PR statuses to speed up the desktop sidebar and cut N+1 GitHub calls. Also hardens Slack webhooks to return 400 on bad JSON instead of 500.
New Features
batchGetGitHubStatuses,batchFetchGitHubPRStatuses, anduseBatchGitHubStatusto seed per-workspace caches without overwriting richer data.Bug Fixes
JSON.parsein Slack events and interactions routes with try/catch.{ error: "Invalid JSON payload" }on malformed bodies to prevent 500s and Slack retries.link/route.ts.Written for commit 26960da. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests