docs: design localization (#30) — English/Spanish/French + sync skill - #22
Conversation
Broaden feature #30 from "Spanish only" to full i18n (English default, Spanish, French) covering UI copy and user-facing server errors, with a per-user preferred language (User.PreferredLanguage, nullable -> English). - feature-30-localization.md: design + e2e scenarios/expected output. Client copy via compile-time @angular/localize (per-locale builds under /en//es//fr/; language change is a reload, Q-30-1); server errors localized client-side off stable `code`s with the English `detail` as fallback (Q-30-2). Default is always /en/ (Q-30-3); ship machine-translated marked needs-review (Q-30-4). - .claude/skills/i18n-translations: enforces every UI string lands in all three locales and every server error gets a code + translated message. - feature-status.md: #30 marked Designed; counts kept in step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughThe PR adds English, Spanish, and French localization across the API, Angular application, locale-specific hosting, user preferences, API errors, outbound emails, translation catalogs, and supporting documentation. ChangesLocalization implementation
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
First backend slice of localization (#30): let an account store a preferred UI locale, defaulting to English when unset. - User.PreferredLanguage (nullable) + AddPreferredLanguage migration (a single nullable varchar(16); null = unset -> default English). - SupportedLanguages: the pure en/es/fr validator (trim + lowercase, blank -> null), unit-tested like EmailPolicy/PasswordPolicy. - ProfileResponse gains preferredLanguage; PATCH /api/me/profile validates it as part of the full-profile replace and returns 400 invalid_language on an unsupported code (inline `code` pattern; the ErrorCodes registry + Problem() sweep are a separate follow-up). - Docs: feature-30 status + §3.2 semantics updated to match the built endpoint; feature-status #30 -> partial, counts kept in step. Backend only; the @angular/localize client, locale serving, and the error-code sweep remain. Build + 219 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.claude/skills/i18n-translations/SKILL.md:
- Around line 51-55: Update the i18n skill instructions to support the staged
backend-only rollout: when client localization infrastructure and the
ErrorCodes/ERROR_MESSAGES registry are not yet available, allow backend changes
such as inline Coded(...) errors without requiring extraction or XLIFF updates;
retain the existing client requirements once those prerequisites exist.
In `@docs/feature-30-localization.md`:
- Around line 317-319: Update the browser E2E capability statement in the
localization design document to acknowledge and use the existing Playwright
journeys documented in feature-status for locale redirects, language switching,
and localized error rendering. Keep the verification plan internally consistent
and only retain an exclusion if you document a specific reason.
- Around line 362-375: Align the catalog enforcement statements in sections 8.4
and 8.5 with the actual i18n-translations skill behavior. Either define and
reference an executable CI check with an explicit failure condition for
mismatched ID sets, or revise the claims to describe the extraction and catalog
comparison as manual verification rather than an automated CI failure.
- Around line 125-128: The profile update contract is inconsistent because
MeController treats omitted and explicit-null fields identically while the
documentation claims omitted fields remain unchanged. Choose one contract and
apply it consistently: either require every signed-in profile switcher and E2E
request to send the complete profile, or update the API model/handling to
distinguish omission from clearing; then revise the related xUnit scenario and
all affected sections of the localization design document.
- Around line 186-193: Update the documented per-locale SPA fallback routing
around MapFallbackToFile so each catch-all route is constrained to its own
locale prefix and /es/files resolves to es/index.html rather than another
locale. Replace the placeholder Add call with an actual constraint mechanism,
and add an explicit unprefixed-path fallback or redirect for paths such as
/files, consistent with the documented root/unprefixed redirect behavior.
- Around line 88-98: Resolve the locale contract inconsistency described around
SUPPORTED_LOCALES: either make the API’s SupportedLanguages contract share a
generated or validated source with the client locale definitions, or revise the
documentation to identify SUPPORTED_LOCALES as client-local rather than the
single source of truth. Remove any false verification implication while ensuring
API and client supported locales cannot silently diverge.
In `@docs/feature-status.md`:
- Line 53: Update the localization entry in the feature-status table to replace
the ambiguous “/en//es//fr/” notation with the three distinct locale paths
“/en/”, “/es/”, and “/fr/”, using comma-separated paths or another clear list
format while preserving the surrounding design details.
In `@src/Api/Features/Me/MeController.cs`:
- Around line 103-107: Update the invalid-language branch in the MeController
action to construct ValidationProblemDetails with Detail set to the existing
message and a preferredLanguage validation error, add Extensions["code"] =
"invalid_language", and return it via ValidationProblem(...). Replace the
current Coded(...) call while preserving the existing 400 status and language
validation behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d96e54c-301c-467c-aca1-a227f6d9bb4d
⛔ Files ignored due to path filters (2)
src/Api/Data/Migrations/20260807172527_AddPreferredLanguage.Designer.csis excluded by!**/Migrations/*.Designer.cssrc/Api/Data/Migrations/AppDbContextModelSnapshot.csis excluded by!**/Migrations/*ModelSnapshot.cs
📒 Files selected for processing (9)
.claude/skills/i18n-translations/SKILL.mddocs/feature-30-localization.mddocs/feature-status.mdsrc/Api/Data/AppDbContext.cssrc/Api/Data/Migrations/20260807172527_AddPreferredLanguage.cssrc/Api/Domain/User.cssrc/Api/Features/Localization/SupportedLanguages.cssrc/Api/Features/Me/MeController.cstests/Api.Tests/SupportedLanguagesTests.cs
| - **`PATCH /api/me/profile`** carries `preferredLanguage` as part of the **full-profile replace** | ||
| (like the name fields — the client sends the whole set each call): a supported code sets it; | ||
| blank/null clears it back to the default (English); any other value → `400 invalid_language`. | ||
| Validation is `SupportedLanguages.TryNormalize` (trims + lowercases, blank → null). *(Built.)* |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align all profile updates with the full-replace contract.
PATCH /api/me/profile replaces every profile field. In src/Api/Features/Me/MeController.cs:93-119, a missing field and an explicit null both become null. Therefore, a request containing only { "preferredLanguage": "es" } can clear the user's names, and the E2E claim that an omitted field is unchanged is false.
Require every signed-in switcher to send the complete profile, or change the API to distinguish omitted fields from explicit clearing. Update the xUnit scenario to match the chosen contract.
As per path instructions, this design document must remain internally consistent.
Also applies to: 203-207, 333-336
🤖 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 `@docs/feature-30-localization.md` around lines 125 - 128, The profile update
contract is inconsistent because MeController treats omitted and explicit-null
fields identically while the documentation claims omitted fields remain
unchanged. Choose one contract and apply it consistently: either require every
signed-in profile switcher and E2E request to send the complete profile, or
update the API model/handling to distinguish omission from clearing; then revise
the related xUnit scenario and all affected sections of the localization design
document.
Source: Path instructions
| - **No untranslated keys reach the user** — a missing `es`/`fr` catalog entry falls back to the | ||
| English source (Angular's default), never renders the raw id. The build fails CI if a catalog is | ||
| missing an id (`ng extract-i18n` diff check — the skill, §9). | ||
| - **Every user-facing `Problem()` has a `code`** — asserted by a test that scans controllers (or a | ||
| reviewer checklist) so new errors can't ship code-less and thus untranslatable. | ||
| - **Language change is a reload, not a swap** — after switching, `location.pathname` starts with the | ||
| new `/{lang}/` prefix (the accepted Q-30-1 consequence, verified, not a bug). | ||
|
|
||
| ### 8.5 Build/verification slice | ||
|
|
||
| - `ng build --localize` produces `dist/ClientApp/browser/{en,es,fr}/index.html`, each with the | ||
| correct base href — the deployable proof all three locales compile. | ||
| - `ng extract-i18n` produces a source catalog whose id set **equals** the id sets in `messages.es.xlf` | ||
| and `messages.fr.xlf` (no missing, no orphaned) — the sync check the skill automates. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the catalog enforcement claim with the skill.
The design says a missing translation ID fails CI and that the skill automates the ID-set check. .claude/skills/i18n-translations/SKILL.md:78-81 only instructs a developer to extract the catalog and perform a manual diff. It does not define a CI command or failure condition.
Add an executable CI check, or change the design to state that this is a manual verification step.
As per path instructions, this design document must remain internally consistent.
🤖 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 `@docs/feature-30-localization.md` around lines 362 - 375, Align the catalog
enforcement statements in sections 8.4 and 8.5 with the actual i18n-translations
skill behavior. Either define and reference an executable CI check with an
explicit failure condition for mismatched ID sets, or revise the claims to
describe the extraction and catalog comparison as manual verification rather
than an automated CI failure.
Source: Path instructions
| | # | Feature | Status | Notes | | ||
| |---|---------|--------|-------| | ||
| | 30 | Spanish language (i18n) | ❌ | No i18n framework installed (`@angular/localize`/`ngx-translate`) and no locale config in `angular.json`; all UI copy is hardcoded English. **Not a frontend-only job:** user-facing server strings — validation and gate messages (`EmailPolicy`, `PasswordPolicy`, `RegistrationGate`) — are English prose returned in problem+json `detail` and rendered verbatim by the client. Full Spanish means either the server localizes off `Accept-Language`, or the API returns stable error *codes* and the client owns the copy. The latter is the cleaner fork but reworks the current `detail`-rendering contract | | ||
| | 30 | Localization (i18n): English / Spanish / French | 🟡 | **Designed — [feature-30-localization.md](feature-30-localization.md) (2026-08-07); backend preferred-language slice built (2026-08-07).** Broadened from "Spanish only" to full localization into **English (default), Spanish, and French**, covering **UI copy and user-facing server errors**, with a **per-user preferred language** (`User.PreferredLanguage`, nullable → English). Two decisions taken up front: **client copy is compile-time `@angular/localize`** — one built bundle per locale served under `/en//es//fr/`, so a language change is a **reload into that build**, not an instant swap (Q-30-1); and **server errors are localized client-side off stable `code`s** (each user-facing `Problem()` carries a machine `code`, the client owns the translated copy, the English `detail` is the fallback) — extends the `code` seed already in the repo (`email_in_use`, `email_not_configured`, `email_unverified`) (Q-30-2). **Built (backend):** `User.PreferredLanguage` + `AddPreferredLanguage` migration, `SupportedLanguages` (pure `en`/`es`/`fr` validator, unit-tested), `ProfileResponse.preferredLanguage`, and `PATCH /api/me/profile` validating it (`400 invalid_language`). Phasing: **P1** foundation + primary screens + business-error codes, **P2** field-validation codes, **P3** server-side localized emails. A repo skill **`i18n-translations`** enforces that every UI string lands in all three locales and every server error gets a code + translated message. **Remaining:** the whole client (no i18n framework installed yet, all copy still hardcoded English), the `ErrorCodes` registry + `Problem()` sweep, locale serving in `Program.cs`, and emails | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the locale path notation.
The entry shows /en//es//fr/. The design uses three separate paths: /en/, /es/, and /fr/. The double slashes can be copied into routing or deployment configuration.
Use comma-separated paths or list each locale separately.
As per path instructions, this status document must remain internally consistent.
🤖 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 `@docs/feature-status.md` at line 53, Update the localization entry in the
feature-status table to replace the ambiguous “/en//es//fr/” notation with the
three distinct locale paths “/en/”, “/es/”, and “/fr/”, using comma-separated
paths or another clear list format while preserving the surrounding design
details.
Source: Path instructions
| // Validate the language before touching anything: a blank value clears the preference (null → | ||
| // default English), any non-supported code is a 400 so an unknown locale never reaches the DB. | ||
| if (!SupportedLanguages.TryNormalize(req.PreferredLanguage, out var language)) | ||
| return Coded(StatusCodes.Status400BadRequest, "invalid_language", | ||
| "That isn't a supported language."); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return a validation problem for invalid_language.
For preferredLanguage: "de", this branch calls Coded, which returns a generic ProblemDetails through StatusCode(...). It does not return a ValidationProblemDetails response with a preferredLanguage error.
Create ValidationProblemDetails, set Detail, add Extensions["code"] = "invalid_language", and return it through ValidationProblem(...). This keeps the client error code and gives the Angular client the required validation-error shape.
As per path instructions, error responses must use Problem(...) or ValidationProblem(...). Based on learnings, validation failures must include explicit Detail and validation errors.
🤖 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/Api/Features/Me/MeController.cs` around lines 103 - 107, Update the
invalid-language branch in the MeController action to construct
ValidationProblemDetails with Detail set to the existing message and a
preferredLanguage validation error, add Extensions["code"] = "invalid_language",
and return it via ValidationProblem(...). Replace the current Coded(...) call
while preserving the existing 400 status and language validation behavior.
Sources: Path instructions, Learnings
Localization groundwork (#30 §5): so the client can render server errors
in the user's language, every user-facing problem+json now carries a
stable machine `code` the client maps to translated copy, with the
English `detail` kept as the fallback.
- ErrorCodes registry (~40 codes) + a shared CodedProblem(ControllerBase)
extension in src/Api/Http/. CodedProblem goes through ProblemDetailsFactory,
so coded responses keep Type/Title/traceId (better than the ad-hoc
`new ProblemDetails { … }; Extensions["code"] = …` it replaces).
- Swept every problem+json Problem()/coded return across the 15 controllers.
Existing client-observed codes (email_in_use, email_not_configured,
email_unverified, email_change_pending) keep their exact strings.
- FolderException/TrashException gained a Code (alongside their StatusCode);
the 19 throw sites set it and the forwarding controllers pass it straight
to CodedProblem, so folder/trash/quota errors localize too.
- ErrorCodesTests: code values are unique + snake_case (a dup would collide
in the client's code→message map).
Deferred to Phase 2: the ValidationProblemDetails field-error maps
(password/email/register validation stay English), and the two bespoke
non-problem+json shapes in UploadsController.
Build + 222 tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Api/Features/Auth/EmailChangeController.cs (1)
37-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable caching for every token-preview response.
A successful preview returns an account email for a bearer token in the URL. Without
Cache-Control: no-store, a browser or intermediary cache can retain that response. Add the header before token resolution.no-storemeans a cache must not retain the response.
src/Api/Features/Auth/EmailChangeController.cs#L37-L44: setResponse.Headers.CacheControl = "no-store";at the start ofPreview.src/Api/Features/Auth/PasswordResetController.cs#L133-L140: setResponse.Headers.CacheControl = "no-store";at the start ofPreview.src/Api/Features/Invites/InvitesController.cs#L36-L43: setResponse.Headers.CacheControl = "no-store";at the start ofPreview.🤖 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/Api/Features/Auth/EmailChangeController.cs` around lines 37 - 44, Disable caching at the start of each token preview method by setting Response.Headers.CacheControl to "no-store" before token resolution: EmailChangeController.cs lines 37-44 in Preview, PasswordResetController.cs lines 133-140 in Preview, and InvitesController.cs lines 36-43 in Preview.Source: Learnings
🤖 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 `@docs/feature-30-localization.md`:
- Around line 229-238: Update the localization validation described around
ErrorCodes so every server-defined error code is guaranteed to exist in
ERROR_MESSAGES with English, Spanish, and French translations. Prefer generating
the client message map from the ErrorCodes registry; otherwise add a CI test
that compares all ErrorCodes values against each language map and fails on
missing entries while preserving the existing uniqueness checks.
- Around line 12-16: The localization status must consistently reflect the
completed server error-code sweep and the remaining client work. In
docs/feature-30-localization.md lines 12-16, retain the server-complete wording
only if the documented API changes are complete; in docs/feature-status.md lines
122-126, remove the server work from unfinished items and list only the
outstanding client localization tasks, or align both documents with the actual
implementation state.
- Around line 259-262: Update the Phase 2 deferral in the UploadsController
section so anonymous upload guardrail errors and the 413 quota response use
Problem(...) rather than bespoke { error } or { error, remaining } shapes;
preserve remaining by storing it in ProblemDetails.Extensions. Defer only
translated ValidationProblemDetails field-error messages.
In `@src/Api/Features/Auth/AuthController.cs`:
- Around line 58-60: Update the registration save flow in AuthController to
catch DbUpdateException only when it represents the unique-email constraint
violation, then return the same 409 CodedProblem with ErrorCodes.EmailRegistered
and the existing message. Re-throw all other DbUpdateException instances and
preserve the existing AnyAsync check.
In `@src/Api/Features/Me/MeController.cs`:
- Around line 220-223: Update the DbUpdateException handling in the email-change
flow for both mail-off and mail-on branches to return coded 409 responses only
for PostgreSQL SQLSTATE 23505 violations matching the branch’s expected unique
index: Users.Email or KEEPR.IX_EmailChangeTokens_UserId with UsedAt IS NULL.
Re-throw or otherwise preserve normal server-error handling for all other
database failures, and add tests covering both valid conflict paths and
unrelated DbUpdateException cases.
In `@src/Api/Features/Sharing/SharesController.cs`:
- Around line 94-99: Update the UpdateExpiryAsync status switch in
SharesController so UpdateExpiryStatus.NotFound returns CodedProblem with
ErrorCodes.ShareNotFound and a safe detail message, instead of an empty NotFound
response. Preserve the existing Revoked handling and ensure the response remains
RFC7807 problem+json with the message available through error.detail.
---
Outside diff comments:
In `@src/Api/Features/Auth/EmailChangeController.cs`:
- Around line 37-44: Disable caching at the start of each token preview method
by setting Response.Headers.CacheControl to "no-store" before token resolution:
EmailChangeController.cs lines 37-44 in Preview, PasswordResetController.cs
lines 133-140 in Preview, and InvitesController.cs lines 36-43 in Preview.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2cb13c9b-04dd-48d2-9d45-de0b3de26d73
📒 Files selected for processing (21)
docs/feature-30-localization.mddocs/feature-status.mdsrc/Api/Features/Admin/AdminController.cssrc/Api/Features/Admin/EmailSettingsController.cssrc/Api/Features/Auth/AuthController.cssrc/Api/Features/Auth/EmailChangeController.cssrc/Api/Features/Auth/PasswordResetController.cssrc/Api/Features/Folders/FoldersController.cssrc/Api/Features/Invites/InvitesController.cssrc/Api/Features/Me/MeController.cssrc/Api/Features/Media/MediaController.cssrc/Api/Features/Search/SearchController.cssrc/Api/Features/Sharing/PublicShareController.cssrc/Api/Features/Sharing/SharesController.cssrc/Api/Features/Trash/TrashController.cssrc/Api/Features/Uploads/UploadsController.cssrc/Api/Http/ControllerErrorExtensions.cssrc/Api/Http/ErrorCodes.cssrc/Api/Services/FolderService.cssrc/Api/Services/TrashService.cstests/Api.Tests/ErrorCodesTests.cs
| // src/Api/Http/ErrorCodes.cs — the canonical set of user-facing error codes. | ||
| public static class ErrorCodes | ||
| { | ||
| public const string EmailInUse = "email_in_use"; | ||
| public const string PasswordIncorrect = "password_incorrect"; | ||
| public const string QuotaExceeded = "quota_exceeded"; | ||
| public const string InvalidLanguage = "invalid_language"; | ||
| // …one per user-facing Problem() across the 15 controllers (~40 codes). | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce parity between ErrorCodes and the client message map.
The document calls ErrorCodes authoritative and says it prevents silent client divergence. The documented test checks only unique snake_case values. A new server code can be missing from ERROR_MESSAGES, so non-English clients fall back to the English detail without a test failure. Generate the client map from the registry, or add a CI check that every server code has English, Spanish, and French messages.
🤖 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 `@docs/feature-30-localization.md` around lines 229 - 238, Update the
localization validation described around ErrorCodes so every server-defined
error code is guaranteed to exist in ERROR_MESSAGES with English, Spanish, and
French translations. Prefer generating the client message map from the
ErrorCodes registry; otherwise add a CI test that compares all ErrorCodes values
against each language map and fails on missing entries while preserving the
existing uniqueness checks.
| > **Deferred (Phase 2):** the `ValidationProblemDetails` field-error maps (password/email/register | ||
| > validation stay English for now, §5.3) and the two bespoke non-problem+json shapes in | ||
| > `UploadsController` (the anonymous `{ error }` guardrails and the `413` `{ error, remaining }` | ||
| > quota body, which carries extra data). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not defer anonymous upload error responses.
The design explicitly keeps { error } and { error, remaining } outside RFC7807. This violates the API rule that every error uses Problem(...) or ValidationProblem(...). It also gives the Angular client no detail or code to localize. Return Problem(...) now and carry remaining in ProblemDetails.Extensions; defer only translated field messages.
🤖 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 `@docs/feature-30-localization.md` around lines 259 - 262, Update the Phase 2
deferral in the UploadsController section so anonymous upload guardrail errors
and the 413 quota response use Problem(...) rather than bespoke { error } or {
error, remaining } shapes; preserve remaining by storing it in
ProblemDetails.Extensions. Defer only translated ValidationProblemDetails
field-error messages.
Source: Path instructions
| if (await db.Users.AnyAsync(u => u.Email == email, ct)) | ||
| return Problem("Email already registered.", statusCode: StatusCodes.Status409Conflict); | ||
| return this.CodedProblem(StatusCodes.Status409Conflict, ErrorCodes.EmailRegistered, | ||
| "Email already registered."); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle the duplicate-email write race.
Two concurrent POST /api/auth/register requests with the same new email can both pass AnyAsync at Line 58. The first request commits. The second request then hits the unique index at SaveChangesAsync and returns a 500 response instead of the expected 409 with code: "email_registered".
Catch only the unique-email DbUpdateException around the save and return the same CodedProblem. Re-throw other database failures.
Proposed fix
- await db.SaveChangesAsync(ct);
+ try
+ {
+ await db.SaveChangesAsync(ct);
+ }
+ catch (DbUpdateException ex) when
+ (ex.InnerException is Npgsql.PostgresException { SqlState: "23505" })
+ {
+ return this.CodedProblem(StatusCodes.Status409Conflict,
+ ErrorCodes.EmailRegistered, "Email already registered.");
+ }🤖 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/Api/Features/Auth/AuthController.cs` around lines 58 - 60, Update the
registration save flow in AuthController to catch DbUpdateException only when it
represents the unique-email constraint violation, then return the same 409
CodedProblem with ErrorCodes.EmailRegistered and the existing message. Re-throw
all other DbUpdateException instances and preserve the existing AnyAsync check.
| catch (DbUpdateException) | ||
| { | ||
| return Coded(StatusCodes.Status409Conflict, "email_in_use", "That email is already in use."); | ||
| return this.CodedProblem(StatusCodes.Status409Conflict, ErrorCodes.EmailInUse, "That email is already in use."); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'DbUpdateException|HasIndex|EmailChangeTokens|EmailInUse|EmailChangePending|IsUnique|IX_' \
src/Api tests || trueRepository: imariel2d/keepr
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== MeController relevant section =="
sed -n '160,270p' src/Api/Features/Me/MeController.cs
echo
echo "== MailboxOptions / MailboxSettings and index definitions around mailbox =="
rg -n -C 5 'IX_Mailbox|Mailbox|HasIndex.*Mailbox|EmailChangeTokens|EmailChangePending|EmailInUse|NewEmail|OldEmail' src/Api/Data src/Api/Features tests --glob '*.cs' | head -n 220
echo
echo "== ErrorCodes =="
rg -n 'EmailInUse|EmailChangePending' src/Api --glob '*.cs'Repository: imariel2d/keepr
Length of output: 28734
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== DbContext user email properties/index config =="
rg -n -C 6 "HasIndex\\(u => u\\.Email\\)|Property.*Email|Email.*HasMaxLength|Users.*Email" src/Api/Data src/Api --glob '*.cs' | head -n 220
echo
echo "== Npgsql package versions =="
rg -n 'Npgsql.EntityFrameworkCore.PostgreSQL|Npgsql\..*Version|Microsoft.EntityFrameworkCore' . -g '*.csproj' -g '*.props' -g '*.targets' -g 'package.xml' || true
echo
echo "== Error handler / ProblemHelpers references =="
rg -n -C 3 "ExceptionType|DbUpdateException|ExceptionContext|Problem|500|503|ServiceUnavailable" src/Api tests --glob '*.cs' | head -n 260
echo
echo "== Program exception middleware =="
sed -n '1,220p' src/Api/Program.csRepository: imariel2d/keepr
Length of output: 50371
🌐 Web query:
Npgsql EF Core 23505 unique constraint exception SqlState constraint
💡 Result:
When you encounter a 23505 unique constraint exception while using Npgsql with Entity Framework Core, it indicates that a database operation attempted to insert or update a record with a value that conflicts with an existing unique index or primary key [1][2][3]. Handling this error requires catching the EF Core DbUpdateException and inspecting its inner exception to confirm it is a PostgresException [4][1]. How to Handle and Debug: 1. Identify the Exception: Wrap your SaveChanges or SaveChangesAsync call in a try-catch block targeting DbUpdateException [4]. 2. Access the PostgresException: The inner exception of the DbUpdateException will be an instance of Npgsql.PostgresException [4][1]. 3. Check the SQL State: Verify the SqlState property of the PostgresException. The code 23505 specifically denotes a unique violation [5][3]. 4. Extract Details: The PostgresException object provides useful metadata, including the ConstraintName, TableName, and ColumnName associated with the violation [4][3]. You can use these properties to programmatically determine which field caused the conflict [5]. Example Logic: try { await context.SaveChangesAsync; } catch (DbUpdateException ex) when (ex.InnerException is PostgresException pgEx && pgEx.SqlState == "23505") { // Log details or handle the conflict (e.g., inform the user) var constraintName = pgEx.ConstraintName; var tableName = pgEx.TableName; // Handle error... } Common Causes: - Database Sequences: Often occurs after seeding data if the database identity sequence is not correctly synchronized with existing data, leading the sequence to generate a value that already exists [6][7]. - Entity State: EF Core may treat an existing entity as a new one (e.g., if you manually set a primary key value for an identity column or fail to attach an existing related entity properly), causing an attempt to re-insert it [1][8]. - Debugging Tip: To see the full details of the violation (like the specific conflicting value), enable more verbose error messages by adding Include Error Detail=true; to your Npgsql connection string [2][8].
Citations:
- 1: https://stackoverflow.com/questions/38990857/entity-framework-core-npgsql-postgresexception-23505-duplicate-key-value-viol
- 2: https://stackoverflow.com/questions/78543360/entity-framework-core-with-npgsql-duplicate-key-value-violates-unique-constrain
- 3: https://www.npgsql.org/doc/api/Npgsql.PostgresException.html
- 4: https://haacked.com/archive/2022/12/12/specific-db-exception/
- 5: How to find out which column(s) violated which constraints and checks when calling SaveChangesAsync() npgsql/efcore.pg#1959
- 6: HasData seeding causes Add to fail with PostgresException: 23505 on calling SaveChanges npgsql/efcore.pg#759
- 7: https://stackoverflow.com/questions/62086561/duplicate-key-value-violates-unique-constraint-ef-core-postgressql
- 8: https://stackoverflow.com/questions/78415253/npgsql-postgresexception-0x80004005-23505-duplicate-key-value-violates-uniqu
🌐 Web query:
Rust web framework 409 conflict conflict error response retry idempotency error type
💡 Result:
In the context of Rust web frameworks (such as Axum or Actix-web), handling HTTP 409 Conflict responses requires a structured approach to differentiate between concurrent request collisions, idempotency key violations, and stale resource state [1][2][3]. The HTTP 409 Conflict status indicates that a request could not be completed because of a conflict with the current state of the resource [4]. When implementing idempotency, this status is specifically used to signal that a retry for a given Idempotency-Key is occurring while the original request is still being processed [5][6][7]. Key patterns for implementation include: 1. Standardizing Error Types: Use a domain-specific error enum (e.g., AppError) implemented with the thiserror crate [8][9][10]. For web frameworks like Axum or Actix-web, implement the IntoResponse or ResponseError trait for your error enum to ensure consistent status codes and response bodies [8][9][10]. 2. Idempotency Error Types: To assist clients in automated recovery, return a machine-readable error body instead of a plain message [5][11][2]. The draft IETF standard for Idempotency-Key recommends including a type URI to distinguish specific error scenarios [5][6]: - Concurrent Request Conflict: Returned when a retry arrives before the original request completes [5][6]. - Idempotency Key Mismatch: Returned if the same key is reused with a different request payload [11][6][7]. 3. Recommended Response Structure: Follow a unified JSON response format, for example: { "type": "https://example.com/errors/conflict", "title": "Request in progress", "detail": "A request with this Idempotency-Key is already being processed." } [5][6][12][13] 4. Retry Logic: - Clients should not perform "blind" retries on 409 errors [1]. - If the conflict is due to a request still in progress, the client should wait and retry [11][7]. - If the conflict is due to a key-payload mismatch, the client must correct the request (as this is a semantic error) [5][1][6]. - For state-based conflicts (e.g., version mismatches), the client should re-fetch the latest resource state before attempting the operation again [1][2][3]. By centralizing these error conversions in your Rust framework's error handling layer, you ensure that internal system errors (like database constraint violations or lock contention) are correctly mapped to 409 status codes without leaking sensitive internal details to the client [8][9][10].
Citations:
- 1: https://errorlookup.com/guides/409-conflict/
- 2: https://errorreference.com/errors/http/409
- 3: https://errorreference.com/playbooks/conflict-concurrency-resolution
- 4: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/409
- 5: https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-idempotency-key-header/
- 6: https://blog.frankel.ch/fix-duplicate-api-requests/
- 7: https://codelit.io/blog/api-idempotency-patterns
- 8: https://abrarqasim.com/blog/axum-error-handling-split-that-stopped-opaque-500s/
- 9: https://statuscodefyi.com/guides/rust-actix-error-handling-guide/
- 10: https://leapcell.io/blog/elegant-error-handling-and-unified-responses-in-rust-web-apis
- 11: https://codesprintpro.com/blog/api-idempotency-keys/
- 12: https://ferra.rs/docs/ferra-error-handling.html
- 13: https://www.rustcanon.com/patterns/api-design/error-responses
Map only the expected unique-constraint failures.
A valid email-change request triggers 409 when SaveChangesAsync raises a DbUpdateException for any other database write failure. The mail-off branch returns email_in_use; the mail-on branch returns email_change_pending. This can make the client treat a database outage as a user conflict and stop retrying.
Return the coded 409 only when the PostgreSQL error is 23505 and the violated unique index matches the branch (Users.Email or KEEPR.IX_EmailChangeTokens_UserId partial UsedAt IS NULL). Let other DbUpdateException values reach the normal server-error problem handler. Add tests for both paths.
Also applies to lines 240-245.
🤖 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/Api/Features/Me/MeController.cs` around lines 220 - 223, Update the
DbUpdateException handling in the email-change flow for both mail-off and
mail-on branches to return coded 409 responses only for PostgreSQL SQLSTATE
23505 violations matching the branch’s expected unique index: Users.Email or
KEEPR.IX_EmailChangeTokens_UserId with UsedAt IS NULL. Re-throw or otherwise
preserve normal server-error handling for all other database failures, and add
tests covering both valid conflict paths and unrelated DbUpdateException cases.
| var (status, link) = await shares.UpdateExpiryAsync(linkId, User.UserId(), req.ExpiresInDays, ct); | ||
| return status switch | ||
| { | ||
| UpdateExpiryStatus.NotFound => NotFound(), | ||
| UpdateExpiryStatus.Revoked => Problem( | ||
| "This link has been revoked; create a new one to share again.", | ||
| statusCode: StatusCodes.Status409Conflict), | ||
| UpdateExpiryStatus.Revoked => this.CodedProblem(StatusCodes.Status409Conflict, | ||
| ErrorCodes.ShareRevoked, "This link has been revoked; create a new one to share again."), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return a coded problem for a missing share link.
If UpdateExpiryAsync returns NotFound, Line 97 returns an empty 404 response. For example, PATCH /api/shares/{unknown-guid} has no detail or code, so the Angular client cannot render its error message.
Return CodedProblem with ErrorCodes.ShareNotFound and a safe detail.
Proposed fix
- UpdateExpiryStatus.NotFound => NotFound(),
+ UpdateExpiryStatus.NotFound => this.CodedProblem(
+ StatusCodes.Status404NotFound,
+ ErrorCodes.ShareNotFound,
+ "Share link not found."),As per path instructions, error responses must use RFC7807 problem+json, and the Angular client reads error.detail.
📝 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.
| var (status, link) = await shares.UpdateExpiryAsync(linkId, User.UserId(), req.ExpiresInDays, ct); | |
| return status switch | |
| { | |
| UpdateExpiryStatus.NotFound => NotFound(), | |
| UpdateExpiryStatus.Revoked => Problem( | |
| "This link has been revoked; create a new one to share again.", | |
| statusCode: StatusCodes.Status409Conflict), | |
| UpdateExpiryStatus.Revoked => this.CodedProblem(StatusCodes.Status409Conflict, | |
| ErrorCodes.ShareRevoked, "This link has been revoked; create a new one to share again."), | |
| var (status, link) = await shares.UpdateExpiryAsync(linkId, User.UserId(), req.ExpiresInDays, ct); | |
| return status switch | |
| { | |
| UpdateExpiryStatus.NotFound => this.CodedProblem( | |
| StatusCodes.Status404NotFound, | |
| ErrorCodes.ShareNotFound, | |
| "Share link not found."), | |
| UpdateExpiryStatus.Revoked => this.CodedProblem(StatusCodes.Status409Conflict, | |
| ErrorCodes.ShareRevoked, "This link has been revoked; create a new one to share again."), |
🤖 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/Api/Features/Sharing/SharesController.cs` around lines 94 - 99, Update
the UpdateExpiryAsync status switch in SharesController so
UpdateExpiryStatus.NotFound returns CodedProblem with ErrorCodes.ShareNotFound
and a safe detail message, instead of an empty NotFound response. Preserve the
existing Revoked handling and ensure the response remains RFC7807 problem+json
with the message available through error.detail.
Source: Path instructions
Compile-time localization via @angular/localize (Q-30-1), the client half of
#30. English (source), Spanish, French — one build per locale served under
/{locale}/.
Client:
- angular.json i18n block + "localize": true production build + the
@angular/localize/init polyfill and $localize types; per-locale output is
browser/{en,es,fr}/ with the right base href.
- core/locale.ts (supported set, names, cookie key) + LocaleService: knows the
build's locale and switches by persisting the keepr_lang cookie and reloading
into /{locale}/ (compile-time i18n has no in-place swap), preserving the path.
- Shared LanguageSwitcher (accessible native <select>).
- errorMessage() in problem-details.ts: server `code` -> $localize copy, with
the English `detail` as the fallback (seeded with the auth/profile codes).
- Login screen fully localized; a /profile "Language" card wires the switcher
to PATCH /api/me/profile (full-replace now carries preferredLanguage).
- es/fr XLIFF catalogs, machine-translated, marked needs-review (Q-30-4).
- Register es/fr locale data for CommonModule pipes.
Server:
- Program.cs serves the per-locale builds: UseDefaultFiles + a "/" ->
"/{locale}/" redirect (LocalePicker: keepr_lang cookie or English, Accept-
Language ignored per Q-30-3) + per-locale SPA fallbacks. Dockerfile carries
all three builds unchanged.
Verified: dev/prod builds clean; production emits en/es/fr with translated copy
and no English leak in es; login renders + switcher is a11y-correct + responsive
(mobile, no horizontal scroll) live; API build + 222 tests green. Not exercised
live: the production locale *serving* (needs the container) and the full reload-
switch (single-locale dev can't serve /es/).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extends #30's client localization from login to the whole unauthenticated flow: forgot-password, reset-password, claim, and confirm-email. String- marking only — no layout change (all reuse login's .auth card, already verified responsive + a11y). - Every user-visible string marked with i18n / $localize under stable @@ ids; shared concepts reuse ids (password show/hide, please_wait, field labels, "Back to sign in", "Checking your link…", "Link not valid", "Passwords don't match."). - Ternary button labels and dynamic reveal aria-labels moved to $localize computed props (can't use the template i18n attribute). - reset/claim error fallbacks now go through errorMessage() (localized); confirm-email's two inline error strings localized via $localize. - es/fr catalogs regrown to 62 units (machine-translated, needs-review), id-complete against the source; interpolation + <strong> placeholders spliced correctly. All three locales build; no English leak in es. Verified: dev + production (--localize) builds clean; catalog id sets match the source exactly (no missing/orphaned); es/fr copy present in the bundles. Backend untouched. Live re-render skipped this batch (port 4200 held by another process) — layout is identical to the already-live-verified login. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The chrome shown on every authenticated screen: topbar (skip link, menu button, search box + label + clear, theme toggle, profile, log out), the sidebar nav labels (My Files, Trash, Admin, Accounts, Email), the quota label/note, and the mobile drawer's aria-label. - Static template strings marked with i18n / i18n-<attr> under @@shell.* / @@nav.* ids. - Dynamic labels (theme toggle) moved to a $localize computed; nav labels in navItems() and the quota label/note ($localize with named interpolations) localized in app.ts. - es/fr catalogs grown to 78 units (machine-translated, needs-review), id-complete against the source; the quota interpolation placeholders splice correctly. All three locales build. Note: "My Files" still appears in the es bundle via the not-yet-localized Files screen (its own root label) — expected; the sidebar nav label itself is correctly "Mis archivos". Files/trash/admin/share-viewer copy is the next batch. Verified: dev + production (--localize) builds clean; catalog id sets match the source (source=es=fr=78, no missing/orphaned); shell copy present in es, no shell-string leak. Backend untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every string on the Trash screen, including the tricky bits:
- ICU plurals for the item counts ("1 item" / "N items") in the summary and
the empty-Trash confirmation — replacing the old ternary "item{s}" concat.
- Conditional confirmation copy split into full-sentence variants
(file vs folder) so each translates naturally.
- Dynamic labels ($localize): purge-badge countdown (today / tomorrow /
in N days), the delete-confirmation title, and the restore toast (with the
name-was-taken variant).
- Errors now route through the localized errorMessage() (dropped the local
messageOf reimplementation of problemDetail).
- es/fr catalogs grown to 101 units (machine-translated, needs-review),
id-complete; the shared plural ICU sub-message is translated too.
Verified: dev + production (--localize) builds clean; catalog id sets match
the source (source=es=fr=101, no missing/orphaned); es/fr trash copy +
both plural cases present in the bundles, no trash-string leak in es.
Backend untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The largest content screen and its dialogs — the grid, breadcrumbs, selection bar, context menus, the new-folder/rename/delete modals, and the move / preview / share dialogs. - Count phrases as ICU plurals in templates (result count, N selected, Download N files, "N items" in the delete confirm); a localized noun() helper for the dynamic context-menu labels (Download/Move/Delete N). - Sentence-composition removed: reportFailures builds a full localized message per operation (move/delete) instead of splicing a verb; the delete-confirm body split into file/folder/many full-sentence variants. - Dynamic titles + flashes + menu items via $localize; "My Files" root label centralized (breadcrumb, drop veil, search paths, move dialog). - share-dialog: expiry options + statuses localized (status enum kept for CSS, statusLabel() for display); all error toasts via $localize. - All feature errors route through the localized errorMessage() (dropped two local messageOf reimplementations). - es/fr catalogs grown to 193 units (machine-translated, needs-review), id-complete; inline + embedded ICU sub-messages translated. Verified: dev + production (--localize) builds clean; catalog id sets match the source (source=es=fr=193, no missing/orphaned); es/fr Files copy present in the bundles, no marked-string leak in es. Backend untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The /s/:token page an anonymous recipient opens: loading/ok/not-found/gone/ error states, the download button, and the download-failure toast. - The 404/410 messages now render via errorMessage() off the server's share_not_found / share_unavailable codes (added to ERROR_MESSAGES), with the English detail as fallback — so the not-found/unavailable copy is localized instead of shown verbatim. - State titles/bodies + the download-error toast marked with i18n / $localize. - es/fr catalogs grown to 200 units (machine-translated, needs-review), id-complete. Verified: dev + production (--localize) builds clean; catalog id sets match the source (200, no missing/orphaned). Backend untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The accounts console (table + create/reset/role/quota/remove modals) and the
email-settings screen — completing the UI localization for #30.
- Role enum kept for CSS/logic; roleLabel()/statusText() give the display
forms. Dynamic labels (create/reset/test/save buttons, row-actions aria,
provider status chip) via $localize helpers.
- All notices/errors localized: interpolated success toasts via $localize;
every catch routes through errorMessage() off the server codes (dropped the
problemDetail fallbacks; two catch blocks gained an (e) binding).
- Shared password requirement label ("At least N characters") localized in
core/password-policy so login/claim/reset/admin all benefit.
- Provider brand names (Resend/Brevo/Mailgun) kept verbatim; only "None (off)"
localized.
- es/fr catalogs grown to 292 units (machine-translated, needs-review),
id-complete. Every UI screen is now localized.
Verified: dev + production (--localize) builds clean; catalog id sets match
the source (292, no missing/orphaned); es/fr admin copy present, no
marked-string leak in es. Backend untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The credential validators returned English prose in the field `errors` map, rendered verbatim by the client. They now return stable codes the client localizes per field. - ErrorCodes gains six field codes (email_malformed, email_disposable, password_too_short/too_long/contains_email/breached). - EmailPolicy.Validate / PasswordPolicy.Validate return codes (named *Code members); CredentialValidator's breach failure uses the code. The "at least N characters" interpolation moves to the client. - Client: fieldErrors() maps each known field code to localized copy and passes unknown values through unchanged — so framework validators and the email-settings screen (still prose) are unaffected. The five screens that render field errors (profile, reset, claim, admin, email-settings) use it. - EmailPolicy/PasswordPolicy tests updated to assert the codes. - fields.* catalog entries; es/fr grown to 298 units, id-complete. Verified: API build + 222 tests green; client dev + production (--localize) builds clean; catalog id sets match the source (298, no missing/orphaned). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Invite, password reset, change-email confirmation, and the old-address heads-up now render in the recipient's preferred language. - EmailStrings: per-locale copy for all four emails (en default + fallback), with in-language expiry pluralization and an <html lang> that follows. Selected by EmailStrings.For(locale). English values are byte-identical to the old hardcoded strings, so the default output is unchanged. - EmailTemplates.* take an optional locale and build from EmailStrings; the shared Layout carries the lang tag + localized footer boilerplate. - The four service send methods thread an optional `locale`; each caller passes the recipient's User.PreferredLanguage (admin invite/reset directly, the three background-dispatch paths capture it before the task runs). - Reset keeps its minutes-only expiry (not rolled up to hours) — behavior preserved. Two new tests cover es/fr rendering + the English fallback. Verified: API build clean (0 warnings); 224 tests pass. Client untouched (emails are server-rendered). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Api/Features/Me/MeController.cs (2)
140-141: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the runtime 400 error shape consistently.
ChangePasswordandChangeEmailcan return a coded 400 (forPasswordIncorrect, orEmailUnchangedinChangeEmail) in addition to field-validation 400 responses, so theProducesResponseType<ValidationProblemDetails>entries do not cover every 400 response clients can observe. Add a response that matches the actual OpenAPI-generated shape, such as baseProblemDetailsvalidation, which allows bothtype/code/detailfor coded errors and optionalerrorsfor validation errors.🤖 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/Api/Features/Me/MeController.cs` around lines 140 - 141, Update the response metadata for ChangePassword and ChangeEmail to document coded 400 responses alongside field-validation responses, using the base ProblemDetails shape that supports type/code/detail and optional errors. Ensure the OpenAPI declarations reflect all runtime 400 outcomes, including PasswordIncorrect and EmailUnchanged.
220-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFilter email-change 409 responses by the expected unique constraint.
DbUpdateExceptioncatches at the mail-off update, mail-on token insert, and confirmation commit also match any database failure. Map to409only when the PostgreSQL unique-violation state (23505) matches the branch-specific index:IX_Users_Emailfor email conflicts, andIX_EmailChangeTokens_UserIdfor pending-token conflicts. Other database failures should remain server errors.🤖 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/Api/Features/Me/MeController.cs` around lines 220 - 223, Filter the DbUpdateException handlers at src/Api/Features/Me/MeController.cs lines 220-223 and 240-245, and src/Api/Features/Auth/EmailChangeController.cs lines 129-130 by PostgreSQL state 23505 and the branch-specific constraint: use IX_Users_Email for email conflicts and IX_EmailChangeTokens_UserId for pending-token conflicts. Return the existing 409 responses only for matching violations; allow all other database failures to remain server errors.
🤖 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/Api/Features/Auth/PasswordPolicy.cs`:
- Around line 28-33: Keep PasswordPolicy.MinLength and the client
MIN_PASSWORD_LENGTH synchronized by adding a shared constant contract or test
that asserts both values remain equal to 12. Anchor the change to
PasswordPolicy.MinLength and the existing client-length symbol, preserving
current validation behavior while failing clearly if either value changes
independently.
In `@src/Api/Features/Email/EmailStrings.cs`:
- Around line 11-23: Update every public writable field in EmailStrings,
including the shared fields shown and the remaining localized string members, to
init-only properties while retaining required and their existing types. This
preserves the existing object initializers while preventing mutations to the
static En, Es, and Fr instances after construction.
In `@src/Api/Features/Email/EmailTemplates.cs`:
- Around line 37-53: Extend EmailTemplateTests to render Invite, PasswordReset,
ConfirmEmailChange, and EmailChanged for each locale en, es, and fr, using
representative valid arguments and asserting successful output. Ensure the tests
exercise all localized format strings so malformed placeholders or braces raise
failures.
In `@src/Api/Features/Invites/InviteService.cs`:
- Around line 52-58: Update InviteService.cs lines 52-58: change SendAsync to
place nullable locale before CancellationToken, make locale required, update all
callers, and add a locale param entry to its XML documentation. Apply the same
changes to EmailChangeService.cs lines 58-64 for SendConfirmationAsync and lines
75-78 for SendChangedNoticeAsync, plus PasswordResetService.cs lines 53-58 for
SendAsync; update every caller to pass the recipient’s locale and document
locale on each public method.
In `@src/Api/Program.cs`:
- Around line 276-287: Update the route fallback configuration around
LocalePicker.Supported so unmatched paths without a locale prefix redirect to
the picked locale while preserving the request path and query string. Replace
the current root-only handling in the map setup with an unprefixed catch-all
redirect, while retaining the per-locale App.MapFallbackToFile handlers for
localized SPA deep links.
In `@src/ClientApp/angular.json`:
- Around line 20-26: Update the i18n configuration’s sourceLocale in the Angular
workspace configuration from the string form to an object that preserves "en"
and explicitly sets subPath to "en", ensuring the source locale is emitted under
the /en/ directory.
In `@src/ClientApp/src/app/core/locale.service.ts`:
- Around line 43-47: Update pathWithinLocale to build the locale-prefix matching
pattern from the imported SUPPORTED_LOCALES collection instead of hardcoding
en|es|fr. Preserve the existing removal of the leading locale segment, slash
normalization, query string, and hash behavior so any newly supported locale is
stripped correctly.
In `@src/ClientApp/src/app/core/problem-details.ts`:
- Around line 54-61: Update
src/ClientApp/src/app/core/problem-details.ts#L54-L61 in fieldErrors by creating
FIELD_MESSAGES with a null prototype, skipping entries whose values are not
arrays before calling map, and preserving fallback behavior for unknown field
messages. Update src/ClientApp/src/app/core/problem-details.ts#L86-L91 to guard
ERROR_MESSAGES lookups with Object.hasOwn(ERROR_MESSAGES, code), allowing
unrecognized codes to fall through to the server detail.
In `@src/ClientApp/src/app/features/admin/admin.html`:
- Line 44: Wrap the entire storage sentence in the table cell with one
translatable message, including both formatted byte values and the connector,
following the existing admin.showing pattern. Replace the standalone
admin.storage_of marker, remove that now-unused translation unit from all
catalogs, and regenerate translations with npm run extract-i18n.
In `@src/ClientApp/src/app/features/admin/admin.ts`:
- Around line 439-449: Convert the zero-argument localized label methods to
computed signals while preserving the existing template call syntax: in
src/ClientApp/src/app/features/admin/admin.ts:439-449, update createButtonLabel
and resetButtonLabel; in
src/ClientApp/src/app/features/admin/email-settings.ts:168-185, update
revealKeyLabel, testButtonLabel, saveButtonLabel, and testResult; and in
src/ClientApp/src/app/features/trash/trash.ts:113-117, update purgeTitle. Leave
admin.ts roleLabel, actionsLabel, and revealLabel; email-settings.ts statusText
and statusAria; and trash.ts purgeLabel as methods because they take parameters.
No template changes are needed.
In `@src/ClientApp/src/app/features/profile/profile.ts`:
- Around line 114-145: Serialize saveName() and changeLanguage() through one
shared profile-update operation so concurrent full-replacement requests cannot
overwrite each other. Ensure the successful API response updates ProfileStore
before the next mutation proceeds, and preserve each action’s existing success,
error, loading, and locale-switch behavior.
In `@src/ClientApp/src/app/features/trash/trash.ts`:
- Around line 105-111: Update purgeLabel in TrashItem handling so the nonzero
countdown uses a single ICU pluralized translation message instead of branching
to a fixed “days” string in TypeScript. Preserve the existing today and tomorrow
messages, update the source and all three locale catalogs using the established
ICU patterns such as trash.summary, then regenerate translations with npm run
extract-i18n.
In `@src/ClientApp/src/locale/messages.es.xlf`:
- Around line 228-231: Restore the missing cove-icon placeholders in the Spanish
targets for trans-units admin.create.warn and admin.remove.warn, placing
START_TAG_COVE_ICON followed by CLOSE_TAG_COVE_ICON at the beginning to match
each source. Check the corresponding units in messages.fr.xlf and apply the same
correction wherever those target markers are missing.
In `@src/ClientApp/src/locale/messages.fr.xlf`:
- Around line 1184-1194: Update the French targets in the trash.deletes_today,
trash.deletes_tomorrow, and trash.deletes_in_days translation units to use
future-tense wording, preserving the existing date phrases and days placeholder
while changing “Supprimé” to “Sera supprimé”.
In `@tests/Api.Tests/EmailTemplateTests.cs`:
- Around line 147-162: Extend Emails_render_in_the_recipients_locale to exercise
non-English locales for EmailTemplates.ConfirmEmailChange and
EmailTemplates.EmailChanged, asserting each localized subject, body content, and
HTML lang attribute; keep the existing Invite and PasswordReset coverage
unchanged.
---
Outside diff comments:
In `@src/Api/Features/Me/MeController.cs`:
- Around line 140-141: Update the response metadata for ChangePassword and
ChangeEmail to document coded 400 responses alongside field-validation
responses, using the base ProblemDetails shape that supports type/code/detail
and optional errors. Ensure the OpenAPI declarations reflect all runtime 400
outcomes, including PasswordIncorrect and EmailUnchanged.
- Around line 220-223: Filter the DbUpdateException handlers at
src/Api/Features/Me/MeController.cs lines 220-223 and 240-245, and
src/Api/Features/Auth/EmailChangeController.cs lines 129-130 by PostgreSQL state
23505 and the branch-specific constraint: use IX_Users_Email for email conflicts
and IX_EmailChangeTokens_UserId for pending-token conflicts. Return the existing
409 responses only for matching violations; allow all other database failures to
remain server errors.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d52e6ef-0289-4aff-8f1b-2b2997520618
⛔ Files ignored due to path filters (1)
src/ClientApp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
Dockerfiledocs/feature-30-localization.mddocs/feature-status.mdsrc/Api/Features/Admin/AdminController.cssrc/Api/Features/Auth/CredentialValidator.cssrc/Api/Features/Auth/EmailChangeController.cssrc/Api/Features/Auth/EmailChangeService.cssrc/Api/Features/Auth/EmailPolicy.cssrc/Api/Features/Auth/PasswordPolicy.cssrc/Api/Features/Auth/PasswordResetController.cssrc/Api/Features/Auth/PasswordResetService.cssrc/Api/Features/Email/EmailStrings.cssrc/Api/Features/Email/EmailTemplates.cssrc/Api/Features/Invites/InviteService.cssrc/Api/Features/Me/MeController.cssrc/Api/Http/ErrorCodes.cssrc/Api/Http/LocalePicker.cssrc/Api/Program.cssrc/ClientApp/angular.jsonsrc/ClientApp/package.jsonsrc/ClientApp/src/app/app.htmlsrc/ClientApp/src/app/app.tssrc/ClientApp/src/app/core/locale.service.tssrc/ClientApp/src/app/core/locale.tssrc/ClientApp/src/app/core/models.tssrc/ClientApp/src/app/core/password-policy.tssrc/ClientApp/src/app/core/problem-details.tssrc/ClientApp/src/app/core/profile.service.tssrc/ClientApp/src/app/features/admin/admin.htmlsrc/ClientApp/src/app/features/admin/admin.tssrc/ClientApp/src/app/features/admin/email-settings.htmlsrc/ClientApp/src/app/features/admin/email-settings.tssrc/ClientApp/src/app/features/email-change/confirm-email.htmlsrc/ClientApp/src/app/features/email-change/confirm-email.tssrc/ClientApp/src/app/features/files/files.htmlsrc/ClientApp/src/app/features/files/files.tssrc/ClientApp/src/app/features/files/move-dialog.tssrc/ClientApp/src/app/features/files/preview-overlay.htmlsrc/ClientApp/src/app/features/files/preview-overlay.tssrc/ClientApp/src/app/features/files/share-dialog.tssrc/ClientApp/src/app/features/i18n/language-switcher.tssrc/ClientApp/src/app/features/invites/claim.htmlsrc/ClientApp/src/app/features/invites/claim.tssrc/ClientApp/src/app/features/login/login.htmlsrc/ClientApp/src/app/features/login/login.scsssrc/ClientApp/src/app/features/login/login.tssrc/ClientApp/src/app/features/password-reset/forgot-password.htmlsrc/ClientApp/src/app/features/password-reset/forgot-password.tssrc/ClientApp/src/app/features/password-reset/reset-password.htmlsrc/ClientApp/src/app/features/password-reset/reset-password.tssrc/ClientApp/src/app/features/profile/profile.htmlsrc/ClientApp/src/app/features/profile/profile.tssrc/ClientApp/src/app/features/share/share-viewer.htmlsrc/ClientApp/src/app/features/share/share-viewer.tssrc/ClientApp/src/app/features/trash/trash.htmlsrc/ClientApp/src/app/features/trash/trash.tssrc/ClientApp/src/locale/messages.es.xlfsrc/ClientApp/src/locale/messages.fr.xlfsrc/ClientApp/src/locale/messages.xlfsrc/ClientApp/src/main.tssrc/ClientApp/tsconfig.app.jsontests/Api.Tests/EmailPolicyTests.cstests/Api.Tests/EmailTemplateTests.cstests/Api.Tests/PasswordPolicyTests.cs
| // Stable error codes (not prose) — the client owns the localized copy (#30 P2), including the | ||
| // "at least {MinLength} characters" interpolation. Named members so tests read by intent. | ||
| public const string TooShortCode = ErrorCodes.PasswordTooShort; | ||
| public const string TooLongCode = ErrorCodes.PasswordTooLong; | ||
| public const string ContainsEmailCode = ErrorCodes.PasswordContainsEmail; | ||
| public const string BreachedCode = ErrorCodes.PasswordBreached; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts 'MIN_PASSWORD_LENGTH\s*='
rg -nP --type=cs 'MinLength\s*=\s*\d+'Repository: imariel2d/keepr
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
fd -a 'PasswordPolicy\.cs|claim\.ts|ErrorCodes\.cs' . | sed 's#^\./##'
echo
echo "== C# occurrences =="
rg -n --type=cs 'MinLength|TooShortCode|PasswordTooShort|PasswordPolicy|ErrorCodes' . || true
echo
echo "== TypeScript occurrences =="
rg -n --type=ts 'password|min.*password|minPassword|claim|PasswordPolicy|12|14' . || trueRepository: imariel2d/keepr
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "== inspect relevant files =="
sed -n '1,80p' src/Api/Features/Auth/PasswordPolicy.cs
sed -n '1,110p' src/ClientApp/src/app/features/invites/claim.ts
sed -n '1,55p' src/ClientApp/src/app/core/password-policy.ts
sed -n '30,48p' src/Api/Http/ErrorCodes.cs
echo
echo "== deterministic pin check =="
python3 - <<'PY'
from pathlib import Path
import re
cs_path = Path('src/Api/Features/Auth/PasswordPolicy.cs')
typescript = Path('src/ClientApp/src/app/core/password-policy.ts')
cs_text = cs_path.read_text()
ts_text = typescript.read_text()
m = re.search(r'public\s+const\s+int\s+MinLength\s*=\s*(\d+);', cs_text)
ts_m = re.search(r'export\s+const\s+MIN_PASSWORD_LENGTH\s*=\s*(\d+);', ts_text)
if not m:
print('server_min_length=')
else:
print('server_min_length=' + m.group(1))
if not ts_m:
print('client_min_length=')
else:
print('client_min_length=' + ts_m.group(1))
if m and ts_m:
print('values_match=' + str(m.group(1) == ts_m.group(1)))
PY
echo
echo "== check any pinned cross-layer test =="
rg -n --type=cs 'MIN_PASSWORD_LENGTH|PasswordPolicy\.MinLength|MinLength' tests src/Api/Features/Auth src/Api/Http/ErrorCodes.cs || trueRepository: imariel2d/keepr
Length of output: 10280
Keep the server and client password-length values pinned together.
PasswordPolicy.MinLength and MIN_PASSWORD_LENGTH are both 12, but they can drift when either side changes. Add a test or shared constant contract to catch future changes before one side rejects passwords the other says are valid.
🤖 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/Api/Features/Auth/PasswordPolicy.cs` around lines 28 - 33, Keep
PasswordPolicy.MinLength and the client MIN_PASSWORD_LENGTH synchronized by
adding a shared constant contract or test that asserts both values remain equal
to 12. Anchor the change to PasswordPolicy.MinLength and the existing
client-length symbol, preserving current validation behavior while failing
clearly if either value changes independently.
| public sealed class EmailStrings | ||
| { | ||
| // ---- shared ---- | ||
| public required string LangTag; // <html lang="…"> | ||
| public required string IgnoreFooter; // footer boilerplate on every email | ||
| public required string ExpiresFooterFormat; // "This link expires in {0}. If the button…" | ||
| public required string ExpiresTextFormat; // text-body "This link expires in {0}." | ||
| public required string Day; | ||
| public required string Days; | ||
| public required string Minute; | ||
| public required string Minutes; | ||
| public required string Hour; | ||
| public required string Hours; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
These are public writable fields on shared static instances, so any code can overwrite the copy for every request.
required only forces the field to be set when the object is created. It does not make it read-only afterwards. En, Es, and Fr are static readonly singletons, so a line anywhere in the process such as EmailStrings.En.InviteSubject = "test" silently changes the subject line of every invite the server sends from then on. Nothing does that today, but the type is one careless assignment away from a process-wide content bug, and it would be very hard to trace.
Make them init-only properties. The object initializers below need no change.
♻️ Proposed change (same pattern for every field)
- public required string LangTag; // <html lang="…">
- public required string IgnoreFooter; // footer boilerplate on every email
- public required string ExpiresFooterFormat; // "This link expires in {0}. If the button…"
+ public required string LangTag { get; init; } // <html lang="…">
+ public required string IgnoreFooter { get; init; } // footer boilerplate on every email
+ public required string ExpiresFooterFormat { get; init; } // "This link expires in {0}. If the button…"🤖 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/Api/Features/Email/EmailStrings.cs` around lines 11 - 23, Update every
public writable field in EmailStrings, including the shared fields shown and the
remaining localized string members, to init-only properties while retaining
required and their existing types. This preserves the existing object
initializers while preventing mutations to the static En, Es, and Fr instances
after construction.
| public static EmailContent Invite(string claimUrl, string? invitedBy, int expiryDays, string? locale = null) | ||
| { | ||
| var s = EmailStrings.For(locale); | ||
| var by = string.IsNullOrWhiteSpace(invitedBy) ? null : invitedBy; | ||
| var expiry = expiryDays == 1 ? "1 day" : $"{expiryDays} days"; | ||
| var expiry = s.FormatDays(expiryDays); | ||
|
|
||
| var intro = by is null | ||
| ? "You've been invited to Keepr, a private place to keep your files." | ||
| : $"{by} has invited you to Keepr, a private place to keep your files."; | ||
| var intro = by is null ? s.InviteIntroGeneric : string.Format(s.InviteIntroByFormat, by); | ||
|
|
||
| var bodyHtml = | ||
| Paragraph(intro) + | ||
| Paragraph("Set a password to activate your account and sign in."); | ||
| var bodyHtml = Paragraph(intro) + Paragraph(s.InviteBody2); | ||
|
|
||
| var html = Layout( | ||
| preheader: "Set your password to activate your Keepr account.", | ||
| headline: "You're invited to Keepr", | ||
| var html = Layout(s, | ||
| preheader: s.InvitePreheader, | ||
| headline: s.InviteSubject, | ||
| bodyHtml: bodyHtml, | ||
| ctaText: "Set your password", | ||
| ctaText: s.InviteCta, | ||
| ctaUrl: claimUrl, | ||
| footerNote: $"This link expires in {expiry}. If the button doesn't work, paste this " | ||
| + "address into your browser:"); | ||
| footerNote: string.Format(s.ExpiresFooterFormat, expiry)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'EmailTemplateTests.cs' --exec cat -n {}Repository: imariel2d/keepr
Length of output: 8775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate email-related files"
fd -t f 'Email.*(cs|json|resx|props)$|.*Email.*' . | sed 's#^\./##' | head -200
echo
echo "Find EmailStrings definitions and templates"
rg -n "class EmailStrings|EmailStrings|static .*Invite|PasswordReset|ConfirmEmailChange|EmailChanged|string.Format|For\\(" -S .Repository: imariel2d/keepr
Length of output: 28711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only behavior probe for the string formatting concern.
def bad_invite_strings():
return type("S", (), {
"InviteIntroGeneric": "You { were invited",
"InviteIntroByFormat": "{1} has invited you",
"FormatDays": lambda self, days: "{0} days".format(days),
})()
class EmailTemplates:
`@staticmethod`
def Invite(claimUrl, invitedBy, expiryDays, locale=None):
s = bad_invite_strings()
by = "" if invitedBy is None else invitedBy
return {
"generic": Paragraph(s.InviteIntroGeneric) + Paragraph(s.InviteBody2),
"by": Paragraph(string.Format(s.InviteIntroByFormat, by)) + Paragraph(s.InviteBody2),
}
# No-op for imports needed by the probe.
import string
def Paragraph(s):
return f"<p>{s}</p>\n"
for part in ("generic", "by"):
try:
EmailTemplates.Invite("https://example.com", invitedBy=None if part=="generic" else "Jane Doe", expiryDays=7, locale="de")
print(part, "OK")
except Exception as e:
print(part, type(e).__name__, str(e))
PYRepository: imariel2d/keepr
Length of output: 278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "EmailTemplates relevant implementation"
sed -n '1,190p' src/Api/Features/Email/EmailTemplates.cs | cat -n
echo
echo "EmailStrings English/Spanish/French relevant fields"
sed -n '80,230p' src/Api/Features/Email/EmailStrings.cs | cat -n
echo
echo "Current locale template coverage in EmailTemplateTests.cs"
python3 - <<'PY'
import re
from pathlib import Path
p = Path("tests/Api.Tests/EmailTemplateTests.cs")
s = p.read_text()
for name in [
"EmailTemplates.Invite",
"EmailTemplates.PasswordReset",
"EmailTemplates.ConfirmEmailChange",
"EmailTemplates.EmailChanged",
]:
print(name, s.count(name))
print("locale claims:", s.count('locale: "es"') + s.count("locale: 'es'"), "es")
print("locale claims:", s.count('locale: "fr"') + s.count("locale: 'fr'"), "fr")
PYRepository: imariel2d/keepr
Length of output: 20283
🌐 Web query:
.NET string.Format mismatched placeholder indexes FormatException {1} example
💡 Result:
In.NET, the string.Format method uses zero-based composite formatting [1][2]. A FormatException occurs at runtime if a format item (placeholder) refers to an index that is greater than or equal to the number of arguments provided in the parameter list [1][3][4]. Example of a Mismatched Placeholder Index: In the following example, the format string includes {2}, which refers to the third argument (index 2). However, only two arguments (index 0 and 1) are provided, triggering a FormatException [5][4]. csharp string name = "Alice"; string role = "Developer"; // This will throw System.FormatException: // "Index (zero based) must be greater than or equal to zero and less than the size of the argument list." string result = string.Format("User: {0}, Role: {1}, Status: {2}", name, role); To fix this, ensure that every index used in the format string has a corresponding argument in the method call [5][4]. Corrected code: csharp string name = "Alice"; string role = "Developer"; string status = "Active"; // All indexes {0}, {1}, {2} have corresponding arguments string result = string.Format("User: {0}, Role: {1}, Status: {2}", name, role, status); Key points to remember: - Zero-based indexing: The first argument is {0}, the second is {1}, and so on [2][6]. - Missing arguments: If you use a higher index than the number of arguments available, a FormatException is thrown [1][4]. - Extra arguments: It is perfectly acceptable to have more arguments than format items; unused arguments are simply ignored [2]. - Same index reuse: You can use the same index multiple times in the format string (e.g., {0} is a {0}) without error [1][3].
Citations:
- 1: https://github.com/dotnet/docs/blob/main/docs/fundamentals/runtime-libraries/system-string-format.md
- 2: https://learn.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.string.format?view=net-8.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.formatexception?view=netframework-4.8.1
- 5: https://stackoverflow.com/questions/36307549/formatexception-in-string-format
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.string.format?view=net-9.0
Render every email locale before marking email localization green.
The current EmailTemplateTests.cs coverage is good for Invite and PasswordReset, but it does not render ConfirmEmailChange or EmailChanged in Spanish or French. A bad {1} placeholder or stray { in those machine-translated catalogs would throw FormatException when sending, so add render tests that exercise the four email templates in en, es, and fr.
🤖 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/Api/Features/Email/EmailTemplates.cs` around lines 37 - 53, Extend
EmailTemplateTests to render Invite, PasswordReset, ConfirmEmailChange, and
EmailChanged for each locale en, es, and fr, using representative valid
arguments and asserting successful output. Ensure the tests exercise all
localized format strings so malformed placeholders or braces raise failures.
| public async Task SendAsync( | ||
| string toEmail, string rawToken, string? invitedByName, CancellationToken ct, string? locale = null) | ||
| { | ||
| var s = await settings.GetAsync(ct); | ||
| var expiryDays = Math.Max(1, s.InviteExpiryDays); | ||
| var claimUrl = $"{ResolveBaseUrl(s.PublicBaseUrl)}/claim/{rawToken}"; | ||
| var content = EmailTemplates.Invite(claimUrl, invitedByName, expiryDays); | ||
| var content = EmailTemplates.Invite(claimUrl, invitedByName, expiryDays, locale); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
locale was appended after CancellationToken in all four email-send methods. The .NET convention, enforced by analyzer rule CA1068, puts CancellationToken last. Move locale in front of ct and give it a default, or make it a required parameter and update the callers. Also add a <paramref name="locale"/> line to each XML doc, since these are public members and GenerateDocumentationFile is on.
src/Api/Features/Invites/InviteService.cs#L52-L58: changeSendAsyncto(string toEmail, string rawToken, string? invitedByName, string? locale, CancellationToken ct).src/Api/Features/Auth/EmailChangeService.cs#L58-L64: changeSendConfirmationAsyncto(string newEmail, string rawToken, string? locale, CancellationToken ct).src/Api/Features/Auth/EmailChangeService.cs#L75-L78: changeSendChangedNoticeAsyncto(string oldEmail, string newEmail, string? locale, CancellationToken ct).src/Api/Features/Auth/PasswordResetService.cs#L53-L58: changeSendAsyncto(string toEmail, string rawToken, string? locale, CancellationToken ct).
One upside of dropping the default: the compiler then points at every caller, which makes it obvious if a send site forgot to pass the recipient's language and would have quietly emailed them in English.
📍 Affects 3 files
src/Api/Features/Invites/InviteService.cs#L52-L58(this comment)src/Api/Features/Auth/EmailChangeService.cs#L58-L64src/Api/Features/Auth/EmailChangeService.cs#L75-L78src/Api/Features/Auth/PasswordResetService.cs#L53-L58
🤖 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/Api/Features/Invites/InviteService.cs` around lines 52 - 58, Update
InviteService.cs lines 52-58: change SendAsync to place nullable locale before
CancellationToken, make locale required, update all callers, and add a locale
param entry to its XML documentation. Apply the same changes to
EmailChangeService.cs lines 58-64 for SendConfirmationAsync and lines 75-78 for
SendChangedNoticeAsync, plus PasswordResetService.cs lines 53-58 for SendAsync;
update every caller to pass the recipient’s locale and document locale on each
public method.
Source: Path instructions
| const updated = await this.api.update( | ||
| this.firstName().trim() || null, | ||
| this.lastName().trim() || null, | ||
| this.profile()?.preferredLanguage ?? null // full-replace: keep the stored language untouched | ||
| ); | ||
| this.store.set(updated); | ||
| this.nameNotice.set('Profile saved.'); | ||
| this.nameNotice.set($localize`:@@profile.name.saved:Profile saved.`); | ||
| } catch (e) { | ||
| this.nameError.set(problemDetail(e, 'Could not save your profile.')); | ||
| this.nameError.set(errorMessage(e)); | ||
| } finally { | ||
| this.savingName.set(false); | ||
| } | ||
| } | ||
|
|
||
| // Language (#30). The switcher persists the choice to the account, then reloads into that locale's | ||
| // build (compile-time i18n has no in-place swap). Uses the stored names so it never disturbs them. | ||
| protected readonly savingLanguage = signal(false); | ||
| protected readonly languageError = signal<string | null>(null); | ||
|
|
||
| protected async changeLanguage(locale: Locale): Promise<void> { | ||
| const p = this.profile(); | ||
| if (!p || this.savingLanguage() || locale === this.locale.current) return; | ||
| this.savingLanguage.set(true); | ||
| this.languageError.set(null); | ||
| try { | ||
| await this.api.update(p.firstName, p.lastName, locale); | ||
| this.locale.switchTo(locale); // persists cookie + reloads into /{locale}/ | ||
| } catch (e) { | ||
| this.languageError.set(errorMessage(e)); | ||
| this.savingLanguage.set(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize complete profile updates.
saveName() and changeLanguage() can run at the same time. The API replaces FirstName, LastName, and PreferredLanguage on every request.
For example, start with { firstName: "Ana", preferredLanguage: "en" }. Save the name as "Anne", then select "fr" before the first request completes. One request sends { "Anne", "en" }. The other sends { "Ana", "fr" }. The request that completes last discards the other change.
Use one shared profile-save operation, or disable the other profile mutation while a request is active. Refresh or update ProfileStore from the successful response before allowing the next update.
🤖 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/ClientApp/src/app/features/profile/profile.ts` around lines 114 - 145,
Serialize saveName() and changeLanguage() through one shared profile-update
operation so concurrent full-replacement requests cannot overwrite each other.
Ensure the successful API response updates ProfileStore before the next mutation
proceeds, and preserve each action’s existing success, error, loading, and
locale-switch behavior.
| protected purgeLabel(item: TrashItem): string { | ||
| const days = this.daysLeft(item); | ||
| if (days === 0) return 'Deletes today'; | ||
| return days === 1 ? 'Deletes tomorrow' : `Deletes in ${days} days`; | ||
| if (days === 0) return $localize`:@@trash.deletes_today:Deletes today`; | ||
| return days === 1 | ||
| ? $localize`:@@trash.deletes_tomorrow:Deletes tomorrow` | ||
| : $localize`:@@trash.deletes_in_days:Deletes in ${days}:days: days`; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The day countdown hard-codes English plural rules.
The three branches pick the wording: 0 gives "Deletes today", 1 gives "Deletes tomorrow", and everything else gets "Deletes in N days" with a fixed plural noun. That happens to be grammatical in Spanish and French too, because both use one plural form for every count above one. So nothing is wrong today.
It breaks the first time a fourth locale arrives. Polish, Russian, and Arabic each need two or three different forms depending on the number — 2 days, 5 days, and 22 days take different endings. The catalog cannot express that, because trash.deletes_in_days is a flat string. Nothing would fail a build or a test; the app would just print bad grammar.
Angular's ICU plural syntax handles this, and this catalog already uses it elsewhere — see files.result_count and trash.summary in messages.es.xlf. Using it here keeps the plural decision in the translation instead of in the TypeScript.
Not urgent for en/es/fr. Worth doing before locale four.
♻️ Sketch
protected purgeLabel(item: TrashItem): string {
const days = this.daysLeft(item);
if (days === 0) return $localize`:@@trash.deletes_today:Deletes today`;
- return days === 1
- ? $localize`:@@trash.deletes_tomorrow:Deletes tomorrow`
- : $localize`:@@trash.deletes_in_days:Deletes in ${days}:days: days`;
+ if (days === 1) return $localize`:@@trash.deletes_tomorrow:Deletes tomorrow`;
+ return $localize`:@@trash.deletes_in_days:Deletes in {${days}:days:, plural, other {# days}}`;
}Then re-run npm run extract-i18n and re-translate the unit as an ICU message in all three catalogs.
🤖 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/ClientApp/src/app/features/trash/trash.ts` around lines 105 - 111, Update
purgeLabel in TrashItem handling so the nonzero countdown uses a single ICU
pluralized translation message instead of branching to a fixed “days” string in
TypeScript. Preserve the existing today and tomorrow messages, update the source
and all three locale catalogs using the established ICU patterns such as
trash.summary, then regenerate translations with npm run extract-i18n.
| <trans-unit id="admin.create.warn" datatype="html"> | ||
| <source><x id="START_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/><x id="CLOSE_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/> This email isn't verified. If email delivery is turned on later, the real owner of this address could claim this account. Prefer an email invite once a mailer is configured. </source> | ||
| <target state="needs-review-translation">Este correo electrónico no está verificado. Si la entrega de correo se activa más tarde, el verdadero propietario de esta dirección podría reclamar esta cuenta. Es preferible una invitación por correo una vez configurado un servicio de correo.</target> | ||
| </trans-unit> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Two Spanish translations drop the icon placeholders that their sources contain.
Look at admin.create.warn. The <source> on line 229 opens with <x id="START_TAG_COVE_ICON" .../> and <x id="CLOSE_TAG_COVE_ICON" .../>. Those two markers stand in for the <cove-icon name="alert-triangle"> element in admin.html line 149. The <target> on line 230 contains neither.
admin.remove.warn has the same gap: source line 317 carries both icon markers, target line 318 carries none.
What happens: @angular/localize does not fail when a target omits a placeholder — it just leaves that part out. So in the Spanish build the warning triangle icon vanishes from the "this email isn't verified" notice and from the "remove account" danger note. English and French keep it. Nothing crashes; the two locales simply look different, and the visual danger cue is weakest exactly where the copy is most severe.
Add both markers back at the start of each target, matching the source order.
🔧 Proposed fix
<trans-unit id="admin.create.warn" datatype="html">
<source><x id="START_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/><x id="CLOSE_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/> This email isn't verified. If email delivery is turned on later, the real owner of this address could claim this account. Prefer an email invite once a mailer is configured. </source>
- <target state="needs-review-translation">Este correo electrónico no está verificado. Si la entrega de correo se activa más tarde, el verdadero propietario de esta dirección podría reclamar esta cuenta. Es preferible una invitación por correo una vez configurado un servicio de correo.</target>
+ <target state="needs-review-translation"><x id="START_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/><x id="CLOSE_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="14" />"/> Este correo electrónico no está verificado. Si la entrega de correo se activa más tarde, el verdadero propietario de esta dirección podría reclamar esta cuenta. Es preferible una invitación por correo una vez configurado un servicio de correo.</target> <trans-unit id="admin.remove.warn" datatype="html">
<source><x id="START_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="15" />"/><x id="CLOSE_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="15" />"/> This signs them out immediately and permanently deletes their account and all files — including trash. There is no recovery. </source>
- <target state="needs-review-translation">Esto cierra su sesión de inmediato y elimina permanentemente su cuenta y todos los archivos, incluida la papelera. No hay recuperación posible.</target>
+ <target state="needs-review-translation"><x id="START_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="15" />"/><x id="CLOSE_TAG_COVE_ICON" ctype="x-cove_icon" equiv-text="<cove-icon name="alert-triangle" [size]="15" />"/> Esto cierra su sesión de inmediato y elimina permanentemente su cuenta y todos los archivos, incluida la papelera. No hay recuperación posible.</target>messages.fr.xlf is not in this review. Check the same two units there.
#!/bin/bash
# Description: Find every trans-unit whose target omits a placeholder present in its source.
set -euo pipefail
pip install --quiet lxml >/dev/null 2>&1 || true
python - <<'PY'
import re, glob
from xml.etree import ElementTree as ET
NS = {'x': 'urn:oasis:names:tc:xliff:document:1.2'}
def ids(el):
return sorted(c.get('id') for c in el.iter() if c.tag.endswith('}x') and c.get('id'))
for path in sorted(glob.glob('src/ClientApp/src/locale/messages.*.xlf')):
tree = ET.parse(path)
bad = 0
for unit in tree.iter('{urn:oasis:names:tc:xliff:document:1.2}trans-unit'):
src = unit.find('x:source', NS)
tgt = unit.find('x:target', NS)
if src is None or tgt is None:
continue
s, t = ids(src), ids(tgt)
if s != t:
bad += 1
print(f"{path} {unit.get('id')}\n source: {s}\n target: {t}")
print(f"{path}: {bad} mismatched unit(s)\n")
PYAlso applies to: 316-319
🤖 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/ClientApp/src/locale/messages.es.xlf` around lines 228 - 231, Restore the
missing cove-icon placeholders in the Spanish targets for trans-units
admin.create.warn and admin.remove.warn, placing START_TAG_COVE_ICON followed by
CLOSE_TAG_COVE_ICON at the beginning to match each source. Check the
corresponding units in messages.fr.xlf and apply the same correction wherever
those target markers are missing.
| <trans-unit id="trash.deletes_today" datatype="html"> | ||
| <source>Deletes today</source> | ||
| <target state="needs-review-translation">Supprimé aujourd'hui</target> | ||
| </trans-unit> | ||
| <trans-unit id="trash.deletes_tomorrow" datatype="html"> | ||
| <source>Deletes tomorrow</source> | ||
| <target state="needs-review-translation">Supprimé demain</target> | ||
| </trans-unit> | ||
| <trans-unit id="trash.deletes_in_days" datatype="html"> | ||
| <source>Deletes in <x id="days" equiv-text="days"/> days</source> | ||
| <target state="needs-review-translation">Supprimé dans <x id="days" equiv-text="days"/> jours</target> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use future-tense wording for scheduled deletion.
For an item scheduled for deletion today or tomorrow, the targets say Supprimé aujourd'hui, Supprimé demain, and Supprimé dans ... jours. These phrases mean the item was deleted. Users may misunderstand whether the item is still recoverable.
Use wording such as Sera supprimé aujourd'hui, Sera supprimé demain, and Sera supprimé dans ... jours.
Suggested French targets
- <target state="needs-review-translation">Supprimé aujourd'hui</target>
+ <target state="needs-review-translation">Sera supprimé aujourd'hui</target>
...
- <target state="needs-review-translation">Supprimé demain</target>
+ <target state="needs-review-translation">Sera supprimé demain</target>
...
- <target state="needs-review-translation">Supprimé dans <x id="days" equiv-text="days"/> jours</target>
+ <target state="needs-review-translation">Sera supprimé dans <x id="days" equiv-text="days"/> jours</target>📝 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.
| <trans-unit id="trash.deletes_today" datatype="html"> | |
| <source>Deletes today</source> | |
| <target state="needs-review-translation">Supprimé aujourd'hui</target> | |
| </trans-unit> | |
| <trans-unit id="trash.deletes_tomorrow" datatype="html"> | |
| <source>Deletes tomorrow</source> | |
| <target state="needs-review-translation">Supprimé demain</target> | |
| </trans-unit> | |
| <trans-unit id="trash.deletes_in_days" datatype="html"> | |
| <source>Deletes in <x id="days" equiv-text="days"/> days</source> | |
| <target state="needs-review-translation">Supprimé dans <x id="days" equiv-text="days"/> jours</target> | |
| <trans-unit id="trash.deletes_today" datatype="html"> | |
| <source>Deletes today</source> | |
| <target state="needs-review-translation">Sera supprimé aujourd'hui</target> | |
| </trans-unit> | |
| <trans-unit id="trash.deletes_tomorrow" datatype="html"> | |
| <source>Deletes tomorrow</source> | |
| <target state="needs-review-translation">Sera supprimé demain</target> | |
| </trans-unit> | |
| <trans-unit id="trash.deletes_in_days" datatype="html"> | |
| <source>Deletes in <x id="days" equiv-text="days"/> days</source> | |
| <target state="needs-review-translation">Sera supprimé dans <x id="days" equiv-text="days"/> jours</target> |
🤖 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/ClientApp/src/locale/messages.fr.xlf` around lines 1184 - 1194, Update
the French targets in the trash.deletes_today, trash.deletes_tomorrow, and
trash.deletes_in_days translation units to use future-tense wording, preserving
the existing date phrases and days placeholder while changing “Supprimé” to
“Sera supprimé”.
|
|
||
| [Fact] | ||
| public void Emails_render_in_the_recipients_locale() | ||
| { | ||
| // #30 Phase 3: the recipient's preferred language selects the copy; the expiry pluralizes | ||
| // in-language and the <html lang> follows. | ||
| var es = EmailTemplates.Invite(ClaimUrl, invitedBy: null, expiryDays: 7, locale: "es"); | ||
| Assert.Contains("Te han invitado a Keepr", es.Subject); | ||
| Assert.Contains("7 días", es.TextBody); | ||
| Assert.Contains("lang=\"es\"", es.HtmlBody); | ||
|
|
||
| var fr = EmailTemplates.PasswordReset(ResetUrl, expiryMinutes: 1, locale: "fr"); | ||
| Assert.Contains("Réinitialisez votre mot de passe Keepr", fr.Subject); | ||
| Assert.Contains("1 minute", fr.TextBody); | ||
| Assert.Contains("lang=\"fr\"", fr.HtmlBody); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Cover the remaining localized email templates.
The new test passes "es" only to Invite and "fr" only to PasswordReset. Existing tests for ConfirmEmailChange and EmailChanged use the default locale. If either template ignores a non-English locale, the current suite still passes. Add one non-English assertion for each template, including the subject, body, and language attribute.
Based on the PR objectives, Phase 3 includes confirmation and old-address email localization.
🤖 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/Api.Tests/EmailTemplateTests.cs` around lines 147 - 162, Extend
Emails_render_in_the_recipients_locale to exercise non-English locales for
EmailTemplates.ConfirmEmailChange and EmailTemplates.EmailChanged, asserting
each localized subject, body content, and HTML lang attribute; keep the existing
Invite and PasswordReset coverage unchanged.
…en (#30)
The locale-serving change only redirected "/" — so any unprefixed deep path
(/login, /claim/:token, /s/token) 404'd once the app moved under /en//es//fr/.
That broke the Playwright e2e suite, which navigates to unprefixed paths.
- Program.cs: a catch-all MapFallback redirects every non-locale-prefixed
path to the same path under the picked locale (cookie-or-English),
preserving path + query. The per-locale fallbacks stay more specific, so
they still win for /{locale}/…; an unmatched /api route still 404s.
- admin: the "Set quota" row-menu label had been reworded to "Set storage
quota" (reusing the modal-title id) — restored to "Set quota" with its own
id @@admin.quota.menu, since the e2e clicks that menu item by name.
- es/fr catalogs → 299 units, id-complete.
The regex URL assertions (/\/files/, /\/login/) still match with the /en/
prefix, and the email-content assertions are unaffected (invited/reset
accounts have no language preference → English, byte-identical). API build +
224 tests green; client builds clean. Not run locally: the full dockerised
Playwright stack — the fix targets the identified 404 root cause + the one
reworded label.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What
Designs feature #30 Localization, broadened from "Spanish only" to full i18n into English (default), Spanish, and French — covering UI copy and user-facing server errors — plus a per-user preferred language (
User.PreferredLanguage, nullable → English). No code yet; this is the design + the discipline to keep translations in step.Deliverables
docs/feature-30-localization.md— the design, with the mandatory e2e scenarios + expected output (per thefeature-e2e-designskill)..claude/skills/i18n-translations/SKILL.md— a repo skill that makes "translations are complete in all three locales" a load-bearing rule: it triggers whenever UI text changes or a server error is added.docs/feature-status.md— #30 broadened and marked 📐 Designed; header/summary counts kept in step.Key decisions (all settled with the requester)
@angular/localize: one built bundle per locale served under/en/ /es/ /fr/. A language change is a reload into that build, not an in-place swap (accepted). Drives the one non-mechanical server change:Program.csmoves from a single SPA fallback to a root redirect + per-locale fallbacks.codes: every user-facingProblem()carries a machinecode; the client owns the translated copy; the Englishdetailis the fallback. Extends thecodeseed already in the repo (email_in_use,email_not_configured,email_unverified)./with no cookie always defaults to/en/;Accept-Languageis ignored for the redirect.es/frmachine-translated, entries markedstate="needs-review", corrected iteratively.Phasing
P1 foundation + primary screens + business-error codes → P2 field-validation codes → P3 server-side localized emails.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests