ECHO-587 Chat select all feature + tags add to filter #405
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds a feature-flagged "select all" flow for adding conversations to chat context with tag/verified/search filters, new frontend modals and hooks, API and type changes for batch context addition, server-side select-all implementation, conversation-list filtering service, and extensive localization updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
echo/frontend/src/locales/it-IT.po (1)
8-8: Regenerate the Italian locale file — theit-IT.poLanguage header is set toen-USinstead ofit-IT.All other locale files have correct Language headers matching their filename. The lingui.config.ts is correctly configured with
it-ITlisted in the locales array. Runlingui extract && lingui compileto regenerate the locale files and fix the header.
🤖 Fix all issues with AI agents
In `@echo/frontend/src/components/conversation/ConversationAccordion.tsx`:
- Around line 891-954: Add a new state like selectAllError (string | null)
alongside selectAllLoading/result and set it to null in handleSelectAllClick to
clear previous errors; in handleSelectAllConfirm's catch block do NOT close the
modal—set selectAllError to the caught error message (or a fallback string) and
keep selectAllModalOpened unchanged; pass selectAllError as errorMessage to
SelectAllConfirmationModal and update that component's initial view to render a
Mantine Alert when errorMessage is present; also clear selectAllError on
successful confirm (after setSelectAllResult) or when the modal is explicitly
closed.
In `@echo/frontend/src/components/conversation/ConversationLinks.tsx`:
- Around line 114-120: The ConversationLinks component currently declares unused
optional props color and hoverUnderlineColor; remove these dead props from the
component signature and its props type so the function only accepts {
conversations }: { conversations: Conversation[] } (update the export const
ConversationLinks destructuring and its inline type annotation), and run a quick
grep for any callers that might pass those props so they can be cleaned up or
ignored as needed.
- Around line 140-152: The mapped Fragment inside the visibleConversations.map
is missing a key which will trigger React warnings; fix by either removing the
unnecessary Fragment wrapper around I18nLink or adding a unique key (e.g.,
key={conversation.id}) to the Fragment so each child in the list is keyed;
locate the map over visibleConversations in ConversationLinks.tsx and update the
JSX around I18nLink accordingly (use conversation.id as the key if keeping the
Fragment).
In `@echo/frontend/src/components/conversation/SelectAllConfirmationModal.tsx`:
- Around line 404-437: The badge group currently only renders when searchText or
filterNames are present, hiding the Verified badge for verified-only filters;
update the rendering condition for the Group that wraps the badges to include
hasVerifiedOutcomesFilter (i.e., change the conditional from (searchText ||
filterNames.length > 0) to (searchText || filterNames.length > 0 ||
hasVerifiedOutcomesFilter)) so the Verified Badge (rendered via
hasVerifiedOutcomesFilter and IconRosetteDiscountCheckFilled) displays even when
it is the sole active filter.
- Around line 610-614: The copy for select.all.modal.other.reason.description in
SelectAllConfirmationModal.tsx is misleading because skippedDueToOther includes
reasons like "too_long" and "error", not only missing transcripts; update the
translation string (and all locale files) to a more general phrase (e.g., “These
conversations were excluded for other reasons, such as length limits or
processing errors.”) and ensure the new wording replaces the current Trans
id="select.all.modal.other.reason.description" so all locales are synced; keep
the Trans id the same or update usages consistently if you change the key.
In `@echo/frontend/src/locales/en-US.po`:
- Around line 3598-3601: Update the translation for msgid
"select.all.modal.skip.reason" (used in SelectAllConfirmationModal.tsx) to a
human-friendly full sentence with capitalization and clarity; replace the
fragmentary msgstr with something like "Some items may be skipped due to an
empty transcript or context limits." Ensure the updated msgstr is a complete,
clear sentence.
In `@echo/server/dembrane/api/chat.py`:
- Around line 346-356: Update the validation error messages that check
options_provided (the block containing the two HTTPException raises) to include
auto_select_bool as a valid option; specifically, change the detail strings in
the branches that currently say "conversation_id or select_all is required" and
"Only one of conversation_id or select_all can be provided" to reference
"conversation_id, select_all, or auto_select_bool" (and similarly "Only one of
conversation_id, select_all, or auto_select_bool can be provided") so callers
see all accepted mutually exclusive parameters.
- Around line 473-485: The selection loop currently stops only when
current_token_usage + conversation_usage > 1 (100%); change the cap to 85% by
comparing against 0.85 instead (i.e., enforce current_token_usage +
conversation_usage > 0.85) so Select-all enforces the required 85% context cap;
update the surrounding comment to reflect the 85% limit and keep use of
conversation_usage, MAX_CHAT_CONTEXT_LENGTH, current_token_usage,
context_limit_reached, SelectAllConversationResult, and skipped unchanged
otherwise.
Comment on lines
+891
to
+954
| // Select All state | ||
| const [selectAllModalOpened, setSelectAllModalOpened] = useState(false); | ||
| const [selectAllResult, setSelectAllResult] = | ||
| useState<SelectAllContextResponse | null>(null); | ||
| const [selectAllLoading, setSelectAllLoading] = useState(false); | ||
| const selectAllMutation = useSelectAllContextMutation(); | ||
|
|
||
| // Add Tag Filter Modal state | ||
| const [addTagFilterModalOpened, addTagFilterModalHandlers] = | ||
| useDisclosure(false); | ||
| const [selectedTagForFilter, setSelectedTagForFilter] = | ||
| useState<ConversationProjectTag | null>(null); | ||
|
|
||
| // Handle tag click | ||
| const handleTagClick = (tag: ConversationProjectTag) => { | ||
| setSelectedTagForFilter(tag); | ||
| addTagFilterModalHandlers.open(); | ||
| }; | ||
|
|
||
| // Handle tag filter confirmation | ||
| const handleAddTagFilter = () => { | ||
| if (!selectedTagForFilter) return; | ||
|
|
||
| const tagId = (selectedTagForFilter.project_tag_id as ProjectTag)?.id; | ||
| if (tagId && !selectedTagIds.includes(tagId)) { | ||
| setSelectedTagIds((prev) => [...prev, tagId]); | ||
| } | ||
| }; | ||
|
|
||
| const handleTagFilterModalExitTransitionEnd = () => { | ||
| // Clear data after modal has fully closed | ||
| setSelectedTagForFilter(null); | ||
| }; | ||
|
|
||
| // Handle select all | ||
| const handleSelectAllClick = () => { | ||
| setSelectAllModalOpened(true); | ||
| setSelectAllResult(null); | ||
| }; | ||
|
|
||
| const handleSelectAllConfirm = async () => { | ||
| if (!chatId || !projectId) { | ||
| toast.error(t`Failed to add conversations to context`); | ||
| console.error("Missing required parameters for select all"); | ||
| return; | ||
| } | ||
|
|
||
| setSelectAllLoading(true); | ||
| try { | ||
| const result = await selectAllMutation.mutateAsync({ | ||
| chatId, | ||
| projectId, | ||
| searchText: debouncedConversationSearchValue || undefined, | ||
| tagIds: selectedTagIds.length > 0 ? selectedTagIds : undefined, | ||
| verifiedOnly: showOnlyVerified || undefined, | ||
| }); | ||
| setSelectAllResult(result); | ||
| } catch (_error) { | ||
| toast.error(t`Failed to add conversations to context`); | ||
| setSelectAllModalOpened(false); | ||
| } finally { | ||
| setSelectAllLoading(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and verify the file exists
fd -t f "ConversationAccordion.tsx" --exec wc -l {} \;Repository: Dembrane/echo
Length of output: 131
🏁 Script executed:
# Check the error handling section mentioned (lines 949-951 area)
fd -t f "ConversationAccordion.tsx" -x cat -n {} | sed -n '940,960p'Repository: Dembrane/echo
Length of output: 801
🏁 Script executed:
# Find SelectAllConfirmationModal component
fd -t f "SelectAllConfirmationModal*"Repository: Dembrane/echo
Length of output: 129
🏁 Script executed:
# Inspect SelectAllConfirmationModal component
cat -n echo/frontend/src/components/conversation/SelectAllConfirmationModal.tsxRepository: Dembrane/echo
Length of output: 22974
🏁 Script executed:
# Also check what props are currently passed to SelectAllConfirmationModal in ConversationAccordion
rg -A 15 "SelectAllConfirmationModal" echo/frontend/src/components/conversation/ConversationAccordion.tsx | head -40Repository: Dembrane/echo
Length of output: 982
Keep the Select‑All modal open and surface inline error feedback via Mantine Alert.
The error handler (lines 948–951) closes the modal immediately and fires only a toast—users lose context. Per guidelines, keep the modal open on error, store the message in state, and render it inline with a Mantine Alert component. The modal already uses Alert for warnings elsewhere, so the pattern is established.
Add selectAllError state, pass it as errorMessage to SelectAllConfirmationModal, and render it in the modal's initial view.
🤖 Prompt for AI Agents
In `@echo/frontend/src/components/conversation/ConversationAccordion.tsx` around
lines 891 - 954, Add a new state like selectAllError (string | null) alongside
selectAllLoading/result and set it to null in handleSelectAllClick to clear
previous errors; in handleSelectAllConfirm's catch block do NOT close the
modal—set selectAllError to the caught error message (or a fallback string) and
keep selectAllModalOpened unchanged; pass selectAllError as errorMessage to
SelectAllConfirmationModal and update that component's initial view to render a
Mantine Alert when errorMessage is present; also clear selectAllError on
successful confirm (after setSelectAllResult) or when the modal is explicitly
closed.
Contributor
Author
There was a problem hiding this comment.
the error could only be if the server does not respond and in this case its better if user retries by reopening the modal. so no issues
Contributor
There was a problem hiding this comment.
@ussaama Fair point! If it's a network/server timeout scenario, forcing users to reopen makes sense – keeps the UX clean and doesn't trap them in a half-broken modal state. LGTM. 👍
✏️ Learnings added
Learnt from: ussaama
Repo: Dembrane/echo PR: 405
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:891-954
Timestamp: 2026-01-19T14:02:49.038Z
Learning: In echo/frontend/src/components/conversation/ConversationAccordion.tsx, the select-all error handling (handleSelectAllConfirm) intentionally closes the modal on error and shows only a toast. Errors in this flow only occur when the server doesn't respond, and the design decision is to let users retry by reopening the modal rather than keeping them in an error state within the modal.
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.
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 256
File: echo/frontend/src/components/participant/MicrophoneTest.tsx:54-54
Timestamp: 2025-08-06T13:38:30.769Z
Learning: In echo/frontend/src/components/participant/MicrophoneTest.tsx, the useDisclosure hook is used where only the `close` function is needed locally (called in handleConfirmMicChange and handleCancelMicChange), while `opened` and `open` are unused because the modal state is managed by a parent component. The `showSecondModal` state variable is used separately to control content switching within the modal between the main UI and confirmation step.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/{components,routes}/**/*.tsx : UI mutations should surface inline feedback: pair toasts with contextual Mantine `Alert` components inside modals/forms for errors or warnings
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Learnt from: spashii
Repo: Dembrane/echo PR: 142
File: echo/frontend/src/lib/query.ts:680-696
Timestamp: 2025-05-13T15:20:25.165Z
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.
Learnt from: ussaama
Repo: Dembrane/echo PR: 336
File: echo/server/dembrane/api/chat.py:593-630
Timestamp: 2025-10-15T11:06:42.397Z
Learning: In `echo/server/dembrane/api/chat.py`, the auto-select conversation flow (lines 589-631) deliberately uses incremental system message generation with `create_system_messages_for_chat` and `token_counter` for each candidate conversation to ensure accurate token count estimation before adding conversations. The user (ussaama) prioritizes accuracy over the O(n²) performance cost to stay within the 80% context threshold precisely.
Comment on lines
114
to
120
| export const ConversationLinks = ({ | ||
| conversations, | ||
| color, | ||
| hoverUnderlineColor, | ||
| }: { | ||
| conversations: Conversation[]; | ||
| color?: string; | ||
| hoverUnderlineColor?: string; | ||
| }) => { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead props — color and hoverUnderlineColor are declared but never used.
The component signature still accepts these props but they're not wired up anywhere. Ship cleaner code by removing them.
🔧 Proposed fix
export const ConversationLinks = ({
conversations,
}: {
conversations: Conversation[];
- color?: string;
- hoverUnderlineColor?: string;
}) => {🤖 Prompt for AI Agents
In `@echo/frontend/src/components/conversation/ConversationLinks.tsx` around lines
114 - 120, The ConversationLinks component currently declares unused optional
props color and hoverUnderlineColor; remove these dead props from the component
signature and its props type so the function only accepts { conversations }: {
conversations: Conversation[] } (update the export const ConversationLinks
destructuring and its inline type annotation), and run a quick grep for any
callers that might pass those props so they can be cleaned up or ignored as
needed.
Contributor
Author
There was a problem hiding this comment.
no issue they are just here if needed
Contributor
There was a problem hiding this comment.
@ussaama LGTM. Keeping those props in the signature for future flexibility – totally valid approach. 🚀
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 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 useSuspenseQuery with Directus aggregate, which always returns string numbers like "0", "1", "2" and suspends during loading instead of returning undefined. Therefore, Number(conversationsCountQuery.data) ?? 0 is safe and the Number() conversion is necessary for type conversion from string to number.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/components/chat/**/*.tsx : Chat modes have dedicated accent colors (Overview: Spring Green `#1EFFA1`, Deep Dive: Cyan `#00FFFF`) defined in `src/components/chat/ChatModeSelector.tsx` (`MODE_COLORS`); keep these hardcoded for consistent mode identification
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Learnt from: spashii
Repo: Dembrane/echo PR: 142
File: echo/frontend/src/lib/query.ts:680-696
Timestamp: 2025-05-13T15:20:25.165Z
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.
Learnt from: ussaama
Repo: Dembrane/echo PR: 336
File: echo/server/dembrane/api/chat.py:593-630
Timestamp: 2025-10-15T11:06:42.397Z
Learning: In `echo/server/dembrane/api/chat.py`, the auto-select conversation flow (lines 589-631) deliberately uses incremental system message generation with `create_system_messages_for_chat` and `token_counter` for each candidate conversation to ensure accurate token count estimation before adding conversations. The user (ussaama) prioritizes accuracy over the O(n²) performance cost to stay within the 80% context threshold precisely.
echo/frontend/src/components/conversation/SelectAllConfirmationModal.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+610
to
+614
| <Text size="xs" c="dimmed"> | ||
| <Trans id="select.all.modal.other.reason.description"> | ||
| These conversations were excluded due to missing | ||
| transcripts. | ||
| </Trans> |
Contributor
There was a problem hiding this comment.
“Other reasons” copy is misleading.
skippedDueToOther includes too_long and error, not just missing transcripts. Update the string (and sync locales) to be more general.
💬 Suggested copy
-<Trans id="select.all.modal.other.reason.description">
- These conversations were excluded due to missing transcripts.
-</Trans>
+<Trans id="select.all.modal.other.reason.description">
+ These conversations were skipped for other reasons (see badges for details).
+</Trans>🤖 Prompt for AI Agents
In `@echo/frontend/src/components/conversation/SelectAllConfirmationModal.tsx`
around lines 610 - 614, The copy for select.all.modal.other.reason.description
in SelectAllConfirmationModal.tsx is misleading because skippedDueToOther
includes reasons like "too_long" and "error", not only missing transcripts;
update the translation string (and all locale files) to a more general phrase
(e.g., “These conversations were excluded for other reasons, such as length
limits or processing errors.”) and ensure the new wording replaces the current
Trans id="select.all.modal.other.reason.description" so all locales are synced;
keep the Trans id the same or update usages consistently if you change the key.
Contributor
Author
There was a problem hiding this comment.
the other reasons just include "No Content" conversations as there is no other error for them. Additional checks for other reasons are just a failsafe method
Contributor
There was a problem hiding this comment.
@ussaama Got it! Thanks for clarifying—if "No Content" is the only reason that actually appears in practice, then the current copy makes total sense. LGTM. 🚀
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 180
File: echo/frontend/src/locales/es-ES.ts:1-1
Timestamp: 2025-06-06T05:43:44.935Z
Learning: When users indicate they have already addressed translation issues in locale files, verify the current state before assuming the issue persists. Translation files are often updated in bulk and previously flagged English text may have been properly localized.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/components/chat/**/*.tsx : Chat modes have dedicated accent colors (Overview: Spring Green `#1EFFA1`, Deep Dive: Cyan `#00FFFF`) defined in `src/components/chat/ChatModeSelector.tsx` (`MODE_COLORS`); keep these hardcoded for consistent mode identification
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/{components,routes}/**/*.tsx : UI mutations should surface inline feedback: pair toasts with contextual Mantine `Alert` components inside modals/forms for errors or warnings
Learnt from: ussaama
Repo: Dembrane/echo PR: 256
File: echo/frontend/src/components/participant/MicrophoneTest.tsx:54-54
Timestamp: 2025-08-06T13:38:30.769Z
Learning: In echo/frontend/src/components/participant/MicrophoneTest.tsx, the useDisclosure hook is used where only the `close` function is needed locally (called in handleConfirmMicChange and handleCancelMicChange), while `opened` and `open` are unused because the modal state is managed by a parent component. The `showSecondModal` state variable is used separately to control content switching within the modal between the main UI and confirmation step.
Learnt from: ussaama
Repo: Dembrane/echo PR: 293
File: echo/frontend/src/components/chat/TemplatesModal.tsx:34-41
Timestamp: 2025-09-16T08:35:18.796Z
Learning: In TemplatesModal.tsx, the user confirmed that titles should be used as template keys rather than IDs, consistent with their established pattern. IDs were added primarily for list rendering, not for selection logic.
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Learnt from: spashii
Repo: Dembrane/echo PR: 142
File: echo/frontend/src/lib/query.ts:680-696
Timestamp: 2025-05-13T15:20:25.165Z
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.
Learnt from: ussaama
Repo: Dembrane/echo PR: 336
File: echo/server/dembrane/api/chat.py:593-630
Timestamp: 2025-10-15T11:06:42.397Z
Learning: In `echo/server/dembrane/api/chat.py`, the auto-select conversation flow (lines 589-631) deliberately uses incremental system message generation with `create_system_messages_for_chat` and `token_counter` for each candidate conversation to ensure accurate token count estimation before adding conversations. The user (ussaama) prioritizes accuracy over the O(n²) performance cost to stay within the 80% context threshold precisely.
Comment on lines
+346
to
+356
| if options_provided == 0: | ||
| raise HTTPException( | ||
| status_code=400, detail="conversation_id or auto_select_bool is required" | ||
| status_code=400, | ||
| detail="conversation_id or select_all is required", | ||
| ) | ||
|
|
||
| if body.conversation_id is not None and body.auto_select_bool is not None: | ||
| if options_provided > 1: | ||
| raise HTTPException( | ||
| status_code=400, detail="conversation_id and auto_select_bool cannot both be provided" | ||
| status_code=400, | ||
| detail="Only one of conversation_id or select_all can be provided", | ||
| ) |
Contributor
There was a problem hiding this comment.
Validation error messages should mention auto_select_bool.
The logic allows auto_select_bool as a valid option, but the error text omits it, which is confusing for API callers.
✏️ Suggested tweak
- detail="conversation_id or select_all is required",
+ detail="conversation_id, auto_select_bool, or select_all is required",
...
- detail="Only one of conversation_id or select_all can be provided",
+ detail="Only one of conversation_id, auto_select_bool, or select_all can be provided",📝 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.
Suggested change
| if options_provided == 0: | |
| raise HTTPException( | |
| status_code=400, detail="conversation_id or auto_select_bool is required" | |
| status_code=400, | |
| detail="conversation_id or select_all is required", | |
| ) | |
| if body.conversation_id is not None and body.auto_select_bool is not None: | |
| if options_provided > 1: | |
| raise HTTPException( | |
| status_code=400, detail="conversation_id and auto_select_bool cannot both be provided" | |
| status_code=400, | |
| detail="Only one of conversation_id or select_all can be provided", | |
| ) | |
| if options_provided == 0: | |
| raise HTTPException( | |
| status_code=400, | |
| detail="conversation_id, auto_select_bool, or select_all is required", | |
| ) | |
| if options_provided > 1: | |
| raise HTTPException( | |
| status_code=400, | |
| detail="Only one of conversation_id, auto_select_bool, or select_all can be provided", | |
| ) |
🤖 Prompt for AI Agents
In `@echo/server/dembrane/api/chat.py` around lines 346 - 356, Update the
validation error messages that check options_provided (the block containing the
two HTTPException raises) to include auto_select_bool as a valid option;
specifically, change the detail strings in the branches that currently say
"conversation_id or select_all is required" and "Only one of conversation_id or
select_all can be provided" to reference "conversation_id, select_all, or
auto_select_bool" (and similarly "Only one of conversation_id, select_all, or
auto_select_bool can be provided") so callers see all accepted mutually
exclusive parameters.
Contributor
Author
There was a problem hiding this comment.
nice catch but we will remove the "auto select" functionality altogether soon from the code so this is not relevant. i updated the error messages to comply with future changes that we're going to make.
Contributor
There was a problem hiding this comment.
@ussaama Sounds good! LGTM – makes total sense to align with the upcoming removal rather than polish something that's on the way out. 🚀
🧠 Learnings used
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/server/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:58.885Z
Learning: Applies to echo/server/dembrane/service/conversation.py : Validate `project_tag_id_list` (TODO in `dembrane/service/conversation.py:101`)
Learnt from: ussaama
Repo: Dembrane/echo PR: 336
File: echo/server/dembrane/api/chat.py:593-630
Timestamp: 2025-10-15T11:06:42.397Z
Learning: In `echo/server/dembrane/api/chat.py`, the auto-select conversation flow (lines 589-631) deliberately uses incremental system message generation with `create_system_messages_for_chat` and `token_counter` for each candidate conversation to ensure accurate token count estimation before adding conversations. The user (ussaama) prioritizes accuracy over the O(n²) performance cost to stay within the 80% context threshold precisely.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/server/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:58.885Z
Learning: Applies to echo/server/dembrane/api/chat.py : Fill module stub, add RAG shortcut when quotes exist, implement Directus project fetch, conversation endpoint completion, admin auth checks (TODOs in `dembrane/api/chat.py`)
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Learnt from: spashii
Repo: Dembrane/echo PR: 142
File: echo/frontend/src/lib/query.ts:680-696
Timestamp: 2025-05-13T15:20:25.165Z
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@echo/frontend/src/components/conversation/ConversationLinks.tsx`:
- Around line 153-167: The condensed badge is only clickable and not
keyboard-accessible; update the Badge inside the shouldCondense block so it is
focusable and operable via keyboard by adding tabIndex={0}, role="button", an
accessible label (use the same tooltip label from t`Click to see all
${totalCount} conversations` or aria-label) and an onKeyDown handler that calls
setModalOpened(true) when Enter or Space is pressed; keep the existing onClick
and className to preserve pointer behavior and visual styling.
In `@echo/frontend/src/locales/de-DE.po`:
- Around line 4149-4151: The German translation for the Overview Mode tip (msgid
starting "Welcome to Overview Mode!") has incorrect phrasing "start eine neue
Chat"; update the msgstr to use correct imperative and gender/accusative:
replace with "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all
deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen
Daten. Für genaue Zitate starte einen neuen Chat im Modus „Genauer Kontext“." —
locate the entry associated with ProjectChatRoute.tsx:516 (msgid shown) and edit
the msgstr accordingly.
- Around line 3269-3273: Update the German translation for msgid
"select.all.modal.skip.disclaimer": replace the incorrect phrase "der
Kontext-Limit erreicht ist" with a grammatically correct and consistent term
used elsewhere, e.g. "das Kontextlimit erreicht ist" so the msgstr becomes
"Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder
das Kontextlimit erreicht ist."
In `@echo/frontend/src/locales/es-ES.po`:
- Around line 4026-4029: Replace the English translation for the msgid
"conversation.filters.verified.text" by updating the msgstr value currently set
to "Verified" to the Spanish "Verificado"; locate the entry for msgid
"conversation.filters.verified.text" and change its msgstr to "Verificado" so it
matches the surrounding Spanish translations.
In `@echo/frontend/src/locales/fr-FR.po`:
- Around line 136-141: The French translations for the pluralized counts are
currently using only the plural form (e.g., msgid "select.all.modal.added.count"
-> msgstr "{0} ajoutées"), which will display incorrectly for singular (1);
update the translations to use ICU-style pluralization (one/other) for those
message ids (at least "select.all.modal.added.count" and the corresponding
not-added id) so the UI shows "1 ajoutée" vs "2+ ajoutées", or alternatively
ensure the code paths that call formatMessage never pass a value of 1 for these
keys—modify either the .po entries to include plural variants or change the
message keys to ICU plural keys used by SelectAllConfirmationModal and related
components referenced in the diff.
In `@echo/server/dembrane/api/chat.py`:
- Around line 372-376: The logger currently emits raw search text
(body.search_text) which may contain PII; update the logging in the try block
where logger.info is called to avoid logging the raw query. Replace inclusion of
body.search_text with a safe indicator such as whether a search was provided and
its length (e.g., search_present=True/False and search_length=int) or a
truncated/hashed token, while keeping project_id, body.tag_ids and
body.verified_only unchanged; edit the logger.info call that formats "Select
All: Fetching conversations - project_id=..., tags=..., verified=...,
search='...'" to use these safe fields instead.
♻️ Duplicate comments (1)
echo/frontend/src/locales/en-US.po (1)
3599-3602: Tighten the skip-disclaimer phrasing (Line 3601).Current wording is a bit awkward; passive voice reads cleaner. Based on learnings, this is a preference-level copy tweak.
✍️ Suggested copy
-msgstr "Some conversations might skip due to empty transcript or context limit" +msgstr "Some conversations may be skipped due to empty transcripts or the context limit."
Comment on lines
+153
to
+167
| {shouldCondense && ( | ||
| <Tooltip | ||
| label={t`Click to see all ${totalCount} conversations`} | ||
| position="top" | ||
| withArrow | ||
| > | ||
| {conversation.participant_name} | ||
| </Anchor> | ||
| </I18nLink> | ||
| )) ?? null} | ||
| </Group> | ||
| <Badge | ||
| size="md" | ||
| variant="light" | ||
| ml="xs" | ||
| className="cursor-pointer not-italic" | ||
| onClick={() => setModalOpened(true)} | ||
| > | ||
| <Trans>+{hiddenCount} conversations</Trans> | ||
| </Badge> |
Contributor
There was a problem hiding this comment.
Make the condensed badge keyboard-accessible (a11y blocker).
Right now the badge is click-only; keyboard users can’t open the modal. Not LGTM for accessibility.
🔧 Suggested fix
{shouldCondense && (
<Tooltip
label={t`Click to see all ${totalCount} conversations`}
position="top"
withArrow
>
- <Badge
- size="md"
- variant="light"
- ml="xs"
- className="cursor-pointer not-italic"
- onClick={() => setModalOpened(true)}
- >
- <Trans>+{hiddenCount} conversations</Trans>
- </Badge>
+ <button
+ type="button"
+ onClick={() => setModalOpened(true)}
+ className="cursor-pointer border-0 bg-transparent p-0"
+ >
+ <Badge size="md" variant="light" ml="xs" className="not-italic">
+ <Trans>+{hiddenCount} conversations</Trans>
+ </Badge>
+ </button>
</Tooltip>
)}🤖 Prompt for AI Agents
In `@echo/frontend/src/components/conversation/ConversationLinks.tsx` around lines
153 - 167, The condensed badge is only clickable and not keyboard-accessible;
update the Badge inside the shouldCondense block so it is focusable and operable
via keyboard by adding tabIndex={0}, role="button", an accessible label (use the
same tooltip label from t`Click to see all ${totalCount} conversations` or
aria-label) and an onKeyDown handler that calls setModalOpened(true) when Enter
or Space is pressed; keep the existing onClick and className to preserve pointer
behavior and visual styling.
Comment on lines
+3269
to
+3273
| #. js-lingui-explicit-id | ||
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | ||
| msgid "select.all.modal.skip.disclaimer" | ||
| msgstr "Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder der Kontext-Limit erreicht ist." | ||
|
|
Contributor
There was a problem hiding this comment.
Polish the context-limit disclaimer grammar (DE).
“der Kontext-Limit” is grammatically off and inconsistent with “Kontextlimit” elsewhere.
🔧 Suggested fix
-msgstr "Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder der Kontext-Limit erreicht ist."
+msgstr "Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder das Kontextlimit erreicht ist."📝 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.
Suggested change
| #. js-lingui-explicit-id | |
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | |
| msgid "select.all.modal.skip.disclaimer" | |
| msgstr "Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder der Kontext-Limit erreicht ist." | |
| #. js-lingui-explicit-id | |
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | |
| msgid "select.all.modal.skip.disclaimer" | |
| msgstr "Einige Gespräche können ausgelassen werden, weil sie kein Transkript haben oder das Kontextlimit erreicht ist." |
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/de-DE.po` around lines 3269 - 3273, Update the
German translation for msgid "select.all.modal.skip.disclaimer": replace the
incorrect phrase "der Kontext-Limit erreicht ist" with a grammatically correct
and consistent term used elsewhere, e.g. "das Kontextlimit erreicht ist" so the
msgstr becomes "Einige Gespräche können ausgelassen werden, weil sie kein
Transkript haben oder das Kontextlimit erreicht ist."
Comment on lines
+4149
to
4151
| #: src/routes/project/chat/ProjectChatRoute.tsx:516 | ||
| msgid "Welcome to Overview Mode! I have summaries of all your conversations loaded. Ask me about patterns, themes, and insights across your data. For exact quotes, start a new chat in Specific Context mode." | ||
| msgstr "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate start eine neue Chat im Modus „Genauer Kontext“." |
Contributor
There was a problem hiding this comment.
Fix German phrasing in the Overview Mode tip.
“start eine neue Chat” reads wrong; should be “starte einen neuen Chat.”
🔧 Suggested fix
-msgstr "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate start eine neue Chat im Modus „Genauer Kontext“."
+msgstr "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate starte einen neuen Chat im Modus „Genauer Kontext“."📝 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.
Suggested change
| #: src/routes/project/chat/ProjectChatRoute.tsx:516 | |
| msgid "Welcome to Overview Mode! I have summaries of all your conversations loaded. Ask me about patterns, themes, and insights across your data. For exact quotes, start a new chat in Specific Context mode." | |
| msgstr "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate start eine neue Chat im Modus „Genauer Kontext“." | |
| #: src/routes/project/chat/ProjectChatRoute.tsx:516 | |
| msgid "Welcome to Overview Mode! I have summaries of all your conversations loaded. Ask me about patterns, themes, and insights across your data. For exact quotes, start a new chat in Specific Context mode." | |
| msgstr "Willkommen im Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen. Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate starte einen neuen Chat im Modus „Genauer Kontext"." |
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/de-DE.po` around lines 4149 - 4151, The German
translation for the Overview Mode tip (msgid starting "Welcome to Overview
Mode!") has incorrect phrasing "start eine neue Chat"; update the msgstr to use
correct imperative and gender/accusative: replace with "Willkommen im
Übersichtmodus! Ich hab Zusammenfassungen von all deinen Gesprächen geladen.
Frag mich nach Mustern, Themen und Insights in deinen Daten. Für genaue Zitate
starte einen neuen Chat im Modus „Genauer Kontext“." — locate the entry
associated with ProjectChatRoute.tsx:516 (msgid shown) and edit the msgstr
accordingly.
Comment on lines
+136
to
+141
| #. js-lingui-explicit-id | ||
| #. placeholder {0}: result.added.length | ||
| #: src/components/conversation/SelectAllConfirmationModal.tsx:452 | ||
| msgid "select.all.modal.added.count" | ||
| msgstr "{0} ajoutées" | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find SelectAllConfirmationModal.tsx to understand how the message is used
fd -t f "SelectAllConfirmationModal.tsx" --exec head -20 {}Repository: Dembrane/echo
Length of output: 331
🏁 Script executed:
# Search for the specific message ID to see how it's being called
rg "select.all.modal.added.count" -B 5 -A 5Repository: Dembrane/echo
Length of output: 50370
🏁 Script executed:
# Find Lingui configuration to understand pluralization support
fd -t f "lingui\.config\|\.linguirc\|babel\.config" -o fd -t f "package\.json" | head -5Repository: Dembrane/echo
Length of output: 180
🏁 Script executed:
# Check for lines 151-156 in the po file to confirm the same pattern
sed -n '151,156p' echo/frontend/src/locales/fr-FR.poRepository: Dembrane/echo
Length of output: 256
Verify pluralization handles singular counts correctly for added/not added strings.
Both msgstr "{0} ajoutées" and msgstr "{0} non ajoutées" are plural-only forms—displaying "1 ajoutées" would be grammatically wrong. Either the source message uses ICU pluralization (one/other forms), or the UI never displays count=1. LGTM once confirmed.
Also applies to: 151-156
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/fr-FR.po` around lines 136 - 141, The French
translations for the pluralized counts are currently using only the plural form
(e.g., msgid "select.all.modal.added.count" -> msgstr "{0} ajoutées"), which
will display incorrectly for singular (1); update the translations to use
ICU-style pluralization (one/other) for those message ids (at least
"select.all.modal.added.count" and the corresponding not-added id) so the UI
shows "1 ajoutée" vs "2+ ajoutées", or alternatively ensure the code paths that
call formatMessage never pass a value of 1 for these keys—modify either the .po
entries to include plural variants or change the message keys to ICU plural keys
used by SelectAllConfirmationModal and related components referenced in the
diff.
Comment on lines
+372
to
+376
| try: | ||
| logger.info( | ||
| f"Select All: Fetching conversations - " | ||
| f"project_id={project_id}, tags={body.tag_ids}, verified={body.verified_only}, search='{body.search_text}'" | ||
| ) |
Contributor
There was a problem hiding this comment.
Don’t log raw search text (PII risk).
Search queries can include names/emails; this shouldn’t land in logs. Log presence/length instead.
🔧 Safer logging
- logger.info(
- f"Select All: Fetching conversations - "
- f"project_id={project_id}, tags={body.tag_ids}, verified={body.verified_only}, search='{body.search_text}'"
- )
+ logger.info(
+ "Select All: Fetching conversations - project_id=%s, tags=%s, verified=%s, search_provided=%s",
+ project_id,
+ body.tag_ids,
+ body.verified_only,
+ bool(body.search_text),
+ )🤖 Prompt for AI Agents
In `@echo/server/dembrane/api/chat.py` around lines 372 - 376, The logger
currently emits raw search text (body.search_text) which may contain PII; update
the logging in the try block where logger.info is called to avoid logging the
raw query. Replace inclusion of body.search_text with a safe indicator such as
whether a search was provided and its length (e.g., search_present=True/False
and search_length=int) or a truncated/hashed token, while keeping project_id,
body.tag_ids and body.verified_only unchanged; edit the logger.info call that
formats "Select All: Fetching conversations - project_id=..., tags=...,
verified=..., search='...'" to use these safe fields instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@echo/frontend/src/locales/es-ES.po`:
- Around line 4036-4039: The msgstr for msgid "verified artifacts" is still in
English; update the translation in the es-ES.po entry (the entry referenced from
ConversationAccordion.tsx) to the Spanish equivalent, e.g., set msgstr to
"artefactos verificados" so the locale file uses Spanish for that key.
- Around line 3276-3279: Update the Spanish translation for the msgid
"select.all.modal.skip.disclaimer" to correct the misspelling: replace
"transcripto" with "transcripción" in the msgstr so it reads "Algunas
conversaciones pueden omitirse debido a un transcripción vacío o un límite de
contexto" (ensure correct gender/grammar if needed; the intended correct string
is "Algunas conversaciones pueden omitirse debido a una transcripción vacía o un
límite de contexto").
Comment on lines
+3276
to
+3279
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | ||
| msgid "select.all.modal.skip.disclaimer" | ||
| msgstr "Algunas conversaciones pueden omitirse debido a un transcripto vacío o un límite de contexto" | ||
|
|
Contributor
There was a problem hiding this comment.
Fix “transcripto” → “transcripción”.
Spanish term is off; this reads more natural and correct.
🛠️ Proposed fix
-msgstr "Algunas conversaciones pueden omitirse debido a un transcripto vacío o un límite de contexto"
+msgstr "Algunas conversaciones pueden omitirse debido a una transcripción vacía o un límite de contexto"📝 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.
Suggested change
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | |
| msgid "select.all.modal.skip.disclaimer" | |
| msgstr "Algunas conversaciones pueden omitirse debido a un transcripto vacío o un límite de contexto" | |
| #: src/components/conversation/SelectAllConfirmationModal.tsx:317 | |
| msgid "select.all.modal.skip.disclaimer" | |
| msgstr "Algunas conversaciones pueden omitirse debido a una transcripción vacía o un límite de contexto" |
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/es-ES.po` around lines 3276 - 3279, Update the
Spanish translation for the msgid "select.all.modal.skip.disclaimer" to correct
the misspelling: replace "transcripto" with "transcripción" in the msgstr so it
reads "Algunas conversaciones pueden omitirse debido a un transcripción vacío o
un límite de contexto" (ensure correct gender/grammar if needed; the intended
correct string is "Algunas conversaciones pueden omitirse debido a una
transcripción vacía o un límite de contexto").
Comment on lines
+4036
to
4039
| #: src/components/conversation/ConversationAccordion.tsx:579 | ||
| msgid "verified artifacts" | ||
| msgstr "verified artifacts" | ||
|
|
Contributor
There was a problem hiding this comment.
Translate “verified artifacts” to Spanish.
This one is still English; should match the rest of the locale.
🛠️ Proposed fix
-msgstr "verified artifacts"
+msgstr "artefactos verificados"📝 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.
Suggested change
| #: src/components/conversation/ConversationAccordion.tsx:579 | |
| msgid "verified artifacts" | |
| msgstr "verified artifacts" | |
| #: src/components/conversation/ConversationAccordion.tsx:579 | |
| msgid "verified artifacts" | |
| msgstr "artefactos verificados" | |
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/es-ES.po` around lines 4036 - 4039, The msgstr for
msgid "verified artifacts" is still in English; update the translation in the
es-ES.po entry (the entry referenced from ConversationAccordion.tsx) to the
Spanish equivalent, e.g., set msgstr to "artefactos verificados" so the locale
file uses Spanish for that key.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.