fix: report feature improvements and bug fixes#530
Merged
Conversation
- Fix rescheduling 400 error: add scheduled_at to PATCH report schema - Fix view counts shared across all reports: scope queries to specific report_id - Fix date_created not updating when scheduled report generates - Fix cancelled/errored report leaving blank page: detect stale selection and fall back to latest completed report - Fix edit mode persisting when switching reports in sidebar - Add max 2 preset selection limit to "Guide the report" focus selector - Rename "custom" sidebar label to "Guided" with formatted tooltip breakdown - Replace "Update Report" button with always "New Report" + indicator for new conversations - Extract shared FeedbackPortalModal component with QR code; reuse in Header and report form - Replace DateTimePicker with split DatePickerInput + TimeInput via new ScheduleDateTimePicker component - Add consistent 10-minute minimum validation for scheduling/rescheduling on both frontend and backend - Allow scheduling reports on projects with no conversations yet; error gracefully if still empty at generation time - Make sidebar fade scroll-aware: hide when scrolled to bottom - Always show QR code in report CTA with clickable hover overlay - Update report prompt templates to enforce markdown H1 title and H3 question headings - Reduce polling intervals: idle 30s->60s, remove duplicate 5s poll during generation, conversation counts 15s->60s - Add translations for all new strings across all 6 locales
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughRestructures feedback collection and report scheduling UX by introducing a unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
LGTM. Solid multi-domain refactor consolidating feedback flow, tightening scheduling constraints, and harmonizing report UX across the platform. The stale-selection handling in ProjectReportRoute is a nice defensive touch, and the 10-minute minimum on the backend pairs well with the new ScheduleDateTimePicker. Localization coverage across all seven languages is thorough. 🚀 🚥 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 |
![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: 18
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/project.py (1)
940-979:⚠️ Potential issue | 🟠 MajorScope auth to the report before counting metrics.
After switching the filter to
project_report_id, this endpoint no longer proves thatreport_idbelongs toproject_id. A caller who can access one project can pass some other report id here and read its counts.Diff
async def get_report_views( project_id: str, report_id: int, auth: DependencyDirectusSession, ) -> dict: """Get view counts for a report.""" await _verify_project_access(auth, project_id) from dembrane.directus import directus + + report = await run_in_thread_pool( + directus.get_item, + "project_report", + str(report_id), + ) + if not report or str(report.get("project_id")) != project_id: + raise HTTPException(status_code=404, detail="Report not found") # Total views for this report all_metrics = await run_in_thread_pool( directus.get_items, "project_report_metric",🤖 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 940 - 979, The endpoint get_report_views currently only calls _verify_project_access(auth, project_id) but then queries project_report_metric by report_id without ensuring that that report_id belongs to project_id; fix by explicitly validating the report belongs to the project before counting metrics: use the directus client (same module used for metrics) to fetch the project_report (e.g., query "project_report" or get_item for id=report_id with filter project_id=project_id) and if no matching report is returned, raise an appropriate error (HTTPException/NotFound/Forbidden) so subsequent queries against project_report_metric (by report_id) are safe.echo/frontend/src/routes/project/report/ProjectReportRoute.tsx (1)
450-463:⚠️ Potential issue | 🟡 MinorUse the app background token for the fade overlay.
var(--mantine-color-white)will render as a white band on non-white themes. This overlay needs to inherit the same surface color as the rest of the report pane. As per coding guidelines "Use CSS variablesvar(--app-background)andvar(--app-text)instead of hardcoded colors like#F6F4F1or#2D2D2Cto ensure theme changes propagate dynamically."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@echo/frontend/src/routes/project/report/ProjectReportRoute.tsx` around lines 450 - 463, The fade overlay uses a hardcoded white token; update the Box rendering controlled by showFade in ProjectReportRoute (the JSX that creates the absolute-positioned fade) to use the app background CSS variable instead of var(--mantine-color-white) — replace that token in the linear-gradient with var(--app-background) so the overlay matches the report pane surface across themes and continues to preserve the same gradient direction and borderRadius behavior.
🤖 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/common/FeedbackPortalModal.tsx`:
- Around line 48-68: In FeedbackPortalModal, replace the three verbose <Text
size="sm"> blocks with two concise lines: one short intro and one brief "what to
include" hint; update the JSX inside the FeedbackPortalModal component (where
the current <Text size="sm">... blocks are) to render a single sentence like
"Tell us what happened or what you'd like to see" and a second sentence like
"Include where it happened and one key detail (steps, error, or expected
outcome)"; keep the <Trans> wrapper around both for i18n and preserve size="sm"
and surrounding layout.
In `@echo/frontend/src/components/common/QRCode.tsx`:
- Around line 63-75: The QRCode component currently uses hardcoded colors
(className includes bg-white, the hover background rgba(65,105,225,0.85), and a
color="white" further down) which breaks theming; update the QRCode.tsx
component to replace bg-white with the app background token/variable (e.g.,
var(--app-background) or the corresponding brand/colors.json token), replace the
hover inline style rgba(...) with the theme/brand primary token or CSS variable
(and preserve opacity via rgba from token or use HSLA/CSS opacity), and replace
color="white" with var(--app-text) or the appropriate brand text token; ensure
changes touch the JSX elements that use hovered state, className and the element
rendering qrElement so the component uses CSS variables/brand tokens instead of
hardcoded values and remains print-hidden as before.
In `@echo/frontend/src/components/report/CreateReportForm.tsx`:
- Around line 288-302: The Tooltip is currently attached directly to the
disabled Button so it won't appear when the button is disabled; wrap the Button
with a neutral container element (e.g., a div or span) and move the Tooltip to
wrap that container instead so hover/focus events fire even when Button (in
CreateReportForm.tsx) is disabled; keep the Button props and handlers (onClick
-> handleCreate, loading -> isPending, disabled -> isPending ||
!hasConversations, color, style, {...testId("report-create-button")}) unchanged
and keep the Tooltip props (label and disabled flag derived from
hasConversations) but apply the Tooltip to the wrapper element rather than the
Button.
In `@echo/frontend/src/components/report/ScheduleDateTimePicker.tsx`:
- Around line 8-13: getMinScheduleDate computes a minimum Date but leaves
seconds/milliseconds non-zero when the minutes are already on a 5-minute
boundary, causing the picker default to be rejected by isDateFarEnough; fix by
ensuring seconds and milliseconds are zeroed in both branches of
getMinScheduleDate (when remainder !== 0 and when remainder === 0) — i.e., after
computing the target minute (or as part of setMinutes when adjusting) call
setSeconds(0, 0) or otherwise set milliseconds to 0 so the returned Date has
seconds and ms == 0.
In `@echo/frontend/src/components/report/UpdateReportModalButton.tsx`:
- Line 137: The Indicator in UpdateReportModalButton.tsx currently uses a
hardcoded color literal color="salmon"; replace that with the appropriate brand
color token from brand/colors.json (import the token into
UpdateReportModalButton and pass it to the Indicator color prop) so the
component uses the project's color system rather than a literal string; ensure
the imported token matches the token name used for this alert/attention state
and that its type matches Indicator's color prop.
In `@echo/frontend/src/locales/de-DE.po`:
- Around line 449-452: The German translation for the msgid "Add conversations
to your project first" in the locales file is misspelled and grammatically
incorrect; update its msgstr to the correct German "Fügen Sie zuerst Gespräche
zu Ihrem Projekt hinzu" (use umlauts and correct capitalization) and scan the
same de-DE.po for other recently changed msgstr entries (the ones paired with
msgid strings updated in CreateReportForm.tsx and related components) to correct
orthography/grammar consistently across the file.
In `@echo/frontend/src/locales/en-US.po`:
- Around line 3747-3749: The .po is auto-generated so don't edit it directly;
instead update the source string in CreateReportForm.tsx where the msgid
originates (search for the exact text "No conversations yet. You can schedule a
report now and conversations will be included once they are added.") and change
the copy to clarify timing (e.g., append or replace with "…if added before the
scheduled time."), then re-run the Lingui extraction/compile step to regenerate
the .po/.json catalogs so the updated string is propagated.
In `@echo/frontend/src/locales/es-ES.po`:
- Around line 2944-2946: The new Spanish translations for the report flow mix
formal and informal second-person (usted vs tú); decide on one register (e.g.,
use tú throughout) and update the source i18n entries for the report flow (look
for keys used by the CreateReportForm component / msgid "It looks like you don't
have a report for this project yet. Generate one to get an overview of your
conversations." and the other reported msgids at the same flow) to the chosen
register, then run the project's i18n extraction/compilation workflow to
regenerate the .po files instead of editing echo/frontend/src/locales/es-ES.po
directly so all occurrences (including those around the referenced msgids) stay
consistent.
- Around line 2433-2434: The Spanish translations for several new msgid entries
(e.g., msgid "Failed to reschedule. Please choose a time further in the future
and try again." and its msgstr) contain orthography and punctuation regressions
(missing accents and opening inverted question marks); do not edit the .po file
directly—open the project’s translation workflow/translation management system,
update the corresponding Spanish translations to include proper accents and
opening ¿ and corrected words (e.g., más, inténtelo, aún, incluirán, áreas,
código), save/approve the updated translations, then regenerate the i18n
catalogs so the corrected .po entries (including the msgstr for the shown msgid
and the other referenced ranges) are produced automatically.
In `@echo/frontend/src/locales/fr-FR.po`:
- Around line 2763-2767: The translation for msgid "Guided" currently uses the
noun/imperative "Guide" in the fr-FR PO entry; update the msgstr for that msgid
to the adjective form "Guidé" so the UI mode label is semantically correct (edit
the msgstr for the msgid "Guided" in the fr-FR.po file).
- Around line 464-467: The French translations in the generated .po (e.g. the
msgid "Add conversations to your project first") contain accent and phrasing
regressions and must be corrected through the localization workflow rather than
manual edits; regenerate/extract the catalog with the Lingui tooling (run your
i18n extraction/update commands), submit the updated keys (including the entries
at the reported ranges such as the msgid above and the other affected msgids) to
your translators/localization team to fix accents and phrasing (e.g. use
"Ajoutez d'abord des conversations à votre projet" with the correct accent and
punctuation), then commit the updated compiled localization artifacts rather
than editing the .po by hand.
In `@echo/frontend/src/locales/nl-NL.po`:
- Around line 6258-6260: Replace the Dutch translation for the msgid in
echo/frontend/src/locales/nl-NL.po (the string used by WhitelabelLogoCard.tsx at
line 121) with a corrected Dutch phrasing: change "Upload een custom logo om het
dembrane logo te vervangen op het portale, dashboard, rapporten, en host gids."
to a proper translation such as "Upload een aangepast logo om het Dembrane-logo
te vervangen in het portaal, dashboard, rapporten en hostgids." ensuring correct
spelling, capitalization, and compound word usage.
- Around line 4752-4755: The Dutch translation for msgid "Report templates are
on our roadmap." must be fully localized (no English fragments); update the
msgstr to a complete Dutch sentence (for example: "Rapportsjablonen staan op
onze routekaart." or another natural Dutch phrasing) so both usages in
UpdateReportModalButton and CreateReportForm use the fully localized string;
edit the msgstr for that msgid in nl-NL.po accordingly.
In `@echo/frontend/src/locales/uk-UA.po`:
- Around line 1265-1268: The Ukrainian translation "Забронюйте дзвiнок з нами"
(msgid "Book a call with us" in FeedbackPortalModal.tsx at the shown entry)
contains a Latin 'i' instead of Cyrillic 'і'; scan the uk-UA.po file for the
listed ranges (e.g., the entry for "Book a call with us" and the other reported
ranges 2690-2692, 3014-3018, 3596-3599, 3747-3750, 5036-5039, 5310-5313) and
replace any Latin characters (notably Latin 'i') with their correct Cyrillic
counterparts (e.g., replace 'i' -> 'і') so all msgstr values use proper Cyrillic
characters and run a spellcheck/preview to confirm no mixed-script artifacts
remain.
- Around line 2153-2156: The uk-UA.po file contains untranslated msgstr entries
for new UI strings (e.g., the message used by ReportFocusSelector component);
update the untranslated msgstr values to their correct Ukrainian translations
for each affected msgid (those referenced by ReportFocusSelector.tsx and the
other new report-focus/feedback flow strings), making sure to keep the original
msgid text and escape sequences unchanged, save the .po file, and run the
localization build/validation (msgfmt or the project's i18n compile step) to
ensure no syntax errors and that the updated strings are picked up in the app.
In `@echo/frontend/src/routes/project/report/ProjectReportRoute.tsx`:
- Around line 710-725: The current isSelectedStale only checks presence in
allReports so a selected report that still exists but has status "cancelled" or
"error" will incorrectly be used; update the isSelectedStale predicate (the
const named isSelectedStale that references selectedReportId and allReports) to
treat a selected report as stale if its status is cancelled or error (i.e. find
the report by id in allReports and return true when not found OR when found but
report.status is "cancelled" or "error"); this ensures the activeReportId logic
(the IIFE that returns selectedReportId) falls back to the latest completed
report instead of using cancelled/errored selections.
In `@echo/server/dembrane/api/project.py`:
- Around line 585-590: The code normalizes tz-naive scheduled_dt to UTC for
validation but then writes the original raw input back to body.scheduled_at;
change it to persist the normalized, tz-aware time instead (e.g., set
body.scheduled_at to the UTC-normalized scheduled_dt ISO string) wherever you
coerce scheduled_dt (the blocks handling scheduled_dt and the subsequent
validation); apply the same fix in the other similar blocks (the other
scheduled_dt validation branches) so the stored value reflects the validated UTC
instant.
In `@echo/server/dembrane/report_utils.py`:
- Around line 250-252: The refetch branch change in report_utils.py must also be
applied to the synchronous worker path: open
echo/server/dembrane/report_generation.py and locate task_create_report_continue
(and any helper that performs the "refetch" path that currently returns the
placeholder string "No conversations available for report"); replace the code
that returns placeholder content with raising ReportGenerationError with the
same message used in report_utils.py ("No conversations available yet. Add
conversations to your project and try again."), ensuring you import
ReportGenerationError if needed and that the Dramatiq worker flow will bubble
the exception rather than producing archived placeholder reports.
---
Outside diff comments:
In `@echo/frontend/src/routes/project/report/ProjectReportRoute.tsx`:
- Around line 450-463: The fade overlay uses a hardcoded white token; update the
Box rendering controlled by showFade in ProjectReportRoute (the JSX that creates
the absolute-positioned fade) to use the app background CSS variable instead of
var(--mantine-color-white) — replace that token in the linear-gradient with
var(--app-background) so the overlay matches the report pane surface across
themes and continues to preserve the same gradient direction and borderRadius
behavior.
In `@echo/server/dembrane/api/project.py`:
- Around line 940-979: The endpoint get_report_views currently only calls
_verify_project_access(auth, project_id) but then queries project_report_metric
by report_id without ensuring that that report_id belongs to project_id; fix by
explicitly validating the report belongs to the project before counting metrics:
use the directus client (same module used for metrics) to fetch the
project_report (e.g., query "project_report" or get_item for id=report_id with
filter project_id=project_id) and if no matching report is returned, raise an
appropriate error (HTTPException/NotFound/Forbidden) so subsequent queries
against project_report_metric (by report_id) are safe.
🪄 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: d4a39f2c-79a3-490d-96c3-3f7b7ad102e9
📒 Files selected for processing (28)
echo/frontend/src/components/common/FeedbackPortalModal.tsxecho/frontend/src/components/common/QRCode.tsxecho/frontend/src/components/layout/Header.tsxecho/frontend/src/components/project/ProjectQRCode.tsxecho/frontend/src/components/report/CreateReportForm.tsxecho/frontend/src/components/report/ReportFocusSelector.tsxecho/frontend/src/components/report/ReportRenderer.tsxecho/frontend/src/components/report/ScheduleDateTimePicker.tsxecho/frontend/src/components/report/UpdateReportModalButton.tsxecho/frontend/src/components/report/hooks/index.tsecho/frontend/src/locales/de-DE.poecho/frontend/src/locales/en-US.poecho/frontend/src/locales/es-ES.poecho/frontend/src/locales/fr-FR.poecho/frontend/src/locales/it-IT.poecho/frontend/src/locales/nl-NL.poecho/frontend/src/locales/uk-UA.poecho/frontend/src/routes/project/report/ProjectReportRoute.tsxecho/server/dembrane/api/project.pyecho/server/dembrane/report_utils.pyecho/server/dembrane/tasks.pyecho/server/prompt_templates/system_report.de.jinjaecho/server/prompt_templates/system_report.en.jinjaecho/server/prompt_templates/system_report.es.jinjaecho/server/prompt_templates/system_report.fr.jinjaecho/server/prompt_templates/system_report.it.jinjaecho/server/prompt_templates/system_report.nl.jinjaecho/server/prompt_templates/system_report.uk.jinja
Comment on lines
+48
to
+68
| <Text size="sm"> | ||
| <Trans> | ||
| We'd love to hear from you. Whether you have an idea for something | ||
| new, you've hit a bug, spotted a translation that feels off, or | ||
| just want to share how things have been going. | ||
| </Trans> | ||
| </Text> | ||
| <Text size="sm"> | ||
| <Trans> | ||
| To help us act on it, try to include where it happened and what | ||
| you were trying to do. For bugs, tell us what went wrong. For | ||
| ideas, tell us what need it would solve for you. | ||
| </Trans> | ||
| </Text> | ||
| <Text size="sm"> | ||
| <Trans> | ||
| Just talk or type naturally. Your input goes directly to our | ||
| product team and genuinely helps us make dembrane better. We read | ||
| everything. | ||
| </Trans> | ||
| </Text> |
Contributor
There was a problem hiding this comment.
Trim the modal copy.
This is a lot of text for a single decision point, and it reads heavier than the rest of the app. I'd compress this to one short intro plus one short “what to include” hint. As per coding guidelines "use shortest possible copy with highest clarity, avoid jargon and corporate speak, write like explaining to a colleague".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/common/FeedbackPortalModal.tsx` around lines 48
- 68, In FeedbackPortalModal, replace the three verbose <Text size="sm"> blocks
with two concise lines: one short intro and one brief "what to include" hint;
update the JSX inside the FeedbackPortalModal component (where the current <Text
size="sm">... blocks are) to render a single sentence like "Tell us what
happened or what you'd like to see" and a second sentence like "Include where it
happened and one key detail (steps, error, or expected outcome)"; keep the
<Trans> wrapper around both for i18n and preserve size="sm" and surrounding
layout.
Comment on lines
+63
to
+75
| className={`relative block cursor-pointer overflow-hidden rounded-lg bg-white transition-all ${className ?? ""}`} | ||
| style={style} | ||
| data-testid={dataTestId} | ||
| onMouseEnter={() => setHovered(true)} | ||
| onMouseLeave={() => setHovered(false)} | ||
| > | ||
| {qrElement} | ||
| <div | ||
| className="absolute inset-0 flex items-center justify-center rounded-lg transition-all print:hidden" | ||
| style={{ | ||
| backgroundColor: hovered | ||
| ? "rgba(65, 105, 225, 0.85)" | ||
| : "transparent", |
Contributor
There was a problem hiding this comment.
Replace hardcoded QR colors with theme tokens/CSS variables.
bg-white, rgba(65, 105, 225, 0.85), and color="white" hardcode palette values in a shared component. Please switch these to the project’s color tokens / CSS variables so theme and whitelabel behavior stay consistent.
As per coding guidelines: echo/**/*.{ts,tsx,css,scss}: Use color tokens from brand/colors.json for programmatic use instead of hardcoded color values and echo/frontend/**/*.{ts,tsx,css}: Use CSS variables var(--app-background) and var(--app-text) instead of hardcoded colors like #F6F4F1or#2D2D2C``.
Also applies to: 81-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/common/QRCode.tsx` around lines 63 - 75, The
QRCode component currently uses hardcoded colors (className includes bg-white,
the hover background rgba(65,105,225,0.85), and a color="white" further down)
which breaks theming; update the QRCode.tsx component to replace bg-white with
the app background token/variable (e.g., var(--app-background) or the
corresponding brand/colors.json token), replace the hover inline style rgba(...)
with the theme/brand primary token or CSS variable (and preserve opacity via
rgba from token or use HSLA/CSS opacity), and replace color="white" with
var(--app-text) or the appropriate brand text token; ensure changes touch the
JSX elements that use hovered state, className and the element rendering
qrElement so the component uses CSS variables/brand tokens instead of hardcoded
values and remains print-hidden as before.
Comment on lines
+288
to
+302
| <Tooltip | ||
| label={t`Add conversations to your project first`} | ||
| disabled={!!hasConversations} | ||
| > | ||
| <Trans>Generate now</Trans> | ||
| </Button> | ||
| <Button | ||
| onClick={() => handleCreate(false)} | ||
| loading={isPending} | ||
| disabled={isPending || !hasConversations} | ||
| color="primary" | ||
| style={{ flex: 7 }} | ||
| {...testId("report-create-button")} | ||
| > | ||
| <Trans>Generate now</Trans> | ||
| </Button> | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd echo/frontend/src/components/report && wc -l CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 81
🏁 Script executed:
cd echo/frontend/src/components/report && sed -n '285,305p' CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 627
🏁 Script executed:
cd echo/frontend/src/components && grep -r "Tooltip" --include="*.tsx" | grep -i "disabled" | head -20Repository: Dembrane/echo
Length of output: 675
🏁 Script executed:
sed -n '1,80p' echo/frontend/src/components/common/NavigationButton.tsxRepository: Dembrane/echo
Length of output: 2060
🏁 Script executed:
rg "Box.*Tooltip|Tooltip.*Box" echo/frontend/src/components --type tsx -A 3 -B 3Repository: Dembrane/echo
Length of output: 84
🏁 Script executed:
rg "Box.*Tooltip|Tooltip.*Box" echo/frontend/src/components -A 3 -B 3Repository: Dembrane/echo
Length of output: 1242
🏁 Script executed:
sed -n '40,75p' echo/frontend/src/components/common/NavigationButton.tsxRepository: Dembrane/echo
Length of output: 1015
🏁 Script executed:
rg "disabled.*Button|Button.*disabled" echo/frontend/src/components/report/CreateReportForm.tsx -B 3 -A 5Repository: Dembrane/echo
Length of output: 39
🏁 Script executed:
rg "Tooltip.*disabled|disabled.*Tooltip" echo/frontend/src/components --type-list | head -20Repository: Dembrane/echo
Length of output: 446
🏁 Script executed:
fd "CreateReportForm" echo/frontend/src/components/reportRepository: Dembrane/echo
Length of output: 113
🏁 Script executed:
sed -n '65,90p' echo/frontend/src/components/common/NavigationButton.tsxRepository: Dembrane/echo
Length of output: 807
🏁 Script executed:
rg "disabled.*\!hasConversations|!hasConversations.*disabled" echo/frontend/src/components/report/CreateReportForm.tsx -B 5 -A 5Repository: Dembrane/echo
Length of output: 189
🏁 Script executed:
grep -n "hasConversations" echo/frontend/src/components/report/CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 350
🏁 Script executed:
sed -n '288,305p' echo/frontend/src/components/report/CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 570
🏁 Script executed:
rg "Tooltip.*Button.*disabled|disabled.*Button.*Tooltip" echo/frontend/src/components -A 5 -B 2Repository: Dembrane/echo
Length of output: 39
🏁 Script executed:
grep -n "import.*Box\|from.*mantine" echo/frontend/src/components/report/CreateReportForm.tsx | head -20Repository: Dembrane/echo
Length of output: 83
🏁 Script executed:
sed -n '1,20p' echo/frontend/src/components/report/CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 367
🏁 Script executed:
rg "^import.*Box" echo/frontend/src/components/report/CreateReportForm.tsxRepository: Dembrane/echo
Length of output: 39
Wrap the button in a neutral container so the tooltip shows when disabled.
The button is disabled exactly when the hint matters most, but disabled buttons don't trigger hover/focus events. Move the Tooltip to a wrapper instead:
Suggested fix
- <Tooltip
- label={t`Add conversations to your project first`}
- disabled={!!hasConversations}
- >
- <Button
- onClick={() => handleCreate(false)}
- loading={isPending}
- disabled={isPending || !hasConversations}
- color="primary"
- style={{ flex: 7 }}
- {...testId("report-create-button")}
- >
- <Trans>Generate now</Trans>
- </Button>
- </Tooltip>
+ <Tooltip
+ label={t`Add conversations to your project first`}
+ disabled={!!hasConversations}
+ >
+ <Box style={{ flex: 7 }}>
+ <Button
+ onClick={() => handleCreate(false)}
+ loading={isPending}
+ disabled={isPending || !hasConversations}
+ color="primary"
+ fullWidth
+ {...testId("report-create-button")}
+ >
+ <Trans>Generate now</Trans>
+ </Button>
+ </Box>
+ </Tooltip>📝 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
| <Tooltip | |
| label={t`Add conversations to your project first`} | |
| disabled={!!hasConversations} | |
| > | |
| <Trans>Generate now</Trans> | |
| </Button> | |
| <Button | |
| onClick={() => handleCreate(false)} | |
| loading={isPending} | |
| disabled={isPending || !hasConversations} | |
| color="primary" | |
| style={{ flex: 7 }} | |
| {...testId("report-create-button")} | |
| > | |
| <Trans>Generate now</Trans> | |
| </Button> | |
| </Tooltip> | |
| <Tooltip | |
| label={t`Add conversations to your project first`} | |
| disabled={!!hasConversations} | |
| > | |
| <Box style={{ flex: 7 }}> | |
| <Button | |
| onClick={() => handleCreate(false)} | |
| loading={isPending} | |
| disabled={isPending || !hasConversations} | |
| color="primary" | |
| fullWidth | |
| {...testId("report-create-button")} | |
| > | |
| <Trans>Generate now</Trans> | |
| </Button> | |
| </Box> | |
| </Tooltip> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/report/CreateReportForm.tsx` around lines 288 -
302, The Tooltip is currently attached directly to the disabled Button so it
won't appear when the button is disabled; wrap the Button with a neutral
container element (e.g., a div or span) and move the Tooltip to wrap that
container instead so hover/focus events fire even when Button (in
CreateReportForm.tsx) is disabled; keep the Button props and handlers (onClick
-> handleCreate, loading -> isPending, disabled -> isPending ||
!hasConversations, color, style, {...testId("report-create-button")}) unchanged
and keep the Tooltip props (label and disabled flag derived from
hasConversations) but apply the Tooltip to the wrapper element rather than the
Button.
Comment on lines
+8
to
+13
| function getMinScheduleDate(): Date { | ||
| const d = new Date(Date.now() + 10 * 60_000); | ||
| const mins = d.getMinutes(); | ||
| const remainder = mins % 5; | ||
| if (remainder !== 0) d.setMinutes(mins + (5 - remainder), 0, 0); | ||
| return d; |
Contributor
There was a problem hiding this comment.
Zero out seconds even when the time is already on a 5-minute boundary.
Right now 12:50:37 + 10m becomes 13:00:37, but the picker defaults to 13:00. isDateFarEnough then rejects that default for another 37 seconds, so the form can show a valid-looking time as “too soon.” Force seconds and milliseconds to 0 in both branches.
Suggested fix
function getMinScheduleDate(): Date {
const d = new Date(Date.now() + 10 * 60_000);
const mins = d.getMinutes();
const remainder = mins % 5;
- if (remainder !== 0) d.setMinutes(mins + (5 - remainder), 0, 0);
+ if (remainder !== 0) {
+ d.setMinutes(mins + (5 - remainder), 0, 0);
+ } else {
+ d.setSeconds(0, 0);
+ }
return d;
}📝 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
| function getMinScheduleDate(): Date { | |
| const d = new Date(Date.now() + 10 * 60_000); | |
| const mins = d.getMinutes(); | |
| const remainder = mins % 5; | |
| if (remainder !== 0) d.setMinutes(mins + (5 - remainder), 0, 0); | |
| return d; | |
| function getMinScheduleDate(): Date { | |
| const d = new Date(Date.now() + 10 * 60_000); | |
| const mins = d.getMinutes(); | |
| const remainder = mins % 5; | |
| if (remainder !== 0) { | |
| d.setMinutes(mins + (5 - remainder), 0, 0); | |
| } else { | |
| d.setSeconds(0, 0); | |
| } | |
| return d; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/report/ScheduleDateTimePicker.tsx` around lines
8 - 13, getMinScheduleDate computes a minimum Date but leaves
seconds/milliseconds non-zero when the minutes are already on a 5-minute
boundary, causing the picker default to be rejected by isDateFarEnough; fix by
ensuring seconds and milliseconds are zeroed in both branches of
getMinScheduleDate (when remainder !== 0 and when remainder === 0) — i.e., after
computing the target minute (or as part of setMinutes when adjusting) call
setSeconds(0, 0) or otherwise set milliseconds to 0 so the returned Date has
seconds and ms == 0.
| {doesReportNeedUpdate ? ( | ||
| <Trans>Update Report</Trans> | ||
| ) : ( | ||
| <Indicator disabled={!needsUpdate} color="salmon" size={10} offset={4}> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace hardcoded indicator color with a brand color token.
Line 137 introduces color="salmon". Please route this through the project’s color token system instead of a hardcoded color literal.
As per coding guidelines: "Use color tokens from brand/colors.json for programmatic use instead of hardcoded color values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/components/report/UpdateReportModalButton.tsx` at line 137,
The Indicator in UpdateReportModalButton.tsx currently uses a hardcoded color
literal color="salmon"; replace that with the appropriate brand color token from
brand/colors.json (import the token into UpdateReportModalButton and pass it to
the Indicator color prop) so the component uses the project's color system
rather than a literal string; ensure the imported token matches the token name
used for this alert/attention state and that its type matches Indicator's color
prop.
Comment on lines
+1265
to
+1268
| #: src/components/common/FeedbackPortalModal.tsx:90 | ||
| msgid "Book a call with us" | ||
| msgstr "Забронюйте дзвiнок з нами" | ||
|
|
Contributor
There was a problem hiding this comment.
Fix mixed Cyrillic/Latin characters in Ukrainian translations.
Several translated strings use Latin i instead of Cyrillic і (e.g., Line 1267, Line 3017, Line 3598). This creates visible spelling artifacts and inconsistent text processing.
🔧 Suggested normalization examples
-msgstr "Забронюйте дзвiнок з нами"
+msgstr "Забронюйте дзвінок з нами"
-msgstr "З керiвництвом"
+msgstr "З керівництвом"
-msgstr "Новi розмови додано пiсля цього звiту"
+msgstr "Нові розмови додано після цього звіту"
-msgstr "Скануйте або натиснiть, щоб вiдкрити портал зворотного зв'язку"
+msgstr "Скануйте або натисніть, щоб відкрити портал зворотного зв'язку"
-msgstr "Подiлiться своїм голосом, вiдсканувавши QR-код"
+msgstr "Поділіться своїм голосом, відсканувавши QR-код"Also applies to: 2690-2692, 3014-3018, 3596-3599, 3747-3750, 5036-5039, 5310-5313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/locales/uk-UA.po` around lines 1265 - 1268, The Ukrainian
translation "Забронюйте дзвiнок з нами" (msgid "Book a call with us" in
FeedbackPortalModal.tsx at the shown entry) contains a Latin 'i' instead of
Cyrillic 'і'; scan the uk-UA.po file for the listed ranges (e.g., the entry for
"Book a call with us" and the other reported ranges 2690-2692, 3014-3018,
3596-3599, 3747-3750, 5036-5039, 5310-5313) and replace any Latin characters
(notably Latin 'i') with their correct Cyrillic counterparts (e.g., replace 'i'
-> 'і') so all msgstr values use proper Cyrillic characters and run a
spellcheck/preview to confirm no mixed-script artifacts remain.
Comment on lines
+2153
to
2156
| #: src/components/report/ReportFocusSelector.tsx:197 | ||
| msgid "e.g. \"Focus on sustainability themes\" or \"What do participants think about the new policy?\"" | ||
| msgstr "e.g. \"Focus on sustainability themes\" or \"What do participants think about the new policy?\"" | ||
|
|
Contributor
There was a problem hiding this comment.
Localize the remaining new UI strings in uk-UA before shipping.
LGTM on the direction, but these changed entries are still English in msgstr (e.g., Line 3011, Line 4003, Line 3265). This leaves the new report-focus/feedback flow partially untranslated for Ukrainian users.
⚡ Suggested patch (examples)
msgid "Guide the report"
-msgstr "Guide the report"
+msgstr "Спрямуйте звіт"
msgid "Or write your own"
-msgstr "Or write your own"
+msgstr "Або напишіть власний"
msgid "e.g. \"Focus on sustainability themes\" or \"What do participants think about the new policy?\""
-msgstr "e.g. \"Focus on sustainability themes\" or \"What do participants think about the new policy?\""
+msgstr "Напр.: \"Зосередьтеся на темах сталого розвитку\" або \"Що учасники думають про нову політику?\""
msgid "We'd love to hear from you. Whether you have an idea for something new, you've hit a bug, spotted a translation that feels off, or just want to share how things have been going."
-msgstr "We'd love to hear from you. Whether you have an idea for something new, you've hit a bug, spotted a translation that feels off, or just want to share how things have been going."
+msgstr "Ми будемо раді вашому відгуку. Чи то нова ідея, чи баг, чи неточний переклад, або просто бажання поділитися досвідом."
msgid "To help us act on it, try to include where it happened and what you were trying to do. For bugs, tell us what went wrong. For ideas, tell us what need it would solve for you."
-msgstr "To help us act on it, try to include where it happened and what you were trying to do. For bugs, tell us what went wrong. For ideas, tell us what need it would solve for you."
+msgstr "Щоб ми могли швидше відреагувати, вкажіть, де це сталося і що ви намагалися зробити. Для багів — що саме пішло не так. Для ідей — яку вашу потребу це вирішить."Also applies to: 3010-3013, 4002-4005, 3264-3267, 6058-6061, 6657-6660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/locales/uk-UA.po` around lines 2153 - 2156, The uk-UA.po
file contains untranslated msgstr entries for new UI strings (e.g., the message
used by ReportFocusSelector component); update the untranslated msgstr values to
their correct Ukrainian translations for each affected msgid (those referenced
by ReportFocusSelector.tsx and the other new report-focus/feedback flow
strings), making sure to keep the original msgid text and escape sequences
unchanged, save the .po file, and run the localization build/validation (msgfmt
or the project's i18n compile step) to ensure no syntax errors and that the
updated strings are picked up in the app.
Comment on lines
+710
to
725
| // If the selected report was cancelled/errored (or no longer in the list), | ||
| // fall through to the latest completed report instead of showing an empty page. | ||
| const isSelectedStale = | ||
| selectedReportId !== null && | ||
| allReports !== undefined && | ||
| !allReports.some((r) => r.id === selectedReportId); | ||
|
|
||
| // Determine which report to display (never load content for a scheduled or draft report) | ||
| const activeReportId = (() => { | ||
| if (selectedReportId && !isViewingScheduled && !isViewingGenerating) | ||
| if ( | ||
| selectedReportId && | ||
| !isViewingScheduled && | ||
| !isViewingGenerating && | ||
| !isSelectedStale | ||
| ) | ||
| return selectedReportId; |
Contributor
There was a problem hiding this comment.
Cancelled and errored selections still slip through this guard.
isSelectedStale only checks whether the id disappeared from allReports. If the selected report is still present but has moved to cancelled or error, Line 725 still returns it and bypasses the fallback-to-latest-completed path. Fold the selected report status into this predicate.
Suggested fix
- const isSelectedStale =
- selectedReportId !== null &&
- allReports !== undefined &&
- !allReports.some((r) => r.id === selectedReportId);
+ const selectedReport =
+ selectedReportId !== null
+ ? allReports?.find((r) => r.id === selectedReportId)
+ : undefined;
+
+ const isSelectedStale =
+ selectedReportId !== null &&
+ allReports !== undefined &&
+ (!selectedReport ||
+ selectedReport.status === "cancelled" ||
+ selectedReport.status === "error");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If the selected report was cancelled/errored (or no longer in the list), | |
| // fall through to the latest completed report instead of showing an empty page. | |
| const isSelectedStale = | |
| selectedReportId !== null && | |
| allReports !== undefined && | |
| !allReports.some((r) => r.id === selectedReportId); | |
| // Determine which report to display (never load content for a scheduled or draft report) | |
| const activeReportId = (() => { | |
| if (selectedReportId && !isViewingScheduled && !isViewingGenerating) | |
| if ( | |
| selectedReportId && | |
| !isViewingScheduled && | |
| !isViewingGenerating && | |
| !isSelectedStale | |
| ) | |
| return selectedReportId; | |
| // If the selected report was cancelled/errored (or no longer in the list), | |
| // fall through to the latest completed report instead of showing an empty page. | |
| const selectedReport = | |
| selectedReportId !== null | |
| ? allReports?.find((r) => r.id === selectedReportId) | |
| : undefined; | |
| const isSelectedStale = | |
| selectedReportId !== null && | |
| allReports !== undefined && | |
| (!selectedReport || | |
| selectedReport.status === "cancelled" || | |
| selectedReport.status === "error"); | |
| // Determine which report to display (never load content for a scheduled or draft report) | |
| const activeReportId = (() => { | |
| if ( | |
| selectedReportId && | |
| !isViewingScheduled && | |
| !isViewingGenerating && | |
| !isSelectedStale | |
| ) | |
| return selectedReportId; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/frontend/src/routes/project/report/ProjectReportRoute.tsx` around lines
710 - 725, The current isSelectedStale only checks presence in allReports so a
selected report that still exists but has status "cancelled" or "error" will
incorrectly be used; update the isSelectedStale predicate (the const named
isSelectedStale that references selectedReportId and allReports) to treat a
selected report as stale if its status is cancelled or error (i.e. find the
report by id in allReports and return true when not found OR when found but
report.status is "cancelled" or "error"); this ensures the activeReportId logic
(the IIFE that returns selectedReportId) falls back to the latest completed
report instead of using cancelled/errored selections.
Comment on lines
585
to
+590
| if scheduled_dt.tzinfo is None: | ||
| scheduled_dt = scheduled_dt.replace(tzinfo=tz.utc) | ||
| if scheduled_dt <= datetime.now(tz.utc): | ||
| from datetime import timedelta as td | ||
|
|
||
| min_schedule_time = datetime.now(tz.utc) + td(minutes=10) | ||
| if scheduled_dt <= min_schedule_time: |
Contributor
There was a problem hiding this comment.
Persist the normalized schedule time, not the raw input.
Both branches coerce tz-naive values to UTC for validation, then write body.scheduled_at back unchanged. A value like 2026-04-14T15:00:00 passes as UTC here and then gets stored without that timezone, so the scheduler can act on a different instant later.
Diff
if scheduled_dt.tzinfo is None:
scheduled_dt = scheduled_dt.replace(tzinfo=tz.utc)
+ canonical_scheduled_at = scheduled_dt.astimezone(tz.utc).isoformat()
min_schedule_time = datetime.now(tz.utc) + td(minutes=10)
if scheduled_dt <= min_schedule_time:
raise HTTPException(
status_code=400,
detail="Scheduled time must be at least 10 minutes in the future",
)
is_scheduled = True
@@
if is_scheduled:
- update_fields["scheduled_at"] = body.scheduled_at
+ update_fields["scheduled_at"] = canonical_scheduled_at if isinstance(scheduled_dt, datetime) and scheduled_dt.tzinfo is None:
scheduled_dt = scheduled_dt.replace(tzinfo=tz.utc)
+ canonical_scheduled_at = scheduled_dt.astimezone(tz.utc).isoformat()
min_schedule_time = datetime.now(tz.utc) + timedelta(minutes=10)
if isinstance(scheduled_dt, datetime) and scheduled_dt <= min_schedule_time:
raise HTTPException(
status_code=400,
detail="Scheduled time must be at least 10 minutes in the future",
)
- payload["scheduled_at"] = body.scheduled_at
+ payload["scheduled_at"] = canonical_scheduled_atAlso applies to: 624-629, 795-820
🤖 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 585 - 590, The code
normalizes tz-naive scheduled_dt to UTC for validation but then writes the
original raw input back to body.scheduled_at; change it to persist the
normalized, tz-aware time instead (e.g., set body.scheduled_at to the
UTC-normalized scheduled_dt ISO string) wherever you coerce scheduled_dt (the
blocks handling scheduled_dt and the subsequent validation); apply the same fix
in the other similar blocks (the other scheduled_dt validation branches) so the
stored value reflects the validated UTC instant.
Comment on lines
250
to
+252
| if not conversations: | ||
| logger.warning(f"No conversations found on refetch for project {project_id}") | ||
| return "No conversations available for report" | ||
| raise ReportGenerationError("No conversations available yet. Add conversations to your project and try again.") |
Contributor
There was a problem hiding this comment.
This fix misses the live report generator.
task_create_report_continue() still runs through echo/server/dembrane/report_generation.py, and that refetch branch still returns placeholder content instead of raising. Scheduled runs can still get archived with "No conversations available for report" even with this change here.
Based on learnings: "Report generation is fully synchronous (no asyncio) and runs in Dramatiq network-queue workers via dembrane/report_generation.py."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@echo/server/dembrane/report_utils.py` around lines 250 - 252, The refetch
branch change in report_utils.py must also be applied to the synchronous worker
path: open echo/server/dembrane/report_generation.py and locate
task_create_report_continue (and any helper that performs the "refetch" path
that currently returns the placeholder string "No conversations available for
report"); replace the code that returns placeholder content with raising
ReportGenerationError with the same message used in report_utils.py ("No
conversations available yet. Add conversations to your project and try again."),
ensuring you import ReportGenerationError if needed and that the Dramatiq worker
flow will bubble the exception rather than producing archived placeholder
reports.
ussaama
previously approved these changes
Apr 14, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
ussaama
approved these changes
Apr 14, 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements