Conversation
WalkthroughThe update refactors how conversation tags are managed in the frontend by replacing direct tag overwrites with explicit creation and deletion of tag relations using Directus SDK methods. Several hooks are reformatted for clarity, and documentation is enhanced with a troubleshooting section for mypy-related development container issues. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant useUpdateConversationTagsMutation
participant DirectusSDK
UI->>useUpdateConversationTagsMutation: Request to update conversation tags
useUpdateConversationTagsMutation->>DirectusSDK: Fetch existing conversation-project-tag relations
useUpdateConversationTagsMutation->>useUpdateConversationTagsMutation: Compute tags to delete and create
useUpdateConversationTagsMutation->>DirectusSDK: deleteItems (remove old tag relations)
useUpdateConversationTagsMutation->>DirectusSDK: createItems (add new tag relations)
useUpdateConversationTagsMutation->>DirectusSDK: Fetch updated conversation item
useUpdateConversationTagsMutation->>UI: Return updated conversation
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)echo/frontend/src/lib/query.ts (2)
🔇 Additional comments (7)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Nitpick comments (3)
echo/readme.md (1)
134-140: Doc typo & small UX tweak
seperated→separated.
While you’re here, consider adding a one-liner that pipes the PIDs directly intoxargs kill -9so folks don’t have to copy-paste IDs manually.-# grab all the process ids -kill -9 <process ids seperated by spaces> +# grab all the process ids and kill them +ps -aux | grep "mypy" | awk '{print $2}' | xargs -r kill -9echo/frontend/src/lib/query.ts (2)
697-712: Micro-perf: leverageSetfor O(1) look-upsBoth
needToDeleteandneedToCreateperform repeated.includes/.somesearches. Convert the lists toSets first for linear-time clarity.Not blocking, just a heads-up.
2136-2140: Redundantawaitbeforereturn
submitNotificationParticipant()already returns a promise;return awaitadds an unnecessary micro-tick.- return await submitNotificationParticipant( + return submitNotificationParticipant( emails, projectId, conversationId, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
echo/frontend/src/lib/query.ts(8 hunks)echo/readme.md(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
echo/frontend/src/lib/query.ts (2)
echo/server/dembrane/reply_utils.py (1)
Conversation(14-18)echo/frontend/src/lib/api.ts (3)
addChatContext(718-730)deleteChatContext(732-744)submitNotificationParticipant(842-857)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-check-server
| createItems( | ||
| "conversation_project_tag", | ||
| needToCreate.map((tagId) => ({ | ||
| conversation_id: { | ||
| id: conversationId, | ||
| } as Conversation, | ||
| project_tag_id: { | ||
| id: tagId, | ||
| } as ProjectTag, | ||
| })), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use flat FK values – nested objects are unnecessary noise
createItems() happily accepts the raw FK IDs. Passing { id: x } objects just bloats the payload and forces the cast to as Conversation | ProjectTag.
- createItems(
- "conversation_project_tag",
- needToCreate.map((tagId) => ({
- conversation_id: { id: conversationId } as Conversation,
- project_tag_id: { id: tagId } as ProjectTag,
- })),
+ createItems(
+ "conversation_project_tag",
+ needToCreate.map((tagId) => ({
+ conversation_id: conversationId,
+ project_tag_id: tagId,
+ })),
),📝 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.
| createItems( | |
| "conversation_project_tag", | |
| needToCreate.map((tagId) => ({ | |
| conversation_id: { | |
| id: conversationId, | |
| } as Conversation, | |
| project_tag_id: { | |
| id: tagId, | |
| } as ProjectTag, | |
| })), | |
| ), | |
| createItems( | |
| "conversation_project_tag", | |
| needToCreate.map((tagId) => ({ | |
| conversation_id: conversationId, | |
| project_tag_id: tagId, | |
| })), | |
| ), |
There was a problem hiding this comment.
needs to be that way for directus actually. not ideal
There was a problem hiding this comment.
@spashii LGTM, thx for sharing that insider knowledge about Directus! Will def remember that the API requires those nested objects with id fields for FKs despite looking redundant af. 🚀
✏️ Learnings added
Learnt from: spashii
PR: Dembrane/echo#142
File: echo/frontend/src/lib/query.ts:730-740
Timestamp: 2025-05-13T15:18:29.070Z
Learning: When working with Directus API in this codebase, foreign key relationships must be specified using nested objects with `id` properties (e.g., `conversation_id: { id: conversationId } as Conversation`) rather than direct ID values, even though this appears redundant.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const tagsRequest = await directus.request( | ||
| readItems("conversation_project_tag", { | ||
| fields: [ | ||
| "id", | ||
| { | ||
| project_tag_id: ["id"], | ||
| }, | ||
| { | ||
| conversation_id: ["id"], | ||
| }, | ||
| ], | ||
| filter: { | ||
| conversation_id: { _eq: conversationId }, | ||
| }, | ||
| }), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Possible truncation – explicitly lift the Directus limit
readItems("conversation_project_tag", …) relies on the default Directus limit (100).
If a conversation ever acquires > 100 tag relations, the “extra” rows will be silently ignored, leading to accidental data loss on the delete-/create pass below.
readItems("conversation_project_tag", {
fields: [ … ],
filter: { conversation_id: { _eq: conversationId } },
+ limit: -1, // fetch *all* relations
}),📝 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 tagsRequest = await directus.request( | |
| readItems("conversation_project_tag", { | |
| fields: [ | |
| "id", | |
| { | |
| project_tag_id: ["id"], | |
| }, | |
| { | |
| conversation_id: ["id"], | |
| }, | |
| ], | |
| filter: { | |
| conversation_id: { _eq: conversationId }, | |
| }, | |
| }), | |
| ); | |
| const tagsRequest = await directus.request( | |
| readItems("conversation_project_tag", { | |
| fields: [ | |
| "id", | |
| { | |
| project_tag_id: ["id"], | |
| }, | |
| { | |
| conversation_id: ["id"], | |
| }, | |
| ], | |
| filter: { | |
| conversation_id: { _eq: conversationId }, | |
| }, | |
| limit: -1, // fetch *all* relations | |
| }), | |
| ); |
There was a problem hiding this comment.
good suggestion >100 tags should result in user error and complaining back - usually means the user might be having a diff usecase which tags isn't meant for anyway. error is on a logical level above this impl
There was a problem hiding this comment.
@spashii Gotcha, your architecture decision is solid. Setting an implicit cap at 100 tags is a smart anti-pattern - if someone's hitting that limit, the feature's being stretched beyond its intended use case anyway. No need to optimize for the edge case. LGTM! Ship it! 🚀
(╯°□°)╯︵ ┻━┻ <-- me pushing this PR to prod rn
✏️ Learnings added
Learnt from: spashii
PR: Dembrane/echo#142
File: echo/frontend/src/lib/query.ts:680-696
Timestamp: 2025-05-13T15:20:25.152Z
Learning: In the Echo project, the conversation tags feature is designed with an implicit limit of 100 tags per conversation. Having more than 100 tags indicates a misuse of the feature rather than a limitation that needs to be addressed in the code. Errors for exceeding this limit should be handled at a logical level above the implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
… cleaner error handling
…to-make-form-edits-more-robust
spashii
deleted the
feature/echo-136-identify-if-there-are-ways-to-make-form-edits-more-robust
branch
…rs (#142) * ECHO-136 (dashboard) fix updating conversation tags for application users * format * Remove console error logging in useUpdateConversationTagsMutation for cleaner error handling --------- Co-authored-by: Usama <reach.usamazafar@gmail.com>
2 participants
Summary by CodeRabbit