Conversation
| error = %err, | ||
| "plugin/list featured plugin fetch failed; returning empty featured ids" | ||
| ); | ||
| Vec::new() |
There was a problem hiding this comment.
TODO: emit failure metric
|
@codex review |
|
Note To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39be8e1ead
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .any(|marketplace| marketplace.name == OPENAI_CURATED_MARKETPLACE_NAME) | ||
| { | ||
| let auth = self.auth_manager.auth().await; | ||
| match fetch_remote_featured_plugin_ids(&config_for_featured, auth.as_ref()).await { |
There was a problem hiding this comment.
plugin/list now awaits fetch_remote_featured_plugin_ids whenever openai-curated is present, even when forceRemoteSync is false. This turns a local listing RPC into a network-bound call; the fetch helper uses a 30s timeout, so unreachable/slow chatgpt_base_url can stall every list request and UI flow before returning. This is a significant latency regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we have to wait for the featured plugins for directory page. Considering use a 5s timeout
There was a problem hiding this comment.
Is it possible to run this in parallel with list_marketplaces_for_config?
There was a problem hiding this comment.
Another approach is to cache this result. we are not going to change featured plugin that often so we could cache this result.
codex-rs/core/src/plugins/manager.rs
Outdated
| config: &Config, | ||
| auth: Option<&CodexAuth>, | ||
| ) -> Result<Vec<String>, RemotePluginFetchError> { | ||
| if !plugins_feature_enabled_from_stack(&config.config_layer_stack) { |
There was a problem hiding this comment.
use config.feature.enabled. I have a PR to fix/remove plugins_feature_enabled_from_stack.
| thread_manager | ||
| .plugins_manager() | ||
| .maybe_start_curated_repo_sync_for_config(&config, &thread_manager.session_source()); | ||
| codex_message_processor.spawn_featured_plugin_ids_cache_warmup(Arc::clone(&config)); |
There was a problem hiding this comment.
We could merge this into maybe_start_curated_repo_sync_for_config and rename it to something like run_plugin_startup_jobs
| .maybe_start_curated_repo_sync_for_config( | ||
| &config, | ||
| &thread_manager.session_source(), | ||
| &auth_manager, |
There was a problem hiding this comment.
i think you can pass auth_manager.clone() directly.
| let config = config.clone(); | ||
| let auth_manager = auth_manager.clone(); | ||
| let manager = Arc::clone(self); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Maybe we should put the entire maybe_start_curated_repo_sync_for_config in a tokio::spawn
2 participants
No description provided.