ECHO-428 Portal "Conversation Ended" fixed#272
Conversation
- added retry logic to check conversation endpoint
WalkthroughAdds AxiosError import and retry-policy changes in participant hooks; increases chunk upload retries; introduces status-aware retry logic for conversation queries; reformats hook signatures; and updates ParticipantConversation to detect specific HTTP deletion statuses (403/404/410), stop recording, and set conversationDeletedDuringRecording during recording. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Assessment against linked issues
Possibly related PRs
LGTM. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
echo/frontend/src/components/participant/hooks/index.ts(4 hunks)echo/frontend/src/routes/participant/ParticipantConversation.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses regular useQuery (not useSuspenseQuery), which means conversationsCountQuery.data can be undefined during loading states. When using Number(conversationsCountQuery.data) ?? 0, this creates NaN because Number(undefined) = NaN and NaN is not nullish, so the fallback doesn't apply. The correct pattern is Number(conversationsCountQuery.data ?? 0) to ensure the fallback happens before type conversion.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses useSuspenseQuery with Directus aggregate, which always returns string numbers like "0", "1", "2" and suspends during loading instead of returning undefined. Therefore, Number(conversationsCountQuery.data) ?? 0 is safe and the Number() conversion is necessary for type conversion from string to number.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
🧬 Code graph analysis (1)
echo/frontend/src/components/participant/hooks/index.ts (1)
echo/frontend/src/lib/api.ts (1)
getParticipantConversationById(40-44)
⏰ 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). (2)
- GitHub Check: ci-build-servers (dbr-echo-server, ./echo/server, Dockerfile, dbr-echo-server)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
echo/frontend/src/routes/participant/ParticipantConversation.tsx (1)
60-60: LGTM: AxiosError import is correct and scoped to the new effect.echo/frontend/src/components/participant/hooks/index.ts (3)
16-16: LGTM: AxiosError import aligns with retry predicate typing.
311-314: LGTM: Signature reflow only.
361-361: No-op change detected. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
echo/frontend/src/routes/participant/ParticipantConversation.tsx (1)
199-203: Good catch: includeconversationQuery.errorin depsPrevents stale handling when the error object changes while flags remain true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/frontend/src/routes/participant/ParticipantConversation.tsx(2 hunks)
⏰ 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). (2)
- GitHub Check: ci-build-servers (dbr-echo-server, ./echo/server, Dockerfile, dbr-echo-server)
- GitHub Check: ci-build-servers (dbr-echo-directus, ./echo/directus, Dockerfile, dbr-echo-directus)
🔇 Additional comments (3)
echo/frontend/src/routes/participant/ParticipantConversation.tsx (3)
169-175: Solid guardrail: effect short-circuits when not recording and waits for settled queryThis prevents spurious state updates and avoids noise during fetches. LGTM.
179-189: Correctly stop recording on 403/404/410 with PII-safe loggingNice. Stopping immediately and flipping
conversationDeletedDuringRecordingaddresses ECHO-428 without leakingresponse.data.
190-194: Transient error path is clean and non-disruptiveContinuing to record while warning with sanitized fields is the right UX.
echo/frontend/src/routes/participant/ParticipantConversation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
echo/frontend/src/routes/participant/ParticipantConversation.tsx (1)
175-177: Use axios.isAxiosError to narrow before reading response.
Avoids unsafe property access if the error isn’t an AxiosError (and keeps TS happy).Apply within this range:
- const error = conversationQuery.error; - const httpStatus = error?.response?.status; + const error = conversationQuery.error; + const httpStatus = axios.isAxiosError(error) + ? error.response?.status + : undefined;Add import (outside this range):
import axios from "axios";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/frontend/src/routes/participant/ParticipantConversation.tsx(2 hunks)
⏰ 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: ci-check-server
🔇 Additional comments (4)
echo/frontend/src/routes/participant/ParticipantConversation.tsx (4)
62-62: Deletion status codes: solid coverage (includes 410). LGTM.
Covers 403/404/410—matches common “gone/forbidden/not found” mid-session cases.
168-174: Effect guardrails are correct.
Early return when not recording and gating on isError while skipping loading/fetching avoids false positives. Clean.
178-188: Stop-on-deletion path is tight.
Sanitized logging, immediate stopRecording, and state flip prevent repeat triggers via the isRecording gate. LGTM.
198-201: Deps list now reacts to new errors.
Including isLoading/isFetching/error prevents stale effect state. Good catch.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
echo/frontend/src/components/participant/hooks/index.ts (1)
329-349: Type the query for end-to-end safety.Let React Query carry
AxiosErrorso you don’t have to annotate the retry param.- return useQuery({ + return useQuery<Awaited<ReturnType<typeof getParticipantConversationById>>, AxiosError>({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/frontend/src/components/participant/hooks/index.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses regular useQuery (not useSuspenseQuery), which means conversationsCountQuery.data can be undefined during loading states. When using Number(conversationsCountQuery.data) ?? 0, this creates NaN because Number(undefined) = NaN and NaN is not nullish, so the fallback doesn't apply. The correct pattern is Number(conversationsCountQuery.data ?? 0) to ensure the fallback happens before type conversion.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses useSuspenseQuery with Directus aggregate, which always returns string numbers like "0", "1", "2" and suspends during loading instead of returning undefined. Therefore, Number(conversationsCountQuery.data) ?? 0 is safe and the Number() conversion is necessary for type conversion from string to number.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
🧬 Code graph analysis (1)
echo/frontend/src/components/participant/hooks/index.ts (1)
echo/frontend/src/lib/api.ts (1)
getParticipantConversationById(40-44)
🔇 Additional comments (2)
echo/frontend/src/components/participant/hooks/index.ts (2)
311-313: Signature cleanup LGTM.Multiline +
string | undefinedwithenabled: !!conversationIdis solid.
361-362: LGTM.Trailing brace/newline churn only.
| import { useMutation, useQueryClient } from "@tanstack/react-query"; | ||
| import { toast } from "@/components/common/Toaster"; | ||
| import { useQuery } from "@tanstack/react-query"; | ||
| import { AxiosError } from "axios"; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Import is fine; inline type on retry can be avoided.
You can drop the inline AxiosError param typing by typing the query generics instead (see suggestion below). Keeps the import, but avoids repeating the type in function params.
🤖 Prompt for AI Agents
In echo/frontend/src/components/participant/hooks/index.ts around line 16, the
review notes that the inline AxiosError parameter typing can be removed by
supplying the appropriate generics to the query/mutation call instead; keep the
existing AxiosError import, remove the explicit AxiosError type annotation from
the callback parameter, and set the query/mutation generic type parameters
(e.g., <Data, AxiosError, Variables>) so the error type is inferred without
repeating it in the function signature.
| queryFn: () => | ||
| getParticipantConversationById(projectId ?? "", conversationId ?? ""), | ||
| enabled: !!conversationId && !!projectId, | ||
| refetchInterval: 60000, | ||
| retry: (failureCount, error: AxiosError) => { | ||
| const status = error?.response?.status; | ||
| // Don't retry if conversation is deleted | ||
| if (status && [404, 403, 410].includes(status as number)) { | ||
| return false; | ||
| } | ||
|
|
||
| return failureCount < 6; | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Narrow status without casts; kill the as number.
Safer type guard; no functional change.
- retry: (failureCount, error: AxiosError) => {
- const status = error?.response?.status;
- // Don't retry if conversation is deleted
- if (status && [404, 403, 410].includes(status as number)) {
+ retry: (failureCount, error: AxiosError) => {
+ const status = error?.response?.status;
+ // Don't retry if conversation is deleted/forbidden/gone
+ if (typeof status === "number" && [404, 403, 410].includes(status)) {
return false;
}
-
return failureCount < 6;
},📝 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.
| queryFn: () => | |
| getParticipantConversationById(projectId ?? "", conversationId ?? ""), | |
| enabled: !!conversationId && !!projectId, | |
| refetchInterval: 60000, | |
| retry: (failureCount, error: AxiosError) => { | |
| const status = error?.response?.status; | |
| // Don't retry if conversation is deleted | |
| if (status && [404, 403, 410].includes(status as number)) { | |
| return false; | |
| } | |
| return failureCount < 6; | |
| }, | |
| queryFn: () => | |
| getParticipantConversationById(projectId ?? "", conversationId ?? ""), | |
| enabled: !!conversationId && !!projectId, | |
| refetchInterval: 60000, | |
| retry: (failureCount, error: AxiosError) => { | |
| const status = error?.response?.status; | |
| // Don't retry if conversation is deleted/forbidden/gone | |
| if (typeof status === "number" && [404, 403, 410].includes(status)) { | |
| return false; | |
| } | |
| return failureCount < 6; | |
| }, |
🤖 Prompt for AI Agents
In echo/frontend/src/components/participant/hooks/index.ts around lines 335 to
347, the retry handler narrows the AxiosError response status using an
unnecessary type cast "as number"; replace that cast with a proper type guard so
you only check numeric statuses (e.g. ensure status is a number before checking
inclusion in [404, 403, 410]). Update the conditional to first verify typeof
status === "number" (or use Number.isInteger/status != null && typeof status ===
"number") and then test the array membership, removing the "as number" cast.
| refetchInterval: 60000, | ||
| retry: (failureCount, error: AxiosError) => { | ||
| const status = error?.response?.status; | ||
| // Don't retry if conversation is deleted | ||
| if (status && [404, 403, 410].includes(status as number)) { | ||
| return false; | ||
| } | ||
|
|
||
| return failureCount < 6; | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Stop polling after 404/403/410 to avoid minute-by-minute error pings.
Disable refetchInterval when the last error is a deletion code.
- refetchInterval: 60000,
+ refetchInterval: (_data, query) => {
+ const err = query.state.error;
+ const status =
+ (typeof (err as any)?.response?.status === "number"
+ ? (err as any).response.status
+ : undefined);
+ return typeof status === "number" && [404, 403, 410].includes(status) ? false : 60000;
+ },If you prefer a type-safe guard, import isAxiosError:
- import { AxiosError } from "axios";
+ import { AxiosError, isAxiosError } from "axios";And then:
- const status =
- (typeof (err as any)?.response?.status === "number"
- ? (err as any).response.status
- : undefined);
+ const status = isAxiosError(err) ? err.response?.status : 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.
| refetchInterval: 60000, | |
| retry: (failureCount, error: AxiosError) => { | |
| const status = error?.response?.status; | |
| // Don't retry if conversation is deleted | |
| if (status && [404, 403, 410].includes(status as number)) { | |
| return false; | |
| } | |
| return failureCount < 6; | |
| }, | |
| import { AxiosError, isAxiosError } from "axios"; | |
| refetchInterval: (_data, query) => { | |
| const err = query.state.error; | |
| const status = isAxiosError(err) ? err.response?.status : undefined; | |
| // Stop polling on deletion/forbidden/gone | |
| return typeof status === "number" && [404, 403, 410].includes(status) | |
| ? false | |
| : 60000; | |
| }, |
| // Don't retry if conversation is deleted | ||
| if (status && [404, 403, 410].includes(status as number)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Deduplicate deletion status codes (now used in ≥2 places).
These codes also live in ParticipantConversation.tsx; centralize to prevent drift.
+import { CONVERSATION_DELETION_STATUS_CODES } from "@/constants/http";
@@
- if (typeof status === "number" && [404, 403, 410].includes(status)) {
+ if (typeof status === "number" && CONVERSATION_DELETION_STATUS_CODES.includes(status)) {
return false;
}Add a tiny constant (zero “architecture” overhead):
// echo/frontend/src/constants/http.ts
export const CONVERSATION_DELETION_STATUS_CODES = [404, 403, 410] as const;🏁 Script executed:
#!/bin/bash
# Evidence of duplication to justify the constant
rg -nC2 'CONVERSATION_DELETION_STATUS_CODES|\\[(404|403|410)\\]' --type=tsLength of output: 237318
Centralize conversation deletion HTTP status codes
Extract [404, 403, 410] into echo/frontend/src/constants/http.ts as
export const CONVERSATION_DELETION_STATUS_CODES = [404, 403, 410] as const;Then in both files…
src/components/participant/hooks/index.ts(around lines 341–344)src/routes/participant/ParticipantConversation.tsx(remove its localCONVERSATION_DELETION_STATUS_CODESdeclaration)
…replace literal checks with
import { CONVERSATION_DELETION_STATUS_CODES } from "@/constants/http";
…
if (typeof status === "number" && CONVERSATION_DELETION_STATUS_CODES.includes(status)) {
return false;
}🤖 Prompt for AI Agents
In echo/frontend/src/components/participant/hooks/index.ts around lines 341–344,
extract the literal array [404, 403, 410] into
echo/frontend/src/constants/http.ts as a named export
CONVERSATION_DELETION_STATUS_CODES, then import that constant here and replace
the current check with a type-safe number check that uses
CONVERSATION_DELETION_STATUS_CODES.includes(status); also remove the local
CONVERSATION_DELETION_STATUS_CODES declaration from
src/routes/participant/ParticipantConversation.tsx and update that file to
import and use the shared constant instead so both places use the centralized
export.
🚨 Bugbot Trial ExpiredYour team's Bugbot trial has expired. Please contact your team administrator to turn on the paid plan to continue using Bugbot. A team admin can activate the plan in the Cursor dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
echo/frontend/src/components/participant/hooks/index.ts (9)
76-83: Snapshot both caches; current context only captures one, so rollback is partial.You optimistic-update two keys but only snapshot one. Capture both to enable full rollback.
- // Snapshot the previous value - const previousChunks = queryClient.getQueryData([ - "conversations", - variables.conversationId, - "chunks", - ]); + // Snapshot the previous values for both caches we mutate + const previousConversationChunks = queryClient.getQueryData< + ConversationChunk[] | undefined + >(["conversations", variables.conversationId, "chunks"]); + const previousParticipantChunks = queryClient.getQueryData< + ConversationChunk[] | undefined + >(["participant", "conversation_chunks", variables.conversationId]); @@ - // Return a context object with the snapshotted value - return { previousChunks }; + // Return both snapshots for rollback + return { previousConversationChunks, previousParticipantChunks };
84-101: Optimistic update drops the new chunk when cache is empty.If
oldDatais undefined you return[], losing the optimistic chunk.- queryClient.setQueryData( + queryClient.setQueryData( ["conversations", variables.conversationId, "chunks"], (oldData: ConversationChunk[] | undefined) => { - return oldData - ? [ - ...oldData, - { - id: "optimistic-" + Date.now(), - conversation_id: variables.conversationId, - created_at: new Date().toISOString(), - timestamp: new Date().toISOString(), - updated_at: new Date().toISOString(), - transcript: undefined, - } as ConversationChunk, - ] - : []; + const optimistic = { + id: "optimistic-" + Date.now(), + conversation_id: variables.conversationId, + created_at: new Date().toISOString(), + timestamp: new Date().toISOString(), + updated_at: new Date().toISOString(), + transcript: undefined, + } as ConversationChunk; + return oldData ? [...oldData, optimistic] : [optimistic]; }, );
103-121: Same optimistic-update bug on the participant key.Mirror the fix so both caches are consistent.
- queryClient.setQueryData( + queryClient.setQueryData( ["participant", "conversation_chunks", variables.conversationId], (oldData: ConversationChunk[] | undefined) => { - return oldData - ? [ - ...oldData, - { - id: "optimistic-" + Date.now(), - conversation_id: variables.conversationId, - created_at: new Date().toISOString(), - timestamp: new Date().toISOString(), - updated_at: new Date().toISOString(), - transcript: undefined, - } as ConversationChunk, - ] - : []; + const optimistic = { + id: "optimistic-" + Date.now(), + conversation_id: variables.conversationId, + created_at: new Date().toISOString(), + timestamp: new Date().toISOString(), + updated_at: new Date().toISOString(), + transcript: undefined, + } as ConversationChunk; + return oldData ? [...oldData, optimistic] : [optimistic]; }, );
127-133: Rollback both caches on error (currently only one is rolled back).Without restoring the participant key, the optimistic ghost chunk persists there.
- onError: (_err, variables, context) => { - queryClient.setQueryData( - ["conversations", variables.conversationId, "chunks"], - context?.previousChunks, - ); - }, + onError: (_err, variables, context) => { + queryClient.setQueryData( + ["conversations", variables.conversationId, "chunks"], + context?.previousConversationChunks, + ); + queryClient.setQueryData( + ["participant", "conversation_chunks", variables.conversationId], + context?.previousParticipantChunks, + ); + },
150-156: Apply the same status-aware retry + backoff to text chunk uploads.Keeps behavior consistent and reduces error storms on deleted conversations.
return useMutation({ mutationFn: uploadConversationText, - retry: 10, + retry: (failureCount, error) => { + const status = isAxiosError(error) ? error.response?.status : undefined; + if (typeof status === "number" && [400, 401, 403, 404, 409, 410, 422].includes(status)) { + return false; + } + return failureCount < 8; + }, + retryDelay: (attempt) => + Math.min(30000, 1000 * 2 ** attempt) + Math.floor(Math.random() * 500),
172-221: Mirror snapshot + optimistic-update fixes for text chunks.Two fixes: snapshot both caches; don’t return [] on empty cache.
- // Snapshot the previous value - const previousChunks = queryClient.getQueryData([ - "conversations", - variables.conversationId, - "chunks", - ]); + // Snapshot both caches + const previousConversationChunks = queryClient.getQueryData< + ConversationChunk[] | undefined + >(["conversations", variables.conversationId, "chunks"]); + const previousParticipantChunks = queryClient.getQueryData< + ConversationChunk[] | undefined + >(["participant", "conversation_chunks", variables.conversationId]); @@ - queryClient.setQueryData( + queryClient.setQueryData( ["conversations", variables.conversationId, "chunks"], (oldData: ConversationChunk[] | undefined) => { - return oldData - ? [ - ...oldData, - { - id: "optimistic-" + Date.now(), - conversation_id: variables.conversationId, - created_at: new Date().toISOString(), - timestamp: new Date().toISOString(), - updated_at: new Date().toISOString(), - transcript: undefined, - } as ConversationChunk, - ] - : []; + const optimistic = { + id: "optimistic-" + Date.now(), + conversation_id: variables.conversationId, + created_at: new Date().toISOString(), + timestamp: new Date().toISOString(), + updated_at: new Date().toISOString(), + transcript: undefined, + } as ConversationChunk; + return oldData ? [...oldData, optimistic] : [optimistic]; }, ); @@ - // Return a context object with the snapshotted value - return { previousChunks }; + // Return both snapshots + return { previousConversationChunks, previousParticipantChunks };
223-229: Rollback both caches on text upload error.Parity with binary chunk path.
- onError: (_err, variables, context) => { - queryClient.setQueryData( - ["conversations", variables.conversationId, "chunks"], - context?.previousChunks, - ); - }, + onError: (_err, variables, context) => { + queryClient.setQueryData( + ["conversations", variables.conversationId, "chunks"], + context?.previousConversationChunks, + ); + queryClient.setQueryData( + ["participant", "conversation_chunks", variables.conversationId], + context?.previousParticipantChunks, + ); + },
351-361: Do the same “stop polling after deletion” for chunks query.Prevents background error spam when a conversation is gone.
return useQuery({ queryKey: ["participant", "conversation_chunks", conversationId], queryFn: () => getParticipantConversationChunks(projectId ?? "", conversationId ?? ""), - refetchInterval: 60000, + refetchInterval: (_data, query) => { + const err = query.state.error; + const status = isAxiosError(err) ? err.response?.status : undefined; + return typeof status === "number" && [404, 403, 410].includes(status) ? false : 60000; + }, });
134-147: Add conversation chunks invalidation under onSettled
AddqueryClient.invalidateQueries({ queryKey: ["conversations", variables.conversationId, "chunks"], });to the
onSettledcallback so the same["conversations", id, "chunks"]queries (used throughout participant/hooks/index.ts) are properly refreshed.
♻️ Duplicate comments (3)
echo/frontend/src/components/participant/hooks/index.ts (3)
16-16: Prefer query generics over inlineAxiosError; also importisAxiosErrorfor safe guards.Inline typing on the retry callback is unnecessary if you type the query. Add
isAxiosErrorfor status guards used below.-import { AxiosError } from "axios"; +import { AxiosError, isAxiosError } from "axios";
335-347: Narrow status without casts; type the query to drop inlineAxiosError.Use a type guard and let generics flow
AxiosErrorinto the callback. Same suggestion was previously discussed.- return useQuery({ + return useQuery<Awaited<ReturnType<typeof getParticipantConversationById>>, AxiosError>({ @@ - retry: (failureCount, error: AxiosError) => { - const status = error?.response?.status; - // Don't retry if conversation is deleted - if (status && [404, 403, 410].includes(status as number)) { + retry: (failureCount, error) => { + const status = isAxiosError(error) ? error.response?.status : undefined; + // Don't retry if conversation is deleted/forbidden/gone + if (typeof status === "number" && [404, 403, 410].includes(status)) { return false; } return failureCount < 6; },
338-343: Stop polling after 404/403/410 to avoid minute-by-minute error pings.Disable
refetchIntervalwhen last error indicates deletion/gone/forbidden.- refetchInterval: 60000, + refetchInterval: (_data, query) => { + const err = query.state.error; + const status = isAxiosError(err) ? err.response?.status : undefined; + return typeof status === "number" && [404, 403, 410].includes(status) ? false : 60000; + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/frontend/src/components/participant/hooks/index.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses regular useQuery (not useSuspenseQuery), which means conversationsCountQuery.data can be undefined during loading states. When using Number(conversationsCountQuery.data) ?? 0, this creates NaN because Number(undefined) = NaN and NaN is not nullish, so the fallback doesn't apply. The correct pattern is Number(conversationsCountQuery.data ?? 0) to ensure the fallback happens before type conversion.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
📚 Learning: 2025-08-19T10:22:55.323Z
Learnt from: ussaama
PR: Dembrane/echo#266
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:675-678
Timestamp: 2025-08-19T10:22:55.323Z
Learning: In echo/frontend/src/components/conversation/hooks/index.ts, the useConversationsCountByProjectId hook uses useSuspenseQuery with Directus aggregate, which always returns string numbers like "0", "1", "2" and suspends during loading instead of returning undefined. Therefore, Number(conversationsCountQuery.data) ?? 0 is safe and the Number() conversion is necessary for type conversion from string to number.
Applied to files:
echo/frontend/src/components/participant/hooks/index.ts
🧬 Code graph analysis (1)
echo/frontend/src/components/participant/hooks/index.ts (1)
echo/frontend/src/lib/api.ts (1)
getParticipantConversationById(40-44)
⏰ 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). (2)
- GitHub Check: ci-build-servers (dbr-echo-server, ./echo/server, Dockerfile, dbr-echo-server)
- GitHub Check: ci-build-servers (dbr-echo-directus, ./echo/directus, Dockerfile, dbr-echo-directus)
🔇 Additional comments (1)
echo/frontend/src/components/participant/hooks/index.ts (1)
311-313: Signature reflow LGTM.Style-only; no behavior change.
| retry: 20, | ||
| // When mutate is called: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Blindly bumping retries to 20 will DDoS your own API under load; make retries status-aware with exponential backoff + jitter.
Aligns with ECHO-428 (contention during concurrent recordings). Gate on non-transient 4xx and back off.
return useMutation({
mutationFn: uploadConversationChunk,
- retry: 20,
+ retry: (failureCount, error) => {
+ const status = isAxiosError(error) ? error.response?.status : undefined;
+ // Don't retry on non-transient client errors
+ if (typeof status === "number" && [400, 401, 403, 404, 409, 410, 422].includes(status)) {
+ return false;
+ }
+ return failureCount < 8;
+ },
+ retryDelay: (attempt) =>
+ Math.min(30000, 1000 * 2 ** attempt) + Math.floor(Math.random() * 500),📝 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.
| retry: 20, | |
| // When mutate is called: | |
| return useMutation({ | |
| mutationFn: uploadConversationChunk, | |
| retry: (failureCount, error) => { | |
| const status = isAxiosError(error) ? error.response?.status : undefined; | |
| // Don't retry on non-transient client errors | |
| if ( | |
| typeof status === "number" && | |
| [400, 401, 403, 404, 409, 410, 422].includes(status) | |
| ) { | |
| return false; | |
| } | |
| // Retry up to 8 times for transient failures | |
| return failureCount < 8; | |
| }, | |
| retryDelay: (attempt) => | |
| Math.min(30000, 1000 * 2 ** attempt) + Math.floor(Math.random() * 500), | |
| // When mutate is called: | |
| … | |
| }); |
🤖 Prompt for AI Agents
In echo/frontend/src/components/participant/hooks/index.ts around lines 59-60,
the retry count is hard-coded to 20 which can DDoS our API; change retry logic
to be status-aware and use exponential backoff with jitter: detect non-transient
4xx responses and do not retry them, retry transient errors (5xx, network
timeouts) with an increasing delay (baseDelay * 2^attempt) plus random jitter,
and cap the max attempts and max delay; implement this in the mutate/retry
handler so the backoff calculation and status checks determine whether to retry
rather than a fixed retry count.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Recording now stops automatically when a conversation is deleted or access is revoked, preventing stuck or misleading states. - Improved error handling to distinguish permanent errors (stop recording) from transient ones (continue recording). - Reduced unnecessary retry attempts for permanently missing/unavailable conversations; transient errors still retry with limited attempts. - Increased stability during ongoing recordings by continuing through safe, transient fetch issues. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit