Stabilize split-screen panel lifecycle + feedBack rebrand - #23
Open
carochacs wants to merge 8 commits into
Open
Conversation
…etector across highway swaps (#5) - Hide Lyrics/Detect/Channel toggle buttons while a panel is in Lyrics-pane or Jumping-Tab-pane mode, and restore them on exit (matches CLAUDE.md's documented button-hiding contract). - Destroy the per-panel lyrics overlay when entering Lyrics/JT mode (it was rendering on top of the full lyrics pane / JT pane) and restore it on exit if it was on. - Rebuild the per-panel note detector in recreatePanelHighway() so it stays bound to the live highway instance across arrangement switches and viz mode transitions, instead of being orphaned on a stopped one. - Guard popOutPanel() with the _starting in-flight check (same pattern as rebuildLayout()/_redockPanel()) so a pop-out can't race a still-building startSplitScreen(). - Null-check #audio in the tab-view getCurrentTime callback. Addresses items 2-4 and 7 of the shared-architecture bug audit filed in #4 (item 1, invert persistence, was already fixed in this repo). Co-authored-by: Claude <noreply@anthropic.com>
…d docs (#7) - README: title, intro line, install path/clone URL (was pointing at the stale topkoa/slopsmith-plugin-splitscreen fork instead of this repo), "runs on any feedBack core", and the Requirements section. - CLAUDE.md: "loaded by feedBack core", and the Git/PR conventions section (was entirely stale, pointing at topkoa/slopsmith-plugin-splitscreen and an "upstream" that isn't this repo's actual setup). - screen.js: the follower-orphaned overlay's user-visible fallback title ("Main Slopsmith window closed" -> "Main feedBack window closed"). Left untouched: window.slopsmithSplitscreen (back-compat alias consumers still read), the slopsmithViz_<id> factory naming convention, and BroadcastChannel('slopsmith-ss') — these are real external integration contracts other plugins depend on, not branding. Also left sibling-plugin repo links (Stems, Jumping Tab) and slopsmith#NNN issue references as-is since I can't verify whether those external repos/trackers were renamed. Co-authored-by: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe plugin is rebranded for feedBack. Split-screen runtime changes add five- and six-panel layouts, coordinated panel lifecycle handling, multi-panel redocking, and follower or remote-panel synchronization. ChangesSplit-screen updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Popup
participant MainWindow
participant SplitScreen
participant RemoteViewer
Popup->>MainWindow: send multi-panel state
MainWindow->>SplitScreen: restore or grow layout
SplitScreen->>RemoteViewer: synchronize follower panel state
RemoteViewer->>MainWindow: reconnect and resume remote session
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@screen.js`:
- Around line 1665-1667: Update exitLyricsMode() and exitJumpingTabMode() to
call _showVizControls(panel, pluginId) after restoring the mode-specific
controls and before hookPanelReady(). Ensure normal-arrangement transitions
restore each panel’s active visualization controls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a8e2bdf-67ad-47b2-962d-d557cf3f8e84
📒 Files selected for processing (3)
CLAUDE.mdREADME.mdscreen.js
Comment on lines
+1665
to
+1667
| panel.lyricsBtn.style.display = ''; | ||
| if (panel.detectBtn) panel.detectBtn.style.display = ''; | ||
| if (panel.channelBtn) panel.channelBtn.style.display = ''; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 8 'function (exitLyricsMode|exitJumpingTabMode|hookPanelReady)|_hideVizControls|_showVizControls' screen.jsRepository: got-feedBack/feedBack-plugin-splitscreen
Length of output: 9194
🏁 Script executed:
#!/bin/bash
set -eu
printf '--- hookPanelReady ---\n'
sed -n '1544,1608p' screen.js | cat -n
printf '\n--- exitLyricsMode section ---\n'
sed -n '1652,1698p' screen.js | cat -n
printf '\n--- exitJumpingTabMode section ---\n'
sed -n '1735,1810p' screen.js | cat -n
printf '\n--- select change handling ---\n'
sed -n '2008,2072p' screen.js | cat -nRepository: got-feedBack/feedBack-plugin-splitscreen
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1544,1608p' screen.js | cat -n
sed -n '1652,1698p' screen.js | cat -n
sed -n '1735,1810p' screen.js | cat -n
sed -n '2008,2072p' screen.js | cat -nRepository: got-feedBack/feedBack-plugin-splitscreen
Length of output: 13438
Restore visualization controls when exiting lyrics/jumping-tab modes.
enterLyricsMode() and enterJumpingTabMode() call _hideVizControls(panel), but exitLyricsMode() and exitJumpingTabMode() only reset the lyrics, detector, channel, and mastery controls. If the user leaves these modes by selecting a normal arrangement, _showVizControls(panel, pluginId) is never called, so the playlist/transport highway may lose its per-panel vis controls after the first transition. Restore the active viz controls in the exit paths before calling hookPanelReady().
🤖 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 `@screen.js` around lines 1665 - 1667, Update exitLyricsMode() and
exitJumpingTabMode() to call _showVizControls(panel, pluginId) after restoring
the mode-specific controls and before hookPanelReady(). Ensure
normal-arrangement transitions restore each panel’s active visualization
controls.
…eflow (#6) * fix: notify plugins when recreatePanelHighway() replaces panel.canvas recreatePanelHighway() (called on arrangement switches and viz mode enter/exit) discards the old <canvas> and creates a fresh one to sidestep the browser's per-canvas context-type lock, but never emitted splitscreen:panels-changed. The documented window.slopsmithSplitscreen API (panelIndexFor/panelChromeFor/settingsAnchorFor/isCanvasFocused) all resolve via canvas identity, and getPanels() hands consumers a canvas reference — any plugin (e.g. Camera Director) caching that reference would silently stop resolving the panel after the next arrangement switch or viz toggle, with no signal telling it to refresh. * fix: recover all sub-panels when docking a self-split popup; reflow on section-map resize - dockFollowerPanel() now captures every current sub-panel in the popup (finalStates, plural) instead of only the one whose Dock button was clicked. The main-side `popups` Map tracks one entry per popup WINDOW, not per sub-panel, so closing the window on a single sub-panel's dock previously discarded the other 1-3 sub-panels' state. - _redockPanel() now accepts finalStates and builds one newPrefs entry per captured sub-panel, growing `layout` to the smallest LAYOUTS entry that fits the resulting panel count (falls back to the largest available and toasts if it still doesn't fit). This also fixes a related bug where redocking a single panel into an already-full layout silently dropped it (startSplitScreen only ever builds LAYOUTS[layout].panels slots). - sizeCanvases() now lazily attaches a ResizeObserver to #section-map (the Section Map plugin's bar) so panels reflow when it changes height/visibility on its own, not just on the window resize / activation / layout-change / controls-toggle triggers sizeCanvases() was already called from. Resolves the last two items (5 and 6) of the shared-architecture bug audit tracked in #4. --------- Co-authored-by: Claude <noreply@anthropic.com>
Converts every existing single-line `//` comment immediately preceding a function declaration into a `/** ... */` JSDoc block, and adds a minimal JSDoc block (name-derived summary + @param placeholders) to the functions that had no preceding comment at all. 0/144 -> 144/144 functions now have a JSDoc block preceding them. Intended to satisfy the org's reusable CI docstring-coverage gate (got-feedback/.github/.github/workflows/reusable-ci.yml), which reported 37.50% against an 80% threshold on an upstream PR for this plugin (got-feedBack#23). This fork's CI has never actually run the reusable workflow (no workflow runs found), so this is a preemptive fix rather than a response to a failing check here. No behavior change — comment-only diff, verified with node -c and the existing test suite (25/25 passing). Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@screen.js`:
- Around line 3546-3555: Update _redockPanel so a missing currentFilename does
not silently discard the captured popup state: handle this condition before
deleting the popups entry, preserve the entry for a later redock, and provide
the same user-facing feedback used by the nearby overflow-drop path. Keep normal
redocking unchanged when currentFilename is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…#9) * fix: destroy live note detector when entering Lyrics/Jumping-Tab mode enterLyricsMode()/enterJumpingTabMode() hide detectBtn/channelBtn (fixed in #5) because they're meaningless once the highway is stopped and the canvas hidden, but a detector already running when the user switches into one of these modes kept running silently — hiding its only on/off control left no way to stop it from the UI, and detect on/off isn't persisted in prefs (only channel/device/offset are), so it's purely an in-session toggle with no other place tracking "should this still be on". Now toggleDetect(panel) runs (destroying it) whenever a live detector exists at mode entry, matching the button hide. Viz mode is unaffected — it keeps the highway alive and detectBtn/channelBtn visible by design. * fix: stop detector before the highway/controls in Lyrics/JT mode entry Addresses CodeRabbit review on #9: toggleDetect(panel) ran after panel.hw.stop() and after detectBtn was hidden, leaving the detector briefly bound to an already-stopped highway and updating button style after the button was already hidden. Moved the detector teardown up to run first, before the highway stops and its controls hide.
✅ Action performedReview finished.
|
_redockPanel() deleted the popups entry and bailed silently when currentFilename was empty, dropping the popped-out panel's state with no feedback — unlike the overflow-drop path a few lines down, which already toasts. Addresses a CodeRabbit review comment on upstream got-feedBack#23. Co-authored-by: Claude <noreply@anthropic.com>
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
carochacs
changed the title
carochacs
changed the title
* fix: switch quad/tri/five/six layouts to CSS grid (#1) Flex-wrap containers with %-height panel items left bottom-row panels non-interactive: the wrap's height comes from position insets, not an explicit height, which some browsers resolve as indefinite for % height inside a flex-wrap container. Confirmed this reproduces here too (tri-top/tri-bottom/quad all shared the same flexWrap:wrap + % height shape). Ported the sibling slopsmith fork's CSS grid approach (grid-template-columns/rows sized from cols/rows) for quad, tri-top, tri-bottom, and the popup follower window's quad layout, plus added five/six-panel layouts as the tracked follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: fade split-screen wrap in/out on start/stop (#2) Matches the fade convention already used elsewhere in this codebase (_showMainToast, player HUD fade): the wrap starts at opacity 0, fades to 1 once panels are built and sized at the end of startSplitScreen(), and fades back to 0 before the real teardown on the user-initiated Stop path (toggle() -> new _fadeOutWrapThenStop()). The navigation-driven auto-stop paths (song change, leaving the player) still call stopSplitScreen() directly and synchronously, since those rely on `active` flipping immediately - only the explicit user Stop gets the deferred fade-then-teardown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: add bulk "Dock all" button for popped-out panels (#3) Adds a Dock all button (visible only while >=1 panel is popped out, independent of `active` since popping out every panel can leave the main window inactive) that broadcasts a `dock-all` request over the existing BroadcastChannel. Each live popup answers by calling its own dockFollowerPanel() — the same path its own Dock button uses - so the redock carries live state, not a stale pop-out-time snapshot, and closes the popup itself. No extra single-flight guard was needed for the bulk case: _redockPanel already queues behind `_starting` on the main side, so concurrent `docked` replies from multiple popups serialise safely. Side-finding from the issue's guard concern: popOutPanel() already has the `_starting` guard (checked and no-ops with a toast if a start is in flight) - the "missing guard" premise didn't hold here, so no fix was needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@screen.js`:
- Around line 3440-3455: Prevent duplicate teardown when a stop is already
pending: update _fadeOutWrapThenStop and/or stopSplitScreen to track an
in-flight fade/stop or clear the existing timer before scheduling another
setTimeout. Ensure repeated toggle calls produce only one stopSplitScreen
execution and teardownPanels call, while preserving the immediate synchronous
stop behavior when no fade is needed.
In `@tests/screen.test.js`:
- Around line 321-329: Add an assertion in the _bestFitLayout test for n = 2,
verifying it returns the intended two-panel default layout while preserving the
existing coverage for one through six panels and the capped result for larger
values.
- Around line 273-284: The multi-panel layout test should verify grid capacity,
not merely positive dimensions. In the loop over LAYOUTS keys, assert that each
layout’s cols multiplied by rows is at least its configured panel count, using
the layout’s existing panel-count symbol or property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e66b72e-0131-49f9-9a5c-a82ba6ca4c84
📒 Files selected for processing (2)
screen.jstests/screen.test.js
_fadeOutWrapThenStop() scheduled a new setTimeout(stopSplitScreen) on every call, but active doesn't flip false until that timeout fires — so a second toggle() while the fade was in flight queued a second stopSplitScreen()/teardownPanels() run against already-torn-down state. Track the pending timer and no-op repeat calls; stopSplitScreen clears it too, so a direct (non-fade) stop can't leave a stale timer to fire against a later session. Also strengthens two existing tests per review: the multi-panel layout test now asserts each grid's cols*rows actually covers its panel count, and the _bestFitLayout test adds the n=2 case. Claude-Session: https://claude.ai/code/session_01NzB8E8ExunXPfJzNotqkx4 Co-authored-by: Claude <noreply@anthropic.com>
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
screen.js (1)
1196-1219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the new grid-span placement logic.
createPanel()assigns explicitgridColumnspans fortri-top,tri-bottom, andfivelayouts. The provided test evidence coversapplyLayoutStyleforquad,five,six,top-bottom, andleft-right, but notcreatePanel's per-index span assignment. A wrong span here silently breaks panel visibility/click targets, the exact class of bug this PR's grid migration is meant to fix.Add unit tests asserting
panelDiv.style.gridColumnfor each index intri-top,tri-bottom, andfivelayouts.🤖 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 `@screen.js` around lines 1196 - 1219, Add unit coverage for createPanel that asserts panelDiv.style.gridColumn for every index in the tri-top, tri-bottom, and five layouts: tri-top should span only index 0, tri-bottom only index 2, and five should use span 3 for indices 0–1 and span 2 for indices 2–4. Keep the existing behavior for non-spanning panels and avoid changing createPanel itself.
🤖 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 `@screen.js`:
- Around line 1196-1219: Add unit coverage for createPanel that asserts
panelDiv.style.gridColumn for every index in the tri-top, tri-bottom, and five
layouts: tri-top should span only index 0, tri-bottom only index 2, and five
should use span 3 for indices 0–1 and span 2 for indices 2–4. Keep the existing
behavior for non-spanning panels and avoid changing createPanel itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: daa3ec52-08b2-4bc2-8e1b-0fa6df3e1cc6
📒 Files selected for processing (2)
screen.jstests/screen.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/screen.test.js
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.
Panel lifecycle fixes (screen.js)
is in Lyrics-pane or Jumping-Tab-pane mode — they're meaningless once the
highway is stopped and the canvas is hidden.
leaving them running with no visible on/off control.
visibly on top of the full lyrics pane) and restore it on exit if it was on.
live highway instance instead of a stale, stopped one.
popOutPanel()against a race whereteardownPanels()could dismantlea half-built layout while
startSplitScreen()is still constructing it.itself into multiple panels; grow the layout to fit instead of silently
dropping panels past capacity.
loaded at dock time.
via a
ResizeObserverinstead of only on window resize.splitscreen:panels-changedwhen canvas identity changes (arrangementswitch, viz enter/exit), so consumers resolving panels by canvas reference
(e.g. Camera Director) don't silently desync.
getCurrentTime()callback.Rebranding (README.md, CLAUDE.md, screen.js)
"feedBack" — README title/intro/install path/clone URL, CLAUDE.md's
"loaded by feedBack core" and Git/PR conventions section, and the
follower-orphaned overlay's fallback title.
window.slopsmithSplitscreen(back-compat alias —window.feedBackSplitscreenis the canonical one already exposed alongsideit), the
slopsmithViz_factory naming convention,BroadcastChannel('slopsmith-ss'),and external
slopsmith#NNNissue links (can't verify those trackers were renamed).Docs (screen.js)
reusable CI docstring-coverage gate, which reported ~31% against an 80%
threshold.
Test plan
node -c screen.js— syntax validnode --test tests/screen.test.js— 25/25 passthe detector stops and re-enabling after switching back works
confirm all 4 return to the main window
instead of the panel silently vanishing
panels reflow without a window resize
Summary by CodeRabbit
New Features
Bug Fixes
Documentation