Resolved: maximize while the modal loads
The first revision, 4ea9a1b5ee, introduced a wait for the modal chunk without guarding the pending action. Repeated clicks could open two modals, and closing the player while it loaded could still open the full player afterward. This was a P2 regression introduced by the PR.
The follow-up, e774e58c88, gives each request a token and ignores further taps while it is pending. After the import resolves, it checks the token, whether the player is still minimized, and the current track key. Closing the player clears the token through the state subscription; destruction clears it directly. The finally block releases only its own request.
Updated maximize action · Fix commit
Independent regression results
| While the import is pending | First revision | Updated revision |
|---|---|---|
| Tap maximize twice | 2 modal opens | 1 modal open |
| Close the player | 1 later modal open | 0 modal opens |
| Switch to a different track | 1 later modal open | 0 modal opens |
| Destroy the component | 1 later modal open | 0 modal opens |
I also checked a failed import followed by a retry. Failure leaves audio untouched and clears the pending guard; retry opens one modal. A cancelled request finishing after a newer request starts neither opens a modal nor clears the newer token.
The tests extract the relevant methods and getter from each revision with the TypeScript parser, transpile them, and replace the dynamic import with a controllable promise. They use stub modal and state services, including the close-state transition. These are executable method-level checks, not a browser or audio-playback test. Every listed result passed assertions.
The type-only modal import adds typing for the extracted helper without introducing a runtime import. Playback transfer stays in the helper and runs only after the pending request passes its checks.
The extraction is sound in scope
The PR converts 13 classes used by the shell, including the header, sidebar, user menu, profile display, two modals, and their pipes and directive. SharedModule imports those classes and preserves its existing exports. Consumers can keep importing SharedModule while later PRs replace those imports.
AppModule now names the shell components directly. The new provider helper preserves SharedModule's explicit provider list, supplies selected transitive library providers, and registers the X and SMS icons through an environment initializer. That addresses the root-injector dependency we flagged during planning.
The follow-up changes only the minimized player. The shell and provider extraction is unchanged from the first review, where source inspection and compilation found no additional defect. I did not independently repeat the author's complete runtime comparison of all 25 provider tokens.
What the size reduction means
The author reports initial QA JavaScript falling from 10.09 MB to 6.86 MB raw, and from 2.33 MB to 1.61 MB gzip. Those are the author's base-versus-PR measurements. I have not independently repeated that comparison.
The same note says 41 lazy routes still import SharedModule and sampled pages save about 12 KB of downloaded JavaScript. Moving code out of the initial bundle does not remove it from those pages. Treat this PR as preparation for smaller page downloads. The next useful change is to replace SharedModule imports on small public pages with the components, directives, and pipes they actually use.
Review checks
| Check | Result |
|---|---|
| Review scope | First review covered all 17 changed files. This review checked the complete follow-up diff and the player state transitions at e774e58c88. |
| Angular compiler | Passed again at e774e58c88 with ngc -p tsconfig.app.json --noEmit and a 10 GB heap limit. |
| QA application build | Passed again at e774e58c88. Emitted 42 warnings: 41 CommonJS dependency warnings and one warning about two skipped CSS selectors. The updated PR description reports the same warnings on base; I did not rebuild base in this review. |
| Music-player ordering reproduction | Four scenarios compared against the first revision; failure/retry and stale-request isolation also passed. |
| Diff whitespace check | Passed. |
| Logged-in flows, browser playback, SSR comparison, white-label build, prerender | Not independently run in this review. |
The compiler and build use the checked-out PR source and the workspace's installed dependencies. The QA build skips prefetch and prerender, so it does not verify regenerated marketing data or static rendering.
Before merge
- Run a browser playback check: play, minimize, maximize, and confirm the track resumes at the correct position. The method-level race tests pass.
- Check the logged-in header and sidebar, delegate switching with OTP, and creator requirements. These are the main converted views.
- Open a payment or subscription modal and a share modal from a lazy page to check root providers and icons.
- Build and load one generated white-label project. The PR author's checklist also leaves this untested.
I would keep the SharedModule-first sequence. The code finding is resolved, and I found no new defect in the follow-up. The remaining items are validation gaps, not confirmed regressions.