ECHO-450 Add conversation to chat context#292
Conversation
… on 'Ask' from a conversation
…icks 'ask' from a conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds propagation of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-19T10:22:55.323ZApplied to files:
🧬 Code graph analysis (1)echo/frontend/src/components/project/hooks/index.ts (1)
🔇 Additional comments (2)
✨ 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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
Disabled knowledge base sources:
- 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/project/ProjectSidebar.tsx(1 hunks)echo/frontend/src/components/project/hooks/index.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/project/hooks/index.ts
🧬 Code graph analysis (1)
echo/frontend/src/components/project/hooks/index.ts (1)
echo/frontend/src/components/conversation/hooks/index.ts (1)
useAddChatContextMutation(273-404)
⏰ 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-directus, ./echo/directus, Dockerfile, dbr-echo-directus)
- GitHub Check: ci-build-servers (dbr-echo-server, ./echo/server, Dockerfile, dbr-echo-server)
🔇 Additional comments (3)
echo/frontend/src/components/project/hooks/index.ts (3)
22-23: LGTM: localizing the chat-context mutation here is the right call.
118-121: Early navigate FTW.Immediate route switch post-create is the right UX for this flow.
109-121: Confirm product intent: disabling auto-select when a conversation is preselected.Logic now forces auto_select=false when conversationId is provided and enhanced audio is enabled. If the goal is “stick with the explicit conversation, don’t auto-switch,” this is perfect. If auto-select is independent of conversation pinning, revisit.
Would you like me to add an e2e covering: navigating from /conversations/:conversationId → Ask → verify chat context includes that conversation and auto_select=false?
| const chat = await directus.request( | ||
| createItem("project_chat", { | ||
| ...(payload as any), | ||
| auto_select: !!project.is_enhanced_audio_processing_enabled, | ||
| auto_select: payload.conversationId && project.is_enhanced_audio_processing_enabled | ||
| ? false | ||
| : !!project.is_enhanced_audio_processing_enabled, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Blocker: don’t spread client-only keys into Directus createItem.
Spreading the whole payload leaks navigateToNewChat and conversationId to the API. Depending on Directus config, this can 400 or silently drop fields. Send only collection fields.
Apply:
- const chat = await directus.request(
- createItem("project_chat", {
- ...(payload as any),
- auto_select: payload.conversationId && project.is_enhanced_audio_processing_enabled
- ? false
- : !!project.is_enhanced_audio_processing_enabled,
- }),
- );
+ const chat = await directus.request(
+ createItem("project_chat", {
+ project_id: payload.project_id,
+ auto_select:
+ payload.conversationId &&
+ project.is_enhanced_audio_processing_enabled
+ ? false
+ : !!project.is_enhanced_audio_processing_enabled,
+ }),
+ );Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In echo/frontend/src/components/project/hooks/index.ts around lines 109 to 116,
the current createItem spreads the entire payload which leaks client-only keys
like navigateToNewChat and conversationId into the Directus API; instead, build
and pass an object that contains only the collection fields (whitelist) or
explicitly omit client-only keys before calling createItem, and compute
auto_select from payload.conversationId and
project.is_enhanced_audio_processing_enabled without including conversationId
itself; ensure you either pick allowed fields or destructure to remove
navigateToNewChat and conversationId so only valid Directus fields are sent.
| auto_select: payload.conversationId && project.is_enhanced_audio_processing_enabled | ||
| ? false | ||
| : !!project.is_enhanced_audio_processing_enabled, | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tiny cleanup: pull auto_select into a local const for clarity.
No behavior change; easier to reason and reuse.
Apply:
- auto_select:
- payload.conversationId &&
- project.is_enhanced_audio_processing_enabled
- ? false
- : !!project.is_enhanced_audio_processing_enabled,
+ auto_select: shouldAutoSelect,And just above:
const shouldAutoSelect =
payload.conversationId && project.is_enhanced_audio_processing_enabled
? false
: !!project.is_enhanced_audio_processing_enabled;🤖 Prompt for AI Agents
In echo/frontend/src/components/project/hooks/index.ts around lines 112 to 115,
extract the inline ternary used for auto_select into a local constant named
shouldAutoSelect (defined just above the object) and then replace the inline
expression with auto_select: shouldAutoSelect; keep the exact logic: if
payload.conversationId is truthy and
project.is_enhanced_audio_processing_enabled is truthy then false, otherwise
coerce project.is_enhanced_audio_processing_enabled to boolean.
| createChatMutation.mutate({ | ||
| project_id: { id: projectId ?? "" }, | ||
| conversationId: conversationId, | ||
| navigateToNewChat: true, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Gate conversationId to avoid sending undefined; also use shorthand.
Keep the payload clean and avoid passing undefined. Minor readability bump too.
Apply:
createChatMutation.mutate({
- project_id: { id: projectId ?? "" },
- conversationId: conversationId,
+ project_id: { id: projectId! },
+ ...(conversationId ? { conversationId } : {}),
navigateToNewChat: true,
});🤖 Prompt for AI Agents
In echo/frontend/src/components/project/ProjectSidebar.tsx around lines 35-39,
the mutation payload currently sends conversationId even when it may be
undefined and uses the longhand property assignment; instead build the payload
object, only add conversationId when it's defined (e.g. if (conversationId !=
null) payload.conversationId = conversationId), and use shorthand property
syntax for navigateToNewChat (and conversationId when present), then call
createChatMutation.mutate(payload) so undefined is not sent and readability is
improved.
There was a problem hiding this comment.
no need its better when we want to see what parameters are coming in the request
There was a problem hiding this comment.
@ussaama Gotcha, no worries! LGTM then. 🚀
The code works as-is and the mutation will handle the undefined conversationId appropriately. Thanks for the quick response!
🧠 Learnings used
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.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Starting a chat can now continue an existing conversation when launched from a linked context. - Bug Fixes - Chats created from a project reliably associate with the intended conversation. - Improvements - Immediate navigation to newly created chats for a smoother experience. - Smarter default selection of enhanced audio processing when continuing conversations. - Chores - Background changes to make adding chat context non-blocking and improve responsiveness. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores