Skip to content

fix(i18n): promote framed send_failed out of the designTweak namespace (#4240) - #4251

Merged
bolichen97 merged 1 commit into
mainfrom
fix/promote-send-failed-catalog-4240
Aug 18, 2026
Merged

fix(i18n): promote framed send_failed out of the designTweak namespace (#4240)#4251
bolichen97 merged 1 commit into
mainfrom
fix/promote-send-failed-catalog-4240

Conversation

@chenmingwei23

Copy link
Copy Markdown
Contributor

Problem / Motivation

The framed send-failure string "Send failed: {{error}}" lives at apps.designTweak.status.send_failed -- an APP's catalog namespace. Design Review flagged on PR #4214 that core chrome (the refused-send error rows planned for #4198) frames the server reason with that key because it is the only framed variant in the catalog. Core UI text would then be owned by an app's catalog subtree.

Why it matters

An app-side string edit (rewording designTweak's send-failure status) would silently reword core chat error rows in all 12 languages -- with no test, gate, or reviewer signal connecting the two surfaces. Namespace ownership is the boundary that prevents that class of silent cross-surface drift.

What changed (motivation -> approach -> change)

Symptom: core call sites must reference an apps.designTweak.* key to get a framed send-failure string. Root cause: the framed variant was only ever added to the app namespace. Fix: promote the entry to the shared namespace the issue proposes (pages.chatPage).

  • The literal key pages.chatPage.send_failed already exists (unframed "Send failed", live in ChatPage.tsx with a no-parameter call), so the framed variant lands as a sibling: pages.chatPage.send_failed_with_error. Reusing the existing key would have changed ChatPage's rendered text ("One key, one meaning" per website/docs/i18n-catalog.md).
  • Moved the entry in all 12 hand-authored catalogs (en.manual.json + 11 translations) -- values byte-identical in every locale; this is a namespace move, not a wording change. Regenerated en-XA via scripts/gen-pseudolocale.mjs.
  • Repointed designTweak's three call sites (DesignTweakPage.tsx) and its test to the shared key, and deleted the app-namespace entry (designTweak no longer uses it, and a dangling duplicate would rot).

Coordination note: open PR #4214 references the old key from its new call sites (App.tsx, useSceneInteraction.tsx). Once this merges, its rebase repoints them to pages.chatPage.send_failed_with_error -- its own check-i18n-keys hard-zero gate makes that unmissable rather than silent.

Tests

  • New website/src/i18n/sendFailedSharedKey.test.ts (mutation-verified in both directions):
    • every runtime catalog carries pages.chatPage.send_failed_with_error with the {{error}} placeholder intact (a missing key would render the raw key string as UI text);
    • no catalog still carries the retired apps.designTweak.status.send_failed;
    • no source file references the retired key (guards against re-creating the core-depends-on-app coupling).
  • Updated DesignTweakRequestsCov80.test.tsx asserts the send-failure status renders from the shared key.

Manual verification

N/A -- unit coverage sufficient: the change is a catalog key move with byte-identical values, locked by the parity/i18n gate chain (i18n:check green: pseudolocale, key-refs hard-zero, DNT, parity) plus the new drift-guard test.

Screenshots / video

Why no screenshot: pure i18n key rename with byte-identical string values in every locale -- no pixel changes.

Related Issues

Closes #4240

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) -- N/A, no documented behavior changed
  • No secrets, credentials, or internal references in the diff

#4240)

The refused-send error rows planned for core chrome frame the server reason
with apps.designTweak.status.send_failed because it was the only framed
variant in the catalog. That makes core depend on an app's catalog
namespace: an app-side string edit could silently reword core error rows.

Move the entry to pages.chatPage.send_failed_with_error (a sibling of the
existing unframed pages.chatPage.send_failed) across all 12 hand-authored
locale catalogs, regenerate en-XA, repoint designTweak's three call sites
and its test, and delete the app-namespace entry. String values are
byte-identical in every locale: this is a namespace move, not a wording
change.

A drift-guard test asserts the shared key exists framed in every runtime
catalog and that no source file references the retired app key.

Closes #4240
@chenmingwei23
chenmingwei23 requested a review from a team August 18, 2026 02:56
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 18, 2026 02:56
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 053064465d0a0eb1f637028707d274464a8a134a and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0530644

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 053064465d0a0eb1f637028707d274464a8a134a: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 053064465d0a0eb1f637028707d274464a8a134a — updated in place on each push; does not block merge.

This PR is purely an i18n key relocation — the string "Send failed: {{error}}" moves from the app-specific apps.designTweak.status.send_failed namespace to the shared pages.chatPage.send_failed_with_error, byte-identical in all 12 locales (English lives in en.manual.json, which merges over the generated en.json, so the "missing" key there is expected). The three call sites in DesignTweakPage.tsx are repointed, the old key is deleted, and a drift-guard test pins the new arrangement. No rendered pixel, label, flow, or state changes for any user; no screenshots in the diff. The raw-{{error}} interpolation pattern predates this PR and is unchanged.

UX-Verdict: PASS

Pure key relocation — every locale string is byte-identical, so no user sees anything different.

[UX-REVIEWED] 0530644

@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 053064465d0a0eb1f637028707d274464a8a134a — updated in place on each push; does not block merge.

Design review complete. The diff is a namespace move (app → core catalog) with byte-identical values across all 12 locales, a new drift-guard test, and repointed call sites; I checked the catalog structure (no common namespace exists — pages.chatPage is the right home since the unframed sibling and the planned #4198 consumer both live there), the "one key, one meaning" rule in website/docs/i18n-catalog.md (same meaning and grammatical role, so sharing is sanctioned), and the coordination story with open PR #4214 (its hard-zero key-refs gate catches the retired key on rebase). The reversed coupling — designTweak now rendering a core-owned string — is the standard app-consumes-core direction and deliberate, not drift.

Design-Verdict: PASS

Root-cause fix at the right ownership boundary: core-owned key, sibling not reuse, all locales moved atomically, dependent-PR handoff gated rather than hoped.

[DESIGN-REVIEWED] 0530644

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 053064465d0a0eb1f637028707d274464a8a134a — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 0530644

Verdict parsed from the review's SHA-scoped output markers for commit 053064465d0a0eb1f637028707d274464a8a134a.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 053064465d0a0eb1f637028707d274464a8a134a: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Advisory premise-level review of 053064465d0a0eb1f637028707d274464a8a134a — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

First-Principles-Verdict: CONCERNS

The key move is a cause-level fix with a recorded decision behind it; the 85-line guard test mostly re-tests three gates that already exist.

What this change ships

Intent: keep core chat error text out of an app-owned catalog namespace before #4214 makes core depend on it — a FIX.

  1. Framed "Send failed: {{error}}" now lives at pages.chatPage.send_failed_with_error in all 12 locales — justified (issue Promote framed send_failed catalog entry out of the designTweak app namespace #4240; "one key, one meaning" forces the sibling key).
  2. designTweak's three status call sites read the shared key, rendered text unchanged — justified.
  3. apps.designTweak.status.send_failed deleted from every catalog — justified; grep for non-app references to apps.* keys: 0 source files, so the class is fully closed, no unfixed siblings.
  4. New sendFailedSharedKey.test.ts drift guard — rides along; duplicate of catalogParity.test.ts + [key-refs] + deadKeys.test.ts.
  5. DesignTweakRequestsCov80.test.tsx asserts the new key — justified.

Watch

  • Test 1 ("every locale carries the key with {{error}}") is a second spelling: catalogParity.test.ts:149 (missing-key) and :208 (placeholder parity) cover the 11 translations, and the [key-refs] hard zero (check-i18n-keys.mjs:699) covers en via the 3 live call sites. Test 3 ("no source references the retired key") is the same gate again — your own description says so: "its own check-i18n-keys hard-zero gate makes that unmissable." The test header's claim that both invariants "failed silently before" is contradicted by that gate chain.
  • Only test 2 (retired key must not reappear in a catalog) has residual value, and even there an unreferenced re-add already fails deadKeys.test.ts's exact-baseline check.

Subtractions

  • Shrink sendFailedSharedKey.test.ts to the single retired-key-absence assertion (its test 2); delete tests 1 and 3 — each is owned by a named existing gate (catalogParity.test.ts, check-i18n-keys.mjs).

[FIRST-PRINCIPLES-REVIEWED] 0530644

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 18, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the First Principles CONCERNS (advisory, head 0530644):

Finding: tests 1 and 3 of sendFailedSharedKey.test.ts re-test gates that already exist; shrink to the retired-key-absence assertion (test 2).

Disposition: rebutted-in-part (proportionality), accepted-and-deferred-in-part.

  • The redundancy analysis is substantively correct and I am not disputing the coverage mapping: test 1's missing-key/placeholder half is covered by catalogParity.test.ts for the 11 translations plus the [key-refs] hard-zero for en, and test 3 overlaps the same check-i18n-keys gate. One nuance keeps them from being pure duplicates: this file asserts the INTENT ("the framed send-failed string is core-owned") as one named contract with the issue's rationale in its header, which the three general-purpose gates express only as an emergent property scattered across them. That is documentation value, not detection value -- so it justifies keeping the file, not necessarily all three assertions.
  • What I decline to do is push a test-shrink amend onto an all-green head. The change the finding asks for has zero behavioral delta and its only cost today is ~170ms of vitest time, while a new push re-arms every reviewer lane on this PR and re-rolls the non-deterministic review dice for no merge benefit. That trade is disproportional right now.
  • Accepted for the first legitimate re-push: if any blocking finding or conflict forces a new commit on this branch, the trim (drop tests 1 and 3, keep test 2 and the header) rides along in the same amend. Recorded in the operator's local backlog as f-20260818-02 so it is not lost if the PR merges untouched.

@bolichen97
bolichen97 enabled auto-merge (squash) August 18, 2026 04:02

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: cosmetic-UI/i18n (16 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: i18n key promotion from design-tweak namespace to shared namespace.

@bolichen97
bolichen97 merged commit 2ffbcca into main Aug 18, 2026
58 checks passed
@bolichen97
bolichen97 deleted the fix/promote-send-failed-catalog-4240 branch August 18, 2026 04:03
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 18, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
kirodotdev#4240) (kirodotdev#4251)

The refused-send error rows planned for core chrome frame the server reason
with apps.designTweak.status.send_failed because it was the only framed
variant in the catalog. That makes core depend on an app's catalog
namespace: an app-side string edit could silently reword core error rows.

Move the entry to pages.chatPage.send_failed_with_error (a sibling of the
existing unframed pages.chatPage.send_failed) across all 12 hand-authored
locale catalogs, regenerate en-XA, repoint designTweak's three call sites
and its test, and delete the app-namespace entry. String values are
byte-identical in every locale: this is a namespace move, not a wording
change.

A drift-guard test asserts the shared key exists framed in every runtime
catalog and that no source file references the retired app key.

Closes kirodotdev#4240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Promote framed send_failed catalog entry out of the designTweak app namespace

2 participants