ECHO-702 Allow custom host prompts in verify flow#470
Merged
Conversation
- Added a new CustomTopicModal component for creating and editing custom verification topics. - Integrated modal functionality into ProjectPortalEditor for managing custom topics. - Implemented hooks for creating, updating, and deleting custom topics, enhancing project customization. - Updated UI components to support new topic label and icon handling, improving user experience. - Enhanced backend API to support custom topic creation and management, including validation and error handling.
Contributor
WalkthroughAdds custom verification-topic support: backend CRUD endpoints and topic_label propagation, frontend API/types and UI (CustomTopicModal, editor integration, label/icon fallback changes), Directus snapshot for topic_label, and localized strings across supported languages. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
LGTM. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
echo/frontend/src/components/project/hooks/index.ts (1)
419-495:⚠️ Potential issue | 🟡 MinorLocalize the new webhook error toasts.
Lines 419-495 add four user-facing fallback messages that bypass Lingui, so they’ll stay English in non-English dashboards. Wrap them in
tthe same way the new custom-topic toasts are wrapped.Diff
- const message = - error?.response?.data?.detail || "Failed to create webhook"; + const message = + error?.response?.data?.detail || t`Failed to create webhook`; - const message = - error?.response?.data?.detail || "Failed to update webhook"; + const message = + error?.response?.data?.detail || t`Failed to update webhook`; - const message = - error?.response?.data?.detail || "Failed to delete webhook"; + const message = + error?.response?.data?.detail || t`Failed to delete webhook`; - const message = error?.response?.data?.detail || "Failed to test webhook"; + const message = error?.response?.data?.detail || t`Failed to test webhook`;As per coding guidelines, "Translations must use
<Trans>component orttemplate literal in frontend React code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/components/project/hooks/index.ts` around lines 419 - 495, Update the user-facing fallback toast messages in the webhook mutation hooks to use Lingui localization: wrap each literal string passed to toast.error and toast.success inside the t template (e.g., in the onError handlers and onSuccess handlers of useCreateWebhookMutation, useUpdateWebhookMutation, useDeleteWebhookMutation, and useTestWebhookMutation), replacing plain English fallbacks like "Failed to create webhook", "Webhook created successfully", etc., with t`...` equivalents so the messages are translatable while keeping the existing error detail fallback logic (e.g., error?.response?.data?.detail || t`Failed to create webhook`).echo/frontend/src/locales/de-DE.po (1)
1698-1712:⚠️ Potential issue | 🟡 MinorSource copy still says “AI” here.
These newly extracted strings introduce platform copy that says
AI/KI, but the repo rule is to saylanguage model. Fix the source.tsxstring and rerun Lingui instead of patching this catalog directly.As per coding guidelines,
UI copy should be shortest possible with highest clarity; say "language model" not "AI" for platform features.
Based on learnings,.po files ... should not be manually edited.Also applies to: 4552-4554
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/locales/de-DE.po` around lines 1698 - 1712, The source strings in ProjectPortalEditor.tsx still use "AI"/"AI-gesteuerte" — update the user-facing string literals in ProjectPortalEditor.tsx (the messages that currently read "Enable this feature to allow participants to request AI-powered responses..." and the variants like "Get Reply"/"Go deeper") to use "language model" (and German equivalent if localized in source) instead of "AI"/"KI", then re-run Lingui extraction to regenerate the .po catalog instead of editing the .po file directly; also fix the other occurrence noted (the similar string around the other occurrence referenced in the review, e.g. the msgids corresponding to lines ~4552-4554) so all source copies use "language model" before re-extracting.echo/frontend/src/components/project/ProjectPortalEditor.tsx (2)
793-801:⚠️ Potential issue | 🟡 MinorReplace “AI” with “language model” in this new host copy.
These are new platform-facing strings, so they should follow the product wording convention here too.
As per coding guidelines, UI copy should be shortest possible with highest clarity; say "language model" not "AI" for platform features.
Also applies to: 965-971
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/components/project/ProjectPortalEditor.tsx` around lines 793 - 801, Update the platform-facing copy in ProjectPortalEditor (component ProjectPortalEditor.tsx) to replace the token "AI" with "language model" inside the Trans-wrapped strings used for the host-facing feature description (the Text element shown) and the second occurrence later (around the block at lines referenced 965-971); keep the rest of the sentence unchanged and ensure both Trans strings are updated so the UI uses "language model" instead of "AI".
1059-1067:⚠️ Potential issue | 🟠 MajorKeep
Add Topicreachable in the zero-topic state.When
availableVerifyTopics.length === 0, this branch only renders the empty message, because the create badge lives in the non-empty branch. That blocks the first custom topic from ever being created in the one state that most needs it.Also applies to: 1205-1232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/components/project/ProjectPortalEditor.tsx` around lines 1059 - 1067, The empty-state branch for verify topics only renders the "No verify topics available." message and omits the "Add Topic" create badge, preventing users from adding the first topic; update the rendering logic in ProjectPortalEditor so the create/add UI is always reachable: either render the create badge/button outside of the availableVerifyTopics length conditional (so it shows for both empty and non-empty states) or add the same create badge into the zero-length branch; touch the JSX that uses isVerificationTopicsLoading and availableVerifyTopics (the conditional that currently renders the empty message) and ensure the create control is present there (also apply the same change to the similar block around the later occurrence).echo/frontend/src/locales/it-IT.po (1)
579-620:⚠️ Potential issue | 🟠 MajorThe new custom-topic flow is still English in
it-IT.These added
msgstrvalues are untranslated, so Italian users will see English for create/edit/delete topic, prompt, and success/error feedback in the new verify flow. Please land this through the normal Lingui localization pipeline before merge rather than shipping fallback English. As per coding guidelinesAll .po translation files in frontend/src/locales/ must cover supported languages: en-US, nl-NL, de-DE, fr-FR, es-ES, it-IT, and based on learnings.po files (gettext internationalization files) are auto-generated by i18n tools like lingui/cli and should not be manually edited.Also applies to: 841-844, 1656-1658, 1823-1884, 2166-2172, 2284-2286, 2674-2676, 3694-3696, 4946-4960
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/locales/it-IT.po` around lines 579 - 620, The Italian .po entries for new custom-topic/verify strings are still in English (e.g., msgid "Add Custom Topic", "Add Topic", "Add translations", and strings from CustomTopicModal.tsx, ProjectPortalEditor.tsx, ProjectUploadSection.tsx, ProjectTagsInput.tsx, AddTagFilterModal.tsx, ConversationAccordion.tsx), which indicates the .po was manually edited; revert any manual changes and regenerate the locale files via the Lingui pipeline (run the i18n extraction/build commands used by the project) so the new keys are picked up and properly translated for it-IT (and ensure the auto-generated .po includes equivalents for all supported locales: en-US, nl-NL, de-DE, fr-FR, es-ES, it-IT) before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@echo/directus/sync/snapshot/fields/conversation_artifact/topic_label.json`:
- Around line 29-31: The DB column limits topic_label to 255 chars, so update
the API to enforce that before attempting to create an artifact: in
echo/server/dembrane/api/verify.py::_create_conversation_artifact validate the
incoming topic_label and either truncate it to 255 characters or return a 4xx
validation error; ensure the sanitized/trimmed value is what you pass to
client.create_item and add a clear error message when rejecting. Also consider
adding the same 255-char guard to the request/model layer so invalid values are
caught earlier.
In `@echo/frontend/src/components/project/CustomTopicModal.tsx`:
- Around line 35-47: The modal lacks an inline error channel—update the
CustomTopicModalProps to accept an error?: string | null (or similar) prop and,
inside the CustomTopicModal component (where the modal body/form is rendered),
render a Mantine Alert (e.g., <Alert color="red" icon={<IconAlertCircle
/>}>error</Alert>) above the form fields when error is truthy; ensure the Alert
shows the provided error text and keep existing toast behavior so failures show
both a toast and this contextual inline Alert for create/update flows.
In `@echo/frontend/src/components/project/ProjectPortalEditor.tsx`:
- Around line 427-445: The confirmDeleteCustomTopic function needs a local
try/catch around deleteCustomTopicMutation.mutateAsync so failures are handled
inline: wrap the mutateAsync call in try/catch, on success carry on updating
verification_topics and call setDeleteConfirmKey(null) as now, but on catch do
not clear deleteConfirmKey and instead set a local error state (e.g.,
deleteTopicError via useState) with the error message and keep calling the
existing toast/onError; then render a Mantine Alert in the delete confirmation
modal using that deleteTopicError to show contextual inline feedback (follow the
same pattern used by handleCustomTopicSubmit to show the Alert and preserve the
open modal on error).
- Around line 1687-1692: The memo comparator for the ProjectPortalEditor
component is incorrectly only comparing prevProps.project.id, which prevents
updates when the project object changes but retains the same id; update the
comparator used in the React.memo call (the function that returns
prevProps.project.id === nextProps.project.id ...) to compare the full project
reference (e.g., prevProps.project === nextProps.project) or perform a
shallow/deep compare of relevant project fields instead, while keeping the
existing checks for verificationTopics and isVerificationTopicsLoading so the
component re-renders when the project object itself changes.
- Around line 1161-1200: The Tooltip currently wraps a disabled ActionIcon so it
never opens; move the Tooltip to wrap a neutral wrapper element (e.g., a <div>
or <span>) that contains the ActionIcon instead. Keep the same enable/disable
logic (use the existing watchedVerifyEnabled and field.value checks) but apply
them to Tooltip's disabled prop and to the ActionIcon's disabled prop separately
so hover/focus events fire on the wrapper while the ActionIcon remains
non-interactive; keep the existing onClick handler that calls
setDeleteConfirmKey(topic.key) and preserve
testId(`custom-topic-delete-${topic.key}`) and IconTrash usage.
In `@echo/frontend/src/locales/es-ES.po`:
- Around line 709-711: The translated string for the email toggle ("Ask for
Email?") and its dependency hint must be identical; update the msgstr for the
dependency hint entry that references the same functionality so it matches
"¿Preguntar por el email?" exactly (also apply the same change to the other
occurrence noted around the second location for lines 3701-3703). Locate the
entries by msgid "Ask for Email?" and the dependent hint msgid and make their
msgstr values identical.
- Around line 1687-1689: The Spanish translation uses "reportes" but the locale
consistently uses "informe"; update the msgstr for the msgid "Enable Report
Notifications" to use "informe" (e.g., "Habilitar notificaciones de informe")
and make the same consistent replacement for the other occurrence mentioned (the
duplicate block that also uses "reportes"); ensure capitalization/accents match
existing locale style and mirror the original msgid casing.
- Around line 3923-3925: The Spanish translation for the delete guard is
grammatically wrong; update the msgstr for the msgid "Select at least one other
topic before deleting this one" (used in ProjectPortalEditor.tsx) to a correct
phrase such as "Selecciona al menos otro tema antes de eliminar este" (or
"Selecciona al menos un tema más antes de eliminar este") so the plural/article
usage is fixed in the locales entry.
- Around line 5214-5216: The Spanish translation in the msgstr for the
anonymization description has agreement mistakes: change "tendrán información
personal ... reemplazadas" to use singular feminine agreement ("reemplazada") to
match "información", and fix "el descarga de audio" to "la descarga de audio";
update the msgstr entry corresponding to msgid in ProjectPortalEditor.tsx so the
string reads with those corrected forms (keep the rest of the sentence
unchanged).
- Around line 815-817: The Spanish translation in the msgstr for the string from
ProjectPortalEditor.tsx uses the wrong article ("después de la resumen"); update
the msgstr to use the correct masculine article and contraction ("después del
resumen") and ensure the sentence reads smoothly — e.g., replace "después de la
resumen. El título..." with "después del resumen. El título..." and optionally
normalize participant phrasing to singular ("si lo proporcionó") to match "el
participante"; edit the msgstr entry accordingly.
- Around line 4555-4558: Fix the Spanish translation for the msgid used in
ProjectPortalEditor.tsx by replacing the awkward “Esta prompt / la prompt” and
“formar el tipo” with correct gender/agreement and a better verb; update the
msgstr corresponding to that msgid so it reads something like: use “Este prompt
guía cómo la IA responde a los participantes. Personaliza este prompt para
moldear el tipo de retroalimentación o participación que quieres fomentar.” This
ensures masculine agreement for “prompt” and replaces “formar” with “moldear.”
- Around line 491-493: The Spanish .po entries in ProjectPortalEditor.tsx
contain mixed English ("best practices" and "brainstorming") in msgstr values;
update the msgstr for msgid "Advanced (Tips and best practices)" to use fully
Spanish text (e.g., "Avanzado (Consejos y mejores prácticas)") and also find the
other affected msgid around lines referenced (the "brainstorming" label) and
replace with a Spanish equivalent (e.g., "lluvia de ideas") so all labels are
fully localized.
In `@echo/frontend/src/locales/fr-FR.po`:
- Around line 4533-4535: Update the French translations for the specific msgid
strings to correct grammar and natural phrasing: for msgid "This is a live
preview of the participant's portal. You will need to refresh the page to see
the latest changes." replace the msgstr with a grammatically correct French
sentence (e.g., "Ceci est un aperçu en direct du portail du participant. Vous
devrez actualiser la page pour voir les dernières modifications."); similarly
locate and fix the other problematic msgids referenced (the ones matching "This
prompt" and phrases about "all new transcriptions"/"retranscribe it") at the
occurrences noted (around the blocks near msgids at lines 4570-4572 and
5233-5235) and provide natural French equivalents (e.g., "Cette invite" ->
"Cette invite" only if appropriate or better context-specific wording, and "tous
les nouveaux transcriptions"/"la rétranscrire" -> "toutes les nouvelles
transcriptions"/"la retranscrire") ensuring gender/number agreement and fluent
phrasing.
- Around line 1571-1574: The French translation of "noun phrases" is incorrect:
replace "phrases adjectivales" with the correct term such as "groupes nominaux"
(or "phrases nominales") in the msgstr for the msgid shown in
ProjectPortalEditor (msgid: e.g. "Use short noun phrases like 'Urban Green
Spaces' or 'Youth Employment'. Avoid generic titles."); update the msgstr to
read something like: "par exemple : \"Utiliser des groupes nominaux courts comme
'Espaces verts urbains' ou 'Emploi des jeunes'. Éviter les titres
génériques.\"".
In `@echo/frontend/src/locales/nl-NL.po`:
- Around line 4073-4076: The Dutch translation uses formal "u/uw" and must be
changed to informal "je/jij" via the localization workflow rather than editing
generated .po files directly: update the nl-NL translation entry for the msgid
"Select at least one other topic before deleting this one" in the translation
platform to use informal phrasing (e.g., "Selecteer ten minste één ander
onderwerp voordat je dit verwijdert"), and fix the other affected msgids flagged
in the comment (the entries around the other reported msgids) so all portal
strings from ProjectPortalEditor
(src/components/project/ProjectPortalEditor.tsx) use "je/jij" consistently
before regenerating nl-NL.po.
- Around line 1501-1504: The Dutch translation uses the incorrect article "de"
for "taalmodel"; update the upstream source translation (the string used in
CustomTopicModal.tsx where the msgid is "Describe what the language model should
extract or summarize from the conversation...") to use "het taalmodel" instead
of "de taalmodel", then re-run your i18n extraction/compilation step to
regenerate nl-NL.po rather than editing the .po file directly so the corrected
phrase propagates into the generated catalog.
- Around line 4828-4831: The Dutch translation for msgid "Topic label" in the
generated .po currently uses a literal split form "Onderwerp label"; update the
upstream source/translation entry (not the generated
echo/frontend/src/locales/nl-NL.po) so the translator string becomes a natural
Dutch compound like "Onderwerplabel" (or another idiomatic alternative) and then
regenerate the i18n catalog so CustomTopicModal (msgid "Topic label" referenced
in CustomTopicModal.tsx) picks up the corrected translation automatically.
In `@echo/server/dembrane/api/verify.py`:
- Around line 393-432: The code only updates translations when body.translations
is not None, so a PATCH with only body.label is a no-op; change logic to ensure
body.label is synced into the "en-US" translation even when body.translations is
None by normalizing body_translations = dict(body.translations) if present else
{} and then setting body_translations["en-US"] = body.label when body.label is
not None (trim label), then run the existing loop that builds
translation_updates/translation_creates using
existing_translations/existing_by_lang and finally attach translations_nested to
topic_updates["translations"] if non-empty; update references: body.translations
-> body_translations, existing_translations, existing_by_lang,
translation_updates, translation_creates, translations_nested, topic_updates.
- Around line 482-496: When removing a topic from
selected_verification_key_list, ensure we never write an empty selection: after
computing key_list/updated_keys from existing_keys (variable names
existing_keys, key_list, updated_keys) check if updated_keys would be None/empty
and if project_check indicates there are other topics; in that case reject the
request (raise an HTTP 400/ValueError) or preserve the current selection instead
of writing None. Make this check before calling client.update_item (the call
using client.update_item, project_id) so the API enforces the "keep one selected
topic" invariant rather than allowing an empty selection that
_parse_selected_topics() later expands.
- Around line 918-928: The code currently resolves resolved_label using
target_topic.translations.get("en-US") which forces English; change it to look
up the locale provided by the request (e.g., body.locale or request_locale) and
fall back to "en-US" only if that locale translation is missing. Update the
resolved_label computation to use target_topic.translations.get(<request
locale>, VerificationTopicTranslation(label="")).label (or equivalent) and pass
that into _create_conversation_artifact via topic_label so the persisted
artifact uses the localized label rather than a hard-coded English one.
- Around line 333-344: The code collapses a NULL
"selected_verification_key_list" (meaning “all topics”) into a CSV string by
using `project.get("selected_verification_key_list") or ""`; change the logic to
preserve NULL: read `existing_keys =
project.get("selected_verification_key_list")` without the `or ""`, and only
compute/`client.update_item` with a new CSV when `existing_keys` is not None
(i.e., an explicit selection); if `existing_keys` is None, skip updating
`selected_verification_key_list` so `_parse_selected_topics()` retains the
implicit “all topics” behavior.
---
Outside diff comments:
In `@echo/frontend/src/components/project/hooks/index.ts`:
- Around line 419-495: Update the user-facing fallback toast messages in the
webhook mutation hooks to use Lingui localization: wrap each literal string
passed to toast.error and toast.success inside the t template (e.g., in the
onError handlers and onSuccess handlers of useCreateWebhookMutation,
useUpdateWebhookMutation, useDeleteWebhookMutation, and useTestWebhookMutation),
replacing plain English fallbacks like "Failed to create webhook", "Webhook
created successfully", etc., with t`...` equivalents so the messages are
translatable while keeping the existing error detail fallback logic (e.g.,
error?.response?.data?.detail || t`Failed to create webhook`).
In `@echo/frontend/src/components/project/ProjectPortalEditor.tsx`:
- Around line 793-801: Update the platform-facing copy in ProjectPortalEditor
(component ProjectPortalEditor.tsx) to replace the token "AI" with "language
model" inside the Trans-wrapped strings used for the host-facing feature
description (the Text element shown) and the second occurrence later (around the
block at lines referenced 965-971); keep the rest of the sentence unchanged and
ensure both Trans strings are updated so the UI uses "language model" instead of
"AI".
- Around line 1059-1067: The empty-state branch for verify topics only renders
the "No verify topics available." message and omits the "Add Topic" create
badge, preventing users from adding the first topic; update the rendering logic
in ProjectPortalEditor so the create/add UI is always reachable: either render
the create badge/button outside of the availableVerifyTopics length conditional
(so it shows for both empty and non-empty states) or add the same create badge
into the zero-length branch; touch the JSX that uses isVerificationTopicsLoading
and availableVerifyTopics (the conditional that currently renders the empty
message) and ensure the create control is present there (also apply the same
change to the similar block around the later occurrence).
In `@echo/frontend/src/locales/de-DE.po`:
- Around line 1698-1712: The source strings in ProjectPortalEditor.tsx still use
"AI"/"AI-gesteuerte" — update the user-facing string literals in
ProjectPortalEditor.tsx (the messages that currently read "Enable this feature
to allow participants to request AI-powered responses..." and the variants like
"Get Reply"/"Go deeper") to use "language model" (and German equivalent if
localized in source) instead of "AI"/"KI", then re-run Lingui extraction to
regenerate the .po catalog instead of editing the .po file directly; also fix
the other occurrence noted (the similar string around the other occurrence
referenced in the review, e.g. the msgids corresponding to lines ~4552-4554) so
all source copies use "language model" before re-extracting.
In `@echo/frontend/src/locales/it-IT.po`:
- Around line 579-620: The Italian .po entries for new custom-topic/verify
strings are still in English (e.g., msgid "Add Custom Topic", "Add Topic", "Add
translations", and strings from CustomTopicModal.tsx, ProjectPortalEditor.tsx,
ProjectUploadSection.tsx, ProjectTagsInput.tsx, AddTagFilterModal.tsx,
ConversationAccordion.tsx), which indicates the .po was manually edited; revert
any manual changes and regenerate the locale files via the Lingui pipeline (run
the i18n extraction/build commands used by the project) so the new keys are
picked up and properly translated for it-IT (and ensure the auto-generated .po
includes equivalents for all supported locales: en-US, nl-NL, de-DE, fr-FR,
es-ES, it-IT) before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f12f09f0-92f0-44e7-9101-efb0490b1816
📒 Files selected for processing (23)
echo/directus/sync/snapshot/fields/conversation_artifact/conversation_id.jsonecho/directus/sync/snapshot/fields/conversation_artifact/topic_label.jsonecho/frontend/src/components/conversation/VerifiedArtefactsSection.tsxecho/frontend/src/components/participant/verify/VerifiedArtefactsList.tsxecho/frontend/src/components/participant/verify/VerifySelection.tsxecho/frontend/src/components/project/CustomTopicModal.tsxecho/frontend/src/components/project/ProjectPortalEditor.tsxecho/frontend/src/components/project/hooks/index.tsecho/frontend/src/lib/api.tsecho/frontend/src/lib/typesDirectus.d.tsecho/frontend/src/locales/de-DE.poecho/frontend/src/locales/de-DE.tsecho/frontend/src/locales/en-US.poecho/frontend/src/locales/en-US.tsecho/frontend/src/locales/es-ES.poecho/frontend/src/locales/es-ES.tsecho/frontend/src/locales/fr-FR.poecho/frontend/src/locales/fr-FR.tsecho/frontend/src/locales/it-IT.poecho/frontend/src/locales/it-IT.tsecho/frontend/src/locales/nl-NL.poecho/frontend/src/locales/nl-NL.tsecho/server/dembrane/api/verify.py
echo/directus/sync/snapshot/fields/conversation_artifact/topic_label.json
Show resolved
Hide resolved
spashii
approved these changes
Mar 6, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
echo/server/dembrane/api/verify.py (1)
233-242:⚠️ Potential issue | 🟠 MajorDon't leak host prompts from the public topics endpoint.
This route has no auth dependency, but
available_topicsincludesprompt. After this PR, anyone with aproject_idcan read the host-authored custom verification instructions. Either gate this endpoint withDependencyDirectusSessionor strippromptfrom the public response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/verify.py` around lines 233 - 242, The public GET handler get_verification_topics currently returns available_topics that include the host-authored `prompt` field (from `_get_verification_topics_for_project`), leaking sensitive host prompts since the route has no auth; fix by either adding the `DependencyDirectusSession` dependency to the route decorator or (preferred minimal change) sanitize `topics` before returning by removing or blanking the `prompt` key on each topic object (ensure `_parse_selected_topics` still receives needed fields or sanitize a copy), so the returned `GetVerificationTopicsResponse.available_topics` never contains `prompt`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@echo/server/dembrane/api/verify.py`:
- Around line 308-325: Incoming topic label and prompt are not trimmed or
validated before persistence (see slug = _slugify(body.label),
translations_payload, and client.create_item for "verification_topic"), which
allows whitespace-only values that break later logic; trim body.label,
body.prompt, and each translation value, then validate they are non-empty
(reject with a user-facing error/HTTP 4xx on create and update) before calling
client.create_item or update flows so no blank/whitespace-only labels or prompts
are saved.
- Around line 319-343: The current two-step flow
(run_in_thread_pool(client.create_item, ...) then reading and rewriting
project.selected_verification_key_list) is subject to last-writer-wins races and
duplicate topic_key creation; change to an atomic/idempotent update: avoid
reading the stale CSV and instead perform a single server-side update that
appends topic_key only if missing (or use a transactional/upsert API) so
creation of the "verification_topic" and updating the project's
selected_verification_key_list are done safely — e.g., use a single server
transaction or an atomic update operator on the project record (referencing
client.create_item, client.update_item, selected_verification_key_list,
topic_key, and run_in_thread_pool) to ensure idempotency and no lost concurrent
updates.
---
Outside diff comments:
In `@echo/server/dembrane/api/verify.py`:
- Around line 233-242: The public GET handler get_verification_topics currently
returns available_topics that include the host-authored `prompt` field (from
`_get_verification_topics_for_project`), leaking sensitive host prompts since
the route has no auth; fix by either adding the `DependencyDirectusSession`
dependency to the route decorator or (preferred minimal change) sanitize
`topics` before returning by removing or blanking the `prompt` key on each topic
object (ensure `_parse_selected_topics` still receives needed fields or sanitize
a copy), so the returned `GetVerificationTopicsResponse.available_topics` never
contains `prompt`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b91fbb6-d685-49a9-816f-e28fa7c5d0d3
📒 Files selected for processing (1)
echo/server/dembrane/api/verify.py
Comment on lines
+308
to
+325
| slug = _slugify(body.label) | ||
| short_id = generate_uuid()[:8] | ||
| topic_key = f"{slug}-{short_id}" | ||
|
|
||
| translations_payload = [ | ||
| {"languages_code": "en-US", "label": body.label}, | ||
| ] | ||
| for lang_code, label_text in body.translations.items(): | ||
| if lang_code != "en-US" and label_text and label_text.strip(): | ||
| translations_payload.append({"languages_code": lang_code, "label": label_text.strip()}) | ||
|
|
||
| await run_in_thread_pool( | ||
| client.create_item, | ||
| "verification_topic", | ||
| item_data={ | ||
| "key": topic_key, | ||
| "prompt": body.prompt, | ||
| "icon": body.icon or None, |
Contributor
There was a problem hiding this comment.
Reject blank topic text before persisting it.
body.label and body.prompt are written raw here. A whitespace-only label/prompt produces a broken custom topic, and an empty prompt later hard-fails /generate at Line 831. Trim and validate non-empty on create/update so the API cannot store unusable topics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/verify.py` around lines 308 - 325, Incoming topic
label and prompt are not trimmed or validated before persistence (see slug =
_slugify(body.label), translations_payload, and client.create_item for
"verification_topic"), which allows whitespace-only values that break later
logic; trim body.label, body.prompt, and each translation value, then validate
they are non-empty (reject with a user-facing error/HTTP 4xx on create and
update) before calling client.create_item or update flows so no
blank/whitespace-only labels or prompts are saved.
Comment on lines
+319
to
+343
| await run_in_thread_pool( | ||
| client.create_item, | ||
| "verification_topic", | ||
| item_data={ | ||
| "key": topic_key, | ||
| "prompt": body.prompt, | ||
| "icon": body.icon or None, | ||
| "project_id": project_id, | ||
| "translations": { | ||
| "create": translations_payload, | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| existing_keys = project.get("selected_verification_key_list") | ||
| if existing_keys: | ||
| key_list = [k.strip() for k in existing_keys.split(",") if k.strip()] | ||
| if topic_key not in key_list: | ||
| key_list.append(topic_key) | ||
| await run_in_thread_pool( | ||
| client.update_item, | ||
| "project", | ||
| project_id, | ||
| {"selected_verification_key_list": ",".join(key_list)}, | ||
| ) |
Contributor
There was a problem hiding this comment.
This two-step write is still last-writer-wins.
The topic row is created at Line 319, then selected_verification_key_list is rewritten from the project snapshot loaded earlier at Line 305. Overlapping create/delete requests on the same project will clobber each other, and a retry after a partial failure can create a duplicate topic because topic_key is regenerated each time. This needs an atomic/idempotent update path, not a stale CSV read-modify-write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/verify.py` around lines 319 - 343, The current
two-step flow (run_in_thread_pool(client.create_item, ...) then reading and
rewriting project.selected_verification_key_list) is subject to last-writer-wins
races and duplicate topic_key creation; change to an atomic/idempotent update:
avoid reading the stale CSV and instead perform a single server-side update that
appends topic_key only if missing (or use a transactional/upsert API) so
creation of the "verification_topic" and updating the project's
selected_verification_key_list are done safely — e.g., use a single server
transaction or an atomic update operator on the project record (referencing
client.create_item, client.update_item, selected_verification_key_list,
topic_key, and run_in_thread_pool) to ensure idempotency and no lost concurrent
updates.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Improvements