fix: phase 1#476
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR consolidates chat runtime lifecycle management from a separate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Made-with: Cursor
755eaf3 to
eee794f
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 (1)
src/components/chat/ThreadListDropdown.tsx (1)
68-74: Consider memoizingvisibleThreadsto avoid repeat sort/filter on every render.This is likely fine at small scale, but wrapping this computation in
useMemoavoids unnecessary work during frequent UI re-renders.Possible refactor
import { type FC, useCallback, useEffect, + useMemo, useRef, useState, } from "react"; @@ - const visibleThreads = orderThreadsByRuntimeActivity( - (threads ?? []).filter((thread) => thread.status !== "archived"), - { - getThreadStatus: getThreadStatusSnapshot, - getThreadLastStartedAt, - }, - ); + const visibleThreads = useMemo( + () => + orderThreadsByRuntimeActivity( + (threads ?? []).filter((thread) => thread.status !== "archived"), + { + getThreadStatus: getThreadStatusSnapshot, + getThreadLastStartedAt, + }, + ), + [threads, getThreadStatusSnapshot, getThreadLastStartedAt], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/ThreadListDropdown.tsx` around lines 68 - 74, visibleThreads is being recomputed on every render via orderThreadsByRuntimeActivity(...). Wrap that computation in React.useMemo so the filter/sort only runs when its inputs change: memoize the call that uses threads, getThreadStatusSnapshot, and getThreadLastStartedAt (and any other dependent values) so visibleThreads only recalculates when those dependencies change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/chat/ChatProvider.tsx`:
- Around line 52-69: getStatusSubscription currently relies on the internal
Chat["~registerStatusCallback"] API which is unstable; replace this by wiring
into the documented useChat hook outputs: stop referencing
"~registerStatusCallback" in getStatusSubscription and instead accept/consume
the stable `status` field from useChat (or the hook's returned object) and
register lifecycle callbacks via the documented options such as `onFinish`,
`onError`, etc.; concretely, update getStatusSubscription/getter call sites to
take the hook's `status` (or a documented subscriber API) and return a proper
unsubscribe function created from the documented callbacks rather than binding
to "~registerStatusCallback", and update any places using getStatusSubscription
to provide the useChat `status` or pass through the hook's onFinish/onError
handlers.
- Around line 351-360: The current catch in the opts.hydrate block (inside
ChatProvider, around the initial variable assignment) swallows all errors and
forces initial = [], hiding transient/server failures; since fetchThreadMessages
already returns [] for 404, change the catch to log the error and rethrow (or
propagate it) instead of setting initial = [], so only genuine "not found"
becomes empty while other errors surface to the caller; update the try/catch
around fetchThreadMessages(threadId) (referencing opts.hydrate,
fetchThreadMessages, and initial) accordingly.
In `@src/lib/ai/supermemory-workspace-migration.ts`:
- Around line 44-49: The check for empty customId should trim whitespace first
so whitespace-only values are treated as missing: in the function that uses
customId (the block creating trimmed and calling isThreadUuidCustomId), move or
add the trim earlier so you evaluate trimmed === "" or trimmed == null before
calling isThreadUuidCustomId; return { status: "unresolved", reason:
"no_custom_id" } for null/undefined/blank (trimmed === "") and only call
isThreadUuidCustomId(trimmed) afterwards to decide on { status: "unresolved",
reason: "non_thread_custom_id" }.
---
Nitpick comments:
In `@src/components/chat/ThreadListDropdown.tsx`:
- Around line 68-74: visibleThreads is being recomputed on every render via
orderThreadsByRuntimeActivity(...). Wrap that computation in React.useMemo so
the filter/sort only runs when its inputs change: memoize the call that uses
threads, getThreadStatusSnapshot, and getThreadLastStartedAt (and any other
dependent values) so visibleThreads only recalculates when those dependencies
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee917791-edad-4642-9258-14f6a398298c
📒 Files selected for processing (13)
src/components/chat/ChatProvider.tsxsrc/components/chat/ChatRuntimes.tsxsrc/components/chat/Messages.tsxsrc/components/chat/ThreadBody.tsxsrc/components/chat/ThreadListDropdown.tsxsrc/components/chat/use-thread-viewport-controller.tssrc/components/layout/DashboardLayout.tsxsrc/lib/ai/__tests__/supermemory-workspace-migration.test.tssrc/lib/ai/supermemory-workspace-migration.tssrc/lib/chat/__tests__/thread-order.test.tssrc/lib/chat/__tests__/thread-viewport-policy.test.tssrc/lib/chat/thread-order.tssrc/lib/chat/thread-runtime-state.ts
💤 Files with no reviewable changes (1)
- src/components/chat/ChatRuntimes.tsx
| function getStatusSubscription(chat: Chat<ChatMessage>) { | ||
| const register = ( | ||
| chat as Chat<ChatMessage> & { | ||
| "~registerStatusCallback"?: (onChange: () => void) => () => void; | ||
| } | ||
| )["~registerStatusCallback"]; | ||
|
|
||
| if (typeof register !== "function") { | ||
| if (process.env.NODE_ENV !== "production") { | ||
| console.warn( | ||
| "[ChatProvider] Chat['~registerStatusCallback'] is unavailable. Per-thread status subscriptions are disabled.", | ||
| ); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| return register.bind(chat); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For @ai-sdk/reactversion 3.0.170, isChat["~registerStatusCallback"] a documented/stable public API? If not, what is the supported way to subscribe to chat status changes?
💡 Result:
Not stable/public. In @ai-sdk/react’s public documentation for useChat, the documented supported callback/observable surface is the hook’s returned status plus documented callbacks like onToolCall, onData, onFinish, and onError (and related options). The symbol Chat["registerStatusCallback"] is an internal “” method used by the hook to wire updates via React’s useSyncExternalStore, not a documented/stable public API. Supported way to subscribe to chat status changes: - Use the status value returned by useChat and react to it in your component (e.g., in a useEffect). Example: const { status } = useChat(...); useEffect( => { // status changes: e.g. "submitted" | "streaming" | "ready" | "error" // ...handle it }, [status]); Notes: - The useChat hook’s implementation internally calls chatRef.current['~registerStatusCallback'] to support the status state, which indicates that ~registerStatusCallback is intentionally private/internal. - For “push-style” reactions during the lifecycle, prefer documented useChat callbacks such as onFinish and onError.
Citations:
- 1: https://ai-sdk.dev/docs/reference/ai-sdk-ui/use-chat
- 2: https://ai-sdk.dev/v4/docs/reference/ai-sdk-ui/use-chat
- 3: https://github.com/vercel/ai/blob/main/packages/react/src/use-chat.ts
- 4: https://ai-sdk.dev/v5/docs/ai-sdk-ui/chatbot
Refactor status subscription to use documented Chat API instead of internal ~registerStatusCallback.
The code relies on Chat["~registerStatusCallback"], which is an internal (non-public) API. While the code handles its absence gracefully, if this internal method is removed or renamed in future @ai-sdk/react versions, per-thread status subscriptions will silently no-op, breaking reactivity and thread-order updates.
Instead, use the documented status field returned by useChat hook and the stable callback options (onFinish, onError, etc.) provided by the hook to subscribe to chat state changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/ChatProvider.tsx` around lines 52 - 69,
getStatusSubscription currently relies on the internal
Chat["~registerStatusCallback"] API which is unstable; replace this by wiring
into the documented useChat hook outputs: stop referencing
"~registerStatusCallback" in getStatusSubscription and instead accept/consume
the stable `status` field from useChat (or the hook's returned object) and
register lifecycle callbacks via the documented options such as `onFinish`,
`onError`, etc.; concretely, update getStatusSubscription/getter call sites to
take the hook's `status` (or a documented subscriber API) and return a proper
unsubscribe function created from the documented callbacks rather than binding
to "~registerStatusCallback", and update any places using getStatusSubscription
to provide the useChat `status` or pass through the hook's onFinish/onError
handlers.
| if (opts.hydrate) { | ||
| try { | ||
| initial = await fetchThreadMessages(threadId); | ||
| } catch (error) { | ||
| console.warn("[ChatProvider] failed to hydrate thread", { | ||
| threadId, | ||
| error, | ||
| }); | ||
| initial = []; | ||
| } |
There was a problem hiding this comment.
Don’t silently turn hydration failures into empty thread history.
This catch path converts any load failure into [], so an existing thread can appear blank on transient/server errors. fetchThreadMessages already maps 404 to empty; other errors should be surfaced explicitly.
Suggested fix
- if (opts.hydrate) {
- try {
- initial = await fetchThreadMessages(threadId);
- } catch (error) {
- console.warn("[ChatProvider] failed to hydrate thread", {
- threadId,
- error,
- });
- initial = [];
- }
- }
+ if (opts.hydrate) {
+ initial = await fetchThreadMessages(threadId);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/ChatProvider.tsx` around lines 351 - 360, The current
catch in the opts.hydrate block (inside ChatProvider, around the initial
variable assignment) swallows all errors and forces initial = [], hiding
transient/server failures; since fetchThreadMessages already returns [] for 404,
change the catch to log the error and rethrow (or propagate it) instead of
setting initial = [], so only genuine "not found" becomes empty while other
errors surface to the caller; update the try/catch around
fetchThreadMessages(threadId) (referencing opts.hydrate, fetchThreadMessages,
and initial) accordingly.
| if (customId == null || customId === "") { | ||
| return { status: "unresolved", reason: "no_custom_id" }; | ||
| } | ||
| const trimmed = customId.trim(); | ||
| if (!isThreadUuidCustomId(trimmed)) { | ||
| return { status: "unresolved", reason: "non_thread_custom_id" }; |
There was a problem hiding this comment.
Treat blank customId values as missing.
The null/empty check happens before trimming, so whitespace-only values fall through to non_thread_custom_id. If blank IDs can appear from Supermemory, this should return no_custom_id instead.
Suggested fix
export function routeDocumentToWorkspaceTag(args: {
customId: string | null | undefined;
userId: string;
/** Map chat_threads.id → workspace_id for this user */
threadToWorkspaceId: Map<string, string>;
}): RoutingOutcome {
const { customId, userId, threadToWorkspaceId } = args;
- if (customId == null || customId === "") {
+ const trimmed = customId?.trim();
+ if (!trimmed) {
return { status: "unresolved", reason: "no_custom_id" };
}
- const trimmed = customId.trim();
if (!isThreadUuidCustomId(trimmed)) {
return { status: "unresolved", reason: "non_thread_custom_id" };
}📝 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.
| if (customId == null || customId === "") { | |
| return { status: "unresolved", reason: "no_custom_id" }; | |
| } | |
| const trimmed = customId.trim(); | |
| if (!isThreadUuidCustomId(trimmed)) { | |
| return { status: "unresolved", reason: "non_thread_custom_id" }; | |
| const trimmed = customId?.trim(); | |
| if (!trimmed) { | |
| return { status: "unresolved", reason: "no_custom_id" }; | |
| } | |
| if (!isThreadUuidCustomId(trimmed)) { | |
| return { status: "unresolved", reason: "non_thread_custom_id" }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/ai/supermemory-workspace-migration.ts` around lines 44 - 49, The
check for empty customId should trim whitespace first so whitespace-only values
are treated as missing: in the function that uses customId (the block creating
trimmed and calling isThreadUuidCustomId), move or add the trim earlier so you
evaluate trimmed === "" or trimmed == null before calling isThreadUuidCustomId;
return { status: "unresolved", reason: "no_custom_id" } for null/undefined/blank
(trimmed === "") and only call isThreadUuidCustomId(trimmed) afterwards to
decide on { status: "unresolved", reason: "non_thread_custom_id" }.
Greptile SummaryThis PR consolidates Confidence Score: 4/5Safe to merge; all findings are P2 style/hardening suggestions with no present runtime breakage. No P0/P1 bugs found. Three P2 issues: a missing character-set guard in the Supermemory tag builder, an O(n) lookup in useThreadStatus that runs per thread row, and a narrow RAF cleanup race in useThreadViewportController. All are low-risk in practice. src/components/chat/use-thread-viewport-controller.ts (RAF cleanup race), src/lib/ai/supermemory-workspace-migration.ts (character-set validation) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -64,76 +195,315 @@ function generateThreadId(): string { | |||
| } | |||
| return `thread-${Math.random().toString(36).slice(2)}-${Date.now()}`; | |||
| } | |||
| /** | |||
| * Owns the *visible* threadId selection for a workspace and binds the | |||
| * current Chat runtime to the rest of the chat surface. | |||
| * | |||
| * Concurrent generation across threads is achieved by storing one `Chat` | |||
| * instance per threadId in `<ChatRuntimesProvider>`'s in-memory registry. | |||
| * Switching threads simply rebinds via `useChat({ chat })`; previous | |||
| * runtimes keep streaming in the background. | |||
| */ | |||
|
|
|||
| export function ChatProvider({ workspaceId, children }: ChatProviderProps) { | |||
| const queryClient = useQueryClient(); | |||
|
|
|||
| const persistedThreadId = useWorkspaceStore( | |||
| selectCurrentThreadId(workspaceId), | |||
| ); | |||
| const setCurrentThreadId = useWorkspaceStore( | |||
There was a problem hiding this comment.
includes inside useThreadStatus — called per thread row
aliveThreadIds.includes(threadId) is O(n) and runs on every render for each ThreadListItemRow in ThreadListDropdown. For workspaces with many threads this adds up to O(n²) linear scans per render cycle. Storing alive thread IDs in a Set (or deriving a Set from the array in the same useState) would make this lookup O(1).
// e.g. expose an aliveThreadIdSet derived from the same state:
const chat =
threadId && aliveThreadIdSet.has(threadId)
? getRuntime(threadId)
: undefined;| let innerFrameId: number | null = null; | ||
| const outerFrameId = requestAnimationFrame(() => { | ||
| if (previousStatus == null) { | ||
| innerFrameId = requestAnimationFrame(runScroll); | ||
| return; | ||
| } | ||
| runScroll(); | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelAnimationFrame(outerFrameId); | ||
| if (innerFrameId !== null) { | ||
| cancelAnimationFrame(innerFrameId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
innerFrameId is assigned inside the outer RAF callback. If React re-runs this effect (e.g. status or viewportHeight changes) between the two frames, the cleanup runs, cancelAnimationFrame(outerFrameId) is a no-op (the outer RAF already fired), and innerFrameId is still null in the cleanup closure — so the inner RAF is not cancelled. The inner RAF then calls runScroll() against a stale list snapshot from the previous effect invocation.
In practice this race window is tiny (~16 ms), but on slow renders or during stress-testing it can fire spurious scrolls. A reliable fix is a cancelled flag:
let cancelled = false;
const outerFrameId = requestAnimationFrame(() => {
if (cancelled) return;
if (previousStatus == null) {
requestAnimationFrame(() => { if (!cancelled) runScroll(); });
return;
}
runScroll();
});
return () => {
cancelled = true;
cancelAnimationFrame(outerFrameId);
};
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| ? threadId | ||
| : `user:${userId}`; | ||
|
|
||
| const containerTag = `${userId}_${workspaceId}`; |
There was a problem hiding this comment.
[🟡 Medium] [🟡 Investigate]
The containerTag passed to withSupermemory changed from userId to ${userId}_${workspaceId}. Since Supermemory indexes stored memories by containerTag, all memories previously stored under the old userId-only tag will no longer be retrievable — the new workspace-scoped tag creates a fresh, empty namespace for every existing user.
// src/lib/ai/supermemory.ts
const containerTag = `${userId}_${workspaceId}`;If production users already have stored memory content, this change silently orphans it. Verify whether a migration step or backward-compatible fallback (e.g. querying both old and new tags) is needed, or confirm that orphaning old data is acceptable.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements