From 6d94b885138760cdde115b8d6ac3c06573b3df76 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Fri, 13 Nov 2026 15:56:40 +0000 Subject: [PATCH 1/5] plan(#91): Simple Format import: "(Table Topics Speech)" imports as 'Table Topics' with no timing rules Co-Authored-By: Claude Opus 5.5 --- .factory/plans/91-table-topics-role.md | 134 +++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 .factory/plans/91-table-topics-role.md diff --git a/.factory/plans/91-table-topics-role.md b/.factory/plans/91-table-topics-role.md new file mode 100644 index 0000000..5228460 --- /dev/null +++ b/.factory/plans/91-table-topics-role.md @@ -0,0 +1,134 @@ +# Simple Format import: "(Table Topics Speech)" imports as 'Table Topics' with no timing rules (#91) + +## Problem +In Agenda → Import Text → Simple Format, any line whose role says "Table Topics" imports with +role `Table Topics`. That is not a rules key, so the row gets Standard Speech's 5/6/7 min +rules, the Live tab dropdown has no matching option, and START shows "Please set timing rules +first". It affects the demo file's `(Table Topics Speech)` and the Zoom placeholder's +`(Table Topics)`. + +## Findings +- Reproduced on `origin/dev` (a4439d6) with the shared parser and no custom roles: + `David Kim (Table Topics Speech)`, `Alex (Table Topics)`, `Sam (Table Topics Master)`, + `Jo (Table Topics Evaluator)` and the bare line `Table Topics` all return role + `Table Topics`, and `'Table Topics' in DEFAULT_ROLE_RULES` is `false`. +- Cause: `detectRoleFromText` in + [timingRules.js](../../packages/shared/timingRules.js) returns a role that is not a key: + - `:82` exact `table topics` → `'Table Topics'` + - `:113-114` any text containing `table topics` → `'Table Topics'` + - `DEFAULT_ROLE_RULES` (`:3-12`) only has `'Table Topics Speech'` and `'Table Topics Evaluation'`. +- Same ladder, same bug: `table topics master` (`:155`, short roles) and + `table topics evaluator` can't be reached because `:113` catches them first. `evaluation` + is matched at `:90`, but `evaluator` isn't, and the evaluator branch at `:144` skips text + that contains `table topics`. +- Since #90 (c3a0f78), `parseSimpleFormatText` + ([simpleFormatParser.js:93-96](../../packages/shared/simpleFormatParser.js)) tries custom + roles, then `detectRoleFromText(bracket, names, null)`, then the whole line. The bracket + `Table Topics Speech` doesn't exactly match a built-in name in `detectRoleFromText`, so it + reaches `:113`. `parseSimpleFormatText` is the only caller of `detectRoleFromText`. + EasySpeak import uses its own mapping (`easySpeakParser.js`). +- Downstream, both apps do `rules: roleRules[role] || DEFAULT_ROLE_RULES['Standard Speech']` + (`apps/web/src/context/TimerContext.jsx:244-249`, `apps/zoom-app/src/context/TimerContext.jsx:476-481`). + `LiveTab.handleStart` re-reads `roleRules[selectedRole]` and rejects + (`apps/web/src/components/LiveTab.jsx:205`, `apps/zoom-app/src/components/LiveTab.jsx:614-630`). + With a valid key from the parser, these work unchanged. +- Tests that currently assert the bug: + - `packages/shared/__tests__/timingRules.test.js:130` (`'Table Topics'` with null fallback) + and `:209-215` (`table topics master` → `Table Topics`, with a comment explaining why). + - `packages/shared/__tests__/simpleFormatParser.test.js:91-93`: the demo-file test expects + `Table Topics`. + - `packages/shared/__tests__/simpleFormatParser.test.js:127-130` (#90 AC-2) expects + `Table Topics`. + - `e2e/zoom.timer-flow.spec.js:186-202` (#90) expects `(Table Topics)`. + - Zoom-client spec `e2e/zoom-client/agenda-import.md` IM-05 expects the row and + `toastmaster_agenda` to show `Table Topics`. "Not covered" lists #91 as a known open bug. + - The two #90 test files are listed in `.factory/oracles/90.json`. The oracle guard and + `check-oracle.mjs` only lock oracles of the branch's own issue, so #91 may update them. + The #90 point still holds: the bracket beats the name's `General Evaluation`. Only the + role string it lands on changes. +- Spec coverage: `agenda-import` covers `simpleFormatParser.js`, but no spec covers + `timingRules.js`. Without a `covers` entry, `stop-gate.mjs --hits` finds nothing for this + diff and the Zoom station would skip the real-client run. + +## Approach +Fix the ladder in `detectRoleFromText` so every "table topics" text maps to a real key. +- Exact match: `table topics` → `'Table Topics Speech'`; add `table topics speech` → + `'Table Topics Speech'`. +- Partial matches, in this order, replacing `:112-115`: + 1. `table topics evaluator` → `'Table Topics Evaluation'` (`evaluation` is already at `:90`) + 2. `table topics master` → `'Short Roles'` (the role the dead branch at `:155` meant) + 3. `table topics` → `'Table Topics Speech'` +- Remove the dead `table topics master` entry at `:155`, and the now-redundant + `!normalized.includes('table topics')` guard at `:144` if the tests allow. +- No change to `simpleFormatParser.js`, the TimerContexts or the LiveTabs. +- Update the tests that assert the old result (listed above) and the `agenda-import` spec: + - IM-05 now expects `Table Topics Speech`. + - New step IM-06 covers the `(Table Topics Speech)` / `(Table Topics)` lines and START. + - Add `packages/shared/timingRules.js` to `covers`. + - Remove the #91 line from "Not covered". + +Rejected: +- Alias `'Table Topics'` → `'Table Topics Speech'` in `importBulkSpeakers` and `handleStart` + in both apps. That fixes the symptom in four places and leaves the parser returning a + non-key. +- Add a `'Table Topics'` key to `DEFAULT_ROLE_RULES`. That makes a duplicate role in the + dropdown, Edit Rules, club presets and analytics. +- Migrate stored agenda rows already saved as `Table Topics`. See Q2. + +## Acceptance criteria +- AC-1: Given no custom roles, when `David Kim (Table Topics Speech)` is imported with Simple + Format, then the row is `David Kim` / `Table Topics Speech` with rules 60/90/120 s (grace 30). +- AC-2: Given no custom roles, when `Alex (Table Topics)`, or the bare line `Table Topics`, is + imported, then the role is `Table Topics Speech`. +- AC-3: Given an imported `Table Topics Speech` row, when it is loaded into the Live tab and + START is pressed, then the dropdown shows `Table Topics Speech`, the rules read 1 / 1:30 / + 2 min, the timer runs, and no "Please set timing rules first" toast appears. This holds in + both the web and Zoom apps. +- AC-4 (proposed): Given no custom roles, when `Sam (Table Topics Master)` is imported, then + the role is `Short Roles`. +- AC-5 (proposed): Given no custom roles, when `Jo (Table Topics Evaluator)` or + `Jo (Table Topics Evaluation)` is imported, then the role is `Table Topics Evaluation`. +- AC-6: Given `Dominique - General Evaluation (Table Topics)`, when imported, then the role is + `Table Topics Speech`, not `General Evaluation` (#90 still holds). +- AC-7: For every text, `detectRoleFromText` returns one of: a `DEFAULT_ROLE_RULES` key, + `BREAK_ROLE`, a passed custom role name, or the fallback. It never returns `'Table Topics'`. +- AC-8: A custom role named exactly `Table Topics` still wins for `(Table Topics)`. Existing + results for other built-in roles are unchanged: the current suites pass, apart from the + assertions listed in Findings. + +## Test list +| AC | Unit | Playwright | Zoom-client | +|---|---|---|---| +| AC-1 | `simpleFormatParser.test.js`: demo-file test expects `Table Topics Speech`. `apps/{web,zoom-app}/src/context/TimerContext.test.jsx`: `importBulkSpeakers('David Kim (Table Topics Speech)')` stores rules 60/90/120 | `e2e/zoom.timer-flow.spec.js`: NEW test, import both lines, rows show `(Table Topics Speech)` | `agenda-import` NEW IM-06 | +| AC-2 | `timingRules.test.js`: exact + partial `table topics`; `:130` → `Table Topics Speech`. `simpleFormatParser.test.js`: `Alex (Table Topics)` | same NEW test | IM-06 | +| AC-3 | `apps/{web,zoom-app}/src/components/LiveTab.test.jsx`: START with `currentSpeaker.role = 'Table Topics Speech'` from an imported row shows no warning. If an existing test already proves this, reference it instead | same NEW test: click the row, Live tab dropdown is `Table Topics Speech`, rules line reads 1 minute / 1 minute 30 seconds / 2 minutes, START starts | IM-06: rules line + START on real video, card reaches green at 1:00 | +| AC-4 | `timingRules.test.js:209-215` rewritten → `Short Roles`. `simpleFormatParser.test.js`: `Sam (Table Topics Master)` | — | — (unit only) | +| AC-5 | `timingRules.test.js` + `simpleFormatParser.test.js`: evaluator / evaluation | — | — (unit only) | +| AC-6 | `simpleFormatParser.test.js:127-130` (#90 AC-2) → `Table Topics Speech` | `e2e/zoom.timer-flow.spec.js:186-202` (#90) expects `(Table Topics Speech)` | IM-05 updated | +| AC-7 | `timingRules.test.js`: NEW table test over ~25 inputs (built-in names, the demo files' lines, EasySpeak role labels, breaks, junk), asserting each result is in `ROLE_OPTIONS ∪ {BREAK_ROLE}` | — | — | +| AC-8 | `simpleFormatParser.test.js`: `matchCustomRole`/parse with custom `Table Topics` → `Table Topics`. Existing suites unchanged | `e2e/web.agenda-flow.spec.js` unchanged and green | IM-01…IM-04 unchanged | + +## Risk +Low. One pure function in `packages/shared/timingRules.js`, whose only caller is the Simple +Format parser. No `riskHigh` paths. No stored data, sync format or API changes. +Visible change: Simple Format lines that used to import as `Table Topics` (broken) now import +as `Table Topics Speech`, and `Table Topics Master` now imports as `Short Roles`. The change +edits tests that #90's oracle recorded (see Findings). That is intended and outside the guard's +scope, but a reviewer should know. + +## Open questions +- Q1: Should `Table Topics Master` import as `Short Roles` (AC-4) and `Table Topics Evaluator` + as `Table Topics Evaluation` (AC-5)? Neither is in the issue's expected result; both + currently land on the broken `Table Topics`. **Default: yes, include both.** They are the + same bug in the same three lines. +- Q2: Agenda rows already saved as `Table Topics` (localStorage or club sync) stay broken until + they are re-imported or edited. Should load or START map `Table Topics` → `Table Topics Speech`? + **Default: no.** Out of scope: it adds an alias in both apps for data that is easy to fix + by re-importing. File a follow-up if PostHog `unknown_role` still shows `Table Topics` + after release. +- Q3: Add `packages/shared/timingRules.js` to `agenda-import`'s `covers`? Then later edits + to the timing rules (breaks, defaults) will also flag this spec for drift review. + **Default: yes.** Otherwise no spec covers this diff and the Zoom station skips the + real-client run. +- Q4: Keep the Zoom placeholder `Alex (Table Topics)` in `AgendaTab.jsx:454`? It works after + this fix. **Default: keep it** (no UI change). From fd50e0c59fbf4dcc774e8fd8251d09c01d02893f Mon Sep 17 00:00:00 2001 From: Shuhao Date: Fri, 13 Nov 2026 11:32:04 -0500 Subject: [PATCH 2/5] plan(#91): don't guess roles on import; ask for the ones that don't match The first plan fixed the keyword ladder so "(Table Topics)" landed on Table Topics Speech. The direction changed: a fallback to a built-in can land on a role the user removed, and the closest-match step from #77 gives false positives ("(Table Topics)" became a custom Table Topics Master). Import now takes only an exact role name and highlights the rest on the Agenda tab for a manual pick. --- .factory/plans/91-table-topics-role.md | 244 +++++++++++++------------ 1 file changed, 125 insertions(+), 119 deletions(-) diff --git a/.factory/plans/91-table-topics-role.md b/.factory/plans/91-table-topics-role.md index 5228460..78b232a 100644 --- a/.factory/plans/91-table-topics-role.md +++ b/.factory/plans/91-table-topics-role.md @@ -1,134 +1,140 @@ -# Simple Format import: "(Table Topics Speech)" imports as 'Table Topics' with no timing rules (#91) +# Simple Format import: ask for a role when the bracket doesn't name one, instead of guessing (#91) ## Problem -In Agenda → Import Text → Simple Format, any line whose role says "Table Topics" imports with -role `Table Topics`. That is not a rules key, so the row gets Standard Speech's 5/6/7 min -rules, the Live tab dropdown has no matching option, and START shows "Please set timing rules -first". It affects the demo file's `(Table Topics Speech)` and the Zoom placeholder's -`(Table Topics)`. +Agenda → Import Text → Simple Format guesses a role whenever the bracket does not name one of +your roles exactly. The guess is often wrong and the app says nothing: +- `(Table Topics Speech)` and `(Table Topics)` land on `Table Topics`, which is not a rules + key. The row gets Standard Speech's 5/6/7 min, and START shows "Please set timing rules + first" (the original report). +- A bracket close to a custom role takes that role: `(Table Topics)` becomes a custom + `Table Topics Master`, `(Speaker)` becomes `Keynote Speaker`. +- A built-in role you hid in Edit Rules still imports, with Standard Speech timing. + +Direction (2026-11-13, replaces the first plan that fixed the keyword ladder): import does +not guess. A bracket that names one of your roles gets it. Every other row is highlighted on +the Agenda tab and you pick its role from your saved roles. ## Findings -- Reproduced on `origin/dev` (a4439d6) with the shared parser and no custom roles: - `David Kim (Table Topics Speech)`, `Alex (Table Topics)`, `Sam (Table Topics Master)`, - `Jo (Table Topics Evaluator)` and the bare line `Table Topics` all return role - `Table Topics`, and `'Table Topics' in DEFAULT_ROLE_RULES` is `false`. -- Cause: `detectRoleFromText` in - [timingRules.js](../../packages/shared/timingRules.js) returns a role that is not a key: - - `:82` exact `table topics` → `'Table Topics'` - - `:113-114` any text containing `table topics` → `'Table Topics'` - - `DEFAULT_ROLE_RULES` (`:3-12`) only has `'Table Topics Speech'` and `'Table Topics Evaluation'`. -- Same ladder, same bug: `table topics master` (`:155`, short roles) and - `table topics evaluator` can't be reached because `:113` catches them first. `evaluation` - is matched at `:90`, but `evaluator` isn't, and the evaluator branch at `:144` skips text - that contains `table topics`. -- Since #90 (c3a0f78), `parseSimpleFormatText` - ([simpleFormatParser.js:93-96](../../packages/shared/simpleFormatParser.js)) tries custom - roles, then `detectRoleFromText(bracket, names, null)`, then the whole line. The bracket - `Table Topics Speech` doesn't exactly match a built-in name in `detectRoleFromText`, so it - reaches `:113`. `parseSimpleFormatText` is the only caller of `detectRoleFromText`. - EasySpeak import uses its own mapping (`easySpeakParser.js`). -- Downstream, both apps do `rules: roleRules[role] || DEFAULT_ROLE_RULES['Standard Speech']` - (`apps/web/src/context/TimerContext.jsx:244-249`, `apps/zoom-app/src/context/TimerContext.jsx:476-481`). - `LiveTab.handleStart` re-reads `roleRules[selectedRole]` and rejects - (`apps/web/src/components/LiveTab.jsx:205`, `apps/zoom-app/src/components/LiveTab.jsx:614-630`). - With a valid key from the parser, these work unchanged. -- Tests that currently assert the bug: - - `packages/shared/__tests__/timingRules.test.js:130` (`'Table Topics'` with null fallback) - and `:209-215` (`table topics master` → `Table Topics`, with a comment explaining why). - - `packages/shared/__tests__/simpleFormatParser.test.js:91-93`: the demo-file test expects - `Table Topics`. - - `packages/shared/__tests__/simpleFormatParser.test.js:127-130` (#90 AC-2) expects - `Table Topics`. - - `e2e/zoom.timer-flow.spec.js:186-202` (#90) expects `(Table Topics)`. - - Zoom-client spec `e2e/zoom-client/agenda-import.md` IM-05 expects the row and - `toastmaster_agenda` to show `Table Topics`. "Not covered" lists #91 as a known open bug. - - The two #90 test files are listed in `.factory/oracles/90.json`. The oracle guard and - `check-oracle.mjs` only lock oracles of the branch's own issue, so #91 may update them. - The #90 point still holds: the bracket beats the name's `General Evaluation`. Only the - role string it lands on changes. -- Spec coverage: `agenda-import` covers `simpleFormatParser.js`, but no spec covers - `timingRules.js`. Without a `covers` entry, `stop-gate.mjs --hits` finds nothing for this - diff and the Zoom station would skip the real-client run. +- Verified on `dev` a4439d6 with the shared parser, custom roles `Keynote Speaker` and + `Table Topics Master`, `Ice Breaker` hidden (rules = what `importBulkSpeakers` stores): + + | Line | Role | Rules (s) | How | + |---|---|---|---| + | `A (Standard Speech)` | Standard Speech | 300/360/420 | exact | + | `B (keynote speaker)` | Keynote Speaker | 600/720/840 | exact, case ignored | + | `C (Keynote)` | Keynote Speaker | 600/720/840 | closest custom role | + | `D (Speaker)` | Keynote Speaker | 600/720/840 | closest custom role | + | `E (Table Topics Speech)` | Table Topics | 300/360/420 | keyword, not a key | + | `F (Table Topics)` | Table Topics Master | 10/20/30 | closest custom role | + | `G (Toastmaster of the Evening)` | Short Roles | 30/45/60 | keyword | + | `H (Educational Talk)` | Standard Speech | 300/360/420 | default | + | `I (Ice Breaker)` | Ice Breaker | 300/360/420 | hidden role | + | `J` (no bracket) | Standard Speech | 300/360/420 | default | + | `Coffee break` | Take a Break | 300/360/420 | keyword, Standard Speech rules | + +- Order today in `parseSimpleFormatText` (`packages/shared/simpleFormatParser.js:80-98`): + exact custom role → closest custom role (`matchCustomRole`, #77) → keywords in the + bracket (`detectRoleFromText(bracket, names, null)`, #90) → keywords in the whole line → + `'Standard Speech'`. Built-in names are never checked against the user's list, so hidden + built-ins still match. +- `importBulkSpeakers` (web `TimerContext.jsx:243-258`, Zoom `:475-490`) stores + `rules: roleRules[role] || DEFAULT_ROLE_RULES['Standard Speech']`. +- Users can remove roles: `removeRoleRules` hides a built-in (`toastmaster_hidden_builtin_roles`) + or deletes a custom role. `roleOptions` is the list the role dropdowns show (visible + built-ins, then custom roles; the Zoom app also lists `Take a Break`). +- `parseSimpleFormatText` is the only caller of `detectRoleFromText` and `matchCustomRole`. + EasySpeak import has its own mapping (`easySpeakParser.js`). +- `toastmaster_agenda` is in `SYNCED_KEYS`, so rows reach other devices and older builds. + Older builds treat an unknown `role` string like today's `Table Topics` row. +- The Live tab (both apps, e876ca8) moves a loaded speaker whose role is not in + `roleOptions` to Standard Speech or the first listed role, with no notice. That covers the + Live tab's agenda picker and the auto-advance after FINISH. ## Approach -Fix the ladder in `detectRoleFromText` so every "table topics" text maps to a real key. -- Exact match: `table topics` → `'Table Topics Speech'`; add `table topics speech` → - `'Table Topics Speech'`. -- Partial matches, in this order, replacing `:112-115`: - 1. `table topics evaluator` → `'Table Topics Evaluation'` (`evaluation` is already at `:90`) - 2. `table topics master` → `'Short Roles'` (the role the dead branch at `:155` meant) - 3. `table topics` → `'Table Topics Speech'` -- Remove the dead `table topics master` entry at `:155`, and the now-redundant - `!normalized.includes('table topics')` guard at `:144` if the tests allow. -- No change to `simpleFormatParser.js`, the TimerContexts or the LiveTabs. -- Update the tests that assert the old result (listed above) and the `agenda-import` spec: - - IM-05 now expects `Table Topics Speech`. - - New step IM-06 covers the `(Table Topics Speech)` / `(Table Topics)` lines and START. - - Add `packages/shared/timingRules.js` to `covers`. - - Remove the #91 line from "Not covered". +Shared (`packages/shared/simpleFormatParser.js`): +- `findRoleByName(text, roleNames)`: the role in `roleNames` with the same words in the same + order as `text` (case, spacing and punctuation ignored), or `null`. +- `parseSimpleFormatText(text, roleNames)` returns `{ name, role, roleText }`. `roleText` is + the last `(...)` group as written; for a line with no bracket it is the whole line when that + names a role, else `''`. `role` is `findRoleByName(roleText, roleNames)`, else `null`. The + name rule is unchanged. +- `agendaItemNeedsRole(item, roleNames)`: true for a row that is not completed, is not a + break, and whose role is not in `roleNames`. Derived on every render, so rows saved before + this change (`Table Topics`) and rows whose role was later removed are flagged too. +- Remove `matchCustomRole` and `detectRoleFromText` (no callers left). -Rejected: -- Alias `'Table Topics'` → `'Table Topics Speech'` in `importBulkSpeakers` and `handleStart` - in both apps. That fixes the symptom in four places and leaves the parser returning a - non-key. -- Add a `'Table Topics'` key to `DEFAULT_ROLE_RULES`. That makes a duplicate role in the - dropdown, Edit Rules, club presets and analytics. -- Migrate stored agenda rows already saved as `Table Topics`. See Q2. +Apps (web and Zoom, same change): +- `importBulkSpeakers` matches against `roleOptions`. A matched row stores the role's exact + name and its rules. An unmatched row stores `role: roleText` and no `rules`. Returns + `{ count, needsRole }`. `agenda_imported` gains `needs_role_count`. +- Agenda tab, for a row that needs a role: red row with the text as written + ("(Table Topics) is not one of your roles", or "No role given") and a "Pick a role" select + of `roleOptions`. Picking calls `updateAgendaItem`, which stores that role's rules. A + banner above the list counts these rows. Clicking such a row does not open the Live tab: a + toast asks for a role first. Its Edit modal starts on "Pick a role…" with Update disabled. +- Import toast: "Imported N speaker(s)" when every row matched (unchanged); otherwise + "Imported N speaker(s). M need a role" as a warning. +- Add Item defaults to Standard Speech, or the first listed role when Standard Speech is + hidden (today it shows the first role but saves Standard Speech, which would now be flagged). + +Not changed: EasySpeak's mapping (its rows are flagged only when the mapped role is not in +your list), the Live tab, the TimerContext load/update paths. ## Acceptance criteria -- AC-1: Given no custom roles, when `David Kim (Table Topics Speech)` is imported with Simple - Format, then the row is `David Kim` / `Table Topics Speech` with rules 60/90/120 s (grace 30). -- AC-2: Given no custom roles, when `Alex (Table Topics)`, or the bare line `Table Topics`, is - imported, then the role is `Table Topics Speech`. -- AC-3: Given an imported `Table Topics Speech` row, when it is loaded into the Live tab and - START is pressed, then the dropdown shows `Table Topics Speech`, the rules read 1 / 1:30 / - 2 min, the timer runs, and no "Please set timing rules first" toast appears. This holds in - both the web and Zoom apps. -- AC-4 (proposed): Given no custom roles, when `Sam (Table Topics Master)` is imported, then - the role is `Short Roles`. -- AC-5 (proposed): Given no custom roles, when `Jo (Table Topics Evaluator)` or - `Jo (Table Topics Evaluation)` is imported, then the role is `Table Topics Evaluation`. -- AC-6: Given `Dominique - General Evaluation (Table Topics)`, when imported, then the role is - `Table Topics Speech`, not `General Evaluation` (#90 still holds). -- AC-7: For every text, `detectRoleFromText` returns one of: a `DEFAULT_ROLE_RULES` key, - `BREAK_ROLE`, a passed custom role name, or the fallback. It never returns `'Table Topics'`. -- AC-8: A custom role named exactly `Table Topics` still wins for `(Table Topics)`. Existing - results for other built-in roles are unchanged: the current suites pass, apart from the - assertions listed in Findings. +- AC-1: A bracket with the same words as one of your roles imports with that role's exact + name and rules: `B (keynote speaker)` → `Keynote Speaker`; `David Kim (Table Topics + Speech)` → `Table Topics Speech` with 60/90/120 s. +- AC-2: A bracket that is not exactly one of your roles imports with no role and no rules, + keeping the text as written: `Alex (Table Topics)`, `C (Keynote)`, `D (Speaker)`, + `Lee (Moment of Reflection)`, `Yu (Closing Remarks - 2 min)`. A line with no bracket that + doesn't name a role imports with role `''`. +- AC-3: A bracket naming a hidden built-in or a removed custom role is not matched. +- AC-4: The Agenda tab highlights every row that needs a role, shows its text as written, and + offers a "Pick a role" select of your roles. A banner counts these rows. Completed rows and + breaks are never highlighted. +- AC-5: Picking a role sets the row's role and that role's rules, and the highlight goes. +- AC-6: Clicking a highlighted row keeps the Agenda tab open and shows "Pick a role for + {name} first". Its Edit modal starts on "Pick a role…" and Update is disabled until a role + is picked. +- AC-7: Import toast and analytics as in Approach. +- AC-8: A row saved earlier with a role that is not in your list (e.g. `Table Topics`) is + highlighted when the Agenda tab opens. +- AC-9: #90 still holds: a keyword in the name never sets the role. + `Dominique - General Evaluation (Table Topics)` needs a role; it is not General Evaluation. +- AC-10: Add Item's role starts on Standard Speech, or on the first listed role when Standard + Speech is hidden. ## Test list -| AC | Unit | Playwright | Zoom-client | +| AC | Unit | Playwright | Zoom-client (`agenda-import`) | |---|---|---|---| -| AC-1 | `simpleFormatParser.test.js`: demo-file test expects `Table Topics Speech`. `apps/{web,zoom-app}/src/context/TimerContext.test.jsx`: `importBulkSpeakers('David Kim (Table Topics Speech)')` stores rules 60/90/120 | `e2e/zoom.timer-flow.spec.js`: NEW test, import both lines, rows show `(Table Topics Speech)` | `agenda-import` NEW IM-06 | -| AC-2 | `timingRules.test.js`: exact + partial `table topics`; `:130` → `Table Topics Speech`. `simpleFormatParser.test.js`: `Alex (Table Topics)` | same NEW test | IM-06 | -| AC-3 | `apps/{web,zoom-app}/src/components/LiveTab.test.jsx`: START with `currentSpeaker.role = 'Table Topics Speech'` from an imported row shows no warning. If an existing test already proves this, reference it instead | same NEW test: click the row, Live tab dropdown is `Table Topics Speech`, rules line reads 1 minute / 1 minute 30 seconds / 2 minutes, START starts | IM-06: rules line + START on real video, card reaches green at 1:00 | -| AC-4 | `timingRules.test.js:209-215` rewritten → `Short Roles`. `simpleFormatParser.test.js`: `Sam (Table Topics Master)` | — | — (unit only) | -| AC-5 | `timingRules.test.js` + `simpleFormatParser.test.js`: evaluator / evaluation | — | — (unit only) | -| AC-6 | `simpleFormatParser.test.js:127-130` (#90 AC-2) → `Table Topics Speech` | `e2e/zoom.timer-flow.spec.js:186-202` (#90) expects `(Table Topics Speech)` | IM-05 updated | -| AC-7 | `timingRules.test.js`: NEW table test over ~25 inputs (built-in names, the demo files' lines, EasySpeak role labels, breaks, junk), asserting each result is in `ROLE_OPTIONS ∪ {BREAK_ROLE}` | — | — | -| AC-8 | `simpleFormatParser.test.js`: `matchCustomRole`/parse with custom `Table Topics` → `Table Topics`. Existing suites unchanged | `e2e/web.agenda-flow.spec.js` unchanged and green | IM-01…IM-04 unchanged | +| AC-1 | `simpleFormatParser.test.js` | `zoom.timer-flow`, `web.agenda-flow` | IM-02, IM-06 | +| AC-2 | `simpleFormatParser.test.js`, both `TimerContext.test.jsx` | same | IM-02, IM-06 | +| AC-3 | `simpleFormatParser.test.js`, Zoom `TimerContext.test.jsx` | — | — | +| AC-4 | both `AgendaTab.test.jsx` | `zoom.timer-flow`, `web.agenda-flow` | IM-02, IM-06 | +| AC-5 | both `AgendaTab.test.jsx` | same | IM-06 | +| AC-6 | both `AgendaTab.test.jsx` | — | IM-06 | +| AC-7 | both `TimerContext.test.jsx`, `AgendaTab.test.jsx` | toast text | IM-02 (PostHog) | +| AC-8 | both `AgendaTab.test.jsx` | — | — | +| AC-9 | `simpleFormatParser.test.js` | `zoom.timer-flow` (#90 test) | IM-05 | +| AC-10 | both `AgendaTab.test.jsx` | — | — | ## Risk -Low. One pure function in `packages/shared/timingRules.js`, whose only caller is the Simple -Format parser. No `riskHigh` paths. No stored data, sync format or API changes. -Visible change: Simple Format lines that used to import as `Table Topics` (broken) now import -as `Table Topics Speech`, and `Table Topics Master` now imports as `Short Roles`. The change -edits tests that #90's oracle recorded (see Findings). That is intended and outside the guard's -scope, but a reviewer should know. +Medium-low. Client-side parsing and the Agenda tab in both apps. No API, Worker, sync format +or migration. Visible change: lines that used to get a guessed role now need a pick, so an +agenda pasted with loose role names needs a few clicks. Rows synced to an older build keep +their text as role and no rules; that build times them as Standard Speech, as it does today +for `Table Topics`. + +## Decisions (2026-11-13) +- Exact means same words, same order; case, spacing and punctuation don't count. +- A line with no bracket gets a role only when the whole line names one; otherwise it needs a + role. No keyword guessing anywhere in Simple Format import. +- The first plan's questions are moot: there is no `Table Topics Master` → `Short Roles` + mapping (Q1), old `Table Topics` rows are flagged by AC-8 (Q2), `timingRules.js` only + loses dead code (Q3), and the Zoom placeholder `Alex (Table Topics)` is replaced by a line + with built-in names (Q4). -## Open questions -- Q1: Should `Table Topics Master` import as `Short Roles` (AC-4) and `Table Topics Evaluator` - as `Table Topics Evaluation` (AC-5)? Neither is in the issue's expected result; both - currently land on the broken `Table Topics`. **Default: yes, include both.** They are the - same bug in the same three lines. -- Q2: Agenda rows already saved as `Table Topics` (localStorage or club sync) stay broken until - they are re-imported or edited. Should load or START map `Table Topics` → `Table Topics Speech`? - **Default: no.** Out of scope: it adds an alias in both apps for data that is easy to fix - by re-importing. File a follow-up if PostHog `unknown_role` still shows `Table Topics` - after release. -- Q3: Add `packages/shared/timingRules.js` to `agenda-import`'s `covers`? Then later edits - to the timing rules (breaks, defaults) will also flag this spec for drift review. - **Default: yes.** Otherwise no spec covers this diff and the Zoom station skips the - real-client run. -- Q4: Keep the Zoom placeholder `Alex (Table Topics)` in `AgendaTab.jsx:454`? It works after - this fix. **Default: keep it** (no UI change). +## Follow-up (not in this PR) +- The Live tab can still load a highlighted row through its agenda picker or the auto-advance + after FINISH, and then times it with the default role (e876ca8). Refusing those loads + touches `LiveTab.jsx` in both apps, which three more Zoom specs cover. From 568a64d418bf82891e527b43b82df2c52ba86dc4 Mon Sep 17 00:00:00 2001 From: Shuhao Date: Fri, 13 Nov 2026 11:32:04 -0500 Subject: [PATCH 3/5] fix(#91): Simple Format import takes exact role names and asks for the rest A line gets a role only when its bracket names one of the roles in the dropdown (same words; case, spacing and punctuation ignored). Any other line keeps its text as written, with no rules. The Agenda tab highlights such rows, with a "Pick a role" list of the saved roles and a banner that counts them; clicking one asks for a role instead of opening the Live tab. The check is derived from the current roles, so rows saved earlier as 'Table Topics' and rows whose role was later removed are highlighted too. Completed rows and breaks never are. matchCustomRole and detectRoleFromText had no callers left and are removed. Add Item now starts on the first listed role when Standard Speech is hidden, so it never saves a role the picker didn't show. agenda-import gains IM-06 and new expectations for IM-02 and IM-05. timer-core is reviewed: importBulkSpeakers is under its Not covered. --- apps/web/src/components/AgendaTab.jsx | 76 +++-- apps/web/src/components/AgendaTab.test.jsx | 124 ++++++- apps/web/src/context/TimerContext.jsx | 24 +- apps/web/src/context/TimerContext.test.jsx | 28 +- apps/zoom-app/src/components/AgendaTab.jsx | 113 +++++-- .../src/components/AgendaTab.test.jsx | 179 ++++++++++ apps/zoom-app/src/context/TimerContext.jsx | 24 +- .../src/context/TimerContext.test.jsx | 65 +++- e2e/web.agenda-flow.spec.js | 38 ++- e2e/zoom-client/INDEX.md | 2 +- e2e/zoom-client/agenda-import.md | 131 +++++--- e2e/zoom-client/timer-core.md | 4 +- e2e/zoom.timer-flow.spec.js | 69 +++- .../__tests__/simpleFormatParser.test.js | 309 ++++++++---------- packages/shared/__tests__/timingRules.test.js | 134 -------- packages/shared/simpleFormatParser.js | 97 ++---- packages/shared/timingRules.js | 109 ------ 17 files changed, 910 insertions(+), 616 deletions(-) create mode 100644 apps/zoom-app/src/components/AgendaTab.test.jsx diff --git a/apps/web/src/components/AgendaTab.jsx b/apps/web/src/components/AgendaTab.jsx index 3c6f117..bb6c7e4 100644 --- a/apps/web/src/components/AgendaTab.jsx +++ b/apps/web/src/components/AgendaTab.jsx @@ -1,7 +1,7 @@ import { useState, memo } from 'react'; import { useTimer } from '../context/TimerContext'; import { useToast } from '../context/ToastContext'; -import { Plus, Upload, X, Edit2, Trash2, GripVertical, Check } from 'lucide-react'; +import { Plus, Upload, X, Edit2, Trash2, GripVertical, Check, AlertTriangle } from 'lucide-react'; import ConfirmModal from './ConfirmModal'; import { DndContext, @@ -19,16 +19,17 @@ import { verticalListSortingStrategy, } from '@dnd-kit/sortable'; import { CSS } from '@dnd-kit/utilities'; -import { DEFAULT_ROLE_RULES, DEFAULT_CUSTOM_RULES, loadTimeInputMode, saveTimeInputMode } from '@toastmaster-timer/shared'; +import { DEFAULT_ROLE_RULES, DEFAULT_CUSTOM_RULES, loadTimeInputMode, saveTimeInputMode, agendaItemNeedsRole } from '@toastmaster-timer/shared'; import TimeInput, { TimeInputModeToggle } from './TimeInput'; import { trackEvent } from '../utils/posthog'; -function SortableItem({ item, isActive, isCompleted, onEdit, onDelete, onClick }) { +function SortableItem({ item, isActive, isCompleted, needsRole, roleOptions, onPickRole, onEdit, onDelete, onClick }) { const { attributes, listeners, setNodeRef, transform, transition } = useSortable({ id: item.id }); const style = { transform: CSS.Transform.toString(transform), transition }; + const tone = needsRole ? 'bg-red-50 hover:bg-red-100 border-l-4 border-l-red-500' : isActive ? 'bg-blue-50' : 'bg-white hover:bg-gray-50'; return ( -
+
{isCompleted ? (
) : ( @@ -36,7 +37,17 @@ function SortableItem({ item, isActive, isCompleted, onEdit, onDelete, onClick } )}
{item.name}
-
({item.role}{item.role === 'Short Roles' && item.originalShortRole && - {item.originalShortRole}})
+ {needsRole ? ( + <> +
+ + + ) : ( +
({item.role}{item.role === 'Short Roles' && item.originalShortRole && - {item.originalShortRole}})
+ )}
@@ -49,6 +60,11 @@ function SortableItem({ item, isActive, isCompleted, onEdit, onDelete, onClick } export default memo(function AgendaTab({ onSwitchToLive }) { const { agenda, activeSpeakerId, addToAgenda, removeFromAgenda, reorderAgenda, updateAgendaItem, loadSpeakerFromAgenda, importBulkSpeakers, importEasySpeakSpeakers, clearAllAgenda, roleRules, roleOptions } = useTimer(); const { showToast } = useToast(); + // Standard Speech unless it was removed in Edit Rules; then the first listed + // role, so the picker never shows one role while saving another. + const defaultRole = roleOptions.includes('Standard Speech') ? 'Standard Speech' : roleOptions[0] || 'Standard Speech'; + const needsRole = (item) => agendaItemNeedsRole(item, roleOptions); + const needsRoleCount = agenda.filter(needsRole).length; const [showImportModal, setShowImportModal] = useState(false); const [showAddModal, setShowAddModal] = useState(false); const [importTab, setImportTab] = useState('easyspeak'); @@ -56,7 +72,7 @@ export default memo(function AgendaTab({ onSwitchToLive }) { const [easySpeakText, setEasySpeakText] = useState(''); const [editItem, setEditItem] = useState(null); const [newSpeakerName, setNewSpeakerName] = useState(''); - const [newSpeakerRole, setNewSpeakerRole] = useState('Standard Speech'); + const [newSpeakerRole, setNewSpeakerRole] = useState(defaultRole); const [customRules, setCustomRules] = useState({ ...DEFAULT_CUSTOM_RULES }); const [timeInputMode, setTimeInputMode] = useState(loadTimeInputMode); const [showDeleteConfirm, setShowDeleteConfirm] = useState(false); @@ -77,8 +93,9 @@ export default memo(function AgendaTab({ onSwitchToLive }) { const handleSimpleImport = () => { if (importText.trim()) { - const count = importBulkSpeakers(importText); - showToast(`Imported ${count} speaker(s)`, 'success'); + const { count, needsRole: unmatched } = importBulkSpeakers(importText); + if (unmatched > 0) showToast(`Imported ${count} speaker(s). ${unmatched} need${unmatched === 1 ? 's' : ''} a role`, 'warning'); + else showToast(`Imported ${count} speaker(s)`, 'success'); setImportText(''); setShowImportModal(false); } @@ -98,7 +115,7 @@ export default memo(function AgendaTab({ onSwitchToLive }) { if (newSpeakerRole === 'Custom') speakerData.rules = customRules; addToAgenda(speakerData); setNewSpeakerName(''); - setNewSpeakerRole('Standard Speech'); + setNewSpeakerRole(defaultRole); setCustomRules({ ...DEFAULT_CUSTOM_RULES }); setShowAddModal(false); } @@ -107,25 +124,31 @@ export default memo(function AgendaTab({ onSwitchToLive }) { const handleEdit = (item) => { setEditItem(item); setNewSpeakerName(item.name); - setNewSpeakerRole(item.role); + // A role that isn't listed can't be shown by the select; start on "Pick a role…". + setNewSpeakerRole(needsRole(item) ? '' : item.role); if (item.role === 'Custom' && item.rules) setCustomRules({ ...DEFAULT_CUSTOM_RULES, ...item.rules }); else setCustomRules({ ...DEFAULT_CUSTOM_RULES }); setShowAddModal(true); }; const handleUpdate = () => { - if (editItem && newSpeakerName.trim()) { + if (editItem && newSpeakerName.trim() && newSpeakerRole) { const speakerData = { name: newSpeakerName.trim(), role: newSpeakerRole }; if (newSpeakerRole === 'Custom') speakerData.rules = customRules; updateAgendaItem(editItem.id, speakerData); setEditItem(null); setNewSpeakerName(''); - setNewSpeakerRole('Standard Speech'); + setNewSpeakerRole(defaultRole); setCustomRules({ ...DEFAULT_CUSTOM_RULES }); setShowAddModal(false); } }; + const handlePickRole = (item, role) => { + if (!role) return; + updateAgendaItem(item.id, { name: item.name, role }); + trackEvent('agenda_role_picked', { is_custom: !(role in DEFAULT_ROLE_RULES) }); + }; const handleDelete = (id) => { setDeleteItemId(id); setShowDeleteConfirm(true); }; const handleConfirmDelete = () => { if (deleteItemId) removeFromAgenda(deleteItemId); setShowDeleteConfirm(false); setDeleteItemId(null); }; const handleClearAll = () => setShowClearAllConfirm(true); @@ -146,6 +169,12 @@ export default memo(function AgendaTab({ onSwitchToLive }) { setCustomRules(prev => ({ ...prev, [field]: value })); }; const handleSpeakerClick = (item) => { + // Without a role there are no rules to time with; the Live tab would quietly + // fall back to the default role. Ask instead. + if (needsRole(item)) { + showToast(`Pick a role for ${item.name} first`, 'warning'); + return; + } loadSpeakerFromAgenda(item.id); trackEvent('speaker_loaded_from_agenda', {}); if (onSwitchToLive) onSwitchToLive(); @@ -157,7 +186,7 @@ export default memo(function AgendaTab({ onSwitchToLive }) { - {agenda.length > 0 && ( @@ -168,15 +197,23 @@ export default memo(function AgendaTab({ onSwitchToLive }) { {agenda.length === 0 ? (
No speakers in agenda. Add speakers or import from text.
) : ( + <> + {needsRoleCount > 0 && ( +
+
+ )} item.id)} strategy={verticalListSortingStrategy}>
{agenda.map((item) => ( - handleSpeakerClick(item)} /> + handleSpeakerClick(item)} /> ))}
+ )} {showImportModal && ( @@ -201,7 +238,7 @@ export default memo(function AgendaTab({ onSwitchToLive }) {
) : (
-

One speaker per line with role in parentheses. Example: John Doe (Ice Breaker)

+

One speaker per line with the role in parentheses, spelled as in your role list. Example: John Doe (Ice Breaker). A row whose role isn't in the list is highlighted so you can pick one.