Fix control message loop and simplify welcome screen repository display #372
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based on user feedback, document the correct flow for pasting on mobile devices: 1. Press paste button on keyboard toolbar first 2. White input box appears as workaround for browser restrictions 3. Long-press in box to access native paste menu This clarifies the non-obvious interaction pattern for mobile users.
The issue was that response messages with errors were being processed by handlers again, creating an infinite loop. Now response messages that aren't pending requests are properly ignored to prevent this loop. The fix: - Skip all response messages that don't have pending request handlers - This prevents error responses from being re-processed by the system handler - Maintains proper handling of expected responses via pendingRequests map
Instead of listing all discovered repositories in a scrollable view, now just show the count. This provides a cleaner UI when there's limited space in the welcome screen. Changes: - Replace ScrollView list with a simple HStack showing repository count - Show appropriate messages for scanning, no repos found, or count - Remove the 10-repository limit since we're only showing count now - Cleaner, more compact UI that fits better in the welcome flow
Claude review from 7/16/2025, 1:55:35 AM (outdated - click to expand)Claude review from 7/16/2025, 1:55:35 AM (outdated - click to expand)Claude review from 7/16/2025, 1:55:35 AM (outdated - click to expand)Claude finished @steipete's task —— View job 📋 SummaryThis PR addresses two distinct issues: an infinite loop in control message handling and UI simplification in the welcome screen. The control message fix prevents error responses from being re-processed indefinitely, while the UI change replaces a scrollable repository list with a cleaner count display. ✅ Positive Aspects
🔍 Areas for ImprovementCode Quality
Potential Issues
Performance
Security
Testing
💡 Suggestions1. Add Scan Cancellation@State private var scanTask: Task<Void, Never>?
private func scanForRepositories() {
scanTask?.cancel() // Cancel any existing scan
isScanning = true
discoveredRepos = []
scanTask = Task {
// ... existing scanning logic
// Check for cancellation periodically
guard !Task.isCancelled else { return }
}
}2. Improve Error Feedback@State private var scanError: String?
// In the UI:
if let error = scanError {
Text(error)
.font(.system(size: 11))
.foregroundColor(.red)
}3. Add Debug Logging for Message Loop Preventionif (message.type === 'response') {
logger.debug(`Ignoring response message that has no pending request: ${message.id}`, {
category: message.category,
action: message.action,
error: message.error
});
return;
}4. Consider Adding TestsFor the control message fix, a test could verify the loop prevention: it('should not process response messages without pending requests', async () => {
const handler = new ControlUnixHandler();
const errorResponse = createControlResponse(mockMessage, null, 'Test error');
// Spy on handler methods to ensure it doesn't recurse
const spy = jest.spyOn(handler, 'handleMacMessage');
await handler.handleMacMessage(errorResponse);
expect(spy).toHaveBeenCalledTimes(1);
});🎯 Priority Items
Overall, this is a solid PR that fixes real issues with minimal, focused changes. The fixes are appropriate and the code quality is good. The suggestions above would enhance robustness but aren't blockers for merging. Reviewed commit: cd33867 |
🔍 Code Quality ReportThis comment is automatically updated with linting results from CI. Node.js Biome Formatting ✅ Status: PassedNode.js Biome Linting ✅ Status: PassedNode.js Test Coverage ✅ Status: PassedClient Coverage: Server Coverage: Mac Formatting (SwiftFormat) ❌ Status: FailedClick to see detailsMac Linting (SwiftLint) ✅ Status: PassedmacOS Test Coverage ❌ Status: Failed• Coverage: 0% |
When all permissions are granted in the welcome screen, the checkmarks now align vertically on the left side for better visual consistency. Buttons remain centered when shown, but granted permissions align to leading edge. - Added alignment: .leading to all three permission status frames - Keeps checkmarks vertically aligned when all permissions are granted - Maintains centered appearance for buttons when permissions not yet granted
Added a link to the blog post 'Command Your Claude Code Army Reloaded' at the bottom of the Control Agent Army welcome page for users who want to learn more about managing multiple AI agent sessions. - Link styled as caption with accent color - Opens in default browser - Provides additional context for the feature
Removed 'Project Folder' and 'Discovered Repositories' labels to save space and simplify the UI. The interface elements are self-explanatory without these labels. - Removed 'Project Folder' label above the path selector - Removed 'Discovered Repositories' label above the repository count - Adjusted spacing to maintain visual balance - Creates a cleaner, less cluttered interface
❌ CI FailedFailed Jobs:
|
Added Spacers at top and bottom to vertically center the content on the welcome screen. This removes the excessive gap at the top and creates a more balanced layout. - Added Spacer() before the title - Added Spacer() after the tip text - Content now appears centered on the page
Restructured the project folder page to match the consistent layout pattern used in the terminal selection page: - Changed to VStack(spacing: 30) outer container - Updated title to largeTitle font - Updated description to body font with maxWidth: 480 - Changed to .padding() instead of .padding(.horizontal, 30) - Set consistent width of 350 for interactive elements - Properly nested VStacks for better vertical centering This creates a more consistent and properly centered layout across all welcome pages.
The settings UI was using undefined dark-* classes (dark-border, dark-text, dark-text-muted) which caused fallback to white borders. Updated to use the proper theme-aware classes: - border-dark-border → border-base - bg-dark-bg-tertiary → bg-tertiary - text-dark-text → text-primary - text-dark-text-muted → text-muted These classes properly use CSS variables that adapt to the current theme, providing the correct soft grey borders in dark mode.
Claude review from 7/16/2025, 2:12:57 AM (outdated - click to expand)Claude review from 7/16/2025, 2:12:57 AM (outdated - click to expand)Claude finished @steipete's task —— View job 📋 SummaryThis PR addresses two issues: fixing an infinite loop in control message handling for repository-path-update responses and simplifying the welcome screen to show repository count instead of a full list. The changes are minimal and focused on solving specific problems. ✅ Positive Aspects
🔍 Areas for ImprovementCode Quality
Potential Issues
Performance
Security
Testing
💡 Suggestions1. Add Scan Cancellation@State private var scanTask: Task<Void, Never>?
private func scanForRepositories() {
scanTask?.cancel() // Cancel existing scan
isScanning = true
discoveredRepos = []
scanTask = Task {
let expandedPath = (selectedPath as NSString).expandingTildeInPath
let repos = await findGitRepositories(in: expandedPath, maxDepth: 3)
guard !Task.isCancelled else { return }
await MainActor.run {
discoveredRepos = repos.map { path in
RepositoryInfo(name: URL(fileURLWithPath: path).lastPathComponent, path: path)
}
isScanning = false
}
}
}2. Add Debug Loggingif (message.type === 'response') {
logger.debug(`Ignoring response message that has no pending request: ${message.id}`, {
category: message.category,
action: message.action,
error: message.error
});
return;
}3. Add Test for Loop Preventiondescribe('ControlUnixHandler', () => {
it('should not process response messages without pending requests', async () => {
const handler = new ControlUnixHandler();
const errorResponse = createControlResponse(mockMessage, null, 'Test error');
const spy = jest.spyOn(handler, 'handleMacMessage');
await handler.handleMacMessage(errorResponse);
expect(spy).toHaveBeenCalledTimes(1); // Should not recurse
});
});4. Fix Swift FormattingRun 🎯 Priority Items
Overall, this is a well-targeted PR that fixes real issues with minimal changes. The control message fix is particularly important as it prevents server instability. The UI simplification is a good UX improvement. With the formatting fixes and some additional safeguards for the scanning operation, this PR will be ready to merge. Reviewed commit: af2b038 |
- Added detailed logging to track repository-path-update messages - Fixed issue where Mac app updates weren't being processed correctly - Added comprehensive tests for repository path synchronization - Ensured updateRepositoryPath method properly invokes config callback - Added tests for Mac message handling including edge cases
|
Claude finished @steipete's task —— View job 📋 SummaryThis PR addresses two distinct issues: an infinite loop in control message handling for repository-path-update responses and UI simplification in the welcome screen. The control message fix prevents error responses from being re-processed indefinitely, while the UI change replaces a scrollable repository list with a cleaner count display showing the number of discovered repositories. ✅ Positive Aspects
🔍 Areas for ImprovementCode Quality
Potential Issues
Performance
Security
Testing
💡 Suggestions1. Add Scan Cancellation@State private var scanTask: Task<Void, Never>?
private func scanForRepositories() {
scanTask?.cancel() // Cancel existing scan
isScanning = true
discoveredRepos = []
scanTask = Task {
let expandedPath = (selectedPath as NSString).expandingTildeInPath
let repos = await findGitRepositories(in: expandedPath, maxDepth: 3)
guard !Task.isCancelled else { return }
await MainActor.run {
discoveredRepos = repos.map { path in
RepositoryInfo(name: URL(fileURLWithPath: path).lastPathComponent, path: path)
}
isScanning = false
}
}
}2. Add Test for Loop Preventiondescribe('Control message loop prevention', () => {
it('should not process response messages without pending requests', async () => {
const handler = controlUnixHandler as any;
const errorResponse = {
id: 'test-123',
type: 'response' as const,
category: 'system' as const,
action: 'repository-path-update',
error: 'Test error'
};
// Spy on handleMessage to ensure it doesn't recurse
const spy = vi.spyOn(handler, 'handleMacMessage');
// Clear any pending requests
handler.pendingRequests.clear();
// Process the response
await handler.handleMacMessage(errorResponse);
// Should only be called once, not recursively
expect(spy).toHaveBeenCalledTimes(1);
// Should not have tried to send another response
expect(handler.sendToMac).not.toHaveBeenCalled();
});
});3. Add Progress Feedback// In ProjectFolderPageView
@State private var scanProgress: Double = 0.0
// Show progress during scan
if isScanning {
ProgressView(value: scanProgress, total: 1.0)
.progressViewStyle(.linear)
.frame(width: 350)
}4. Fix Swift FormattingRun 5. Improve Empty State Message} else if discoveredRepos.isEmpty {
VStack(spacing: 4) {
Text("No repositories found")
.font(.system(size: 12))
.foregroundColor(.secondary)
Text("Try selecting a folder containing Git projects")
.font(.system(size: 10))
.foregroundColor(.tertiary)
}
}🎯 Priority Items
Overall, this is a well-targeted PR that fixes real issues. The control message loop fix is particularly important as it prevents server instability. The UI simplification improves user experience by showing relevant information in a compact format. With the formatting fixes and additional safeguards for the scanning operation, this PR will be ready to merge. Reviewed commit: d80ab69 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Control Message Loop Fix
The server was getting into an infinite loop when processing repository-path-update messages:
Fix: Added a check to skip processing response messages that don't have pending request handlers. This prevents error responses from being re-processed.
Welcome Screen UI Improvement
The Project Folder page in the welcome flow had limited space but was trying to show a scrollable list of all discovered repositories.
Fix: Replaced the ScrollView list with a simple count display:
Test Plan