From f396454097a189fc0c8b8b395b6b9244ca2c2abc Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 31 Mar 2026 10:36:47 -0600 Subject: [PATCH] Route TUI `/feedback` submission through the app server (#16184) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TUI’s `/feedback` flow was still uploading directly through the local feedback crate, which bypassed app-server behavior such as auth-derived feedback tags like chatgpt_user_id and made TUI feedback handling diverge from other clients. It also meant that remove TUI sessions failed to upload the correct feedback logs and session details. Testing: Manually tested `/feedback` flow and confirmed that it didn't regress. --- codex-rs/tui/src/app.rs | 323 ++++++++++++++++- codex-rs/tui/src/app_event.rs | 15 + codex-rs/tui/src/bottom_pane/feedback_view.rs | 328 +++++++++++------- codex-rs/tui/src/bottom_pane/mod.rs | 2 + codex-rs/tui/src/chatwidget.rs | 17 +- codex-rs/tui/src/chatwidget/tests.rs | 5 - 6 files changed, 531 insertions(+), 159 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 5ea10e70ea..861c289a45 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3,6 +3,7 @@ use crate::app_command::AppCommand; use crate::app_command::AppCommandView; use crate::app_event::AppEvent; use crate::app_event::ExitMode; +use crate::app_event::FeedbackCategory; use crate::app_event::RealtimeAudioDeviceKind; #[cfg(target_os = "windows")] use crate::app_event::WindowsSandboxEnableMode; @@ -55,6 +56,8 @@ use codex_app_server_client::TypedRequestError; use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::CodexErrorInfo as AppServerCodexErrorInfo; use codex_app_server_protocol::ConfigLayerSource; +use codex_app_server_protocol::FeedbackUploadParams; +use codex_app_server_protocol::FeedbackUploadResponse; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; use codex_app_server_protocol::McpServerStatus; @@ -502,6 +505,15 @@ enum ThreadBufferedEvent { Notification(ServerNotification), Request(ServerRequest), HistoryEntryResponse(GetHistoryEntryResponseEvent), + FeedbackSubmission(FeedbackThreadEvent), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct FeedbackThreadEvent { + category: FeedbackCategory, + include_logs: bool, + feedback_audience: FeedbackAudience, + result: Result, } #[derive(Debug)] @@ -523,6 +535,7 @@ impl ThreadEventStore { ThreadBufferedEvent::Request(_) | ThreadBufferedEvent::Notification(ServerNotification::HookStarted(_)) | ThreadBufferedEvent::Notification(ServerNotification::HookCompleted(_)) + | ThreadBufferedEvent::FeedbackSubmission(_) ) } @@ -627,7 +640,8 @@ impl ThreadEventStore { .pending_interactive_replay .should_replay_snapshot_request(request), ThreadBufferedEvent::Notification(_) - | ThreadBufferedEvent::HistoryEntryResponse(_) => true, + | ThreadBufferedEvent::HistoryEntryResponse(_) + | ThreadBufferedEvent::FeedbackSubmission(_) => true, }) .cloned() .collect(), @@ -1054,7 +1068,6 @@ impl App { model_catalog: self.model_catalog.clone(), feedback: self.feedback.clone(), is_first_run: false, - feedback_audience: self.feedback_audience, status_account_display: self.chat_widget.status_account_display().cloned(), initial_plan_type: self.chat_widget.current_plan_type(), model: Some(self.chat_widget.current_model().to_string()), @@ -1942,6 +1955,124 @@ impl App { }); } + fn submit_feedback( + &mut self, + app_server: &AppServerSession, + category: FeedbackCategory, + reason: Option, + include_logs: bool, + ) { + let request_handle = app_server.request_handle(); + let app_event_tx = self.app_event_tx.clone(); + let origin_thread_id = self.chat_widget.thread_id(); + let rollout_path = if include_logs { + self.chat_widget.rollout_path() + } else { + None + }; + let params = build_feedback_upload_params( + origin_thread_id, + rollout_path, + category, + reason, + include_logs, + ); + tokio::spawn(async move { + let result = fetch_feedback_upload(request_handle, params) + .await + .map(|response| response.thread_id) + .map_err(|err| err.to_string()); + app_event_tx.send(AppEvent::FeedbackSubmitted { + origin_thread_id, + category, + include_logs, + result, + }); + }); + } + + fn handle_feedback_thread_event(&mut self, event: FeedbackThreadEvent) { + match event.result { + Ok(thread_id) => { + self.chat_widget + .add_to_history(crate::bottom_pane::feedback_success_cell( + event.category, + event.include_logs, + &thread_id, + event.feedback_audience, + )) + } + Err(err) => self + .chat_widget + .add_to_history(history_cell::new_error_event(format!( + "Failed to upload feedback: {err}" + ))), + } + } + + async fn enqueue_thread_feedback_event( + &mut self, + thread_id: ThreadId, + event: FeedbackThreadEvent, + ) { + let (sender, store) = { + let channel = self.ensure_thread_channel(thread_id); + (channel.sender.clone(), Arc::clone(&channel.store)) + }; + + let should_send = { + let mut guard = store.lock().await; + guard + .buffer + .push_back(ThreadBufferedEvent::FeedbackSubmission(event.clone())); + if guard.buffer.len() > guard.capacity + && let Some(removed) = guard.buffer.pop_front() + && let ThreadBufferedEvent::Request(request) = &removed + { + guard + .pending_interactive_replay + .note_evicted_server_request(request); + } + guard.active + }; + + if should_send { + match sender.try_send(ThreadBufferedEvent::FeedbackSubmission(event)) { + Ok(()) => {} + Err(TrySendError::Full(event)) => { + tokio::spawn(async move { + if let Err(err) = sender.send(event).await { + tracing::warn!("thread {thread_id} event channel closed: {err}"); + } + }); + } + Err(TrySendError::Closed(_)) => { + tracing::warn!("thread {thread_id} event channel closed"); + } + } + } + } + + async fn handle_feedback_submitted( + &mut self, + origin_thread_id: Option, + category: FeedbackCategory, + include_logs: bool, + result: Result, + ) { + let event = FeedbackThreadEvent { + category, + include_logs, + feedback_audience: self.feedback_audience, + result, + }; + if let Some(thread_id) = origin_thread_id { + self.enqueue_thread_feedback_event(thread_id, event).await; + } else { + self.handle_feedback_thread_event(event); + } + } + /// Process the completed MCP inventory fetch: clear the loading spinner, then /// render either the full tool/resource listing or an error into chat history. /// @@ -2536,6 +2667,9 @@ impl App { self.enqueue_thread_history_entry_response(thread_id, event) .await?; } + ThreadBufferedEvent::FeedbackSubmission(event) => { + self.enqueue_thread_feedback_event(thread_id, event).await; + } } } self.chat_widget @@ -3457,7 +3591,6 @@ impl App { model_catalog: model_catalog.clone(), feedback: feedback.clone(), is_first_run, - feedback_audience, status_account_display: status_account_display.clone(), initial_plan_type, model: Some(model.clone()), @@ -3492,7 +3625,6 @@ impl App { model_catalog: model_catalog.clone(), feedback: feedback.clone(), is_first_run, - feedback_audience, status_account_display: status_account_display.clone(), initial_plan_type, model: config.model.clone(), @@ -3532,7 +3664,6 @@ impl App { model_catalog: model_catalog.clone(), feedback: feedback.clone(), is_first_run, - feedback_audience, status_account_display: status_account_display.clone(), initial_plan_type, model: config.model.clone(), @@ -4293,6 +4424,22 @@ impl App { AppEvent::OpenFeedbackConsent { category } => { self.chat_widget.open_feedback_consent(category); } + AppEvent::SubmitFeedback { + category, + reason, + include_logs, + } => { + self.submit_feedback(app_server, category, reason, include_logs); + } + AppEvent::FeedbackSubmitted { + origin_thread_id, + category, + include_logs, + result, + } => { + self.handle_feedback_submitted(origin_thread_id, category, include_logs, result) + .await; + } AppEvent::LaunchExternalEditor => { if self.chat_widget.external_editor_state() == ExternalEditorState::Active { self.launch_external_editor(tui).await; @@ -5384,6 +5531,9 @@ impl App { ThreadBufferedEvent::HistoryEntryResponse(event) => { self.chat_widget.handle_history_entry_response(event); } + ThreadBufferedEvent::FeedbackSubmission(event) => { + self.handle_feedback_thread_event(event); + } } if needs_refresh { self.refresh_status_line(); @@ -5401,6 +5551,9 @@ impl App { ThreadBufferedEvent::HistoryEntryResponse(event) => { self.chat_widget.handle_history_entry_response(event) } + ThreadBufferedEvent::FeedbackSubmission(event) => { + self.handle_feedback_thread_event(event); + } } } @@ -5869,6 +6022,38 @@ async fn fetch_plugin_uninstall( .wrap_err("plugin/uninstall failed in TUI") } +fn build_feedback_upload_params( + origin_thread_id: Option, + rollout_path: Option, + category: FeedbackCategory, + reason: Option, + include_logs: bool, +) -> FeedbackUploadParams { + let extra_log_files = if include_logs { + rollout_path.map(|rollout_path| vec![rollout_path]) + } else { + None + }; + FeedbackUploadParams { + classification: crate::bottom_pane::feedback_classification(category).to_string(), + reason, + thread_id: origin_thread_id.map(|thread_id| thread_id.to_string()), + include_logs, + extra_log_files, + } +} + +async fn fetch_feedback_upload( + request_handle: AppServerRequestHandle, + params: FeedbackUploadParams, +) -> Result { + let request_id = RequestId::String(format!("feedback-upload-{}", Uuid::new_v4())); + request_handle + .request_typed(ClientRequest::FeedbackUpload { request_id, params }) + .await + .wrap_err("feedback/upload failed in TUI") +} + /// Convert flat `McpServerStatus` responses into the per-server maps used by the /// in-process MCP subsystem (tools keyed as `mcp__{server}__{tool}`, plus /// per-server resource/template/auth maps). Test-only because the TUI @@ -6266,7 +6451,6 @@ mod tests { model_catalog: app.model_catalog.clone(), feedback: codex_feedback::CodexFeedback::new(), is_first_run: false, - feedback_audience: app.feedback_audience, status_account_display: None, initial_plan_type: None, model: Some(model), @@ -9100,6 +9284,132 @@ guardian_approval = true ); } + #[test] + fn build_feedback_upload_params_includes_thread_id_and_rollout_path() { + let thread_id = ThreadId::new(); + let rollout_path = PathBuf::from("/tmp/rollout.jsonl"); + + let params = build_feedback_upload_params( + Some(thread_id), + Some(rollout_path.clone()), + FeedbackCategory::SafetyCheck, + Some("needs follow-up".to_string()), + /*include_logs*/ true, + ); + + assert_eq!(params.classification, "safety_check"); + assert_eq!(params.reason, Some("needs follow-up".to_string())); + assert_eq!(params.thread_id, Some(thread_id.to_string())); + assert_eq!(params.include_logs, true); + assert_eq!(params.extra_log_files, Some(vec![rollout_path])); + } + + #[test] + fn build_feedback_upload_params_omits_rollout_path_without_logs() { + let params = build_feedback_upload_params( + /*origin_thread_id*/ None, + Some(PathBuf::from("/tmp/rollout.jsonl")), + FeedbackCategory::GoodResult, + /*reason*/ None, + /*include_logs*/ false, + ); + + assert_eq!(params.classification, "good_result"); + assert_eq!(params.reason, None); + assert_eq!(params.thread_id, None); + assert_eq!(params.include_logs, false); + assert_eq!(params.extra_log_files, None); + } + + #[tokio::test] + async fn feedback_submission_without_thread_emits_error_history_cell() { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + + app.handle_feedback_submitted( + /*origin_thread_id*/ None, + FeedbackCategory::Bug, + /*include_logs*/ true, + Err("boom".to_string()), + ) + .await; + + let cell = match app_event_rx.try_recv() { + Ok(AppEvent::InsertHistoryCell(cell)) => cell, + other => panic!("expected feedback error history cell, saw {other:?}"), + }; + assert_eq!( + lines_to_single_string(&cell.display_lines(/*width*/ 120)), + "■ Failed to upload feedback: boom" + ); + } + + #[tokio::test] + async fn feedback_submission_for_inactive_thread_replays_into_origin_thread() { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let origin_thread_id = ThreadId::new(); + let active_thread_id = ThreadId::new(); + let origin_session = test_thread_session(origin_thread_id, PathBuf::from("/tmp/origin")); + let active_session = test_thread_session(active_thread_id, PathBuf::from("/tmp/active")); + app.thread_event_channels.insert( + origin_thread_id, + ThreadEventChannel::new_with_session( + THREAD_EVENT_CHANNEL_CAPACITY, + origin_session.clone(), + Vec::new(), + ), + ); + app.thread_event_channels.insert( + active_thread_id, + ThreadEventChannel::new_with_session( + THREAD_EVENT_CHANNEL_CAPACITY, + active_session.clone(), + Vec::new(), + ), + ); + app.activate_thread_channel(active_thread_id).await; + app.chat_widget.handle_thread_session(active_session); + while app_event_rx.try_recv().is_ok() {} + + app.handle_feedback_submitted( + Some(origin_thread_id), + FeedbackCategory::Bug, + /*include_logs*/ true, + Ok("uploaded-thread".to_string()), + ) + .await; + + assert_matches!( + app_event_rx.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty) + ); + + let snapshot = { + let channel = app + .thread_event_channels + .get(&origin_thread_id) + .expect("origin thread channel should exist"); + let store = channel.store.lock().await; + assert!(matches!( + store.buffer.back(), + Some(ThreadBufferedEvent::FeedbackSubmission(_)) + )); + store.snapshot() + }; + + app.replay_thread_snapshot(snapshot, /*resume_restored_queue*/ false); + + let mut rendered_cells = Vec::new(); + while let Ok(event) = app_event_rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + rendered_cells.push(lines_to_single_string(&cell.display_lines(/*width*/ 120))); + } + } + assert!(rendered_cells.iter().any(|cell| { + cell.contains("• Feedback uploaded. Please open an issue using the following URL:") + && cell.contains("uploaded-thread") + })); + } + fn next_user_turn_op(op_rx: &mut tokio::sync::mpsc::UnboundedReceiver) -> Op { let mut seen = Vec::new(); while let Ok(op) = op_rx.try_recv() { @@ -10005,7 +10315,6 @@ guardian_approval = true model_catalog: app.model_catalog.clone(), feedback: app.feedback.clone(), is_first_run: false, - feedback_audience: app.feedback_audience, status_account_display: app.chat_widget.status_account_display().cloned(), initial_plan_type: app.chat_widget.current_plan_type(), model: Some(app.chat_widget.current_model().to_string()), diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 2ae175013b..a666860e4f 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -518,6 +518,21 @@ pub(crate) enum AppEvent { category: FeedbackCategory, }, + /// Submit feedback for the current thread via the app-server feedback RPC. + SubmitFeedback { + category: FeedbackCategory, + reason: Option, + include_logs: bool, + }, + + /// Result of a feedback upload request initiated by the TUI. + FeedbackSubmitted { + origin_thread_id: Option, + category: FeedbackCategory, + include_logs: bool, + result: Result, + }, + /// Launch the external editor after a normal draw has completed. LaunchExternalEditor, diff --git a/codex-rs/tui/src/bottom_pane/feedback_view.rs b/codex-rs/tui/src/bottom_pane/feedback_view.rs index f9fb507648..f4f641c5d5 100644 --- a/codex-rs/tui/src/bottom_pane/feedback_view.rs +++ b/codex-rs/tui/src/bottom_pane/feedback_view.rs @@ -1,6 +1,3 @@ -use std::cell::RefCell; -use std::path::PathBuf; - use codex_feedback::feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME; use codex_feedback::feedback_diagnostics::FeedbackDiagnostics; use crossterm::event::KeyCode; @@ -15,13 +12,13 @@ use ratatui::widgets::Clear; use ratatui::widgets::Paragraph; use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::Widget; +use std::cell::RefCell; use crate::app_event::AppEvent; use crate::app_event::FeedbackCategory; use crate::app_event_sender::AppEventSender; use crate::history_cell; use crate::render::renderable::Renderable; -use codex_protocol::protocol::SessionSource; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; @@ -44,15 +41,12 @@ pub(crate) enum FeedbackAudience { External, } -/// Minimal input overlay to collect an optional feedback note, then upload -/// both logs and rollout with classification + metadata. +/// Minimal input overlay to collect an optional feedback note, then submit it +/// through the app-server-managed feedback flow. pub(crate) struct FeedbackNoteView { category: FeedbackCategory, - snapshot: codex_feedback::FeedbackSnapshot, - rollout_path: Option, app_event_tx: AppEventSender, include_logs: bool, - feedback_audience: FeedbackAudience, // UI state textarea: TextArea, @@ -63,19 +57,13 @@ pub(crate) struct FeedbackNoteView { impl FeedbackNoteView { pub(crate) fn new( category: FeedbackCategory, - snapshot: codex_feedback::FeedbackSnapshot, - rollout_path: Option, app_event_tx: AppEventSender, include_logs: bool, - feedback_audience: FeedbackAudience, ) -> Self { Self { category, - snapshot, - rollout_path, app_event_tx, include_logs, - feedback_audience, textarea: TextArea::new(), textarea_state: RefCell::new(TextAreaState::default()), complete: false, @@ -84,90 +72,12 @@ impl FeedbackNoteView { fn submit(&mut self) { let note = self.textarea.text().trim().to_string(); - let reason_opt = if note.is_empty() { - None - } else { - Some(note.as_str()) - }; - let attachment_paths = if self.include_logs { - self.rollout_path.iter().cloned().collect::>() - } else { - Vec::new() - }; - let classification = feedback_classification(self.category); - - let mut thread_id = self.snapshot.thread_id.clone(); - - let result = self.snapshot.upload_feedback( - classification, - reason_opt, - self.include_logs, - &attachment_paths, - Some(SessionSource::Cli), - /*logs_override*/ None, - ); - - match result { - Ok(()) => { - let prefix = if self.include_logs { - "• Feedback uploaded." - } else { - "• Feedback recorded (no logs)." - }; - let issue_url = - issue_url_for_category(self.category, &thread_id, self.feedback_audience); - let mut lines = vec![Line::from(match issue_url.as_ref() { - Some(_) if self.feedback_audience == FeedbackAudience::OpenAiEmployee => { - format!("{prefix} Please report this in #codex-feedback:") - } - Some(_) => format!("{prefix} Please open an issue using the following URL:"), - None => format!("{prefix} Thanks for the feedback!"), - })]; - match issue_url { - Some(url) if self.feedback_audience == FeedbackAudience::OpenAiEmployee => { - lines.extend([ - "".into(), - Line::from(vec![" ".into(), url.cyan().underlined()]), - "".into(), - Line::from(" Share this and add some info about your problem:"), - Line::from(vec![ - " ".into(), - format!("https://go/codex-feedback/{thread_id}").bold(), - ]), - ]); - } - Some(url) => { - lines.extend([ - "".into(), - Line::from(vec![" ".into(), url.cyan().underlined()]), - "".into(), - Line::from(vec![ - " Or mention your thread ID ".into(), - std::mem::take(&mut thread_id).bold(), - " in an existing issue.".into(), - ]), - ]); - } - None => { - lines.extend([ - "".into(), - Line::from(vec![ - " Thread ID: ".into(), - std::mem::take(&mut thread_id).bold(), - ]), - ]); - } - } - self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( - history_cell::PlainHistoryCell::new(lines), - ))); - } - Err(e) => { - self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( - history_cell::new_error_event(format!("Failed to upload feedback: {e}")), - ))); - } - } + let reason = if note.is_empty() { None } else { Some(note) }; + self.app_event_tx.send(AppEvent::SubmitFeedback { + category: self.category, + reason, + include_logs: self.include_logs, + }); self.complete = true; } } @@ -382,7 +292,7 @@ fn feedback_title_and_placeholder(category: FeedbackCategory) -> (String, String } } -fn feedback_classification(category: FeedbackCategory) -> &'static str { +pub(crate) fn feedback_classification(category: FeedbackCategory) -> &'static str { match category { FeedbackCategory::BadResult => "bad_result", FeedbackCategory::GoodResult => "good_result", @@ -392,6 +302,60 @@ fn feedback_classification(category: FeedbackCategory) -> &'static str { } } +pub(crate) fn feedback_success_cell( + category: FeedbackCategory, + include_logs: bool, + thread_id: &str, + feedback_audience: FeedbackAudience, +) -> history_cell::PlainHistoryCell { + let prefix = if include_logs { + "• Feedback uploaded." + } else { + "• Feedback recorded (no logs)." + }; + let issue_url = issue_url_for_category(category, thread_id, feedback_audience); + let mut lines = vec![Line::from(match issue_url.as_ref() { + Some(_) if feedback_audience == FeedbackAudience::OpenAiEmployee => { + format!("{prefix} Please report this in #codex-feedback:") + } + Some(_) => format!("{prefix} Please open an issue using the following URL:"), + None => format!("{prefix} Thanks for the feedback!"), + })]; + match issue_url { + Some(url) if feedback_audience == FeedbackAudience::OpenAiEmployee => { + lines.extend([ + "".into(), + Line::from(vec![" ".into(), url.cyan().underlined()]), + "".into(), + Line::from(" Share this and add some info about your problem:"), + Line::from(vec![ + " ".into(), + format!("https://go/codex-feedback/{thread_id}").bold(), + ]), + ]); + } + Some(url) => { + lines.extend([ + "".into(), + Line::from(vec![" ".into(), url.cyan().underlined()]), + "".into(), + Line::from(vec![ + " Or mention your thread ID ".into(), + thread_id.to_string().bold(), + " in an existing issue.".into(), + ]), + ]); + } + None => { + lines.extend([ + "".into(), + Line::from(vec![" Thread ID: ".into(), thread_id.to_string().bold()]), + ]); + } + } + history_cell::PlainHistoryCell::new(lines) +} + fn issue_url_for_category( category: FeedbackCategory, thread_id: &str, @@ -625,18 +589,25 @@ mod tests { lines.join("\n") } + fn render_cell(cell: &impl history_cell::HistoryCell, width: u16) -> String { + cell.display_lines(width) + .into_iter() + .map(|line| { + line.spans + .into_iter() + .map(|span| span.content.into_owned()) + .collect::() + .trim_end() + .to_string() + }) + .collect::>() + .join("\n") + } + fn make_view(category: FeedbackCategory) -> FeedbackNoteView { let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); let tx = AppEventSender::new(tx_raw); - let snapshot = codex_feedback::CodexFeedback::new().snapshot(/*session_id*/ None); - FeedbackNoteView::new( - category, - snapshot, - /*rollout_path*/ None, - tx, - /*include_logs*/ true, - FeedbackAudience::External, - ) + FeedbackNoteView::new(category, tx, /*include_logs*/ true) } #[test] @@ -678,33 +649,56 @@ mod tests { fn feedback_view_with_connectivity_diagnostics() { let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::(); let tx = AppEventSender::new(tx_raw); - let diagnostics = FeedbackDiagnostics::new(vec![ - FeedbackDiagnostic { - headline: "Proxy environment variables are set and may affect connectivity." - .to_string(), - details: vec!["HTTP_PROXY = http://proxy.example.com:8080".to_string()], - }, - FeedbackDiagnostic { - headline: "OPENAI_BASE_URL is set and may affect connectivity.".to_string(), - details: vec!["OPENAI_BASE_URL = https://example.com/v1".to_string()], - }, - ]); - let snapshot = codex_feedback::CodexFeedback::new() - .snapshot(/*session_id*/ None) - .with_feedback_diagnostics(diagnostics); - let view = FeedbackNoteView::new( - FeedbackCategory::Bug, - snapshot, - /*rollout_path*/ None, - tx, - /*include_logs*/ false, - FeedbackAudience::External, - ); + let view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ false); let rendered = render(&view, /*width*/ 60); insta::assert_snapshot!("feedback_view_with_connectivity_diagnostics", rendered); } + #[test] + fn submit_feedback_emits_submit_event_with_trimmed_note() { + let (tx_raw, mut rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ true); + view.textarea.insert_str(" something broke "); + + view.submit(); + + let event = rx.try_recv().expect("submit feedback event"); + assert!(matches!( + event, + AppEvent::SubmitFeedback { + category: FeedbackCategory::Bug, + reason: Some(reason), + include_logs: true, + } if reason == "something broke" + )); + assert_eq!(view.is_complete(), true); + } + + #[test] + fn submit_feedback_omits_empty_note() { + let (tx_raw, mut rx) = tokio::sync::mpsc::unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = FeedbackNoteView::new( + FeedbackCategory::GoodResult, + tx, + /*include_logs*/ false, + ); + + view.submit(); + + let event = rx.try_recv().expect("submit feedback event"); + assert!(matches!( + event, + AppEvent::SubmitFeedback { + category: FeedbackCategory::GoodResult, + reason: None, + include_logs: false, + } + )); + } + #[test] fn should_show_feedback_connectivity_details_only_for_non_good_result_with_diagnostics() { let diagnostics = FeedbackDiagnostics::new(vec![FeedbackDiagnostic { @@ -774,4 +768,76 @@ mod tests { let expected_external_url = "https://github.com/openai/codex/issues/new?template=3-cli.yml&steps=Uploaded%20thread:%20t"; assert_eq!(bug_url_non_employee.as_deref(), Some(expected_external_url)); } + + #[test] + fn feedback_success_cell_matches_external_bug_copy() { + let rendered = render_cell( + &feedback_success_cell( + FeedbackCategory::Bug, + /*include_logs*/ true, + "thread-1", + FeedbackAudience::External, + ), + /*width*/ 120, + ); + assert_eq!( + rendered, + "• Feedback uploaded. Please open an issue using the following URL:\n\n https://github.com/openai/codex/issues/new?template=3-cli.yml&steps=Uploaded%20thread:%20thread-1\n\n Or mention your thread ID thread-1 in an existing issue." + ); + } + + #[test] + fn feedback_success_cell_matches_employee_bug_copy() { + let rendered = render_cell( + &feedback_success_cell( + FeedbackCategory::Bug, + /*include_logs*/ true, + "thread-2", + FeedbackAudience::OpenAiEmployee, + ), + /*width*/ 120, + ); + assert_eq!( + rendered, + "• Feedback uploaded. Please report this in #codex-feedback:\n\n http://go/codex-feedback-internal\n\n Share this and add some info about your problem:\n https://go/codex-feedback/thread-2" + ); + } + + #[test] + fn feedback_success_cell_matches_good_result_copy() { + let rendered = render_cell( + &feedback_success_cell( + FeedbackCategory::GoodResult, + /*include_logs*/ false, + "thread-3", + FeedbackAudience::External, + ), + /*width*/ 120, + ); + assert_eq!( + rendered, + "• Feedback recorded (no logs). Thanks for the feedback!\n\n Thread ID: thread-3" + ); + } + + #[test] + fn feedback_success_cell_uses_issue_links_for_remaining_categories() { + for category in [ + FeedbackCategory::BadResult, + FeedbackCategory::SafetyCheck, + FeedbackCategory::Other, + ] { + let rendered = render_cell( + &feedback_success_cell( + category, + /*include_logs*/ false, + "thread-4", + FeedbackAudience::External, + ), + /*width*/ 120, + ); + assert!(rendered.contains("Please open an issue using the following URL:")); + assert!(rendered.contains("thread-4")); + } + } } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 4016821d44..91b4861dd5 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -93,8 +93,10 @@ pub(crate) use list_selection_view::popup_content_width; pub(crate) use list_selection_view::side_by_side_layout_widths; mod feedback_view; pub(crate) use feedback_view::FeedbackAudience; +pub(crate) use feedback_view::feedback_classification; pub(crate) use feedback_view::feedback_disabled_params; pub(crate) use feedback_view::feedback_selection_params; +pub(crate) use feedback_view::feedback_success_cell; pub(crate) use feedback_view::feedback_upload_consent_params; pub(crate) use skills_toggle_view::SkillsToggleItem; pub(crate) use skills_toggle_view::SkillsToggleView; diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 1267e6b732..f6ff6a3f1d 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -301,7 +301,6 @@ use crate::bottom_pane::ColumnWidthMode; use crate::bottom_pane::DOUBLE_PRESS_QUIT_SHORTCUT_ENABLED; use crate::bottom_pane::ExperimentalFeatureItem; use crate::bottom_pane::ExperimentalFeaturesView; -use crate::bottom_pane::FeedbackAudience; use crate::bottom_pane::InputResult; use crate::bottom_pane::LocalImageAttachment; use crate::bottom_pane::McpServerElicitationFormRequest; @@ -553,7 +552,6 @@ pub(crate) struct ChatWidgetInit { pub(crate) model_catalog: Arc, pub(crate) feedback: codex_feedback::CodexFeedback, pub(crate) is_first_run: bool, - pub(crate) feedback_audience: FeedbackAudience, pub(crate) status_account_display: Option, pub(crate) initial_plan_type: Option, pub(crate) model: Option, @@ -915,7 +913,6 @@ pub(crate) struct ChatWidget { last_rendered_width: std::cell::Cell>, // Feedback sink for /feedback feedback: codex_feedback::CodexFeedback, - feedback_audience: FeedbackAudience, // Current session rollout path (if known) current_rollout_path: Option, // Current working directory (if known) @@ -2115,28 +2112,18 @@ impl ChatWidget { category: crate::app_event::FeedbackCategory, include_logs: bool, ) { - let snapshot = self.feedback.snapshot(self.thread_id); - self.show_feedback_note(category, include_logs, snapshot); + self.show_feedback_note(category, include_logs); } fn show_feedback_note( &mut self, category: crate::app_event::FeedbackCategory, include_logs: bool, - snapshot: codex_feedback::FeedbackSnapshot, ) { - let rollout = if include_logs { - self.current_rollout_path.clone() - } else { - None - }; let view = crate::bottom_pane::FeedbackNoteView::new( category, - snapshot, - rollout, self.app_event_tx.clone(), include_logs, - self.feedback_audience, ); self.bottom_pane.show_view(Box::new(view)); self.request_redraw(); @@ -4649,7 +4636,6 @@ impl ChatWidget { model_catalog, feedback, is_first_run, - feedback_audience, status_account_display, initial_plan_type, model, @@ -4788,7 +4774,6 @@ impl ChatWidget { turn_runtime_metrics: RuntimeMetricsSummary::default(), last_rendered_width: std::cell::Cell::new(None), feedback, - feedback_audience, current_rollout_path: None, current_cwd, session_network_proxy: None, diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index ebb92909e8..3a364f5e0e 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -10,7 +10,6 @@ use crate::app_event::ExitMode; #[cfg(not(target_os = "linux"))] use crate::app_event::RealtimeAudioDeviceKind; use crate::app_event_sender::AppEventSender; -use crate::bottom_pane::FeedbackAudience; use crate::bottom_pane::LocalImageAttachment; use crate::bottom_pane::MentionBinding; use crate::chatwidget::realtime::RealtimeConversationPhase; @@ -2009,7 +2008,6 @@ async fn helpers_are_available_and_do_not_panic() { model_catalog: test_model_catalog(&cfg), feedback: codex_feedback::CodexFeedback::new(), is_first_run: true, - feedback_audience: FeedbackAudience::External, status_account_display: None, initial_plan_type: None, model: Some(resolved_model), @@ -2183,7 +2181,6 @@ async fn make_chatwidget_manual( turn_runtime_metrics: RuntimeMetricsSummary::default(), last_rendered_width: std::cell::Cell::new(None), feedback: codex_feedback::CodexFeedback::new(), - feedback_audience: FeedbackAudience::External, current_rollout_path: None, current_cwd: None, session_network_proxy: None, @@ -6952,7 +6949,6 @@ async fn collaboration_modes_defaults_to_code_on_startup() { model_catalog: test_model_catalog(&cfg), feedback: codex_feedback::CodexFeedback::new(), is_first_run: true, - feedback_audience: FeedbackAudience::External, status_account_display: None, initial_plan_type: None, model: Some(resolved_model.clone()), @@ -6997,7 +6993,6 @@ async fn experimental_mode_plan_is_ignored_on_startup() { model_catalog: test_model_catalog(&cfg), feedback: codex_feedback::CodexFeedback::new(), is_first_run: true, - feedback_audience: FeedbackAudience::External, status_account_display: None, initial_plan_type: None, model: Some(resolved_model.clone()),