feat(tui): add project enrollment toggles#610
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds cloud sync settings, configuration persistence, health checks, status and enrollment screens to the TUI. It also updates pending mutation counting, token fallback documentation, and related test coverage. ChangesCloud sync functionality
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TUI
participant pingCloudServer
participant CloudServer
participant cloud.json
TUI->>pingCloudServer: Validate URL and send effective token
pingCloudServer->>CloudServer: GET /health
CloudServer-->>pingCloudServer: Return HTTP health status
pingCloudServer-->>TUI: Return ping result
TUI->>cloud.json: Save reachable or unauthorized server URL
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds the TUI “Cloud sync settings” flow (settings menu → config/status/enrollment screens) so users can configure the cloud server URL, view sync status, and enroll/unenroll projects directly from the terminal UI. This integrates with existing store enrollment methods and adds small store helpers to support the status screen.
Changes:
- Adds new TUI screens: Cloud Settings menu, Cloud Config form (ping + save), Cloud Status, and Project Enrollment toggles.
- Introduces TUI cloud config persistence helpers (
cloud.jsonserver_url write while preserving any existing token) and a health ping helper. - Extends store with
DataDir()andCountPendingSyncMutations()to support TUI status rendering; updates troubleshooting docs to clarify token fallback behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/view.go | Adds view router cases and renders new cloud settings/config/status/enrollment screens. |
| internal/tui/update.go | Adds key handling and message update flows for cloud screens (navigation, ping/save, enrollment toggles). |
| internal/tui/model.go | Adds new screen constants, cloud-specific model fields, and load commands. |
| internal/tui/cloud.go | Implements cloud.json load/save, token source resolution, URL validation, and /health ping. |
| internal/tui/view_test.go | Adds coverage for cloud screen rendering branches and router coverage. |
| internal/tui/update_test.go | Adds navigation + behavior tests for cloud flows (save/ping/enrollment toggles). |
| internal/tui/model_test.go | Adds tests for new screen constants and cloud load commands. |
| internal/tui/cloud_test.go | Adds unit tests for cloud.json behavior, token source precedence, ping behavior, and URL validation. |
| internal/store/store.go | Adds DataDir() and CountPendingSyncMutations() for TUI status/pending counts. |
| internal/store/store_test.go | Adds tests for DataDir() and CountPendingSyncMutations(). |
| docs/engram-cloud/troubleshooting.md | Documents intentional cloud.json.token fallback and env precedence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| b.WriteString("\n") | ||
| } | ||
|
|
||
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) |
| if m.CloudStatusServerURL == "" { | ||
| b.WriteString(noResultsStyle.Render("Cloud status: not configured")) | ||
| b.WriteString("\n") | ||
| b.WriteString(helpStyle.Render("\n esc/q back")) | ||
| return b.String() | ||
| } |
| if len(m.CloudEnrollmentItems) == 0 { | ||
| b.WriteString(noResultsStyle.Render("No projects found.")) | ||
| b.WriteString("\n\n") | ||
| if m.CloudEnrollmentError != "" { | ||
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | ||
| b.WriteString("\n") | ||
| } | ||
| b.WriteString(helpStyle.Render(" esc back")) | ||
| return b.String() | ||
| } |
| case "enter", " ": | ||
| switch m.Cursor { |
| m.CloudConfigError = "" | ||
| m.CloudConfigPingStatus = "" | ||
| validatedURL, err := validateCloudServerURL(m.CloudConfigInput.Value()) | ||
| if err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| m.CloudConfigTest = testOnly | ||
| m.CloudConfigSaving = !testOnly | ||
| return m, pingCloudServer(validatedURL, effectiveCloudToken(m.store.DataDir())) |
| return "unreachable", err | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/store_test.go`:
- Around line 1392-1436: Extend TestCountPendingSyncMutations to create a
mutation for a project that is not enrolled, then verify
CountPendingSyncMutations excludes it while still counting the enrolled “engram”
mutations. Keep the existing enrolled-project assertions intact and use the
available session/observation creation helpers to exercise the enrollment
filter.
In `@internal/tui/cloud_test.go`:
- Around line 48-75: Extend the token-source tests with a malformed cloud
configuration case using tokenSourceMessage: write invalid JSON via
writeCloudJSON, clear ENGRAM_CLOUD_TOKEN, and assert the result is
TokenSourceNone, covering the loadCloudConfig error fallback.
In `@internal/tui/cloud.go`:
- Around line 132-154: Update validateCloudServerURL to normalize the parsed
path by removing a trailing slash before returning the canonical URL, so a root
URL such as https://cloud.example.com/ becomes https://cloud.example.com.
Preserve non-root paths and existing query/fragment validation, and ensure
pingCloudServerStatus no longer receives a URL that creates a double slash
before cloudHealthPath.
- Around line 53-67: Update saveCloudConfig to create dataDir with owner-only
permissions (0700) and write cloud.json with owner-only permissions (0600). Also
explicitly tighten permissions on an existing cloudConfigPath before or after
writing, so previously world-readable configuration files containing tokens are
migrated to owner-only access.
In `@internal/tui/update.go`:
- Around line 154-162: Remove the unconditional CloudEnrollmentError reset from
the successful cloudEnrollmentLoadedMsg branch in the update handler, while
retaining item updates and loading-state changes. Ensure the existing
screen-entry reset in handleCloudSettingsKeys remains responsible for clearing
stale errors so enrollment/unenrollment failures survive a successful reload.
In `@internal/tui/view.go`:
- Around line 242-319: Update viewCloudStatus and viewCloudEnrollment to check
CloudStatusLoading and CloudEnrollmentLoading before rendering their empty or
configured content, respectively. While either corresponding flag is true,
render the existing spinner/loading indicator pattern used by the
CloudConfigSaving view (for example, the “Pinging server...” state) and retain
the current content rendering once loading completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ec4dfbe-fc3c-46b7-9a76-bcd5d5896ad9
📒 Files selected for processing (11)
docs/engram-cloud/troubleshooting.mdinternal/store/store.gointernal/store/store_test.gointernal/tui/cloud.gointernal/tui/cloud_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/update.gointernal/tui/update_test.gointernal/tui/view.gointernal/tui/view_test.go
| func TestCountPendingSyncMutations(t *testing.T) { | ||
| s := newTestStore(t) | ||
|
|
||
| count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey) | ||
| if err != nil { | ||
| t.Fatalf("CountPendingSyncMutations empty: %v", err) | ||
| } | ||
| if count != 0 { | ||
| t.Fatalf("count = %d, want 0", count) | ||
| } | ||
|
|
||
| if err := s.EnrollProject("engram"); err != nil { | ||
| t.Fatalf("enroll: %v", err) | ||
| } | ||
| if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { | ||
| t.Fatalf("create session: %v", err) | ||
| } | ||
|
|
||
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | ||
| if err != nil { | ||
| t.Fatalf("CountPendingSyncMutations after session: %v", err) | ||
| } | ||
| if count != 1 { | ||
| t.Fatalf("count = %d, want 1", count) | ||
| } | ||
|
|
||
| if _, err := s.AddObservation(AddObservationParams{ | ||
| SessionID: "s1", | ||
| Type: "bugfix", | ||
| Title: "one", | ||
| Content: "content", | ||
| Project: "engram", | ||
| Scope: "project", | ||
| }); err != nil { | ||
| t.Fatalf("add observation: %v", err) | ||
| } | ||
|
|
||
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | ||
| if err != nil { | ||
| t.Fatalf("CountPendingSyncMutations after observation: %v", err) | ||
| } | ||
| if count != 2 { | ||
| t.Fatalf("count = %d, want 2", count) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing negative-path coverage for the enrollment exclusion filter.
The test only exercises mutations for the enrolled "engram" project; it never creates a mutation for a project that is not enrolled, so the core new gate (sm.project = '' OR sep.project IS NOT NULL) in CountPendingSyncMutations is never verified to actually exclude non-enrolled projects.
As per path instructions, **/*_test.go should "Verify coverage of happy path, error paths, and edge cases."
🧪 Suggested addition
count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey)
if err != nil {
t.Fatalf("CountPendingSyncMutations after observation: %v", err)
}
if count != 2 {
t.Fatalf("count = %d, want 2", count)
}
+
+ // A mutation for a non-enrolled project must not be counted.
+ if err := s.CreateSession("s2", "other-project", "/tmp/other"); err != nil {
+ t.Fatalf("create session for unenrolled project: %v", err)
+ }
+ count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey)
+ if err != nil {
+ t.Fatalf("CountPendingSyncMutations after unenrolled mutation: %v", err)
+ }
+ if count != 2 {
+ t.Fatalf("count = %d, want 2 (unenrolled project mutation should be excluded)", count)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestCountPendingSyncMutations(t *testing.T) { | |
| s := newTestStore(t) | |
| count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations empty: %v", err) | |
| } | |
| if count != 0 { | |
| t.Fatalf("count = %d, want 0", count) | |
| } | |
| if err := s.EnrollProject("engram"); err != nil { | |
| t.Fatalf("enroll: %v", err) | |
| } | |
| if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { | |
| t.Fatalf("create session: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after session: %v", err) | |
| } | |
| if count != 1 { | |
| t.Fatalf("count = %d, want 1", count) | |
| } | |
| if _, err := s.AddObservation(AddObservationParams{ | |
| SessionID: "s1", | |
| Type: "bugfix", | |
| Title: "one", | |
| Content: "content", | |
| Project: "engram", | |
| Scope: "project", | |
| }); err != nil { | |
| t.Fatalf("add observation: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after observation: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2", count) | |
| } | |
| } | |
| func TestCountPendingSyncMutations(t *testing.T) { | |
| s := newTestStore(t) | |
| count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations empty: %v", err) | |
| } | |
| if count != 0 { | |
| t.Fatalf("count = %d, want 0", count) | |
| } | |
| if err := s.EnrollProject("engram"); err != nil { | |
| t.Fatalf("enroll: %v", err) | |
| } | |
| if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { | |
| t.Fatalf("create session: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after session: %v", err) | |
| } | |
| if count != 1 { | |
| t.Fatalf("count = %d, want 1", count) | |
| } | |
| if _, err := s.AddObservation(AddObservationParams{ | |
| SessionID: "s1", | |
| Type: "bugfix", | |
| Title: "one", | |
| Content: "content", | |
| Project: "engram", | |
| Scope: "project", | |
| }); err != nil { | |
| t.Fatalf("add observation: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after observation: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2", count) | |
| } | |
| // A mutation for a non-enrolled project must not be counted. | |
| if err := s.CreateSession("s2", "other-project", "/tmp/other"); err != nil { | |
| t.Fatalf("create session for unenrolled project: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after unenrolled mutation: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2 (unenrolled project mutation should be excluded)", count) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/store_test.go` around lines 1392 - 1436, Extend
TestCountPendingSyncMutations to create a mutation for a project that is not
enrolled, then verify CountPendingSyncMutations excludes it while still counting
the enrolled “engram” mutations. Keep the existing enrolled-project assertions
intact and use the available session/observation creation helpers to exercise
the enrollment filter.
Source: Path instructions
| func TestTokenSourceEnvOverridesFile(t *testing.T) { | ||
| dir := t.TempDir() | ||
| writeCloudJSON(t, dir, `{"token":"file-token"}`) | ||
|
|
||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "env-token") | ||
| if got := tokenSourceMessage(dir); got != TokenSourceEnv { | ||
| t.Fatalf("source = %q, want %q", got, TokenSourceEnv) | ||
| } | ||
| } | ||
|
|
||
| func TestTokenSourceFileFallback(t *testing.T) { | ||
| dir := t.TempDir() | ||
| writeCloudJSON(t, dir, `{"token":"file-token"}`) | ||
|
|
||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "") | ||
| if got := tokenSourceMessage(dir); got != TokenSourceFile { | ||
| t.Fatalf("source = %q, want %q", got, TokenSourceFile) | ||
| } | ||
| } | ||
|
|
||
| func TestTokenSourceNone(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "") | ||
| if got := tokenSourceMessage(dir); got != TokenSourceNone { | ||
| t.Fatalf("source = %q, want %q", got, TokenSourceNone) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing edge-case test: malformed cloud.json.
loadCloudConfig returns an error on invalid JSON, which tokenSourceMessage/effectiveCloudToken silently swallow (falling back to TokenSourceNone/""). No test exercises this path. As per path instructions, tests should verify error paths and edge cases.
✅ Suggested test
func TestTokenSourceCorruptJSONFallsBackToNone(t *testing.T) {
dir := t.TempDir()
writeCloudJSON(t, dir, `{invalid`)
t.Setenv("ENGRAM_CLOUD_TOKEN", "")
if got := tokenSourceMessage(dir); got != TokenSourceNone {
t.Fatalf("source = %q, want %q", got, TokenSourceNone)
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/cloud_test.go` around lines 48 - 75, Extend the token-source
tests with a malformed cloud configuration case using tokenSourceMessage: write
invalid JSON via writeCloudJSON, clear ENGRAM_CLOUD_TOKEN, and assert the result
is TokenSourceNone, covering the loadCloudConfig error fallback.
Source: Path instructions
| func saveCloudConfig(dataDir, serverURL string) error { | ||
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | ||
| return err | ||
| } | ||
| cc, err := loadCloudConfig(dataDir) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| cc.ServerURL = serverURL | ||
| b, err := json.MarshalIndent(cc, "", " ") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
cloud.json is written world-readable, exposing the stored token.
saveCloudConfig persists the config (which may contain token, per the fallback documented in docs/engram-cloud/troubleshooting.md) via os.WriteFile(..., 0o644), and the directory is created with os.MkdirAll(dataDir, 0o755). On multi-user systems this makes the cloud token readable by any local user. Credential files should use owner-only permissions (0600/0700), matching conventions used by tools like AWS CLI and SSH.
Note: since os.WriteFile does not change permissions of an already-existing file, an existing world-readable cloud.json won't be tightened by this fix alone; a first-run migration or explicit os.Chmod may be needed for pre-existing files.
🔒 Proposed fix to restrict permissions
func saveCloudConfig(dataDir, serverURL string) error {
- if err := os.MkdirAll(dataDir, 0o755); err != nil {
+ if err := os.MkdirAll(dataDir, 0o700); err != nil {
return err
}
cc, err := loadCloudConfig(dataDir)
if err != nil {
return err
}
cc.ServerURL = serverURL
b, err := json.MarshalIndent(cc, "", " ")
if err != nil {
return err
}
- return os.WriteFile(cloudConfigPath(dataDir), b, 0o644)
+ return os.WriteFile(cloudConfigPath(dataDir), b, 0o600)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func saveCloudConfig(dataDir, serverURL string) error { | |
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | |
| return err | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return err | |
| } | |
| cc.ServerURL = serverURL | |
| b, err := json.MarshalIndent(cc, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) | |
| } | |
| func saveCloudConfig(dataDir, serverURL string) error { | |
| if err := os.MkdirAll(dataDir, 0o700); err != nil { | |
| return err | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return err | |
| } | |
| cc.ServerURL = serverURL | |
| b, err := json.MarshalIndent(cc, "", " ") | |
| if err != nil { | |
| return err | |
| } | |
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o600) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/cloud.go` around lines 53 - 67, Update saveCloudConfig to create
dataDir with owner-only permissions (0700) and write cloud.json with owner-only
permissions (0600). Also explicitly tighten permissions on an existing
cloudConfigPath before or after writing, so previously world-readable
configuration files containing tokens are migrated to owner-only access.
| func validateCloudServerURL(raw string) (string, error) { | ||
| trimmed := strings.TrimSpace(raw) | ||
| parsed, err := url.ParseRequestURI(trimmed) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| scheme := strings.ToLower(strings.TrimSpace(parsed.Scheme)) | ||
| if scheme != "http" && scheme != "https" { | ||
| return "", fmt.Errorf("scheme must be http or https") | ||
| } | ||
| if strings.TrimSpace(parsed.Host) == "" || strings.TrimSpace(parsed.Hostname()) == "" { | ||
| return "", fmt.Errorf("host is required") | ||
| } | ||
| if strings.TrimSpace(parsed.RawQuery) != "" { | ||
| return "", fmt.Errorf("query is not allowed") | ||
| } | ||
| if strings.TrimSpace(parsed.Fragment) != "" { | ||
| return "", fmt.Errorf("fragment is not allowed") | ||
| } | ||
| parsed.RawQuery = "" | ||
| parsed.Fragment = "" | ||
| return parsed.String(), nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trailing slash in serverURL produces a malformed health-check URL.
If the user configures a server URL with a trailing slash (e.g. https://cloud.example.com/), parsed.Path becomes "/", which is not stripped here. pingCloudServerStatus then concatenates cloudHealthPath directly, producing https://cloud.example.com//health — likely a 404/unreachable result even though the server is up. This isn't covered by the existing TestValidateCloudServerURL cases.
🐛 Proposed fix to normalize trailing slash
parsed.RawQuery = ""
parsed.Fragment = ""
+ parsed.Path = strings.TrimRight(parsed.Path, "/")
return parsed.String(), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validateCloudServerURL(raw string) (string, error) { | |
| trimmed := strings.TrimSpace(raw) | |
| parsed, err := url.ParseRequestURI(trimmed) | |
| if err != nil { | |
| return "", err | |
| } | |
| scheme := strings.ToLower(strings.TrimSpace(parsed.Scheme)) | |
| if scheme != "http" && scheme != "https" { | |
| return "", fmt.Errorf("scheme must be http or https") | |
| } | |
| if strings.TrimSpace(parsed.Host) == "" || strings.TrimSpace(parsed.Hostname()) == "" { | |
| return "", fmt.Errorf("host is required") | |
| } | |
| if strings.TrimSpace(parsed.RawQuery) != "" { | |
| return "", fmt.Errorf("query is not allowed") | |
| } | |
| if strings.TrimSpace(parsed.Fragment) != "" { | |
| return "", fmt.Errorf("fragment is not allowed") | |
| } | |
| parsed.RawQuery = "" | |
| parsed.Fragment = "" | |
| return parsed.String(), nil | |
| } | |
| func validateCloudServerURL(raw string) (string, error) { | |
| trimmed := strings.TrimSpace(raw) | |
| parsed, err := url.ParseRequestURI(trimmed) | |
| if err != nil { | |
| return "", err | |
| } | |
| scheme := strings.ToLower(strings.TrimSpace(parsed.Scheme)) | |
| if scheme != "http" && scheme != "https" { | |
| return "", fmt.Errorf("scheme must be http or https") | |
| } | |
| if strings.TrimSpace(parsed.Host) == "" || strings.TrimSpace(parsed.Hostname()) == "" { | |
| return "", fmt.Errorf("host is required") | |
| } | |
| if strings.TrimSpace(parsed.RawQuery) != "" { | |
| return "", fmt.Errorf("query is not allowed") | |
| } | |
| if strings.TrimSpace(parsed.Fragment) != "" { | |
| return "", fmt.Errorf("fragment is not allowed") | |
| } | |
| parsed.RawQuery = "" | |
| parsed.Fragment = "" | |
| parsed.Path = strings.TrimRight(parsed.Path, "/") | |
| return parsed.String(), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/cloud.go` around lines 132 - 154, Update validateCloudServerURL
to normalize the parsed path by removing a trailing slash before returning the
canonical URL, so a root URL such as https://cloud.example.com/ becomes
https://cloud.example.com. Preserve non-root paths and existing query/fragment
validation, and ensure pingCloudServerStatus no longer receives a URL that
creates a double slash before cloudHealthPath.
| case cloudEnrollmentLoadedMsg: | ||
| m.CloudEnrollmentLoading = false | ||
| if msg.err != nil { | ||
| m.CloudEnrollmentError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| m.CloudEnrollmentItems = msg.items | ||
| m.CloudEnrollmentError = "" | ||
| return m, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enrollment/unenrollment errors are wiped by the immediately-following successful reload.
In the space-key handler, on EnrollProject/UnenrollProject failure, m.CloudEnrollmentError is set (line 789/793) and then loadCloudEnrollmentCmd(m.store) is unconditionally returned (line 796). If that reload succeeds — plausible whenever the toggle fails for a reason unrelated to store health (e.g. a validation error) — the cloudEnrollmentLoadedMsg success branch (lines 160-161) unconditionally clears CloudEnrollmentError, so the error is visible for at most one render frame before being erased. TestCloudEnrollmentToggleErrorSurfaced only covers the case where the store is closed (so both the toggle and the reload fail), which doesn't exercise this path.
🐛 Proposed fix
case cloudEnrollmentLoadedMsg:
m.CloudEnrollmentLoading = false
if msg.err != nil {
m.CloudEnrollmentError = msg.err.Error()
return m, nil
}
m.CloudEnrollmentItems = msg.items
- m.CloudEnrollmentError = ""
return m, nilThis is safe: every screen-entry path (e.g. handleCloudSettingsKeys case 2) already resets CloudEnrollmentError = "" before firing the load command, so this line was only ever clobbering a legitimate toggle error, never providing a needed reset.
Also applies to: 783-797
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/update.go` around lines 154 - 162, Remove the unconditional
CloudEnrollmentError reset from the successful cloudEnrollmentLoadedMsg branch
in the update handler, while retaining item updates and loading-state changes.
Ensure the existing screen-entry reset in handleCloudSettingsKeys remains
responsible for clearing stale errors so enrollment/unenrollment failures
survive a successful reload.
| func (m Model) viewCloudStatus() string { | ||
| var b strings.Builder | ||
|
|
||
| b.WriteString(headerStyle.Render(" Cloud status")) | ||
| b.WriteString("\n\n") | ||
|
|
||
| if m.CloudStatusServerURL == "" { | ||
| b.WriteString(noResultsStyle.Render("Cloud status: not configured")) | ||
| b.WriteString("\n") | ||
| b.WriteString(helpStyle.Render("\n esc/q back")) | ||
| return b.String() | ||
| } | ||
|
|
||
| b.WriteString(renderCloudStatusRow("Server URL:", m.CloudStatusServerURL)) | ||
| b.WriteString(renderCloudStatusRow("Health:", renderCloudHealth(m.CloudStatusHealth))) | ||
| b.WriteString(renderCloudStatusRow("Token source:", m.CloudStatusTokenSource)) | ||
| b.WriteString(renderCloudStatusRow("Last sync:", m.CloudStatusLastSync)) | ||
| b.WriteString(renderCloudStatusRow("Pending:", fmt.Sprintf("%d", m.CloudStatusPendingCount))) | ||
| lastError := m.CloudStatusLastError | ||
| if lastError == "" { | ||
| lastError = "none" | ||
| } | ||
| b.WriteString(renderCloudStatusRow("Last error:", lastError)) | ||
|
|
||
| b.WriteString(helpStyle.Render("\n esc/q back")) | ||
|
|
||
| return b.String() | ||
| } | ||
|
|
||
| func (m Model) viewCloudEnrollment() string { | ||
| var b strings.Builder | ||
|
|
||
| b.WriteString(headerStyle.Render(" Enroll projects")) | ||
| b.WriteString("\n\n") | ||
|
|
||
| if len(m.CloudEnrollmentItems) == 0 { | ||
| b.WriteString(noResultsStyle.Render("No projects found.")) | ||
| b.WriteString("\n\n") | ||
| if m.CloudEnrollmentError != "" { | ||
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | ||
| b.WriteString("\n") | ||
| } | ||
| b.WriteString(helpStyle.Render(" esc back")) | ||
| return b.String() | ||
| } | ||
|
|
||
| for i, item := range m.CloudEnrollmentItems { | ||
| cursor := " " | ||
| style := listItemStyle | ||
| if i == m.Cursor { | ||
| cursor = "▸ " | ||
| style = listSelectedStyle | ||
| } | ||
|
|
||
| badge := "[not enrolled]" | ||
| badgeStyle := timestampStyle | ||
| if item.enrolled { | ||
| badge = "[enrolled]" | ||
| badgeStyle = lipgloss.NewStyle().Foreground(colorGreen).Bold(true) | ||
| } | ||
|
|
||
| line := fmt.Sprintf("%s%s %s", | ||
| cursor, | ||
| style.Render(truncateStr(item.project, 50)), | ||
| badgeStyle.Render(badge)) | ||
| b.WriteString(line) | ||
| b.WriteString("\n") | ||
| } | ||
|
|
||
| if m.CloudEnrollmentError != "" { | ||
| b.WriteString("\n") | ||
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | ||
| } | ||
|
|
||
| b.WriteString(helpStyle.Render("\n j/k navigate • space toggle • esc/q back")) | ||
|
|
||
| return b.String() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CloudStatusLoading/CloudEnrollmentLoading are tracked but never rendered.
Both flags are set to true on screen entry and cleared when the corresponding *LoadedMsg arrives, but viewCloudStatus and viewCloudEnrollment never check them. Until the async load completes, users briefly see the misleading "Cloud status: not configured" / "No projects found." messages instead of a loading indicator — even though the screen for CloudConfigSaving correctly renders a spinner ("Pinging server...") for the analogous state at lines 226-230, showing the pattern was simply missed here.
💡 Proposed fix
func (m Model) viewCloudStatus() string {
var b strings.Builder
b.WriteString(headerStyle.Render(" Cloud status"))
b.WriteString("\n\n")
+ if m.CloudStatusLoading {
+ b.WriteString(noResultsStyle.Render("Loading cloud status..."))
+ b.WriteString("\n")
+ b.WriteString(helpStyle.Render("\n esc/q back"))
+ return b.String()
+ }
+
if m.CloudStatusServerURL == "" { func (m Model) viewCloudEnrollment() string {
var b strings.Builder
b.WriteString(headerStyle.Render(" Enroll projects"))
b.WriteString("\n\n")
+ if m.CloudEnrollmentLoading {
+ b.WriteString(noResultsStyle.Render("Loading projects..."))
+ b.WriteString("\n\n")
+ b.WriteString(helpStyle.Render(" esc back"))
+ return b.String()
+ }
+
if len(m.CloudEnrollmentItems) == 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m Model) viewCloudStatus() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Cloud status")) | |
| b.WriteString("\n\n") | |
| if m.CloudStatusServerURL == "" { | |
| b.WriteString(noResultsStyle.Render("Cloud status: not configured")) | |
| b.WriteString("\n") | |
| b.WriteString(helpStyle.Render("\n esc/q back")) | |
| return b.String() | |
| } | |
| b.WriteString(renderCloudStatusRow("Server URL:", m.CloudStatusServerURL)) | |
| b.WriteString(renderCloudStatusRow("Health:", renderCloudHealth(m.CloudStatusHealth))) | |
| b.WriteString(renderCloudStatusRow("Token source:", m.CloudStatusTokenSource)) | |
| b.WriteString(renderCloudStatusRow("Last sync:", m.CloudStatusLastSync)) | |
| b.WriteString(renderCloudStatusRow("Pending:", fmt.Sprintf("%d", m.CloudStatusPendingCount))) | |
| lastError := m.CloudStatusLastError | |
| if lastError == "" { | |
| lastError = "none" | |
| } | |
| b.WriteString(renderCloudStatusRow("Last error:", lastError)) | |
| b.WriteString(helpStyle.Render("\n esc/q back")) | |
| return b.String() | |
| } | |
| func (m Model) viewCloudEnrollment() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Enroll projects")) | |
| b.WriteString("\n\n") | |
| if len(m.CloudEnrollmentItems) == 0 { | |
| b.WriteString(noResultsStyle.Render("No projects found.")) | |
| b.WriteString("\n\n") | |
| if m.CloudEnrollmentError != "" { | |
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString(helpStyle.Render(" esc back")) | |
| return b.String() | |
| } | |
| for i, item := range m.CloudEnrollmentItems { | |
| cursor := " " | |
| style := listItemStyle | |
| if i == m.Cursor { | |
| cursor = "▸ " | |
| style = listSelectedStyle | |
| } | |
| badge := "[not enrolled]" | |
| badgeStyle := timestampStyle | |
| if item.enrolled { | |
| badge = "[enrolled]" | |
| badgeStyle = lipgloss.NewStyle().Foreground(colorGreen).Bold(true) | |
| } | |
| line := fmt.Sprintf("%s%s %s", | |
| cursor, | |
| style.Render(truncateStr(item.project, 50)), | |
| badgeStyle.Render(badge)) | |
| b.WriteString(line) | |
| b.WriteString("\n") | |
| } | |
| if m.CloudEnrollmentError != "" { | |
| b.WriteString("\n") | |
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | |
| } | |
| b.WriteString(helpStyle.Render("\n j/k navigate • space toggle • esc/q back")) | |
| return b.String() | |
| } | |
| func (m Model) viewCloudStatus() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Cloud status")) | |
| b.WriteString("\n\n") | |
| if m.CloudStatusLoading { | |
| b.WriteString(noResultsStyle.Render("Loading cloud status...")) | |
| b.WriteString("\n") | |
| b.WriteString(helpStyle.Render("\n esc/q back")) | |
| return b.String() | |
| } | |
| if m.CloudStatusServerURL == "" { | |
| b.WriteString(noResultsStyle.Render("Cloud status: not configured")) | |
| b.WriteString("\n") | |
| b.WriteString(helpStyle.Render("\n esc/q back")) | |
| return b.String() | |
| } | |
| b.WriteString(renderCloudStatusRow("Server URL:", m.CloudStatusServerURL)) | |
| b.WriteString(renderCloudStatusRow("Health:", renderCloudHealth(m.CloudStatusHealth))) | |
| b.WriteString(renderCloudStatusRow("Token source:", m.CloudStatusTokenSource)) | |
| b.WriteString(renderCloudStatusRow("Last sync:", m.CloudStatusLastSync)) | |
| b.WriteString(renderCloudStatusRow("Pending:", fmt.Sprintf("%d", m.CloudStatusPendingCount))) | |
| lastError := m.CloudStatusLastError | |
| if lastError == "" { | |
| lastError = "none" | |
| } | |
| b.WriteString(renderCloudStatusRow("Last error:", lastError)) | |
| b.WriteString(helpStyle.Render("\n esc/q back")) | |
| return b.String() | |
| } | |
| func (m Model) viewCloudEnrollment() string { | |
| var b strings.Builder | |
| b.WriteString(headerStyle.Render(" Enroll projects")) | |
| b.WriteString("\n\n") | |
| if m.CloudEnrollmentLoading { | |
| b.WriteString(noResultsStyle.Render("Loading projects...")) | |
| b.WriteString("\n\n") | |
| b.WriteString(helpStyle.Render(" esc back")) | |
| return b.String() | |
| } | |
| if len(m.CloudEnrollmentItems) == 0 { | |
| b.WriteString(noResultsStyle.Render("No projects found.")) | |
| b.WriteString("\n\n") | |
| if m.CloudEnrollmentError != "" { | |
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString(helpStyle.Render(" esc back")) | |
| return b.String() | |
| } | |
| for i, item := range m.CloudEnrollmentItems { | |
| cursor := " " | |
| style := listItemStyle | |
| if i == m.Cursor { | |
| cursor = "▸ " | |
| style = listSelectedStyle | |
| } | |
| badge := "[not enrolled]" | |
| badgeStyle := timestampStyle | |
| if item.enrolled { | |
| badge = "[enrolled]" | |
| badgeStyle = lipgloss.NewStyle().Foreground(colorGreen).Bold(true) | |
| } | |
| line := fmt.Sprintf("%s%s %s", | |
| cursor, | |
| style.Render(truncateStr(item.project, 50)), | |
| badgeStyle.Render(badge)) | |
| b.WriteString(line) | |
| b.WriteString("\n") | |
| } | |
| if m.CloudEnrollmentError != "" { | |
| b.WriteString("\n") | |
| b.WriteString(errorStyle.Render("Error: " + m.CloudEnrollmentError)) | |
| } | |
| b.WriteString(helpStyle.Render("\n j/k navigate • space toggle • esc/q back")) | |
| return b.String() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/view.go` around lines 242 - 319, Update viewCloudStatus and
viewCloudEnrollment to check CloudStatusLoading and CloudEnrollmentLoading
before rendering their empty or configured content, respectively. While either
corresponding flag is true, render the existing spinner/loading indicator
pattern used by the CloudConfigSaving view (for example, the “Pinging server...”
state) and retain the current content rendering once loading completes.
|
@coderabbitai re-review please — just added godoc comments to new exported functions (commit 6dde3ce on this branch's base, and b129225 on the enrollment branch). Want to confirm the docstring coverage warning is now resolved. Thanks! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin pull/610/head:pr610 2>&1 | tail -5
git checkout pr610 2>&1 | tail -5
git log --oneline -10
echo "---- check for commits ----"
git log --oneline | grep -E "6dde3ce|b129225" || echo "not found in log"
echo "---- store.go exported funcs ----"
ast-grep run --pattern 'func (s *Store) $NAME($$$) $$$' --lang go internal/store/store.go
echo "---- view context around DataDir and CountPendingSyncMutations ----"
rg -n -B3 "func \(s \*Store\) DataDir" internal/store/store.go
rg -n -B3 "func \(s \*Store\) CountPendingSyncMutations" internal/store/store.goLength of output: 150596 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp 2>/dev/null || true
cd - >/dev/null
echo "=== cloud.go exported symbols with preceding comment check ==="
awk '
/^func [A-Z]/ || /^func \([a-zA-Z]+ \*?[A-Za-z]+\) [A-Z]/ || /^type [A-Z]/ || /^var [A-Z]/ || /^const [A-Z]/ {
print NR": "$0
}
' internal/tui/cloud.go
echo "=== check comment line immediately before each ==="
grep -n "^func\|^type\|^var\|^const" internal/tui/cloud.go | grep -E '\b[A-Z]'Length of output: 822 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check exported func/type/const/var across all files in this PR (comment line before check) ==="
for f in internal/store/store.go internal/tui/cloud.go internal/tui/model.go internal/tui/update.go internal/tui/view.go; do
echo "--- $f ---"
awk '
/^func / || /^type / || /^var / || /^const /{
line=$0
# find identifier
print NR": "line
}' "$f" | grep -E '(func|type|var|const) (\([^)]*\) )?[A-Z]'
doneLength of output: 9590 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== model.go new types/consts context ==="
rg -n -B2 "ScreenCloudSettings|ScreenCloudConfig|ScreenCloudStatus|ScreenCloudEnrollment|CloudConfigLoaded|CloudStatusLoaded|CloudEnrollmentLoaded|CloudPingResult|cloudEnrollmentItem" internal/tui/model.go | head -150Length of output: 1235
The docstring coverage warning is resolved for this PR. Nice work — thanks for following up quickly! |
Summary
Adds the Project Enrollment screen: lists local projects with enrolled/not-enrolled badges, toggles enrollment with
spaceviaStore.EnrollProject/UnenrollProject.Spec
What's included
fca5aa5feat(tui): add project enrollment togglesDesign notes
Store.EnrollProjectandStore.UnenrollProject(the same methods the CLI uses).Test evidence
go test ./...passesgo vet ./...cleanMerge order
This is PR 4 of 6. Depends on PR 3 (merged first). After PR 3 merges, click "Update branch" on this PR.
Order:
Issue link
Part of #577
Label request
Please add
type:featureper CONTRIBUTING.md (contributor cannot add labels).Summary by CodeRabbit
New Features
Documentation