fix: close 3 remaining low-priority audit gaps (youtube lifecycle, catalog parity, a11y)#234
Merged
Conversation
…talog parity, a11y) Follow-up to the completeness audit — three independent, low-severity gaps a deep-read + adversarial-verify pass confirmed as real (a fourth, the Firefox strict_min_version, was investigated and dismissed: 121 is conservative vs the true ~109 API floor, so it over-restricts rather than over-promises). 1. YouTube subtitle-manager lifecycle (src/content/content.js) YouTubeSubtitleManager.destroy() existed + was unit-tested but was NEVER called, so SPA-navigating onto a Skilljar certification page left its MutationObserver + global window 'message' listener alive for the page's lifetime. Wire a symmetric lifecycle: destroy()+null on the cert-disable branch of onRouteChange, and rebuild a fresh manager (via a new ensureSubtitleManager() helper, also used at init) when returning to a normal lesson — the manager has no in-place reset, so a bare destroy() without re-creation would silently kill subtitles for the rest of the session. New e2e youtube-lifecycle.spec.js drives create→teardown→rebuild via the read-only _sb.hasSubtitleManager seam (revert-checked). 2. Catalog partial translation (src/data/*.json + check-dict-coverage.js) Two course titles — "Claude with Amazon Bedrock" and "Claude with Google Cloud's Vertex AI" — sat untranslated (value == English key) in 7 of 12 catalogs while the SAME files' cloudDeployment section translated them. Fill the 14 catalog values from each locale's own cloudDeployment translation (brand tokens stay English, as they already do there). Add Check 6 to check-dict-coverage.js (run in CI) to fail any catalog key translated in some dicts but left English in others — uniform passthrough like "Claude 101" is not flagged. (The terms Vertex AI / Bedrock themselves are correct protected passthroughs; only the title strings were the gap.) 3. Accessibility (keyboard-shortcuts.js, popup.html, constants.js) The shortcuts help overlay had no dialog semantics; add role="dialog", aria-modal, and aria-labelledby pointing at the (now id'd) title, plus a translated aria-label on the icon-only × button (new SHORTCUT_LABELS.close, 13 locales — the glyph alone announces as "multiplication sign"). The popup language <select> label gains for="lang-select" so it has an accessible name. New e2e a11y.spec.js + unit popup-a11y.test.js (both revert-checked). All green: lint, format, every check:* gate, 559 unit, 25 e2e.
The companion Claude Code plugin re-exposes src/data/*.json; the catalog title fix (7 locales) made the bundled terms.*.json copies stale. Rebuilt via `npm run build:plugin` (no prettier on generated files).
heznpc
added a commit
that referenced
this pull request
Jun 17, 2026
…237) An adversarial review of PRs #234-236 found no runtime bugs (the youtube lifecycle, dict reverts, a11y changes, and Check 7 all verified correct), but flagged two test/gate-quality issues + one harness nit: - Check 7 negative test was VACUOUS: it set de.claudePlatform.Skills = "Fähigkeiten", which is already the current value, so the mutation was a no-op and the test proved nothing. Rewrite it as a real-data assertion that asserts its precondition explicitly (de translates "Skills"; ja's _protected registers it; de's does not) so it can never go vacuous, then proves the full run stays clean — locking that Check 7 is per-locale, not a global term blocklist. - Document Check 7's SCOPE: it catches protected terms used as standalone section KEYS; embedded brand tokens inside longer strings are owned by the runtime restore pass (a substring scan would false-positive). - shortcutsOverlayA11y op now guards on _sb/toggleShortcutsHelp before calling, matching the other harness ops. lint · format · check:dict-coverage · 9 dict-coverage unit tests · a11y e2e — green.
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.
Follow-up to the completeness audit. A deep-read + adversarial-verify pass confirmed three independent, low-severity gaps as real. A fourth — Firefox
strict_min_version— was investigated and dismissed: 121 is conservative vs the true ~109 API floor (the build dropsservice_worker), so it over-restricts rather than over-promises.1. YouTube subtitle-manager lifecycle
YouTubeSubtitleManager.destroy()existed and was unit-tested but was never called — SPA-navigating onto a Skilljar certification page left itsMutationObserver+ globalwindow'message'listener alive for the page's lifetime.destroy()+ null on the cert-disable branch ofonRouteChange, and rebuild a fresh manager (via a newensureSubtitleManager()helper, also used at init) when returning to a normal lesson. The manager has no in-place reset, so a baredestroy()without re-creation would silently kill subtitles for the rest of the SPA session — the re-creation prevents that regression.youtube-lifecycle.spec.jsdrives create → teardown → rebuild via a read-only_sb.hasSubtitleManagerseam. Revert-checked (fails on the teardown assertion without the fix).2. Catalog partial translation
Two course titles —
Claude with Amazon BedrockandClaude with Google Cloud's Vertex AI— sat untranslated (value == English key) in 7 of 12 catalogs, while the same files'cloudDeploymentsection translated them.cloudDeploymenttranslation (brand tokens stay English, as they already do there).check-dict-coverage.js(already run in CI) to fail any catalog key translated in some dicts but left English in others. Uniform passthrough (Claude 101) is not flagged. The terms Vertex AI / Bedrock themselves are correct protected passthroughs — only the title strings were the gap.3. Accessibility
role="dialog",aria-modal,aria-labelledby(title now id'd), and a translatedaria-labelon the icon-only×button (newSHORTCUT_LABELS.close, 13 locales — the glyph alone announces as "multiplication sign").<select>label gainsfor="lang-select"for an accessible name.a11y.spec.js+ unitpopup-a11y.test.js, both revert-checked.Validation
lint · format · every
check:*gate (incl. new Check 6) · 559 unit · 25 e2e — all green. Each fix independently revert-checked.