-
Notifications
You must be signed in to change notification settings - Fork 413
feat: add record-replay system for playground HTTP calls #3336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Changes from all commits
38ef05d
0472a09
b184221
e87ceef
7b40007
01fc300
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,11 +63,14 @@ pub async fn pick_port(base_port: u16, max_attempts: u16) -> anyhow::Result<(Tcp | |
| // Shared state for Axum handlers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| use crate::ReplayStoreMap; | ||
|
|
||
| #[derive(Clone)] | ||
| struct WsState { | ||
| bex: Arc<dyn bex_project::BexLsp>, | ||
| broadcast_tx: broadcast::Sender<WsOutMessage>, | ||
| env_state: Arc<PlaygroundEnvState>, | ||
| replay_stores: ReplayStoreMap, | ||
| } | ||
|
|
||
| /// Start the playground server on the given listener. | ||
|
|
@@ -76,8 +79,9 @@ pub async fn run( | |
| bex: Arc<dyn bex_project::BexLsp>, | ||
| broadcast_tx: broadcast::Sender<WsOutMessage>, | ||
| env_state: Arc<PlaygroundEnvState>, | ||
| replay_stores: ReplayStoreMap, | ||
| ) -> anyhow::Result<()> { | ||
| let app = build_router(bex, broadcast_tx, env_state)?; | ||
| let app = build_router(bex, broadcast_tx, env_state, replay_stores)?; | ||
|
|
||
| tracing::info!( | ||
| "Playground: http://localhost:{}", | ||
|
|
@@ -93,11 +97,13 @@ fn build_router( | |
| bex: Arc<dyn bex_project::BexLsp>, | ||
| broadcast_tx: broadcast::Sender<WsOutMessage>, | ||
| env_state: Arc<PlaygroundEnvState>, | ||
| replay_stores: ReplayStoreMap, | ||
| ) -> anyhow::Result<Router> { | ||
| let ws_state = WsState { | ||
| bex, | ||
| broadcast_tx, | ||
| env_state, | ||
| replay_stores, | ||
| }; | ||
|
|
||
| let api = Router::new() | ||
|
|
@@ -419,6 +425,47 @@ async fn handle_ws_in_message( | |
| tracing::warn!("Failed to send cursor context"); | ||
| } | ||
| } | ||
|
|
||
| WsInMessage::ToggleReplay { | ||
| project, | ||
| fetch_id, | ||
| pinned, | ||
| } => { | ||
| let stores = state.replay_stores.lock().unwrap(); | ||
| if let Some(store) = stores.get(&project) { | ||
| store.write().unwrap().set_pinned(fetch_id, pinned); | ||
| } else { | ||
| tracing::warn!("ToggleReplay: no replay store for project {project}"); | ||
| } | ||
|
Comment on lines
+429
to
+439
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle unknown
💡 Suggested change WsInMessage::ToggleReplay {
project,
fetch_id,
pinned,
} => {
let stores = state.replay_stores.lock().unwrap();
if let Some(store) = stores.get(&project) {
- store.write().unwrap().set_pinned(fetch_id, pinned);
+ if !store.write().unwrap().set_pinned(fetch_id, pinned) {
+ tracing::warn!("ToggleReplay: unknown fetch_id {fetch_id} for project {project}");
+ }
} else {
tracing::warn!("ToggleReplay: no replay store for project {project}");
}
} |
||
| } | ||
|
|
||
| WsInMessage::RequestReplayState { project } => { | ||
| // Collect entries while holding the lock, then drop it before await. | ||
| let entries: Vec<serde_json::Value> = { | ||
| let stores = state.replay_stores.lock().unwrap(); | ||
| if let Some(store) = stores.get(&project) { | ||
| let store = store.read().unwrap(); | ||
| store | ||
| .snapshot() | ||
| .into_iter() | ||
| .map(|g| serde_json::to_value(g).unwrap()) | ||
| .collect() | ||
| } else { | ||
| vec![] | ||
| } | ||
| }; | ||
| // Send via sink (per-session), not broadcast. | ||
| if let Err(e) = sink | ||
| .send(axum::extract::ws::Message::Text( | ||
| serde_json::to_string(&WsOutMessage::ReplayState { entries }) | ||
| .unwrap() | ||
| .into(), | ||
| )) | ||
| .await | ||
| { | ||
| tracing::warn!("Failed to send ReplayState: {e}"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Keep
response_to_fetchout of the public API.Exposing a mutable
Mutex<HashMap<...>>field makes this fetch/body correlation invariant externally mutable. If the replay wiring only needs it insidebaml_lsp_server,pub(crate)or a small accessor keeps the API surface tighter.🔒 Suggested change
pub struct PlaygroundHttpState { broadcast_tx: broadcast::Sender<WsOutMessage>, next_fetch_id: Arc<AtomicU64>, /// Maps response body pointer → (call_id, fetch_id) for response_text tracking. - pub response_to_fetch: std::sync::Mutex<HashMap<usize, (u64, u64)>>, + pub(crate) response_to_fetch: std::sync::Mutex<HashMap<usize, (u64, u64)>>, }📝 Committable suggestion