Stabilize split-screen panel lifecycle + feedBack rebrand#23
Open
carochacs wants to merge 6 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>
📝 WalkthroughWalkthroughThe plugin is rebranded for feedBack and its split-screen runtime is hardened across device binding, detector and overlay lifecycles, layout resizing, pop-out/redock restoration, 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 finalStates
MainWindow->>SplitScreen: restore multi-panel 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
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