Plan for upgrading core to modern react#430
Open
andrew-polk wants to merge 10 commits into
Open
Conversation
Phased plan: safety net, in-place refactoring of the class, removal of static back-channels, MUI v5 and Swiper upgrades, hooks conversion, and the React 18 upgrade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 500ms post-load timeout in finishUp dereferenced this.swiperInstance unconditionally. It is null if the player was unmounted before the timer fired, and never set at all for a book that shows the required-version message, producing an uncaught TypeError. Also, the retry loops in showingPage and addScrollbarsToPageWhenReady rescheduled themselves every 50ms forever once swiper was destroyed, since the page they wait for can never appear. Found while adding component-level tests (react modernization plan, Phase 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Phase 0 of the react modernization plan: pin down the externally observable behavior of BloomPlayerCore before refactoring it. The tests render the real component in jsdom with axios mocked to serve a fixture book, covering: loading by folder and .htm url, lazy page placeholders, load-failure and required-version UI, reportBookProperties and controlsCallback, pageChanged / slideNext / startPageIndex / #pageId navigation, and language switching. Adds @testing-library/react 12 (the last major supporting React 17), a fixture-book builder, and a vitest setup file working around two jsdom gaps (no <script> tag for the segment snippet, no innerText). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents the designed-but-not-yet-implemented Playwright e2e setup: standalone Playwright driving dist/bloomplayer.htm with a small static server over the checked-in public/testBooks fixtures, an initial suite of ~8 specs, and e2e steps in the release workflow. Also marks the Phase 0 component smoke tests as done. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds coverage for: %2f-encoded folder urls, landscape/portrait switching (for rotatable books), hideSwiperButtons, RTL books (swiper dir + isRtl report), activity pages included/skipActivities, quiz page generation from legacy questions.json, and forced-pause reporting via PlayFailed/ PlayUnblocked (BL-8864). Investigation note, documented in the test file: swiper 4's internal slides collection stays empty under jsdom, so showingPage's deferred side effects (reportPageProperties, narration start) are untestable here and belong to the planned e2e suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Once the equivalent Playwright specs are green in CI, the play() functions and BloomPlayerTester get deleted; storybook remains the dev environment and covers only what can't be asserted automatically (visual fidelity, motion quality, RTL appearance, touch devices, live-from-editor). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements E2E-TESTING-PLAN.md: 12 tests in e2e/ drive the real dist/bloomplayer.htm in headless chromium against the checked-in testBooks fixtures, served by a small node static server that also provides the /book/<instanceId> host rewrite (mirroring storybook). Covers load smoke, real swiper page turning, start-page / hideNavButtons / showBackButton url params, the language menu, blocked-autoplay narration recovery (BL-16146; autoplay blockage is simulated by stubbing HTMLMediaElement.play since headless chromium ignores --autoplay-policy), the full multi-book navigation and history flow ported from navigation.stories.tsx, quiz activity feedback, and iframe host embedding with postMessage assertions. Run with: pnpm build:standalone && pnpm test:e2e (or pnpm test:e2e:build). Added to the release workflow after unit tests; Playwright report uploads as an artifact on failure. Also scopes vitest away from e2e/ and raises its testTimeout to 15s: some component tests legitimately exceed the 5s default because they wait out the player's 500ms startup timer plus waitFor polls. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records the implementation notes that differ from the original design: headless chromium ignores --autoplay-policy (blocked path is tested by stubbing HTMLMediaElement.play), back button with showBackButton=true is enabled with empty history because it defers to the host, and the 500ms startup window has no DOM ready signal yet, so tests wait it out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The slideChange handler called setState({inPauseForced: false})
unconditionally. Swiper can emit slideChange from swiper.update(),
which react-id-swiper invokes on every React render, so the redundant
setState could become a render -> update -> slideChange -> setState
cycle, surfacing as 'Maximum update depth exceeded' during pnpm test
(and a needless extra render in production). Now it only sets state
when inPauseForced is actually set.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 500ms startup timer in finishUp (and the 200ms language-change timer, showingPage's deferred work, and the retry timers) kept running after the component unmounted and called setState on the dead instance, producing React's 'state update on an unmounted component' warning. Timers that touch state or swiper now go through setPlayerTimeout, which componentWillUnmount cancels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is
Devin review