From e8b56c4cb2de8c1b89576202e0879303cf712034 Mon Sep 17 00:00:00 2001 From: Shuhao Date: Sat, 7 Nov 2026 16:18:04 -0500 Subject: [PATCH 1/5] plan(#90): Simple Format import: words in the speaker name override the role in parentheses --- .factory/plans/90-bracket-role-wins.md | 88 ++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 .factory/plans/90-bracket-role-wins.md diff --git a/.factory/plans/90-bracket-role-wins.md b/.factory/plans/90-bracket-role-wins.md new file mode 100644 index 0000000..9097beb --- /dev/null +++ b/.factory/plans/90-bracket-role-wins.md @@ -0,0 +1,88 @@ +# Simple Format import: words in the speaker name override the role in parentheses (#90) + +## Problem +In Agenda → Import Text → Simple Format, a role keyword in the speaker's name beats the +built-in role written in the brackets. `Tim Timer (Standard Speech)` imports as Short Roles, +so the timer shows 1/1:30/2 min cards instead of 5/6/7 for that speaker. + +## Findings +- The custom-role half of the issue is already fixed: since #77 (724944f, f34bdff), + `parseSimpleFormatText` matches the last bracket against custom roles first + ([simpleFormatParser.js:92](../../packages/shared/simpleFormatParser.js)). The issue's + `(Closing)` and `(Reflection)` lines import correctly when those custom roles exist. +- What is still broken: when the bracket names no custom role, the parser falls back to + `detectRoleFromText(line, names)` on the **whole line** + ([simpleFormatParser.js:93](../../packages/shared/simpleFormatParser.js)). The keyword + ladder in `detectRoleFromText` + ([timingRules.js:67-174](../../packages/shared/timingRules.js)) checks `includes()` in a + fixed order, so a name keyword earlier in the ladder wins over the bracket's role: + - `Tim Timer (Standard Speech)` → Short Roles (`timer`; exact `standard speech` only + matches when it is the whole text). + - `Dominique - General Evaluation (Table Topics)` → General Evaluation (GE is checked + before Table Topics). +- Both apps call the same shared parser (`apps/zoom-app/src/context/TimerContext.jsx:476`, + `apps/web/src/context/TimerContext.jsx:244`), so one fix covers both. +- Coverage: unit `packages/shared/__tests__/simpleFormatParser.test.js`; Playwright + `e2e/zoom.timer-flow.spec.js` "Simple Format import assigns custom roles" and + `e2e/web.agenda-flow.spec.js`; Zoom-client spec `agenda-import` (covers + `simpleFormatParser.js`; its "Not covered" lists #90 as a known open bug). + +## Approach +Detect the role from the bracket before the whole line: + +``` +role = matchCustomRole(bracket) // unchanged (#77) + ?? detectRoleFromText(bracket, names, null) // new: bracket keywords, no default + ?? detectRoleFromText(line, names) // unchanged fallback +``` + +`detectRoleFromText` gets an optional third parameter `fallback` (default +`'Standard Speech'`, today's behaviour for every existing caller). Passing `null` returns +`null` when no exact name or keyword matched, so a bracket with no role words (`(A)`, +`(Pathways L2)`) still falls back to today's whole-line detection. + +Rejected: +- Detecting from the bracket only, never the line: changes lines whose bracket holds a + duration or project and whose role is in the name. Wider blast radius than the issue. +- Stripping the name before detection: the name isn't reliably separable ("Tim Timer", + "Dominique - General Evaluation"). + +## Acceptance criteria +- AC-1: Given no custom roles, when `Tim Timer (Standard Speech)` is imported, then the + item is `{ name: 'Tim Timer', role: 'Standard Speech' }`. +- AC-2: Given no custom roles, when `Dominique - General Evaluation (Table Topics)` is + imported, then its role is `Table Topics`: the bracket's role beats an earlier keyword in + the name. +- AC-3: Given a bracket with no role words, when `Person (A)` (no custom roles) and + `Tim Timer (Pathways L2)` are imported, then today's whole-line detection still applies: + `Standard Speech` and `Short Roles`. +- AC-4: Given a custom role `Closing`, when `Tim Timer (Closing)` is imported, then its role + is `Closing` (#77's custom-first order is unchanged). +- AC-5: In the real Zoom client, pasting `Tim Timer (Standard Speech)` in Simple Format shows + that row with Standard Speech and its 5/6/7 min thresholds. + +## Test list +| AC | Unit | Playwright | Zoom-client spec | +|---|---|---|---| +| AC-1 | `simpleFormatParser.test.js`, new describe "built-in role in brackets (#90)" | `e2e/zoom.timer-flow.spec.js`, new test "Simple Format import: the bracket's built-in role beats the name (#90)" | `agenda-import` IM-05 (NEW) | +| AC-2 | same describe | same test | — | +| AC-3 | same describe | — | — | +| AC-4 | same describe | — | — | +| AC-5 | — | — | `agenda-import` IM-05 (NEW) | + +`timingRules.test.js` gets a unit test for the new `fallback` parameter during implement +(it is an implementation detail, so it is not an oracle test). + +## Risk +Low. Shared import parsing only; no `riskHigh` path. EasySpeak import and every other +`detectRoleFromText` caller keep the default fallback, so their behaviour is unchanged. + +## Open questions +- **Q1.** A bracket with no role words but a role keyword in the name + (`Tim Timer (Pathways L2)`): keep today's whole-line detection (Short Roles)? + **Default: yes**, keep it. The bracket says nothing about the role, and some agendas put + the role in the name part. +- **Q2.** A bracket keyword that is less specific than the name + (`Ice Breaker - Ana (Speaker 1)`): the bracket wins (`Speaker` → Standard Speech), as the + issue's "each item gets the role in its parentheses" asks? **Default: yes**, the bracket + wins. From 02d4ff8340c78eb8477f13f557d2dc7d9f1c58a3 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:09:30 +0000 Subject: [PATCH 2/5] =?UTF-8?q?test(#90):=20oracle=20for=20AC-1=E2=80=A6AC?= =?UTF-8?q?-5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 5.5 --- .factory/oracles/90.json | 1 + e2e/zoom-client/agenda-import.md | 22 +++++++++++++--- e2e/zoom.timer-flow.spec.js | 19 ++++++++++++++ .../__tests__/simpleFormatParser.test.js | 26 +++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 .factory/oracles/90.json diff --git a/.factory/oracles/90.json b/.factory/oracles/90.json new file mode 100644 index 0000000..21e2454 --- /dev/null +++ b/.factory/oracles/90.json @@ -0,0 +1 @@ +{ "issue": 90, "files": ["packages/shared/__tests__/simpleFormatParser.test.js", "e2e/zoom.timer-flow.spec.js"] } diff --git a/e2e/zoom-client/agenda-import.md b/e2e/zoom-client/agenda-import.md index c9af94c..44a5f8a 100644 --- a/e2e/zoom-client/agenda-import.md +++ b/e2e/zoom-client/agenda-import.md @@ -1,7 +1,7 @@ --- id: agenda-import title: Simple Format import assigns custom roles in the Zoom client -status: draft +status: stale covers: - packages/shared/simpleFormatParser.js - apps/zoom-app/src/context/TimerContext.jsx @@ -92,6 +92,21 @@ device. Evidence: page script: `toastmaster_agenda` is empty or absent, and `toastmaster_role_order` is `[]`. +### IM-05 · auto · A built-in role in the brackets beats a keyword in the name +Do: first run IM-04 (clean device). Agenda tab → Import Text → "Simple Format". Put these two +lines on the clipboard with `pbcopy`, click the textarea, paste with ⌘V, press Import. +``` +Tim Timer (Standard Speech) +Dominique - General Evaluation (Table Topics) +``` +Expect: toast "Imported 2 speaker(s)". Row "Tim Timer" shows Standard Speech and row +"Dominique - General Evaluation" shows Table Topics. Click "Tim Timer": the Live tab role +dropdown shows Standard Speech and the timing rules read Green: 5 minutes, Yellow: 6 minutes, +Red: 7 minutes (not Short Roles' 1 / 1:30 / 2). +Evidence: page script: `toastmaster_agenda` items are `Tim Timer` / `Standard Speech` and +`Dominique - General Evaluation` / `Table Topics`. Screenshot of the Live tab rules line. +Then Clear All in the Agenda tab. + ## Not covered - Custom roles that come from Club Pro presets, or from older saved rules with no role order. The PR now covers these too (`customRoleNames` comes from the dropdown, not from @@ -102,9 +117,8 @@ Evidence: page script: `toastmaster_agenda` is empty or absent, and bracket like `(Ice Breaker)` is never taken by a partial custom match). Unit tests in `simpleFormatParser.test.js` cover these, and the client adds nothing to them. - EasySpeak import. This change did not touch it, and it has no spec yet. -- `(Table Topics Speech)` imports as a `Table Topics` role with no timing rules (#91). A - built-in keyword in the name overrides a built-in role in the bracket (#90). Both are - known open bugs. No step locks in either result. +- `(Table Topics Speech)` imports as a `Table Topics` role with no timing rules (#91), a + known open bug. No step locks in that result. IM-05 covers #90. - The web app (`apps/web`) runs the same shared parser. Unit tests cover it in `apps/web/src/context/TimerContext.test.jsx`, and `e2e/web.agenda-flow.spec.js` runs this import (IM-01, IM-02) in a plain browser. diff --git a/e2e/zoom.timer-flow.spec.js b/e2e/zoom.timer-flow.spec.js index 7e7a696..c434d3d 100644 --- a/e2e/zoom.timer-flow.spec.js +++ b/e2e/zoom.timer-flow.spec.js @@ -182,6 +182,25 @@ test.describe('Zoom App — Functional Checks', () => { await expect(page.getByText('Timing rules: Green: 15 seconds, Yellow: 30 seconds, Red: 45 seconds')).toBeVisible(); }); + test("Simple Format import: the bracket's built-in role beats the name (#90)", async ({ page }) => { + await page.goto('/zoom/'); + + await page.getByRole('button', { name: /AGENDA/i }).click(); + await page.getByRole('button', { name: /Import Text/i }).click(); + await page.getByRole('button', { name: 'Simple Format' }).click(); + await page + .getByPlaceholder('John Doe (Ice Breaker)') + .fill('Tim Timer (Standard Speech)\nDominique - General Evaluation (Table Topics)'); + await page.getByRole('button', { name: 'Import', exact: true }).click(); + + await expect(page.getByText('Imported 2 speaker(s)')).toBeVisible(); + + const rows = page.locator('.cursor-pointer').filter({ has: page.getByRole('button', { name: /^Edit / }) }); + await expect(rows).toHaveCount(2); + await expect(rows.locator('.font-medium')).toHaveText(['Tim Timer', 'Dominique - General Evaluation']); + await expect(rows.locator('.text-sm')).toHaveText(['(Standard Speech)', '(Table Topics)']); + }); + test('no console errors about missing context providers', async ({ page }) => { await page.goto('/zoom/'); await page.waitForTimeout(1000); diff --git a/packages/shared/__tests__/simpleFormatParser.test.js b/packages/shared/__tests__/simpleFormatParser.test.js index ba41a87..f889e70 100644 --- a/packages/shared/__tests__/simpleFormatParser.test.js +++ b/packages/shared/__tests__/simpleFormatParser.test.js @@ -117,6 +117,32 @@ describe('parseSimpleFormatText: PRD worked examples', () => { }); }); +describe('parseSimpleFormatText: built-in role in brackets (#90)', () => { + it('AC-1: the bracket role beats a keyword in the name ("Tim Timer (Standard Speech)")', () => { + expect(parseSimpleFormatText('Tim Timer (Standard Speech)', [])).toEqual([ + { name: 'Tim Timer', role: 'Standard Speech' }, + ]); + }); + + it('AC-2: the bracket role beats an earlier keyword in the name ("General Evaluation (Table Topics)")', () => { + const [item] = parseSimpleFormatText('Dominique - General Evaluation (Table Topics)', []); + expect(item.role).toBe('Table Topics'); + }); + + it('AC-3: a bracket with no role words keeps whole-line detection', () => { + expect(parseSimpleFormatText('Person (A)', [])).toEqual([{ name: 'Person', role: 'Standard Speech' }]); + expect(parseSimpleFormatText('Tim Timer (Pathways L2)', [])).toEqual([ + { name: 'Tim Timer', role: 'Short Roles' }, + ]); + }); + + it('AC-4: a custom role in the bracket still wins ("Tim Timer (Closing)")', () => { + expect(parseSimpleFormatText('Tim Timer (Closing)', ['Closing'])).toEqual([ + { name: 'Tim Timer', role: 'Closing' }, + ]); + }); +}); + describe('matchCustomRole', () => { it('matches ignoring case, spacing and punctuation', () => { expect(matchCustomRole(' opening REMARKS ', ['Opening Remarks'])).toBe('Opening Remarks'); From c3a0f78e8599559d9fcb0137f1ad159122d6320c Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:10:48 +0000 Subject: [PATCH 3/5] fix(import): the bracket's built-in role beats keywords in the speaker name Simple Format detected the role from the whole line, so 'Tim Timer (Standard Speech)' became Short Roles. Try the bracket's own keywords first, via a new optional fallback on detectRoleFromText (default unchanged for other callers), then fall back to whole-line detection as before. Co-Authored-By: Claude Sonnet 5.5 --- packages/shared/__tests__/timingRules.test.js | 10 ++++++++++ packages/shared/simpleFormatParser.js | 6 +++++- packages/shared/timingRules.js | 6 +++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/packages/shared/__tests__/timingRules.test.js b/packages/shared/__tests__/timingRules.test.js index 7f27437..5841811 100644 --- a/packages/shared/__tests__/timingRules.test.js +++ b/packages/shared/__tests__/timingRules.test.js @@ -121,6 +121,16 @@ describe('getDefaultGraceAfterRed', () => { }); describe('detectRoleFromText', () => { + describe('fallback parameter', () => { + it('returns null instead of Standard Speech when fallback is null and nothing matches', () => { + expect(detectRoleFromText('Pathways L2', null, null)).toBeNull(); + }); + + it('still returns a matched role when fallback is null', () => { + expect(detectRoleFromText('Table Topics', null, null)).toBe('Table Topics'); + }); + }); + describe('exact case-insensitive matches', () => { it('maps "standard speech" to Standard Speech', () => { expect(detectRoleFromText('standard speech')).toBe('Standard Speech'); diff --git a/packages/shared/simpleFormatParser.js b/packages/shared/simpleFormatParser.js index f95a243..a9b7184 100644 --- a/packages/shared/simpleFormatParser.js +++ b/packages/shared/simpleFormatParser.js @@ -89,7 +89,11 @@ export function parseSimpleFormatText(text, customRoleNames = []) { const groups = [...line.matchAll(BRACKET_GROUP)]; const bracket = groups.length > 0 ? groups[groups.length - 1][1] : ''; const customRole = bracket ? matchCustomRole(bracket, names) : null; - const role = customRole ?? detectRoleFromText(line, names); + // Bracket first (custom, then built-in keywords) so a keyword in the name can't beat it (#90) + const role = + customRole ?? + (bracket ? detectRoleFromText(bracket, names, null) : null) ?? + detectRoleFromText(line, names); return { name, role }; }); } diff --git a/packages/shared/timingRules.js b/packages/shared/timingRules.js index 887f71e..6576e5a 100644 --- a/packages/shared/timingRules.js +++ b/packages/shared/timingRules.js @@ -66,7 +66,8 @@ export function getDefaultGraceAfterRed(role) { // Helper to detect role from text // customRoleNames: optional array of user-added role names; exact match (case-insensitive) is tried first -export function detectRoleFromText(text, customRoleNames = null) { +// fallback: role returned when nothing matched; pass null to get null instead (caller has its own next step) +export function detectRoleFromText(text, customRoleNames = null, fallback = 'Standard Speech') { const normalized = text.toLowerCase().trim(); if (customRoleNames && customRoleNames.length > 0) { @@ -169,6 +170,5 @@ export function detectRoleFromText(text, customRoleNames = null) { return 'Speech Evaluation'; } - // Default fallback - return 'Standard Speech'; + return fallback; } From 0ba1f1ccf2576d6088e6a92b410ed2ac8505e826 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:10:58 +0000 Subject: [PATCH 4/5] docs(specs): agenda-import notes reviewed against the #90 fix Co-Authored-By: Claude Sonnet 5.5 --- e2e/zoom-client/agenda-import.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/zoom-client/agenda-import.md b/e2e/zoom-client/agenda-import.md index 44a5f8a..3b726b6 100644 --- a/e2e/zoom-client/agenda-import.md +++ b/e2e/zoom-client/agenda-import.md @@ -118,7 +118,7 @@ Then Clear All in the Agenda tab. `simpleFormatParser.test.js` cover these, and the client adds nothing to them. - EasySpeak import. This change did not touch it, and it has no spec yet. - `(Table Topics Speech)` imports as a `Table Topics` role with no timing rules (#91), a - known open bug. No step locks in that result. IM-05 covers #90. + known open bug. No step locks in that result. IM-05 covers #90 (fixed: the bracket's built-in role is tried before the whole line). - The web app (`apps/web`) runs the same shared parser. Unit tests cover it in `apps/web/src/context/TimerContext.test.jsx`, and `e2e/web.agenda-flow.spec.js` runs this import (IM-01, IM-02) in a plain browser. From cf14fc54157567c7044ac49b8bf40397ff5fb454 Mon Sep 17 00:00:00 2001 From: Shuhao Date: Wed, 11 Nov 2026 16:52:00 -0500 Subject: [PATCH 5/5] test(zoom-client): verify agenda-import on 0ba1f1c (#90) Real Zoom run 2026-11-11: IM-01 to IM-04 pass, IM-05 import passes. IM-05's Live-tab check was blocked by a device that hides built-in roles; add that precondition to needs and note the fallback under Not covered. --- e2e/zoom-client/INDEX.md | 2 +- e2e/zoom-client/agenda-import.md | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/e2e/zoom-client/INDEX.md b/e2e/zoom-client/INDEX.md index 65f0f53..b6c25ee 100644 --- a/e2e/zoom-client/INDEX.md +++ b/e2e/zoom-client/INDEX.md @@ -5,7 +5,7 @@ Read this first. Update it in the same PR as any spec change. | Spec | Status | Verified | Covers | |---|---|---|---| | [timer-core](timer-core.md) | draft | never | start/colours/finish, resume after close | -| [agenda-import](agenda-import.md) | draft | never | Simple Format import assigns custom roles from the bracket text; imported role's thresholds on real video (#77) | +| [agenda-import](agenda-import.md) | current | 0ba1f1c, 2026-11-11 | Simple Format import assigns custom roles from the bracket text; imported role's thresholds on real video (#77) | | [zoom-deeplink](zoom-deeplink.md) | draft | never | "Open in Zoom" deeplink resolves and launches this build's app (dev vs production client id) | | auth-identity | **not migrated** | — | source: `docs/ZOOM_TEST_PLAN.md` Steps 1, 1b | | [connection-notice](connection-notice.md) | draft | never | guest-mode banner and "Approve Toastmusters Timer in Zoom" modal at open; raised mid-session when Zoom drops the grant, modal held until the timer stops (#85) | diff --git a/e2e/zoom-client/agenda-import.md b/e2e/zoom-client/agenda-import.md index 3b726b6..249d533 100644 --- a/e2e/zoom-client/agenda-import.md +++ b/e2e/zoom-client/agenda-import.md @@ -1,7 +1,7 @@ --- id: agenda-import title: Simple Format import assigns custom roles in the Zoom client -status: stale +status: current covers: - packages/shared/simpleFormatParser.js - apps/zoom-app/src/context/TimerContext.jsx @@ -15,8 +15,9 @@ needs: - camera on, in a meeting - club presets not live on this device (not in a club, or "Use mine" in Edit Rules) - no custom roles on the device before IM-01 (note and remove any in the run log) -verified_at: never -verified_on: never + - built-in roles not hidden on this device (Standard Speech and Table Topics in the role dropdown), or IM-05's Live-tab check falls back to the first listed role +verified_at: 0ba1f1c +verified_on: 2026-11-11 (Zoom 7.2.5, macOS 26.7.2, dev c80dca2b) --- # Simple Format import assigns custom roles in the Zoom client @@ -106,6 +107,10 @@ Red: 7 minutes (not Short Roles' 1 / 1:30 / 2). Evidence: page script: `toastmaster_agenda` items are `Tim Timer` / `Standard Speech` and `Dominique - General Evaluation` / `Table Topics`. Screenshot of the Live tab rules line. Then Clear All in the Agenda tab. +Run 2026-11-11 (0ba1f1c): the import half passed on the real client. The Live-tab half could +not be checked: that device hid the built-in roles, so the dropdown had no Standard Speech and +`LiveTab.jsx` moved the row to the first listed role (ChairToast1, 0:30/0:45/1:00). See the +new `needs` line. ## Not covered - Custom roles that come from Club Pro presets, or from older saved rules with no role @@ -116,6 +121,9 @@ Then Clear All in the Agenda tab. - Tie-breaks between custom roles, whole-word-only matching, and the built-in guard (a bracket like `(Ice Breaker)` is never taken by a partial custom match). Unit tests in `simpleFormatParser.test.js` cover these, and the client adds nothing to them. +- An agenda row whose role is a hidden built-in: selecting it times the speaker with the + first listed role, with no notice, while the row keeps the original role (seen 2026-11-11; + behaviour from e876ca8, not this spec's change). No step locks it in. - EasySpeak import. This change did not touch it, and it has no spec yet. - `(Table Topics Speech)` imports as a `Table Topics` role with no timing rules (#91), a known open bug. No step locks in that result. IM-05 covers #90 (fixed: the bracket's built-in role is tried before the whole line).