test(tui): close coverage gaps and harden env-var tests#611
Open
mgaldamez wants to merge 10 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds cloud sync settings to the TUI, including configuration persistence, token resolution, server health checks, sync status, project enrollment, navigation, rendering, and tests. ChangesCloud sync TUI
Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant Store
participant CloudServer
User->>TUI: Open cloud sync settings
TUI->>Store: Load configuration and sync state
Store-->>TUI: Return config, token source, and pending count
TUI->>CloudServer: Ping health endpoint
CloudServer-->>TUI: Return health status
TUI-->>User: Render cloud status or save result
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 introduces (and tests) a new Cloud sync settings flow in the TUI, including cloud config persistence (cloud.json), health pinging, status display, and project enrollment toggling, plus a doc update clarifying the cloud.json.token fallback behavior.
Changes:
- Adds new TUI screens + navigation for Cloud sync settings (configure server, view status, enroll projects).
- Introduces cloud config helpers (load/save config, token source resolution,
/healthping) and store helpers used by the TUI. - Expands test coverage across the new TUI cloud flows, cloud helpers, and new store helper methods; updates troubleshooting docs.
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 rendering for cloud settings/config/status/enrollment screens and refactors menu rendering. |
| internal/tui/view_test.go | Adds view-level tests covering the new cloud screens and branches. |
| internal/tui/update.go | Adds key handling, message handling, and commands wiring for cloud settings workflows. |
| internal/tui/update_test.go | Adds extensive navigation + behavior tests for cloud settings/config/status/enrollment flows. |
| internal/tui/model.go | Adds new screens, message types, model fields, and load commands for cloud status/config/enrollment. |
| internal/tui/model_test.go | Adds tests for new screen constants and load command behavior (including env-var hardening). |
| internal/tui/cloud.go | New cloud config + token resolution + /health ping helpers used by the TUI. |
| internal/tui/cloud_test.go | Adds unit tests for config load/save, token source behavior, pinging, and URL validation. |
| internal/store/store.go | Adds DataDir() and CountPendingSyncMutations() used by the TUI cloud status screen. |
| internal/store/store_test.go | Adds tests for DataDir() and CountPendingSyncMutations(). |
| docs/engram-cloud/troubleshooting.md | Documents cloud.json.token fallback and env-var precedence for cloud auth tokens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+226
to
+231
| 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(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) |
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() | ||
| } |
Comment on lines
+277
to
+286
| 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() | ||
| } |
Comment on lines
+1
to
+5
| package tui | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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-1437: Add an assertion to TestCountPendingSyncMutations that
creates a pending mutation associated with a project not enrolled via
EnrollProject, then verifies CountPendingSyncMutations does not include it.
Preserve the existing enrolled-project assertions and confirm the count remains
unchanged after adding the non-enrolled project mutation.
In `@internal/tui/cloud.go`:
- Around line 53-67: Update saveCloudConfig to write cloud.json with owner-only
permissions by changing the mode passed to os.WriteFile from 0o644 to 0o600,
while preserving the existing configuration serialization and save flow.
In `@internal/tui/model_test.go`:
- Around line 91-195: Consolidate TestScreenCloudSettingsConstant,
TestScreenCloudConfigConstant, TestScreenCloudStatusConstant, and
TestScreenCloudEnrollmentConstant into one table-driven test that defines the
expected consecutive relationships and validates the complete Screen constant
set once for uniqueness. Keep each constant’s successor expectation covered
without repeatedly listing all prior screens.
In `@internal/tui/update.go`:
- Around line 137-150: Update the cloud ping flow around cloudStatusLoadedMsg
and pingCloudServer to carry the ping kind, validated request URL, and unique
request ID in cloudPingMsg. Track the active request and ignore replies whose ID
or URL no longer matches, and prevent launching duplicate save pings; apply
response state only for the matching request rather than reading mutable screen
input.
- Around line 783-796: Move the EnrollProject/UnenrollProject write currently
performed in Update’s space-key handler into an asynchronous Bubble Tea command,
so Update only schedules the operation and remains responsive. Ensure the
command returns a message that preserves CloudEnrollmentError when the write
fails, rather than allowing a successful loadCloudEnrollmentCmd refresh to clear
it. Add coverage for the write-fails/read-succeeds scenario.
In `@internal/tui/view.go`:
- Around line 288-309: Update the CloudEnrollmentItems rendering loop in the
view’s View method to display a height-bounded, cursor-centered window with
appropriate start/end bounds, keeping the selected project visible while
preserving item styling and selection behavior. Add deterministic navigation
tests covering oversized enrollment lists and cursor movement at viewport
boundaries.
- Around line 226-239: Update the configuration-screen rendering in the view
method containing the CloudConfigSaving and CloudConfigPingStatus branches to
display CloudConfigError with actionable error details, including validation,
ping, and persistence failures. Preserve the existing saving spinner and
ping-status rendering, and add deterministic tests covering successful rendering
plus each of these error paths.
🪄 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: fb84b836-734b-497a-9cc8-6d47891784e3
📒 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
+1437
| 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 coverage for the non-enrolled-project exclusion branch.
The test only exercises mutations under an enrolled project. It never asserts that a mutation tied to a project that is not enrolled is excluded from the count — that's the actual new eligibility rule the LEFT JOIN ... sep.project IS NOT NULL clause implements, and it's currently unverified.
✅ Suggested additional assertion
count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey)
if err != nil {
t.Fatalf("CountPendingSyncMutations after observation: %v", err)
}
if count != 2 {
t.Fatalf("count = %d, want 2", count)
}
+
+ // A session/observation under a project that is NOT enrolled must not
+ // be counted as a pending, eligible-to-sync mutation.
+ if err := s.CreateSession("s2", "unenrolled-project", "/tmp/other"); err != nil {
+ t.Fatalf("create session for unenrolled project: %v", err)
+ }
+ count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey)
+ if err != nil {
+ t.Fatalf("CountPendingSyncMutations after unenrolled session: %v", err)
+ }
+ if count != 2 {
+ t.Fatalf("count = %d, want 2 (unenrolled project mutation should not count)", count)
+ }
}As per path instructions, **/*_test.go: "Verify coverage of happy path, error paths, and edge cases."
📝 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 TestCountPendingSyncMutations(t *testing.T) { | |
| s := newTestStore(t) | |
| count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations empty: %v", err) | |
| } | |
| if count != 0 { | |
| t.Fatalf("count = %d, want 0", count) | |
| } | |
| if err := s.EnrollProject("engram"); err != nil { | |
| t.Fatalf("enroll: %v", err) | |
| } | |
| if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { | |
| t.Fatalf("create session: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after session: %v", err) | |
| } | |
| if count != 1 { | |
| t.Fatalf("count = %d, want 1", count) | |
| } | |
| if _, err := s.AddObservation(AddObservationParams{ | |
| SessionID: "s1", | |
| Type: "bugfix", | |
| Title: "one", | |
| Content: "content", | |
| Project: "engram", | |
| Scope: "project", | |
| }); err != nil { | |
| t.Fatalf("add observation: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after observation: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2", count) | |
| } | |
| } | |
| func TestCountPendingSyncMutations(t *testing.T) { | |
| s := newTestStore(t) | |
| count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations empty: %v", err) | |
| } | |
| if count != 0 { | |
| t.Fatalf("count = %d, want 0", count) | |
| } | |
| if err := s.EnrollProject("engram"); err != nil { | |
| t.Fatalf("enroll: %v", err) | |
| } | |
| if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil { | |
| t.Fatalf("create session: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after session: %v", err) | |
| } | |
| if count != 1 { | |
| t.Fatalf("count = %d, want 1", count) | |
| } | |
| if _, err := s.AddObservation(AddObservationParams{ | |
| SessionID: "s1", | |
| Type: "bugfix", | |
| Title: "one", | |
| Content: "content", | |
| Project: "engram", | |
| Scope: "project", | |
| }); err != nil { | |
| t.Fatalf("add observation: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after observation: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2", count) | |
| } | |
| // A session/observation under a project that is NOT enrolled must not | |
| // be counted as a pending, eligible-to-sync mutation. | |
| if err := s.CreateSession("s2", "unenrolled-project", "/tmp/other"); err != nil { | |
| t.Fatalf("create session for unenrolled project: %v", err) | |
| } | |
| count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) | |
| if err != nil { | |
| t.Fatalf("CountPendingSyncMutations after unenrolled session: %v", err) | |
| } | |
| if count != 2 { | |
| t.Fatalf("count = %d, want 2 (unenrolled project mutation should not count)", count) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/store_test.go` around lines 1392 - 1437, Add an assertion to
TestCountPendingSyncMutations that creates a pending mutation associated with a
project not enrolled via EnrollProject, then verifies CountPendingSyncMutations
does not include it. Preserve the existing enrolled-project assertions and
confirm the count remains unchanged after adding the non-enrolled project
mutation.
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 config file is written world/group-readable, exposing the auth token.
cloud.json can hold a bearer token (see tuiCloudConfig.Token, preserved across saves), but it's written with 0o644 under a 0o755 directory — any other local user on a shared host can read it.
🔒 Proposed fix: restrict file 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)
}Note: os.MkdirAll only applies the given mode when it actually creates the directory, so if dataDir pre-exists with looser permissions this alone won't retroactively tighten it — but it removes the risk for fresh setups and is a safe no-op otherwise.
📝 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 to write
cloud.json with owner-only permissions by changing the mode passed to
os.WriteFile from 0o644 to 0o600, while preserving the existing configuration
serialization and save flow.
Comment on lines
+91
to
+195
| func TestScreenCloudSettingsConstant(t *testing.T) { | ||
| if ScreenCloudSettings != ScreenSetup+1 { | ||
| t.Fatalf("ScreenCloudSettings = %d, want %d (ScreenSetup+1)", ScreenCloudSettings, ScreenSetup+1) | ||
| } | ||
|
|
||
| seen := map[Screen]bool{} | ||
| for _, s := range []Screen{ | ||
| ScreenDashboard, | ||
| ScreenSearch, | ||
| ScreenSearchResults, | ||
| ScreenRecent, | ||
| ScreenObservationDetail, | ||
| ScreenTimeline, | ||
| ScreenSessions, | ||
| ScreenSessionDetail, | ||
| ScreenSetup, | ||
| ScreenCloudSettings, | ||
| } { | ||
| if seen[s] { | ||
| t.Fatalf("screen constant %d is duplicated", s) | ||
| } | ||
| seen[s] = true | ||
| } | ||
| } | ||
|
|
||
| func TestScreenCloudConfigConstant(t *testing.T) { | ||
| if ScreenCloudConfig != ScreenCloudSettings+1 { | ||
| t.Fatalf("ScreenCloudConfig = %d, want %d (ScreenCloudSettings+1)", ScreenCloudConfig, ScreenCloudSettings+1) | ||
| } | ||
|
|
||
| seen := map[Screen]bool{} | ||
| for _, s := range []Screen{ | ||
| ScreenDashboard, | ||
| ScreenSearch, | ||
| ScreenSearchResults, | ||
| ScreenRecent, | ||
| ScreenObservationDetail, | ||
| ScreenTimeline, | ||
| ScreenSessions, | ||
| ScreenSessionDetail, | ||
| ScreenSetup, | ||
| ScreenCloudSettings, | ||
| ScreenCloudConfig, | ||
| } { | ||
| if seen[s] { | ||
| t.Fatalf("screen constant %d is duplicated", s) | ||
| } | ||
| seen[s] = true | ||
| } | ||
| } | ||
|
|
||
| func TestScreenCloudStatusConstant(t *testing.T) { | ||
| if ScreenCloudStatus != ScreenCloudConfig+1 { | ||
| t.Fatalf("ScreenCloudStatus = %d, want %d (ScreenCloudConfig+1)", ScreenCloudStatus, ScreenCloudConfig+1) | ||
| } | ||
|
|
||
| seen := map[Screen]bool{} | ||
| for _, s := range []Screen{ | ||
| ScreenDashboard, | ||
| ScreenSearch, | ||
| ScreenSearchResults, | ||
| ScreenRecent, | ||
| ScreenObservationDetail, | ||
| ScreenTimeline, | ||
| ScreenSessions, | ||
| ScreenSessionDetail, | ||
| ScreenSetup, | ||
| ScreenCloudSettings, | ||
| ScreenCloudConfig, | ||
| ScreenCloudStatus, | ||
| } { | ||
| if seen[s] { | ||
| t.Fatalf("screen constant %d is duplicated", s) | ||
| } | ||
| seen[s] = true | ||
| } | ||
| } | ||
|
|
||
| func TestScreenCloudEnrollmentConstant(t *testing.T) { | ||
| if ScreenCloudEnrollment != ScreenCloudStatus+1 { | ||
| t.Fatalf("ScreenCloudEnrollment = %d, want %d (ScreenCloudStatus+1)", ScreenCloudEnrollment, ScreenCloudStatus+1) | ||
| } | ||
|
|
||
| seen := map[Screen]bool{} | ||
| for _, s := range []Screen{ | ||
| ScreenDashboard, | ||
| ScreenSearch, | ||
| ScreenSearchResults, | ||
| ScreenRecent, | ||
| ScreenObservationDetail, | ||
| ScreenTimeline, | ||
| ScreenSessions, | ||
| ScreenSessionDetail, | ||
| ScreenSetup, | ||
| ScreenCloudSettings, | ||
| ScreenCloudConfig, | ||
| ScreenCloudStatus, | ||
| ScreenCloudEnrollment, | ||
| } { | ||
| if seen[s] { | ||
| t.Fatalf("screen constant %d is duplicated", s) | ||
| } | ||
| seen[s] = true | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consolidate the four near-duplicate screen-constant tests.
Each test re-lists the entire growing set of Screen constants just to add one more value; this duplication will keep growing every time a screen is added.
♻️ Proposed consolidation
-func TestScreenCloudSettingsConstant(t *testing.T) {
- if ScreenCloudSettings != ScreenSetup+1 {
- t.Fatalf("ScreenCloudSettings = %d, want %d (ScreenSetup+1)", ScreenCloudSettings, ScreenSetup+1)
- }
- ...
-}
-
-func TestScreenCloudConfigConstant(t *testing.T) { ... }
-func TestScreenCloudStatusConstant(t *testing.T) { ... }
-func TestScreenCloudEnrollmentConstant(t *testing.T) { ... }
+func TestCloudScreenConstants(t *testing.T) {
+ ordered := []struct {
+ name string
+ got Screen
+ want Screen
+ }{
+ {"ScreenCloudSettings", ScreenCloudSettings, ScreenSetup + 1},
+ {"ScreenCloudConfig", ScreenCloudConfig, ScreenCloudSettings + 1},
+ {"ScreenCloudStatus", ScreenCloudStatus, ScreenCloudConfig + 1},
+ {"ScreenCloudEnrollment", ScreenCloudEnrollment, ScreenCloudStatus + 1},
+ }
+ for _, tt := range ordered {
+ if tt.got != tt.want {
+ t.Errorf("%s = %d, want %d", tt.name, tt.got, tt.want)
+ }
+ }
+
+ seen := map[Screen]bool{}
+ for _, s := range []Screen{
+ ScreenDashboard, ScreenSearch, ScreenSearchResults, ScreenRecent,
+ ScreenObservationDetail, ScreenTimeline, ScreenSessions, ScreenSessionDetail,
+ ScreenSetup, ScreenCloudSettings, ScreenCloudConfig, ScreenCloudStatus, ScreenCloudEnrollment,
+ } {
+ if seen[s] {
+ t.Fatalf("screen constant %d is duplicated", s)
+ }
+ seen[s] = true
+ }
+}📝 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 TestScreenCloudSettingsConstant(t *testing.T) { | |
| if ScreenCloudSettings != ScreenSetup+1 { | |
| t.Fatalf("ScreenCloudSettings = %d, want %d (ScreenSetup+1)", ScreenCloudSettings, ScreenSetup+1) | |
| } | |
| seen := map[Screen]bool{} | |
| for _, s := range []Screen{ | |
| ScreenDashboard, | |
| ScreenSearch, | |
| ScreenSearchResults, | |
| ScreenRecent, | |
| ScreenObservationDetail, | |
| ScreenTimeline, | |
| ScreenSessions, | |
| ScreenSessionDetail, | |
| ScreenSetup, | |
| ScreenCloudSettings, | |
| } { | |
| if seen[s] { | |
| t.Fatalf("screen constant %d is duplicated", s) | |
| } | |
| seen[s] = true | |
| } | |
| } | |
| func TestScreenCloudConfigConstant(t *testing.T) { | |
| if ScreenCloudConfig != ScreenCloudSettings+1 { | |
| t.Fatalf("ScreenCloudConfig = %d, want %d (ScreenCloudSettings+1)", ScreenCloudConfig, ScreenCloudSettings+1) | |
| } | |
| seen := map[Screen]bool{} | |
| for _, s := range []Screen{ | |
| ScreenDashboard, | |
| ScreenSearch, | |
| ScreenSearchResults, | |
| ScreenRecent, | |
| ScreenObservationDetail, | |
| ScreenTimeline, | |
| ScreenSessions, | |
| ScreenSessionDetail, | |
| ScreenSetup, | |
| ScreenCloudSettings, | |
| ScreenCloudConfig, | |
| } { | |
| if seen[s] { | |
| t.Fatalf("screen constant %d is duplicated", s) | |
| } | |
| seen[s] = true | |
| } | |
| } | |
| func TestScreenCloudStatusConstant(t *testing.T) { | |
| if ScreenCloudStatus != ScreenCloudConfig+1 { | |
| t.Fatalf("ScreenCloudStatus = %d, want %d (ScreenCloudConfig+1)", ScreenCloudStatus, ScreenCloudConfig+1) | |
| } | |
| seen := map[Screen]bool{} | |
| for _, s := range []Screen{ | |
| ScreenDashboard, | |
| ScreenSearch, | |
| ScreenSearchResults, | |
| ScreenRecent, | |
| ScreenObservationDetail, | |
| ScreenTimeline, | |
| ScreenSessions, | |
| ScreenSessionDetail, | |
| ScreenSetup, | |
| ScreenCloudSettings, | |
| ScreenCloudConfig, | |
| ScreenCloudStatus, | |
| } { | |
| if seen[s] { | |
| t.Fatalf("screen constant %d is duplicated", s) | |
| } | |
| seen[s] = true | |
| } | |
| } | |
| func TestScreenCloudEnrollmentConstant(t *testing.T) { | |
| if ScreenCloudEnrollment != ScreenCloudStatus+1 { | |
| t.Fatalf("ScreenCloudEnrollment = %d, want %d (ScreenCloudStatus+1)", ScreenCloudEnrollment, ScreenCloudStatus+1) | |
| } | |
| seen := map[Screen]bool{} | |
| for _, s := range []Screen{ | |
| ScreenDashboard, | |
| ScreenSearch, | |
| ScreenSearchResults, | |
| ScreenRecent, | |
| ScreenObservationDetail, | |
| ScreenTimeline, | |
| ScreenSessions, | |
| ScreenSessionDetail, | |
| ScreenSetup, | |
| ScreenCloudSettings, | |
| ScreenCloudConfig, | |
| ScreenCloudStatus, | |
| ScreenCloudEnrollment, | |
| } { | |
| if seen[s] { | |
| t.Fatalf("screen constant %d is duplicated", s) | |
| } | |
| seen[s] = true | |
| } | |
| } | |
| func TestCloudScreenConstants(t *testing.T) { | |
| ordered := []struct { | |
| name string | |
| got Screen | |
| want Screen | |
| }{ | |
| {"ScreenCloudSettings", ScreenCloudSettings, ScreenSetup + 1}, | |
| {"ScreenCloudConfig", ScreenCloudConfig, ScreenCloudSettings + 1}, | |
| {"ScreenCloudStatus", ScreenCloudStatus, ScreenCloudConfig + 1}, | |
| {"ScreenCloudEnrollment", ScreenCloudEnrollment, ScreenCloudStatus + 1}, | |
| } | |
| for _, tt := range ordered { | |
| if tt.got != tt.want { | |
| t.Errorf("%s = %d, want %d", tt.name, tt.got, tt.want) | |
| } | |
| } | |
| seen := map[Screen]bool{} | |
| for _, s := range []Screen{ | |
| ScreenDashboard, | |
| ScreenSearch, | |
| ScreenSearchResults, | |
| ScreenRecent, | |
| ScreenObservationDetail, | |
| ScreenTimeline, | |
| ScreenSessions, | |
| ScreenSessionDetail, | |
| ScreenSetup, | |
| ScreenCloudSettings, | |
| ScreenCloudConfig, | |
| ScreenCloudStatus, | |
| ScreenCloudEnrollment, | |
| } { | |
| if seen[s] { | |
| t.Fatalf("screen constant %d is duplicated", s) | |
| } | |
| seen[s] = 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/model_test.go` around lines 91 - 195, Consolidate
TestScreenCloudSettingsConstant, TestScreenCloudConfigConstant,
TestScreenCloudStatusConstant, and TestScreenCloudEnrollmentConstant into one
table-driven test that defines the expected consecutive relationships and
validates the complete Screen constant set once for uniqueness. Keep each
constant’s successor expectation covered without repeatedly listing all prior
screens.
Comment on lines
+137
to
+150
| 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())) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files internal/tui/update.go internal/tui/view.go internal/tui/*.go | sed -n '1,120p'
printf '\n--- outline: internal/tui/update.go ---\n'
ast-grep outline internal/tui/update.go --view expanded || true
printf '\n--- outline: internal/tui/view.go ---\n'
ast-grep outline internal/tui/view.go --view expanded || true
printf '\n--- search cloudPingMsg / pingCloudServer / CloudConfigTest ---\n'
rg -n "cloudPingMsg|pingCloudServer|CloudConfigTest|CloudStatusLoaded|CloudStatusLoading|Screen" internal/tui -g '*.go' -A 4 -B 4
printf '\n--- relevant slices from internal/tui/update.go ---\n'
sed -n '1,260p' internal/tui/update.go
printf '\n--- relevant slices from internal/tui/view.go ---\n'
sed -n '1,260p' internal/tui/view.goRepository: Gentleman-Programming/engram
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- internal/tui/update.go (120-210) ---\n'
sed -n '120,210p' internal/tui/update.go
printf '\n--- internal/tui/update.go (889-920) ---\n'
sed -n '889,920p' internal/tui/update.go
printf '\n--- internal/tui/cloud.go (90-130) ---\n'
sed -n '90,130p' internal/tui/cloud.go
printf '\n--- internal/tui/update_test.go cloud-related tests ---\n'
rg -n "CloudConfig|CloudStatus|cloudPing|pingCloudServer|saveCloudConfig|loadCloudConfig" internal/tui/update_test.go internal/tui/cloud_test.go -A 4 -B 4
printf '\n--- internal/tui/update_test.go focused slices ---\n'
sed -n '1,260p' internal/tui/update_test.goRepository: Gentleman-Programming/engram
Length of output: 49435
Tag cloud ping responses with request metadata.
cloudPingMsg is still routed from mutable screen state, so a late response can be applied to the wrong form or persist the current input instead of the request that produced it. Carry the ping kind, validated URL, and a request ID in the message, then ignore stale replies and prevent duplicate save pings.
🤖 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 - 150, Update the cloud ping flow
around cloudStatusLoadedMsg and pingCloudServer to carry the ping kind,
validated request URL, and unique request ID in cloudPingMsg. Track the active
request and ignore replies whose ID or URL no longer matches, and prevent
launching duplicate save pings; apply response state only for the matching
request rather than reading mutable screen input.
Source: Path instructions
Comment on lines
+783
to
+796
| 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) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '740,840p' internal/tui/update.go
printf '\n---\n'
rg -n "loadCloudEnrollmentCmd|CloudEnrollmentError|EnrollProject|UnenrollProject" internal/tui -S
printf '\n--- tests ---\n'
rg -n "CloudEnrollment|EnrollProject|UnenrollProject|loadCloudEnrollmentCmd" internal/tui -g '*_test.go' -SRepository: Gentleman-Programming/engram
Length of output: 10980
🏁 Script executed:
sed -n '330,390p' internal/tui/model.go
printf '\n--- store search ---\n'
rg -n "func .*EnrollProject|func .*UnenrollProject|EnrollProject\\(|UnenrollProject\\(" internal -S
printf '\n--- relevant tests ---\n'
sed -n '440,545p' internal/tui/update_test.go
printf '\n---\n'
sed -n '315,360p' internal/tui/model_test.goRepository: Gentleman-Programming/engram
Length of output: 14011
🏁 Script executed:
sed -n '120,180p' internal/tui/update.go
printf '\n--- store methods ---\n'
sed -n '4340,4415p' internal/store/store.goRepository: Gentleman-Programming/engram
Length of output: 4082
Move the enrollment write out of Update.
EnrollProject can block on a transaction/backfill here, which stalls Bubble Tea input/rendering. The reload path also clears CloudEnrollmentError on a successful read, so a write failure can disappear if the refresh succeeds. Add coverage for the write-fails/read-succeeds case.
🤖 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 783 - 796, Move the
EnrollProject/UnenrollProject write currently performed in Update’s space-key
handler into an asynchronous Bubble Tea command, so Update only schedules the
operation and remains responsive. Ensure the command returns a message that
preserves CloudEnrollmentError when the write fails, rather than allowing a
successful loadCloudEnrollmentCmd refresh to clear it. Add coverage for the
write-fails/read-succeeds scenario.
Source: Path instructions
Comment on lines
+226
to
+239
| 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") | ||
| } | ||
|
|
||
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | ||
|
|
||
| return b.String() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render CloudConfigError on the configuration screen.
Invalid URLs set only CloudConfigError and return no command, so Save appears to do nothing. Ping and persistence failures also lose their actionable details.
Proposed fix
} else if m.CloudConfigPingStatus != "" {
b.WriteString("\n")
b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus))
b.WriteString("\n")
}
+
+ if m.CloudConfigError != "" {
+ b.WriteString("\n")
+ b.WriteString(errorStyle.Render("Error: " + m.CloudConfigError))
+ b.WriteString("\n")
+ }Add view coverage for validation, ping, and save errors.
As per path instructions, **/*_test.go: “Verify coverage of happy path, error paths, and edge cases. Tests must be deterministic. Behavior changes without tests should be blocked.”
📝 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
| 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") | |
| } | |
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | |
| return b.String() | |
| 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") | |
| } | |
| if m.CloudConfigError != "" { | |
| b.WriteString("\n") | |
| b.WriteString(errorStyle.Render("Error: " + m.CloudConfigError)) | |
| b.WriteString("\n") | |
| } | |
| b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) | |
| return b.String() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tui/view.go` around lines 226 - 239, Update the configuration-screen
rendering in the view method containing the CloudConfigSaving and
CloudConfigPingStatus branches to display CloudConfigError with actionable error
details, including validation, ping, and persistence failures. Preserve the
existing saving spinner and ping-status rendering, and add deterministic tests
covering successful rendering plus each of these error paths.
Source: Path instructions
Comment on lines
+288
to
+309
| for i, item := range m.CloudEnrollmentItems { | ||
| cursor := " " | ||
| style := listItemStyle | ||
| if i == m.Cursor { | ||
| cursor = "▸ " | ||
| style = listSelectedStyle | ||
| } | ||
|
|
||
| badge := "[not enrolled]" | ||
| badgeStyle := timestampStyle | ||
| if item.enrolled { | ||
| badge = "[enrolled]" | ||
| badgeStyle = lipgloss.NewStyle().Foreground(colorGreen).Bold(true) | ||
| } | ||
|
|
||
| line := fmt.Sprintf("%s%s %s", | ||
| cursor, | ||
| style.Render(truncateStr(item.project, 50)), | ||
| badgeStyle.Render(badge)) | ||
| b.WriteString(line) | ||
| b.WriteString("\n") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep the selected enrollment project inside a visible viewport.
The key handler can move Cursor across the full list, but this view renders every item without clipping or scroll state. With more rows than the terminal height, the selected project moves off-screen and cannot be operated reliably.
Render a height-bounded window around the cursor and add an oversized-list navigation test.
As per path instructions, **/*_test.go: “Verify coverage of happy path, error paths, and edge cases. Tests must be deterministic. Behavior changes without tests should be 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/tui/view.go` around lines 288 - 309, Update the CloudEnrollmentItems
rendering loop in the view’s View method to display a height-bounded,
cursor-centered window with appropriate start/end bounds, keeping the selected
project visible while preserving item styling and selection behavior. Add
deterministic navigation tests covering oversized enrollment lists and cursor
movement at viewport boundaries.
Source: Path instructions
This was referenced Jul 14, 2026
Contributor
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
set -euo pipefail
gh pr view 611 --repo Gentleman-Programming/engram --json commits --jq '.commits[-10:] | .[] | {oid: .oid, message: .messageHeadline}'
echo "---diff for latest commits---"
gh pr diff 611 --repo Gentleman-Programming/engram | head -300Length of output: 10323 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd $(git rev-parse --show-toplevel 2>/dev/null || echo .)
git fetch origin pull/611/head:pr611 2>&1 | tail -5 || true
git log --oneline -15 pr611 2>&1 || true
echo "---search commit 6dde3ce---"
git show 6dde3ce --stat 2>&1 | head -30 || echo "not found"
echo "---search commit b129225---"
git show b129225 --stat 2>&1 | head -30 || echo "not found"Length of output: 2006 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 611 --repo Gentleman-Programming/engram --json headRefName,baseRefName,updatedAt,commits --jq '{head: .headRefName, base: .baseRefName, updated: .updatedAt}'
echo "---search for commits across repo---"
git fetch origin 6dde3ce b129225 2>&1 | tail -20 || true
echo "---list branches containing docstring/godoc changes---"
gh api search/commits -X GET -f q='hash:6dde3ce repo:Gentleman-Programming/engram' --jq '.items[] | {sha: .sha, message: .commit.message}' 2>&1 || true
gh api search/commits -X GET -f q='hash:b129225 repo:Gentleman-Programming/engram' --jq '.items[] | {sha: .sha, message: .commit.message}' 2>&1 || trueLength of output: 1025 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 611 --repo Gentleman-Programming/engram --json commits --jq '.commits[-1]'
gh api repos/Gentleman-Programming/engram/pulls/611/commits --jq '.[-5:] | .[] | {sha: .sha, message: .commit.message}'Length of output: 1304 The commits mentioned (6dde3ce and b129225) haven't actually landed on PR
✅ Action performedReview finished.
|
Collaborator
|
Reviewed as part of the #607–#612 series — full feedback is on #607 to keep it in one place. Short version: |
Alan-TheGentleman
added
type:feature
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
Closes per-function coverage gaps on the cloud config helpers and hardens the tests against
ENGRAM_CLOUD_TOKENenv-var leaks. Test-only changes — no behavior changes.Spec
What's included
47f2b48test(tui): close per-function coverage gaps on cloud config748f09ftest(tui): harden cloud config tests against ENGRAM_CLOUD_TOKEN envDesign notes
TestLoadCloudStatusCommandusest.Setenv("ENGRAM_CLOUD_TOKEN", "")so tests pass regardless of the shell environment.*_test.go).Test evidence
go test ./...passes (with and withoutENGRAM_CLOUD_TOKENset)internal/tuiMerge order
This is PR 5 of 6. Depends on PR 4 (merged first). After PR 4 merges, click "Update branch" on this PR.
Order:
Issue link
Part of #577
Summary by CodeRabbit
New Features
Documentation