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

Commit 5922a92

Browse files
committed
Remove keepalive, use long timeout for user feedback
**Simpler Solution:** **Problem:** Keepalive mechanism had race conditions and added complexity **Solution:** Just use long timeout (1 hour) for user feedback messages **Changes:** - Removed KeepAlive variant from FeedbackData enum - Removed keepalive logic from extension - Added message-type-based timeout selection in send_message_with_reply() - CreateSyntheticPr/UpdateSyntheticPr: 1 hour timeout - Other IPC messages: 5 second timeout **Benefits:** - **No race conditions** - Single response per request - **Simple implementation** - No complex keepalive protocol - **User control** - Users can Ctrl-C if they want to abort - **Reasonable timeout** - 1 hour is plenty for code review **Result:** Clean, simple solution that handles long user interactions without complexity.
1 parent de8bd46 commit 5922a92

File tree

4 files changed

+73
-96
lines changed

4 files changed

+73
-96
lines changed

extension/src/extension.ts

Lines changed: 63 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ interface CommentThread {
9797
}
9898

9999
interface UserFeedback {
100-
feedback_type: 'comment' | 'complete_review' | 'keep_alive';
100+
feedback_type: 'comment' | 'complete_review';
101101
review_id: string;
102102
// For Comment variant
103103
file_path?: string;
@@ -476,96 +476,76 @@ class DaemonClient implements vscode.Disposable {
476476
* This method blocks until the user provides feedback via UI
477477
*/
478478
private async collectUserFeedback(reviewId: string): Promise<UserFeedback> {
479-
// Set up keepalive mechanism
480-
const keepaliveInterval = setInterval(() => {
481-
// Send keepalive back to server
482-
this.sendKeepAlive(reviewId);
483-
}, 30000); // Every 30 seconds
479+
// Show quick pick for user action
480+
const action = await vscode.window.showQuickPick([
481+
{
482+
label: '💬 Add Comment',
483+
description: 'Leave a comment on the review',
484+
action: 'comment'
485+
},
486+
{
487+
label: '✅ Request Changes',
488+
description: 'Ask the agent to make changes',
489+
action: 'request_changes'
490+
},
491+
{
492+
label: '📝 Checkpoint Work',
493+
description: 'Ask the agent to commit and document progress',
494+
action: 'checkpoint'
495+
},
496+
{
497+
label: '↩️ Return to Agent',
498+
description: 'Return control without specific request',
499+
action: 'return'
500+
}
501+
], {
502+
placeHolder: 'What would you like to do with this review?',
503+
ignoreFocusOut: true
504+
});
484505

485-
try {
486-
// Show quick pick for user action
487-
const action = await vscode.window.showQuickPick([
488-
{
489-
label: '💬 Add Comment',
490-
description: 'Leave a comment on the review',
491-
action: 'comment'
492-
},
493-
{
494-
label: '✅ Request Changes',
495-
description: 'Ask the agent to make changes',
496-
action: 'request_changes'
497-
},
498-
{
499-
label: '📝 Checkpoint Work',
500-
description: 'Ask the agent to commit and document progress',
501-
action: 'checkpoint'
502-
},
503-
{
504-
label: '↩️ Return to Agent',
505-
description: 'Return control without specific request',
506-
action: 'return'
507-
}
508-
], {
509-
placeHolder: 'What would you like to do with this review?',
506+
if (!action) {
507+
// User cancelled - return default feedback
508+
return {
509+
feedback_type: 'complete_review',
510+
review_id: reviewId,
511+
completion_action: 'return'
512+
};
513+
}
514+
515+
if (action.action === 'comment') {
516+
// Collect comment from user
517+
const commentText = await vscode.window.showInputBox({
518+
prompt: 'Enter your comment',
519+
placeHolder: 'Type your comment here...',
510520
ignoreFocusOut: true
511521
});
512522

513-
if (!action) {
514-
// User cancelled - return default feedback
515-
return {
516-
feedback_type: 'complete_review',
517-
review_id: reviewId,
518-
completion_action: 'return'
519-
};
520-
}
521-
522-
if (action.action === 'comment') {
523-
// Collect comment from user
524-
const commentText = await vscode.window.showInputBox({
525-
prompt: 'Enter your comment',
526-
placeHolder: 'Type your comment here...',
523+
return {
524+
feedback_type: 'comment',
525+
review_id: reviewId,
526+
comment_text: commentText || '',
527+
file_path: 'review', // TODO: Get actual file if commenting on specific file
528+
line_number: 1 // TODO: Get actual line number
529+
};
530+
} else {
531+
// Completion action
532+
let additionalNotes: string | undefined;
533+
534+
if (action.action === 'request_changes' || action.action === 'checkpoint') {
535+
additionalNotes = await vscode.window.showInputBox({
536+
prompt: 'Any additional notes? (optional)',
537+
placeHolder: 'Additional instructions or context...',
527538
ignoreFocusOut: true
528539
});
529-
530-
return {
531-
feedback_type: 'comment',
532-
review_id: reviewId,
533-
comment_text: commentText || '',
534-
file_path: 'review', // TODO: Get actual file if commenting on specific file
535-
line_number: 1 // TODO: Get actual line number
536-
};
537-
} else {
538-
// Completion action
539-
let additionalNotes: string | undefined;
540-
541-
if (action.action === 'request_changes' || action.action === 'checkpoint') {
542-
additionalNotes = await vscode.window.showInputBox({
543-
prompt: 'Any additional notes? (optional)',
544-
placeHolder: 'Additional instructions or context...',
545-
ignoreFocusOut: true
546-
});
547-
}
548-
549-
return {
550-
feedback_type: 'complete_review',
551-
review_id: reviewId,
552-
completion_action: action.action as 'request_changes' | 'checkpoint' | 'return',
553-
additional_notes: additionalNotes
554-
};
555540
}
556-
} finally {
557-
// Always clear the keepalive interval
558-
clearInterval(keepaliveInterval);
559-
}
560-
}
561541

562-
/**
563-
* Send keepalive message to indicate user is still active
564-
*/
565-
private sendKeepAlive(reviewId: string): void {
566-
this.outputChannel.appendLine(`[KEEPALIVE] Sending keepalive for review: ${reviewId}`);
567-
// Note: This would need to be sent back through the same response channel
568-
// For now, we'll implement this as a separate message or extend the current response mechanism
542+
return {
543+
feedback_type: 'complete_review',
544+
review_id: reviewId,
545+
completion_action: action.action as 'request_changes' | 'checkpoint' | 'return',
546+
additional_notes: additionalNotes
547+
};
548+
}
569549
}
570550

571551
private sendResponse(messageId: string, response: ResponsePayload): void {

server/src/ipc.rs

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -497,13 +497,19 @@ impl IPCCommunicator {
497497

498498
/// Sends an IPC message and waits for a response from VSCode extension
499499
///
500-
/// Sets up response correlation using the message UUID and waits up to 5 seconds
501-
/// for the background reader task to deliver the matching response.
500+
/// Sets up response correlation using the message UUID and waits for response.
502501
/// Uses the underlying `write_message` primitive to send the data.
503502
async fn send_message_with_reply<R>(&self, message: IPCMessage) -> Result<R>
504503
where
505504
R: DeserializeOwned,
506505
{
506+
// Use longer timeout for user feedback messages
507+
let timeout_duration = match message.message_type {
508+
IPCMessageType::CreateSyntheticPr | IPCMessageType::UpdateSyntheticPr => {
509+
std::time::Duration::from_secs(3600) // 1 hour for user interactions
510+
}
511+
_ => std::time::Duration::from_secs(5), // 5 seconds for normal operations
512+
};
507513
debug!(
508514
"Sending IPC message with ID: {} (PID: {})",
509515
message.id,
@@ -530,8 +536,8 @@ impl IPCCommunicator {
530536

531537
trace!("Waiting for response with 5 second timeout...");
532538

533-
// Wait for response with timeout
534-
let response = tokio::time::timeout(std::time::Duration::from_secs(5), rx)
539+
// Wait for response with appropriate timeout
540+
let response = tokio::time::timeout(timeout_duration, rx)
535541
.await
536542
.map_err(|_| {
537543
// Clean up the leaked entry on timeout to fix memory leak
@@ -893,7 +899,6 @@ impl IPCCommunicator {
893899
additional_notes: feedback_payload.additional_notes,
894900
}
895901
}
896-
"keep_alive" => crate::synthetic_pr::FeedbackData::KeepAlive,
897902
_ => crate::synthetic_pr::FeedbackData::CompleteReview {
898903
completion_action: crate::synthetic_pr::CompletionAction::Return,
899904
additional_notes: None,

server/src/server.rs

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,6 @@ impl DialecticServer {
154154
),
155155
}
156156
}
157-
crate::synthetic_pr::FeedbackData::KeepAlive => {
158-
format!(
159-
"User is still reviewing (keepalive received). Please wait for their feedback.\n\n\
160-
Invoke: update_review(review_id: '{}', action: WaitForFeedback)",
161-
&feedback.review_id
162-
)
163-
}
164157
}
165158
}
166159

server/src/synthetic_pr/mcp_tools.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ pub enum FeedbackData {
5656
completion_action: CompletionAction,
5757
additional_notes: Option<String>,
5858
},
59-
KeepAlive,
6059
}
6160

6261
/// Data generated from the working directory and sent over IPC to the extension

0 commit comments

Comments
 (0)