feat(web): redesign the wizard UI on the Metzler design system with full UI i18n - #163
feat(web): redesign the wizard UI on the Metzler design system with full UI i18n#163Liohtml wants to merge 1 commit into
Conversation
…ull UI i18n Complete UI/UX overhaul of the browser wizard: - Rebuild index.html on the Metzler design system: design tokens (teal palette, graphite text scale, spacing/radius/shadow tokens), 100rem container, PDP stepper instead of ad-hoc tabs, floating-label form inputs, kit cards/alerts/buttons/badges/file-download components, teal-700 footer, and inline stroke icons throughout. All measurements in rem, all colors from tokens. - Localize the entire UI via the existing i18n catalogs: ~70 new ui_* keys in en/de/fr/es plus a header language switcher (?lang=, validated server-side, falls back to MEDCHECK_LANGUAGE). - Replace the fake progress simulation with an honest submit flow: POST /api/analyze as JSON and surface the real server response (501 preview -> warning alert, other errors -> danger alert), with text inserted via textContent only. - Add an explicit cloud-LLM consent checkbox (allow_cloud_llm) that is required for cloud models and hidden for the local model, mirroring the CLI --allow-cloud-llm consent gate. - Align the form with the API schema: report languages limited to en/de/fr/es and formats to pdf/json/html (pt/ja/zh and txt used to fail validation), anatomy options matched to the shipped prompt regions. - Serve the favicon from /static so the strict CSP (default-src 'self') keeps blocking data: URIs; keep htmx vendored and zero inline JS. - Tests: language switcher, fallback for unknown locales, API-supported report languages only, and i18n catalog key parity across locales. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GCe7Hb5biAZmtDcVPuqT2z
📝 WalkthroughWalkthroughThe web interface now supports English, German, French, and Spanish. The index route selects the locale. The template provides a redesigned three-step wizard. JavaScript submits analyses through JSON and handles consent, progress, results, and errors. ChangesLocalized analysis wizard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant IndexRoute
participant I18nCatalog
participant AnalysisAPI
Browser->>IndexRoute: Request homepage with lang
IndexRoute->>I18nCatalog: Load selected locale
I18nCatalog-->>IndexRoute: Return localized strings
IndexRoute-->>Browser: Render localized wizard
Browser->>AnalysisAPI: POST analysis payload as JSON
AnalysisAPI-->>Browser: Return progress, success, warning, or error response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/test_i18n.py (1)
21-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the parity test to catch keys removed from all catalogs.
The test compares locales against English only. If a key is dropped from all four catalogs, the assertions still pass, and
index.htmlrenders an empty string for that label. Jinja2 resolves an unknown attribute toUndefined, which renders as empty text instead of raising.Add an assertion that every
t.<key>reference inindex.htmlexists in the English catalog.♻️ Proposed additional assertion
def test_catalogs_have_identical_keys(): # Every locale must carry the full key set (report + ui_*) so no language # silently falls back to English for individual strings. keys = {} for lang in ("en", "de", "fr", "es"): with open(I18N_DIR / f"{lang}.json", encoding="utf-8") as f: keys[lang] = set(json.load(f)) assert keys["de"] == keys["en"] assert keys["fr"] == keys["en"] assert keys["es"] == keys["en"] + + +def test_template_keys_exist_in_catalog(): + # Guard against a key that is removed from every catalog at once: Jinja2 + # would render an empty label instead of raising. + import re + from pathlib import Path + + template = Path(__file__).parents[2] / "src/medcheck/web/templates/index.html" + used = set(re.findall(r"\bt\.([a-zA-Z0-9_]+)", template.read_text(encoding="utf-8"))) + with open(I18N_DIR / "en.json", encoding="utf-8") as f: + available = set(json.load(f)) + assert used <= available, sorted(used - available)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_i18n.py` around lines 21 - 30, Extend test_catalogs_have_identical_keys to parse index.html, collect every referenced t.<key> translation key, and assert those keys are a subset of keys["en"]. Preserve the existing locale parity assertions.tests/unit/test_web.py (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the configured-default branch, not only the English clamp.
index()falls back tosettings.default_languagefor unknown?lang=, then clamps unsupported configured defaults to"en". This test uses default settings, so both branches return English. Add a case withdefault_languageset to a non-English value to ensure the lookup is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_web.py` around lines 99 - 104, The test test_homepage_unknown_language_falls_back_to_default currently only exercises the English default. Add a case that configures settings.default_language to a supported non-English language, requests an unknown lang value, and asserts the response uses that configured language and its catalog content, while retaining coverage for the unsupported-default clamp to English.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/medcheck/web/static/app.js`:
- Around line 86-102: Update submitAnalysis in src/medcheck/web/static/app.js at
lines 86-102 to submit the selected file bytes via FormData or upload it first
and use the returned identifier as source, and include all checked
input[name="modules"] values in the request. In
src/medcheck/web/templates/index.html lines 1108-1151, retain the module tiles
only when their selections are included in the request. In
src/medcheck/web/templates/index.html lines 977-993, wire the dropzone/fileInput
upload to the request so the “Auto-detect active” state reflects an actual
upload, or disable the dropzone and clearly limit the flow to remote URLs.
- Around line 112-132: Update the response handling in the fetch promise to read
the body defensively, only parse valid JSON, and fall back to the response
status text for empty or non-JSON responses while preserving the existing result
flow. Add and use a localized generic-failure message in the catch branch
instead of rendering String(err), including for parser and network failures.
- Around line 104-111: Add a timeout around the fetch flow in the submit handler
so stalled analyze requests reject and reach the existing final/settle handler
that re-enables startBtn. Ensure the timeout is cleared when the request
settles, and preserve the current success and error handling for responses
completed before the timeout.
In `@src/medcheck/web/templates/index.html`:
- Line 379: Update the .pdp-stepper-step:not(.active) .pdp-stepper-label rule so
inactive labels are visually hidden without using display:none, while keeping
them available to assistive technologies and preserving the existing visual
layout.
- Around line 961-964: Update the analyzeForm fallback behavior so it does not
natively submit multipart data to /api/analyze when JavaScript is unavailable.
Point the form at a non-submitting target or add a noscript notice indicating
that JavaScript is required, while preserving the existing JavaScript JSON
submission flow.
---
Nitpick comments:
In `@tests/unit/test_i18n.py`:
- Around line 21-30: Extend test_catalogs_have_identical_keys to parse
index.html, collect every referenced t.<key> translation key, and assert those
keys are a subset of keys["en"]. Preserve the existing locale parity assertions.
In `@tests/unit/test_web.py`:
- Around line 99-104: The test
test_homepage_unknown_language_falls_back_to_default currently only exercises
the English default. Add a case that configures settings.default_language to a
supported non-English language, requests an unknown lang value, and asserts the
response uses that configured language and its catalog content, while retaining
coverage for the unsupported-default clamp to English.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d02c4c39-71a4-442c-b5fa-b92b44bb82bc
⛔ Files ignored due to path filters (1)
src/medcheck/web/static/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
src/medcheck/i18n/de.jsonsrc/medcheck/i18n/en.jsonsrc/medcheck/i18n/es.jsonsrc/medcheck/i18n/fr.jsonsrc/medcheck/web/app.pysrc/medcheck/web/static/app.jssrc/medcheck/web/templates/index.htmltests/unit/test_i18n.pytests/unit/test_web.py
| var urlInput = document.getElementById('sourceUrl'); | ||
| var fileInput = document.getElementById('fileInput'); | ||
| var source = (urlInput && urlInput.value.trim()) || | ||
| (fileInput && fileInput.files && fileInput.files[0] && fileInput.files[0].name) || | ||
| 'browser-upload'; | ||
|
|
||
| var anatomy = document.getElementById('anatomy'); | ||
| var language = document.getElementById('reportLanguage'); | ||
| var format = document.getElementById('reportFormat'); | ||
|
|
||
| var body = { | ||
| source: source, | ||
| report_format: format ? format.value : 'json', | ||
| language: language ? language.value : 'en', | ||
| allow_cloud_llm: !!(cloud && consent && consent.checked) | ||
| }; | ||
| if (anatomy && anatomy.value) body.anatomy = anatomy.value; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
The submitted payload ignores most of the wizard form. submitAnalysis builds a fixed object with source, report_format, language, allow_cloud_llm, and an optional anatomy. The template presents an upload dropzone and six module checkboxes that imply the server receives them. The selected file is reduced to its name, and the module selection is discarded. A user who uploads a scan and selects modules gets an analysis request that contains neither.
src/medcheck/web/static/app.js#L86-L102: build the request from the form state instead of a fixed literal. Readmodulesfrom the checkedinput[name="modules"]elements. To transmit the file bytes, sendFormDatato a multipart endpoint, or upload the file first and send the returned identifier assource. Line 89 currently sends onlyfileInput.files[0].name, which the server cannot resolve.src/medcheck/web/templates/index.html#L1108-L1151: after the payload carriesmodules, keep these tiles. Until then, the six checkboxes and the:has(.checkbox-input:checked)highlight at Line 542 show state that has no effect on the request.src/medcheck/web/templates/index.html#L977-L993: the dropzone and#fileInputaccept a file, andfileSelectedreveals the "Auto-detect active" badge atapp.jsLines 35-36. That badge confirms an upload that does not happen. Either wire the upload, or disable the dropzone and state that the browser flow accepts a remote URL only.
📍 Affects 2 files
src/medcheck/web/static/app.js#L86-L102(this comment)src/medcheck/web/templates/index.html#L1108-L1151src/medcheck/web/templates/index.html#L977-L993
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/medcheck/web/static/app.js` around lines 86 - 102, Update submitAnalysis
in src/medcheck/web/static/app.js at lines 86-102 to submit the selected file
bytes via FormData or upload it first and use the returned identifier as source,
and include all checked input[name="modules"] values in the request. In
src/medcheck/web/templates/index.html lines 1108-1151, retain the module tiles
only when their selections are included in the request. In
src/medcheck/web/templates/index.html lines 977-993, wire the dropzone/fileInput
upload to the request so the “Auto-detect active” state reflects an actual
upload, or disable the dropzone and clearly limit the flow to remote URLs.
| if (startBtn) startBtn.disabled = true; | ||
| setProgress(30, form.getAttribute('data-msg-sending')); | ||
|
|
||
| fetch(form.getAttribute('action'), { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a request timeout so the submit button cannot stay disabled.
Line 104 disables #startBtn. The settle handler at Line 133 re-enables it. fetch has no default timeout. If the connection stalls, no handler runs, and the button stays disabled for the rest of the page session. The user then has no way to retry without a reload.
The analyze endpoint is rate limited at app.py Line 189 and is described as expensive, so a slow or stalled response is realistic.
🛡️ Proposed fix
if (startBtn) startBtn.disabled = true;
setProgress(30, form.getAttribute('data-msg-sending'));
+ var controller = new AbortController();
+ var timeoutId = window.setTimeout(function () { controller.abort(); }, 120000);
+
fetch(form.getAttribute('action'), {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify(body)
+ body: JSON.stringify(body),
+ signal: controller.signal
})Clear the timer in the final handler:
.then(function () {
+ window.clearTimeout(timeoutId);
if (startBtn) startBtn.disabled = false;
});🧰 Tools
🪛 ast-grep (0.45.0)
[error] 104-104: React's useState should not be directly called
Context: setProgress(30, form.getAttribute('data-msg-sending'))
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/medcheck/web/static/app.js` around lines 104 - 111, Add a timeout around
the fetch flow in the submit handler so stalled analyze requests reject and
reach the existing final/settle handler that re-enables startBtn. Ensure the
timeout is cleared when the request settles, and preserve the current success
and error handling for responses completed before the timeout.
| .then(function (resp) { | ||
| return resp.json().then(function (data) { | ||
| return { ok: resp.ok, status: resp.status, data: data }; | ||
| }); | ||
| }) | ||
| .then(function (result) { | ||
| var detail = result.data && result.data.detail; | ||
| if (typeof detail !== 'string') detail = JSON.stringify(result.data); | ||
| if (result.ok) { | ||
| setProgress(100, ''); | ||
| showResultAlert('success', detail); | ||
| } else { | ||
| setProgress(0, form.getAttribute('data-msg-waiting')); | ||
| // 501 = known preview limitation -> warning; anything else -> danger. | ||
| showResultAlert(result.status === 501 ? 'warning' : 'danger', detail); | ||
| } | ||
| }) | ||
| .catch(function (err) { | ||
| setProgress(0, form.getAttribute('data-msg-waiting')); | ||
| showResultAlert('danger', String(err)); | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A non-JSON response surfaces a raw parser message to the user.
Line 113 calls resp.json() for every response. A 502 HTML error page from a proxy, a 204 with an empty body, or any non-JSON error body rejects that promise. Control passes to the .catch at Line 129, and Line 131 renders String(err), for example SyntaxError: Unexpected token < in JSON at position 0.
The user sees an internal parser string instead of a localized message. Read the body defensively and fall back to the status text.
🛡️ Proposed fix
.then(function (resp) {
- return resp.json().then(function (data) {
- return { ok: resp.ok, status: resp.status, data: data };
- });
+ return resp.text().then(function (raw) {
+ var data = null;
+ try { data = JSON.parse(raw); } catch (e) { data = null; }
+ return { ok: resp.ok, status: resp.status, data: data };
+ });
})
.then(function (result) {
var detail = result.data && result.data.detail;
- if (typeof detail !== 'string') detail = JSON.stringify(result.data);
+ if (typeof detail !== 'string') {
+ detail = 'HTTP ' + result.status;
+ }
if (result.ok) {Add a localized generic-failure key so the .catch branch at Line 131 does not print String(err) either.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 120-120: React's useState should not be directly called
Context: setProgress(100, '')
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 123-123: React's useState should not be directly called
Context: setProgress(0, form.getAttribute('data-msg-waiting'))
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 129-129: React's useState should not be directly called
Context: setProgress(0, form.getAttribute('data-msg-waiting'))
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/medcheck/web/static/app.js` around lines 112 - 132, Update the response
handling in the fetch promise to read the body defensively, only parse valid
JSON, and fall back to the response status text for empty or non-JSON responses
while preserving the existing result flow. Add and use a localized
generic-failure message in the catch branch instead of rendering String(err),
including for parser and network failures.
| .pdp-stepper-step.completed { color: rgba(255, 255, 255, 0.85); } | ||
| .pdp-stepper-num { font-weight: 700; } | ||
| .pdp-stepper-label { font-weight: 400; } | ||
| .pdp-stepper-step:not(.active) .pdp-stepper-label { display: none; } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Inactive stepper buttons expose only a number as their accessible name.
display: none removes the label from the accessibility tree. Each inactive step button then announces only "2." or "3.". A screen-reader user cannot identify the target step before activating it.
Keep the label available to assistive technology and hide it visually only.
♿ Proposed fix
- .pdp-stepper-step:not(.active) .pdp-stepper-label { display: none; }
+ .pdp-stepper-step:not(.active) .pdp-stepper-label {
+ position: absolute;
+ width: 0.0625rem;
+ height: 0.0625rem;
+ padding: 0;
+ overflow: hidden;
+ clip-path: inset(50%);
+ white-space: nowrap;
+ }📝 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.
| .pdp-stepper-step:not(.active) .pdp-stepper-label { display: none; } | |
| .pdp-stepper-step:not(.active) .pdp-stepper-label { | |
| position: absolute; | |
| width: 0.0625rem; | |
| height: 0.0625rem; | |
| padding: 0; | |
| overflow: hidden; | |
| clip-path: inset(50%); | |
| white-space: nowrap; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/medcheck/web/templates/index.html` at line 379, Update the
.pdp-stepper-step:not(.active) .pdp-stepper-label rule so inactive labels are
visually hidden without using display:none, while keeping them available to
assistive technologies and preserving the existing visual layout.
| <form id="analyzeForm" action="/api/analyze" method="post" enctype="multipart/form-data" | ||
| data-msg-consent="{{ t.ui_consent_required }}" | ||
| data-msg-sending="{{ t.ui_prog_sending }}" | ||
| data-msg-waiting="{{ t.ui_progress_waiting }}"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The form still declares a native multipart submission that the API cannot accept.
app.js calls e.preventDefault() at Line 194 and posts a JSON body at Lines 107-111. The enctype="multipart/form-data" attribute therefore has no effect on the normal path.
If JavaScript is disabled or /static/app.js fails to load, the browser performs the native POST /api/analyze as multipart. AnalyzeRequest expects a JSON body, so the request returns 422 and replaces the wizard with a raw error page.
Point the form at a non-submitting target, or add a <noscript> notice that the browser flow requires JavaScript.
♻️ Proposed change
- <form id="analyzeForm" action="/api/analyze" method="post" enctype="multipart/form-data"
+ <form id="analyzeForm" action="/api/analyze" method="post"
data-msg-consent="{{ t.ui_consent_required }}"
data-msg-sending="{{ t.ui_prog_sending }}"
data-msg-waiting="{{ t.ui_progress_waiting }}">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/medcheck/web/templates/index.html` around lines 961 - 964, Update the
analyzeForm fallback behavior so it does not natively submit multipart data to
/api/analyze when JavaScript is unavailable. Point the form at a non-submitting
target or add a noscript notice indicating that JavaScript is required, while
preserving the existing JavaScript JSON submission flow.
2 participants
Complete UI/UX overhaul of the browser wizard:
palette, graphite text scale, spacing/radius/shadow tokens), 100rem
container, PDP stepper instead of ad-hoc tabs, floating-label form
inputs, kit cards/alerts/buttons/badges/file-download components,
teal-700 footer, and inline stroke icons throughout. All measurements
in rem, all colors from tokens.
keys in en/de/fr/es plus a header language switcher (?lang=, validated
server-side, falls back to MEDCHECK_LANGUAGE).
POST /api/analyze as JSON and surface the real server response
(501 preview -> warning alert, other errors -> danger alert), with
text inserted via textContent only.
required for cloud models and hidden for the local model, mirroring
the CLI --allow-cloud-llm consent gate.
en/de/fr/es and formats to pdf/json/html (pt/ja/zh and txt used to
fail validation), anatomy options matched to the shipped prompt
regions.
keeps blocking data: URIs; keep htmx vendored and zero inline JS.
report languages only, and i18n catalog key parity across locales.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GCe7Hb5biAZmtDcVPuqT2z
Summary by CodeRabbit
New Features
Bug Fixes