Yuliusupwork/myc 4513 fix unauthorized access in get apimemoriesget enforce room#1624
Conversation
…ation - Added header validation and room ownership checks to the GET /api/memories/get endpoint. - Implemented error handling for missing room ID, unauthorized access, room not found, and forbidden access. - Created unit tests for various scenarios including authentication, room existence, and memory retrieval success. - Updated useMessageLoader and related hooks to include access token for fetching messages.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughUpdated JSDoc documentation in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hooks/useMessageLoader.ts (2)
23-62:⚠️ Potential issue | 🟠 MajorInclude
setMessagesin the dependency array.The
setMessagescallback is used inside the effect but missing from the dependency array. This violates React's exhaustive-deps rule and could cause stale closures if the callback reference changes.🔧 Proposed fix
loadMessages(); - }, [userId, roomId, accessToken, apiOverride]); + }, [userId, roomId, accessToken, apiOverride, setMessages]);As per coding guidelines: "For custom hooks, ensure: Use proper dependency arrays" - all values referenced inside the effect must be included.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useMessageLoader.ts` around lines 23 - 62, The useEffect inside useMessageLoader references the setMessages callback but doesn't include it in the dependency array; update the effect's dependency array to include setMessages so the effect re-runs if that callback changes (the effect that defines loadMessages and calls getChatMessages should list setMessages along with userId, roomId, accessToken, and apiOverride).
49-49:⚠️ Potential issue | 🟠 MajorAdd explicit return type to
getChatMessagesand remove the type assertion.The
getChatMessagesfunction lacks an explicit return type annotation, and the castas UIMessage[]bypasses type checking. The returned data structure{ id, role, content }may not fully align with theUIMessageinterface from the "ai" package (which may require additional fields or constrained role values like'user' | 'assistant').Add a return type to
getChatMessagesand ensure the mapped data matchesUIMessagerequirements, or create a proper type-safe transformation function rather than relying on a cast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useMessageLoader.ts` at line 49, The getChatMessages function must declare an explicit return type (e.g., UIMessage[] or a local mapped type) and stop using the unsafe cast at the setMessages call; update getChatMessages to return a properly typed array by mapping each source object to the UIMessage shape required by the "ai" package (ensuring role is constrained to 'user' | 'assistant', include any required fields like id/content/role), then call setMessages with that typed return value instead of using "as UIMessage[]"; adjust or add a dedicated transformer function if needed to guarantee the mapped objects conform to UIMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useMessageLoader.ts`:
- Around line 5-12: Add a `@returns` block to the JSDoc for the useMessageLoader
hook that documents the returned object shape: include descriptions for
isLoading (boolean indicating loading state), error (Error | null for any fetch
error), and hasError (boolean derived from error) so consumers understand the
hook's consistent interface; update the JSDoc above the useMessageLoader
function to explicitly list each property and its type/meaning.
---
Outside diff comments:
In `@hooks/useMessageLoader.ts`:
- Around line 23-62: The useEffect inside useMessageLoader references the
setMessages callback but doesn't include it in the dependency array; update the
effect's dependency array to include setMessages so the effect re-runs if that
callback changes (the effect that defines loadMessages and calls getChatMessages
should list setMessages along with userId, roomId, accessToken, and
apiOverride).
- Line 49: The getChatMessages function must declare an explicit return type
(e.g., UIMessage[] or a local mapped type) and stop using the unsafe cast at the
setMessages call; update getChatMessages to return a properly typed array by
mapping each source object to the UIMessage shape required by the "ai" package
(ensuring role is constrained to 'user' | 'assistant', include any required
fields like id/content/role), then call setMessages with that typed return value
instead of using "as UIMessage[]"; adjust or add a dedicated transformer
function if needed to guarantee the mapped objects conform to UIMessage.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 668dc66e-a328-4919-87c2-6aa8b4c33011
📒 Files selected for processing (1)
hooks/useMessageLoader.ts
| /** | ||
| * Hook for loading existing messages from a room | ||
| * Hook for loading existing messages from a room via the chats API (Bearer auth). | ||
| * @param roomId - The room ID to load messages from (undefined to skip loading) | ||
| * @param userId - The current user ID (messages won't load if user is not authenticated) | ||
| * @param accessToken - Privy access token for the API request | ||
| * @param apiOverride - Optional API base URL override (e.g. preview) | ||
| * @param setMessages - Callback function to set the loaded messages | ||
| * @returns Loading state and error information | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add @returns documentation to complete the JSDoc.
The JSDoc is missing documentation for the return value. The hook returns an object with isLoading, error, and hasError properties that consumers need to understand.
📝 Proposed documentation addition
/**
* Hook for loading existing messages from a room via the chats API (Bearer auth).
* `@param` roomId - The room ID to load messages from (undefined to skip loading)
* `@param` userId - The current user ID (messages won't load if user is not authenticated)
* `@param` accessToken - Privy access token for the API request
* `@param` apiOverride - Optional API base URL override (e.g. preview)
* `@param` setMessages - Callback function to set the loaded messages
+ * `@returns` Object containing loading state, error state, and error flag
*/As per coding guidelines: "For custom hooks, ensure: Return consistent interface" - documenting the return value helps maintain clarity about the hook's consistent interface.
📝 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.
| /** | |
| * Hook for loading existing messages from a room | |
| * Hook for loading existing messages from a room via the chats API (Bearer auth). | |
| * @param roomId - The room ID to load messages from (undefined to skip loading) | |
| * @param userId - The current user ID (messages won't load if user is not authenticated) | |
| * @param accessToken - Privy access token for the API request | |
| * @param apiOverride - Optional API base URL override (e.g. preview) | |
| * @param setMessages - Callback function to set the loaded messages | |
| * @returns Loading state and error information | |
| */ | |
| /** | |
| * Hook for loading existing messages from a room via the chats API (Bearer auth). | |
| * `@param` roomId - The room ID to load messages from (undefined to skip loading) | |
| * `@param` userId - The current user ID (messages won't load if user is not authenticated) | |
| * `@param` accessToken - Privy access token for the API request | |
| * `@param` apiOverride - Optional API base URL override (e.g. preview) | |
| * `@param` setMessages - Callback function to set the loaded messages | |
| * `@returns` Object containing loading state, error state, and error flag | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useMessageLoader.ts` around lines 5 - 12, Add a `@returns` block to the
JSDoc for the useMessageLoader hook that documents the returned object shape:
include descriptions for isLoading (boolean indicating loading state), error
(Error | null for any fetch error), and hasError (boolean derived from error) so
consumers understand the hook's consistent interface; update the JSDoc above the
useMessageLoader function to explicitly list each property and its type/meaning.
1 participant
Summary
testinto this branch and aligns message loading with the current architecture: the app usesgetChatMessages→GET {API}/api/chats/{roomId}/messageswithAuthorization: Bearer(Privy access token)./api/memories/getis no longer used ontest; authentication and room/account access for message history are enforced on the API (validateChatAccessonGET /api/chats/[id]/messagesinrecoupable/api).What changed (chat)
hooks/useMessageLoader.ts— loads viagetChatMessages, passesaccessToken+ optionalapiOverride.hooks/useVercelChat.ts— wiresuseMessageLoaderwith the correct arguments after the merge.Supersedes / related
recoupable/chatdirectly, per team workflow).