feat(tui): add CLI parity fields to cloud status view#612
Open
mgaldamez wants to merge 11 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds cloud sync configuration, authentication precedence, health checks, daemon probing, pending-mutation status, project enrollment, and four new TUI screens with navigation, persistence, and tests. ChangesCloud sync TUI
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TUI
participant loadCloudStatusCmd
participant Store
participant CloudServer
participant LocalDaemon
TUI->>loadCloudStatusCmd: Load cloud status
loadCloudStatusCmd->>Store: Read sync state and pending count
loadCloudStatusCmd->>CloudServer: Ping health endpoint
loadCloudStatusCmd->>LocalDaemon: Probe local health endpoint
loadCloudStatusCmd-->>TUI: Update cloud status
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
This PR extends the TUI Cloud Settings/Status experience to match CLI “cloud status” parity by adding new status fields (auth status framing, sync readiness, local daemon probe, sync diagnostics) and wiring the necessary model/update/load plumbing to populate them.
Changes:
- Adds Cloud Settings navigation screens (Configure server / View status / Enroll projects) and extends the Cloud Status view with CLI-parity fields and sync diagnostics.
- Introduces TUI-specific cloud config/ping + local daemon
/healthprobe utilities (mirroring CLI behavior where needed). - Adds store helpers to support status display (
Store.DataDir()andCountPendingSyncMutations) and updates docs/tests accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/view.go | Adds new cloud-related screens and renders new Cloud Status parity fields. |
| internal/tui/view_test.go | Adds rendering tests for the new cloud screens and parity fields. |
| internal/tui/update.go | Adds navigation and key handling for cloud screens; wires load/ping/enroll flows. |
| internal/tui/update_test.go | Adds extensive navigation/update tests for cloud settings/config/status/enrollment flows. |
| internal/tui/model.go | Adds new screen constants, model fields, and load commands for cloud screens. |
| internal/tui/model_test.go | Adds tests for new screen constants and cloud load command behavior. |
| internal/tui/cloud.go | New cloud helpers: config load/save, token source resolution, health ping, daemon probe. |
| internal/tui/cloud_test.go | Unit tests for cloud helpers (config, token resolution, ping, daemon probe). |
| internal/store/store.go | Adds DataDir() accessor and a new pending-mutation count helper scoped to enrolled/global mutations. |
| internal/store/store_test.go | Tests for DataDir() and CountPendingSyncMutations. |
| docs/engram-cloud/troubleshooting.md | Documents intentional cloud.json.token fallback with env var precedence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+226
to
+235
| if m.CloudConfigSaving { | ||
| b.WriteString("\n") | ||
| b.WriteString(m.SetupSpinner.View()) | ||
| b.WriteString(" Pinging server...") | ||
| b.WriteString("\n") | ||
| } else if m.CloudConfigPingStatus != "" { | ||
| b.WriteString("\n") | ||
| b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) | ||
| b.WriteString("\n") | ||
| } |
Comment on lines
+326
to
+335
| 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() | ||
| } |
| return "unreachable", err | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) |
Comment on lines
+839
to
+843
| case "esc": | ||
| m.CloudConfigInput.Blur() | ||
| m.Screen = ScreenCloudSettings | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| return m, nil |
Comment on lines
+248
to
+253
| 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() | ||
| } |
This was referenced Jul 14, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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.go`:
- Around line 3769-3786: The CountPendingSyncMutations eligibility behavior
needs verification. In internal/store/store.go:3769-3786, inspect
enqueueSyncMutationTx and backfillProjectSyncMutationsTx and ensure global
mutations persist project as an empty string rather than SQL NULL so the
existing filter matches them; in internal/store/store_test.go:1392-1436, add
coverage proving non-enrolled project mutations are excluded and
global/project-empty mutations are included.
In `@internal/tui/cloud.go`:
- Around line 74-94: Consolidate the shared token-precedence lookup used by
tokenSourceMessage and effectiveCloudToken into a single source-of-truth helper
that evaluates the environment token before the cloud config token and loads the
config only once per lookup. Update both callers to reuse that helper while
preserving TokenSourceEnv, TokenSourceFile, TokenSourceNone, and empty-token
behavior, including whitespace-only environment values.
- Around line 106-159: Update validateCloudServerURL to remove trailing slashes
from the validated base URL before pingCloudServerStatus appends
cloudHealthPath, while preserving the root URL and existing validation behavior.
Add a TestValidateCloudServerURL case verifying a URL such as https://host/ is
normalized so the health-check request does not contain a double slash.
- Around line 58-72: Restrict cloud configuration permissions in saveCloudConfig
by creating dataDir with owner-only access (0700) and writing cloud.json with
owner read/write access (0600). Ensure existing files are also secured, since
os.WriteFile’s mode does not change permissions when the file already exists.
In `@internal/tui/model_test.go`:
- Around line 322-428: Add tests for loadCloudStatusCmd covering both
daemonProbeNotRunning and the default/unreachable probe outcomes, in addition to
the existing HTTP-OK cases. Configure daemonProbeTransport to produce each
outcome, invoke loadCloudStatusCmd, and assert the resulting
cloudStatusLoadedMsg contains the expected localDaemon status and daemonHint
text, while preserving existing parity-field assertions where applicable.
In `@internal/tui/model.go`:
- Around line 443-459: Update loadCloudEnrollmentCmd to retrieve the
enrolled-project set with one store query, then determine each listed project's
enrolled status via local set membership instead of calling IsProjectEnrolled
inside the loop. Preserve the existing error propagation and cloudEnrollmentItem
construction behavior.
In `@internal/tui/update.go`:
- Around line 792-825: Update handleCloudEnrollmentKeys so the space-key branch
does not call EnrollProject or UnenrollProject synchronously; return a tea.Cmd
that performs the selected store write asynchronously and then reloads
enrollment data via loadCloudEnrollmentCmd. Preserve the existing item
validation, error propagation through CloudEnrollmentError, and
enrolled/unenrolled selection behavior.
- Around line 137-163: Add originating-screen context to cloudPingMsg and
populate it at each ping call site, including the status-view path in the
cloudStatusLoadedMsg handler. In the cloudPingMsg handling logic, only apply
config updates and screen transitions for config-originated pings, and only
update cloud-status fields for status-originated pings; ignore stale responses
received on unrelated screens.
- Around line 929-940: Update runCloudConfigPing to return a command that starts
m.SetupSpinner.Tick while retaining the pingCloudServer operation, and ensure
the update handling continues forwarding spinner.TickMsg whenever
CloudConfigSaving is true so the saving spinner animates until the ping
completes.
- Around line 710-776: Guard both cloud actions against a nil store before
dereferencing it: in handleCloudSettingsKeys, prevent the “Configure server”
path from calling m.store.DataDir(), and in runCloudConfigPing, add the same
m.store == nil handling before accessing DataDir. Return safely or disable the
actions while preserving normal behavior when a store is present.
In `@internal/tui/view.go`:
- Around line 242-253: Update viewCloudStatus at internal/tui/view.go:242-253 to
handle CloudStatusLoading before checking CloudStatusServerURL, rendering a
loading indicator and surfacing CloudStatusLastError when present. Update
viewCloudEnrollment at internal/tui/view.go:320-335 to handle
CloudEnrollmentLoading before the empty-items branch, rendering its loading
state instead of “No projects found.”
🪄 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: aa234f9b-cbd7-44ba-ab9d-55fbe60549e0
📒 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
Comment on lines
+3769
to
+3786
| // CountPendingSyncMutations returns the number of unacknowledged mutations for | ||
| // the given target that are currently eligible to sync (global/project-empty or | ||
| // enrolled projects). | ||
| func (s *Store) CountPendingSyncMutations(targetKey string) (int64, error) { | ||
| targetKey = normalizeSyncTargetKey(targetKey) | ||
| var count int64 | ||
| row := s.db.QueryRow(` | ||
| SELECT COUNT(*) | ||
| FROM sync_mutations sm | ||
| LEFT JOIN sync_enrolled_projects sep ON sm.project = sep.project | ||
| WHERE sm.target_key = ? AND sm.acked_at IS NULL | ||
| AND (sm.project = '' OR sep.project IS NOT NULL) | ||
| `, targetKey) | ||
| if err := row.Scan(&count); err != nil { | ||
| return 0, err | ||
| } | ||
| return count, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
CountPendingSyncMutations eligibility semantics for global and non-enrolled mutations are unverified. The query's (sm.project = '' OR sep.project IS NOT NULL) filter depends on global mutations being persisted with an actual empty string (not SQL NULL, which this codebase often uses via nullableString(...) for optional columns elsewhere), and the exclusion of non-enrolled projects' mutations is never asserted by a test — both are core, previously-changed behaviors per the PR summary.
internal/store/store.go#L3769-L3786: confirmenqueueSyncMutationTx/backfillProjectSyncMutationsTxstore global mutations withproject = ''(not NULL) sosm.project = ''actually matches them.internal/store/store_test.go#L1392-L1436: add a case creating a mutation for a non-enrolled project and assert it's excluded from the count (and ideally a global/project-empty case asserting inclusion).
📍 Affects 2 files
internal/store/store.go#L3769-L3786(this comment)internal/store/store_test.go#L1392-L1436
🤖 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.go` around lines 3769 - 3786, The
CountPendingSyncMutations eligibility behavior needs verification. In
internal/store/store.go:3769-3786, inspect enqueueSyncMutationTx and
backfillProjectSyncMutationsTx and ensure global mutations persist project as an
empty string rather than SQL NULL so the existing filter matches them; in
internal/store/store_test.go:1392-1436, add coverage proving non-enrolled
project mutations are excluded and global/project-empty mutations are included.
Comment on lines
+58
to
+72
| 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 config file/dir permissions are world-readable despite storing a token.
saveCloudConfig creates the data dir with 0o755 and writes cloud.json with 0o644. Per the updated docs, this file can carry a token value (preserved across saves), so other local users can read it. os.WriteFile's mode only takes effect on file creation, so this weak permission persists for the file's lifetime once created.
🔒 Proposed fix: 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)
}🤖 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 58 - 72, Restrict cloud configuration
permissions in saveCloudConfig by creating dataDir with owner-only access (0700)
and writing cloud.json with owner read/write access (0600). Ensure existing
files are also secured, since os.WriteFile’s mode does not change permissions
when the file already exists.
Comment on lines
+74
to
+94
| func tokenSourceMessage(dataDir string) string { | ||
| if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" { | ||
| return TokenSourceEnv | ||
| } | ||
| cc, err := loadCloudConfig(dataDir) | ||
| if err == nil && strings.TrimSpace(cc.Token) != "" { | ||
| return TokenSourceFile | ||
| } | ||
| return TokenSourceNone | ||
| } | ||
|
|
||
| func effectiveCloudToken(dataDir string) string { | ||
| if token := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")); token != "" { | ||
| return token | ||
| } | ||
| cc, err := loadCloudConfig(dataDir) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| return strings.TrimSpace(cc.Token) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate token-precedence logic between tokenSourceMessage and effectiveCloudToken.
Both functions re-implement the same env-var → file-token precedence and each calls loadCloudConfig independently. Given how subtle this precedence already is (see the dedicated empty-env-var regression test), keeping two copies risks them drifting out of sync.
♻️ Proposed fix: single source of truth
func tokenSourceMessage(dataDir string) string {
- if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" {
+ if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" {
return TokenSourceEnv
}
- cc, err := loadCloudConfig(dataDir)
- if err == nil && strings.TrimSpace(cc.Token) != "" {
+ if effectiveCloudToken(dataDir) != "" {
return TokenSourceFile
}
return TokenSourceNone
}📝 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.
Suggested change
| func tokenSourceMessage(dataDir string) string { | |
| if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" { | |
| return TokenSourceEnv | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err == nil && strings.TrimSpace(cc.Token) != "" { | |
| return TokenSourceFile | |
| } | |
| return TokenSourceNone | |
| } | |
| func effectiveCloudToken(dataDir string) string { | |
| if token := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")); token != "" { | |
| return token | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return "" | |
| } | |
| return strings.TrimSpace(cc.Token) | |
| } | |
| func tokenSourceMessage(dataDir string) string { | |
| if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" { | |
| return TokenSourceEnv | |
| } | |
| if effectiveCloudToken(dataDir) != "" { | |
| return TokenSourceFile | |
| } | |
| return TokenSourceNone | |
| } | |
| func effectiveCloudToken(dataDir string) string { | |
| if token := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")); token != "" { | |
| return token | |
| } | |
| cc, err := loadCloudConfig(dataDir) | |
| if err != nil { | |
| return "" | |
| } | |
| return strings.TrimSpace(cc.Token) | |
| } |
🤖 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 74 - 94, Consolidate the shared
token-precedence lookup used by tokenSourceMessage and effectiveCloudToken into
a single source-of-truth helper that evaluates the environment token before the
cloud config token and loads the config only once per lookup. Update both
callers to reuse that helper while preserving TokenSourceEnv, TokenSourceFile,
TokenSourceNone, and empty-token behavior, including whitespace-only environment
values.
Comment on lines
+106
to
+159
| func pingCloudServerStatus(serverURL, token string) (string, error) { | ||
| validatedURL, err := validateCloudServerURL(serverURL) | ||
| if err != nil { | ||
| return "unreachable", err | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | ||
| if err != nil { | ||
| return "unreachable", err | ||
| } | ||
| if token != "" { | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| } | ||
|
|
||
| client := &http.Client{Timeout: 3 * time.Second, Transport: pingCloudTransport} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return "unreachable", err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| switch { | ||
| case resp.StatusCode == http.StatusUnauthorized: | ||
| return "unauthorized", nil | ||
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | ||
| return "reachable", nil | ||
| default: | ||
| return "unreachable", fmt.Errorf("unexpected status %d", resp.StatusCode) | ||
| } | ||
| } | ||
|
|
||
| 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 server URL produces a double-slash health-check request.
validateCloudServerURL doesn't strip a trailing path slash, so https://host/ becomes https://host/ + /health = https://host//health at Line 112. This can make the ping fail for a perfectly valid, commonly-pasted URL. Failure degrades gracefully to "unreachable" rather than crashing, but it's an easy fix. TestValidateCloudServerURL also has no case covering this.
🐛 Proposed fix
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.
Suggested change
| func pingCloudServerStatus(serverURL, token string) (string, error) { | |
| validatedURL, err := validateCloudServerURL(serverURL) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| if token != "" { | |
| req.Header.Set("Authorization", "Bearer "+token) | |
| } | |
| client := &http.Client{Timeout: 3 * time.Second, Transport: pingCloudTransport} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| defer resp.Body.Close() | |
| switch { | |
| case resp.StatusCode == http.StatusUnauthorized: | |
| return "unauthorized", nil | |
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | |
| return "reachable", nil | |
| default: | |
| return "unreachable", fmt.Errorf("unexpected status %d", resp.StatusCode) | |
| } | |
| } | |
| 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 pingCloudServerStatus(serverURL, token string) (string, error) { | |
| validatedURL, err := validateCloudServerURL(serverURL) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| if token != "" { | |
| req.Header.Set("Authorization", "Bearer "+token) | |
| } | |
| client := &http.Client{Timeout: 3 * time.Second, Transport: pingCloudTransport} | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return "unreachable", err | |
| } | |
| defer resp.Body.Close() | |
| switch { | |
| case resp.StatusCode == http.StatusUnauthorized: | |
| return "unauthorized", nil | |
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | |
| return "reachable", nil | |
| default: | |
| return "unreachable", fmt.Errorf("unexpected status %d", resp.StatusCode) | |
| } | |
| } | |
| 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 106 - 159, Update validateCloudServerURL
to remove trailing slashes from the validated base URL before
pingCloudServerStatus appends cloudHealthPath, while preserving the root URL and
existing validation behavior. Add a TestValidateCloudServerURL case verifying a
URL such as https://host/ is normalized so the health-check request does not
contain a double slash.
Comment on lines
+322
to
+428
| func TestLoadCloudStatusCommandPopulatesParityFields(t *testing.T) { | ||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "") | ||
| t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") | ||
|
|
||
| origTransport := daemonProbeTransport | ||
| daemonProbeTransport = &fakePingTransport{statusCode: http.StatusOK} | ||
| defer func() { daemonProbeTransport = origTransport }() | ||
|
|
||
| fx := newTestFixture(t) | ||
| if err := os.WriteFile(filepath.Join(fx.store.DataDir(), "cloud.json"), []byte(`{"server_url":"https://cloud.example.com","token":"file-token"}`), 0o644); err != nil { | ||
| t.Fatalf("write cloud.json: %v", err) | ||
| } | ||
|
|
||
| msg := loadCloudStatusCmd(fx.store)() | ||
| loaded, ok := msg.(cloudStatusLoadedMsg) | ||
| if !ok { | ||
| t.Fatalf("message type = %T", msg) | ||
| } | ||
| if loaded.err != nil { | ||
| t.Fatalf("unexpected error: %v", loaded.err) | ||
| } | ||
| if loaded.target != "cloud" { | ||
| t.Fatalf("target = %q, want cloud", loaded.target) | ||
| } | ||
| if !strings.Contains(loaded.authStatus, "ready (token provided via runtime cloud config)") { | ||
| t.Fatalf("authStatus = %q", loaded.authStatus) | ||
| } | ||
| if !strings.Contains(loaded.syncReadiness, "ready for explicit --project sync") { | ||
| t.Fatalf("syncReadiness = %q", loaded.syncReadiness) | ||
| } | ||
| if !strings.Contains(loaded.localDaemon, "running on port") { | ||
| t.Fatalf("localDaemon = %q", loaded.localDaemon) | ||
| } | ||
| if loaded.authWarning != "" { | ||
| t.Fatalf("authWarning = %q, want empty", loaded.authWarning) | ||
| } | ||
| if loaded.authHint != "" { | ||
| t.Fatalf("authHint = %q, want empty", loaded.authHint) | ||
| } | ||
| if loaded.syncLifecycle == "" { | ||
| t.Fatal("syncLifecycle should not be empty") | ||
| } | ||
| } | ||
|
|
||
| func TestLoadCloudStatusCommandParityFieldsNoToken(t *testing.T) { | ||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "") | ||
| t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") | ||
|
|
||
| origTransport := daemonProbeTransport | ||
| daemonProbeTransport = &fakePingTransport{statusCode: http.StatusOK} | ||
| defer func() { daemonProbeTransport = origTransport }() | ||
|
|
||
| fx := newTestFixture(t) | ||
| if err := os.WriteFile(filepath.Join(fx.store.DataDir(), "cloud.json"), []byte(`{"server_url":"https://cloud.example.com"}`), 0o644); err != nil { | ||
| t.Fatalf("write cloud.json: %v", err) | ||
| } | ||
|
|
||
| msg := loadCloudStatusCmd(fx.store)() | ||
| loaded, ok := msg.(cloudStatusLoadedMsg) | ||
| if !ok { | ||
| t.Fatalf("message type = %T", msg) | ||
| } | ||
| if loaded.err != nil { | ||
| t.Fatalf("unexpected error: %v", loaded.err) | ||
| } | ||
| if !strings.Contains(loaded.authStatus, "token not configured") { | ||
| t.Fatalf("authStatus = %q", loaded.authStatus) | ||
| } | ||
| if loaded.authHint == "" { | ||
| t.Fatal("authHint should be set when no token") | ||
| } | ||
| if loaded.authWarning != "" { | ||
| t.Fatalf("authWarning = %q, want empty", loaded.authWarning) | ||
| } | ||
| } | ||
|
|
||
| func TestLoadCloudStatusCommandParityFieldsInsecure(t *testing.T) { | ||
| t.Setenv("ENGRAM_CLOUD_TOKEN", "") | ||
| t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "1") | ||
|
|
||
| origTransport := daemonProbeTransport | ||
| daemonProbeTransport = &fakePingTransport{statusCode: http.StatusOK} | ||
| defer func() { daemonProbeTransport = origTransport }() | ||
|
|
||
| fx := newTestFixture(t) | ||
| if err := os.WriteFile(filepath.Join(fx.store.DataDir(), "cloud.json"), []byte(`{"server_url":"https://cloud.example.com"}`), 0o644); err != nil { | ||
| t.Fatalf("write cloud.json: %v", err) | ||
| } | ||
|
|
||
| msg := loadCloudStatusCmd(fx.store)() | ||
| loaded, ok := msg.(cloudStatusLoadedMsg) | ||
| if !ok { | ||
| t.Fatalf("message type = %T", msg) | ||
| } | ||
| if loaded.err != nil { | ||
| t.Fatalf("unexpected error: %v", loaded.err) | ||
| } | ||
| if !strings.Contains(loaded.authStatus, "insecure local-dev mode") { | ||
| t.Fatalf("authStatus = %q", loaded.authStatus) | ||
| } | ||
| if loaded.authWarning == "" { | ||
| t.Fatal("authWarning should be set in insecure mode") | ||
| } | ||
| if loaded.authHint != "" { | ||
| t.Fatalf("authHint = %q, want empty in insecure mode", loaded.authHint) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Daemon "not running" / "unreachable" branches of loadCloudStatusCmd are untested here.
All three parity-field tests stub daemonProbeTransport with statusCode: http.StatusOK, only exercising the daemonProbeRunning case in model.go's switch statement. Neither the daemonProbeNotRunning branch (which sets daemonHint) nor the default/unreachable branch is exercised through loadCloudStatusCmd, so a regression in either message format or the daemonHint text wouldn't be caught at this layer.
As per path instructions, **/*_test.go: "Verify coverage of happy path, error paths, and edge cases."
🤖 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/model_test.go` around lines 322 - 428, Add tests for
loadCloudStatusCmd covering both daemonProbeNotRunning and the
default/unreachable probe outcomes, in addition to the existing HTTP-OK cases.
Configure daemonProbeTransport to produce each outcome, invoke
loadCloudStatusCmd, and assert the resulting cloudStatusLoadedMsg contains the
expected localDaemon status and daemonHint text, while preserving existing
parity-field assertions where applicable.
Source: Path instructions
Comment on lines
+137
to
+163
| case cloudStatusLoadedMsg: | ||
| m.CloudStatusLoading = false | ||
| if msg.err != nil { | ||
| m.CloudStatusLastError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| m.CloudStatusServerURL = msg.serverURL | ||
| m.CloudStatusTokenSource = msg.tokenSource | ||
| m.CloudStatusLastSync = msg.lastSync | ||
| m.CloudStatusPendingCount = msg.pendingCount | ||
| m.CloudStatusLastError = msg.lastError | ||
| m.CloudStatusTarget = msg.target | ||
| m.CloudStatusAuthStatus = msg.authStatus | ||
| m.CloudStatusAuthWarning = msg.authWarning | ||
| m.CloudStatusAuthHint = msg.authHint | ||
| m.CloudStatusSyncReadiness = msg.syncReadiness | ||
| m.CloudStatusLocalDaemon = msg.localDaemon | ||
| m.CloudStatusDaemonHint = msg.daemonHint | ||
| m.CloudStatusSyncLifecycle = msg.syncLifecycle | ||
| m.CloudStatusSyncReasonCode = msg.syncReasonCode | ||
| m.CloudStatusSyncReasonMessage = msg.syncReasonMessage | ||
| if m.CloudStatusServerURL != "" && m.store != nil { | ||
| // Reuse the existing ping helper (validated by T-03) to check health. | ||
| return m, pingCloudServer(m.CloudStatusServerURL, effectiveCloudToken(m.store.DataDir())) | ||
| } | ||
| return m, nil | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'Files:\n'
git ls-files 'internal/tui/*' | sed -n '1,120p'
printf '\nSearch symbols:\n'
rg -n "type cloudPingMsg|pingCloudServer|saveCloudConfig|ScreenCloudStatus|ScreenCloudConfig|CloudStatusLoaded" internal/tui -S
printf '\nOutline update.go:\n'
ast-grep outline internal/tui/update.go --view expanded | sed -n '1,220p'
printf '\nOutline cloud.go:\n'
ast-grep outline internal/tui/cloud.go --view expanded | sed -n '1,220p'Repository: Gentleman-Programming/engram
Length of output: 9967
🏁 Script executed:
set -euo pipefail
sed -n '120,220p' internal/tui/update.go
printf '\n---\n'
sed -n '900,980p' internal/tui/update.go
printf '\n---\n'
rg -n "type cloudPingMsg|cloudPingMsg\\{|pingCloudServer\\(|saveCloudConfig\\(" internal/tui -SRepository: Gentleman-Programming/engram
Length of output: 6638
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('internal/tui/update.go'), Path('internal/tui/cloud.go')]:
print(f'### {path}')
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if 'cloudPingMsg' in line or 'pingCloudServer' in line or 'saveCloudConfig' in line or 'ScreenCloudStatus' in line or 'ScreenCloudConfig' in line:
start = max(1, i-12)
end = min(len(text), i+18)
print(f'--- lines {start}-{end} ---')
for j in range(start, end+1):
print(f'{j:4}: {text[j-1]}')
PYRepository: Gentleman-Programming/engram
Length of output: 16286
🏁 Script executed:
set -euo pipefail
# Show the relevant sections with line numbers only, minimizing output.
nl -ba internal/tui/update.go | sed -n '130,230p'
printf '\n---\n'
nl -ba internal/tui/update.go | sed -n '920,980p'
printf '\n---\n'
nl -ba internal/tui/cloud.go | sed -n '1,240p'Repository: Gentleman-Programming/engram
Length of output: 206
Tag cloudPingMsg with its originating screen. cloudPingMsg has no source context, so any delayed response received while m.Screen != ScreenCloudStatus is treated as a config result. That can save CloudConfigInput to disk and switch screens even when the ping came from the status view, or update status fields from a config-side ping.
🤖 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 137 - 163, Add originating-screen
context to cloudPingMsg and populate it at each ping call site, including the
status-view path in the cloudStatusLoadedMsg handler. In the cloudPingMsg
handling logic, only apply config updates and screen transitions for
config-originated pings, and only update cloud-status fields for
status-originated pings; ignore stale responses received on unrelated screens.
Comment on lines
+710
to
+776
| func (m Model) handleCloudSettingsKeys(key string) (tea.Model, tea.Cmd) { | ||
| switch key { | ||
| case "up", "k": | ||
| if m.Cursor > 0 { | ||
| m.Cursor-- | ||
| } | ||
| case "down", "j": | ||
| if m.Cursor < len(cloudSettingsMenuItems)-1 { | ||
| m.Cursor++ | ||
| } | ||
| case "enter", " ": | ||
| switch m.Cursor { | ||
| case 0: // Configure server | ||
| m.PrevScreen = ScreenCloudSettings | ||
| m.Screen = ScreenCloudConfig | ||
| m.Cursor = 0 | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| m.CloudConfigError = "" | ||
| m.CloudConfigPingStatus = "" | ||
| m.CloudConfigSaving = false | ||
| m.CloudConfigTest = false | ||
| m.CloudConfigInput.SetValue("") | ||
| m.CloudConfigInput.Focus() | ||
| return m, loadCloudConfigCmd(m.store.DataDir()) | ||
| case 1: // View status | ||
| m.PrevScreen = ScreenCloudSettings | ||
| m.Screen = ScreenCloudStatus | ||
| m.Cursor = 0 | ||
| m.CloudStatusLoading = true | ||
| m.CloudStatusServerURL = "" | ||
| m.CloudStatusTokenSource = "" | ||
| m.CloudStatusHealth = "" | ||
| m.CloudStatusLastSync = "" | ||
| m.CloudStatusPendingCount = 0 | ||
| m.CloudStatusLastError = "" | ||
| m.CloudStatusTarget = "" | ||
| m.CloudStatusAuthStatus = "" | ||
| m.CloudStatusAuthWarning = "" | ||
| m.CloudStatusAuthHint = "" | ||
| m.CloudStatusSyncReadiness = "" | ||
| m.CloudStatusLocalDaemon = "" | ||
| m.CloudStatusDaemonHint = "" | ||
| m.CloudStatusSyncLifecycle = "" | ||
| m.CloudStatusSyncReasonCode = "" | ||
| m.CloudStatusSyncReasonMessage = "" | ||
| return m, loadCloudStatusCmd(m.store) | ||
| case 2: // Enroll projects | ||
|
|
||
| m.PrevScreen = ScreenCloudSettings | ||
| m.Screen = ScreenCloudEnrollment | ||
| m.Cursor = 0 | ||
| m.CloudEnrollmentLoading = true | ||
| m.CloudEnrollmentItems = nil | ||
| m.CloudEnrollmentError = "" | ||
| return m, loadCloudEnrollmentCmd(m.store) | ||
| case len(cloudSettingsMenuItems) - 1: // Back | ||
| m.Screen = ScreenDashboard | ||
| m.Cursor = 0 | ||
| return m, loadStats(m.store) | ||
| } | ||
| case "esc", "q": | ||
| m.Screen = ScreenDashboard | ||
| m.Cursor = 0 | ||
| return m, loadStats(m.store) | ||
| } | ||
| return m, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the surrounding code.
printf '\n## internal/tui/update.go outline\n'
ast-grep outline internal/tui/update.go --view expanded | sed -n '1,220p'
printf '\n## internal/tui/update.go around cloud settings and ping\n'
sed -n '650,980p' internal/tui/update.go | cat -n
printf '\n## search for store initialization / nil checks / interface type\n'
rg -n "type Model|store\s+.*|m\.store == nil|DataDir\(\)|effectiveCloudToken|loadCloudConfigCmd|loadCloudStatusCmd|loadCloudEnrollmentCmd" internal/tui -S
printf '\n## internal/tui/view.go relevant sections\n'
sed -n '220,360p' internal/tui/view.go | cat -n
printf '\n## any tests or constructors mentioning nil store\n'
rg -n "NewModel|Model\{|store:" internal/tui -SRepository: Gentleman-Programming/engram
Length of output: 23002
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## internal/tui/update.go outline\n'
ast-grep outline internal/tui/update.go --view expanded | sed -n '1,260p'
printf '\n## internal/tui/update.go around cloud settings and ping\n'
sed -n '680,960p' internal/tui/update.go | cat -n
printf '\n## search for store initialization / nil checks / interface type\n'
rg -n "type Model|m\.store == nil|DataDir\(\)|effectiveCloudToken|loadCloudConfigCmd|loadCloudStatusCmd|loadCloudEnrollmentCmd" internal/tui -S
printf '\n## internal/tui/view.go relevant sections\n'
sed -n '220,360p' internal/tui/view.go | cat -n
printf '\n## any constructors / model initialization mentioning store\n'
rg -n "NewModel|Model\\{|store:" internal/tui -SRepository: Gentleman-Programming/engram
Length of output: 21583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the model definition and constructors to determine whether `m.store` can be nil.
sed -n '1,220p' internal/tui/model.go | cat -n
printf '\n## references to store field and constructor assignments\n'
rg -n "store\s+\*|store\s+Store|store:" internal/tui -S
printf '\n## cloud status / enrollment nil-guard sites mentioned in the review comment\n'
sed -n '130,180p' internal/tui/update.go | cat -n
sed -n '790,825p' internal/tui/update.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 10696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## constructor / entrypoints\n'
sed -n '220,290p' internal/tui/model.go | cat -n
printf '\n## update tests around the nil-store case\n'
sed -n '560,620p' internal/tui/update_test.go | cat -n
printf '\n## any direct Model literals in production code\n'
rg -n "Model\{" -S --glob '!**/*_test.go' .Repository: Gentleman-Programming/engram
Length of output: 6074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the store API used by the TUI and whether DataDir tolerates a nil receiver.
rg -n "func \(.*DataDir\(" internal/store -S
sed -n '1,220p' internal/store/*.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 8530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect DataDir implementation and any nil-receiver handling in store.Store.
sed -n '470,520p' internal/store/store.go | cat -n
rg -n "func \(s \*Store\) DataDir|nil receiver|panic|if s == nil" internal/store -SRepository: Gentleman-Programming/engram
Length of output: 2930
Guard m.store before cloud actions
New(nil, ...) is allowed, and Store.DataDir() dereferences s.cfg, so both paths can panic when the model has no store:
internal/tui/update.go:733—m.store.DataDir()in “Configure server”internal/tui/update.go:939—m.store.DataDir()inrunCloudConfigPing
Return early or disable these actions when m.store == nil.
📍 Affects 1 file
internal/tui/update.go#L710-L776(this comment)internal/tui/update.go#L929-L940
🤖 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 710 - 776, Guard both cloud actions
against a nil store before dereferencing it: in handleCloudSettingsKeys, prevent
the “Configure server” path from calling m.store.DataDir(), and in
runCloudConfigPing, add the same m.store == nil handling before accessing
DataDir. Return safely or disable the actions while preserving normal behavior
when a store is present.
Comment on lines
+792
to
+825
| func (m Model) handleCloudEnrollmentKeys(key string) (tea.Model, tea.Cmd) { | ||
| count := len(m.CloudEnrollmentItems) | ||
|
|
||
| switch key { | ||
| case "up", "k": | ||
| if m.Cursor > 0 { | ||
| m.Cursor-- | ||
| } | ||
| case "down", "j": | ||
| if m.Cursor < count-1 { | ||
| m.Cursor++ | ||
| } | ||
| case " ": | ||
| if count > 0 && m.Cursor < count && m.store != nil { | ||
| item := m.CloudEnrollmentItems[m.Cursor] | ||
| m.CloudEnrollmentError = "" | ||
| if item.enrolled { | ||
| if err := m.store.UnenrollProject(item.project); err != nil { | ||
| m.CloudEnrollmentError = err.Error() | ||
| } | ||
| } else { | ||
| if err := m.store.EnrollProject(item.project); err != nil { | ||
| m.CloudEnrollmentError = err.Error() | ||
| } | ||
| } | ||
| return m, loadCloudEnrollmentCmd(m.store) | ||
| } | ||
| case "esc", "q": | ||
| m.Screen = ScreenCloudSettings | ||
| m.Cursor = 2 // keep Enroll projects selected for smoother back navigation | ||
| return m, nil | ||
| } | ||
| return m, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Enroll/unenroll DB write runs synchronously inside Update(), blocking the Bubble Tea loop.
m.store.EnrollProject/UnenrollProject (lines 809/813) execute directly in the key handler rather than inside the returned tea.Cmd. Every other data operation in this PR (loadCloudEnrollmentCmd, loadCloudStatusCmd, pingCloudServer) is dispatched asynchronously via tea.Cmd; this one isn't. Since Bubble Tea's Update/render loop is effectively single-threaded, a slow write (a full transaction plus backfillProjectSyncMutationsTx, with busy_timeout=5000 on the underlying store) freezes the entire UI — unresponsive to any key, including quit — until it returns.
🔒 Proposed fix: defer the write into the returned command
case " ":
if count > 0 && m.Cursor < count && m.store != nil {
item := m.CloudEnrollmentItems[m.Cursor]
m.CloudEnrollmentError = ""
- if item.enrolled {
- if err := m.store.UnenrollProject(item.project); err != nil {
- m.CloudEnrollmentError = err.Error()
- }
- } else {
- if err := m.store.EnrollProject(item.project); err != nil {
- m.CloudEnrollmentError = err.Error()
- }
- }
- return m, loadCloudEnrollmentCmd(m.store)
+ s := m.store
+ return m, func() tea.Msg {
+ var err error
+ if item.enrolled {
+ err = s.UnenrollProject(item.project)
+ } else {
+ err = s.EnrollProject(item.project)
+ }
+ if err != nil {
+ return cloudEnrollmentLoadedMsg{err: err}
+ }
+ return loadCloudEnrollmentCmd(s)()
+ }
}📝 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.
Suggested change
| func (m Model) handleCloudEnrollmentKeys(key string) (tea.Model, tea.Cmd) { | |
| count := len(m.CloudEnrollmentItems) | |
| switch key { | |
| case "up", "k": | |
| if m.Cursor > 0 { | |
| m.Cursor-- | |
| } | |
| case "down", "j": | |
| if m.Cursor < count-1 { | |
| m.Cursor++ | |
| } | |
| case " ": | |
| if count > 0 && m.Cursor < count && m.store != nil { | |
| item := m.CloudEnrollmentItems[m.Cursor] | |
| m.CloudEnrollmentError = "" | |
| if item.enrolled { | |
| if err := m.store.UnenrollProject(item.project); err != nil { | |
| m.CloudEnrollmentError = err.Error() | |
| } | |
| } else { | |
| if err := m.store.EnrollProject(item.project); err != nil { | |
| m.CloudEnrollmentError = err.Error() | |
| } | |
| } | |
| return m, loadCloudEnrollmentCmd(m.store) | |
| } | |
| case "esc", "q": | |
| m.Screen = ScreenCloudSettings | |
| m.Cursor = 2 // keep Enroll projects selected for smoother back navigation | |
| return m, nil | |
| } | |
| return m, nil | |
| } | |
| func (m Model) handleCloudEnrollmentKeys(key string) (tea.Model, tea.Cmd) { | |
| count := len(m.CloudEnrollmentItems) | |
| switch key { | |
| case "up", "k": | |
| if m.Cursor > 0 { | |
| m.Cursor-- | |
| } | |
| case "down", "j": | |
| if m.Cursor < count-1 { | |
| m.Cursor++ | |
| } | |
| case " ": | |
| if count > 0 && m.Cursor < count && m.store != nil { | |
| item := m.CloudEnrollmentItems[m.Cursor] | |
| m.CloudEnrollmentError = "" | |
| s := m.store | |
| return m, func() tea.Msg { | |
| var err error | |
| if item.enrolled { | |
| err = s.UnenrollProject(item.project) | |
| } else { | |
| err = s.EnrollProject(item.project) | |
| } | |
| if err != nil { | |
| return cloudEnrollmentLoadedMsg{err: err} | |
| } | |
| return loadCloudEnrollmentCmd(s)() | |
| } | |
| } | |
| case "esc", "q": | |
| m.Screen = ScreenCloudSettings | |
| m.Cursor = 2 // keep Enroll projects selected for smoother back navigation | |
| return m, nil | |
| } | |
| return m, 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/update.go` around lines 792 - 825, Update
handleCloudEnrollmentKeys so the space-key branch does not call EnrollProject or
UnenrollProject synchronously; return a tea.Cmd that performs the selected store
write asynchronously and then reloads enrollment data via
loadCloudEnrollmentCmd. Preserve the existing item validation, error propagation
through CloudEnrollmentError, and enrolled/unenrolled selection behavior.
Comment on lines
+929
to
+940
| func (m Model) runCloudConfigPing(testOnly bool) (tea.Model, tea.Cmd) { | ||
| 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())) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files internal/tui/update.go internal/tui/view.go internal/tui | sed -n '1,120p'
printf '\n== update.go outline ==\n'
ast-grep outline internal/tui/update.go --view expanded | sed -n '1,220p'
printf '\n== view.go outline ==\n'
ast-grep outline internal/tui/view.go --view expanded | sed -n '1,220p'
printf '\n== spinner usage ==\n'
rg -n "SetupSpinner|spinner\.TickMsg|spinner\.Tick|CloudConfigSaving|SetupInstalling|runCloudConfigPing|pingCloudServer" internal/tui/update.go internal/tui/view.goRepository: Gentleman-Programming/engram
Length of output: 5315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '900,970p' internal/tui/update.go
printf '\n---\n'
sed -n '210,250p' internal/tui/update.go
printf '\n---\n'
sed -n '1,220p' internal/tui/view.go | sed -n '1,220p'Repository: Gentleman-Programming/engram
Length of output: 9068
Cloud config spinner needs a tick command runCloudConfigPing returns only pingCloudServer(...), so the CloudConfigSaving state renders a frozen m.SetupSpinner.View(). Return m.SetupSpinner.Tick there and keep forwarding spinner.TickMsg while CloudConfigSaving is true.
🤖 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 929 - 940, Update runCloudConfigPing to
return a command that starts m.SetupSpinner.Tick while retaining the
pingCloudServer operation, and ensure the update handling continues forwarding
spinner.TickMsg whenever CloudConfigSaving is true so the saving spinner
animates until the ping completes.
Comment on lines
+242
to
+253
| 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() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
CloudStatusLoading/CloudEnrollmentLoading are tracked in the model/update flow but never rendered. Both screens fall back to their "empty" messaging ("not configured" / "No projects found.") during the async load window instead of showing a loading indicator.
internal/tui/view.go#L242-L253: add anif m.CloudStatusLoading { ... }early branch inviewCloudStatusbefore the "not configured" check (and surfaceCloudStatusLastErrorthere too, since it's currently swallowed — see per-site diff).internal/tui/view.go#L320-L335: add anif m.CloudEnrollmentLoading { ... }early branch inviewCloudEnrollmentbefore the empty-items check.
📍 Affects 1 file
internal/tui/view.go#L242-L253(this comment)internal/tui/view.go#L320-L335
🤖 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 - 253, Update viewCloudStatus at
internal/tui/view.go:242-253 to handle CloudStatusLoading before checking
CloudStatusServerURL, rendering a loading indicator and surfacing
CloudStatusLastError when present. Update viewCloudEnrollment at
internal/tui/view.go:320-335 to handle CloudEnrollmentLoading before the
empty-items branch, rendering its loading state instead of “No projects found.”
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
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
Adds CLI parity fields to the Cloud Status screen:
Cloud status: configured (target=cloud)header,Auth status(with user-friendly framing: ready / not configured / insecure mode),Sync readiness,Local daemon(HTTP probe to /health on port 7437), andSync diagnostic(lifecycle, reason_code, reason_message). Existing fields (Server URL, Health, Token source, Last sync, Pending, Last error) are unchanged.Spec
What's included
8ff8ed4feat(tui): add CLI parity fields to cloud status viewDesign notes
cmd/engram/cloud_daemon_probe.go:defaultCloudDaemonProbe(re-implemented ininternal/tui/cloud.gobecause the TUI cannot importcmd/engram).Test evidence
go test ./...passes (with and withoutENGRAM_CLOUD_TOKENset)go vet ./...cleaninternal/tuiMerge order
This is PR 6 of 6 (final). Depends on PR 5 (merged first). After PR #5 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