Conversation
Co-authored-by: Sameer Pashikanti <sameer@dembrane.com>
…ements (#510) …cleanup on avatar/logo removal - Replace all window.confirm() calls with ConfirmModal across 12 components: ChatAccordion (delete chat), ConversationDangerZone (delete conversation), ProjectDangerZone (delete project — now 2-step confirmation), ProjectPortalEditor (disable anonymization, delete custom topic), ProjectTagsInput (delete tag), LanguagePicker (change language during chat), ProjectConversationOverview (regenerate summary), ProjectLibrary (generate library), ProjectReportRoute (delete report), TemplatesModal (delete template), AccountSettingsCard (remove avatar), WhitelabelLogoCard (remove logo) - Replace window.prompt() with InputModal in ChatAccordion (rename chat) - Replace window.alert() / native alert() with toast notifications: PermissionErrorModal (microphone denied), VerifyEmail (invalid token), useCopyToRichText (clipboard failure) - Add reusable ConfirmModal and InputModal components to common/ - Align template edit view buttons: Delete as subtle red button + Save as primary, in a right-aligned Group (matching ConfirmModal layout) - Backend: delete orphaned Directus files when removing avatar or whitelabel logo (previously files were left behind) - Backend: wrap blocking Directus calls in run_in_thread_pool for async safety - Fix MarkdownWYSIWYG toolbar position to use CSS variable instead of hardcoded sticky, and set it from report editor context - Minor formatting cleanups (line length, import ordering) - Doc updates for future references
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (12)
⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughMigrates quick‑access prefs from per‑item Directus collections to a user JSON field; removes community marketplace and rating APIs/UI; adds transcript anonymization flag and UX; replaces browser blocking dialogs with reusable Confirm/Input modals; updates Directus sync snapshots/operations and many localization entries. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
LGTM. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
echo/server/dembrane/api/project_webhook.py (1)
142-155: 🧹 Nitpick | 🔵 TrivialAdd warning log when
webhooksis not a list.Defensive check is solid, but you're silently swallowing unexpected return types. For observability, add a warning like you did in
project.py:122-124.🔧 Proposed fix
if not isinstance(webhooks, list): + logger.warning("get_items returned non-list for webhooks: %s", webhooks) webhooks = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/project_webhook.py` around lines 142 - 155, The code silently replaces a non-list `webhooks` with an empty list; add a warning log when that happens. In the function returning the list of WebhookResponseSchema objects, detect when `webhooks` is not a list and call the module logger (or the same logger used in project.py) to emit a warning noting the unexpected type/value of `webhooks` before assigning `webhooks = []`; reference the `webhooks` variable and the surrounding list-comprehension that builds `WebhookResponseSchema(...)` (and keep `_parse_webhook_events` usage unchanged).echo/frontend/src/hooks/useCopyToRichText.ts (1)
53-66:⚠️ Potential issue | 🟡 MinorPotential race:
setCopied(true)called before clipboard write completesThe
setCopied(true)on line 65 fires immediately afternavigator.clipboard.write()is called, not after it succeeds. This meanscopiedbecomestrueeven if the write fails, causing a flash of "copied" feedback followed by an error toast.The success callback on line 55 already sets
setCopied(true), so line 65 appears redundant and creates a race condition.🔧 Proposed fix
navigator.clipboard.write(data).then( () => { setCopied(true); }, (_e) => { navigator.clipboard.write([fallBackData]).catch((e) => { console.error("Rich text copy failed:", e); toast.error(t`Could not copy to clipboard. Please try again.`); }); }, ); - - setCopied(true); }, [setCopied], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/hooks/useCopyToRichText.ts` around lines 53 - 66, The call to setCopied(true) after invoking navigator.clipboard.write() creates a race because it runs before the write completes; remove the trailing setCopied(true) outside the promise and only setCopied(true) inside the successful write handler(s). Specifically, delete the redundant setCopied(true) after navigator.clipboard.write(...) and ensure the success callback for navigator.clipboard.write and the fallback write([fallBackData]) success path (or their .then handlers) setCopied(true) while the error paths call toast.error(t`Could not copy to clipboard. Please try again.`) and log the error.echo/frontend/src/routes/project/chat/ProjectChatRoute.tsx (1)
357-388:⚠️ Potential issue | 🟠 MajorDon’t let stale quick-access prefs collapse the rail to empty.
With the new JSON-backed
{ type, id }shape, stale entries are expected during migration and after template deletion. If every saved item misses, this memo returns[]because onlydata.length === 0hits the fallback path, so the quick-access shortcuts disappear for existing users until they manually rebuild them.💡 Tight fix
const quickAccessItems: QuickAccessItem[] = useMemo(() => { - if (!quickAccessQuery.data || quickAccessQuery.data.length === 0) - return Templates.slice(0, 3).map((t) => ({ + const fallback = Templates.slice(0, 3).map((t) => ({ + type: "static" as const, + id: t.id, + title: t.title, + })); + + if (!quickAccessQuery.data || quickAccessQuery.data.length === 0) { + return fallback; + } + + const resolved = quickAccessQuery.data + .map((pref) => { + if (pref.type === "static") { + const found = Templates.find((t) => t.id === pref.id); + if (found) + return { + type: "static" as const, + id: found.id, + title: found.title, + }; + } else if (pref.type === "user") { + const found = userTemplatesQuery.data?.find((t) => t.id === pref.id); + if (found) + return { + type: "user" as const, + id: found.id, + title: found.title, + }; + } + return null; + }) + .filter(Boolean) as QuickAccessItem[]; + + if (resolved.length > 0 || userTemplatesQuery.isLoading) { + return resolved; + } + + return fallback; - type: "static" as const, - id: t.id, - title: t.title, - })); - return quickAccessQuery.data - .map((pref) => { - if (pref.type === "static") { - const found = Templates.find((t) => t.id === pref.id); - if (found) - return { - type: "static" as const, - id: found.id, - title: found.title, - }; - } else if (pref.type === "user") { - const found = userTemplatesQuery.data?.find( - (t) => t.id === pref.id, - ); - if (found) - return { - type: "user" as const, - id: found.id, - title: found.title, - }; - } - return null; - }) - .filter(Boolean) as QuickAccessItem[]; -}, [quickAccessQuery.data, userTemplatesQuery.data]); +}, [quickAccessQuery.data, userTemplatesQuery.data, userTemplatesQuery.isLoading]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/routes/project/chat/ProjectChatRoute.tsx` around lines 357 - 388, The quickAccessItems memo can end up as [] when quickAccessQuery.data exists but all entries are stale; update the logic in the quickAccessItems useMemo (the mapping over quickAccessQuery.data and use of Templates) to first map/filter into a temporary array and then if that resulting array is empty fall back to Templates.slice(0,3); in other words, keep the current mapping that checks pref.type and looks up Templates or userTemplatesQuery.data, but after mapping call if (mapped.length === 0) return Templates.slice(0, 3). This ensures the rail shows default static templates when saved prefs are all invalid.
🤖 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/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.md`:
- Around line 273-275: The plan currently lists
directus_users.quick_access_preferences as a manual prerequisite which will
break fresh environments and the /templates/quick-access flow; update the plan
to include a versioned Directus schema change (a migration or Directus schema
snapshot) as a concrete step—e.g., add a migration script or exported Directus
schema JSON that creates the quick_access_preferences JSON field (nullable,
default []), document how to apply it during setup, and reference this step in
Task 3 and the same checklist entries that mention the prerequisite so
environments are reproducible rather than relying on manual admin changes.
- Around line 535-542: The provided sample translations for the key
participant.anonymization.notice contain missing diacritics/umlauts and will
propagate broken strings if copy‑pasted; update each locale example (en-US.po,
nl-NL.po, de-DE.po, fr-FR.po, es-ES.po, it-IT.po) to use the correct accented
characters/umlauts (e.g., "anhoren" → "anhören", "anonymisee" → "anonymisée",
etc.) so the paste-ready .po entries are valid; apply the same corrections to
the similar block referenced later to ensure all example strings include proper
diacritics.
- Around line 24-78: The change hardcodes Mantine's "blue" for buttons, which
breaks the app theme abstraction; instead update the components
CreateReportForm, UpdateReportModalButton, and ProjectReportRoute to use the
shared brand color token from the frontend theme (the same token used elsewhere
in the repo) for the color prop and Switch variant—replace occurrences of
color="blue" with the theme token access used by your codebase (e.g., via
useTheme()/getToken()/theme.colors['<brand-token>'] or the shared constant
imported from brand/colors.json) so the components (Schedule Report, Generate
now, Update Report trigger, Confirm reschedule button and Publish Switch) follow
the centralized palette.
- Around line 626-632: Remove the direct mutation of the React Hook Form
internals: delete the assignment to control._formValues.anonymize_transcripts in
the Button onClick handler and rely solely on the public API call
setValue("anonymize_transcripts", false, { shouldDirty: true }); keep the call
to closeAnonymizeModal() unchanged; this prevents touching the private
control._formValues implementation and uses the supported setValue method to
update state and mark the form dirty.
In
`@echo/docs/superpowers/specs/2026-04-03-template-community-report-anonymization-design.md`:
- Line 20: The spec currently leaves where to store quick_access_preferences
ambiguous; update the spec to explicitly state the field is added to the
directus_users collection (or name the exact user settings collection ops should
create) so reads/writes match the implementation that uses directus.update_user
in echo/server/dembrane/api/template.py; ensure any other references (e.g., the
quick-access endpoints and the create/update paths) are updated to target the
same collection name `quick_access_preferences` on `directus_users`.
- Around line 22-29: The Markdown code fence under the "**Schema**:" heading is
missing the required blank lines that trigger MD031; add a blank line both
before the opening ```json fence and after the closing ``` fence around the JSON
block (the block containing [{"type": "static", "id": "summarize"}, ... {"type":
"user", "id": "abc-123"}]) so the schema code fence is separated from
surrounding text and passes markdownlint-cli2.
In `@echo/frontend/src/components/chat/ChatAccordion.tsx`:
- Around line 181-188: The current onConfirm handler calls
deleteChatMutation.mutate(...) and immediately calls navigate, which redirects
even if deletion fails; change it to wait for the mutation to succeed before
navigating: either call deleteChatMutation.mutate with an onSuccess callback
that performs navigate(`/projects/${chat.project_id}/overview`) and
closeDeleteConfirm(), or use deleteChatMutation.mutateAsync(...) with await and
then call navigate and closeDeleteConfirm() inside the success path; keep error
handling for failure (e.g., show toast) and reference the existing
deleteChatMutation, mutate/mutateAsync, onConfirm, navigate, and
closeDeleteConfirm symbols.
In `@echo/frontend/src/components/common/ConfirmModal.tsx`:
- Line 10: The ConfirmModal prop "message" is declared as a generic ReactNode
but is always wrapped in a Text component (see ConfirmModal and the Text usage),
which breaks when callers pass block nodes; change the rendering to respect the
contract by only wrapping primitive strings/numbers in <Text> and rendering
non-primitive React nodes directly: update ConfirmModal to check typeof message
=== 'string' || typeof message === 'number' (or use a small util) and render
<Text>{message}</Text> for primitives, otherwise return message as-is so callers
can pass block elements safely.
In `@echo/frontend/src/components/common/InputModal.tsx`:
- Around line 34-38: In InputModal, the current useEffect resets value whenever
initialValue changes even while the modal is open; change the effect to only
reset on the closed→open edge by running the effect when opened changes and
calling setValue(initialValue) only when opened is true (i.e., depend on opened,
not initialValue) so in useEffect check if (opened) setValue(initialValue) with
[opened] as the dependency list.
In `@echo/frontend/src/components/conversation/ConversationDangerZone.tsx`:
- Around line 81-99: The confirm flow currently triggers navigate and
closeConfirm immediately and allows duplicate submits; update the ConfirmModal
onConfirm handler to i) prevent double-submit by checking or using
deleteConversationByIdMutation.isLoading (or pass a prop like
confirmDisabled/confirmLoading) so the confirm button is disabled while the
mutation is pending, and ii) wait for the server delete to succeed before
navigating/closing (use deleteConversationByIdMutation.mutateAsync or move
navigate(`/projects/${projectId}/overview`) and closeConfirm() into the
mutation's onSuccess callback), and handle errors in onError (log/show error and
keep the modal open or re-enable confirm).
In `@echo/frontend/src/components/layout/Header.tsx`:
- Around line 214-223: The Menu.Item elements in Header.tsx that open external
links with target="_blank" (e.g., the Menu.Item using docUrl with leftSection
IconNotes and rightSection IconExternalLink and any other Menu.Item instances
referenced at the other locations) need to include rel="noopener noreferrer" to
prevent tabnabbing and isolate the opener; update each Menu.Item (and any
anchor-like components that use target="_blank") to add rel="noopener
noreferrer" alongside target="_blank" and preserve existing props such as href
and testId.
- Line 254: The Badge component in Header.tsx currently uses a hardcoded color
attribute (color="blue"); replace this with the approved brand token or CSS
variable so the tokenized theme system is respected—update the Badge usage to
reference the brand color token from brand/colors.json (or the CSS variable like
--app-accent/--app-brand) instead of "blue" and remove the hardcoded string so
theme changes propagate dynamically; ensure you modify the Badge prop (color) or
its style wrapper in Header.tsx accordingly to consume the token/variable.
In `@echo/frontend/src/components/participant/ParticipantBody.tsx`:
- Around line 194-203: The markdown prop in ParticipantBody.tsx duplicates most
of the localized string across two t() calls; refactor to create a single base
localized message (use t in one template string) and then conditionally append
the anonymization sentence when isAnonymized is true before passing it to the
markdown prop (keep references to the markdown prop, the isAnonymized boolean,
and the t localization function). Ensure only one core t() string is used so
translators maintain a single source and append the extra anonymization line
programmatically.
In `@echo/frontend/src/components/project/ProjectDangerZone.tsx`:
- Around line 71-79: The handleDelete function currently calls
deleteProjectByIdMutation.mutate(...) and immediately navigates away; change it
to wait for the mutation to complete before calling navigate("/projects"). Use
deleteProjectByIdMutation.mutateAsync(project.id) and await it (making
handleDelete async) or pass an onSuccess callback to
deleteProjectByIdMutation.mutate to call navigate only on success; ensure errors
from mutate/mutateAsync are caught so you don't navigate on failure and the
confirmation modal's loading state can be handled via the mutation's
isLoading/isError states (references: handleDelete, deleteProjectByIdMutation,
navigate).
In `@echo/frontend/src/components/project/ProjectPortalEditor.tsx`:
- Around line 1534-1543: The onConfirm handler is double-triggering autosave by
calling setValue("anonymize_transcripts", false, ...) which already causes the
form-wide watch autosave, and then manually calling dispatchAutoSave(...);
remove the explicit dispatchAutoSave(...) call from the onConfirm block (leaving
setValue and anonymizeModalHandlers.close) so the change is saved only once via
the existing watch/autosave flow; reference: setValue, anonymize_transcripts,
anonymizeModalHandlers.close, dispatchAutoSave, and getValues.
In `@echo/frontend/src/lib/api.ts`:
- Around line 1817-1825: Add an explicit JSDoc describing the ordering contract
for quick-access preferences: note that getQuickAccessPreferences and
saveQuickAccessPreferences persist a bare QuickAccessPreference[] where the
array index denotes slot order (position matters), the API expects/returns the
array as-is, and callers must preserve array order and must not re-sort or
normalize the array when saving; place this comment above the
getQuickAccessPreferences and saveQuickAccessPreferences helper declarations
(referencing those function names and the QuickAccessPreference[] return/type)
so future contributors understand the positional ordering guarantee.
In `@echo/frontend/src/locales/de-DE.po`:
- Around line 1701-1704: Update the German translations that currently use "KI"
to use "Sprachmodell" instead: for the msgid "Dembrane is powered by AI. Please
double-check responses." (the msgstr currently "dembrane läuft mit KI. Prüf die
Antworten noch mal gegen.") replace "KI" with "Sprachmodell" and adjust
capitalization and phrasing to match product copy standards; make the same
replacement for the other instances referenced (around the other msgid
occurrences at the noted positions 2036-2038 and 5502-5504) so all German
product-facing strings say "Sprachmodell" rather than "KI".
- Around line 3905-3923: Translate and populate both German msgstr entries for
the two msgid blocks coming from src/components/participant/ParticipantBody.tsx
(the shorter variant at :200 and the longer at :196) so they are consistent and
match the UI localization: replace the English "Record" with the localized
button label "Aufnehmen", add the missing translation for the shorter variant
(currently empty msgstr), and ensure the anonymization sentence is
present/identical in both msgstrs ("Ihre Transkription wird anonymisiert und Ihr
Host kann Ihre Aufnahme nicht anhören.") so both variants show German text
consistently.
In `@echo/frontend/src/locales/en-US.po`:
- Around line 381-384: Remove the stale obsolete PO entry for msgid
"participant.anonymization.notice" (the block with the `#~` prefix) from the
en-US.po catalog to prevent catalog drift; delete the entire obsolete entry so
only the active anonymization message remains in the file and run the i18n
extraction/validation step to ensure no broken references to
"participant.anonymization.notice" remain.
In `@echo/frontend/src/locales/es-ES.po`:
- Around line 3911-3928: The Spanish .po file has an empty msgstr for the first
msgid and the translated variant still uses the English label "Record", causing
mixed-language UI; update the first msgstr to match the second translation
(translate the full paragraph and replace "Record" with the localized label,
e.g. "Grabar"), ensure both msgstr entries consistently translate the entire
msgid including the bolded and parenthetical lines, then follow the project
workflow by updating the source usage in ParticipantBody.tsx to use <Trans> or t
template literals if not already and run pnpm messages:extract followed by pnpm
messages:compile to regenerate compiled messages.
- Around line 207-210: Remove the obsolete markers so the Spanish translation
for "participant.anonymization.notice" is active (restore the msgid/msgstr pair
without the "#~" markers) so the component ParticipantBody.tsx will pick up the
live translation; then regenerate and compile message catalogs by using the
project workflow (run pnpm messages:extract followed by pnpm messages:compile)
after ensuring the source uses the <Trans> component or t`...` template literal
for that string.
In `@echo/frontend/src/locales/fr-FR.po`:
- Around line 3926-3943: The fr-FR .po entries for the participant instructions
are incomplete: fill the empty msgstr for the non-anonymized ParticipantBody
msgid (the one without the anonymization line) with a proper French translation
matching the anonymized variant, and update the CTA text in both msgstrs to
localize "Record" (e.g., use "Enregistrer" or the agreed French CTA) so the
displayed instruction matches the button label; locate the strings by the
ParticipantBody.tsx msgid blocks shown and replace the empty msgstr and the
hardcoded "Record" token with the correct French translations.
In `@echo/frontend/src/locales/it-IT.po`:
- Around line 899-901: The Italian locale file is missing translations for the
anonymization UI and participant recording prompt; open
frontend/src/locales/it-IT.po and replace the English msgstr entries for the
msgid "Anonymize Transcripts" and the participant prompt found near the
referenced msgids in ProjectPortalEditor.tsx (check the blocks around the diff
positions: 2293-2295, 4114-4119, 6625-6627) with proper Italian equivalents
(e.g., translate "Anonymize Transcripts" and the participant recording prompt
and any control descriptions), ensuring each msgid has an Italian msgstr and
that formatting/pluralization matches existing entries.
In `@echo/frontend/src/locales/nl-NL.po`:
- Around line 4055-4057: Replace the hard-coded English CTA "Record" in the two
instruction variants that start with "Neem je reactie op door op de knop
\"Record\" hieronder te klikken..." and the similar block at 4065-4069 with the
localized CTA already used elsewhere in this locale (the translated label
"Opname starten" or the same msgid/msgstr token used for the button label) so
the instruction matches the actual button text users will see.
In `@echo/frontend/src/routes/project/report/ProjectReportRoute.tsx`:
- Around line 1263-1272: The fullscreen wrapper in ProjectReportRoute.tsx
currently hardcodes backgroundColor: "white" in the style object (the ternary
style passed as React.CSSProperties), which breaks theme propagation; change
that to use the app CSS variable (backgroundColor: "var(--app-background)") so
the fullscreen branch uses the --app-background value (and ensure any other
hardcoded theme values there are replaced with --app-text / --app-font-family as
needed) while keeping the rest of the style keys (overflow, padding, toolbar
vars) unchanged.
- Around line 1374-1384: The ConfirmModal's loading prop is ineffective because
closeDeleteModal() is called immediately inside handleConfirmDelete(); change
handleConfirmDelete to call deleteReport(...) and move closeDeleteModal() into
the mutation callback (e.g., onSettled or onSuccess) so the modal remains open
while isDeletingReport is true; ensure you still guard for missing data.id or
projectId in handleConfirmDelete and keep setting setSelectedReportId(null) in
onSuccess while calling closeDeleteModal() in onSettled to always close after
the mutation finishes.
In `@echo/server/dembrane/api/project_webhook.py`:
- Around line 203-207: When webhooks is coerced to an empty list (the block with
"if not isinstance(webhooks, list): webhooks = []"), add a warning log to
improve observability; use the module's existing logger (e.g., logger or
process_logger) to emit a message that the incoming "webhooks" payload was not a
list and has been replaced with an empty list, including any relevant context
(project id or payload) if available, before continuing to build "result" and
iterating over "webhooks".
In `@echo/server/dembrane/api/project.py`:
- Around line 644-655: The call to client.get_items() inside the
directus_client_context block is synchronous blocking I/O and must be executed
in a thread pool; wrap the call with run_in_thread_pool from
dembrane.async_helpers (same pattern used earlier in this file) so the blocking
work runs off the event loop—i.e., import run_in_thread_pool if not present and
replace the direct client.get_items(...) invocation with an awaited
run_in_thread_pool(lambda: client.get_items(...)) while keeping the surrounding
try/with directus_client_context(auth.client) and variable names (projects)
intact.
- Around line 656-661: The current except Exception as err: raise
HTTPException(status_code=404, detail="Project not found") from err masks real
errors; update the error handling around the code that fetches projects to catch
and translate only genuine "not found" cases (e.g., specific DirectusNotFound or
custom NotFound exceptions) to HTTPException(404), and for other exceptions log
the error (include err and context like the projects request) then either
re-raise the original exception or raise HTTPException(status_code=500,
detail="Internal server error"); ensure you reference the existing variables and
symbols (projects, err, HTTPException) and add logging before re-raising so
operational issues aren’t hidden.
In `@echo/server/dembrane/api/template.py`:
- Around line 183-203: The async endpoints get_quick_access and
save_quick_access are calling blocking Directus client methods
(directus.get_users, directus.get_item, directus.update_user) synchronously and
must be offloaded to a thread so they don't block the event loop; change those
directus calls to run inside an async-to-sync wrapper (e.g.,
asyncio.to_thread(...) or starlette.concurrency.run_in_threadpool(...)) and
await the result, and also wrap the tight validation loop that performs multiple
directus.get_item calls so each call is executed via the same threadpool wrapper
(preserve existing error handling and return values in functions
get_quick_access and save_quick_access).
- Around line 48-50: QuickAccessItemIn currently allows empty or whitespace-only
ids because id: str has no validation; update QuickAccessItemIn to use pydantic
Field with min_length=1 (same pattern used in agentic.py) so id rejects blank
strings (e.g., id: str = Field(..., min_length=1)); ensure the Field import is
added if missing and validate that QuickAccessItemIn preserves the
Literal["static","user"] type for type.
---
Outside diff comments:
In `@echo/frontend/src/hooks/useCopyToRichText.ts`:
- Around line 53-66: The call to setCopied(true) after invoking
navigator.clipboard.write() creates a race because it runs before the write
completes; remove the trailing setCopied(true) outside the promise and only
setCopied(true) inside the successful write handler(s). Specifically, delete the
redundant setCopied(true) after navigator.clipboard.write(...) and ensure the
success callback for navigator.clipboard.write and the fallback
write([fallBackData]) success path (or their .then handlers) setCopied(true)
while the error paths call toast.error(t`Could not copy to clipboard. Please try
again.`) and log the error.
In `@echo/frontend/src/routes/project/chat/ProjectChatRoute.tsx`:
- Around line 357-388: The quickAccessItems memo can end up as [] when
quickAccessQuery.data exists but all entries are stale; update the logic in the
quickAccessItems useMemo (the mapping over quickAccessQuery.data and use of
Templates) to first map/filter into a temporary array and then if that resulting
array is empty fall back to Templates.slice(0,3); in other words, keep the
current mapping that checks pref.type and looks up Templates or
userTemplatesQuery.data, but after mapping call if (mapped.length === 0) return
Templates.slice(0, 3). This ensures the rail shows default static templates when
saved prefs are all invalid.
In `@echo/server/dembrane/api/project_webhook.py`:
- Around line 142-155: The code silently replaces a non-list `webhooks` with an
empty list; add a warning log when that happens. In the function returning the
list of WebhookResponseSchema objects, detect when `webhooks` is not a list and
call the module logger (or the same logger used in project.py) to emit a warning
noting the unexpected type/value of `webhooks` before assigning `webhooks = []`;
reference the `webhooks` variable and the surrounding list-comprehension that
builds `WebhookResponseSchema(...)` (and keep `_parse_webhook_events` usage
unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8f66081-f5f8-4b99-b40d-9cd58641b181
📒 Files selected for processing (84)
echo/AGENTS.mdecho/directus/sync/collections/operations.jsonecho/directus/sync/collections/policies.jsonecho/directus/sync/snapshot/collections/prompt_template_preference.jsonecho/directus/sync/snapshot/collections/prompt_template_rating.jsonecho/directus/sync/snapshot/fields/directus_users/quick_access_preferences.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/date_created.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/id.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/prompt_template_id.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/sort.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/template_type.jsonecho/directus/sync/snapshot/fields/prompt_template_preference/user_created.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/chat_message_id.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/date_created.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/date_updated.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/id.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/prompt_template_id.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/rating.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/user_created.jsonecho/directus/sync/snapshot/fields/prompt_template_rating/user_updated.jsonecho/directus/sync/snapshot/relations/prompt_template_preference/prompt_template_id.jsonecho/directus/sync/snapshot/relations/prompt_template_preference/user_created.jsonecho/directus/sync/snapshot/relations/prompt_template_rating/chat_message_id.jsonecho/directus/sync/snapshot/relations/prompt_template_rating/prompt_template_id.jsonecho/directus/sync/snapshot/relations/prompt_template_rating/user_created.jsonecho/directus/sync/snapshot/relations/prompt_template_rating/user_updated.jsonecho/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.mdecho/docs/superpowers/specs/2026-04-03-template-community-report-anonymization-design.mdecho/frontend/AGENTS.mdecho/frontend/src/components/chat/ChatAccordion.tsxecho/frontend/src/components/chat/ChatTemplatesMenu.tsxecho/frontend/src/components/chat/CommunityTab.tsxecho/frontend/src/components/chat/CommunityTemplateCard.tsxecho/frontend/src/components/chat/PublishTemplateForm.tsxecho/frontend/src/components/chat/QuickAccessConfigurator.tsxecho/frontend/src/components/chat/TemplateRatingPills.tsxecho/frontend/src/components/chat/TemplatesModal.tsxecho/frontend/src/components/chat/hooks/useCommunityTemplates.tsecho/frontend/src/components/chat/hooks/useUserTemplates.tsecho/frontend/src/components/chat/templateKey.tsecho/frontend/src/components/common/ConfirmModal.tsxecho/frontend/src/components/common/InputModal.tsxecho/frontend/src/components/conversation/ConversationDangerZone.tsxecho/frontend/src/components/form/MarkdownWYSIWYG/styles.cssecho/frontend/src/components/language/LanguagePicker.tsxecho/frontend/src/components/layout/Header.tsxecho/frontend/src/components/participant/ParticipantBody.tsxecho/frontend/src/components/participant/ParticipantConversationAudioContent.tsxecho/frontend/src/components/participant/ParticipantConversationText.tsxecho/frontend/src/components/participant/PermissionErrorModal.tsxecho/frontend/src/components/project/ProjectDangerZone.tsxecho/frontend/src/components/project/ProjectPortalEditor.tsxecho/frontend/src/components/project/ProjectTagsInput.tsxecho/frontend/src/components/report/CreateReportForm.tsxecho/frontend/src/components/report/ReportFocusSelector.tsxecho/frontend/src/components/report/UpdateReportModalButton.tsxecho/frontend/src/components/settings/AccountSettingsCard.tsxecho/frontend/src/components/settings/WhitelabelLogoCard.tsxecho/frontend/src/components/view/CreateViewForm.tsxecho/frontend/src/hooks/useCopyToRichText.tsecho/frontend/src/lib/api.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/frontend/src/routes/auth/VerifyEmail.tsxecho/frontend/src/routes/project/chat/ProjectChatRoute.tsxecho/frontend/src/routes/project/conversation/ProjectConversationOverview.tsxecho/frontend/src/routes/project/library/ProjectLibrary.tsxecho/frontend/src/routes/project/report/ProjectReportRoute.tsxecho/server/dembrane/api/participant.pyecho/server/dembrane/api/project.pyecho/server/dembrane/api/project_webhook.pyecho/server/dembrane/api/template.pyecho/server/dembrane/api/user_settings.pyecho/server/dembrane/tasks.py
💤 Files with no reviewable changes (28)
- echo/directus/sync/snapshot/relations/prompt_template_preference/user_created.json
- echo/directus/sync/snapshot/relations/prompt_template_preference/prompt_template_id.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/template_type.json
- echo/directus/sync/snapshot/collections/prompt_template_preference.json
- echo/directus/sync/snapshot/relations/prompt_template_rating/user_created.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/sort.json
- echo/directus/sync/snapshot/relations/prompt_template_rating/user_updated.json
- echo/directus/sync/snapshot/collections/prompt_template_rating.json
- echo/directus/sync/snapshot/relations/prompt_template_rating/prompt_template_id.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/id.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/date_updated.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/date_created.json
- echo/directus/sync/snapshot/relations/prompt_template_rating/chat_message_id.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/id.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/user_updated.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/prompt_template_id.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/user_created.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/chat_message_id.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/user_created.json
- echo/directus/sync/snapshot/fields/prompt_template_preference/date_created.json
- echo/directus/sync/snapshot/fields/prompt_template_rating/rating.json
- echo/frontend/src/components/chat/CommunityTemplateCard.tsx
- echo/frontend/src/components/chat/CommunityTab.tsx
- echo/directus/sync/snapshot/fields/prompt_template_rating/prompt_template_id.json
- echo/frontend/src/components/chat/QuickAccessConfigurator.tsx
- echo/frontend/src/components/chat/hooks/useCommunityTemplates.ts
- echo/frontend/src/components/chat/TemplateRatingPills.tsx
- echo/frontend/src/components/chat/PublishTemplateForm.tsx
Comment on lines
+24
to
+78
| ### Task 1: Report Buttons -- Teal to Blue | ||
|
|
||
| The simplest change. All primary buttons in the report feature use `color="teal"` but should use `color="blue"` per brand guidelines. | ||
|
|
||
| **Files:** | ||
| - Modify: `frontend/src/components/report/CreateReportForm.tsx:258,295` | ||
| - Modify: `frontend/src/components/report/UpdateReportModalButton.tsx:149,274,311` | ||
| - Modify: `frontend/src/routes/project/report/ProjectReportRoute.tsx:548,992` | ||
|
|
||
| - [ ] **Step 1: Change CreateReportForm.tsx buttons** | ||
|
|
||
| In `frontend/src/components/report/CreateReportForm.tsx`, change both instances of `color="teal"` to `color="blue"`: | ||
|
|
||
| Line 258 (Schedule Report button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| Line 295 (Generate now button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Change UpdateReportModalButton.tsx buttons** | ||
|
|
||
| In `frontend/src/components/report/UpdateReportModalButton.tsx`, change all three instances of `color="teal"` to `color="blue"`: | ||
|
|
||
| Line 149 (Update Report trigger button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| Line 274 (Schedule Report button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| Line 311 (Generate now button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| - [ ] **Step 3: Change ProjectReportRoute.tsx buttons** | ||
|
|
||
| In `frontend/src/routes/project/report/ProjectReportRoute.tsx`, change both instances of `color="teal"` to `color="blue"`: | ||
|
|
||
| Line 548 (Confirm reschedule button): | ||
| ```tsx | ||
| color="blue" | ||
| ``` | ||
|
|
||
| Line 992 (Publish toggle Switch): | ||
| ```tsx | ||
| color="blue" | ||
| ``` |
Contributor
There was a problem hiding this comment.
Use the shared theme token here, not Mantine blue.
This plan hardcodes blue across report controls. That will drift from the app theme/whitelabel palette and undercuts the frontend color abstraction the rest of the PR is moving toward.
As per coding guidelines, echo/frontend/src/**/*.{tsx,ts,css,scss}: Use color tokens from brand/colors.json for programmatic use in frontend styling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@echo/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.md`
around lines 24 - 78, The change hardcodes Mantine's "blue" for buttons, which
breaks the app theme abstraction; instead update the components
CreateReportForm, UpdateReportModalButton, and ProjectReportRoute to use the
shared brand color token from the frontend theme (the same token used elsewhere
in the repo) for the color prop and Switch variant—replace occurrences of
color="blue" with the theme token access used by your codebase (e.g., via
useTheme()/getToken()/theme.colors['<brand-token>'] or the shared constant
imported from brand/colors.json) so the components (Schedule Report, Generate
now, Update Report trigger, Confirm reschedule button and Publish Switch) follow
the centralized palette.
Comment on lines
+273
to
+275
| **Prerequisite:** User must create a JSON field `quick_access_preferences` on `directus_users` in Directus admin. Type: JSON, default: `[]`, nullable: true. | ||
|
|
||
| - [ ] **Step 1: Replace backend quick-access endpoints in template.py** |
Contributor
There was a problem hiding this comment.
Don't leave the Directus schema change as a manual prerequisite.
Task 3 assumes directus_users.quick_access_preferences exists. If that only lives in this checklist, fresh environments will drift and /templates/quick-access will fail at runtime. Add the versioned Directus schema/snapshot work to the plan itself.
Also applies to: 691-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@echo/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.md`
around lines 273 - 275, The plan currently lists
directus_users.quick_access_preferences as a manual prerequisite which will
break fresh environments and the /templates/quick-access flow; update the plan
to include a versioned Directus schema change (a migration or Directus schema
snapshot) as a concrete step—e.g., add a migration script or exported Directus
schema JSON that creates the quick_access_preferences JSON field (nullable,
default []), document how to apply it during setup, and reference this step in
Task 3 and the same checklist entries that mention the prerequisite so
environments are reproducible rather than relying on manual admin changes.
Comment on lines
+535
to
+542
| Edit each locale file to add the translation for `participant.anonymization.notice`: | ||
|
|
||
| - `en-US.po`: "Your transcription will be anonymized and your host will not be able to listen to your recording." | ||
| - `nl-NL.po`: "Je transcriptie wordt geanonimiseerd en je host kan niet naar je opname luisteren." | ||
| - `de-DE.po`: "Ihre Transkription wird anonymisiert und Ihr Host kann Ihre Aufnahme nicht anhoren." | ||
| - `fr-FR.po`: "Votre transcription sera anonymisee et votre hote ne pourra pas ecouter votre enregistrement." | ||
| - `es-ES.po`: "Tu transcripcion sera anonimizada y tu anfitrion no podra escuchar tu grabacion." | ||
| - `it-IT.po`: "La tua trascrizione sara anonimizzata e il tuo host non potra ascoltare la tua registrazione." |
Contributor
There was a problem hiding this comment.
Fix the sample locale strings before they get copy-pasted.
These examples drop required diacritics/umlauts (anhoren, anonymisee, beinvloed, etc.). Because this section is a paste-ready recipe for .po files, keeping them here will propagate broken translations into production locales.
Also applies to: 653-658
🧰 Tools
🪛 LanguageTool
[grammar] ~539-~539: Ensure spelling is correct
Context: ...t und Ihr Host kann Ihre Aufnahme nicht anhoren." - fr-FR.po: "Votre transcription se...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~540-~540: Ensure spelling is correct
Context: ...- fr-FR.po: "Votre transcription sera anonymisee et votre hote ne pourra pas ecouter vot...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~540-~540: Ensure spelling is correct
Context: ... transcription sera anonymisee et votre hote ne pourra pas ecouter votre enregistrem...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~540-~540: Ensure spelling is correct
Context: ... anonymisee et votre hote ne pourra pas ecouter votre enregistrement." - es-ES.po: "T...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~541-~541: Ensure spelling is correct
Context: ...otre enregistrement." - es-ES.po: "Tu transcripcion sera anonimizada y tu anfitrion no podra esc...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~541-~541: Ensure spelling is correct
Context: ..."Tu transcripcion sera anonimizada y tu anfitrion no podra escuchar tu grabacion." - `it-...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~541-~541: Ensure spelling is correct
Context: ...cion sera anonimizada y tu anfitrion no podra escuchar tu grabacion." - it-IT.po: "...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~541-~541: Ensure spelling is correct
Context: ...ada y tu anfitrion no podra escuchar tu grabacion." - it-IT.po: "La tua trascrizione sa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~542-~542: Ensure spelling is correct
Context: ...one sara anonimizzata e il tuo host non potra ascoltare la tua registrazione." ```ba...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@echo/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.md`
around lines 535 - 542, The provided sample translations for the key
participant.anonymization.notice contain missing diacritics/umlauts and will
propagate broken strings if copy‑pasted; update each locale example (en-US.po,
nl-NL.po, de-DE.po, fr-FR.po, es-ES.po, it-IT.po) to use the correct accented
characters/umlauts (e.g., "anhoren" → "anhören", "anonymisee" → "anonymisée",
etc.) so the paste-ready .po entries are valid; apply the same corrections to
the similar block referenced later to ensure all example strings include proper
diacritics.
Comment on lines
+626
to
+632
| <Button | ||
| color="red" | ||
| onClick={() => { | ||
| control._formValues.anonymize_transcripts = false; | ||
| // Use setValue from react-hook-form to properly update and mark dirty | ||
| setValue("anonymize_transcripts", false, { shouldDirty: true }); | ||
| closeAnonymizeModal(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is control._formValuespart of React Hook Form's public API, or shouldsetValue() be used instead to update a field and dirty state?
💡 Result:
No, control._formValues is not part of React Hook Form's public API. The official documentation explicitly states that the control object contains methods for registering components and warns: "Important: do not access any of the properties inside this object directly. It's for internal usage only." Instead, use the public setValue(name, value, options) method from useForm to update field values. This properly updates the form state, including dirty state when using options like { shouldDirty: true, shouldValidate: true }, as documented in the setValue API page. Accessing internal properties like _formValues risks breaking changes and bypasses proper state management.
Citations:
- 1: https://react-hook-form.com/docs/useform/control
- 2: https://react-hook-form.com/docs/useform/setvalue
- 3: https://react-hook-form.com/docs/useform
Remove the direct mutation of control._formValues.
Line 629 reaches into control._formValues, which is a private implementation detail of React Hook Form. The official RHF docs explicitly warn against accessing internal control properties. Since setValue("anonymize_transcripts", false, { shouldDirty: true }) already handles the update through the public API, the direct mutation is redundant and exposes the code to breaking changes.
🛠️ Safer plan snippet
- control._formValues.anonymize_transcripts = false;
- // Use setValue from react-hook-form to properly update and mark dirty
setValue("anonymize_transcripts", false, { shouldDirty: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@echo/docs/superpowers/plans/2026-04-03-template-community-report-anonymization.md`
around lines 626 - 632, Remove the direct mutation of the React Hook Form
internals: delete the assignment to control._formValues.anonymize_transcripts in
the Button onClick handler and rely solely on the public API call
setValue("anonymize_transcripts", false, { shouldDirty: true }); keep the call
to closeAnonymizeModal() unchanged; this prevents touching the private
control._formValues implementation and uses the supported setValue method to
update state and mark the form dirty.
echo/docs/superpowers/specs/2026-04-03-template-community-report-anonymization-design.md
Show resolved
Hide resolved
Comment on lines
+203
to
+207
| if not isinstance(webhooks, list): | ||
| webhooks = [] | ||
|
|
||
| result = [] | ||
| for webhook in webhooks or []: | ||
| for webhook in webhooks: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same pattern — add warning log for observability.
Consistent with the suggestion above and the pattern in project.py.
🔧 Proposed fix
if not isinstance(webhooks, list):
+ logger.warning("get_items returned non-list for copyable webhooks: %s", webhooks)
webhooks = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/project_webhook.py` around lines 203 - 207, When
webhooks is coerced to an empty list (the block with "if not
isinstance(webhooks, list): webhooks = []"), add a warning log to improve
observability; use the module's existing logger (e.g., logger or process_logger)
to emit a message that the incoming "webhooks" payload was not a list and has
been replaced with an empty list, including any relevant context (project id or
payload) if available, before continuing to build "result" and iterating over
"webhooks".
Comment on lines
+656
to
+661
| except Exception as err: | ||
| raise HTTPException(status_code=404, detail="Project not found") from err | ||
|
|
||
| if not isinstance(projects, list) or not projects: | ||
| raise HTTPException(status_code=404, detail="Project not found") | ||
|
|
Contributor
There was a problem hiding this comment.
Catching all exceptions as 404 may mask actual errors.
Treating any Exception as "Project not found" will hide real issues like network failures, auth problems, or Directus outages. Consider narrowing the catch or logging before re-raising.
🔧 Proposed fix — log before masking
except Exception as err:
+ logger.warning("Failed to fetch project %s: %s", project_id, err)
raise HTTPException(status_code=404, detail="Project not found") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/project.py` around lines 656 - 661, The current
except Exception as err: raise HTTPException(status_code=404, detail="Project
not found") from err masks real errors; update the error handling around the
code that fetches projects to catch and translate only genuine "not found" cases
(e.g., specific DirectusNotFound or custom NotFound exceptions) to
HTTPException(404), and for other exceptions log the error (include err and
context like the projects request) then either re-raise the original exception
or raise HTTPException(status_code=500, detail="Internal server error"); ensure
you reference the existing variables and symbols (projects, err, HTTPException)
and add logging before re-raising so operational issues aren’t hidden.
Comment on lines
+48
to
50
| class QuickAccessItemIn(BaseModel): | ||
| type: Literal["static", "user"] | ||
| id: str |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current state of the file
cat -n echo/server/dembrane/api/template.py | head -60Repository: Dembrane/echo
Length of output: 1909
🏁 Script executed:
# Find and check Pydantic version in requirements or pyproject.toml
fd -e "requirements.txt" -o -e "pyproject.toml" -o -e "setup.py" | head -10 | xargs grep -l "pydantic" 2>/dev/nullRepository: Dembrane/echo
Length of output: 175
🏁 Script executed:
# Check imports at the top of the file
head -30 echo/server/dembrane/api/template.pyRepository: Dembrane/echo
Length of output: 779
🏁 Script executed:
# Find and display pyproject.toml or requirements files with pydantic version
find . -maxdepth 3 -type f \( -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" \) | xargs cat 2>/dev/null | grep -i "pydantic" -A 2 -B 2 | head -20Repository: Dembrane/echo
Length of output: 461
🏁 Script executed:
# Search for existing uses of StringConstraints or Annotated in the codebase to confirm the pattern
rg "StringConstraints|Annotated" --type py -B 2 -A 2 | head -50Repository: Dembrane/echo
Length of output: 1553
🏁 Script executed:
# Search for StringConstraints usage in the codebase
rg "StringConstraints" --type py -B 3 -A 1Repository: Dembrane/echo
Length of output: 39
🏁 Script executed:
# Also check for min_length constraint patterns in Pydantic v2
rg "min_length|max_length.*Field" --type py | grep -i "id.*:" | head -20Repository: Dembrane/echo
Length of output: 139
🏁 Script executed:
# Check the agentic.py file to see how min_length is used with Field
rg "project_id.*Field" echo/server/dembrane/api/agentic.py -B 2 -A 2Repository: Dembrane/echo
Length of output: 231
🏁 Script executed:
# Search for Pydantic constraint patterns in the codebase to understand the project's convention
rg "Field\(" --type py -A 1 | grep -E "min_length|max_length|pattern" | head -20Repository: Dembrane/echo
Length of output: 1189
Use Field() constraint to reject blank IDs.
id: str allows "" and whitespace, enabling invalid entries like {"type":"static","id":""}. Fix by adding min_length=1 via Field() to match the project's constraint pattern (already used in agentic.py):
class QuickAccessItemIn(BaseModel):
type: Literal["static", "user"]
- id: str
+ id: str = Field(..., min_length=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/template.py` around lines 48 - 50, QuickAccessItemIn
currently allows empty or whitespace-only ids because id: str has no validation;
update QuickAccessItemIn to use pydantic Field with min_length=1 (same pattern
used in agentic.py) so id rejects blank strings (e.g., id: str = Field(...,
min_length=1)); ensure the Field import is added if missing and validate that
QuickAccessItemIn preserves the Literal["static","user"] type for type.
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 (2)
echo/server/dembrane/api/template.py (2)
166-181:⚠️ Potential issue | 🟠 MajorPrune dangling quick-access items after deletes.
quick_access_preferencesis plain user JSON now. Deleting a prompt template no longer removes{"type":"user","id": template_id}, andget_quick_access()returns the stored list as-is, so dead shortcuts live forever. Reconcile them during delete or filter/revalidateuserentries before returning preferences.Also applies to: 188-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/template.py` around lines 166 - 181, When deleting a prompt template, remove any dangling quick-access entries that point to that template: after verifying ownership with directus.get_item and after successful directus.delete_item, load the user's quick_access_preferences (via get_quick_access or the stored JSON), filter out entries where entry.get("type") == "user" and entry.get("id") == template_id, persist the cleaned preferences back to the user record, and handle errors; apply the same pruning logic to the other delete path referenced around lines 188-207 so dead {"type":"user","id": template_id} shortcuts are removed consistently.
61-105:⚠️ Potential issue | 🔴 CriticalWrap blocking Directus I/O in
run_in_thread_pool.These async handlers call synchronous
directus.*I/O inline without wrapping, which blocks the event loop on slow Directus round-trips. Migrate toawait run_in_thread_pool(lambda: directus.get_items(...))instead. Per the guidelines: "Always wrap blocking I/O calls usingrun_in_thread_poolfromdembrane.async_helpersin backend code. Wrap calls todirectus.*... Prefer converting endpoints toasync defandawaitresults."Affected: lines 65, 114, 123, 139, 153, 167, 177, 193, 234, 247, 263.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/template.py` around lines 61 - 105, The async handler list_prompt_templates is calling synchronous Directus I/O (directus.get_items) inline and must be wrapped in run_in_thread_pool from dembrane.async_helpers; replace calls like directus.get_items(...) in list_prompt_templates with await run_in_thread_pool(lambda: directus.get_items(...)) so the blocking call runs off the event loop, and apply the same pattern to every other directus.* call in this module (e.g., other get_items/get_item/create/update/delete uses noted in the review), ensuring you import run_in_thread_pool and await the wrapped call before processing its result.
🤖 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/frontend/src/components/language/LanguagePicker.tsx`:
- Line 87: The redirect currently sets window.location.href using only
`/${validLanguage}${newPathname}`, dropping any existing query string or hash;
update the redirect in LanguagePicker (where `validLanguage` and `newPathname`
are used) to preserve `window.location.search` and `window.location.hash` (or
build the new URL via the URL API using the current location, set pathname to
`/${validLanguage}${newPathname}`, and keep search and hash) so query and
fragment are retained during the language change.
In `@echo/server/dembrane/api/template.py`:
- Around line 230-242: The current loop rewrites any Directus/transport failure
into a 400 by catching all Exceptions; update the validation in the loop that
handles item.type == "user" (the block using directus.get_item, template,
item.id, and HTTPException) so that only explicit "not found" or
ownership-mismatch cases raise HTTPException(status_code=400), while other
exceptions from directus.get_item are logged and re-raised or converted to a 5xx
(e.g., 500 or 502) HTTPException so backend/transport failures are surfaced
correctly; ensure you do not swallow underlying exceptions and keep the existing
ownership check (template.get("user_created") != auth.user_id) as the 400
branch.
---
Outside diff comments:
In `@echo/server/dembrane/api/template.py`:
- Around line 166-181: When deleting a prompt template, remove any dangling
quick-access entries that point to that template: after verifying ownership with
directus.get_item and after successful directus.delete_item, load the user's
quick_access_preferences (via get_quick_access or the stored JSON), filter out
entries where entry.get("type") == "user" and entry.get("id") == template_id,
persist the cleaned preferences back to the user record, and handle errors;
apply the same pruning logic to the other delete path referenced around lines
188-207 so dead {"type":"user","id": template_id} shortcuts are removed
consistently.
- Around line 61-105: The async handler list_prompt_templates is calling
synchronous Directus I/O (directus.get_items) inline and must be wrapped in
run_in_thread_pool from dembrane.async_helpers; replace calls like
directus.get_items(...) in list_prompt_templates with await
run_in_thread_pool(lambda: directus.get_items(...)) so the blocking call runs
off the event loop, and apply the same pattern to every other directus.* call in
this module (e.g., other get_items/get_item/create/update/delete uses noted in
the review), ensuring you import run_in_thread_pool and await the wrapped call
before processing its result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7f53073-e975-4bd8-a909-ef42c7c6444e
📒 Files selected for processing (2)
echo/frontend/src/components/language/LanguagePicker.tsxecho/server/dembrane/api/template.py
…mprove error logging in Directus client and API endpoints - Updated LanguagePicker to include search and hash parameters in the URL when changing languages. - Added logging for JSON parsing errors in DirectusClient to improve error tracking. - Refactored API project and template files for better readability and error handling, including async safety for Directus calls.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
echo/server/dembrane/api/template.py (2)
233-244:⚠️ Potential issue | 🟠 MajorDon’t downgrade Directus failures into
400 Template not found.This still maps every unexpected
directus.get_item()failure to a client error. Transport/auth/Directus issues should surface as 5xx, while only genuine missing or foreign templates should stay in the 400 path.💡 Proposed fix
+ from dembrane.directus import DirectusBadRequest + # Validate user templates exist and belong to user for item in body: if item.type == "user": try: template = await run_in_thread_pool(directus.get_item, "prompt_template", item.id) if not template or template.get("user_created") != auth.user_id: raise HTTPException(status_code=400, detail=f"Template not found: {item.id}") except HTTPException: raise - except Exception: + except DirectusBadRequest: + raise HTTPException( + status_code=400, detail=f"Template not found: {item.id}" + ) from None + except Exception: + logger.exception("Failed to validate quick-access template %s", item.id) raise HTTPException( - status_code=400, detail=f"Template not found: {item.id}" + status_code=502, detail="Failed to validate template" ) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/template.py` around lines 233 - 244, The current loop around run_in_thread_pool(directus.get_item, "prompt_template", item.id) swallows all errors and turns them into a 400, which hides transport/auth/Directus failures; change the try/except in that block to only convert a genuine missing or foreign template to HTTPException(status_code=400) by checking for a not-found response (e.g., directus raising a 404 or template is falsy or template.get("user_created") != auth.user_id) and re-raise any other exceptions (or wrap them as 5xx) so transport/auth errors surface as server errors; update the logic around run_in_thread_pool, directus.get_item, template and auth.user_id to implement this behavior.
224-230:⚠️ Potential issue | 🟡 MinorReject blank quick-access IDs before persisting them.
The later lookup only runs for
type == "user", so{ "type": "static", "id": "" }still passes and gets stored. That invalid row will then leak straight back throughGET /quick-access.💡 Proposed fix
# Validate no duplicates seen = set() for item in body: + if not item.id.strip(): + raise HTTPException(status_code=400, detail="Quick access item id must not be blank") key = (item.type, item.id) if key in seen: raise HTTPException(status_code=400, detail=f"Duplicate item: {item.type}:{item.id}") seen.add(key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/template.py` around lines 224 - 230, The duplicate-check loop currently allows items with empty IDs to pass; add a validation before or within the loop that rejects blank or whitespace-only quick-access IDs by raising HTTPException(400) when item.id is empty (e.g., if not item.id or item.id.strip() == ""), referencing the same loop over body and the item fields item.type and item.id (the seen set/duplicate logic) so invalid rows are not persisted and won't leak back on GET /quick-access.echo/server/dembrane/api/project.py (1)
652-669:⚠️ Potential issue | 🟠 MajorKeep backend failures distinct from “project not found”.
This bypasses the
ProjectService.get_by_id_or_raise()contract inecho/server/dembrane/service/project.py:28-60and then rewrites every failure as 404. A Directus outage, auth error, or malformed response will now look identical to a missing project.💡 Proposed fix
async def _verify_project_access(auth: DependencyDirectusSession, project_id: str) -> dict: """Verify the authenticated user has access to the given project. Returns the project dict.""" - try: - projects = await run_in_thread_pool( - auth.client.get_items, - "project", - { - "query": { - "filter": {"id": {"_eq": project_id}}, - "fields": ["id", "directus_user_id"], - "limit": 1, - } - }, - ) - except Exception as err: - logger.warning("Failed to fetch project %s: %s", project_id, err) - raise HTTPException(status_code=404, detail="Project not found") from err - - if not isinstance(projects, list) or not projects: - raise HTTPException(status_code=404, detail="Project not found") - - project = projects[0] + project_service = ProjectService(directus_client=auth.client) + try: + project = await run_in_thread_pool(project_service.get_by_id_or_raise, project_id) + except ProjectNotFoundException as err: + raise HTTPException(status_code=404, detail="Project not found") from err + except ProjectServiceException as err: + logger.warning("Failed to fetch project %s: %s", project_id, err) + raise HTTPException(status_code=500, detail="Failed to fetch project") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/project.py` around lines 652 - 669, The current try/except around run_in_thread_pool(auth.client.get_items, ...) swallows all backend errors and converts them to a 404, which violates the ProjectService.get_by_id_or_raise() contract; change the logic so backend failures propagate as server errors while only an empty/missing result becomes a 404. Concretely: either replace this raw fetch with a call to ProjectService.get_by_id_or_raise(project_id) (preferred) or modify the except to re-raise the original exception (or raise a 5xx HTTPException with the original error) and keep the existing 404 only in the subsequent "if not isinstance(projects, list) or not projects" branch; reference run_in_thread_pool and auth.client.get_items (or ProjectService.get_by_id_or_raise) to locate and fix the code.
🤖 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/directus.py`:
- Around line 380-384: The current SEARCH response handler swallows JSON parse
errors and returns a sentinel dict which makes get_items()/get_users() appear
successful; instead, after logging via logger.exception("Failed to parse SEARCH
response JSON"), re-raise a clear error that includes the response status and
body (use response.status_code and response.text or the parsed JSON if
available) so callers see the failure immediately; do not return {"error": ...}.
Ensure the exception raised preserves the original exception context (e.g.,
raise RuntimeError(...) from e) so upstream callers can handle or surface the
real Directus error.
---
Duplicate comments:
In `@echo/server/dembrane/api/project.py`:
- Around line 652-669: The current try/except around
run_in_thread_pool(auth.client.get_items, ...) swallows all backend errors and
converts them to a 404, which violates the ProjectService.get_by_id_or_raise()
contract; change the logic so backend failures propagate as server errors while
only an empty/missing result becomes a 404. Concretely: either replace this raw
fetch with a call to ProjectService.get_by_id_or_raise(project_id) (preferred)
or modify the except to re-raise the original exception (or raise a 5xx
HTTPException with the original error) and keep the existing 404 only in the
subsequent "if not isinstance(projects, list) or not projects" branch; reference
run_in_thread_pool and auth.client.get_items (or
ProjectService.get_by_id_or_raise) to locate and fix the code.
In `@echo/server/dembrane/api/template.py`:
- Around line 233-244: The current loop around
run_in_thread_pool(directus.get_item, "prompt_template", item.id) swallows all
errors and turns them into a 400, which hides transport/auth/Directus failures;
change the try/except in that block to only convert a genuine missing or foreign
template to HTTPException(status_code=400) by checking for a not-found response
(e.g., directus raising a 404 or template is falsy or
template.get("user_created") != auth.user_id) and re-raise any other exceptions
(or wrap them as 5xx) so transport/auth errors surface as server errors; update
the logic around run_in_thread_pool, directus.get_item, template and
auth.user_id to implement this behavior.
- Around line 224-230: The duplicate-check loop currently allows items with
empty IDs to pass; add a validation before or within the loop that rejects blank
or whitespace-only quick-access IDs by raising HTTPException(400) when item.id
is empty (e.g., if not item.id or item.id.strip() == ""), referencing the same
loop over body and the item fields item.type and item.id (the seen set/duplicate
logic) so invalid rows are not persisted and won't leak back on GET
/quick-access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a94b37af-35db-4c36-97fc-0e566571e41a
📒 Files selected for processing (4)
echo/frontend/src/components/language/LanguagePicker.tsxecho/server/dembrane/api/project.pyecho/server/dembrane/api/template.pyecho/server/dembrane/directus.py
Comment on lines
380
to
+384
| try: | ||
| return response.json()["data"] | ||
| except Exception as exc: # noqa: BLE001 - want best-effort fallback | ||
| return {"error": f"No data found for this request : {exc}"} | ||
| except Exception: # noqa: BLE001 - want best-effort fallback | ||
| logger.exception("Failed to parse SEARCH response JSON") | ||
| return {"error": "No data found for this request"} |
Contributor
There was a problem hiding this comment.
Fail fast on SEARCH failures instead of returning a sentinel dict.
A missing data key here usually means Directus returned an error payload, not “no data”. Returning {"error": ...} from the client makes get_items() / get_users() look successful and pushes the real failure downstream as empty states, 404s, or type errors.
💡 Proposed fix
try:
- return response.json()["data"]
- except Exception: # noqa: BLE001 - want best-effort fallback
- logger.exception("Failed to parse SEARCH response JSON")
- return {"error": "No data found for this request"}
+ payload = response.json()
+ except ValueError as exc:
+ logger.exception("Failed to parse SEARCH response JSON")
+ raise DirectusBadRequest("Invalid SEARCH response JSON") from exc
+
+ if "errors" in payload:
+ raise AssertionError(payload["errors"])
+
+ try:
+ return payload["data"]
+ except KeyError as exc:
+ logger.exception("SEARCH response missing data field")
+ raise DirectusBadRequest("Invalid SEARCH response format") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/directus.py` around lines 380 - 384, The current SEARCH
response handler swallows JSON parse errors and returns a sentinel dict which
makes get_items()/get_users() appear successful; instead, after logging via
logger.exception("Failed to parse SEARCH response JSON"), re-raise a clear error
that includes the response status and body (use response.status_code and
response.text or the parsed JSON if available) so callers see the failure
immediately; do not return {"error": ...}. Ensure the exception raised preserves
the original exception context (e.g., raise RuntimeError(...) from e) so
upstream callers can handle or surface the real Directus error.
spashii
approved these changes
Apr 6, 2026
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
3 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
Bug Fixes
Refactor
Style
Docs