diff --git a/.factory/oracles/107.json b/.factory/oracles/107.json new file mode 100644 index 0000000..4c04224 --- /dev/null +++ b/.factory/oracles/107.json @@ -0,0 +1,7 @@ +{ + "issue": 107, + "files": [ + "apps/zoom-app/src/components/LiveTab.test.jsx", + "apps/web/src/components/LiveTab.test.jsx" + ] +} diff --git a/.factory/plans/107-timer-reach-audience.md b/.factory/plans/107-timer-reach-audience.md new file mode 100644 index 0000000..71aca9c --- /dev/null +++ b/.factory/plans/107-timer-reach-audience.md @@ -0,0 +1,98 @@ +# Timer reach metric: attach audience_size to timer_started events (#107) + +## Problem +`timer_started` counts operator presses, not people served: a class of 20 where the teacher +presses START 15 times looks like 15 solo web starts. Analytics (`traffic_report.py`, the +PostHog "Timer reach (daily)" insight) is already waiting for an `audience_size` property on +`timer_started` from the Zoom app. Only a count leaves the device, never names. + +## Findings +- `apps/zoom-app/src/components/LiveTab.jsx:601` `handleStart`; the event is sent at + `:634-642` inside `(window.requestIdleCallback || setTimeout)(...)` with `speaker_name`, + `role`, `timing_rules`. It is the only `timer_started` in the Zoom app (`handleContinue` + at `:645` sends `timer_continued`). +- `apps/zoom-app/src/utils/zoomSdk.js:4444` `getZoomParticipants()` awaits SDK init, then + merges `getUserContext` (self) with `getMeetingParticipants` (everyone else), deduped. It + returns `{ participants, role, restricted }`; with no SDK it returns `participants: []`, + `restricted: false` (`:4452-4455`). `restricted` is true only when the list call failed + and the role is known and not host/co-host (`:4498-4499`). It does not throw on API + errors; `initializeZoomSdk()` is the only await that could. +- `apps/zoom-app/src/hooks/useZoomParticipants.js:18` fetches once per mount and never + refreshes. `LiveTab` mounts when the app opens, so a hook count misses late joiners (the + owner's comment on the issue). The hook is not used. +- `zoom_meeting_id` is already registered on every event as a session property + (`apps/zoom-app/src/main.jsx:101-105`), which is what the report groups reach by. No change. +- Web timer: `apps/web/src/components/LiveTab.jsx:203` sends `{ role, speaker_name }` and has + no Zoom SDK. No change needed there; AC-4 only pins it down. +- Tests: `apps/zoom-app/src/components/LiveTab.test.jsx` already mocks `getZoomParticipants` + (`:42`) and `trackEvent` (`:47`) and uses fake timers (`:87`); nothing asserts on + `timer_started` yet. `apps/web/src/components/LiveTab.test.jsx` exists. +- Zoom-client spec `e2e/zoom-client/timer-core.md` covers `LiveTab.jsx` and `zoomSdk.js`; + TC-10 already checks a `timer_started` in PostHog, so there is precedent for event evidence. + `timer-sounds.md` also lists `LiveTab.jsx`; its steps don't involve the event. + +## Approach +In `handleStart`, keep `startTimer()` synchronous and unchanged. In the idle callback, call +`getZoomParticipants()`, then send `timer_started` with the existing properties plus +`audience_size: participants.length` only when `!restricted && participants.length > 0`. +If the fetch rejects, send the event without the property (`.catch` → empty result), so a +failed participant read never loses a start. Capture `speakerName`/`selectedRole`/`rules` +before the await (they already are, by closure). About 10 lines in one file. + +Rejected: +- `useZoomParticipants()` in `LiveTab` (the issue's original diff): count frozen at app open, + and a third mount-time fetch. +- Polling or subscribing to participant changes: more SDK traffic for a number only needed + at START. +- Sending `audience_size: 0` or `1` when withheld: the issue wants it omitted, and analytics + already treats missing as 1. +- Adding it to `speech_finished` / `timer_continued`: out of scope for the issue. + +## Acceptance criteria +- AC-1: Given the app runs in a Zoom meeting as host or co-host and `getZoomParticipants` + reports N ≥ 1 participants with `restricted: false`, when the organizer presses START, + then `timer_started` carries `audience_size: N` alongside the unchanged `speaker_name`, + `role` and `timing_rules`. +- AC-2: Given the participant count changes after the app opened (people join), when START + is pressed, then `audience_size` is the count read at START, not at mount. +- AC-3: Given the organizer is neither host nor co-host (`restricted: true`), when START is + pressed, then `timer_started` is sent without `audience_size`. +- AC-4: Given no Zoom SDK (the Zoom app in a plain browser, or the web timer at + `apps/web`), when START is pressed, then `timer_started` is sent without `audience_size`. +- AC-5: Given `getZoomParticipants` rejects, when START is pressed, then the timer still + starts immediately and `timer_started` is still sent, without `audience_size`. *(proposed)* +- AC-6: Given the build is released to production, the PostHog "Timer reach (daily)" + insight shows reach above starts on days with multi-person meetings. *(post-release; + not checked by this PR)* + +## Test list +| AC | Unit | Playwright | Zoom-client spec | +|---|---|---|---| +| AC-1 | `apps/zoom-app/src/components/LiveTab.test.jsx`: mock 3 participants, START, run idle timer, `trackEvent` called with `timer_started` incl. `audience_size: 3` and the old props | — | `timer-core` NEW TC-11 (auto): host presses START; PostHog `timer_started` has `audience_size` equal to Zoom's Participants count | +| AC-2 | same file: mock returns 1 participant first, 4 after render; START → `audience_size: 4`, and `getZoomParticipants` called after the press | — | `timer-core` TC-11 (open the app first, then a second participant joins if available; else note it) | +| AC-3 | same file: `restricted: true` with self only → no `audience_size` key | — | `timer-core` NEW TC-12 (human): second Zoom account, not host/co-host, presses START; event has no `audience_size` | +| AC-4 | same file: `participants: []` → no key. `apps/web/src/components/LiveTab.test.jsx`: `timer_started` props have no `audience_size` | existing `e2e/zoom.timer-flow.spec.js` START flow keeps passing (no SDK path) | — | +| AC-5 | same file: `getZoomParticipants` rejects → `timer_started` still sent without the key; timer running right after the click | — | — | +| AC-6 | — | — | — (post-release; listed under "Not verified" in the PR) | + +Spec edit: add TC-11 and TC-12 to `e2e/zoom-client/timer-core.md` (no renumbering), update +the INDEX row's "Covers" text, run `npm run test:specs`. + +## Risk +Low. Analytics-only change in `apps/zoom-app/src/components/LiveTab.jsx`; START itself is +not touched, and the event path catches its own failure. One extra +`getUserContext`/`getMeetingParticipants` pair per START (a non-host gets a logged warning +per START). No `config.json` `riskHigh` paths. + +## Open questions +- Q1: The acceptance criteria are the issue's, rewritten as Given/When/Then; AC-2 and AC-5 + are new (AC-2 from the owner's comment, AC-5 proposed). Default: keep all six. +- Q2: Does deriving a participant count from `getMeetingParticipants` need a line in the + privacy policy or the Marketplace data-use answers? Only a number leaves the device. + Default: no policy change in this PR; flag it in the PR's "Not verified" for the human. +- Q3: The dev test meeting usually has only the host, so TC-11 would see `audience_size: 1`. + Default: accept 1 as proof the property is attached (it matches Zoom's own count); AC-2 is + proven by the unit test, and TC-11 checks a larger count only if a second participant is + present. +- Q4: A non-host gets one "Participant list needs host or co-host" warning in the debug log + per START. Default: accept it; don't add a cache or role pre-check. diff --git a/apps/web/src/components/LiveTab.test.jsx b/apps/web/src/components/LiveTab.test.jsx index de0bfd3..cb41e9b 100644 --- a/apps/web/src/components/LiveTab.test.jsx +++ b/apps/web/src/components/LiveTab.test.jsx @@ -4,6 +4,7 @@ import LiveTab from './LiveTab'; import { renderWithProviders } from '../test/helpers'; import { useEffect } from 'react'; import { useTimer } from '../context/TimerContext'; +import { trackEvent } from '../utils/posthog'; describe('LiveTab', () => { describe('initial state', () => { @@ -96,6 +97,24 @@ describe('LiveTab', () => { expect(onTimerStart).toHaveBeenCalledTimes(1); }); }); + + // #107: the web timer has no Zoom meeting to count, so reach falls back to + // one per start. A guessed audience_size here would inflate it. + it('sends timer_started without audience_size', async () => { + const user = userEvent.setup(); + trackEvent.mockClear(); + renderWithProviders(); + + await user.click(await screen.findByRole('button', { name: /start/i })); + + await waitFor(() => { + expect(trackEvent).toHaveBeenCalledWith('timer_started', expect.anything()); + }); + const calls = trackEvent.mock.calls.filter(([name]) => name === 'timer_started'); + expect(calls).toHaveLength(1); + expect(calls[0][1]).toMatchObject({ role: 'Standard Speech' }); + expect(calls[0][1]).not.toHaveProperty('audience_size'); + }); }); describe('stopping the timer', () => { diff --git a/apps/zoom-app/src/components/LiveTab.jsx b/apps/zoom-app/src/components/LiveTab.jsx index 16ea0b1..3fc6dcd 100644 --- a/apps/zoom-app/src/components/LiveTab.jsx +++ b/apps/zoom-app/src/components/LiveTab.jsx @@ -11,7 +11,7 @@ const CardImagesModal = lazy(() => import('./CardImagesModal')); const SoundsModal = lazy(() => import('./SoundsModal')); import TimeInput, { TimeInputModeToggle } from './TimeInput'; import { DEFAULT_ROLE_RULES, DEFAULT_CUSTOM_RULES, loadTimeInputMode, saveTimeInputMode, BREAK_ROLE, BREAK_QUICK_PICKS, DEFAULT_BREAK_SECONDS, deriveBreakRules, getDisplaySeconds, initCardImages } from '@toastmaster-timer/shared'; -import { getVideoState, setVideoState, applyOverlay, removeOverlay, clearVideoPipelines, isOverlayActive, getBackgroundUrl, getSdkStatus, setLogCallback, getOverlayMode, setOverlayMode, getOverlayTimePosition, setOverlayTimePosition, getOverlayTimeScale, setOverlayTimeScale, isOverlayTimeVisible, setOverlayTimeVisible, setOverlayTimeLabel, setPopoutChangeCallback, setShareChangeCallback, setAppShare, setAppPopout, isAppShareActive, isAppPoppedOut, isVideoOverlayMode, getClubBadgePlacement, setClubBadgePlacement, resetClubBadgePlacement, isClubBadgeMoved, OVERLAY_MODE_CARD, OVERLAY_MODE_STAGE } from '../utils/zoomSdk'; +import { getVideoState, setVideoState, applyOverlay, removeOverlay, clearVideoPipelines, isOverlayActive, getBackgroundUrl, getSdkStatus, setLogCallback, getOverlayMode, setOverlayMode, getOverlayTimePosition, setOverlayTimePosition, getOverlayTimeScale, setOverlayTimeScale, isOverlayTimeVisible, setOverlayTimeVisible, setOverlayTimeLabel, setPopoutChangeCallback, setShareChangeCallback, setAppShare, setAppPopout, isAppShareActive, isAppPoppedOut, isVideoOverlayMode, getClubBadgePlacement, setClubBadgePlacement, resetClubBadgePlacement, isClubBadgeMoved, getZoomParticipants, OVERLAY_MODE_CARD, OVERLAY_MODE_STAGE } from '../utils/zoomSdk'; import { useClub } from '../hooks/useClub'; import { SOUNDS_LOCK_MESSAGES, useSoundsAccess } from '../hooks/useSounds'; import { formatTime, saveOverlayMode, saveStageClockHidden, loadStageClockHidden, saveRevealFaceWhenIdle, loadRevealFaceWhenIdle } from '@toastmaster-timer/shared'; @@ -630,16 +630,23 @@ export default memo(function LiveTab() { startTimer(); - // Track timer started event - (window.requestIdleCallback || setTimeout)(() => trackEvent('timer_started', { - speaker_name: speakerName || 'Unnamed', - role: selectedRole, - timing_rules: { - green: rules.green, - yellow: rules.yellow, - red: rules.red - } - })); + // Track timer started event. audience_size is the meeting's head count + // read at START (people join after the app opens); it is left out when + // Zoom withholds the list (non-host), there's no SDK, or the read fails. + (window.requestIdleCallback || setTimeout)(() => { + getZoomParticipants() + .catch(() => ({ participants: [], restricted: true })) + .then(({ participants, restricted }) => trackEvent('timer_started', { + speaker_name: speakerName || 'Unnamed', + role: selectedRole, + timing_rules: { + green: rules.green, + yellow: rules.yellow, + red: rules.red + }, + ...(!restricted && participants.length > 0 && { audience_size: participants.length }), + })); + }); }; const handleContinue = () => { diff --git a/apps/zoom-app/src/components/LiveTab.test.jsx b/apps/zoom-app/src/components/LiveTab.test.jsx index fe736d2..048af4b 100644 --- a/apps/zoom-app/src/components/LiveTab.test.jsx +++ b/apps/zoom-app/src/components/LiveTab.test.jsx @@ -4,7 +4,8 @@ import { ToastProvider } from '../context/ToastContext'; import { TimerProvider } from '../context/TimerContext'; import LiveTab from './LiveTab'; import { BREAK_ROLE, DEFAULT_ROLE_RULES, deriveBreakRules } from '@toastmaster-timer/shared'; -import { applyOverlay, clearVideoPipelines, isOverlayActive } from '../utils/zoomSdk'; +import { applyOverlay, clearVideoPipelines, isOverlayActive, getZoomParticipants } from '../utils/zoomSdk'; +import { trackEvent } from '../utils/posthog'; // Stubbed rather than imported: the real module pulls in @zoom/appssdk, which // hangs vitest under jsdom. Faithful to the real return shapes, since the tab @@ -230,3 +231,124 @@ describe('the starting role when Standard Speech is removed', () => { expect(applyOverlay).toHaveBeenCalled(); }); }); + +// Reach: how many people a START served, not how many times it was pressed. +// Only a count leaves the device, and only when Zoom gave us the whole list. +describe('timer_started and the meeting audience (#107)', () => { + const NO_PARTICIPANTS = { participants: [], role: 'host', restricted: false }; + const people = (n) => Array.from({ length: n }, (_, i) => ({ id: `p${i}`, name: `Person ${i}` })); + const startButton = () => screen.getByRole('button', { name: /start/i }); + const STANDARD = DEFAULT_ROLE_RULES['Standard Speech']; + + afterEach(() => { + // Back to the module mock's default so other suites see an empty list. + getZoomParticipants.mockReset(); + getZoomParticipants.mockResolvedValue(NO_PARTICIPANTS); + }); + + /** Let the idle callback and any participant read behind it run to completion. */ + async function flushIdleWork() { + await act(async () => { + await vi.advanceTimersByTimeAsync(100); + }); + await act(async () => { + for (let i = 0; i < 10; i += 1) await Promise.resolve(); + }); + } + + /** + * Open the app with `atOpen` in the meeting, let the mount-time reads + * (the speaker suggestions) finish, then forget them: every assertion on + * getZoomParticipants below is about the read START makes. + */ + async function openApp(atOpen = NO_PARTICIPANTS) { + getZoomParticipants.mockResolvedValue(atOpen); + renderLive(); + await flushIdleWork(); + getZoomParticipants.mockClear(); + } + + const timerStartedCalls = () => trackEvent.mock.calls.filter(([name]) => name === 'timer_started'); + + /** The one timer_started event's properties; fails if it was not sent exactly once. */ + function timerStartedProps() { + const calls = timerStartedCalls(); + expect(calls).toHaveLength(1); + return calls[0][1]; + } + + it('AC-1: carries audience_size for a host with the full list, alongside the usual properties', async () => { + const meeting = { participants: people(3), role: 'host', restricted: false }; + await openApp(meeting); + getZoomParticipants.mockResolvedValue(meeting); + pickFromAgenda('Alice'); + + fireEvent.click(startButton()); + await flushIdleWork(); + + expect(timerStartedProps()).toEqual({ + speaker_name: 'Alice', + role: 'Standard Speech', + timing_rules: { green: STANDARD.green, yellow: STANDARD.yellow, red: STANDARD.red }, + audience_size: 3, + }); + }); + + it('AC-2: counts the people in the meeting at START, not when the app opened', async () => { + await openApp({ participants: people(1), role: 'host', restricted: false }); + // Three more people join after the app opened. + getZoomParticipants.mockResolvedValue({ participants: people(4), role: 'host', restricted: false }); + + fireEvent.click(startButton()); + await flushIdleWork(); + + expect(getZoomParticipants).toHaveBeenCalled(); + expect(timerStartedProps()).toMatchObject({ audience_size: 4 }); + }); + + it('AC-3: leaves audience_size out when the organizer is neither host nor co-host', async () => { + const meeting = { participants: people(1), role: 'attendee', restricted: true }; + await openApp(meeting); + getZoomParticipants.mockResolvedValue(meeting); + + fireEvent.click(startButton()); + await flushIdleWork(); + + // The read at START was made, so leaving the count out is the result of + // `restricted`, not of never asking. + expect(getZoomParticipants).toHaveBeenCalled(); + const props = timerStartedProps(); + expect(props).not.toHaveProperty('audience_size'); + expect(props).toMatchObject({ role: 'Standard Speech' }); + }); + + it('AC-4: leaves audience_size out when there is no Zoom SDK (empty participant list)', async () => { + const noSdk = { participants: [], role: '', restricted: false }; + await openApp(noSdk); + getZoomParticipants.mockResolvedValue(noSdk); + + fireEvent.click(startButton()); + await flushIdleWork(); + + expect(getZoomParticipants).toHaveBeenCalled(); + const props = timerStartedProps(); + expect(props).not.toHaveProperty('audience_size'); + expect(props).toMatchObject({ role: 'Standard Speech' }); + }); + + it('AC-5: starts at once and still sends timer_started, without audience_size, when the participant read fails', async () => { + await openApp({ participants: people(2), role: 'host', restricted: false }); + getZoomParticipants.mockRejectedValue(new Error('SDK init failed')); + + fireEvent.click(startButton()); + // START never waits on Zoom: the timer is running before any idle work. + expect(screen.getByRole('button', { name: 'FINISH' })).toBeInTheDocument(); + + await flushIdleWork(); + + expect(getZoomParticipants).toHaveBeenCalled(); + const props = timerStartedProps(); + expect(props).not.toHaveProperty('audience_size'); + expect(props).toMatchObject({ role: 'Standard Speech' }); + }); +}); diff --git a/e2e/zoom-client/INDEX.md b/e2e/zoom-client/INDEX.md index 65f0f53..ce9125d 100644 --- a/e2e/zoom-client/INDEX.md +++ b/e2e/zoom-client/INDEX.md @@ -4,7 +4,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 | +| [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) | | [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 | diff --git a/e2e/zoom-client/timer-core.md b/e2e/zoom-client/timer-core.md index 3d49d91..4b5a033 100644 --- a/e2e/zoom-client/timer-core.md +++ b/e2e/zoom-client/timer-core.md @@ -1,7 +1,7 @@ --- id: timer-core title: Core timer in the Zoom client -status: draft +status: current covers: - apps/zoom-app/src/context/TimerContext.jsx - apps/zoom-app/src/components/LiveTab.jsx @@ -13,8 +13,8 @@ automated: needs: - dev deployed - camera on, in a meeting -verified_at: never -verified_on: never +verified_at: 2e965b6 +verified_on: 2026-11-11 (Zoom 7.2.5, macOS 26.7.2, dev a4c66603) --- # Core timer in the Zoom client @@ -35,10 +35,12 @@ Do: Apps in the sidebar, open Toastmaster Timer. Expect: it loads from the `zoom.` host (`zoom.timer-dev.simple-tech.app` on dev), not `www.`. Evidence: `reqs.sh` shows the page load on the `zoom.` host. -### TC-02 · auto · Green on start, count-up on video -Do: Live tab, pick "Table Topics: 1–1.5–2 min", press Start with camera on. -Expect: background turns green; elapsed time is drawn over the video and rises once a second -(a virtual foreground layer, not baked into the background). +### TC-02 · auto · Count-up on start, green at the threshold +Do: Live tab, pick "Table Topics: 1–1.5–2 min" (or any short role if the device hides it), +press Start with camera on. +Expect: elapsed time is drawn over the video at once and rises once a second (a virtual +foreground layer, not baked into the background). No colour card before the green threshold; +green from the green threshold. Evidence: screenshot of the video frame; debug log shows one `setVirtualBackground` call. ### TC-03 · auto · Yellow then red at the thresholds @@ -64,7 +66,9 @@ Expect: at most four new fixed colour backgrounds. None with a timestamp baked i Evidence: file count before and after. ### TC-06 · auto · Timer survives closing the app -Do: start a speech, run ~10 s, close the app with X (not the back arrow), wait ~5 s, reopen. +Do: start a speech, run ~10 s, close the app with ⋯ → Close current app, wait ~5 s, reopen +from Apps. The Apps panel's own X only hides the app: Zoom keeps the webview, so there is no +reload and no toast (seen 2026-11-11). Expect: a toast says the timer resumed; elapsed includes the closed seconds; card colour matches the elapsed time (yellow if the threshold passed while closed). Evidence: toast screenshot; saved timer state in `localStorage`. @@ -93,6 +97,26 @@ that role, and START runs the timer. No "Please set timing rules first" warning. Evidence: screenshot of the picker and the running timer; a `timer_started` event in PostHog carrying that role. +### TC-11 · auto · timer_started carries the meeting's audience size (#107) +Do: as host, open the app first; if a second participant is available, let them join after +the app is open. Then press START on any role. +Expect: the `timer_started` event for this START has `audience_size` equal to the count in +Zoom's Participants panel at the moment of the press (late joiners included), alongside the +usual `speaker_name`, `role` and `timing_rules`. A host-only test meeting gives +`audience_size: 1`; that is accepted as proof the property is attached. No names leave the +device, only the number. The timer starts at once, not after the participant read. +Evidence: PostHog `timer_started` for this START (`audience_size`, `zoom_meeting_id`); +screenshot of Zoom's Participants count at the press. Note whether a second participant was +present. + +### TC-12 · human · No audience_size for a non-host organizer (#107) +Do: a second Zoom account that is neither host nor co-host opens the app in the same meeting +and presses START. +Expect: the timer runs normally and `timer_started` is sent without `audience_size`. The debug +log shows one "Participant list needs host or co-host" warning for the press. +Evidence: PostHog `timer_started` from that account with no `audience_size`; debug log line. +Human because it needs a second signed-in Zoom account. + ## Not covered - Colour accuracy on every camera/background setting. - A second participant seeing the video (needs a second computer). @@ -102,3 +126,5 @@ PostHog carrying that role. the `sounds` flag is on and the account is Pro). See [timer-sounds](timer-sounds.md). - Agenda import, and the roles it assigns to imported lines (`importBulkSpeakers` in `TimerContext.jsx`). See [agenda-import](agenda-import.md). +- A failed participant read at START (`timer_started` still sent, no `audience_size`). The + Zoom client can't be made to fail it on demand; `LiveTab.test.jsx` covers it (#107 AC-5). diff --git a/e2e/zoom-client/timer-sounds.md b/e2e/zoom-client/timer-sounds.md index 20e4a7f..1374bb6 100644 --- a/e2e/zoom-client/timer-sounds.md +++ b/e2e/zoom-client/timer-sounds.md @@ -113,3 +113,5 @@ log. - The web timer's chime (`apps/web/src/components/TimerSounds.jsx`). It runs in a plain browser, which `e2e/` covers. - Sound on Zoom mobile and web clients. +- START's `timer_started` analytics, including `audience_size` (#107). See + [timer-core](timer-core.md) TC-10 to TC-12.