Skip to content
This repository was archived by the owner on Sep 23, 2025. It is now read-only.

Commit ce3a303

Browse files
nikomatsakisnikomatsakis
committed
Refactor UserFeedback to struct with review_id + enum for feedback data
**Excellent Design Improvement:** **Before:** Enum with duplicated review_id in every variant **After:** Struct with common review_id + enum for feedback-specific data **New Structure:** ```rust struct UserFeedback { review_id: String, // Common to all feedback #[serde(flatten)] feedback: FeedbackData, // Variant-specific data } enum FeedbackData { Comment { file_path, line_number, comment_text, context_lines }, CompleteReview { completion_action, additional_notes }, } ``` **Benefits:** - **No duplication** - review_id appears once at the struct level - **Cleaner separation** - common fields vs variant-specific fields - **Same JSON structure** - serde(flatten) maintains wire compatibility - **Better organization** - logical grouping of related data **JSON remains unchanged:** ```json {"review_id": "abc123", "feedback_type": "comment", "comment_text": "..."} ``` **Result:** Much cleaner type design that eliminates redundancy while maintaining API compatibility.
1 parent 3f58d73 commit ce3a303

File tree

3 files changed

+65
-55
lines changed

3 files changed

+65
-55
lines changed

‎server/src/ipc.rs‎

Lines changed: 50 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -341,10 +341,12 @@ impl IPCCommunicator {
341341
) -> Result<UserFeedback> {
342342
if self.test_mode {
343343
info!("Send create synthetic PR called (test mode)");
344-
return Ok(UserFeedback::CompleteReview {
344+
return Ok(UserFeedback {
345345
review_id: "test_review".to_string(),
346-
completion_action: crate::synthetic_pr::CompletionAction::Return,
347-
additional_notes: None,
346+
feedback: crate::synthetic_pr::FeedbackData::CompleteReview {
347+
completion_action: crate::synthetic_pr::CompletionAction::Return,
348+
additional_notes: None,
349+
},
348350
});
349351
}
350352

@@ -389,16 +391,18 @@ impl IPCCommunicator {
389391
review_id
390392
);
391393
// Return mock feedback for testing
392-
return Ok(UserFeedback::Comment {
394+
return Ok(UserFeedback {
393395
review_id: review_id.to_string(),
394-
file_path: Some("test.rs".to_string()),
395-
line_number: Some(42),
396-
comment_text: "This is a test comment".to_string(),
397-
context_lines: Some(vec![
398-
"fn test() {".to_string(),
399-
" // Line 42".to_string(),
400-
"}".to_string(),
401-
]),
396+
feedback: crate::synthetic_pr::FeedbackData::Comment {
397+
file_path: Some("test.rs".to_string()),
398+
line_number: Some(42),
399+
comment_text: "This is a test comment".to_string(),
400+
context_lines: Some(vec![
401+
"fn test() {".to_string(),
402+
" // Line 42".to_string(),
403+
"}".to_string(),
404+
]),
405+
},
402406
});
403407
}
404408

@@ -442,12 +446,14 @@ impl IPCCommunicator {
442446
) -> Result<crate::synthetic_pr::UserFeedback> {
443447
if self.test_mode {
444448
info!("Send review update called (test mode)");
445-
return Ok(UserFeedback::Comment {
449+
return Ok(UserFeedback {
446450
review_id: "test_review".to_string(),
447-
file_path: Some("test.rs".to_string()),
448-
line_number: Some(42),
449-
comment_text: "This looks good to me!".to_string(),
450-
context_lines: None,
451+
feedback: crate::synthetic_pr::FeedbackData::Comment {
452+
file_path: Some("test.rs".to_string()),
453+
line_number: Some(42),
454+
comment_text: "This looks good to me!".to_string(),
455+
context_lines: None,
456+
},
451457
});
452458
}
453459

@@ -863,34 +869,34 @@ impl IPCCommunicator {
863869
};
864870

865871
// Convert to UserFeedback enum
866-
let user_feedback = match feedback_payload.feedback_type.as_str() {
867-
"comment" => UserFeedback::Comment {
868-
review_id: feedback_payload.review_id.clone(),
869-
file_path: feedback_payload.file_path,
870-
line_number: feedback_payload.line_number,
871-
comment_text: feedback_payload.comment_text.unwrap_or_default(),
872-
context_lines: feedback_payload.context_lines,
873-
},
874-
"complete_review" => {
875-
let completion_action = feedback_payload.completion_action.as_deref()
876-
.and_then(|action| match action {
877-
"request_changes" => Some(crate::synthetic_pr::CompletionAction::RequestChanges),
878-
"checkpoint" => Some(crate::synthetic_pr::CompletionAction::Checkpoint),
879-
"return" => Some(crate::synthetic_pr::CompletionAction::Return),
880-
_ => None,
881-
})
882-
.unwrap_or(crate::synthetic_pr::CompletionAction::Return);
883-
884-
UserFeedback::CompleteReview {
885-
review_id: feedback_payload.review_id.clone(),
886-
completion_action,
887-
additional_notes: feedback_payload.additional_notes,
872+
let user_feedback = UserFeedback {
873+
review_id: feedback_payload.review_id.clone(),
874+
feedback: match feedback_payload.feedback_type.as_str() {
875+
"comment" => crate::synthetic_pr::FeedbackData::Comment {
876+
file_path: feedback_payload.file_path,
877+
line_number: feedback_payload.line_number,
878+
comment_text: feedback_payload.comment_text.unwrap_or_default(),
879+
context_lines: feedback_payload.context_lines,
880+
},
881+
"complete_review" => {
882+
let completion_action = feedback_payload.completion_action.as_deref()
883+
.and_then(|action| match action {
884+
"request_changes" => Some(crate::synthetic_pr::CompletionAction::RequestChanges),
885+
"checkpoint" => Some(crate::synthetic_pr::CompletionAction::Checkpoint),
886+
"return" => Some(crate::synthetic_pr::CompletionAction::Return),
887+
_ => None,
888+
})
889+
.unwrap_or(crate::synthetic_pr::CompletionAction::Return);
890+
891+
crate::synthetic_pr::FeedbackData::CompleteReview {
892+
completion_action,
893+
additional_notes: feedback_payload.additional_notes,
894+
}
895+
}
896+
_ => crate::synthetic_pr::FeedbackData::CompleteReview {
897+
completion_action: crate::synthetic_pr::CompletionAction::Return,
898+
additional_notes: None,
888899
}
889-
}
890-
_ => UserFeedback::CompleteReview {
891-
review_id: feedback_payload.review_id.clone(),
892-
completion_action: crate::synthetic_pr::CompletionAction::Return,
893-
additional_notes: None,
894900
}
895901
};
896902

‎server/src/server.rs‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,8 @@ impl DialecticServer {
8686

8787
/// Format user feedback into clear instructions for the LLM
8888
fn format_user_feedback_message(&self, feedback: &UserFeedback) -> String {
89-
match feedback {
90-
UserFeedback::Comment {
91-
review_id,
89+
match &feedback.feedback {
90+
crate::synthetic_pr::FeedbackData::Comment {
9291
file_path,
9392
line_number,
9493
comment_text,
@@ -117,11 +116,10 @@ impl DialecticServer {
117116
line_number,
118117
comment_text,
119118
context,
120-
review_id
119+
&feedback.review_id
121120
)
122121
}
123-
UserFeedback::CompleteReview {
124-
review_id,
122+
crate::synthetic_pr::FeedbackData::CompleteReview {
125123
completion_action,
126124
additional_notes,
127125
} => {
@@ -140,14 +138,14 @@ impl DialecticServer {
140138
You may now edit files as needed.\n\n\
141139
When finished, invoke: update_review(review_id: '{}', action: Approve)",
142140
notes_section,
143-
review_id
141+
&feedback.review_id
144142
),
145143
CompletionAction::Checkpoint => format!(
146144
"User completed their review and selected: 'Request agent to checkpoint this work'{}\n\
147145
Please commit the current changes and document the work completed.\n\n\
148146
When finished, invoke: update_review(review_id: '{}', action: Approve)",
149147
notes_section,
150-
review_id
148+
&feedback.review_id
151149
),
152150
CompletionAction::Return => format!(
153151
"User completed their review and selected: 'Return to agent without explicit request'{}\n\

‎server/src/synthetic_pr/mcp_tools.rs‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,23 @@ pub enum CompletionAction {
3636

3737
/// User feedback from VSCode extension
3838
#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone)]
39+
pub struct UserFeedback {
40+
pub review_id: String,
41+
#[serde(flatten)]
42+
pub feedback: FeedbackData,
43+
}
44+
45+
/// Different types of feedback data
46+
#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone)]
3947
#[serde(tag = "feedback_type", rename_all = "snake_case")]
40-
pub enum UserFeedback {
48+
pub enum FeedbackData {
4149
Comment {
42-
review_id: String,
4350
file_path: Option<String>,
4451
line_number: Option<u32>,
4552
comment_text: String,
4653
context_lines: Option<Vec<String>>,
4754
},
4855
CompleteReview {
49-
review_id: String,
5056
completion_action: CompletionAction,
5157
additional_notes: Option<String>,
5258
},

0 commit comments

Comments
 (0)