diff --git a/.factory/plans/91-table-topics-role.md b/.factory/plans/91-table-topics-role.md new file mode 100644 index 0000000..78b232a --- /dev/null +++ b/.factory/plans/91-table-topics-role.md @@ -0,0 +1,140 @@ +# Simple Format import: ask for a role when the bracket doesn't name one, instead of guessing (#91) + +## Problem +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 +- 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 +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). + +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: 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 (`agenda-import`) | +|---|---|---|---| +| 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 +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). + +## 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. 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.