Install global hooks only once per page; refresh stale docs

[carochacs]

Found during a cross-repository audit of the eight feedBack repos.

The bug

The Host may re-execute screen.js on plugin reload, which re-runs this IIFE. A second run must not execute any top-level statement with observable side effects, and before this PR it executed a lot of them.

The obvious half was five hooks installed on shared globals, which accumulated:

Site	Effect of a second evaluation
window.playSong	wrapped a second time around the already-wrapped version — every song load runs the wrapper twice
window.showScreen	same double-wrap
resize	fires twice per event
beforeunload	posts main-closed twice
pointerdown (capture)	closes viz popovers twice per click

The subtler half — caught in review, after a first attempt that guarded only those five sites — is that skipping hook installation is exactly what makes run #1's closures the live state, so anything run #2 builds is unreachable, and some of it actively fights run #1:

• The settings-sync wiring re-bound its change handlers. Those mutate run Add fade transitions around split-screen wrap on start/stop #2's layout/alwaysSplit, which run Port CSS Grid layout fix for multi-panel layouts (quad/6-panel non-interactive panels) #1's live playSong wrapper never reads — so toggling "Always Split" silently stopped working after a reload.
• _maybeResumeLanShare() couldn't see run Port CSS Grid layout fix for multi-panel layouts (quad/6-panel non-interactive panels) #1's _lanShare, so it opened a second relay socket and broadcaster for the same room. Worse, _ensureMainBroadcasterAndListener() reassigns ch.onmessage on the shared BroadcastChannel, clobbering run Port CSS Grid layout fix for multi-panel layouts (quad/6-panel non-interactive panels) #1's handler — the one that processes popup docked/closed, so a popped-out panel could no longer be re-docked.
• bootFollowerMode() / bootRemoteJoin() rebuilt a second layout in a follower or ?ss= viewer window.

The fix

Return out of the IIFE when window.__feedBackSplitscreenHooksInstalled is already set. The flag lives on window precisely because it has to outlive the re-execution that resets everything else; on a second run the first run's hooks — still bound to live closures — stay in place. Same shape as section_map's guard, applied at body level, which subsumes all five per-site guards.

A fresh page load has no flag, so crash-recovery share resume and follower boot still run.

plugin-runtime-idempotent.v1 is now declared in the manifest. Version 1.14.1 → 1.14.2.

Verification

A regression test loads screen.js twice against one shared window — the reload scenario — and asserts listener counts, wrapper identity, and settings-wiring count:

	resize	beforeunload	pointerdown	playSong re-wrapped	settings re-wired
before	2	2	2	yes	yes
after	1	1	1	no	no

The settings-wiring assertion is what catches the second class; confirmed it fails against the intermediate per-site-guard version. Suite 33/33.

Docs

got-feedBack/feedBack-plugin-splitscreen is the canonical repo and this checkout is a fork of it. The README cloned a personal fork (carochacs/…); an earlier revision of this PR moved it to get-flashbacks, which was still a fork and still wrong — installing from a fork makes the plugin fork-specific and stops it being useful to the repo it came from. It now installs from got-feedBack, as do all the README's other links.

CLAUDE.md gets the same correction plus the reasoning, since agents read it verbatim: canonical repo named explicitly, user-facing links kept upstream, day-to-day PRs still targeting this fork, and a note that non-fork-specific fixes are worth opening upstream too. Its Git/PR section previously pointed at the personal fork three times, including a literal gh pr create --repo carochacs/….

Also updated the viz-factory references (slopsmithViz_* → feedBackViz_*) in CLAUDE.md and six screen.js comments. No code change there — VIZ_FACTORY_PREFIXES has resolved feedBackViz_ first for a while; this was documentation drift only.

Compliance checks

compliance.yml adds three targeted spec checks, since this repo's ci.yml runs the suites and nothing else. Hardened after review: diff against the merge base rather than the base tip, compare the [Unreleased] section itself rather than checking the file was touched, count .css as functional source, and document that the guard job proves a guard exists rather than that every hotpath sits inside one. Belongs upstream in got-feedBack/.github's reusable-ci.yml — the file says so.

On the red DeepSource check

Not this PR's to fix, as far as I can tell. All four category grades are A, every inline finding raised against this diff is resolved (Pullfrog confirms all nine of its threads are now [OUTDATED], anchored to the intermediate guard version), and DeepSource's own summary says "Some issues found as part of this review are outside of the diff in this pull request." The sibling PRs carrying the identical new workflow pass cleanly — sectionmap#8 is grade A with Shell passing — so the blockers are pre-existing debt in this repo's 5,700-line screen.js. Clearing that is separate work on main; happy to take it as its own PR.

Also merged main, picking up the new LICENSE and #23's CLAUDE.md reconciliation, which merged cleanly against this PR's edit to the same section.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f851eaa-c307-4e7a-a41c-73ff9d31b285

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 4288ffb...f2622f9 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
JavaScript		Aug 13, 2026 3:09a.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[deepsource-io]

Variable name is too small

Short variable names affect code readability and complicate code refactoring, because of the difficulty in searching and replacing such short characters.

[deepsource-io]

Unexpected empty method 'appendChild'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty method 'add'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty method 'remove'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty method 'addEventListener'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty method 'appendChild'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty method 'setAttribute'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty async function 'original'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[deepsource-io]

Unexpected empty function 'original'

Having empty functions hurts readability, and is considered a code-smell. There's almost always a way to avoid using them. If you must use one, consider adding a comment to inform the reader of its purpose.

[pullfrog]

Important

The reload guard is applied per hook site, not at the module level: a second evaluation still runs the settings-sync wiring, _maybeResumeLanShare(), and the follower/remote boot blocks against a fresh module state that no live hook will ever read — leaving real fallout in the exact reload scenario this PR targets. See the section below.

Reviewed changes — Reviewed the full PR diff (6 files, 2 commits).

• Reload-idempotency guard (screen.js) — HOOK_KEY flag on window gates the five shared-global hook sites: the playSong/showScreen wrappers (assignment-guarded; definitions renamed to _ssPlaySong/_ssShowScreen), the resize/beforeunload listeners, and the pointerdown capture listener. The flag is set before any hook site and genuinely survives re-evaluation.
• Regression test (tests/screen.test.js) — loads screen.js twice against one shared window, asserting listener counts and wrapper identity stay constant. Exact assertions, not truthiness — confirmed it genuinely fails against the pre-fix code (matches the author's "not ok 33").
• Compliance workflow (.github/workflows/compliance.yml) — three self-skipping jobs (version-bump, reload-guard heuristic, CHANGELOG). The version-bump and guard jobs both pass against this PR; the CHANGELOG job self-skips (repo has no CHANGELOG.md). plugin-runtime-idempotent.v1 is a real manifest standard in this ecosystem (it appears in the slopsmith plugin-manifest template), so the plugin.json standards addition is legitimate.
• Docs refresh — README clone URL and CLAUDE.md Git/PR conventions moved from the carochacs/… fork to get-flashbacks/…; CLAUDE.md viz-factory references updated to feedBackViz_<id> (legacy slopsmithViz_ fallback), matching the actual VIZ_FACTORY_PREFIXES lookup at screen.js:296. Version bumped 1.14.1 → 1.14.2.

⚠️ A second evaluation still runs side-effecting top-level statements against a state no live hook reads

The guard keeps run #1's hooks alive (that's the design), which means run #1's closures are the live state — but run #2's IIFE body still executes its module-level side effects against a brand-new state that nothing will ever drive. The PR's premise that "module state resetting is harmless — it gets rebuilt" doesn't hold once hook installation is skipped: the new state is built but never read. Concretely, on a reload:

1. Settings changes silently no-op afterwards. The settings-sync block (screen.js:728-767) re-wires the change handlers. If the settings panel is re-rendered after the reload (the normal flow — the panel is usually closed during a reload), only run #2's handlers exist, and they mutate run #2's alwaysSplit/layout — which run #1's live playSong wrapper never reads (it reads its closure at screen.js:4599/4606). Toggling "Always Split" or the default layout after a reload does nothing, with no error. If the settings DOM persisted instead, the controls carry two handlers that both fire.
2. A live LAN share spawns a duplicate connection and breaks popup docking. _maybeResumeLanShare() (screen.js:5714) sees run #2's own _lanShare as null and connects a second relay socket to the same room plus a second broadcaster. Stop in the UI stops only run #1's socket; run #2's keeps broadcasting. Worse, _ensureMainBroadcasterAndListener() (screen.js:4092) reassigns ch.onmessage on the shared BroadcastChannel (its _mainChannelListenerAttached flag is module-local), clobbering run #1's handler — so popup docked/closed messages stop being processed and a popped-out panel can no longer be re-docked or its slot released.
3. Follower/remote boot re-runs. bootFollowerMode()/bootRemoteJoin() (screen.js:5711-5713) are not gated, so a reload in a follower or ?ss= viewer window rebuilds a second layout and re-registers the buildFollowerLayout beforeunload/resize listeners (screen.js:4973/4994).

The cleaner fix is to early-return the whole body when the flag is already set, which makes the per-site guards redundant and closes all of the above at once.

Technical details

# Gate the IIFE body behind the reload guard, not just the hook sites

## Affected sites
- screen.js:728-767 — settings-sync wiring re-attaches change handlers on reload; handlers mutate run #2's `alwaysSplit`/`layout`, which run #1's live wrapper reads as closures (screen.js:4599/4606). Post-reload settings toggles either double-fire (DOM persisted) or silently no-op (panel re-rendered).
- screen.js:5714 — `_maybeResumeLanShare()` re-runs on reload; its `if (FOLLOWER || REMOTE_JOIN || _lanShare) return` dedupe can't see run #1's live share. Second socket + second broadcaster; `_ensureMainBroadcasterAndListener()` (screen.js:4092) then overwrites `ch.onmessage` on the shared channel, killing run #1's popup dock/closed handling.
- screen.js:5711-5713 — `bootFollowerMode()` / `bootRemoteJoin()` re-run on reload in follower/remote windows, building a duplicate layout and re-registering buildFollowerLayout's beforeunload/resize listeners (screen.js:4973/4994).

## Required outcome
- A second evaluation of screen.js must not execute any top-level statement with observable side effects (DOM wiring, sockets, listeners, boot blocks).

## Suggested approach
- Early-return at the top of the IIFE, before any other statement:
  `if (typeof window !== 'undefined' && window[HOOK_KEY]) return;` then set the flag. A fresh page load has no flag, so the full body (including crash-recovery share resume and follower boot) still runs; a plugin reload skips everything and run #1's live closures keep driving. The node test harness is unaffected because each case builds a fresh `global.window`. The `module.exports` block (screen.js:5719) is skipped by an early return on a second evaluation, which existing tests already tolerate.
- If per-site guards are kept for diff readability, at minimum gate `_maybeResumeLanShare()` and the settings-sync block on `!_hooksAlreadyInstalled` — but a body-level skip is strictly safer than enumerating sites.

## Open questions for the human
- Does the Host re-render `settings.html` on plugin reload? Both variants (persisted DOM → double handlers, re-render → silent no-op) are wrong; the wiring simply shouldn't run a second time.
- Are follower/`?ss=` windows expected to receive the same plugin reloads? If so, their boot blocks need the same treatment.

ℹ️ Nitpicks

• Same "stale docs" class the PR fixed in CLAUDE.md still lives in screen.js comments: screen.js:687, screen.js:719, and screen.js:1897 still reference window.slopsmithViz_* as if that were the primary factory name. Harmless (the legacy prefix is a valid fallback), but a one-line touch-up would match the PR's stated theme.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[pullfrog]

ℹ️ The refactor in this commit is clean and the suite still passes; it doesn't touch the guard logic, so the IMPORTANT finding from the initial review stands unchanged.

Reviewed changes — Incremental review of a510def ("Share the DOM stubs between tests; widen a re-indented variable name"), the delta since the previous review (8360492).

• Test-harness refactor (tests/screen.test.js) — makeDocumentStub(onAddEventListener) plus a shared noop replace the per-test inline stubs, and loadPlugin() dedupes the require-cache dance. The reload test deliberately builds one shared window with a tally counting listener and makeDocumentStub(tally), with a comment explaining why it must not use freshPlugin(). Verified the final stub has no duplicate keys (the range-diff rendering was misleading) and the exact-count deepEqual / wrapper-identity assertions are preserved.
• Variable rename (screen.js) — t → target inside the guarded pointerdown handler, condition wrapped across lines; behavior identical.

Verified with node --check screen.js and node tests/screen.test.js (33/33 pass).

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[carochacs]

Pushed 426767f. Two rounds of review addressed, plus a merge from main.

The guard is now body-level, not per-site — @pullfrog was right, and the original was genuinely incomplete rather than merely inelegant. Skipping hook installation is exactly what makes run #1's closures the live state, so three top-level blocks were still executing on a second evaluation and building state nothing reads:

• the settings-sync wiring re-bound its change handlers, which mutate run Add fade transitions around split-screen wrap on start/stop #2's layout/alwaysSplit — so "Always Split" silently stopped working after a reload;
• _maybeResumeLanShare() couldn't see run Port CSS Grid layout fix for multi-panel layouts (quad/6-panel non-interactive panels) #1's _lanShare, so it opened a second relay socket and broadcaster for the same room, and _ensureMainBroadcasterAndListener() clobbered ch.onmessage on the shared BroadcastChannel — the handler that processes popup docked/closed, so a popped-out panel could no longer be re-docked;
• bootFollowerMode()/bootRemoteJoin() rebuilt a second layout in follower and ?ss= windows.

Returning out of the IIFE subsumes all five per-site guards, which are gone. The test now also counts settings-control wirings — that assertion is what catches this class, and it fails against the previous per-site version.

Compliance workflow hardened after the sibling reviews: diff against the merge base rather than the base tip (the two-dot diff would fail a stale branch for a bump it doesn't owe), require the [Unreleased] section itself to change rather than merely touching CHANGELOG.md, count .css as functional source, and record in the file that the guard job proves a guard exists, not that every hotpath sits inside one.

Also merged main — picking up the new LICENSE (which independently closes one of the audit's findings) and #23's CLAUDE.md reconciliation, which turned out not to conflict with this PR's edit to the same section.

On the red check: DeepSource: JavaScript is still failing, and I don't think it's this PR's to fix. All four category grades are A, every inline finding it raised against my diff is resolved, and its own summary says "Some issues found as part of this review are outside of the diff in this pull request." The sibling PRs carrying the identical new workflow pass DeepSource cleanly — sectionmap#8 is grade A with Shell passing — so the blockers are pre-existing in this repo's 5,700-line screen.js rather than anything introduced here. Clearing them is a separate piece of work on main, and I'd rather not fold an unrelated cleanup into a correctness fix. Happy to take it on as its own PR if you want it.

Suite is 33/33 with the double-load harness confirming hooks install exactly once.

---

Generated by Claude Code

[pullfrog]

✅ No new issues found.

Reviewed changes — Incremental review of d991f5e ("fix(reload): gate the whole IIFE body, not just the hook sites"), the delta since the prior pullfrog review (a510def).

• Body-level reload gate (screen.js) — replaced the per-site _hooksAlreadyInstalled guards with an early return at the top of the IIFE when window.__feedBackSplitscreenHooksInstalled is already set. The playSong/showScreen wrappers revert to unconditional assignment (original names restored) and the resize/beforeunload/pointerdown capture listeners install unconditionally on a first run. This closes the reload gaps the prior review flagged: the settings-sync re-wiring (silent no-op of Always Split/layout after reload), the duplicate LAN-share relay socket plus the ch.onmessage broadcast-channel clobber (popup re-dock dying), and the follower/remote boot re-run.
• Test strengthening (tests/screen.test.js) — added a settingsWirings counter asserting the settings-sync change handler is wired exactly once across two evaluations of the shared-window reload test.
• Stale-comment cleanup (screen.js) — updated the remaining window.slopsmithViz_* comment references to feedBackViz_*, addressing the prior review's nitpick.

Verified node --check screen.js and node tests/screen.test.js (33/33 pass). Also confirmed the new settingsWirings assertion genuinely fails against the pre-d991f5e screen.js (second evaluation must not re-wire settings handlers, expected 1) — the coverage is real, not theatre.

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — Incremental review of the commits landed since the prior pullfrog review (d991f5e): 55bfd6f ("ci(compliance): diff against the merge base, and check [Unreleased] itself") and the 426767f merge of origin/main.

• Merge-base diffing (compliance.yml) — the version-bump and changelog jobs now resolve BASE through git merge-base "$BASE" HEAD before diffing, fixing a real false-positive: on pull_request, github.event.pull_request.base.sha is main's live tip, so a two-dot diff would count files main advanced after the fork as this PR's functional changes and demand a bump it doesn't owe. Fallback || echo "$BASE" covers the no-common-ancestor edge. Consistent between the two diffing jobs.
• [Unreleased] section comparison (compliance.yml) — the changelog job now compares the extracted [Unreleased] section between merge-base and HEAD instead of "file was touched", catching the exact slip of editing an already-released section. The awk extraction correctly stops at the next ## [ header; a release cut that empties [Unreleased] still counts as a change (documented in the error message).
• .css in functional-source extensions — both jobs now treat CSS as functional source requiring a version bump / changelog entry. Consistent policy call.
• Guard-heuristic limitation comment (compliance.yml) — documents that the reload-guard job proves a guard exists, not that every hotpath sits inside one. Honest framing of the known heuristic floor.
• Merge of origin/main — range-diff confirms the merge introduced no content changes to the other files; the plugin code, tests, and docs are byte-identical to the previously approved d991f5e state.

Re-verified node --check screen.js and node tests/screen.test.js (33/33 pass) on the merged HEAD; working tree clean.

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — Incremental review of 55bfd6f plus the main merge (426767f), the delta since the prior pullfrog review (d991f5e).

• Compliance workflow hardening (.github/workflows/compliance.yml) — the version-bump and changelog jobs now diff against the merge base (git merge-base "$BASE" HEAD) rather than the base tip, so a stale branch isn't failed for bumps main acquired after it forked; the changelog job compares the [Unreleased] section body itself instead of file touch, so editing an already-released section no longer slips through; .css counts as functional source in both jobs; the guard job documents its known limitation (proves a guard exists, not that every hotpath sits inside one).
• Merge of main — pulls in the new LICENSE (MIT) and #23's CLAUDE.md reconciliation, which merged cleanly against this PR's edit to the same section: the merge-base diff (4288ffb..HEAD) is exactly the PR's intended changes, nothing from either side dropped.

Verified node --check screen.js and the full suite (33/33, double-load harness). Simulated both compliance jobs against this PR: version-bump passes (merge-base 4288ffb, 1.14.1 → 1.14.2, screen.js functional), guard passes (hotpath detected + guard present), changelog self-skips (no CHANGELOG.md).

On the red DeepSource check: all nine of its inline threads on this PR are now [OUTDATED] — they anchor to the intermediate per-site-guard version (_hooksAlreadyInstalled, inline test stubs) that the body-level guard replaced — so the residual category-level failure is pre-existing screen.js debt outside this PR's diff, matching the analysis above.

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}