Add repository onboarding to macOS - #424
Conversation
SergeSerb2
left a comment
There was a problem hiding this comment.
Comprehensive repository onboarding feature adds four flows: import folder, clone repository, initialize repository, and publish folder. Well-structured implementation with consistent patterns across UI, model, backend protocol, and implementations. Type-safe request enums, proper async/error handling, and defensive validations. Test coverage is basic but covers the happy path.
Strengths:
- Clean separation of concerns (request types → model orchestration → backend protocol → implementations)
- All backend conformers updated (Live, Mock, default protocol)
- UI properly gates options by mode (e.g., hides git-URL when publishing)
- Defensive nil-safety checks and state reset on mode changes
- RPC contract types are properly Sendable/Codable
Minor observations:
- Protocol params for clone (provider, repository, remoteURL) all optional with no type-level validation—UI ensures valid combos, backend can validate; acceptable
- Tests only cover happy path (request properties, registration); no error cases or RPC mocking
- Onboarding error silently returns nil; relies on model.report(error) to surface it to user (consistent with SurgeCode patterns but could be more explicit)
SurgeCode auto-review · model=codex/gpt-5.6-luna · head=76be4829af35
| @@ -476,7 +612,45 @@ struct NewSessionSheet: View { | |||
| } | |||
There was a problem hiding this comment.
info: When onboardProject returns nil (after calling model.report(error)), the UI silently does nothing. This is consistent with the model-driven error-reporting pattern, but consider making it explicit: store the onboarding error in a @State and display it, or ensure the model's lastError is visibly wired to the sheet. Current behavior relies on error being shown via the model's external error state.
| public func cloneRepository( | ||
| provider: SourceControlProviderKind? = nil, | ||
| repository: String? = nil, | ||
| remoteURL: String? = nil, |
There was a problem hiding this comment.
nit: Parameters provider, repository, and remoteURL are all optional, with no type-level constraint that at least one must be present (e.g., provider+repository or remoteURL alone). The UI builds valid combos and the backend can validate, but consider a docstring clarifying the contract: "either provider+repository or remoteURL must be non-nil."
| #expect(model.projects.contains { $0.path == request.projectPath }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Test coverage is limited to happy-path registration and enum properties. Consider adding tests for: error cases in onboardProject (backend failure, path already registered), deduplication when a path is already a known project, and the model state after onboarding (projects array updated, project returned matches expectations).
Summary
vcs.initRPCs, centralize operation sequencing inAppModel, and cover the workflow with mock-backed tests.This brings the macOS client up to the repository onboarding capabilities already exposed by the server RPC surface.
Area
apps/mac— native macOS appapps/windows— Windows desktop appapps/mobile— iPhone companion appapps/server— backend serverpackages/contracts,packages/shared,packages/client-runtime, …) or relay (infra/relay)Release size
size:XS— tiny internal, documentation, or narrowly scoped fixsize:S— small user-visible fix or contained improvementsize:M— normal feature or meaningful behavior changesize:L— broad feature or substantial cross-package changesize:XL— release-system, architecture, or ecosystem-scale changeVerification
pnpm run verify --allpasses (check + typecheck + tests; addslint:mobileand the Swift suite)pnpm run test:macpasses (required forapps/macchanges)Manually verified (describe below)
pnpm run verifypassed, including 836 SurgeCodeMac tests plus T3Kit and SidecarKit suites.pnpm run test:mac --filter ProjectOnboardingTestspassed (2 tests).pnpm run verify --allpassed formatting and typechecking, then had one load-sensitive timeout in the unrelated serverAcpJsonRpcConnection.test.tssuite; the exact file passed all 16 tests immediately in isolation withvp test run src/provider/acp/AcpJsonRpcConnection.test.ts.Screenshots / Recordings
Not captured: GUI computer-control/screenshot tooling was unavailable in this worktree. The Add Project surface is covered by the native build and model tests above.
Release notes
Add project onboarding can now import folders, clone provider repositories or Git URLs, initialize new Git repositories, and publish local folders.