Repository navigation
fix(playground): reuse the browser window on OpenPlayground (B-808) - #3968
Conversation
In `baml playground` (browser) mode the LSP is served over the /api/lsp WebSocket, so an editor's inline "Open Playground" lens reaches the server as an OpenPlayground request. The native sender unconditionally called `webbrowser::open()` and returned early, so every inline click spawned a fresh browser window at the root URL, losing the function/test target. Browser-mode OpenPlayground now: - broadcasts over the WS so an already-open page navigates in place; - only opens a browser window when no page is connected (`receiver_count() == 0`), debounced so two quick clicks before the first window connects don't double-open; - records the target and replays it to a page on RequestState, so a freshly opened or reconnected window lands on the right function/test. Editor (VS Code) mode is unchanged: the LSP-notification path stays, and the shared target stays None so the WS replay is a no-op there. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBrowser-mode playground openings now share the latest target, broadcast it to connected pages, debounce new browser launches, and replay it when a page requests state. ChangesPlayground target replay
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NativePlaygroundSender
participant WebSocketServer
participant BrowserPage
participant SystemBrowser
NativePlaygroundSender->>WebSocketServer: broadcast OpenPlayground
WebSocketServer->>BrowserPage: deliver notification to connected page
BrowserPage->>WebSocketServer: RequestState
WebSocketServer->>BrowserPage: replay latest target
NativePlaygroundSender->>SystemBrowser: open URL when no page is connected
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_lsp_server/src/playground_sender.rs (1)
79-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a unit test for the debounce logic.
claim_browser_openencapsulates the double-open prevention (the mutex correctly serializes concurrent callers and the short-circuit&&avoids bumpinglast_browser_openwhen a page is already connected). This is easily testable in isolation and would lock in the 5s window behavior.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
🤖 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 `@baml_language/crates/baml_lsp_server/src/playground_sender.rs` around lines 79 - 89, Add Rust unit tests for `claim_browser_open` covering the first call, immediate repeated call rejection, and acceptance after the `BROWSER_SPAWN_DEBOUNCE` interval; verify the timestamp is not refreshed by rejected calls, using suitable synchronization or a testable time abstraction if needed.Source: Coding guidelines
🤖 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 `@baml_language/crates/baml_lsp_server/src/playground_sender.rs`:
- Around line 79-89: Add Rust unit tests for `claim_browser_open` covering the
first call, immediate repeated call rejection, and acceptance after the
`BROWSER_SPAWN_DEBOUNCE` interval; verify the timestamp is not refreshed by
rejected calls, using suitable synchronization or a testable time abstraction if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f81f48c6-8dd2-4b64-983e-825b7b2c62da
📒 Files selected for processing (3)
baml_language/crates/baml_lsp_server/src/lib.rsbaml_language/crates/baml_lsp_server/src/playground_sender.rsbaml_language/crates/baml_lsp_server/src/playground_server.rs
- browser-mode OpenPlayground broadcasts the openPlayground notification in place (correctly serialized for the page) and records the target for RequestState replay; - claim_browser_open debounces rapid spawns so two quick opens before the first window connects don't stack windows. Holding a live broadcast receiver keeps receiver_count() > 0, which exercises the reuse path and guarantees the test never launches a real browser. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Binary size checks passed✅ 7 passed
Generated by |
Problem
B-808: "open playground" inline opens a new playground window.
In
baml playground(browser) mode the LSP is served over the/api/lspWebSocket (lib.rs:223), so an editor's inline ▶ Open Playground lens reaches the server as anOpenPlaygroundrequest. The native sender (playground_sender.rs) unconditionally calledwebbrowser::open()and returned early — it never told the already-open page anything. So every inline click spawned a fresh browser window, at the root URL, losing the function/test target.The two editor surfaces (VS Code
WebviewPanel, Monaco tab) are already singletons, so this only bit thebaml playground/ browser flow — which is exactly the crowd that runs it without opening a heavy editor.Fix
Browser-mode
OpenPlaygroundnow treats "open" as navigate the one surface, don't spawn one:openPlaygroundnotification —ExecutionPanel.tsx:1270).webbrowser::openwhenreceiver_count() == 0(no page connected), debounced 5s so two quick clicks before the first window connects don't double-open.RequestState(which every page sends on mount), so a freshly opened or reconnected window lands on the right function/test.Editor (VS Code) mode is untouched:
open_in_browser == falsekeeps the existing LSP-notification path, and the shared target staysNone, so the WS replay is a no-op there.Scope
Single-project only, by design — multi-project is intentionally out of scope for this PR. (For the record: with ≥2 BAML projects there's a separate latent VS Code issue where
baml.openBamlPanelis auto-registered per language client and the second collides; that's a distinct fix for later.)Files
playground_sender.rs— reuse logic,OpenPlaygroundTarget/SharedOpenTarget, debounce.playground_server.rs—WsState.current_open_target+RequestStatereplay, threaded throughrun/build_router.lib.rs— creates the shared target, passes it to the sender and server.Testing
cargo check,cargo check --tests, andcargo clippyall pass forbaml_lsp_server. Manual verification of thebaml playgroundreuse flow to follow.🤖 Generated with Claude Code
Summary by CodeRabbit