From a6964bfef63e545e2235a619ac0cfe38369324ab Mon Sep 17 00:00:00 2001 From: canvrno-oai Date: Mon, 4 May 2026 15:10:44 -0700 Subject: [PATCH] codex: address PR review feedback (#21085) --- .../schema/json/ServerNotification.json | 7 ++++++- .../codex_app_server_protocol.schemas.json | 7 ++++++- .../codex_app_server_protocol.v2.schemas.json | 7 ++++++- .../json/v2/AppListUpdatedNotification.json | 7 ++++++- .../v2/AppListUpdatedNotification.ts | 6 +++++- .../src/protocol/v2/apps.rs | 2 ++ .../src/request_processors/apps_processor.rs | 19 +++++++++++++++---- .../request_processors/config_processor.rs | 5 ++++- codex-rs/tui/src/chatwidget.rs | 13 ++++++++++--- codex-rs/tui/src/chatwidget/protocol.rs | 2 +- .../chatwidget/tests/popups_and_settings.rs | 3 ++- 11 files changed, 63 insertions(+), 15 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 6af19f89e2..3c5e1edce9 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -284,10 +284,15 @@ "$ref": "#/definitions/AppInfo" }, "type": "array" + }, + "isFinal": { + "description": "Whether the notification includes the complete merged app list.", + "type": "boolean" } }, "required": [ - "data" + "data", + "isFinal" ], "type": "object" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 4e061f8591..3de275ac44 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -5919,10 +5919,15 @@ "$ref": "#/definitions/v2/AppInfo" }, "type": "array" + }, + "isFinal": { + "description": "Whether the notification includes the complete merged app list.", + "type": "boolean" } }, "required": [ - "data" + "data", + "isFinal" ], "title": "AppListUpdatedNotification", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index c7130126f9..2cdce55316 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -479,10 +479,15 @@ "$ref": "#/definitions/AppInfo" }, "type": "array" + }, + "isFinal": { + "description": "Whether the notification includes the complete merged app list.", + "type": "boolean" } }, "required": [ - "data" + "data", + "isFinal" ], "title": "AppListUpdatedNotification", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/v2/AppListUpdatedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/AppListUpdatedNotification.json index d4e99f5086..342c0bb343 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/AppListUpdatedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/AppListUpdatedNotification.json @@ -266,10 +266,15 @@ "$ref": "#/definitions/AppInfo" }, "type": "array" + }, + "isFinal": { + "description": "Whether the notification includes the complete merged app list.", + "type": "boolean" } }, "required": [ - "data" + "data", + "isFinal" ], "title": "AppListUpdatedNotification", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/AppListUpdatedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/AppListUpdatedNotification.ts index c6ad87f2cf..80eee73613 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/AppListUpdatedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/AppListUpdatedNotification.ts @@ -6,4 +6,8 @@ import type { AppInfo } from "./AppInfo"; /** * EXPERIMENTAL - notification emitted when the app list changes. */ -export type AppListUpdatedNotification = { data: Array, }; +export type AppListUpdatedNotification = { data: Array, +/** + * Whether the notification includes the complete merged app list. + */ +isFinal: boolean, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/apps.rs b/codex-rs/app-server-protocol/src/protocol/v2/apps.rs index 9f46525e6c..b3c1086efc 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/apps.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/apps.rs @@ -143,4 +143,6 @@ pub struct AppsListResponse { /// EXPERIMENTAL - notification emitted when the app list changes. pub struct AppListUpdatedNotification { pub data: Vec, + /// Whether the notification includes the complete merged app list. + pub is_final: bool, } diff --git a/codex-rs/app-server/src/request_processors/apps_processor.rs b/codex-rs/app-server/src/request_processors/apps_processor.rs index 07550d263a..799839a08b 100644 --- a/codex-rs/app-server/src/request_processors/apps_processor.rs +++ b/codex-rs/app-server/src/request_processors/apps_processor.rs @@ -194,7 +194,12 @@ impl AppsRequestProcessor { accessible_loaded, all_loaded, ) { - send_app_list_updated_notification(outgoing, merged.clone()).await; + send_app_list_updated_notification( + outgoing, + merged.clone(), + /*is_final*/ false, + ) + .await; last_notified_apps = Some(merged); } } @@ -254,7 +259,12 @@ impl AppsRequestProcessor { all_loaded, ) && last_notified_apps.as_ref() != Some(&merged) { - send_app_list_updated_notification(outgoing, merged.clone()).await; + send_app_list_updated_notification( + outgoing, + merged.clone(), + /*is_final*/ accessible_loaded && all_loaded, + ) + .await; last_notified_apps = Some(merged.clone()); } @@ -381,10 +391,11 @@ fn paginate_apps( async fn send_app_list_updated_notification( outgoing: &Arc, data: Vec, + is_final: bool, ) { outgoing .send_server_notification(ServerNotification::AppListUpdated( - AppListUpdatedNotification { data }, + AppListUpdatedNotification { data, is_final }, )) .await; } @@ -428,6 +439,6 @@ async fn send_force_refetched_app_list_updated_notification( &config, ); if data != previous_data { - send_app_list_updated_notification(outgoing, data).await; + send_app_list_updated_notification(outgoing, data, /*is_final*/ true).await; } } diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index 5365030d6b..ddc7fa58d1 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -272,7 +272,10 @@ impl ConfigRequestProcessor { ); outgoing .send_server_notification(ServerNotification::AppListUpdated( - AppListUpdatedNotification { data }, + AppListUpdatedNotification { + data, + is_final: true, + }, )) .await; }); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e4e01ae6b9..d3d6c5109a 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -87,6 +87,7 @@ use crate::version::CODEX_CLI_VERSION; use codex_app_server_protocol::AddCreditsNudgeCreditType; use codex_app_server_protocol::AddCreditsNudgeEmailStatus; use codex_app_server_protocol::AppInfo; +use codex_app_server_protocol::AppListUpdatedNotification; use codex_app_server_protocol::AppSummary; use codex_app_server_protocol::CodexErrorInfo as AppServerCodexErrorInfo; use codex_app_server_protocol::CollabAgentTool; @@ -10034,10 +10035,16 @@ impl ChatWidget { } } - fn on_app_list_updated(&mut self, connectors: Vec) { + fn on_app_list_updated(&mut self, notification: AppListUpdatedNotification) { + if !notification.is_final && !self.connectors.prefetch_in_flight { + return; + } + let is_final = notification.is_final && !self.connectors.prefetch_in_flight; self.on_connectors_loaded( - Ok(ConnectorsSnapshot { connectors }), - /*is_final*/ !self.connectors.prefetch_in_flight, + Ok(ConnectorsSnapshot { + connectors: notification.data, + }), + is_final, ); } diff --git a/codex-rs/tui/src/chatwidget/protocol.rs b/codex-rs/tui/src/chatwidget/protocol.rs index 7c8b347193..99e2c3b893 100644 --- a/codex-rs/tui/src/chatwidget/protocol.rs +++ b/codex-rs/tui/src/chatwidget/protocol.rs @@ -159,7 +159,7 @@ impl ChatWidget { } ServerNotification::AppListUpdated(notification) => { if !from_replay { - self.on_app_list_updated(notification.data); + self.on_app_list_updated(notification); } } ServerNotification::ItemGuardianApprovalReviewStarted(notification) => { diff --git a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs index 67de579e58..2f0a7d44d1 100644 --- a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs +++ b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs @@ -1659,7 +1659,7 @@ async fn apps_refresh_failure_with_cached_snapshot_triggers_pending_force_refetc } #[tokio::test] -async fn apps_popup_keeps_existing_full_snapshot_while_partial_refresh_loads() { +async fn apps_popup_ignores_stale_partial_app_list_update_after_refresh() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; set_chatgpt_auth(&mut chat); chat.config @@ -1742,6 +1742,7 @@ async fn apps_popup_keeps_existing_full_snapshot_while_partial_refresh_loads() { plugin_display_names: Vec::new(), }, ], + is_final: false, }), /*replay_kind*/ None, );