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/.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. diff --git a/e2e/zoom-client/INDEX.md b/e2e/zoom-client/INDEX.md index ce9125d..ae275b2 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) | current | 2e965b6, 2026-11-11 | start/colours/finish, resume after close; `audience_size` on `timer_started` for hosts, omitted for non-hosts (#107) | -| [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 c9af94c..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: draft +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 @@ -92,6 +93,25 @@ 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. +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 order. The PR now covers these too (`customRoleNames` comes from the dropdown, not from @@ -101,10 +121,12 @@ Evidence: page script: `toastmaster_agenda` is empty or absent, and - 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 - 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 (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. 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'); 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; }