Agent context features and collection mutation cleanup#455
Conversation
Allow each builtin agent to declare which context features it needs (e.g., integrations) so the beforeContext hook can skip unnecessary queries for agents that don't use them.
…nation Merge duplicate beforeGenerateHook implementations into shared lib/agent_chat module and remove the redundant onError hook. Add a tools summary section to the context window with token estimation for system prompt and tools. Fix thread listing to paginate through all pages instead of only the first 100, and sort threads client-side by creation time. Switch customer delete from collection API to direct mutation.
Add AssistantMessageInfo component to test chat panel so users can inspect message metadata. Replace reset icon with Trash2 and implement proper chat reset by unmounting/remounting the panel. Style h3 headings in context window content for better readability.
…tations for custom agents Simplify useUpdateCustomAgent and useUpdateCustomAgentMetadata hooks by removing the Collection dependency and using useConvexMutation directly. This removes the need for useCustomAgentCollection across all agent config tabs (general, instructions, tools, knowledge).
📝 WalkthroughWalkthroughThis pull request implements a comprehensive refactor converting collection-based mutation hooks to Convex mutation-based hooks across multiple features. The changes remove Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform/app/features/custom-agents/components/test-chat-panel.tsx (1)
27-40: 🧹 Nitpick | 🔵 TrivialAvoid eager per‑message metadata fetch.
useMessageMetadataruns for every assistant message even when the dialog is closed. This can generate N+1 queries and slow the panel for long threads. Consider skipping the query until the dialog is opened.⚡ Suggested lazy fetch
- const { metadata } = useMessageMetadata(messageId); + const { metadata } = useMessageMetadata(isOpen ? messageId : null);Also applies to: 114-142, 711-717
🤖 Fix all issues with AI agents
In `@services/platform/app/features/chat/hooks/queries.ts`:
- Around line 22-29: The useThreads hook currently uses React's useMemo (sorted
= useMemo(...)) which forces tests to call hooks via a dispatcher; remove
useMemo to allow calling useThreads directly in tests: replace the useMemo-based
computed value with a plain inline computation (e.g., compute sorted by copying
and sorting data with the same comparator) inside the useThreads implementation
and return that value as threads; ensure you remove the useMemo import/usage and
keep the sorting logic (b._creationTime - a._creationTime) associated with the
sorted variable so the hook remains pure and testable without renderHook.
In `@services/platform/app/features/custom-agents/components/test-chat-panel.tsx`:
- Around line 144-156: The onReset prop is optional but handleClearChat
currently relies on it, leaving threadId and messages stale if omitted; update
TestChatPanelContent to either make onReset required in TestChatPanelProps or
(preferred) add a local fallback reset inside handleClearChat: after the delete
thread call, if onReset is provided call it, otherwise locally reset the panel
state (clear threadId via setThreadId(null or ''), clear messages via
setMessages([]), reset input/state flags like setIsStreaming(false)/setInput('')
as needed) so the UI fully resets; reference TestChatPanelProps,
TestChatPanelContent and the handleClearChat function when making this change.
In `@services/platform/app/features/vendors/hooks/mutations.ts`:
- Around line 4-16: The three hooks return inconsistent shapes:
useBulkCreateVendors returns the full mutation object while useDeleteVendor and
useUpdateVendor return only mutateAsync; to fix, make the return shapes
consistent—either return the full mutation object from useDeleteVendor and
useUpdateVendor (by returning the result of useConvexMutation in useDeleteVendor
and useUpdateVendor) or return only mutateAsync from useBulkCreateVendors;
update useDeleteVendor and useUpdateVendor (or useBulkCreateVendors) so all
hooks (useBulkCreateVendors, useDeleteVendor, useUpdateVendor) expose the same
API and add a short comment if a special-case is intentional.
| const sorted = useMemo( | ||
| () => data?.slice().sort((a, b) => b._creationTime - a._creationTime), | ||
| [data], | ||
| ); | ||
|
|
||
| return { | ||
| threads: data, | ||
| threads: sorted, | ||
| isLoading, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file mentioned in the review
find . -path "*services/platform/app/features/chat/hooks/queries.ts" -o -path "*features/chat/hooks/queries.ts" | head -20Repository: tale-project/tale
Length of output: 115
🏁 Script executed:
# Also check if the file exists with different path patterns
fd "queries.ts" -t f | grep -i chat | head -20Repository: tale-project/tale
Length of output: 113
🏁 Script executed:
# List all test files related to chat hooks
fd -t f "\.test\.|\.spec\." | grep -i "chat.*hook" | head -20Repository: tale-project/tale
Length of output: 209
🏁 Script executed:
# Search for useThreads hook usage patterns
rg "useThreads" -t ts -t tsx --max-count 50Repository: tale-project/tale
Length of output: 88
🏁 Script executed:
# Read the queries.ts file to see the actual code
cat -n services/platform/app/features/chat/hooks/queries.ts | head -50Repository: tale-project/tale
Length of output: 2203
🏁 Script executed:
# Search for useThreads in test files with proper context
rg "useThreads" -t ts -C 5 services/platform/app/features/chat/hooks/__tests__/Repository: tale-project/tale
Length of output: 3728
🏁 Script executed:
# Check if renderHook is being used in the test files
rg "renderHook|render" -t ts services/platform/app/features/chat/hooks/__tests__/ | head -20Repository: tale-project/tale
Length of output: 43
🏁 Script executed:
# Check the full test file to see test setup and imports
cat -n services/platform/app/features/chat/hooks/__tests__/collection-hooks.test.ts | head -80Repository: tale-project/tale
Length of output: 1621
🏁 Script executed:
# Search for renderHook usage in the codebase
rg "renderHook" -t ts --max-count 20Repository: tale-project/tale
Length of output: 1134
🏁 Script executed:
# Check what testing libraries are available
rg "from.*testing|import.*testing" services/platform/app/features/chat/hooks/__tests__/ -t tsRepository: tale-project/tale
Length of output: 43
Update test to use renderHook for useThreads hook or remove useMemo to avoid dispatcher error.
The test calls useThreads directly (lines 29, 41 in collection-hooks.test.ts) but useThreads is a hook containing useMemo. When hooks are invoked outside React's dispatcher context, useMemo throws Cannot read properties of null (reading 'useMemo').
Either wrap the hook call with renderHook from @testing-library/react (which is available in the project), or remove useMemo and compute the sorted array inline. The latter approach is simpler and avoids unnecessary hook overhead in this case.
🔧 Suggested code change (remove `useMemo` to keep tests hook‑free)
- const sorted = useMemo(
- () => data?.slice().sort((a, b) => b._creationTime - a._creationTime),
- [data],
- );
-
return {
- threads: sorted,
+ threads: data?.slice().sort((a, b) => b._creationTime - a._creationTime),
isLoading,
};📝 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.
| const sorted = useMemo( | |
| () => data?.slice().sort((a, b) => b._creationTime - a._creationTime), | |
| [data], | |
| ); | |
| return { | |
| threads: data, | |
| threads: sorted, | |
| isLoading, | |
| return { | |
| threads: data?.slice().sort((a, b) => b._creationTime - a._creationTime), | |
| isLoading, |
🧰 Tools
🪛 GitHub Actions: Test
[error] 22-22: Cannot read properties of null (reading 'useMemo') in useMemo call while processing useThreads data.
🪛 GitHub Check: Test
[failure] 22-22: app/features/chat/hooks/tests/collection-hooks.test.ts > useThreads > returns data even while loading
TypeError: Cannot read properties of null (reading 'useMemo')
❯ process.env.NODE_ENV.exports.useMemo ../../node_modules/react/cjs/react.development.js:1251:33
❯ Module.useThreads app/features/chat/hooks/queries.ts:22:18
❯ app/features/chat/hooks/tests/collection-hooks.test.ts:41:20
[failure] 22-22: app/features/chat/hooks/tests/collection-hooks.test.ts > useThreads > returns data when loaded
TypeError: Cannot read properties of null (reading 'useMemo')
❯ process.env.NODE_ENV.exports.useMemo ../../node_modules/react/cjs/react.development.js:1251:33
❯ Module.useThreads app/features/chat/hooks/queries.ts:22:18
❯ app/features/chat/hooks/tests/collection-hooks.test.ts:29:20
🤖 Prompt for AI Agents
In `@services/platform/app/features/chat/hooks/queries.ts` around lines 22 - 29,
The useThreads hook currently uses React's useMemo (sorted = useMemo(...)) which
forces tests to call hooks via a dispatcher; remove useMemo to allow calling
useThreads directly in tests: replace the useMemo-based computed value with a
plain inline computation (e.g., compute sorted by copying and sorting data with
the same comparator) inside the useThreads implementation and return that value
as threads; ensure you remove the useMemo import/usage and keep the sorting
logic (b._creationTime - a._creationTime) associated with the sorted variable so
the hook remains pure and testable without renderHook.
| interface TestChatPanelProps { | ||
| organizationId: string; | ||
| agentId: string; | ||
| onClose: () => void; | ||
| onReset?: () => void; | ||
| } | ||
|
|
||
| function TestChatPanelContent({ | ||
| organizationId, | ||
| agentId, | ||
| onClose, | ||
| onReset, | ||
| }: TestChatPanelProps) { |
There was a problem hiding this comment.
Clear‑chat now relies on onReset and can leave stale state.
If a caller omits onReset, handleClearChat deletes the thread but leaves threadId and local messages intact, so the UI won’t actually reset. Either make onReset required or add a local fallback reset path.
🧹 Suggested fallback reset when onReset is undefined
- const handleClearChat = useCallback(() => {
+ const handleClearChat = useCallback(() => {
if (threadId) {
void deleteChatThread({ threadId }).catch((error) => {
console.error('Error deleting test chat thread:', error);
});
}
- onReset?.();
- }, [threadId, deleteChatThread, onReset]);
+ if (onReset) {
+ onReset();
+ return;
+ }
+ setThreadId(null);
+ setMessages([]);
+ setPendingUserMessage(null);
+ setInputValue('');
+ clearAttachments();
+ }, [threadId, deleteChatThread, onReset, clearAttachments]);Also applies to: 525-532
🤖 Prompt for AI Agents
In `@services/platform/app/features/custom-agents/components/test-chat-panel.tsx`
around lines 144 - 156, The onReset prop is optional but handleClearChat
currently relies on it, leaving threadId and messages stale if omitted; update
TestChatPanelContent to either make onReset required in TestChatPanelProps or
(preferred) add a local fallback reset inside handleClearChat: after the delete
thread call, if onReset is provided call it, otherwise locally reset the panel
state (clear threadId via setThreadId(null or ''), clear messages via
setMessages([]), reset input/state flags like setIsStreaming(false)/setInput('')
as needed) so the UI fully resets; reference TestChatPanelProps,
TestChatPanelContent and the handleClearChat function when making this change.
| export function useBulkCreateVendors() { | ||
| return useConvexMutation(api.vendors.mutations.bulkCreateVendors); | ||
| } | ||
|
|
||
| export function useDeleteVendor(collection: Collection<Vendor, string>) { | ||
| return useCallback( | ||
| async (args: { vendorId: string }) => { | ||
| const tx = collection.delete(args.vendorId); | ||
| await tx.isPersisted.promise; | ||
| }, | ||
| [collection], | ||
| ); | ||
| export function useDeleteVendor() { | ||
| const { mutateAsync } = useConvexMutation(api.vendors.mutations.deleteVendor); | ||
| return mutateAsync; | ||
| } | ||
|
|
||
| export function useUpdateVendor(collection: Collection<Vendor, string>) { | ||
| return useCallback( | ||
| async (args: { | ||
| vendorId: string; | ||
| name?: string; | ||
| email?: string; | ||
| phone?: string; | ||
| externalId?: string; | ||
| source?: 'manual_import' | 'file_upload' | 'circuly'; | ||
| locale?: string; | ||
| address?: { | ||
| street?: string; | ||
| city?: string; | ||
| state?: string; | ||
| zip?: string; | ||
| country?: string; | ||
| }; | ||
| tags?: string[]; | ||
| metadata?: Record<string, unknown>; | ||
| notes?: string; | ||
| }) => { | ||
| const tx = collection.update(args.vendorId, (draft) => { | ||
| if (args.name !== undefined) draft.name = args.name; | ||
| if (args.email !== undefined) draft.email = args.email; | ||
| if (args.phone !== undefined) draft.phone = args.phone; | ||
| if (args.externalId !== undefined) draft.externalId = args.externalId; | ||
| if (args.source !== undefined) draft.source = args.source; | ||
| if (args.locale !== undefined) draft.locale = args.locale; | ||
| if (args.address !== undefined) draft.address = args.address; | ||
| if (args.tags !== undefined) draft.tags = args.tags; | ||
| if (args.metadata !== undefined) draft.metadata = args.metadata; | ||
| if (args.notes !== undefined) draft.notes = args.notes; | ||
| }); | ||
| await tx.isPersisted.promise; | ||
| }, | ||
| [collection], | ||
| ); | ||
| export function useUpdateVendor() { | ||
| const { mutateAsync } = useConvexMutation(api.vendors.mutations.updateVendor); | ||
| return mutateAsync; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent return patterns across hooks.
useBulkCreateVendors returns the full mutation object (including mutateAsync, isPending, etc.), while useDeleteVendor and useUpdateVendor return only mutateAsync. This inconsistency means consumers get different APIs from the same module.
If intentional (e.g., bulk operations need isPending for progress UI), consider adding a brief comment explaining the design choice. Otherwise, consider aligning all hooks to return the same shape for consistency.
🤖 Prompt for AI Agents
In `@services/platform/app/features/vendors/hooks/mutations.ts` around lines 4 -
16, The three hooks return inconsistent shapes: useBulkCreateVendors returns the
full mutation object while useDeleteVendor and useUpdateVendor return only
mutateAsync; to fix, make the return shapes consistent—either return the full
mutation object from useDeleteVendor and useUpdateVendor (by returning the
result of useConvexMutation in useDeleteVendor and useUpdateVendor) or return
only mutateAsync from useBulkCreateVendors; update useDeleteVendor and
useUpdateVendor (or useBulkCreateVendors) so all hooks (useBulkCreateVendors,
useDeleteVendor, useUpdateVendor) expose the same API and add a short comment if
a special-case is intentional.
1 participant
Summary
contextFeaturesto agent config so builtin agents can declare which context features they need, allowing thebeforeContexthook to skip unnecessary queriesbeforeGenerateHookimplementations into the sharedlib/agent_chatmodule and remove the redundantonErrorhookTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX Improvements