GT-2321 Support updating language names and force-language-name flag

[wjames111]

Jira: GT-2321

Summary

• Adds inline editing to the Languages admin UI: each language row now has an Edit button that lets an admin update the language name, the crowdin code (which crowdin language it pulls from), and toggle the new force-language-name flag, with Save/Cancel controls
• Displays crowdin code and force-language-name as new columns in the languages list
• Adds the crowdin code input and force-language-name checkbox to the "Add new language" form (both attributes are permitted by the API on create)
• Adds LanguageService.updateLanguage() (PUT /languages/:id) and extends the create payload; attribute keys use the dashed JSON:API names the API serializes (crowdin-code, force-language-name)
• Language code remains read-only when editing, matching the API which does not permit code on update

Test plan

• yarn test --watch=false --browsers=ChromeHeadless — all 143 specs pass (2 new: update success alert, single-row editing)
• yarn lint and yarn prettier:check pass; yarn build succeeds
• Manual: load the Languages page, edit a language's name/crowdin code, toggle force-language-name, save, and confirm the list reflects the changes after reload
• Manual: Cancel discards unsaved edits; create a language with crowdin code + force-language-name set and confirm via the API response

🤖 Generated with Claude Code

[wjames111]

Multi-Agent Code Review (standard mode)

Verdict: APPROVED WITH SUGGESTIONS — no blockers. Risk: LOW (1/10).

5 specialized agents (Architecture, Testing, Data Integrity, UX, Standards) + dependency-impact analysis, with cross-examination and rebuttal rounds. 15 findings total: 6 medium-priority (posted as line comments below, dismissible via /dismiss: <reason> replies), 9 informational suggestions (listed here).

Verified correct (recorded as non-findings)

• Payload contract verified end-to-end against the Rails controller/serializer/schema: dashed attribute keys round-trip exactly; code correctly omitted on update (server does not permit it); omitting direction is safe (Rails permit only touches provided keys); '' || null is the only mechanism for clearing a crowdin code and the backend nullifies correctly; undefined || false matches the DB default for force_language_name.
• No breaking changes: model fields are additive; updateLanguage is net-new with a single caller; no other consumer reads the new fields; UI-state fields (isEditing, canConfirmDelete) can never leak into payloads.
• Failed save correctly retains the user's edits in the row for retry — any fix for the row-switch finding must preserve this.

Informational suggestions (severity < 5, do not require action or /dismiss)

• direction field added to the model but referenced nowhere (4.0) — wire it up or drop it.
• Last-write-wins on concurrent admin edits; no optimistic locking (4.5) — acceptable for this tool.
• createLanguage/updateLanguage duplicate the attributes block; siblings extract a private getPayload() (4.0).
• Save/error feedback renders at top of page, far from the edited row (UX 6.0, single-agent → suggestion tier; matches the component's pre-existing pattern).
• Blank crowdin-code cell could render — (3.5); no Enter/Escape keyboard handling in edit row (3.5); cancelEdit() untested (3.5); update-success spec asserts only the alert, not the service call (3.0); 5-column layout cramped below ~768px (3.5).

Debate highlights

The top finding (dirty row-switch) was contested 6.0–7.5 across agents and settled at 6.5 unanimously after rebuttal: it is display-only, self-healing, and consistent with the repo's existing modal editors (whose cancel paths revert nothing) — but all 5 agents endorse fixing it via the edit-a-clone approach before or shortly after merge.

🤖 Generated with Claude Code

[wjames111]

Multi-Agent Code Review — Round 2 (standard mode)

Verdict: APPROVED WITH SUGGESTIONS — 0 blockers. Risk: LOW (1/10).

Re-review after the 5 fix commits (97b62d2…85cb157). 5 specialized agents + dependency analysis, full cross-examination debate. The round-1 fixes were verified: the clone refactor is correct by construction (every path traced — failed saves retain edits, no route lets unsaved input display as saved or reach the wire), the new language.service.spec.ts correctly pins the wire contract, and no breaking changes exist.

⚠️ 1 Important finding (7.0) — strongly recommended; cannot be dismissed via /dismiss: the round-1 aria-label fix on the view-mode checkbox is non-functional (see line comment). One-token fix.

Suggestions (severity < 5, informational)

• [4.5] Delete/create while a row is being edited silently discards the draft via loadLanguages(). Recommended remedy (matches the component's one-operation-at-a-time invariants): disable Edit/Delete/create-Save while any row is editing.
• [4.0] direction on the Language model is still dead code (converged from 5.5/3.5 in debate; customPages sets precedent for typed-but-unread fields, so this is scope hygiene, not a defect).
• [3.5] Whitespace-only crowdin code is sent verbatim (' ' is truthy) and would bypass the crowdin_service.rb:17 truthiness guard server-side, turning a clean error into an obscure Crowdin API failure. Cheap fix: (language['crowdin-code'] || '').trim() || null in both payload builders.
• [3.0] Create-form Save button has no [disabled]="saving" (pre-existing line, shared-flag porosity).
• [3.0] "Saving…"/"Success!" alerts render inside the Add-new-language card during edit-row saves (shared flag; screen readers still get role="alert").
• [3.0] isEditing and canConfirmDelete can coexist on one row (Delete-confirm then Edit); each entry method should clear both flags.
• [2.5] Empty-name PUT: settled harmless — the API has validates :name, presence: true (422 surfaced by the existing alert, whitespace included).
• CI observation: Karma exits 0 despite browser-level ERROR output (NG0303, page-reload). Consider enabling @angular-eslint/template accessibility rules in a follow-up so the aria-label bug class can't recur silently.

Debate highlights

Testing self-revised its top finding 7.5→6.5 (missing regression guard over working code shouldn't outrank a shipped defect). The aria-label finding was verified independently four ways (Karma NG0303 ×4 with exit 0, ɵɵpropertyInterpolate1 in the prod bundle, ariaLabel absent from Angular 13's DOM schema, and a red-first getAttribute test that fails on current code). Architecture self-revised the post-save window 6.0→5.0 after confirming the pattern is inherited verbatim from createLanguage on master.

🤖 Generated with Claude Code

[wjames111]

[Important] **This aria-label never reaches the DOM — the round-1 a11y fix is non-functional** (UX; verified independently by Architecture, Standards, Testing, Data Integrity).

In Angular 13 an interpolated aria-label compiles to a property binding (ɵɵpropertyInterpolate1 — confirmed in dist/main.js), and aria-label is not in the DOM schema. Dev mode logs NG0303: Can't bind to 'aria-label' since it isn't a known property of 'input' (4× in every Karma run, which still exits 0 — invisible to CI); prod mode sets a JS expando, not the attribute. Either way getAttribute('aria-label') is null and the checkbox has no accessible name — a red-first test asserting the attribute was run against this code and failed. Since commit 743d77f claims this fix, the broken-but-looks-applied state is worse than no fix.

One-token fix (apply to both checkboxes):

<!-- line 71 (view mode, mandatory) -->
attr.aria-label="Force language name for {{ language.name }}"

<!-- line 112 (edit mode, recommended for consistency — use language.name, not editedLanguage.name) -->
attr.aria-label="Force language name for {{ language.name }}"

And lock it in: expect(checkbox.getAttribute('aria-label')).toContain('Force language name') — this assertion fails today and passes after the fix.

Note: findings with severity ≥ 7 cannot be dismissed via /dismiss.

[wjames111]

[Medium] **The clone refactor is unpinned by any test** (Testing; supported by all agents; self-revised from 7.5).

Mutation-verified twice: reverting this line to this.editedLanguage = language; keeps all 148 specs green — no spec references editedLanguage, and the edit row's bindings are never instantiated under test. The invariant this PR's core fix guarantees (list objects never mutated by typing; Cancel restores truth) has no automated guard, and this exact bug class already shipped once in round 1.

it('re-snapshots editedLanguage on row switch and does not mutate the original row', () => {
  const languageOne = new Language();
  languageOne.name = 'French';
  const languageTwo = new Language();
  languageTwo.name = 'German';
  comp.languages = [languageOne, languageTwo];

  comp.editLanguage(languageOne);
  comp.editedLanguage.name = 'Français'; // simulate typing
  expect(languageOne.name).toBe('French'); // clone isolation

  comp.editLanguage(languageTwo);
  expect(comp.editedLanguage.name).toBe('German'); // fresh snapshot
});

[wjames111]

[Medium] **`cancelEdit()` has zero test coverage** (Testing). Its revert contract is implicit — edits live only on the clone — which is exactly the kind of contract that silently breaks if state handling changes. One spec covers it: `editLanguage(language)` → mutate `comp.editedLanguage.name` → `cancelEdit(language)` → expect `language.isEditing === false` and `language.name` unchanged. Can be folded into the row-switch regression spec suggested on line 57.

[wjames111]

[Medium] **Post-save window: `saving` clears before this fire-and-forget reload settles** (Architecture 5.0 after self-revision; Data 4.5; UX 4.5-5.0; empirically proven with a deferred `getLanguages` stub — `saving === false`, `isEditing === true`, `loading === true` hold concurrently).

During the reload round-trip the edit row is still rendered with Save/Cancel re-enabled: a double-click sends a duplicate (idempotent) PUT, keystrokes typed in the window are discarded on swap, and if the reload rejects the row is stranded open showing "Success!" next to an error (Cancel remains a one-click escape). The reload is also the only mechanism that closes the edit row, running unobserved.

Fix (one line each): make loadLanguages() return its promise and chain it:

private loadLanguages(): Promise<void> {
  this.loading = true;
  return this.languageService.getLanguages()
    .then((languages) => (this.languages = languages))
    .catch(this.handleError.bind(this))
    .then(() => (this.loading = false));
}
// in updateLanguage's success handler:
.then(() => {
  this.showSuccess();
  return this.loadLanguages();
})

This pattern is inherited from createLanguage on master, so aligning that flow too is optional but keeps them symmetric.

[wjames111]

[Medium] **No test that the edit row actually closes after a successful save** (Testing; corroborated by Architecture and Data Integrity). Closure relies entirely on `loadLanguages()` replacing the array with fresh objects (`isEditing` undefined) — the success handler holds only the clone and cannot reach the row's flag. If the reload were removed, memoized, or failed, the row would stay open and no test would notice.

One deferred-stub spec covers this AND the post-save window finding together: stub getLanguages with a controllable promise, save, assert saving stays true until the reload resolves, then assert the refreshed list has no editing row. Recommended to land in the same commit as the loadLanguages() promise-chaining fix.