Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a 30s hard timeout to the Mattermost bootstrap POST handler (returns 504 on expiry), moves bootstrap logic into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BootstrapAPI as "Bootstrap Handler"
participant QueryClient
participant Subscriptions as "Subscriptions Fetcher"
participant Timer
participant Response
Client->>BootstrapAPI: POST /api/mattermost/bootstrap
BootstrapAPI->>QueryClient: construct per-request QueryClient
BootstrapAPI->>Timer: start 30s timeout race
par Main execution
BootstrapAPI->>Subscriptions: fetch subscriptions (uses QueryClient)
Subscriptions-->>BootstrapAPI: data / error
BootstrapAPI->>BootstrapAPI: perform bootstrap logic (handleBootstrap)
and Timeout race
Timer->>Timer: wait 30 seconds
end
alt Bootstrap completes
BootstrapAPI->>QueryClient: clear/reset (finally)
BootstrapAPI->>Response: return 200 with result
else Timeout expires
Timer->>BootstrapAPI: reject with BootstrapTimeoutError
BootstrapAPI->>QueryClient: clear/reset (finally)
BootstrapAPI->>Response: return 504 { error: "bootstrap timed out" }
else Other error
BootstrapAPI->>QueryClient: clear/reset (finally)
BootstrapAPI->>Response: return 500 { error: <message> }
end
Response->>Client: HTTP response
sequenceDiagram
participant Client
participant Server as "SSR Server"
participant ReactQuery as "React Query"
participant Timer
participant Cache as "Query Cache"
Client->>Server: Request page (SSR)
Server->>ReactQuery: prefetchQuery(key, fn)
alt isServer true
ReactQuery->>Timer: wrap prefetch with withSsrTimeout (SSR_PREFETCH_TIMEOUT_MS)
par Prefetch execution
ReactQuery->>ReactQuery: execute fetch function
ReactQuery->>Cache: store result if completes
and Timeout monitor
Timer->>Timer: wait timeout
end
alt Prefetch completes
ReactQuery->>Cache: data stored
else Timeout expires
Timer->>ReactQuery: resolve to undefined (no rejection)
end
else isServer false
ReactQuery->>ReactQuery: call prefetch directly (no timeout)
end
ReactQuery->>Cache: getQueryData(key)
Cache->>Server: return cached data or undefined
Server->>Client: Rendered HTML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/app/api/mattermost/bootstrap/route.ts (3)
119-127: Consider disconnecting the QueryClient after use.The per-request
QueryClientis a good choice to avoid stale cache issues (as the comment explains). However, callingqc.clear()after use ensures any internal timers or subscriptions are cleaned up immediately rather than waiting for garbage collection.♻️ Proposed fix
let subscriptions: Subscription[] = []; try { const qc = new QueryClient(); - subscriptions = - (await qc.fetchQuery( - getAccountSubscriptionsQueryOptions(username) - )) || []; + try { + subscriptions = + (await qc.fetchQuery( + getAccountSubscriptionsQueryOptions(username) + )) || []; + } finally { + qc.clear(); + } } catch (error) { console.error("MM bootstrap: Unable to load Hive/Ecency subscriptions", error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/mattermost/bootstrap/route.ts` around lines 119 - 127, The QueryClient instance (qc) created to fetch subscriptions may leave timers/subscriptions active; ensure you call qc.clear() after use to immediately clean up internal state—wrap the fetchQuery call in a try/finally (or ensure in both success and catch paths) so that qc.clear() is invoked regardless of outcome; reference the QueryClient instance named qc used with fetchQuery(getAccountSubscriptionsQueryOptions(username)) and clear it after setting subscriptions.
47-60: Consider adding runtime validation for the request body.Using
anyfor the parsed body loses type safety. A runtime validation library likezodwould provide both type inference and validation, reducing the risk of unexpected runtime errors from malformed requests.// Example with zod import { z } from "zod"; const BootstrapBodySchema = z.object({ username: z.string().optional(), accessToken: z.string().optional(), refreshToken: z.string().optional(), displayName: z.string().optional(), communityTitle: z.string().optional(), community: z.string().optional(), }); // Then in handleBootstrap: const parsed = BootstrapBodySchema.safeParse(await req.json()); if (!parsed.success) { return NextResponse.json({ error: "invalid json" }, { status: 400 }); } const body = parsed.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/mattermost/bootstrap/route.ts` around lines 47 - 60, Replace the loose any parsing of req.json() with runtime validation: define a BootstrapBodySchema (e.g., using zod) that lists optional string fields username, accessToken, refreshToken, displayName, communityTitle, and community, then call safeParse on the parsed JSON before using it; if validation fails return the same 400 response, otherwise assign const body = parsed.data and continue using body.username / body.accessToken / body.refreshToken / body.displayName / body.communityTitle / body.community to ensure typed, validated inputs for the bootstrap handler.
24-29: Timer is not cleared whenhandleBootstrapresolves first; consider cleanup.When
handleBootstrapcompletes before the 30-second timeout, thesetTimeoutremains scheduled until it fires. While the reject becomes a no-op on the settled promise, it's cleaner to clear the timer explicitly.Additionally, more critically, when the timeout fires,
handleBootstrapcontinues executing in the background—making Mattermost API calls whose results are discarded. This wastes server resources and could contribute to rate limiting.♻️ Proposed fix with timer cleanup and optional AbortSignal
export async function POST(req: Request) { try { + const controller = new AbortController(); + let timeoutId: NodeJS.Timeout | undefined; + + const timeoutPromise = new Promise<never>((_, reject) => { + timeoutId = setTimeout(() => { + controller.abort(); + reject(new BootstrapTimeoutError()); + }, BOOTSTRAP_TIMEOUT_MS); + }); + + try { - return await Promise.race([ - handleBootstrap(req), - new Promise<never>((_, reject) => - setTimeout(() => reject(new BootstrapTimeoutError()), BOOTSTRAP_TIMEOUT_MS) - ) - ]); + return await Promise.race([ + handleBootstrap(req, controller.signal), + timeoutPromise + ]); + } finally { + clearTimeout(timeoutId); + } } catch (error) {Then pass the signal to
handleBootstrapand propagate it to any fetch calls that support it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/api/mattermost/bootstrap/route.ts` around lines 24 - 29, The current Promise.race usage leaves the timeout scheduled and doesn't stop handleBootstrap when the timeout wins; change the pattern to create a timer via setTimeout returning a handle, and clearTimeout(timer) when handleBootstrap resolves or rejects to avoid dangling timers; also create an AbortController and pass controller.signal into handleBootstrap (and update handleBootstrap to accept an optional AbortSignal and forward it to any fetch/async calls) and call controller.abort() when the timeout fires (i.e., when you reject with BootstrapTimeoutError) so ongoing background work is canceled; reference symbols: handleBootstrap, BootstrapTimeoutError, BOOTSTRAP_TIMEOUT_MS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/api/mattermost/bootstrap/route.ts`:
- Around line 119-127: The QueryClient instance (qc) created to fetch
subscriptions may leave timers/subscriptions active; ensure you call qc.clear()
after use to immediately clean up internal state—wrap the fetchQuery call in a
try/finally (or ensure in both success and catch paths) so that qc.clear() is
invoked regardless of outcome; reference the QueryClient instance named qc used
with fetchQuery(getAccountSubscriptionsQueryOptions(username)) and clear it
after setting subscriptions.
- Around line 47-60: Replace the loose any parsing of req.json() with runtime
validation: define a BootstrapBodySchema (e.g., using zod) that lists optional
string fields username, accessToken, refreshToken, displayName, communityTitle,
and community, then call safeParse on the parsed JSON before using it; if
validation fails return the same 400 response, otherwise assign const body =
parsed.data and continue using body.username / body.accessToken /
body.refreshToken / body.displayName / body.communityTitle / body.community to
ensure typed, validated inputs for the bootstrap handler.
- Around line 24-29: The current Promise.race usage leaves the timeout scheduled
and doesn't stop handleBootstrap when the timeout wins; change the pattern to
create a timer via setTimeout returning a handle, and clearTimeout(timer) when
handleBootstrap resolves or rejects to avoid dangling timers; also create an
AbortController and pass controller.signal into handleBootstrap (and update
handleBootstrap to accept an optional AbortSignal and forward it to any
fetch/async calls) and call controller.abort() when the timeout fires (i.e.,
when you reject with BootstrapTimeoutError) so ongoing background work is
canceled; reference symbols: handleBootstrap, BootstrapTimeoutError,
BOOTSTRAP_TIMEOUT_MS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 375a8dee-68e9-4800-aab4-3e78bf17e15f
📒 Files selected for processing (2)
apps/web/src/app/api/mattermost/bootstrap/route.tsapps/web/src/core/react-query/query-helpers.ts
feruzm
added
patch
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.
Summary by CodeRabbit
Performance
Reliability
Chores