feat(tui): add cloud status view#609
Open
mgaldamez wants to merge 7 commits into
Open
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds cloud sync settings to the TUI, including persisted server configuration, token precedence, connectivity checks, sync status metrics, navigation, save flows, and screen rendering. The store exposes data-directory and pending-mutation helpers, with tests and troubleshooting documentation updated. ChangesCloud sync settings
Estimated code review effort: 4 (Complex) | ~60 minutes 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 a new Cloud Status read-only screen to the Bubbletea TUI, alongside Cloud Settings navigation and Cloud Config UI helpers, and introduces a store accessor to surface pending sync mutations for status reporting.
Changes:
- Add Cloud Settings submenu and new Cloud Config / Cloud Status screens to the TUI (views + navigation + key handling).
- Introduce TUI cloud helpers for loading/saving
cloud.jsonserver URL, resolving token source, and pinging/health. - Add
Store.CountPendingSyncMutations(and aDataDir()accessor) to support status reporting, with accompanying tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/view.go | Adds new Cloud screens’ rendering and shared menu rendering helper. |
| internal/tui/view_test.go | Adds view coverage for Cloud Config/Status rendering branches. |
| internal/tui/update.go | Adds screen routing, key handling, and async message handling for Cloud screens. |
| internal/tui/update_test.go | Adds navigation + save/ping behavior tests for Cloud screens. |
| internal/tui/model.go | Extends model with Cloud fields/messages and adds async load commands. |
| internal/tui/model_test.go | Adds tests for new screen constants and cloud load commands. |
| internal/tui/cloud.go | New TUI cloud config + /health ping helpers. |
| internal/tui/cloud_test.go | Unit tests for cloud config persistence, token sourcing, and ping behavior. |
| internal/store/store.go | Adds DataDir() and CountPendingSyncMutations() used by Cloud Status. |
| internal/store/store_test.go | Adds tests for DataDir() and CountPendingSyncMutations(). |
| docs/engram-cloud/troubleshooting.md | Updates docs to reflect intentional cloud.json.token fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+224
to
234
| 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
+246
to
+251
| 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() | ||
| } |
Comment on lines
+107
to
+110
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | ||
| if err != nil { | ||
| return "unreachable", err | ||
| } |
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: 9
🤖 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: The TestCountPendingSyncMutations test only covers
basic pending counts; extend it with deterministic subcases for target-key
normalization, excluding mutations from un-enrolled projects, including global
and empty-project mutations, excluding acknowledged mutations, and returning an
error after the database is closed. Keep the existing happy-path assertions and
use the store APIs and mutation setup helpers already available in the test
package.
In `@internal/tui/cloud.go`:
- Around line 53-67: Update saveCloudConfig and its data-directory setup so
cloud.json and the containing directory are restricted to the owner, using
non-world-readable permissions when creating or writing them. Preserve the
existing config serialization and error handling while ensuring existing files
are also secured rather than relying only on creation modes.
- Around line 94-130: Update pingCloudServerStatus to normalize the validated
URL by removing its trailing slash before appending cloudHealthPath, while
preserving existing URL validation behavior. Extend TestValidateCloudServerURL
with a trailing-slash input and assert the normalized URL result.
In `@internal/tui/update.go`:
- Around line 700-723: Guard the “Configure server” and “View status” entry
points, plus the ping-save path around the corresponding store usage, so nil
m.store cannot reach DataDir() or other store methods. Follow the existing
nil-guard pattern near line 148 and preserve the current behavior when a store
is available.
- Around line 698-728: Handle the “Enroll projects” selection in the cloud
settings menu within the enter/space switch on m.Cursor: either implement its
navigation/action flow or remove the corresponding menu item from
cloudSettingsMenuItems until that flow exists. Ensure the remaining cursor cases
and Back behavior stay aligned with the menu entries.
- Around line 137-193: Update cloudPingMsg and its producers to carry the ping
origin and the validated URL used for the request. In the cloudPingMsg handler,
route results using the message origin rather than m.Screen, and use the carried
URL for config persistence instead of re-reading CloudConfigInput.Value();
preserve the existing status-screen and config-save outcomes.
In `@internal/tui/view_test.go`:
- Around line 78-109: Extend TestViewCloudConfigRendersTokenSource to assert the
token-override hint is present for TokenSourceNone and TokenSourceFile, and
absent for TokenSourceEnv. Reuse the existing m.View() checks for each token
source while verifying the conditional behavior implemented by viewCloudConfig.
- Around line 111-143: Replace the broad strings.Contains(out, "2") check in
TestViewCloudStatusRendersConfiguredState with an assertion that specifically
verifies the rendered Pending row contains the configured count 2. Anchor the
check to the expected field label and value so the assertion cannot pass because
of digits in CloudStatusLastSync.
In `@internal/tui/view.go`:
- Around line 212-221: Update the button-rendering loop in the cloud
configuration view to pair each label with its corresponding cloudConfigFocus*
constant explicitly instead of deriving focus with i + 1. Compare
CloudConfigFocus against the paired focus value while preserving the existing
selected and unselected rendering behavior.
🪄 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: f0e933d6-cb08-4ec9-9f32-42c7695edeb4
📒 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
+1392
to
+1436
| 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 | 🟠 Major | ⚡ Quick win
Add error-path and eligibility edge-case coverage before merging.
This test covers only the happy path. Add deterministic cases for target-key normalization, un-enrolled projects being excluded, global/project-empty mutations being included, acknowledged mutations being excluded, and query failure handling (for example, after closing the database). Behavior changes without these tests should be blocked.
As per path instructions, *_test.go files must verify happy paths, error paths, and edge cases, and behavior changes without tests are blocked.
🤖 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, The
TestCountPendingSyncMutations test only covers basic pending counts; extend it
with deterministic subcases for target-key normalization, excluding mutations
from un-enrolled projects, including global and empty-project mutations,
excluding acknowledged mutations, and returning an error after the database is
closed. Keep the existing happy-path assertions and use the store APIs and
mutation setup helpers already available in the test package.
Source: Path instructions
Comment on lines
+53
to
+67
| 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 written world-readable even though it may carry a token.
saveCloudConfig re-serializes the loaded config (including any existing Token, line 30) and writes it with 0o644 into a directory created with 0o755. On any shared/multi-user host this makes a file that can contain a live ENGRAM_CLOUD auth token readable by every local user.
🔒 Restrict permissions on the cloud config file/dir
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.
Suggested change
| 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 and its
data-directory setup so cloud.json and the containing directory are restricted
to the owner, using non-world-readable permissions when creating or writing
them. Preserve the existing config serialization and error handling while
ensuring existing files are also secured rather than relying only on creation
modes.
Comment on lines
+94
to
+130
| func pingCloudServer(serverURL, token string) tea.Cmd { | ||
| return func() tea.Msg { | ||
| status, err := pingCloudServerStatus(serverURL, token) | ||
| return cloudPingMsg{status: status, err: err} | ||
| } | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== internal/tui/cloud.go outline ==\n'
ast-grep outline internal/tui/cloud.go --view expanded || true
printf '\n== internal/tui/cloud.go relevant lines ==\n'
sed -n '1,220p' internal/tui/cloud.go | cat -n
printf '\n== test files mentioning validateCloudServerURL ==\n'
rg -n "validateCloudServerURL|TestValidateCloudServerURL|cloudHealthPath|pingCloudServerStatus" internal/tui -g '*_test.go' -g '*.go' || trueRepository: Gentleman-Programming/engram
Length of output: 7170
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('internal/tui/cloud.go')
print(p.exists())
PYRepository: Gentleman-Programming/engram
Length of output: 171
🏁 Script executed:
set -euo pipefail
sed -n '1,220p' internal/tui/cloud.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 5047
🏁 Script executed:
set -euo pipefail
grep -n "func validateCloudServerURL\|cloudHealthPath\|TestValidateCloudServerURL" -n internal/tui/cloud.go internal/tui/*_test.goRepository: Gentleman-Programming/engram
Length of output: 493
🏁 Script executed:
set -euo pipefail
sed -n '180,250p' internal/tui/cloud_test.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 1857
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from urllib.parse import urlparse
raw = 'https://cloud.example.com/'
parsed = urlparse(raw)
print(parsed.geturl())
print(parsed.path + '/health')
PYRepository: Gentleman-Programming/engram
Length of output: 202
Normalize the server URL before appending /health. A trailing / is preserved by validateCloudServerURL, so validatedURL + cloudHealthPath can become //health and make a valid server look unreachable. Add a trailing-slash case to TestValidateCloudServerURL.
🤖 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 94 - 130, Update pingCloudServerStatus to
normalize the validated URL by removing its trailing slash before appending
cloudHealthPath, while preserving existing URL validation behavior. Extend
TestValidateCloudServerURL with a trailing-slash input and assert the normalized
URL result.
Source: Path instructions
Comment on lines
+137
to
+193
| 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 | ||
| 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 | ||
|
|
||
| case cloudPingMsg: | ||
| // Status screen and config screen share the same ping result type, but | ||
| // update different model fields. | ||
| if m.Screen == ScreenCloudStatus { | ||
| m.CloudStatusHealth = msg.status | ||
| if msg.err != nil { | ||
| m.CloudStatusHealth = "unreachable" | ||
| m.CloudStatusLastError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| return m, nil | ||
| } | ||
| m.CloudConfigSaving = false | ||
| m.CloudConfigPingStatus = msg.status | ||
| if msg.err != nil { | ||
| m.CloudConfigError = msg.err.Error() | ||
| return m, nil | ||
| } | ||
| if m.CloudConfigTest { | ||
| // Test-only ping: just show the status. | ||
| return m, nil | ||
| } | ||
| if msg.status == "reachable" || msg.status == "unauthorized" { | ||
| validatedURL, err := validateCloudServerURL(m.CloudConfigInput.Value()) | ||
| if err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| if err := saveCloudConfig(m.store.DataDir(), validatedURL); err != nil { | ||
| m.CloudConfigError = err.Error() | ||
| return m, nil | ||
| } | ||
| m.Screen = ScreenCloudSettings | ||
| m.Cursor = 0 | ||
| m.CloudConfigFocus = cloudConfigFocusInput | ||
| return m, nil | ||
| } | ||
| m.CloudConfigError = "server is unreachable" | ||
| return m, nil | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant symbols and file sizes first.
git ls-files internal/tui | sed -n '1,120p'
printf '\n--- outline: update.go ---\n'
ast-grep outline internal/tui/update.go --view expanded || true
printf '\n--- outline: model.go ---\n'
ast-grep outline internal/tui/model.go --view expanded || true
printf '\n--- outline: cloud.go ---\n'
ast-grep outline internal/tui/cloud.go --view expanded || true
printf '\n--- grep cloudPingMsg / pingCloudServer / runCloudConfigPing ---\n'
rg -n "cloudPingMsg|pingCloudServer|runCloudConfigPing|CloudStatusLoading|CloudConfigTest|ScreenCloudStatus|ScreenCloudSettings" internal/tuiRepository: Gentleman-Programming/engram
Length of output: 13394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- internal/tui/model.go (cloudPingMsg) ---\n'
sed -n '96,110p' internal/tui/model.go | cat -n
printf '\n--- internal/tui/cloud.go (pingCloudServer) ---\n'
sed -n '88,104p' internal/tui/cloud.go | cat -n
printf '\n--- internal/tui/update.go (cloud handlers) ---\n'
sed -n '136,190p' internal/tui/update.go | cat -n
printf '\n--- internal/tui/update.go (runCloudConfigPing) ---\n'
sed -n '851,864p' internal/tui/update.go | cat -n
printf '\n--- internal/tui/update_test.go (cloud ping tests) ---\n'
sed -n '410,440p' internal/tui/update_test.go | cat -n
printf '\n--- internal/tui/update_test.go (cloud config ping tests) ---\n'
sed -n '560,640p' internal/tui/update_test.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 8602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- internal/tui/update.go (cloud config key handling) ---\n'
sed -n '688,865p' internal/tui/update.go | cat -n
printf '\n--- internal/tui/update.go (cloud status key handling) ---\n'
sed -n '739,850p' internal/tui/update.go | cat -n
printf '\n--- internal/tui/update_test.go (cloud config interaction tests) ---\n'
sed -n '240,420p' internal/tui/update_test.go | cat -nRepository: Gentleman-Programming/engram
Length of output: 17025
Tag cloudPingMsg with its origin and validated URL.
Update routes by m.Screen, so an in-flight status ping can be mistaken for a config save after navigation. The save path also re-reads CloudConfigInput.Value() at response time. Carry the ping source and exact URL in the message, and use those in the handler instead.
🤖 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 - 193, Update cloudPingMsg and its
producers to carry the ping origin and the validated URL used for the request.
In the cloudPingMsg handler, route results using the message origin rather than
m.Screen, and use the carried URL for config persistence instead of re-reading
CloudConfigInput.Value(); preserve the existing status-screen and config-save
outcomes.
Comment on lines
+698
to
+728
| 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 = "" | ||
| return m, loadCloudStatusCmd(m.store) | ||
| case len(cloudSettingsMenuItems) - 1: // Back | ||
| m.Screen = ScreenDashboard | ||
| m.Cursor = 0 | ||
| return m, loadStats(m.store) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant TUI file and inspect the menu definition plus the handler.
ast-grep outline internal/tui/update.go --view expanded || true
printf '\n--- menu definition ---\n'
rg -n 'cloudSettingsMenuItems|Enroll projects|case "enter"|case " "' internal/tui/update.go
printf '\n--- nearby lines around menu definition ---\n'
sed -n '240,310p' internal/tui/update.go
printf '\n--- nearby lines around handler ---\n'
sed -n '680,740p' internal/tui/update.goRepository: Gentleman-Programming/engram
Length of output: 5476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether "Enroll projects" is handled anywhere else in the TUI codebase.
rg -n 'Enroll projects|cloudSettingsMenuItems|EnrollProjects|enroll projects|coming soon' internal/tuiRepository: Gentleman-Programming/engram
Length of output: 515
Handle "Enroll projects" in the cloud settings menu. It currently falls through without any action; add a branch for it or remove the item until the flow exists.
🤖 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 698 - 728, Handle the “Enroll projects”
selection in the cloud settings menu within the enter/space switch on m.Cursor:
either implement its navigation/action flow or remove the corresponding menu
item from cloudSettingsMenuItems until that flow exists. Ensure the remaining
cursor cases and Back behavior stay aligned with the menu entries.
Comment on lines
+700
to
+723
| 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 = "" | ||
| return m, loadCloudStatusCmd(m.store) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Inconsistent nil-guard on m.store — NPE risk on "Configure server" / "View status" / ping-save.
Line 148 explicitly guards m.store != nil before calling m.store.DataDir(), showing the author is aware Model can exist with a nil store (many tests build it via New(nil, "")). The same m.store.DataDir() / m.store usage here (lines 711, 723, 861) has no such guard and would panic if ever reached with a nil store. No current test exercises "Configure server"/"View status" selection with New(nil, ""), so this path is untested.
🛡️ Guard the two entry points
case 0: // Configure server
+ if m.store == nil {
+ return m, nil
+ }
m.PrevScreen = ScreenCloudSettings
...
return m, loadCloudConfigCmd(m.store.DataDir())
case 1: // View status
+ if m.store == nil {
+ return m, nil
+ }
m.PrevScreen = ScreenCloudSettings
...
return m, loadCloudStatusCmd(m.store) func (m Model) runCloudConfigPing(testOnly bool) (tea.Model, tea.Cmd) {
m.CloudConfigError = ""
m.CloudConfigPingStatus = ""
+ if m.store == nil {
+ m.CloudConfigError = "no store configured"
+ return m, nil
+ }
validatedURL, err := validateCloudServerURL(m.CloudConfigInput.Value())Also applies to: 851-862
🤖 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 700 - 723, Guard the “Configure server”
and “View status” entry points, plus the ping-save path around the corresponding
store usage, so nil m.store cannot reach DataDir() or other store methods.
Follow the existing nil-guard pattern near line 148 and preserve the current
behavior when a store is available.
Comment on lines
+78
to
+109
| func TestViewCloudConfigRendersTokenSource(t *testing.T) { | ||
| m := New(nil, "") | ||
| m.Screen = ScreenCloudConfig | ||
| m.CloudConfigServerURL = "https://cloud.example.com" | ||
| m.CloudConfigTokenSource = TokenSourceNone | ||
|
|
||
| out := m.View() | ||
| if !strings.Contains(out, "Configure cloud server") { | ||
| t.Fatal("view should render screen title") | ||
| } | ||
| if !strings.Contains(out, "Server URL") { | ||
| t.Fatal("view should render server URL label") | ||
| } | ||
| if !strings.Contains(out, "Token:") { | ||
| t.Fatal("view should render token label") | ||
| } | ||
| if !strings.Contains(out, TokenSourceNone) { | ||
| t.Fatalf("view should render token source %q", TokenSourceNone) | ||
| } | ||
|
|
||
| m.CloudConfigTokenSource = TokenSourceEnv | ||
| out = m.View() | ||
| if !strings.Contains(out, TokenSourceEnv) { | ||
| t.Fatalf("view should render env token source %q", TokenSourceEnv) | ||
| } | ||
|
|
||
| m.CloudConfigTokenSource = TokenSourceFile | ||
| out = m.View() | ||
| if !strings.Contains(out, TokenSourceFile) { | ||
| t.Fatalf("view should render file token source %q", TokenSourceFile) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing coverage for the token-override hint branch.
viewCloudConfig conditionally renders "Set ENGRAM_CLOUD_TOKEN to override cloud.json.token" when CloudConfigTokenSource != TokenSourceEnv (view.go lines 206-209), but no test here asserts the hint's presence for TokenSourceNone/TokenSourceFile or its absence for TokenSourceEnv. As per path instructions, **/*_test.go requires verifying edge-case coverage for behavior branches.
🤖 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_test.go` around lines 78 - 109, Extend
TestViewCloudConfigRendersTokenSource to assert the token-override hint is
present for TokenSourceNone and TokenSourceFile, and absent for TokenSourceEnv.
Reuse the existing m.View() checks for each token source while verifying the
conditional behavior implemented by viewCloudConfig.
Source: Path instructions
Comment on lines
+111
to
+143
| func TestViewCloudStatusRendersConfiguredState(t *testing.T) { | ||
| m := New(nil, "") | ||
| m.Screen = ScreenCloudStatus | ||
| m.CloudStatusServerURL = "https://cloud.example.com" | ||
| m.CloudStatusTokenSource = TokenSourceEnv | ||
| m.CloudStatusHealth = "reachable" | ||
| m.CloudStatusLastSync = "2026-07-07 12:00:00" | ||
| m.CloudStatusPendingCount = 2 | ||
| m.CloudStatusLastError = "last fail" | ||
|
|
||
| out := m.View() | ||
| if !strings.Contains(out, "Cloud status") { | ||
| t.Fatal("view should render screen title") | ||
| } | ||
| if !strings.Contains(out, "https://cloud.example.com") { | ||
| t.Fatal("view should render server URL") | ||
| } | ||
| if !strings.Contains(out, "reachable") { | ||
| t.Fatal("view should render health") | ||
| } | ||
| if !strings.Contains(out, TokenSourceEnv) { | ||
| t.Fatal("view should render token source") | ||
| } | ||
| if !strings.Contains(out, "2026-07-07 12:00:00") { | ||
| t.Fatal("view should render last sync") | ||
| } | ||
| if !strings.Contains(out, "2") { | ||
| t.Fatal("view should render pending count") | ||
| } | ||
| if !strings.Contains(out, "last fail") { | ||
| t.Fatal("view should render last error") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pending-count assertion is a false positive.
m.CloudStatusLastSync = "2026-07-07 12:00:00" already contains multiple "2" characters, so strings.Contains(out, "2") at Line 137 passes regardless of whether the Pending: row actually renders 2. This assertion doesn't verify pending-count rendering at all. As per path instructions, **/*_test.go guidance requires verifying coverage of happy/edge-case behavior with meaningful checks.
🧪 Proposed fix for a meaningful assertion
- if !strings.Contains(out, "2") {
+ if !strings.Contains(out, "Pending: 2") {
t.Fatal("view should render pending 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.
Suggested change
| func TestViewCloudStatusRendersConfiguredState(t *testing.T) { | |
| m := New(nil, "") | |
| m.Screen = ScreenCloudStatus | |
| m.CloudStatusServerURL = "https://cloud.example.com" | |
| m.CloudStatusTokenSource = TokenSourceEnv | |
| m.CloudStatusHealth = "reachable" | |
| m.CloudStatusLastSync = "2026-07-07 12:00:00" | |
| m.CloudStatusPendingCount = 2 | |
| m.CloudStatusLastError = "last fail" | |
| out := m.View() | |
| if !strings.Contains(out, "Cloud status") { | |
| t.Fatal("view should render screen title") | |
| } | |
| if !strings.Contains(out, "https://cloud.example.com") { | |
| t.Fatal("view should render server URL") | |
| } | |
| if !strings.Contains(out, "reachable") { | |
| t.Fatal("view should render health") | |
| } | |
| if !strings.Contains(out, TokenSourceEnv) { | |
| t.Fatal("view should render token source") | |
| } | |
| if !strings.Contains(out, "2026-07-07 12:00:00") { | |
| t.Fatal("view should render last sync") | |
| } | |
| if !strings.Contains(out, "2") { | |
| t.Fatal("view should render pending count") | |
| } | |
| if !strings.Contains(out, "last fail") { | |
| t.Fatal("view should render last error") | |
| } | |
| } | |
| func TestViewCloudStatusRendersConfiguredState(t *testing.T) { | |
| m := New(nil, "") | |
| m.Screen = ScreenCloudStatus | |
| m.CloudStatusServerURL = "https://cloud.example.com" | |
| m.CloudStatusTokenSource = TokenSourceEnv | |
| m.CloudStatusHealth = "reachable" | |
| m.CloudStatusLastSync = "2026-07-07 12:00:00" | |
| m.CloudStatusPendingCount = 2 | |
| m.CloudStatusLastError = "last fail" | |
| out := m.View() | |
| if !strings.Contains(out, "Cloud status") { | |
| t.Fatal("view should render screen title") | |
| } | |
| if !strings.Contains(out, "https://cloud.example.com") { | |
| t.Fatal("view should render server URL") | |
| } | |
| if !strings.Contains(out, "reachable") { | |
| t.Fatal("view should render health") | |
| } | |
| if !strings.Contains(out, TokenSourceEnv) { | |
| t.Fatal("view should render token source") | |
| } | |
| if !strings.Contains(out, "2026-07-07 12:00:00") { | |
| t.Fatal("view should render last sync") | |
| } | |
| if !strings.Contains(out, "Pending: 2") { | |
| t.Fatal("view should render pending count") | |
| } | |
| if !strings.Contains(out, "last fail") { | |
| t.Fatal("view should render last error") | |
| } | |
| } |
🤖 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_test.go` around lines 111 - 143, Replace the broad
strings.Contains(out, "2") check in TestViewCloudStatusRendersConfiguredState
with an assertion that specifically verifies the rendered Pending row contains
the configured count 2. Anchor the check to the expected field label and value
so the assertion cannot pass because of digits in CloudStatusLastSync.
Source: Path instructions
Comment on lines
+212
to
+221
| buttons := []string{"[Test]", "[Save]", "[Cancel]"} | ||
| for i, label := range buttons { | ||
| focus := i + 1 // input is 0 | ||
| if focus == m.CloudConfigFocus { | ||
| b.WriteString(menuSelectedStyle.Render("▸ " + label)) | ||
| } else { | ||
| b.WriteString(menuItemStyle.Render(" " + item)) | ||
| b.WriteString(menuItemStyle.Render(" " + label)) | ||
| } | ||
| b.WriteString(" ") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the actual focus constant ordering in the dependent layer
rg -n 'cloudConfigFocus' -C2 --type=goRepository: Gentleman-Programming/engram
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant tui files and focus symbols
printf '\n## Files under internal/tui\n'
git ls-files 'internal/tui/**' | sed -n '1,120p'
printf '\n## Search for CloudConfigFocus / focus constants\n'
rg -n 'CloudConfigFocus|cloudConfigFocus|Focus' internal/tui -g '*.go' -C 2 || true
printf '\n## Outline candidate files if present\n'
for f in internal/tui/view.go internal/tui/*.go; do
[ -f "$f" ] && { echo "--- $f"; ast-grep outline "$f" --view expanded || true; }
doneRepository: Gentleman-Programming/engram
Length of output: 40221
Pair the labels with their focus constants. i + 1 depends on the current cloudConfigFocus* ordering; explicit pairs keep this renderer aligned if the focus states change.
🤖 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 212 - 221, Update the button-rendering
loop in the cloud configuration view to pair each label with its corresponding
cloudConfigFocus* constant explicitly instead of deriving focus with i + 1.
Compare CloudConfigFocus against the paired focus value while preserving the
existing selected and unselected rendering behavior.
This was referenced Jul 14, 2026
Author
|
@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! |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /tmp
gh pr view 609 --repo Gentleman-Programming/engram --json commits --jq '.commits[-5:] | .[] | {oid: .oid, message: .messageHeadline}'Length of output: 872 🏁 Script executed: #!/bin/bash
cd /tmp
gh pr view 609 --repo Gentleman-Programming/engram --json commits --jq '.commits | length'
gh pr view 609 --repo Gentleman-Programming/engram --json commits --jq '.commits[] | {oid: .oid, message: .messageHeadline}'
echo "---"
gh api repos/Gentleman-Programming/engram/commits/6dde3ce --jq '{sha: .sha, message: .commit.message}' 2>&1
echo "---"
gh api repos/Gentleman-Programming/engram/commits/b129225 --jq '{sha: .sha, message: .commit.message}' 2>&1
echo "---"
gh pr view 609 --repo Gentleman-Programming/engram --json headRefName,headRefOid,baseRefNameLength of output: 1951 🏁 Script executed: #!/bin/bash
cd /tmp
echo "Branches containing 6dde3ce2:"
gh api repos/Gentleman-Programming/engram/commits/6dde3ce2a250eac3e3983059646f70373e0622c6/branches-where-head --jq '.[].name' 2>&1
echo "---"
echo "Branches containing b1292254:"
gh api repos/Gentleman-Programming/engram/commits/b1292254785ba82a085e595271ec44ac7a115927/branches-where-head --jq '.[].name' 2>&1
echo "---"
echo "PRs containing 6dde3ce2:"
gh api search/issues -X GET -f q='6dde3ce2 repo:Gentleman-Programming/engram' --jq '.items[].html_url' 2>&1
echo "---"
gh pr list --repo Gentleman-Programming/engram --state all --search "cloud" --json number,title,headRefName,baseRefNameLength of output: 5743 🏁 Script executed: #!/bin/bash
cd /tmp
gh api repos/Gentleman-Programming/engram/compare/feat/tui-cloud-config-03-status...feat/tui-cloud-config-02-config --jq '{ahead_by: .behind_by, commits: [.commits[].sha]}' 2>&1
echo "---"
gh api repos/Gentleman-Programming/engram/compare/feat/tui-cloud-config-02-config...feat/tui-cloud-config-03-status --jq '{ahead_by: .ahead_by, behind_by: .behind_by}' 2>&1
echo "--- check if 03-status merge-base includes 6dde3ce ---"
gh api repos/Gentleman-Programming/engram/compare/84bfeff83a064bae9a8d8e30b5da1a963b564b23...6dde3ce2a250eac3e3983059646f70373e0622c6 --jq '{status: .status, ahead_by: .ahead_by, behind_by: .behind_by}' 2>&1Length of output: 1198
Since this PR's stack depends on ✅ Action performedReview finished.
|
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 the Cloud Status read-only screen: server URL, connection health, token source, last sync time, pending mutation count, and last error.
Spec
What's included
84bfefffeat(tui): add cloud status viewDesign notes
loadCloudConfigfrom PR fix(store): handle NULL project values in user_prompts #2 for URL + token source.CountPendingSyncMutationsaccessor onstore.Storeto fetch pending count.Test evidence
go test ./...passesgo vet ./...cleanMerge order
This is PR #3 of 6. Depends on PR #2 (merged first). After PR #2 merges, click "Update branch" on this PR.
Order:
Issue link
Part of #577
Summary by CodeRabbit