From 81f9ed96689bdf40fb5df0e2dff8c3a7e898393e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 18:37:39 +0000 Subject: [PATCH 1/2] fix(console): search the Applications page through the row's own label resolver (#4343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AppManagementPage`'s filter read `(app.label || '').toLowerCase()`. `label` and `description` are `I18nLabel` in `AppSchema` (`string | Record` in @objectstack/spec 17.0.0-rc.6), so a non-string label is spec-legal metadata — and an object is truthy, so the `|| ''` guard never fired and `.toLowerCase()` got the object. The TypeError was thrown inside `filter` during render, taking the page out rather than degrading search, and only once someone typed: `if (!searchQuery) return true` short-circuits the empty case. Both reads now go through the resolver the rows already render with — `appTitle` for the label (the one display-name helper #4307 introduced in this file) and the identical `resolveKeyedI18nLabel(…, t)` call the description paragraph makes — so search matches what the operator can see, and a future widening of the resolver reaches display and search together. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../app-management-search-keyed-label-4343.md | 19 ++ .../src/pages/system/AppManagementPage.tsx | 31 +- .../AppManagementPage.search.test.tsx | 271 ++++++++++++++++++ 3 files changed, 318 insertions(+), 3 deletions(-) create mode 100644 .changeset/app-management-search-keyed-label-4343.md create mode 100644 apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx diff --git a/.changeset/app-management-search-keyed-label-4343.md b/.changeset/app-management-search-keyed-label-4343.md new file mode 100644 index 000000000..62acaa362 --- /dev/null +++ b/.changeset/app-management-search-keyed-label-4343.md @@ -0,0 +1,19 @@ +--- +'@object-ui/console': patch +--- + +The Applications page's search box no longer takes the page out on the first keystroke when an app carries a non-string label + +`apps/console/src/pages/system/AppManagementPage.tsx` filtered on `(app.label || '').toLowerCase()`. `label` and `description` are `I18nLabel` in `AppSchema` — `string | Record< string, string >` in `@objectstack/spec` 17.0.0-rc.6 — so an authored non-string label is spec-legal metadata, and an object is **truthy**: the `|| ''` guard never fired for one, and `.toLowerCase()` received the object. + +``` +TypeError: (l || "").toLowerCase is not a function +``` + +That throw happened inside `filter` **during render**, so it took the whole page down rather than degrading search. It stayed invisible until someone typed, because `if (!searchQuery) return true` returns before either read — the page mounted perfectly with the very metadata that killed it one character later. + +Both reads now go through the resolver the rows already render with: `appTitle` (the single display-name helper objectui#4307 introduced in this file) for the label, and the identical `resolveKeyedI18nLabel(…, t)` call the description paragraph makes. This is a repair to one page's filter, not a new capability — but it does make search match what the operator can actually see: for objectui's keyed label form it now matches the pack's answer rather than the authoring `defaultValue`, and it matches `app.name` wherever the row heading itself falls back to it. + +Routing search through the render path also means it cannot drift out of step again. The inline locale map form (`{ en: 'Storefront', 'zh-CN': '店面' }`) is still resolved by neither path — the row heading falls back to the app name and search now matches on exactly that, instead of crashing on it — so when objectui#4163 widens the resolver, display and search gain the map form in the same commit. + +No first-party app ships a non-string app label today, so this was reachable through authored metadata rather than live in the shipped examples; the crash is real for anyone who authored one, and `AppSchema` accepts it with a green parse. diff --git a/apps/console/src/pages/system/AppManagementPage.tsx b/apps/console/src/pages/system/AppManagementPage.tsx index 5ae57899b..1c5528970 100644 --- a/apps/console/src/pages/system/AppManagementPage.tsx +++ b/apps/console/src/pages/system/AppManagementPage.tsx @@ -138,14 +138,39 @@ export function AppManagementPage() { const [confirmDelete, setConfirmDelete] = useState(null); const [processing, setProcessing] = useState(false); - // Filter apps by search query + /** + * Filter apps by search query — through the SAME resolver the rows render + * with (objectui#4343). + * + * `label` and `description` are `I18nLabel` in `AppSchema` + * (`string | Record< string, string >` in `@objectstack/spec` 17.0.0-rc.6), so + * an authored non-string label is spec-legal. An object is TRUTHY, so the + * `(app.label || '')` guard this filter used to carry never fired for one — it + * handed the object straight to `.toLowerCase()`: + * + * TypeError: (l || "").toLowerCase is not a function + * + * thrown inside `filter` **during render**, so it took the whole page out + * rather than degrading search. And only on the first keystroke, because + * `if (!searchQuery) return true` short-circuits the empty case — the page + * loaded fine and died the moment anyone used it. + * + * The fix is not a wider guard, it is the SAME reading the rows already use: + * `appTitle` (the one display-name helper #4307 introduced) and the identical + * `resolveKeyedI18nLabel(…, t)` call the description `< p >` makes below. So + * search now matches what the operator can actually SEE — the pack's answer + * for a keyed label rather than its authoring `defaultValue`, and `app.name` + * wherever the row heading itself falls back to it — and any future widening + * of the resolver (objectui#4163's inline-locale-map form) reaches search and + * display in the same commit, instead of leaving this filter behind again. + */ const filteredApps = (apps || []).filter((app: any) => { if (!searchQuery) return true; const q = searchQuery.toLowerCase(); return ( (app.name || '').toLowerCase().includes(q) || - (app.label || '').toLowerCase().includes(q) || - (app.description || '').toLowerCase().includes(q) + appTitle(app).toLowerCase().includes(q) || + (resolveKeyedI18nLabel(app.description, t) || '').toLowerCase().includes(q) ); }); diff --git a/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx b/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx new file mode 100644 index 000000000..f9be9ef4d --- /dev/null +++ b/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx @@ -0,0 +1,271 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#4343 — the Applications page's SEARCH filter reads the same fields + * the rows render, and must read them the same way. + * + * Filed out of #4307 (PR #4344), which keyed this page's chrome and introduced + * the `appTitle` helper the rows name an app by. The filter one screen above it + * was left on a raw read: + * + * (app.label || '').toLowerCase().includes(q) + * + * `label` and `description` are `I18nLabel` in `AppSchema` — `string | + * Record` in `@objectstack/spec` 17.0.0-rc.6 — so a non-string + * label is spec-legal metadata. An object is TRUTHY, so `|| ''` never fired for + * one and `.toLowerCase()` got the object: + * + * TypeError: (l || "").toLowerCase is not a function + * + * thrown inside `filter` during render, which takes the page out instead of + * degrading search — and only once someone types, because `if (!searchQuery) + * return true` short-circuits the empty case. + * + * ## What these cases pin, and why the mock answers in a real language + * + * The sister i18n file mocks `t` to answer in no natural language at all, which + * is right for proving the chrome asks the pack. It cannot ask THIS file's + * question, though: search is judged by what an operator can SEE, so the pack + * here answers with real words that deliberately share no substring with the + * call site's own `defaultValue` (`app.crm.label` -> `Vertrieb`, never `CRM`). + * That makes every keyed case two-sided — the resolved text matches, and the + * authoring `defaultValue` does NOT — which is what pins the `t` argument + * rather than merely pinning that some resolver was called. + * + * ## Reverse verification (direction predicted before running) + * + * Restoring the two raw reads turns every keyed and map case RED **by the + * TypeError above**, thrown on the first keystroke, and leaves the plain-string + * control GREEN — that asymmetry is the whole claim, and it is why the control + * case is here at all. It is also why `toThrow`-style coverage would be + * worthless: the defect is not "something throws", it is that one truthy shape + * reaches a string method. + */ + +import { describe, expect, it, vi, beforeEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router-dom'; + +const { refresh, apps } = vi.hoisted(() => ({ + refresh: vi.fn(async () => {}), + apps: { value: [] as any[] }, +})); + +vi.mock('sonner', () => ({ + toast: { success: vi.fn(), error: vi.fn() }, +})); + +vi.mock('@object-ui/app-shell', () => ({ + useMetadata: () => ({ apps: apps.value, refresh }), + useAdapter: () => ({ getClient: () => ({ meta: { saveItem: vi.fn(), deleteItem: vi.fn() } }) }), +})); + +/** + * A pack that answers these two keys in another language, and echoes the call + * site's `defaultValue` for everything else. + * + * The echo keeps the page's own chrome in English so the search box is still + * addressable by its testid and the rows still read naturally; the two real + * answers are what make "matched the pack, not the `defaultValue`" a + * distinguishable outcome. Neither answer contains `crm` — the substring the + * authoring `defaultValue` would have offered. + */ +const PACK: Record = { + 'app.crm.label': 'Vertrieb', + 'app.crm.description': 'Kundenverwaltung', +}; + +vi.mock('@object-ui/i18n', async (importOriginal) => ({ + // Partial, via `importOriginal`: `@object-ui/components` builds its own + // `createSafeTranslation` probes at module scope. + ...(await importOriginal>()), + useObjectTranslation: () => ({ + t: (key: string, options?: Record) => { + // i18next returns the empty string for a null/undefined key + // (`translate()`: `if (keys == null) return '';`, i18next 26.3.6). That + // is not a detail worth inventing a behaviour for — it is exactly the + // call `resolveKeyedI18nLabel` makes when handed an object with no `key`, + // which is the INLINE LOCALE MAP case below. Reproducing it is what lets + // that case describe the real page instead of this mock. + if (key == null) return ''; + return ( + PACK[key] ?? + String(options?.defaultValue ?? key).replace(/\{\{(\w+)\}\}/g, (_m, name: string) => + String(options?.[name] ?? ''), + ) + ); + }, + }), +})); + +const { AppManagementPage } = await import('../AppManagementPage'); + +/** + * Plain-string labels — the shape every first-party app ships today (`crm`, + * `showcase`, `todo`, `setup`, `account`, `studio` are all strings). `WEB`'s + * label and description are deliberately unreachable from its `name`, so the + * control below can exercise the two terms this card changed in isolation. + */ +const OPS = { name: 'ops_app', label: 'Ops', description: 'Billing and invoices', active: true }; +const WEB = { name: 'web_app', label: 'Marketing Site', description: 'Public brochure', active: true }; + +/** + * objectui's KEYED form. `name` deliberately carries no `crm`, so a match on + * the authoring `defaultValue` cannot be smuggled in through the name term. + */ +const KEYED = { + name: 'sales_app', + label: { key: 'app.crm.label', defaultValue: 'CRM' }, + active: true, +}; + +/** + * The spec's INLINE LOCALE MAP form — `string | Record` is what + * `AppSchema.label` accepts today, so this object is what a partner authoring + * `defineApp` can put on the wire with a green parse. It is the shape that + * makes this bug reachable rather than hypothetical. + */ +const MAPPED = { + name: 'mapped_app', + label: { en: 'Storefront', 'zh-CN': '店面' }, + active: true, +}; + +function renderPage() { + return render( + + + , + ); +} + +/** Type into the search box — the keystroke the page used to die on. */ +async function search(text: string) { + const user = userEvent.setup(); + await user.type(screen.getByTestId('app-search-input'), text); +} + +beforeEach(() => { + vi.clearAllMocks(); + apps.value = []; +}); + +describe('a non-string label survives the first keystroke', () => { + it('filters a KEYED label on the text the pack returned', async () => { + apps.value = [KEYED, OPS]; + renderPage(); + + // Before this card: `TypeError: (l || "").toLowerCase is not a function`, + // thrown out of `filter` during the re-render this keystroke causes. + await search('vertr'); + + expect(screen.getByTestId('app-card-sales_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + }); + + it('matches the RESOLVED label, not the authoring `defaultValue`', async () => { + // The other side of the same claim: `t` must actually be handed to the + // resolver. Drop the second argument and the label resolves to its + // authoring `CRM` instead of the pack's `Vertrieb`, which is a string — + // so the page would NOT crash, and only this case would notice. + apps.value = [KEYED, OPS]; + renderPage(); + + await search('crm'); + + expect(screen.queryByTestId('app-card-sales_app')).toBeNull(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + expect(screen.getByTestId('no-apps-message')).toBeInTheDocument(); + }); + + it('filters a KEYED description the same way — the line below the defect', async () => { + apps.value = [ + { name: 'sales_app', label: 'Sales', description: { key: 'app.crm.description' }, active: true }, + OPS, + ]; + renderPage(); + + await search('kunden'); + + expect(screen.getByTestId('app-card-sales_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + }); + + it('survives the spec-legal INLINE LOCALE MAP form, and agrees with the row heading', async () => { + apps.value = [MAPPED, OPS]; + renderPage(); + + // The row heading falls back to the app's `name`, because + // `resolveKeyedI18nLabel` speaks the keyed form and not the map form + // (objectui#4163 is the audit that widens it). That limitation is the + // DISPLAY path's, and the point of this case is that search now shares it + // exactly instead of crashing on the same input: what the heading shows is + // what the filter matches. When #4163 teaches the resolver the map form, + // both assertions below move together rather than drifting apart again. + expect(screen.getByText('mapped_app')).toBeInTheDocument(); + + await search('mapped'); + + expect(screen.getByTestId('app-card-mapped_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + }); + + it('loads every app fine with an EMPTY query — which is why this hid until a keystroke', () => { + // Green before this card too, and deliberately so: `if (!searchQuery) + // return true` returns before either read, so the page mounted perfectly + // with the very metadata that killed it one character later. Pinning the + // short-circuit keeps that asymmetry from being "fixed" away. + apps.value = [OPS, KEYED, MAPPED]; + renderPage(); + + expect(screen.getByTestId('app-card-ops_app')).toBeInTheDocument(); + expect(screen.getByTestId('app-card-sales_app')).toBeInTheDocument(); + expect(screen.getByTestId('app-card-mapped_app')).toBeInTheDocument(); + }); +}); + +describe('plain-string labels filter exactly as before', () => { + // The control, and it holds NO non-string label on purpose: one keyed app + // anywhere in `apps` would take the whole filter down pre-change and this + // describe would go red with the others, which is exactly the asymmetry it + // exists to demonstrate. Every case here is GREEN before and after. + it('still matches on name, on label alone, and on description alone', async () => { + apps.value = [OPS, WEB]; + + const { unmount } = renderPage(); + await search('ops'); + expect(screen.getByTestId('app-card-ops_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-web_app')).toBeNull(); + unmount(); + + // The LABEL term on its own — `Marketing Site` is reachable from neither + // `web_app` nor `Public brochure`, so this is the line the card rewrote. + const second = renderPage(); + await search('marketing'); + expect(screen.getByTestId('app-card-web_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + second.unmount(); + + // The DESCRIPTION term on its own — the other rewritten line. + const third = renderPage(); + await search('invoic'); + expect(screen.getByTestId('app-card-ops_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-web_app')).toBeNull(); + third.unmount(); + + renderPage(); + await search('zzz'); + expect(screen.getByTestId('no-apps-message')).toBeInTheDocument(); + }); + + it('is case-insensitive, as it always was', async () => { + apps.value = [OPS, WEB]; + renderPage(); + + await search('MARKETING'); + + expect(screen.getByTestId('app-card-web_app')).toBeInTheDocument(); + expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); + }); +}); From 4bcdbd13bd0697c45cb202ce059453887703b54b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 18:56:12 +0000 Subject: [PATCH 2/2] test(console): make the inline-locale-map case reach the defect (#4343) Reverse verification predicted four red cases and produced three. The filter is a chain of `||`, so a query the NAME satisfies short-circuits before the label is read: the map case searched `mapped` against `mapped_app` and passed on the UNFIXED page, never reaching the throwing term. It now searches a query that misses every app, which is the only way that term is evaluated, and the header records the rule so the next case written here does not repeat it. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../AppManagementPage.search.test.tsx | 39 ++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx b/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx index f9be9ef4d..635df61af 100644 --- a/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx +++ b/apps/console/src/pages/system/__tests__/AppManagementPage.search.test.tsx @@ -34,12 +34,21 @@ * * ## Reverse verification (direction predicted before running) * - * Restoring the two raw reads turns every keyed and map case RED **by the + * Restoring the two raw reads turns the keyed and map cases RED **by the * TypeError above**, thrown on the first keystroke, and leaves the plain-string * control GREEN — that asymmetry is the whole claim, and it is why the control * case is here at all. It is also why `toThrow`-style coverage would be * worthless: the defect is not "something throws", it is that one truthy shape * reaches a string method. + * + * ⚠️ The prediction was wrong the first time, and the reason is worth keeping. + * Four cases were expected red; three came back red. The filter is a chain of + * `||`, so **a query the NAME already satisfies short-circuits before the label + * is read** — the map case searched `mapped` against `mapped_app` and passed on + * the UNFIXED page, having never reached the throwing term. Every red-first + * case in this file therefore hands the filter a query no earlier term can + * answer (`vertr`, `crm`, `kunden` against `sales_app`; `zzz` against + * everything). A case that matches on the name is testing the name. */ import { describe, expect, it, vi, beforeEach } from 'vitest'; @@ -192,21 +201,33 @@ describe('a non-string label survives the first keystroke', () => { expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); }); - it('survives the spec-legal INLINE LOCALE MAP form, and agrees with the row heading', async () => { + it('survives the spec-legal INLINE LOCALE MAP form on a query that MISSES', async () => { apps.value = [MAPPED, OPS]; - renderPage(); + const { unmount } = renderPage(); // The row heading falls back to the app's `name`, because // `resolveKeyedI18nLabel` speaks the keyed form and not the map form - // (objectui#4163 is the audit that widens it). That limitation is the - // DISPLAY path's, and the point of this case is that search now shares it - // exactly instead of crashing on the same input: what the heading shows is - // what the filter matches. When #4163 teaches the resolver the map form, - // both assertions below move together rather than drifting apart again. + // (objectui#4163 is the audit that widens it). That limitation belongs to + // the DISPLAY path; what this case pins is that search now shares it + // exactly instead of crashing on the same input. expect(screen.getByText('mapped_app')).toBeInTheDocument(); - await search('mapped'); + // ⚠️ The query must MISS every app, and that is load-bearing rather than + // incidental: the filter is a chain of `||`, so a query the NAME satisfies + // short-circuits before the label is ever read. Searching `mapped` here + // passes even on the unfixed page — `'mapped_app'.includes('mapped')` is + // true and the throwing term never runs. A red-first case for this filter + // has to hand it a query no earlier term can answer. + await search('zzz'); + expect(screen.getByTestId('no-apps-message')).toBeInTheDocument(); + unmount(); + + // …and the parity half, on a query that DOES hit: what the heading shows + // is what the filter matches. When #4163 teaches the resolver the map + // form, both halves move together instead of drifting apart again. + renderPage(); + await search('mapped'); expect(screen.getByTestId('app-card-mapped_app')).toBeInTheDocument(); expect(screen.queryByTestId('app-card-ops_app')).toBeNull(); });