ECHO-681 ECHO-698 ECHO-695 ECHO-682 Add per-legal-basis configuration support#466
Merged
ECHO-681 ECHO-698 ECHO-695 ECHO-682 Add per-legal-basis configuration support#466
Conversation
- participant portal datacontroller slide updated to comply with new legal basis
Contributor
WalkthroughAdds two new Directus fields (legal_basis, privacy_policy_url), exposes them on user reads, surfaces them through frontend types, onboarding and settings UI, and backend public schema; implements a PATCH endpoint to update user legal-basis and optional privacy_policy_url. LGTM. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
echo/server/dembrane/api/user_settings.py (1)
61-77:⚠️ Potential issue | 🔴 CriticalBlocker: sync I/O is still running on the async request path.
requests.post(and the subsequentdirectus.update_usercall) should not execute directly inside this async handler; this can stall the event loop under load.⚡ Proposed fix
- response = requests.post( - url, headers=headers, files=files, data=data, verify=directus.verify - ) + response = await run_in_thread_pool( + requests.post, + url, + headers=headers, + files=files, + data=data, + verify=directus.verify, + ) @@ - directus.update_user(auth.user_id, {"whitelabel_logo": file_id}) + await run_in_thread_pool( + directus.update_user, + auth.user_id, + {"whitelabel_logo": file_id}, + )As per coding guidelines: "Always wrap blocking I/O calls using
run_in_thread_pool... Wrap calls todirectus.*... in backend code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/user_settings.py` around lines 61 - 77, The async handler currently performs blocking I/O directly: replace the direct calls to requests.post and the subsequent json parsing and assignment of file_id, and the call to directus.update_user, by executing them inside run_in_thread_pool (i.e., wrap requests.post + response.json() and extraction of file_id in a single run_in_thread_pool call, and wrap directus.update_user(auth.user_id, {"whitelabel_logo": file_id}) in another run_in_thread_pool call) so the event loop isn't blocked; preserve existing error handling by catching exceptions from the threaded calls and re-raising HTTPException with the same messages.
🤖 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/directus_users/legal_basis.json`:
- Around line 20-21: The "text" choice entries currently contain a trailing
space ("dembrane events ") which causes inconsistent rendering; locate the
choice objects with "text": "dembrane events " (paired with "value":
"dembrane-events") and remove the trailing whitespace so the text becomes
"dembrane events" in both occurrences (the duplicate entry further down the
array as well) to ensure consistent labels in Directus.
In `@echo/frontend/src/components/participant/hooks/useOnboardingCards.ts`:
- Around line 1038-1052: Replace the phrasing "AI models" (and localized
equivalents) with "language models" in the onboarding privacy card strings;
specifically update the slides[].extraHelp entries in useOnboardingCards (the
locale objects like "en-US" and the German entry shown) to say "is not used to
train language models" (and the appropriate German/localized phrase), and scan
the same file for any other occurrences of "AI" or "AI models" to keep copy
consistent across all locales.
- Around line 1017-1023: The current branch in useOnboardingCards uses
getConsentPrivacyCard as the default for any unknown legalBasis; change the
logic so unknown values fall back to client-managed instead: explicitly check
for "client-managed" and "dembrane-events" as now, add an explicit check for
"consent" to call getConsentPrivacyCard(lang, privacyPolicyUrl) only when
legalBasis === "consent", and otherwise return getClientManagedPrivacyCard(lang)
for the fallback; reference the legalBasis variable and the helper functions
getClientManagedPrivacyCard, getDembraneEventsPrivacyCard, and
getConsentPrivacyCard to locate and update the branch.
In `@echo/frontend/src/components/participant/ParticipantOnboardingCards.tsx`:
- Around line 384-385: The checkbox label class in ParticipantOnboardingCards
(the label prop string containing "text-lg leading-snug pl-4 text-gray-700
text-left pt-0.5") hardcodes a theme color; replace the theme-dependent
"text-gray-700" with the app CSS variable (e.g. use a utility or inline style
referencing var(--app-text) or a small CSS class that sets color:
var(--app-text)) while preserving the spacing/layout utilities ("text-lg
leading-snug pl-4 text-left pt-0.5") so theme changes propagate correctly.
In `@echo/frontend/src/components/settings/LegalBasisSettingsCard.tsx`:
- Around line 47-60: Replace the raw fetch inside the mutationFn with the shared
postDirectus helper: import postDirectus and call it (e.g., postDirectus or
directus.request wrapper) instead of fetch to POST/PATCH to
"/user-settings/legal-basis", passing the same JSON payload built from
legalBasis and privacyPolicyUrl and preserving the HTTP method ("PATCH") and
response handling; remove manual credentials/headers since postDirectus manages
auth/error conventions and adjust error/response checks to use the postDirectus
return shape.
In `@echo/frontend/src/routes/participant/ParticipantStart.tsx`:
- Line 53: The component currently force-casts project to ParticipantProject
when rendering ParticipantOnboardingCards; instead, guard the render so you only
render ParticipantOnboardingCards when project is defined (e.g. project !==
undefined or Boolean(project)) and pass project directly without using the
unsafe "as ParticipantProject" cast. Locate the usage of
ParticipantOnboardingCards and the project variable in ParticipantStart (the
query/hook that provides project) and update the JSX to conditionally render the
card component with the real typed project value.
In `@echo/server/dembrane/api/user_settings.py`:
- Around line 129-132: When setting update_data["privacy_policy_url"] in the
user settings flow, validate body.privacy_policy_url for a safe absolute URL
(must parse to a scheme of http/https and a non-empty host) before assigning; if
validation fails or the legal_basis isn't "consent", set
update_data["privacy_policy_url"] = None. Locate the assignment block that
checks body.legal_basis and add a guard that parses/validates the URL (e.g.,
using urllib.parse.urlparse or a shared URL validator helper) and only persists
when scheme in ("http","https") and netloc is present.
- Around line 116-118: The domain check on the extracted variable email uses
email.endswith("@dembrane.com") which is case-sensitive and can reject valid
addresses; change the check to compare a case-normalized version (e.g.,
lowercased or casefolded) of email (or just its domain portion) against
"@dembrane.com" before raising the HTTPException so the dembrane-domain
validation is case-insensitive (locate the email variable and the
email.endswith("@dembrane.com") check to modify).
---
Outside diff comments:
In `@echo/server/dembrane/api/user_settings.py`:
- Around line 61-77: The async handler currently performs blocking I/O directly:
replace the direct calls to requests.post and the subsequent json parsing and
assignment of file_id, and the call to directus.update_user, by executing them
inside run_in_thread_pool (i.e., wrap requests.post + response.json() and
extraction of file_id in a single run_in_thread_pool call, and wrap
directus.update_user(auth.user_id, {"whitelabel_logo": file_id}) in another
run_in_thread_pool call) so the event loop isn't blocked; preserve existing
error handling by catching exceptions from the threaded calls and re-raising
HTTPException with the same messages.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
echo/directus/sync/collections/permissions.jsonecho/directus/sync/snapshot/fields/directus_users/legal_basis.jsonecho/directus/sync/snapshot/fields/directus_users/privacy_policy_url.jsonecho/frontend/src/components/auth/hooks/index.tsecho/frontend/src/components/layout/ParticipantHeader.tsxecho/frontend/src/components/participant/ParticipantOnboardingCards.tsxecho/frontend/src/components/participant/hooks/useOnboardingCards.tsecho/frontend/src/components/project/ProjectPortalEditor.tsxecho/frontend/src/components/settings/LegalBasisSettingsCard.tsxecho/frontend/src/lib/api.tsecho/frontend/src/lib/typesDirectus.d.tsecho/frontend/src/routes/participant/ParticipantStart.tsxecho/frontend/src/routes/settings/UserSettingsRoute.tsxecho/server/dembrane/api/participant.pyecho/server/dembrane/api/user_settings.py
Comment on lines
+20
to
+21
| "text": "dembrane events ", | ||
| "value": "dembrane-events" |
Contributor
There was a problem hiding this comment.
Remove trailing whitespace in legal basis choice labels
Line 20 and Line 41 use "dembrane events " with a trailing space. This can cause inconsistent rendering/selection text in Directus UI and exports.
Proposed fix
- "text": "dembrane events ",
+ "text": "dembrane events",
...
- "text": "dembrane events ",
+ "text": "dembrane events",Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/directus/sync/snapshot/fields/directus_users/legal_basis.json` around
lines 20 - 21, The "text" choice entries currently contain a trailing space
("dembrane events ") which causes inconsistent rendering; locate the choice
objects with "text": "dembrane events " (paired with "value": "dembrane-events")
and remove the trailing whitespace so the text becomes "dembrane events" in both
occurrences (the duplicate entry further down the array as well) to ensure
consistent labels in Directus.
Comment on lines
+1017
to
+1023
| if (legalBasis === "client-managed") { | ||
| return getClientManagedPrivacyCard(lang); | ||
| } | ||
| if (legalBasis === "dembrane-events") { | ||
| return getDembraneEventsPrivacyCard(lang); | ||
| } | ||
| return getConsentPrivacyCard(lang, privacyPolicyUrl); |
Contributor
There was a problem hiding this comment.
Harden legal-basis fallback: unknown values should not default to consent flow.
Current branch logic routes any unexpected value to getConsentPrivacyCard, which can unintentionally enforce consent checkbox behavior. Fall back to client-managed instead.
🔧 Proposed fix
- if (legalBasis === "client-managed") {
- return getClientManagedPrivacyCard(lang);
- }
- if (legalBasis === "dembrane-events") {
- return getDembraneEventsPrivacyCard(lang);
- }
- return getConsentPrivacyCard(lang, privacyPolicyUrl);
+ if (legalBasis === "consent") {
+ return getConsentPrivacyCard(lang, privacyPolicyUrl);
+ }
+ if (legalBasis === "dembrane-events") {
+ return getDembraneEventsPrivacyCard(lang);
+ }
+ return getClientManagedPrivacyCard(lang);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/participant/hooks/useOnboardingCards.ts` around
lines 1017 - 1023, The current branch in useOnboardingCards uses
getConsentPrivacyCard as the default for any unknown legalBasis; change the
logic so unknown values fall back to client-managed instead: explicitly check
for "client-managed" and "dembrane-events" as now, add an explicit check for
"consent" to call getConsentPrivacyCard(lang, privacyPolicyUrl) only when
legalBasis === "consent", and otherwise return getClientManagedPrivacyCard(lang)
for the fallback; reference the legalBasis variable and the helper functions
getClientManagedPrivacyCard, getDembraneEventsPrivacyCard, and
getConsentPrivacyCard to locate and update the branch.
Comment on lines
+1038
to
+1052
| "Aufnahmen werden transkribiert und für Erkenntnisse analysiert. Ihre Daten werden auf gesicherten Servern in Europa gespeichert, nicht zum Trainieren von KI-Modellen verwendet und innerhalb von 30 Tagen nach Projektende gelöscht.\nFragen zu Ihrer Privatsphäre? Wenden Sie sich direkt an den Organisator.", | ||
| title: "Verantwortlicher, Nutzung und Sicherheit.", | ||
| }, | ||
| ], | ||
| }, | ||
| "en-US": { | ||
| section: "Privacy", | ||
| slides: [ | ||
| { | ||
| content: | ||
| "The organiser is responsible for how your data is used in this session. dembrane processes your conversation on their behalf.", | ||
| cta: "I understand", | ||
| extraHelp: | ||
| "Recordings are transcribed and analysed for insights. Your data is stored on secured servers in Europe, is not used to train AI models, and is deleted within 30 days after the project has ended.\nQuestions about your privacy? Contact the organiser directly.", | ||
| title: "Data controller, usage and security.", |
Contributor
There was a problem hiding this comment.
Update new privacy copy to use “language model” wording instead of “AI”.
The added strings use “AI models” phrasing (and localized equivalents) across the new privacy-card content. Please align these to “language model” terminology.
As per coding guidelines: "UI copy should be shortest possible with highest clarity; say 'language model' not 'AI' for platform features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/participant/hooks/useOnboardingCards.ts` around
lines 1038 - 1052, Replace the phrasing "AI models" (and localized equivalents)
with "language models" in the onboarding privacy card strings; specifically
update the slides[].extraHelp entries in useOnboardingCards (the locale objects
like "en-US" and the German entry shown) to say "is not used to train language
models" (and the appropriate German/localized phrase), and scan the same file
for any other occurrences of "AI" or "AI models" to keep copy consistent across
all locales.
Comment on lines
+384
to
+385
| label: | ||
| "text-lg leading-snug pl-4 text-gray-700 text-left pt-0.5", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use theme variable for checkbox label color instead of text-gray-700.
text-gray-700 hardcodes a theme-dependent color. Keep spacing/layout utility classes, but move text color to CSS variables so theme changes propagate.
🎯 Proposed adjustment
<Checkbox
@@
classNames={{
body: "items-center",
- label:
- "text-lg leading-snug pl-4 text-gray-700 text-left pt-0.5",
+ label: "text-lg leading-snug pl-4 text-left pt-0.5",
root: "items-start",
}}
+ styles={{ label: { color: "var(--app-text)" } }}
{...testId("portal-onboarding-checkbox")}
/>As per coding guidelines: "Keep static utility classes ... move theme-dependent colors to CSS variables" and "Use var(--app-background) and var(--app-text) instead of hardcoded colors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/participant/ParticipantOnboardingCards.tsx`
around lines 384 - 385, The checkbox label class in ParticipantOnboardingCards
(the label prop string containing "text-lg leading-snug pl-4 text-gray-700
text-left pt-0.5") hardcodes a theme color; replace the theme-dependent
"text-gray-700" with the app CSS variable (e.g. use a utility or inline style
referencing var(--app-text) or a small CSS class that sets color:
var(--app-text)) while preserving the spacing/layout utilities ("text-lg
leading-snug pl-4 text-left pt-0.5") so theme changes propagate correctly.
Comment on lines
+47
to
+60
| mutationFn: async () => { | ||
| const response = await fetch( | ||
| `${API_BASE_URL}/user-settings/legal-basis`, | ||
| { | ||
| body: JSON.stringify({ | ||
| legal_basis: legalBasis, | ||
| privacy_policy_url: | ||
| legalBasis === "consent" ? privacyPolicyUrl || null : null, | ||
| }), | ||
| credentials: "include", | ||
| headers: { "Content-Type": "application/json" }, | ||
| method: "PATCH", | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared settings request path instead of raw fetch.
This works, but it forks request behavior from the rest of settings flows. Please route this through the existing custom Directus request/helper path (postDirectus) for consistency in auth/error handling conventions.
As per coding guidelines: "Use custom Directus POSTs ... by calling directus.request with a function signature ... reuse postDirectus from src/components/settings/hooks/index.ts for consistency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/settings/LegalBasisSettingsCard.tsx` around
lines 47 - 60, Replace the raw fetch inside the mutationFn with the shared
postDirectus helper: import postDirectus and call it (e.g., postDirectus or
directus.request wrapper) instead of fetch to POST/PATCH to
"/user-settings/legal-basis", passing the same JSON payload built from
legalBasis and privacyPolicyUrl and preserving the HTTP method ("PATCH") and
response handling; remove manual credentials/headers since postDirectus manages
auth/error conventions and adjust error/response checks to use the postDirectus
return shape.
| </div> | ||
| ) : ( | ||
| <ParticipantOnboardingCards project={project as Project} /> | ||
| <ParticipantOnboardingCards project={project as ParticipantProject} /> |
Contributor
There was a problem hiding this comment.
Avoid unsafe cast when rendering onboarding cards
Line 53 force-casts project, which can pass undefined at runtime if the query resolves without data. Guard the render path and pass typed data directly.
Proposed fix
- ) : (
- <ParticipantOnboardingCards project={project as ParticipantProject} />
- )}
+ ) : project ? (
+ <ParticipantOnboardingCards project={project} />
+ ) : (
+ <Alert color="info" {...testId("portal-error-alert")}>
+ {t`An error occurred while loading the Portal. Please contact the support team.`}
+ </Alert>
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ParticipantOnboardingCards project={project as ParticipantProject} /> | |
| ) : project ? ( | |
| <ParticipantOnboardingCards project={project} /> | |
| ) : ( | |
| <Alert color="info" {...testId("portal-error-alert")}> | |
| {t`An error occurred while loading the Portal. Please contact the support team.`} | |
| </Alert> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/routes/participant/ParticipantStart.tsx` at line 53, The
component currently force-casts project to ParticipantProject when rendering
ParticipantOnboardingCards; instead, guard the render so you only render
ParticipantOnboardingCards when project is defined (e.g. project !== undefined
or Boolean(project)) and pass project directly without using the unsafe "as
ParticipantProject" cast. Locate the usage of ParticipantOnboardingCards and the
project variable in ParticipantStart (the query/hook that provides project) and
update the JSX to conditionally render the card component with the real typed
project value.
Comment on lines
+129
to
+132
| if body.legal_basis == "consent": | ||
| update_data["privacy_policy_url"] = body.privacy_policy_url | ||
| else: | ||
| update_data["privacy_policy_url"] = None |
Contributor
There was a problem hiding this comment.
Validate privacy_policy_url before persisting participant-facing links.
Right now any string can be stored and later rendered as a clickable URL in onboarding. Add strict URL validation (scheme + host) before update to avoid malformed or unsafe links.
🛡️ Proposed guard
+from urllib.parse import urlparse
@@
update_data: dict = {"legal_basis": body.legal_basis}
if body.legal_basis == "consent":
- update_data["privacy_policy_url"] = body.privacy_policy_url
+ url = (body.privacy_policy_url or "").strip()
+ if url:
+ parsed = urlparse(url)
+ if parsed.scheme not in {"http", "https"} or not parsed.netloc:
+ raise HTTPException(status_code=400, detail="Invalid privacy policy URL")
+ update_data["privacy_policy_url"] = url
+ else:
+ update_data["privacy_policy_url"] = None
else:
update_data["privacy_policy_url"] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/user_settings.py` around lines 129 - 132, When
setting update_data["privacy_policy_url"] in the user settings flow, validate
body.privacy_policy_url for a safe absolute URL (must parse to a scheme of
http/https and a non-empty host) before assigning; if validation fails or the
legal_basis isn't "consent", set update_data["privacy_policy_url"] = None.
Locate the assignment block that checks body.legal_basis and add a guard that
parses/validates the URL (e.g., using urllib.parse.urlparse or a shared URL
validator helper) and only persists when scheme in ("http","https") and netloc
is present.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/project/ProjectPortalEditor.tsx`:
- Around line 600-611: Update the Anchor component in ProjectPortalEditor to
include a semantic href ("/settings#legal-basis") and make the onClick handler
call event.preventDefault() before invoking settingsNavigate so
keyboard/assistive and non-JS navigation works; keep existing props (size="sm",
style) and the Group/Trans/IconExternalLink children, but change the onClick
signature to accept the event, call event.preventDefault(), then call
settingsNavigate("/settings#legal-basis").
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
echo/frontend/src/components/project/ProjectPortalEditor.tsxecho/frontend/src/routes/settings/UserSettingsRoute.tsx
Comment on lines
+600
to
+611
| <Anchor | ||
| size="sm" | ||
| onClick={() => | ||
| settingsNavigate("/settings#legal-basis") | ||
| } | ||
| style={{ cursor: "pointer" }} | ||
| > | ||
| <Group gap={4}> | ||
| <Trans>Go to Settings</Trans> | ||
| <IconExternalLink size={14} /> | ||
| </Group> | ||
| </Anchor> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check the actual code at lines 600-611 in ProjectPortalEditor.tsx
sed -n '595,620p' echo/frontend/src/components/project/ProjectPortalEditor.tsxRepository: Dembrane/echo
Length of output: 910
🏁 Script executed:
# Find where settingsNavigate is defined/imported
rg "settingsNavigate" echo/frontend/src/components/project/ProjectPortalEditor.tsx -B 5 -A 2Repository: Dembrane/echo
Length of output: 541
🏁 Script executed:
# Check imports and overall structure of the file around that area
sed -n '1,50p' echo/frontend/src/components/project/ProjectPortalEditor.tsxRepository: Dembrane/echo
Length of output: 1546
🏁 Script executed:
# Find the useI18nNavigate hook definition
fd "useI18nNavigate" echo/frontend/src --type fRepository: Dembrane/echo
Length of output: 99
🏁 Script executed:
# Search for the hook implementation
rg "export.*useI18nNavigate\|function useI18nNavigate\|const useI18nNavigate" echo/frontend/src -A 10Repository: Dembrane/echo
Length of output: 39
🏁 Script executed:
# Check Mantine Anchor component usage patterns in this file and others
rg "Anchor" echo/frontend/src/components/project/ProjectPortalEditor.tsx -B 2 -A 5Repository: Dembrane/echo
Length of output: 599
🏁 Script executed:
cat -n echo/frontend/src/hooks/useI18nNavigate.tsRepository: Dembrane/echo
Length of output: 2118
Optional: Make the "Go to Settings" link more semantic with an href attribute.
The Anchor at line 600 uses onClick-driven navigation without href. While it's functional, adding an href attribute and preventDefault pattern would make it more semantic and ensure broader keyboard navigation support across environments.
Suggested fix
- <Anchor
- size="sm"
- onClick={() =>
- settingsNavigate("/settings#legal-basis")
- }
- style={{ cursor: "pointer" }}
- >
+ <Anchor
+ href="/settings#legal-basis"
+ size="sm"
+ onClick={(event) => {
+ event.preventDefault();
+ settingsNavigate("/settings#legal-basis");
+ }}
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Anchor | |
| size="sm" | |
| onClick={() => | |
| settingsNavigate("/settings#legal-basis") | |
| } | |
| style={{ cursor: "pointer" }} | |
| > | |
| <Group gap={4}> | |
| <Trans>Go to Settings</Trans> | |
| <IconExternalLink size={14} /> | |
| </Group> | |
| </Anchor> | |
| <Anchor | |
| href="/settings#legal-basis" | |
| size="sm" | |
| onClick={(event) => { | |
| event.preventDefault(); | |
| settingsNavigate("/settings#legal-basis"); | |
| }} | |
| > | |
| <Group gap={4}> | |
| <Trans>Go to Settings</Trans> | |
| <IconExternalLink size={14} /> | |
| </Group> | |
| </Anchor> |
🤖 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
600 - 611, Update the Anchor component in ProjectPortalEditor to include a
semantic href ("/settings#legal-basis") and make the onClick handler call
event.preventDefault() before invoking settingsNavigate so keyboard/assistive
and non-JS navigation works; keep existing props (size="sm", style) and the
Group/Trans/IconExternalLink children, but change the onClick signature to
accept the event, call event.preventDefault(), then call
settingsNavigate("/settings#legal-basis").
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
echo/server/dembrane/api/user_settings.py (1)
129-132:⚠️ Potential issue | 🟠 MajorValidate
privacy_policy_urlbefore persisting participant-facing links.Line 130 currently stores any string when
legal_basis == "consent". Validate absolutehttp/httpsURLs (non-empty host), otherwise reject or storeNone.Suggested fix
+from urllib.parse import urlparse @@ update_data: dict = {"legal_basis": body.legal_basis} if body.legal_basis == "consent": - update_data["privacy_policy_url"] = body.privacy_policy_url + url = (body.privacy_policy_url or "").strip() + if url: + parsed = urlparse(url) + if parsed.scheme not in {"http", "https"} or not parsed.netloc: + raise HTTPException(status_code=400, detail="Invalid privacy policy URL") + update_data["privacy_policy_url"] = url + else: + update_data["privacy_policy_url"] = None else: update_data["privacy_policy_url"] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/server/dembrane/api/user_settings.py` around lines 129 - 132, The code currently assigns body.privacy_policy_url into update_data when body.legal_basis == "consent" without validation; change this to validate that body.privacy_policy_url is a non-empty absolute URL with scheme "http" or "https" and a non-empty host before persisting—if validation fails, set update_data["privacy_policy_url"] = None (or raise/return a validation error if that fits the surrounding handler). Locate the assignment around body.legal_basis and body.privacy_policy_url and perform URL parsing/validation (e.g., using urllib.parse or your project's URL validator) to ensure scheme in {"http","https"} and netloc is non-empty before storing.
🤖 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/user_settings.py`:
- Around line 61-63: The synchronous requests.post call (response =
requests.post(url, headers=headers, files=files, data=data,
verify=directus.verify)) blocks the event loop; wrap that call in an await
run_in_threadpool(...) like the other Directus calls (see the patterns around
the existing run_in_threadpool usage at the other Directus calls) so the HTTP
upload runs off the event loop, and add/import run_in_threadpool at the top of
the module if missing.
---
Duplicate comments:
In `@echo/server/dembrane/api/user_settings.py`:
- Around line 129-132: The code currently assigns body.privacy_policy_url into
update_data when body.legal_basis == "consent" without validation; change this
to validate that body.privacy_policy_url is a non-empty absolute URL with scheme
"http" or "https" and a non-empty host before persisting—if validation fails,
set update_data["privacy_policy_url"] = None (or raise/return a validation error
if that fits the surrounding handler). Locate the assignment around
body.legal_basis and body.privacy_policy_url and perform URL parsing/validation
(e.g., using urllib.parse or your project's URL validator) to ensure scheme in
{"http","https"} and netloc is non-empty before storing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
echo/server/dembrane/api/user_settings.py
Comment on lines
+61
to
+63
| response = requests.post( | ||
| url, headers=headers, files=files, data=data, verify=directus.verify | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and read relevant sections
wc -l echo/server/dembrane/api/user_settings.pyRepository: Dembrane/echo
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the file with line numbers, focusing on imports and the function around lines 61-63
head -80 echo/server/dembrane/api/user_settings.py | cat -nRepository: Dembrane/echo
Length of output: 3606
🏁 Script executed:
#!/bin/bash
# Search for run_in_thread_pool import and usage
rg -n "run_in_thread_pool|from dembrane.async_helpers" echo/server/dembrane/api/user_settings.pyRepository: Dembrane/echo
Length of output: 252
Move that blocking HTTP call off the event loop.
The requests.post() at lines 61–63 is synchronous and will stall the event loop under load. Wrap it with await run_in_thread_pool() like the other Directus calls in this endpoint do (see lines 107 and 135).
Suggested fix
- response = requests.post(
- url, headers=headers, files=files, data=data, verify=directus.verify
- )
+ response = await run_in_thread_pool(
+ requests.post,
+ url,
+ headers=headers,
+ files=files,
+ data=data,
+ verify=directus.verify,
+ timeout=30,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/api/user_settings.py` around lines 61 - 63, The
synchronous requests.post call (response = requests.post(url, headers=headers,
files=files, data=data, verify=directus.verify)) blocks the event loop; wrap
that call in an await run_in_threadpool(...) like the other Directus calls (see
the patterns around the existing run_in_threadpool usage at the other Directus
calls) so the HTTP upload runs off the event loop, and add/import
run_in_threadpool at the top of the module if missing.
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
1 participant
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
Changes