fix(onboarding): hide DictationDemo when sample assets are absent - #153
Conversation
… follow-up) DubbingDemo and DemoPresetGrid already degrade gracefully (hide) when their assets / is_demo profiles are missing, but DictationDemo always rendered its three hardcoded cards — which fail on click without the bundled sample WAVs (rendered by scripts/build_demos.sh; absent in a plain source checkout). Add a mount-time HEAD probe of the first sample; if it's not present, hide the whole demo (mirrors DubbingDemo's missing-manifest behavior). When assets are present, behavior is unchanged. Test: HEAD 404 → demo renders nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesAsset Availability Detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| frontend/src/components/DictationDemo.jsx | Adds a mount-time HEAD probe of the first WAV asset; hides component when assets are absent. The null state (probing in-flight) renders the full demo UI rather than hiding it, which can cause a brief visible flash before the component disappears. |
| frontend/src/test/DictationDemo.test.jsx | New 404-probe test is correct and uses waitFor properly. Existing synchronous tests now leave the HEAD fetch uncontrolled, relying on assertions completing before the async resolution — technically passing but fragile against act() warnings. |
Sequence Diagram
sequenceDiagram
participant Browser
participant DictationDemo
participant Server
Browser->>DictationDemo: "mount (assetsAvailable = null)"
Note over DictationDemo: renders full UI while probing
DictationDemo->>Server: HEAD /demo_audio/dictation/en_conversational.wav
alt assets present (200 OK)
Server-->>DictationDemo: 200 OK
DictationDemo->>DictationDemo: setAssetsAvailable(true)
Note over DictationDemo: renders demo cards normally
else assets absent (404 / network error)
Server-->>DictationDemo: 404 / error
DictationDemo->>DictationDemo: setAssetsAvailable(false)
DictationDemo->>Browser: return null (component hidden)
end
Comments Outside Diff (1)
-
frontend/src/test/DictationDemo.test.jsx, line 24-35 (link)Existing tests leave an uncontrolled async fetch in-flight
'renders the three bundled scripts'and'shows the "no hotkey" warning...'do not mockglobal.fetch. The new mount-time HEAD probe fires on every render, so these tests now issue a real (or uncaught) network request. Because the assertions are synchronous they pass before the Promise chain resolves, but the subsequentsetAssetsAvailable(false)call races against@testing-library/react's cleanup and can produceact()warnings. Adding a{ ok: true }fetch mock tobeforeEach(analogous to what the probe test does explicitly) would make the probe deterministic for all tests.
Reviews (1): Last reviewed commit: "fix(onboarding): hide DictationDemo when..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const [assetsAvailable, setAssetsAvailable] = useState(null); | ||
| const audioRef = useRef(null); | ||
|
|
||
| // Probe whether the bundled dictation samples actually exist; hide the whole | ||
| // demo if not, mirroring DubbingDemo's missing-manifest behavior. | ||
| useEffect(() => { | ||
| let cancelled = false; | ||
| fetch(`${API}${SCRIPTS[0].wav}`, { method: 'HEAD' }) | ||
| .then((r) => { if (!cancelled) setAssetsAvailable(r.ok); }) | ||
| .catch(() => { if (!cancelled) setAssetsAvailable(false); }); | ||
| return () => { cancelled = true; }; | ||
| }, []); |
There was a problem hiding this comment.
While assetsAvailable is null (probe in-flight), the full demo — all three script cards, audio element, and hotkey effects — renders and is visible to the user. If the HEAD probe resolves to false, a brief flash of the unusable cards plays before the component hides itself. On a slow or lagging local server, the flash can be noticeable. Since the intent is to avoid ever showing the broken cards, returning null (or a lightweight placeholder) while the probe is pending would close the window entirely.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/test/DictationDemo.test.jsx (1)
16-35: ⚡ Quick winStub
fetchby default so the mount probe doesn't issue a real network call.With the new mount probe, every render of
DictationDemonow firesfetch(HEAD). The first two tests (Lines 24-29 and 31-35) don't setglobal.fetch, so the probe hits the real configured API base. The synchronous assertions still pass, but this introduces a real outbound request (rejecting in CI) and a possible unhandled-rejection/act warning — making these tests less deterministic. Add a default mock inbeforeEach.♻️ Default the probe to a benign mock
beforeEach(() => { originalFetch = global.fetch; + // Default: asset probe resolves "present" so renders are deterministic + // and no real network call leaks out. Tests override as needed. + global.fetch = vi.fn(() => Promise.resolve({ ok: true, status: 200 })); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/test/DictationDemo.test.jsx` around lines 16 - 35, Tests render of DictationDemo triggers the new mount probe which calls fetch(HEAD), causing real network requests; stub global.fetch in the existing beforeEach to a benign mock that returns a resolved Promise/Response (e.g., a minimal OK-like response) so the probe never issues a real outbound call, and keep the existing afterEach that restores originalFetch; implement the mock in the beforeEach near the existing originalFetch assignment and ensure it handles the HEAD probe used by the mount probe so the synchronous assertions in the tests for DictationDemo succeed deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/test/DictationDemo.test.jsx`:
- Around line 16-35: Tests render of DictationDemo triggers the new mount probe
which calls fetch(HEAD), causing real network requests; stub global.fetch in the
existing beforeEach to a benign mock that returns a resolved Promise/Response
(e.g., a minimal OK-like response) so the probe never issues a real outbound
call, and keep the existing afterEach that restores originalFetch; implement the
mock in the beforeEach near the existing originalFetch assignment and ensure it
handles the HEAD probe used by the mount probe so the synchronous assertions in
the tests for DictationDemo succeed deterministically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2811dc7e-fbbf-479a-b8c6-9efa8c8ab4c3
📒 Files selected for processing (2)
frontend/src/components/DictationDemo.jsxfrontend/src/test/DictationDemo.test.jsx
1 participant
Follow-up to #119/#133 onboarding demos.
Problem
DubbingDemoandDemoPresetGridalready hide themselves when their assets /is_demoprofiles are missing.DictationDemodidn't — it always rendered its three hardcoded cards, which then fail on click without the bundled sample WAVs (/demo_audio/dictation/*.wav, rendered byscripts/build_demos.shand absent in a plain source checkout).Fix
Mount-time
HEADprobe of the first sample; if it's not present, the component returnsnull(hides the whole demo), mirroringDubbingDemo's missing-manifest behavior. With assets present, behavior is unchanged.Test
DictationDemo.test.jsx: HEAD 404 → demo renders nothing (existing 3 tests still pass).No default-behavior change (when assets ship, the demo shows as before), no version bump.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests