diff --git a/cmd/roborev/client_test.go b/cmd/roborev/client_test.go index 9e4980a..abb442c 100644 --- a/cmd/roborev/client_test.go +++ b/cmd/roborev/client_test.go @@ -1,6 +1,6 @@ package main -// Tests for daemon client functions (getResponsesForJob, waitForReview, findJobForCommit) +// Tests for daemon client functions (getCommentsForJob, waitForReview, findJobForCommit) import ( "encoding/json" @@ -14,10 +14,10 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) -func TestGetResponsesForJob(t *testing.T) { +func TestGetCommentsForJob(t *testing.T) { t.Run("returns responses for job", func(t *testing.T) { _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/responses" || r.Method != "GET" { + if r.URL.Path != "/api/comments" || r.Method != "GET" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) return @@ -36,7 +36,7 @@ func TestGetResponsesForJob(t *testing.T) { })) defer cleanup() - responses, err := getResponsesForJob(42) + responses, err := getCommentsForJob(42) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -51,7 +51,7 @@ func TestGetResponsesForJob(t *testing.T) { })) defer cleanup() - _, err := getResponsesForJob(42) + _, err := getCommentsForJob(42) if err == nil { t.Fatal("expected error, got nil") } diff --git a/cmd/roborev/respond_test.go b/cmd/roborev/comment_test.go similarity index 90% rename from cmd/roborev/respond_test.go rename to cmd/roborev/comment_test.go index d481bfd..2a1f34f 100644 --- a/cmd/roborev/respond_test.go +++ b/cmd/roborev/comment_test.go @@ -1,6 +1,6 @@ package main -// Tests for the respond command +// Tests for the comment command import ( "encoding/json" @@ -12,7 +12,7 @@ import ( "github.com/roborev-dev/roborev/internal/version" ) -func TestRespondJobFlag(t *testing.T) { +func TestCommentJobFlag(t *testing.T) { t.Run("--job forces job ID interpretation", func(t *testing.T) { var receivedJobID int64 _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -20,7 +20,7 @@ func TestRespondJobFlag(t *testing.T) { json.NewEncoder(w).Encode(map[string]interface{}{"version": version.Version}) return } - if r.URL.Path == "/api/respond" && r.Method == "POST" { + if r.URL.Path == "/api/comment" && r.Method == "POST" { var req struct { JobID int64 `json:"job_id"` } @@ -34,7 +34,7 @@ func TestRespondJobFlag(t *testing.T) { defer cleanup() // "1234567" could be a SHA, but --job forces job ID interpretation - cmd := respondCmd() + cmd := commentCmd() cmd.SetArgs([]string{"--job", "1234567", "-m", "test message"}) err := cmd.Execute() if err != nil { @@ -52,7 +52,7 @@ func TestRespondJobFlag(t *testing.T) { })) defer cleanup() - cmd := respondCmd() + cmd := commentCmd() cmd.SetArgs([]string{"--job", "abc123", "-m", "test"}) err := cmd.Execute() if err == nil { diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index f12b2ce..e84ad3e 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -54,7 +54,8 @@ func main() { rootCmd.AddCommand(reviewCmd()) rootCmd.AddCommand(statusCmd()) rootCmd.AddCommand(showCmd()) - rootCmd.AddCommand(respondCmd()) + rootCmd.AddCommand(commentCmd()) + rootCmd.AddCommand(respondCmd()) // hidden alias for backward compatibility rootCmd.AddCommand(addressCmd()) rootCmd.AddCommand(installHookCmd()) rootCmd.AddCommand(uninstallHookCmd()) @@ -1199,27 +1200,27 @@ Examples: return cmd } -func respondCmd() *cobra.Command { +func commentCmd() *cobra.Command { var ( - responder string + commenter string message string forceJobID bool ) cmd := &cobra.Command{ - Use: "respond [message]", - Short: "Add a response to a review", - Long: `Add a response or note to a review. + Use: "comment [message]", + Short: "Add a comment to a review", + Long: `Add a comment or note to a review. The first argument can be either a job ID (numeric) or a commit SHA. Using job IDs is recommended since they are displayed in the TUI. Examples: - roborev respond 42 "Fixed the null pointer issue" - roborev respond 42 -m "Added missing error handling" - roborev respond abc123 "Addressed by refactoring" - roborev respond 42 # Opens editor for message - roborev respond --job 1234567 "msg" # Force numeric arg as job ID`, + roborev comment 42 "Fixed the null pointer issue" + roborev comment 42 -m "Added missing error handling" + roborev comment abc123 "Addressed by refactoring" + roborev comment 42 # Opens editor for message + roborev comment --job 1234567 "msg" # Force numeric arg as job ID`, Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { // Ensure daemon is running @@ -1272,7 +1273,7 @@ Examples: editor = "vim" } - tmpfile, err := os.CreateTemp("", "roborev-response-*.md") + tmpfile, err := os.CreateTemp("", "roborev-comment-*.md") if err != nil { return fmt.Errorf("create temp file: %w", err) } @@ -1289,26 +1290,26 @@ Examples: content, err := os.ReadFile(tmpfile.Name()) if err != nil { - return fmt.Errorf("read response: %w", err) + return fmt.Errorf("read comment: %w", err) } message = strings.TrimSpace(string(content)) } if message == "" { - return fmt.Errorf("empty response, aborting") + return fmt.Errorf("empty comment, aborting") } - if responder == "" { - responder = os.Getenv("USER") - if responder == "" { - responder = "anonymous" + if commenter == "" { + commenter = os.Getenv("USER") + if commenter == "" { + commenter = "anonymous" } } // Build request with either job_id or sha reqData := map[string]interface{}{ - "responder": responder, - "response": message, + "commenter": commenter, + "comment": message, } if jobID != 0 { reqData["job_id"] = jobID @@ -1319,7 +1320,7 @@ Examples: reqBody, _ := json.Marshal(reqData) addr := getDaemonAddr() - resp, err := http.Post(addr+"/api/respond", "application/json", bytes.NewReader(reqBody)) + resp, err := http.Post(addr+"/api/comment", "application/json", bytes.NewReader(reqBody)) if err != nil { return fmt.Errorf("failed to connect to daemon: %w", err) } @@ -1327,21 +1328,29 @@ Examples: if resp.StatusCode != http.StatusCreated { body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to add response: %s", body) + return fmt.Errorf("failed to add comment: %s", body) } - fmt.Println("Response added successfully") + fmt.Println("Comment added successfully") return nil }, } - cmd.Flags().StringVar(&responder, "responder", "", "responder name (default: $USER)") - cmd.Flags().StringVarP(&message, "message", "m", "", "response message (opens editor if not provided)") + cmd.Flags().StringVar(&commenter, "commenter", "", "commenter name (default: $USER)") + cmd.Flags().StringVarP(&message, "message", "m", "", "comment message (opens editor if not provided)") cmd.Flags().BoolVar(&forceJobID, "job", false, "force argument to be treated as job ID (not SHA)") return cmd } +// respondCmd returns an alias for commentCmd +func respondCmd() *cobra.Command { + cmd := commentCmd() + cmd.Use = "respond [message]" + cmd.Short = "Alias for 'comment' - add a comment to a review" + return cmd +} + func addressCmd() *cobra.Command { var unaddress bool @@ -1561,19 +1570,19 @@ func enqueueReview(repoPath, gitRef, agentName string) (int64, error) { return job.ID, nil } -// getResponsesForJob fetches responses for a job -func getResponsesForJob(jobID int64) ([]storage.Response, error) { +// getCommentsForJob fetches comments for a job +func getCommentsForJob(jobID int64) ([]storage.Response, error) { addr := getDaemonAddr() client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(fmt.Sprintf("%s/api/responses?job_id=%d", addr, jobID)) + resp, err := client.Get(fmt.Sprintf("%s/api/comments?job_id=%d", addr, jobID)) if err != nil { return nil, err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("fetch responses: %s", resp.Status) + return nil, fmt.Errorf("fetch comments: %s", resp.Status) } var result struct { @@ -2139,7 +2148,7 @@ func syncStatusCmd() *cobra.Command { const maxPending = 1000 jobs, jobsErr := db.GetJobsToSync(machineID, maxPending) reviews, reviewsErr := db.GetReviewsToSync(machineID, maxPending) - responses, responsesErr := db.GetResponsesToSync(machineID, maxPending) + responses, responsesErr := db.GetCommentsToSync(machineID, maxPending) fmt.Println() if jobsErr != nil || reviewsErr != nil || responsesErr != nil { @@ -2153,7 +2162,7 @@ func syncStatusCmd() *cobra.Command { } return fmt.Sprintf("%d", count) } - fmt.Printf("Pending push: %s jobs, %s reviews, %s responses\n", + fmt.Printf("Pending push: %s jobs, %s reviews, %s comments\n", formatCount(len(jobs)), formatCount(len(reviews)), formatCount(len(responses))) // Try to connect to PostgreSQL @@ -2257,13 +2266,13 @@ func syncNowCmd() *cobra.Command { totalJobs := getInt(msg, "total_jobs") totalRevs := getInt(msg, "total_revs") totalResps := getInt(msg, "total_resps") - fmt.Printf("\rPushing: batch %d (total: %d jobs, %d reviews, %d responses) ", + fmt.Printf("\rPushing: batch %d (total: %d jobs, %d reviews, %d comments) ", batch, totalJobs, totalRevs, totalResps) } else if phase == "pull" { totalJobs := getInt(msg, "total_jobs") totalRevs := getInt(msg, "total_revs") totalResps := getInt(msg, "total_resps") - fmt.Printf("\rPulled: %d jobs, %d reviews, %d responses \n", + fmt.Printf("\rPulled: %d jobs, %d reviews, %d comments \n", totalJobs, totalRevs, totalResps) } case "error": @@ -2289,9 +2298,9 @@ func syncNowCmd() *cobra.Command { } fmt.Println("Sync completed") - fmt.Printf("Pushed: %d jobs, %d reviews, %d responses\n", + fmt.Printf("Pushed: %d jobs, %d reviews, %d comments\n", finalPushed.Jobs, finalPushed.Reviews, finalPushed.Responses) - fmt.Printf("Pulled: %d jobs, %d reviews, %d responses\n", + fmt.Printf("Pulled: %d jobs, %d reviews, %d comments\n", finalPulled.Jobs, finalPulled.Reviews, finalPulled.Responses) return nil diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index c495989..bca7105 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -293,7 +293,7 @@ func TestRefineNoChangeRetryLogic(t *testing.T) { })) defer cleanup() - responses, _ := getResponsesForJob(1) + responses, _ := getCommentsForJob(1) // Count no-change attempts noChangeAttempts := 0 @@ -350,7 +350,7 @@ func TestRunRefineSurfacesResponseErrors(t *testing.T) { json.NewEncoder(w).Encode(storage.Review{ ID: 1, JobID: 1, Output: "**Bug found**: fail", Addressed: false, }) - case r.URL.Path == "/api/responses": + case r.URL.Path == "/api/comments": w.WriteHeader(http.StatusInternalServerError) default: w.WriteHeader(http.StatusNotFound) @@ -634,7 +634,7 @@ func createMockRefineHandler(state *mockRefineState) http.Handler { } json.NewEncoder(w).Encode(reviewCopy) - case r.URL.Path == "/api/responses" && r.Method == "GET": + case r.URL.Path == "/api/comments" && r.Method == "GET": jobIDStr := r.URL.Query().Get("job_id") var jobID int64 fmt.Sscanf(jobIDStr, "%d", &jobID) @@ -648,7 +648,7 @@ func createMockRefineHandler(state *mockRefineState) http.Handler { "responses": responses, }) - case r.URL.Path == "/api/respond" && r.Method == "POST": + case r.URL.Path == "/api/comment" && r.Method == "POST": var req struct { JobID int64 `json:"job_id"` Responder string `json:"responder"` @@ -807,7 +807,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, err := getResponsesForJob(42) + responses, err := getCommentsForJob(42) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -834,7 +834,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, _ := getResponsesForJob(42) + responses, _ := getCommentsForJob(42) noChangeAttempts := countNoChangeAttempts(responses) // Should not give up yet (first attempt) @@ -856,7 +856,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, _ := getResponsesForJob(42) + responses, _ := getCommentsForJob(42) noChangeAttempts := countNoChangeAttempts(responses) // Should only count the 1 response from roborev-refine, not the others @@ -1068,7 +1068,7 @@ func TestRefineLoopStaysOnFailedFixChain(t *testing.T) { } json.NewEncoder(w).Encode(review) - case r.URL.Path == "/api/responses" && r.Method == http.MethodGet: + case r.URL.Path == "/api/comments" && r.Method == http.MethodGet: jobIDStr := r.URL.Query().Get("job_id") var jobID int64 fmt.Sscanf(jobIDStr, "%d", &jobID) @@ -1080,7 +1080,7 @@ func TestRefineLoopStaysOnFailedFixChain(t *testing.T) { "responses": responses, }) - case r.URL.Path == "/api/respond" && r.Method == http.MethodPost: + case r.URL.Path == "/api/comment" && r.Method == http.MethodPost: var req struct { JobID int64 `json:"job_id"` Responder string `json:"responder"` diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index 76a06f7..514c7c6 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -351,9 +351,9 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al } // Get previous attempts for context - previousAttempts, err := client.GetResponsesForJob(currentFailedReview.JobID) + previousAttempts, err := client.GetCommentsForJob(currentFailedReview.JobID) if err != nil { - return fmt.Errorf("fetch previous responses: %w", err) + return fmt.Errorf("fetch previous comments: %w", err) } // Build address prompt @@ -434,7 +434,7 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al cleanupWorktree() fmt.Println("Agent made no changes") // Check how many times we've tried this review (only count our own attempts) - attempts, err := client.GetResponsesForJob(currentFailedReview.JobID) + attempts, err := client.GetCommentsForJob(currentFailedReview.JobID) if err != nil { return fmt.Errorf("fetch attempts: %w", err) } @@ -448,12 +448,12 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al // Tried 3 times (including this one), give up on this review // Do NOT mark as addressed - the review still needs attention fmt.Println("Giving up after multiple failed attempts (review remains unaddressed)") - client.AddResponse(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings (attempt 3, giving up)") + client.AddComment(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings (attempt 3, giving up)") skippedReviews[currentFailedReview.ID] = true // Don't re-select this run currentFailedReview = nil // Move on to next oldest failed commit } else { // Record attempt but don't mark addressed - might work on retry with different context - client.AddResponse(currentFailedReview.JobID, "roborev-refine", fmt.Sprintf("Agent could not determine how to address findings (attempt %d)", noChangeAttempts+1)) + client.AddComment(currentFailedReview.JobID, "roborev-refine", fmt.Sprintf("Agent could not determine how to address findings (attempt %d)", noChangeAttempts+1)) fmt.Printf("Attempt %d failed, will retry\n", noChangeAttempts+1) } continue @@ -476,7 +476,7 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al // Add response recording what was done (include full agent output for database) responseText := fmt.Sprintf("Created commit %s to address findings\n\n%s", newCommit[:7], output) - client.AddResponse(currentFailedReview.JobID, "roborev-refine", responseText) + client.AddComment(currentFailedReview.JobID, "roborev-refine", responseText) // Mark old review as addressed if err := client.MarkReviewAddressed(currentFailedReview.ID); err != nil { diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index 43939da..4975c59 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -23,7 +23,7 @@ type mockDaemonClient struct { // Track calls for assertions addressedReviews []int64 - addedResponses []addedResponse + addedComments []addedComment enqueuedReviews []enqueuedReview // Configurable errors for testing error paths @@ -31,10 +31,10 @@ type mockDaemonClient struct { getReviewBySHAErr error } -type addedResponse struct { +type addedComment struct { JobID int64 - Responder string - Response string + Commenter string + Comment string } type enqueuedReview struct { @@ -78,8 +78,8 @@ func (m *mockDaemonClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (m *mockDaemonClient) AddResponse(jobID int64, responder, response string) error { - m.addedResponses = append(m.addedResponses, addedResponse{jobID, responder, response}) +func (m *mockDaemonClient) AddComment(jobID int64, commenter, comment string) error { + m.addedComments = append(m.addedComments, addedComment{jobID, commenter, comment}) return nil } @@ -116,7 +116,7 @@ func (m *mockDaemonClient) FindPendingJobForRef(repoPath, gitRef string) (*stora return nil, nil } -func (m *mockDaemonClient) GetResponsesForJob(jobID int64) ([]storage.Response, error) { +func (m *mockDaemonClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) { return m.responses[jobID], nil } diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index e1033e0..9dc671c 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -65,6 +65,26 @@ var ( // fullSHAPattern matches a 40-character hex git SHA (not ranges or branch names) var fullSHAPattern = regexp.MustCompile(`(?i)^[0-9a-f]{40}$`) +// ansiEscapePattern matches ANSI escape sequences (colors, cursor movement, etc.) +// Handles CSI sequences (\x1b[...X) and OSC sequences terminated by BEL (\x07) or ST (\x1b\\) +var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;?]*[a-zA-Z]|\x1b\]([^\x07\x1b]|\x1b[^\\])*(\x07|\x1b\\)`) + +// sanitizeForDisplay strips ANSI escape sequences and control characters from text +// to prevent terminal injection when displaying untrusted content (e.g., commit messages). +func sanitizeForDisplay(s string) string { + // Strip ANSI escape sequences + s = ansiEscapePattern.ReplaceAllString(s, "") + // Strip control characters except newline (\n) and tab (\t) + var result strings.Builder + result.Grow(len(s)) + for _, r := range s { + if r == '\n' || r == '\t' || !unicode.IsControl(r) { + result.WriteRune(r) + } + } + return result.String() +} + type tuiView int const ( @@ -72,7 +92,9 @@ const ( tuiViewReview tuiViewPrompt tuiViewFilter - tuiViewRespond + tuiViewComment + tuiViewCommitMsg + tuiViewHelp ) // repoFilterItem represents a repo (or group of repos with same display name) in the filter modal @@ -115,11 +137,11 @@ type tuiModel struct { filterSelectedIdx int // Currently highlighted repo in filter list filterSearch string // Search/filter text typed by user - // Respond modal state - respondText string // The response text being typed - respondJobID int64 // Job ID we're responding to - respondCommit string // Short commit SHA for display - respondFromView tuiView // View to return to after respond modal closes + // Comment modal state + commentText string // The response text being typed + commentJobID int64 // Job ID we're responding to + commentCommit string // Short commit SHA for display + commentFromView tuiView // View to return to after comment modal closes // Active filter (applied to queue view) activeRepoFilter []string // Empty = show all, otherwise repo root_paths to filter by @@ -147,6 +169,15 @@ type tuiModel struct { // Daemon reconnection state consecutiveErrors int // Count of consecutive connection failures reconnecting bool // True if currently attempting reconnection + + // Commit message view state + commitMsgContent string // Formatted commit message(s) content + commitMsgScroll int // Scroll position in commit message view + commitMsgJobID int64 // Job ID for the commit message being viewed + commitMsgFromView tuiView // View to return to after closing commit message view + + // Help view state + helpFromView tuiView // View to return to after closing help } // pendingState tracks a pending addressed toggle with sequence number @@ -204,7 +235,7 @@ type tuiReposMsg struct { repos []repoFilterItem totalCount int } -type tuiRespondResultMsg struct { +type tuiCommentResultMsg struct { jobID int64 err error } @@ -212,6 +243,11 @@ type tuiClipboardResultMsg struct { err error view tuiView // The view where copy was triggered (for flash attribution) } +type tuiCommitMsgMsg struct { + jobID int64 + content string + err error +} type tuiReconnectMsg struct { newAddr string // New daemon address if found, empty if not found version string // Daemon version (to avoid sync call in Update) @@ -536,7 +572,7 @@ func (m tuiModel) fetchReview(jobID int64) tea.Cmd { // Fetch responses for this job var responses []storage.Response - respResp, err := m.client.Get(fmt.Sprintf("%s/api/responses?job_id=%d", m.serverAddr, jobID)) + respResp, err := m.client.Get(fmt.Sprintf("%s/api/comments?job_id=%d", m.serverAddr, jobID)) if err == nil { defer respResp.Body.Close() if respResp.StatusCode == http.StatusOK { @@ -551,7 +587,7 @@ func (m tuiModel) fetchReview(jobID int64) tea.Cmd { // Also fetch legacy responses by SHA for single commits (not ranges or dirty reviews) // and merge with job responses to preserve full history during migration if review.Job != nil && !strings.Contains(review.Job.GitRef, "..") && review.Job.GitRef != "dirty" { - shaResp, err := m.client.Get(fmt.Sprintf("%s/api/responses?sha=%s", m.serverAddr, review.Job.GitRef)) + shaResp, err := m.client.Get(fmt.Sprintf("%s/api/comments?sha=%s", m.serverAddr, review.Job.GitRef)) if err == nil { defer shaResp.Body.Close() if shaResp.StatusCode == http.StatusOK { @@ -695,6 +731,96 @@ func (m tuiModel) fetchReviewAndCopy(jobID int64, job *storage.ReviewJob) tea.Cm } } +// fetchCommitMsg fetches commit message(s) for a job. +// For single commits, returns the commit message. +// For ranges, returns all commit messages in the range. +// For dirty reviews or prompt jobs, returns an error. +func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { + jobID := job.ID + return func() tea.Msg { + // Handle prompt/run jobs first (GitRef == "prompt" indicates a run task, not a commit review) + // Check this before dirty to handle backward compatibility with older run jobs + if job.GitRef == "prompt" { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commit message for run tasks"), + } + } + + // Handle dirty reviews (uncommitted changes) + if job.DiffContent != nil || job.GitRef == "dirty" { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commit message for uncommitted changes"), + } + } + + // Handle missing GitRef (could be from incomplete job data or older versions) + if job.GitRef == "" { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no git reference available for this job"), + } + } + + // Check if this is a range (contains "..") + if strings.Contains(job.GitRef, "..") { + // Fetch all commits in range + commits, err := git.GetRangeCommits(job.RepoPath, job.GitRef) + if err != nil { + return tuiCommitMsgMsg{jobID: jobID, err: err} + } + if len(commits) == 0 { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commits in range %s", job.GitRef), + } + } + + // Fetch info for each commit + var content strings.Builder + content.WriteString(fmt.Sprintf("Commits in %s (%d commits):\n\n", job.GitRef, len(commits))) + + for i, sha := range commits { + info, err := git.GetCommitInfo(job.RepoPath, sha) + if err != nil { + content.WriteString(fmt.Sprintf("%d. %s: (error: %v)\n\n", i+1, sha[:7], err)) + continue + } + content.WriteString(fmt.Sprintf("%d. %s %s\n", i+1, info.SHA[:7], info.Subject)) + content.WriteString(fmt.Sprintf(" Author: %s | %s\n", info.Author, info.Timestamp.Format("2006-01-02 15:04"))) + if info.Body != "" { + // Indent body + bodyLines := strings.Split(info.Body, "\n") + for _, line := range bodyLines { + content.WriteString(" " + line + "\n") + } + } + content.WriteString("\n") + } + + return tuiCommitMsgMsg{jobID: jobID, content: sanitizeForDisplay(content.String())} + } + + // Single commit + info, err := git.GetCommitInfo(job.RepoPath, job.GitRef) + if err != nil { + return tuiCommitMsgMsg{jobID: jobID, err: err} + } + + var content strings.Builder + content.WriteString(fmt.Sprintf("Commit: %s\n", info.SHA)) + content.WriteString(fmt.Sprintf("Author: %s\n", info.Author)) + content.WriteString(fmt.Sprintf("Date: %s\n\n", info.Timestamp.Format("2006-01-02 15:04:05 -0700"))) + content.WriteString(info.Subject + "\n") + if info.Body != "" { + content.WriteString("\n" + info.Body + "\n") + } + + return tuiCommitMsgMsg{jobID: jobID, content: sanitizeForDisplay(content.String())} + } +} + func (m tuiModel) addressReview(reviewID, jobID int64, newState, oldState bool, seq uint64) tea.Cmd { return func() tea.Msg { reqBody, err := json.Marshal(map[string]interface{}{ @@ -1138,39 +1264,39 @@ func (m tuiModel) findLastVisibleJob() int { func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.KeyMsg: - // Handle respond view first (it captures most keys for typing) - if m.currentView == tuiViewRespond { + // Handle comment view first (it captures most keys for typing) + if m.currentView == tuiViewComment { switch msg.String() { case "ctrl+c": return m, tea.Quit case "esc": - m.currentView = m.respondFromView - m.respondText = "" - m.respondJobID = 0 + m.currentView = m.commentFromView + m.commentText = "" + m.commentJobID = 0 return m, nil case "enter": - if strings.TrimSpace(m.respondText) != "" { - text := m.respondText - jobID := m.respondJobID + if strings.TrimSpace(m.commentText) != "" { + text := m.commentText + jobID := m.commentJobID // Return to previous view but keep text until submit succeeds - m.currentView = m.respondFromView - return m, m.submitResponse(jobID, text) + m.currentView = m.commentFromView + return m, m.submitComment(jobID, text) } return m, nil case "backspace": - if len(m.respondText) > 0 { - runes := []rune(m.respondText) - m.respondText = string(runes[:len(runes)-1]) + if len(m.commentText) > 0 { + runes := []rune(m.commentText) + m.commentText = string(runes[:len(runes)-1]) } return m, nil default: // Handle typing (supports non-ASCII runes and newlines) if msg.String() == "shift+enter" || msg.String() == "ctrl+j" { - m.respondText += "\n" + m.commentText += "\n" } else if len(msg.Runes) > 0 { for _, r := range msg.Runes { if unicode.IsPrint(r) || r == '\n' || r == '\t' { - m.respondText += string(r) + m.commentText += string(r) } } } @@ -1250,6 +1376,16 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, nil } + if m.currentView == tuiViewCommitMsg { + m.currentView = m.commitMsgFromView + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, nil + } + if m.currentView == tuiViewHelp { + m.currentView = m.helpFromView + return m, nil + } return m, tea.Quit case "up": @@ -1259,6 +1395,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if prevIdx >= 0 { m.selectedIdx = prevIdx m.updateSelectedJobID() + } else { + m.flashMessage = "No newer review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewQueue } } else if m.currentView == tuiViewReview { if m.reviewScroll > 0 { @@ -1268,6 +1408,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if m.promptScroll > 0 { m.promptScroll-- } + } else if m.currentView == tuiViewCommitMsg { + if m.commitMsgScroll > 0 { + m.commitMsgScroll-- + } } case "k", "right": @@ -1296,6 +1440,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { Job: &job, } } + } else { + m.flashMessage = "No newer review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewReview } } else if m.currentView == tuiViewPrompt { if m.promptScroll > 0 { @@ -1314,11 +1462,18 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // At bottom with more jobs available - load them m.loadingMore = true return m, m.fetchMoreJobs() + } else if !m.hasMore || len(m.activeRepoFilter) > 0 { + // Truly at the bottom - no more to load or filter prevents auto-load + m.flashMessage = "No older review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewQueue } } else if m.currentView == tuiViewReview { m.reviewScroll++ } else if m.currentView == tuiViewPrompt { m.promptScroll++ + } else if m.currentView == tuiViewCommitMsg { + m.commitMsgScroll++ } case "j", "left": @@ -1351,6 +1506,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { Job: &job, } } + } else { + m.flashMessage = "No older review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewReview } } else if m.currentView == tuiViewPrompt { m.promptScroll++ @@ -1492,6 +1651,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // No job associated - track by review ID instead m.pendingReviewAddressed[m.currentReview.ID] = pendingState{newState: newState, seq: seq} } + // Don't update selectedIdx here - keep it pointing at the current (now hidden) job. + // The findNextViewableJob/findPrevViewableJob functions start searching from + // selectedIdx +/- 1, so left/right navigation will naturally find the correct + // adjacent visible jobs. Moving selectedIdx would cause navigation to skip a job. return m, m.addressReview(m.currentReview.ID, jobID, newState, oldState, seq) } else if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { job := &m.jobs[m.selectedIdx] @@ -1585,39 +1748,39 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } case "c": - // Open respond modal (from queue or review view) + // Open comment modal (from queue or review view) if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { job := m.jobs[m.selectedIdx] // Only allow responding to completed or failed reviews if job.Status == storage.JobStatusDone || job.Status == storage.JobStatusFailed { // Only clear text if opening for a different job (preserve for retry) - if m.respondJobID != job.ID { - m.respondText = "" + if m.commentJobID != job.ID { + m.commentText = "" } - m.respondJobID = job.ID - m.respondCommit = job.GitRef - if len(m.respondCommit) > 7 { - m.respondCommit = m.respondCommit[:7] + m.commentJobID = job.ID + m.commentCommit = job.GitRef + if len(m.commentCommit) > 7 { + m.commentCommit = m.commentCommit[:7] } - m.respondFromView = tuiViewQueue - m.currentView = tuiViewRespond + m.commentFromView = tuiViewQueue + m.currentView = tuiViewComment } return m, nil } else if m.currentView == tuiViewReview && m.currentReview != nil { // Only clear text if opening for a different job (preserve for retry) - if m.respondJobID != m.currentReview.JobID { - m.respondText = "" + if m.commentJobID != m.currentReview.JobID { + m.commentText = "" } - m.respondJobID = m.currentReview.JobID - m.respondCommit = "" + m.commentJobID = m.currentReview.JobID + m.commentCommit = "" if m.currentReview.Job != nil { - m.respondCommit = m.currentReview.Job.GitRef - if len(m.respondCommit) > 7 { - m.respondCommit = m.respondCommit[:7] + m.commentCommit = m.currentReview.Job.GitRef + if len(m.commentCommit) > 7 { + m.commentCommit = m.commentCommit[:7] } } - m.respondFromView = tuiViewReview - m.currentView = tuiViewRespond + m.commentFromView = tuiViewReview + m.currentView = tuiViewComment return m, nil } @@ -1652,6 +1815,36 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } } + case "m": + // Show commit message(s) for the selected job + if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { + job := m.jobs[m.selectedIdx] + m.commitMsgFromView = m.currentView + m.commitMsgJobID = job.ID + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, m.fetchCommitMsg(&job) + } else if m.currentView == tuiViewReview && m.currentReview != nil && m.currentReview.Job != nil { + job := m.currentReview.Job + m.commitMsgFromView = m.currentView + m.commitMsgJobID = job.ID + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, m.fetchCommitMsg(job) + } + + case "?": + // Toggle help modal + if m.currentView == tuiViewHelp { + m.currentView = m.helpFromView + return m, nil + } + if m.currentView == tuiViewQueue || m.currentView == tuiViewReview { + m.helpFromView = m.currentView + m.currentView = tuiViewHelp + return m, nil + } + case "esc": if m.currentView == tuiViewQueue && len(m.activeRepoFilter) > 0 { // Clear project filter first (keep hide-addressed if active) @@ -1704,6 +1897,14 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.currentView = tuiViewReview m.promptScroll = 0 } + } else if m.currentView == tuiViewCommitMsg { + // Go back to originating view + m.currentView = m.commitMsgFromView + m.commitMsgContent = "" + m.commitMsgScroll = 0 + } else if m.currentView == tuiViewHelp { + // Go back to previous view + m.currentView = m.helpFromView } } @@ -2001,16 +2202,16 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } } - case tuiRespondResultMsg: + case tuiCommentResultMsg: if msg.err != nil { m.err = msg.err - // Keep respondText and respondJobID so user can retry + // Keep commentText and commentJobID so user can retry } else { // Success - clear the response state only if still for the same job // (user may have started a new draft for a different job while this was in flight) - if m.respondJobID == msg.jobID { - m.respondText = "" - m.respondJobID = 0 + if m.commentJobID == msg.jobID { + m.commentText = "" + m.commentJobID = 0 } // Refresh the review to show the new response (if viewing a review) if m.currentView == tuiViewReview && m.currentReview != nil && m.currentReview.JobID == msg.jobID { @@ -2027,6 +2228,21 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.flashView = msg.view // Use view from trigger time, not current view } + case tuiCommitMsgMsg: + // Ignore stale messages (job changed while fetching) + if msg.jobID != m.commitMsgJobID { + return m, nil + } + if msg.err != nil { + m.flashMessage = msg.err.Error() + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = m.currentView + return m, nil + } + m.commitMsgContent = msg.content + m.commitMsgScroll = 0 + m.currentView = tuiViewCommitMsg + case tuiJobsErrMsg: m.err = msg.err m.loadingJobs = false // Clear loading state so refreshes can resume @@ -2105,12 +2321,18 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } func (m tuiModel) View() string { - if m.currentView == tuiViewRespond { + if m.currentView == tuiViewComment { return m.renderRespondView() } if m.currentView == tuiViewFilter { return m.renderFilterView() } + if m.currentView == tuiViewCommitMsg { + return m.renderCommitMsgView() + } + if m.currentView == tuiViewHelp { + return m.renderHelpView() + } if m.currentView == tuiViewPrompt && m.currentReview != nil { return m.renderPromptView() } @@ -2164,7 +2386,20 @@ func (m tuiModel) renderQueueView() string { m.status.CanceledJobs) } b.WriteString(tuiStatusStyle.Render(statusLine)) - b.WriteString("\x1b[K\n\x1b[K\n") // Clear status line and blank line + b.WriteString("\x1b[K\n") // Clear status line + + // Update notification on line 3 (above the table) + if m.updateAvailable != "" { + updateStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("226")).Bold(true) + var updateMsg string + if m.updateIsDevBuild { + updateMsg = fmt.Sprintf("Dev build - latest release: %s - run 'roborev update --force'", m.updateAvailable) + } else { + updateMsg = fmt.Sprintf("Update available: %s - run 'roborev update'", m.updateAvailable) + } + b.WriteString(updateStyle.Render(updateMsg)) + } + b.WriteString("\x1b[K\n") // Clear line 3 visibleJobList := m.getVisibleJobs() visibleSelectedIdx := m.getVisibleSelectedIdx() @@ -2282,25 +2517,16 @@ func (m tuiModel) renderQueueView() string { } b.WriteString("\x1b[K\n") // Clear scroll indicator line - // Status line: flash message (temporary) takes priority, then update notification, then blank + // Status line: flash message (temporary) if m.flashMessage != "" && time.Now().Before(m.flashExpiresAt) && m.flashView == tuiViewQueue { flashStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("46")) // Green b.WriteString(flashStyle.Render(m.flashMessage)) - } else if m.updateAvailable != "" { - updateStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("226")).Bold(true) - var updateMsg string - if m.updateIsDevBuild { - updateMsg = fmt.Sprintf("Dev build - latest release: %s - run 'roborev update --force'", m.updateAvailable) - } else { - updateMsg = fmt.Sprintf("Update available: %s - run 'roborev update'", m.updateAvailable) - } - b.WriteString(updateStyle.Render(updateMsg)) } b.WriteString("\x1b[K\n") // Clear to end of line // Help (two lines) - helpLine1 := "up/down/pgup/pgdn: navigate | enter: review | p: prompt | c: respond | y: copy | q: quit" - helpLine2 := "f: filter | h: hide | a: toggle addressed | x: cancel | r: rerun" + helpLine1 := "↑/↓: navigate | enter: review | y: copy | m: commit msg | q: quit | ?: help" + helpLine2 := "f: filter | h: hide addressed | a: toggle addressed | x: cancel" if len(m.activeRepoFilter) > 0 || m.hideAddressed { helpLine2 += " | esc: clear filters" } @@ -2571,7 +2797,7 @@ func (m tuiModel) renderReviewView() string { // Append responses if any if len(m.currentResponses) > 0 { - content.WriteString("\n\n--- Responses ---\n") + content.WriteString("\n\n--- Comments ---\n") for _, r := range m.currentResponses { timestamp := r.CreatedAt.Format("Jan 02 15:04") content.WriteString(fmt.Sprintf("\n[%s] %s:\n", timestamp, r.Responder)) @@ -2591,7 +2817,7 @@ func (m tuiModel) renderReviewView() string { } // Help text wraps at narrow terminals - const helpText = "up/down: scroll | j/k: prev/next | a: addressed | c: respond | y: copy | p: prompt | esc/q: back" + const helpText = "↑/↓: scroll | j/k: prev/next | a: addressed | y: copy | m: commit msg | ?: help | esc/q: back" helpLines := 1 if m.width > 0 && m.width < len(helpText) { helpLines = (len(helpText) + m.width - 1) / m.width @@ -2838,14 +3064,14 @@ func (m tuiModel) renderFilterView() string { func (m tuiModel) renderRespondView() string { var b strings.Builder - title := "Respond to Review" - if m.respondCommit != "" { - title = fmt.Sprintf("Respond to Review (%s)", m.respondCommit) + title := "Add Comment" + if m.commentCommit != "" { + title = fmt.Sprintf("Add Comment (%s)", m.commentCommit) } b.WriteString(tuiTitleStyle.Render(title)) b.WriteString("\x1b[K\n\x1b[K\n") // Clear title and blank line - b.WriteString(tuiStatusStyle.Render("Enter your response (e.g., \"This is a known issue, can be ignored\")")) + b.WriteString(tuiStatusStyle.Render("Enter your comment (e.g., \"This is a known issue, can be ignored\")")) b.WriteString("\x1b[K\n\x1b[K\n") // Simple text box with border @@ -2863,14 +3089,14 @@ func (m tuiModel) renderRespondView() string { maxTextLines = 3 } - if m.respondText == "" { + if m.commentText == "" { // Show placeholder (styled, but we pad manually to avoid ANSI issues) - placeholder := "Type your response..." + placeholder := "Type your comment..." padded := placeholder + strings.Repeat(" ", boxWidth-2-len(placeholder)) b.WriteString("| " + tuiStatusStyle.Render(padded) + " |\x1b[K\n") textLinesWritten++ } else { - lines := strings.Split(m.respondText, "\n") + lines := strings.Split(m.commentText, "\n") for _, line := range lines { if textLinesWritten >= maxTextLines { break @@ -2911,40 +3137,209 @@ func (m tuiModel) renderRespondView() string { return b.String() } -func (m tuiModel) submitResponse(jobID int64, text string) tea.Cmd { +func (m tuiModel) submitComment(jobID int64, text string) tea.Cmd { return func() tea.Msg { - responder := os.Getenv("USER") - if responder == "" { - responder = "anonymous" + commenter := os.Getenv("USER") + if commenter == "" { + commenter = "anonymous" } payload := map[string]interface{}{ "job_id": jobID, - "responder": responder, - "response": strings.TrimSpace(text), + "commenter": commenter, + "comment": strings.TrimSpace(text), } body, err := json.Marshal(payload) if err != nil { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("marshal request: %w", err)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("marshal request: %w", err)} } resp, err := m.client.Post( - fmt.Sprintf("%s/api/respond", m.serverAddr), + fmt.Sprintf("%s/api/comment", m.serverAddr), "application/json", bytes.NewReader(body), ) if err != nil { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("submit response: %w", err)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit comment: %w", err)} } defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("submit response: HTTP %d", resp.StatusCode)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit comment: HTTP %d", resp.StatusCode)} + } + + return tuiCommentResultMsg{jobID: jobID, err: nil} + } +} + +func (m tuiModel) renderCommitMsgView() string { + var b strings.Builder + + b.WriteString(tuiTitleStyle.Render("Commit Message")) + b.WriteString("\x1b[K\n") // Clear to end of line + + if m.commitMsgContent == "" { + b.WriteString(tuiStatusStyle.Render("Loading commit message...")) + b.WriteString("\x1b[K\n") + // Pad to fill terminal + linesWritten := 2 + for linesWritten < m.height-1 { + b.WriteString("\x1b[K\n") + linesWritten++ } + b.WriteString(tuiHelpStyle.Render("esc/q: back")) + b.WriteString("\x1b[K") + b.WriteString("\x1b[J") + return b.String() + } + + // Wrap text to terminal width minus padding + wrapWidth := max(20, min(m.width-4, 200)) + lines := wrapText(m.commitMsgContent, wrapWidth) + + // Reserve: title(1) + scroll indicator(1) + help(1) + margin(1) + visibleLines := m.height - 4 + if visibleLines < 1 { + visibleLines = 1 + } + + // Clamp scroll position to valid range + maxScroll := len(lines) - visibleLines + if maxScroll < 0 { + maxScroll = 0 + } + start := m.commitMsgScroll + if start > maxScroll { + start = maxScroll + } + if start < 0 { + start = 0 + } + end := min(start+visibleLines, len(lines)) + + linesWritten := 0 + for i := start; i < end; i++ { + b.WriteString(lines[i]) + b.WriteString("\x1b[K\n") // Clear to end of line before newline + linesWritten++ + } - return tuiRespondResultMsg{jobID: jobID, err: nil} + // Pad with clear-to-end-of-line sequences to prevent ghost text + for linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ } + + // Scroll indicator + if len(lines) > visibleLines { + scrollInfo := fmt.Sprintf("[%d-%d of %d lines]", start+1, end, len(lines)) + b.WriteString(tuiStatusStyle.Render(scrollInfo)) + } + b.WriteString("\x1b[K\n") // Clear scroll indicator line + + b.WriteString(tuiHelpStyle.Render("up/down: scroll | esc/q: back")) + b.WriteString("\x1b[K") // Clear help line + b.WriteString("\x1b[J") // Clear to end of screen to prevent artifacts + + return b.String() +} + +func (m tuiModel) renderHelpView() string { + var b strings.Builder + + b.WriteString(tuiTitleStyle.Render("Keyboard Shortcuts")) + b.WriteString("\x1b[K\n\x1b[K\n") + + // Define shortcuts in groups + shortcuts := []struct { + group string + keys []struct{ key, desc string } + }{ + { + group: "Navigation", + keys: []struct{ key, desc string }{ + {"↑/k", "Move up / previous review"}, + {"↓/j", "Move down / next review"}, + {"PgUp/PgDn", "Scroll by page"}, + {"enter", "View review details"}, + {"esc", "Go back / clear filter"}, + {"q", "Quit"}, + }, + }, + { + group: "Actions", + keys: []struct{ key, desc string }{ + {"a", "Mark as addressed"}, + {"c", "Add comment"}, + {"x", "Cancel job"}, + {"r", "Re-run job"}, + {"y", "Copy review to clipboard"}, + {"m", "Show commit message(s)"}, + }, + }, + { + group: "Filtering", + keys: []struct{ key, desc string }{ + {"f", "Filter by repository"}, + {"h", "Toggle hide addressed"}, + }, + }, + { + group: "Review View", + keys: []struct{ key, desc string }{ + {"p", "View prompt"}, + {"↑/↓", "Scroll content"}, + {"←/→", "Previous / next review"}, + }, + }, + } + + // Calculate visible area + // Reserve: title(1) + blank(1) + padding + help(1) + reservedLines := 3 + visibleLines := m.height - reservedLines + if visibleLines < 5 { + visibleLines = 5 + } + + linesWritten := 0 + for _, g := range shortcuts { + if linesWritten >= visibleLines-2 { + break + } + // Group header + b.WriteString(tuiSelectedStyle.Render(g.group)) + b.WriteString("\x1b[K\n") + linesWritten++ + + for _, k := range g.keys { + if linesWritten >= visibleLines { + break + } + line := fmt.Sprintf(" %-12s %s", k.key, k.desc) + b.WriteString(line) + b.WriteString("\x1b[K\n") + linesWritten++ + } + // Blank line between groups + if linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ + } + } + + // Pad remaining space + for linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ + } + + b.WriteString(tuiHelpStyle.Render("esc/q/?: close")) + b.WriteString("\x1b[K") + b.WriteString("\x1b[J") // Clear to end of screen + + return b.String() } func tuiCmd() *cobra.Command { diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index cce9e75..5c2b4ca 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -208,8 +208,9 @@ func TestTUIToggleAddressedNoReview(t *testing.T) { func TestTUIAddressFromReviewViewWithHideAddressed(t *testing.T) { // When hideAddressed is on and user marks a review as addressed from review view, - // selection should move to next visible job when returning to queue (on escape), - // not immediately on 'a' press (so j/k navigation still works in review view) + // selectedIdx should NOT change immediately. The findNextViewableJob/findPrevViewableJob + // functions start from selectedIdx +/- 1, so left/right navigation will naturally + // find the correct adjacent visible jobs. Selection only moves on escape. m := newTuiModel("http://localhost") m.currentView = tuiViewReview m.hideAddressed = true @@ -228,26 +229,29 @@ func TestTUIAddressFromReviewViewWithHideAddressed(t *testing.T) { Job: &m.jobs[1], } - // Press 'a' to mark as addressed - selection should NOT move yet + // Press 'a' to mark as addressed - selection stays at current position updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'a'}}) m2 := updated.(tuiModel) - // Selection stays at index 1 so j/k navigation still works relative to current review + // Selection stays at index 1 so left/right navigation works correctly from current position if m2.selectedIdx != 1 { t.Errorf("After 'a': expected selectedIdx=1 (unchanged), got %d", m2.selectedIdx) } if m2.selectedJobID != 2 { t.Errorf("After 'a': expected selectedJobID=2 (unchanged), got %d", m2.selectedJobID) } + // Still viewing the current review + if m2.currentView != tuiViewReview { + t.Errorf("After 'a': expected view=review (unchanged), got %d", m2.currentView) + } - // Press escape to return to queue - NOW selection should move + // Press escape to return to queue - NOW selection moves via normalizeSelectionIfHidden updated2, _ := m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) m3 := updated2.(tuiModel) - // Selection should have moved to next visible job (job 3, index 2) - // since job 2 is now addressed and hidden + // Selection moved to next visible job (job 3, index 2) if m3.selectedIdx != 2 { - t.Errorf("After escape: expected selectedIdx=2 (next visible job), got %d", m3.selectedIdx) + t.Errorf("After escape: expected selectedIdx=2, got %d", m3.selectedIdx) } if m3.selectedJobID != 3 { t.Errorf("After escape: expected selectedJobID=3, got %d", m3.selectedJobID) @@ -2379,7 +2383,7 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { m.currentView = tuiViewReview m.currentReview = &storage.Review{ID: 10, Job: &storage.ReviewJob{ID: 1}} - // Press 'k' at first viewable job - should be no-op + // Press 'k' (right) at first viewable job - should show flash message updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'k'}}) m2 := updated.(tuiModel) @@ -2392,13 +2396,22 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if cmd != nil { t.Error("Expected no command at boundary") } + if m2.flashMessage != "No newer review" { + t.Errorf("Expected flash message 'No newer review', got %q", m2.flashMessage) + } + if m2.flashView != tuiViewReview { + t.Errorf("Expected flashView to be tuiViewReview, got %d", m2.flashView) + } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } // Now at last viewable job m.selectedIdx = 2 m.selectedJobID = 3 m.currentReview = &storage.Review{ID: 30, Job: &storage.ReviewJob{ID: 3}} - // Press 'j' at last viewable job - should be no-op + // Press 'j' (left) at last viewable job - should show flash message updated, cmd = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'j'}}) m3 := updated.(tuiModel) @@ -2411,6 +2424,15 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if cmd != nil { t.Error("Expected no command at boundary") } + if m3.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review', got %q", m3.flashMessage) + } + if m3.flashView != tuiViewReview { + t.Errorf("Expected flashView to be tuiViewReview, got %d", m3.flashView) + } + if m3.flashExpiresAt.IsZero() || m3.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m3.flashExpiresAt) + } } func TestTUIReviewNavigationFailedJobInline(t *testing.T) { @@ -2612,6 +2634,90 @@ func TestTUIQueueViewArrowsMatchUpDown(t *testing.T) { } } +func TestTUIQueueNavigationBoundaries(t *testing.T) { + // Test flash messages when navigating at queue boundaries + m := newTuiModel("http://localhost") + + m.jobs = []storage.ReviewJob{ + {ID: 1, Status: storage.JobStatusDone}, + {ID: 2, Status: storage.JobStatusDone}, + {ID: 3, Status: storage.JobStatusDone}, + } + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.hasMore = false // No more jobs to load + + // Press 'up' at top of queue - should show flash message + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyUp}) + m2 := updated.(tuiModel) + + if m2.selectedIdx != 0 { + t.Errorf("Expected selectedIdx to remain 0 at top, got %d", m2.selectedIdx) + } + if m2.flashMessage != "No newer review" { + t.Errorf("Expected flash message 'No newer review', got %q", m2.flashMessage) + } + if m2.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m2.flashView) + } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } + + // Now at bottom of queue + m.selectedIdx = 2 + m.selectedJobID = 3 + m.flashMessage = "" // Clear + + // Press 'down' at bottom of queue - should show flash message + updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m3 := updated.(tuiModel) + + if m3.selectedIdx != 2 { + t.Errorf("Expected selectedIdx to remain 2 at bottom, got %d", m3.selectedIdx) + } + if m3.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review', got %q", m3.flashMessage) + } + if m3.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m3.flashView) + } + if m3.flashExpiresAt.IsZero() || m3.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m3.flashExpiresAt) + } +} + +func TestTUIQueueNavigationBoundariesWithFilter(t *testing.T) { + // Test flash messages at bottom when filter is active (prevents auto-load) + m := newTuiModel("http://localhost") + + m.jobs = []storage.ReviewJob{ + {ID: 1, Status: storage.JobStatusDone, RepoPath: "/repo1"}, + {ID: 2, Status: storage.JobStatusDone, RepoPath: "/repo2"}, + } + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.hasMore = true // More jobs available but... + m.activeRepoFilter = []string{"/repo1"} // Filter is active, prevents auto-load + + // Press 'down' - only job 1 matches filter, so we're at bottom + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m2 := updated.(tuiModel) + + // Should show flash since filter prevents loading more + if m2.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review' with filter active, got %q", m2.flashMessage) + } + if m2.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m2.flashView) + } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } +} + func TestTUIJobsRefreshDuringReviewNavigation(t *testing.T) { // Test that jobs refresh during review navigation doesn't reset selection // This tests the race condition fix: user navigates to job 3, but jobs refresh @@ -4035,9 +4141,9 @@ func TestTUIRenderFailedJobNoBranchShown(t *testing.T) { func TestTUIVisibleLinesCalculationNoVerdict(t *testing.T) { // Test that visibleLines = height - 3 when no verdict (title + status + help) - // Help text is 87 chars, so use width >= 87 to avoid wrapping + // Help text is ~106 chars, so use width >= 110 to avoid wrapping m := newTuiModel("http://localhost") - m.width = 100 + m.width = 120 m.height = 10 // Small height to test calculation m.currentView = tuiViewReview // Create 20 lines of content to ensure scrolling @@ -4077,10 +4183,10 @@ func TestTUIVisibleLinesCalculationNoVerdict(t *testing.T) { func TestTUIVisibleLinesCalculationWithVerdict(t *testing.T) { // Test that visibleLines = height - 4 when verdict present (title + verdict + status + help) - // Help text is 87 chars, so use width >= 87 to avoid wrapping + // Help text is ~106 chars, so use width >= 110 to avoid wrapping verdictPass := "P" m := newTuiModel("http://localhost") - m.width = 100 + m.width = 120 m.height = 10 // Small height to test calculation m.currentView = tuiViewReview // Create 20 lines of content to ensure scrolling @@ -4119,7 +4225,7 @@ func TestTUIVisibleLinesCalculationWithVerdict(t *testing.T) { func TestTUIVisibleLinesCalculationNarrowTerminal(t *testing.T) { // Test that visibleLines accounts for help text wrapping at narrow terminals - // Help text is 87 chars, at width=50 it wraps to 2 lines: ceil(87/50) = 2 + // Help text is ~91 chars, at width=50 it wraps to 2 lines: ceil(91/50) = 2 m := newTuiModel("http://localhost") m.width = 50 m.height = 10 @@ -4160,7 +4266,7 @@ func TestTUIVisibleLinesCalculationNarrowTerminal(t *testing.T) { func TestTUIVisibleLinesCalculationNarrowTerminalWithVerdict(t *testing.T) { // Test narrow terminal with verdict - validates extra header line branch - // Help text is 87 chars, at width=50 it wraps to 2 lines: ceil(87/50) = 2 + // Help text is ~91 chars, at width=50 it wraps to 2 lines: ceil(91/50) = 2 verdictFail := "F" m := newTuiModel("http://localhost") m.width = 50 @@ -4234,7 +4340,7 @@ func TestTUIVisibleLinesCalculationLongTitleWraps(t *testing.T) { // "Review #1 very-long-repository-name-here abc1234..def5678 (claude-code) on feature/very-long-branch-name [ADDRESSED]" // = 7 + 3 + 31 + 17 + 14 + 34 + 12 = ~118 chars // At width=50: ceil(118/50) = 3 title lines - // Help at width=50: ceil(87/50) = 2 help lines + // Help at width=50: ceil(91/50) = 2 help lines // Non-content: title (3) + status line (1) + help (2) = 6 // visibleLines = 12 - 6 = 6 contentCount := 0 @@ -4289,7 +4395,7 @@ func TestTUIFetchReviewFallbackSHAResponses(t *testing.T) { return } - if r.URL.Path == "/api/responses" { + if r.URL.Path == "/api/comments" { jobID := r.URL.Query().Get("job_id") sha := r.URL.Query().Get("sha") @@ -4375,7 +4481,7 @@ func TestTUIFetchReviewNoFallbackForRangeReview(t *testing.T) { return } - if r.URL.Path == "/api/responses" { + if r.URL.Path == "/api/comments" { // Return empty responses for job_id json.NewEncoder(w).Encode(map[string]interface{}{ "responses": []storage.Response{}, @@ -5148,28 +5254,28 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.currentView != tuiViewRespond { - t.Fatalf("Expected tuiViewRespond, got %v", m.currentView) + if m.currentView != tuiViewComment { + t.Fatalf("Expected tuiViewComment, got %v", m.currentView) } - if m.respondJobID != 1 { - t.Fatalf("Expected respondJobID=1, got %d", m.respondJobID) + if m.commentJobID != 1 { + t.Fatalf("Expected commentJobID=1, got %d", m.commentJobID) } // 2. Type some text - m.respondText = "My draft response" + m.commentText = "My draft response" // 3. Simulate failed submission - press enter then receive error - m.currentView = m.respondFromView // Simulate what happens on enter - errMsg := tuiRespondResultMsg{jobID: 1, err: fmt.Errorf("network error")} + m.currentView = m.commentFromView // Simulate what happens on enter + errMsg := tuiCommentResultMsg{jobID: 1, err: fmt.Errorf("network error")} updated, _ = m.Update(errMsg) m = updated.(tuiModel) // Text should be preserved after error - if m.respondText != "My draft response" { - t.Errorf("Expected text preserved after error, got %q", m.respondText) + if m.commentText != "My draft response" { + t.Errorf("Expected text preserved after error, got %q", m.commentText) } - if m.respondJobID != 1 { - t.Errorf("Expected respondJobID preserved after error, got %d", m.respondJobID) + if m.commentJobID != 1 { + t.Errorf("Expected commentJobID preserved after error, got %d", m.commentJobID) } // 4. Re-open respond for Job 1 (Retry) - text should still be there @@ -5178,8 +5284,8 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.respondText != "My draft response" { - t.Errorf("Expected text preserved on retry for same job, got %q", m.respondText) + if m.commentText != "My draft response" { + t.Errorf("Expected text preserved on retry for same job, got %q", m.commentText) } // 5. Go back to queue and switch to Job 2 - text should be cleared @@ -5189,11 +5295,11 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.respondText != "" { - t.Errorf("Expected text cleared for different job, got %q", m.respondText) + if m.commentText != "" { + t.Errorf("Expected text cleared for different job, got %q", m.commentText) } - if m.respondJobID != 2 { - t.Errorf("Expected respondJobID=2, got %d", m.respondJobID) + if m.commentJobID != 2 { + t.Errorf("Expected commentJobID=2, got %d", m.commentJobID) } } @@ -5205,32 +5311,32 @@ func TestTUIRespondSuccessClearsOnlyMatchingJob(t *testing.T) { } // User submitted response for job 1, then started drafting for job 2 - m.respondJobID = 2 - m.respondText = "New draft for job 2" + m.commentJobID = 2 + m.commentText = "New draft for job 2" // Success message arrives for job 1 (the old submission) - successMsg := tuiRespondResultMsg{jobID: 1, err: nil} + successMsg := tuiCommentResultMsg{jobID: 1, err: nil} updated, _ := m.Update(successMsg) m = updated.(tuiModel) // Draft for job 2 should NOT be cleared - if m.respondText != "New draft for job 2" { - t.Errorf("Expected draft preserved for different job, got %q", m.respondText) + if m.commentText != "New draft for job 2" { + t.Errorf("Expected draft preserved for different job, got %q", m.commentText) } - if m.respondJobID != 2 { - t.Errorf("Expected respondJobID=2 preserved, got %d", m.respondJobID) + if m.commentJobID != 2 { + t.Errorf("Expected commentJobID=2 preserved, got %d", m.commentJobID) } // Now success for job 2 should clear - successMsg = tuiRespondResultMsg{jobID: 2, err: nil} + successMsg = tuiCommentResultMsg{jobID: 2, err: nil} updated, _ = m.Update(successMsg) m = updated.(tuiModel) - if m.respondText != "" { - t.Errorf("Expected text cleared for matching job, got %q", m.respondText) + if m.commentText != "" { + t.Errorf("Expected text cleared for matching job, got %q", m.commentText) } - if m.respondJobID != 0 { - t.Errorf("Expected respondJobID=0 after success, got %d", m.respondJobID) + if m.commentJobID != 0 { + t.Errorf("Expected commentJobID=0 after success, got %d", m.commentJobID) } } @@ -5268,42 +5374,42 @@ func TestTUIFilterBackspaceMultiByte(t *testing.T) { func TestTUIRespondBackspaceMultiByte(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 // Type text with multi-byte characters updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("Hello 世界")}) m = updated.(tuiModel) - if m.respondText != "Hello 世界" { - t.Errorf("Expected respondText='Hello 世界', got %q", m.respondText) + if m.commentText != "Hello 世界" { + t.Errorf("Expected commentText='Hello 世界', got %q", m.commentText) } // Backspace should remove '界' (one character), not corrupt it updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyBackspace}) m = updated.(tuiModel) - if m.respondText != "Hello 世" { - t.Errorf("Expected respondText='Hello 世' after backspace, got %q", m.respondText) + if m.commentText != "Hello 世" { + t.Errorf("Expected commentText='Hello 世' after backspace, got %q", m.commentText) } // Backspace should remove '世' updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyBackspace}) m = updated.(tuiModel) - if m.respondText != "Hello " { - t.Errorf("Expected respondText='Hello ' after second backspace, got %q", m.respondText) + if m.commentText != "Hello " { + t.Errorf("Expected commentText='Hello ' after second backspace, got %q", m.commentText) } } func TestTUIRespondViewTruncationMultiByte(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 m.width = 30 m.height = 20 // Set text with multi-byte characters that would be truncated // The box has boxWidth-2 available space for text - m.respondText = "あいうえおかきくけこさしすせそ" // 15 Japanese characters (30 cells wide) + m.commentText = "あいうえおかきくけこさしすせそ" // 15 Japanese characters (30 cells wide) // Render should not panic or corrupt characters output := m.renderRespondView() @@ -5342,13 +5448,13 @@ func TestTUIRespondViewTruncationMultiByte(t *testing.T) { func TestTUIRespondViewTabExpansion(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 m.width = 40 m.height = 20 // Set text with tabs - m.respondText = "a\tb\tc" + m.commentText = "a\tb\tc" output := m.renderRespondView() plainOutput := stripANSI(output) @@ -5583,6 +5689,67 @@ func TestTUIFlashMessageNotShownInDifferentView(t *testing.T) { } } +func TestTUIUpdateNotificationInQueueView(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + + output := m.renderQueueView() + if !strings.Contains(output, "Update available: 1.2.3") { + t.Error("Expected update notification in queue view") + } + if !strings.Contains(output, "run 'roborev update'") { + t.Error("Expected update instructions in queue view") + } + + // Verify update notification appears on line 3 (index 2) - above the table + // Layout: line 0 = title, line 1 = status, line 2 = update notification + lines := strings.Split(output, "\n") + if len(lines) < 3 { + t.Fatalf("Expected at least 3 lines, got %d", len(lines)) + } + // Line 2 (third line) should contain the update notification + if !strings.Contains(lines[2], "Update available") { + t.Errorf("Expected update notification on line 3 (index 2), got: %q", lines[2]) + } +} + +func TestTUIUpdateNotificationDevBuild(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + m.updateIsDevBuild = true + + output := m.renderQueueView() + if !strings.Contains(output, "Dev build") { + t.Error("Expected 'Dev build' in notification for dev builds") + } + if !strings.Contains(output, "roborev update --force") { + t.Error("Expected --force flag in update instructions for dev builds") + } +} + +func TestTUIUpdateNotificationNotInReviewView(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewReview + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + m.currentReview = &storage.Review{ + ID: 1, + Output: "Test review content", + } + + output := m.renderReviewView() + if strings.Contains(output, "Update available") { + t.Error("Update notification should not appear in review view") + } +} + func TestTUIFetchReviewAndCopySuccess(t *testing.T) { // Save original clipboard writer and restore after test originalClipboard := clipboardWriter @@ -6285,3 +6452,293 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { } }) } + +func TestTUICommitMsgViewNavigationFromQueue(t *testing.T) { + // Test that pressing escape in commit message view returns to the originating view (queue) + m := newTuiModel("http://localhost") + m.jobs = []storage.ReviewJob{{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone}} + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.commitMsgJobID = 1 // Set to match incoming message (normally set by 'm' key handler) + m.commitMsgFromView = tuiViewQueue // Track where we came from + + // Simulate receiving commit message content (sets view to CommitMsg) + updated, _ := m.Update(tuiCommitMsgMsg{jobID: 1, content: "test message"}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewCommitMsg { + t.Errorf("Expected tuiViewCommitMsg, got %d", m2.currentView) + } + + // Press escape to go back + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewQueue { + t.Errorf("Expected to return to tuiViewQueue, got %d", m3.currentView) + } + if m3.commitMsgContent != "" { + t.Error("Expected commitMsgContent to be cleared") + } +} + +func TestTUICommitMsgViewNavigationFromReview(t *testing.T) { + // Test that pressing escape in commit message view returns to the originating view (review) + m := newTuiModel("http://localhost") + job := &storage.ReviewJob{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone} + m.jobs = []storage.ReviewJob{*job} + m.currentReview = &storage.Review{ID: 1, JobID: 1, Job: job} + m.currentView = tuiViewReview + m.commitMsgFromView = tuiViewReview + m.commitMsgContent = "test message" + m.currentView = tuiViewCommitMsg + + // Press escape to go back + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview, got %d", m2.currentView) + } +} + +func TestTUICommitMsgViewNavigationWithQ(t *testing.T) { + // Test that pressing 'q' in commit message view also returns to originating view + m := newTuiModel("http://localhost") + m.currentView = tuiViewCommitMsg + m.commitMsgFromView = tuiViewReview + m.commitMsgContent = "test message" + + // Press 'q' to go back + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview after 'q', got %d", m2.currentView) + } +} + +func TestFetchCommitMsgJobTypeDetection(t *testing.T) { + // Test that fetchCommitMsg correctly identifies job types and returns appropriate errors + // This is critical: Prompt field is populated for ALL jobs (stores review prompt), + // so we must check GitRef == "prompt" to identify run tasks, not Prompt != "" + + m := newTuiModel("http://localhost") + + tests := []struct { + name string + job storage.ReviewJob + expectError string // empty means no early error (will try git lookup) + }{ + { + name: "regular commit with Prompt populated should not error early", + job: storage.ReviewJob{ + ID: 1, + GitRef: "abc123def456", // valid commit SHA + Prompt: "You are a code reviewer...", // review prompt is stored for all jobs + }, + expectError: "", // should attempt git lookup, not return "run tasks" error + }, + { + name: "run task (GitRef=prompt) should error", + job: storage.ReviewJob{ + ID: 2, + GitRef: "prompt", + Prompt: "Explain this codebase", + }, + expectError: "no commit message for run tasks", + }, + { + name: "dirty job (GitRef=dirty) should error", + job: storage.ReviewJob{ + ID: 3, + GitRef: "dirty", + }, + expectError: "no commit message for uncommitted changes", + }, + { + name: "dirty job with DiffContent should error", + job: storage.ReviewJob{ + ID: 4, + GitRef: "some-ref", + DiffContent: func() *string { s := "diff content"; return &s }(), + }, + expectError: "no commit message for uncommitted changes", + }, + { + name: "empty GitRef should error with missing ref message", + job: storage.ReviewJob{ + ID: 5, + GitRef: "", + }, + expectError: "no git reference available for this job", + }, + { + name: "empty GitRef with Prompt (backward compat run job) should error with missing ref", + job: storage.ReviewJob{ + ID: 6, + GitRef: "", + Prompt: "Explain this codebase", // older run job without GitRef=prompt + }, + expectError: "no git reference available for this job", + }, + { + name: "dirty job with nil DiffContent but GitRef=dirty should error", + job: storage.ReviewJob{ + ID: 7, + GitRef: "dirty", + DiffContent: nil, + }, + expectError: "no commit message for uncommitted changes", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := m.fetchCommitMsg(&tt.job) + msg := cmd() + + result, ok := msg.(tuiCommitMsgMsg) + if !ok { + t.Fatalf("Expected tuiCommitMsgMsg, got %T", msg) + } + + if tt.expectError != "" { + if result.err == nil { + t.Errorf("Expected error %q, got nil", tt.expectError) + } else if result.err.Error() != tt.expectError { + t.Errorf("Expected error %q, got %q", tt.expectError, result.err.Error()) + } + } else { + // For valid commits, we expect a git error (repo doesn't exist in test) + // but NOT the "run tasks" or "uncommitted changes" error + if result.err != nil { + errMsg := result.err.Error() + if errMsg == "no commit message for run tasks" { + t.Errorf("Regular commit with Prompt should not be detected as run task") + } + if errMsg == "no commit message for uncommitted changes" { + t.Errorf("Regular commit should not be detected as uncommitted changes") + } + // Other errors (like git errors) are expected in test environment + } + } + }) + } +} + +func TestTUIHelpViewToggleFromQueue(t *testing.T) { + // Test that '?' opens help from queue and pressing '?' again returns to queue + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + + // Press '?' to open help + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewHelp { + t.Errorf("Expected tuiViewHelp, got %d", m2.currentView) + } + if m2.helpFromView != tuiViewQueue { + t.Errorf("Expected helpFromView to be tuiViewQueue, got %d", m2.helpFromView) + } + + // Press '?' again to close help + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewQueue { + t.Errorf("Expected to return to tuiViewQueue, got %d", m3.currentView) + } +} + +func TestTUIHelpViewToggleFromReview(t *testing.T) { + // Test that '?' opens help from review and escape returns to review + m := newTuiModel("http://localhost") + job := &storage.ReviewJob{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone} + m.currentReview = &storage.Review{ID: 1, JobID: 1, Job: job} + m.currentView = tuiViewReview + + // Press '?' to open help + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewHelp { + t.Errorf("Expected tuiViewHelp, got %d", m2.currentView) + } + if m2.helpFromView != tuiViewReview { + t.Errorf("Expected helpFromView to be tuiViewReview, got %d", m2.helpFromView) + } + + // Press escape to close help + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview, got %d", m3.currentView) + } +} + +func TestSanitizeForDisplay(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "plain text unchanged", + input: "Hello, world!", + expected: "Hello, world!", + }, + { + name: "preserves newlines and tabs", + input: "Line1\n\tIndented", + expected: "Line1\n\tIndented", + }, + { + name: "strips ANSI color codes", + input: "\x1b[31mred text\x1b[0m", + expected: "red text", + }, + { + name: "strips cursor movement", + input: "\x1b[2Jhello\x1b[H", + expected: "hello", + }, + { + name: "strips OSC sequences (title set with BEL)", + input: "\x1b]0;Evil Title\x07normal text", + expected: "normal text", + }, + { + name: "strips OSC sequences (title set with ST)", + input: "\x1b]0;Evil Title\x1b\\normal text", + expected: "normal text", + }, + { + name: "strips control characters", + input: "hello\x00world\x07\x08test", + expected: "helloworldtest", + }, + { + name: "handles complex escape sequence", + input: "\x1b[1;32mBold Green\x1b[0m and \x1b[4munderline\x1b[24m", + expected: "Bold Green and underline", + }, + { + name: "empty string unchanged", + input: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeForDisplay(tt.input) + if got != tt.expected { + t.Errorf("sanitizeForDisplay(%q) = %q, want %q", tt.input, got, tt.expected) + } + }) + } +} diff --git a/e2e_test.go b/e2e_test.go index f22a371..fcc0dbc 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -157,22 +157,22 @@ func TestDatabaseIntegration(t *testing.T) { t.Errorf("Review output doesn't contain expected text: %s", review.Output) } - // Add a response - resp, err := db.AddResponse(commit.ID, "human-reviewer", "Agreed, LGTM!") + // Add a comment + resp, err := db.AddComment(commit.ID, "human-reviewer", "Agreed, LGTM!") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } if resp.Response != "Agreed, LGTM!" { - t.Errorf("Response not saved correctly") + t.Errorf("Comment not saved correctly") } - // Verify response can be fetched - responses, err := db.GetResponsesForCommitSHA("abc123") + // Verify comment can be fetched + comments, err := db.GetCommentsForCommitSHA("abc123") if err != nil { - t.Fatalf("GetResponsesForCommitSHA failed: %v", err) + t.Fatalf("GetCommentsForCommitSHA failed: %v", err) } - if len(responses) != 1 { - t.Errorf("Expected 1 response, got %d", len(responses)) + if len(comments) != 1 { + t.Errorf("Expected 1 comment, got %d", len(comments)) } } diff --git a/internal/daemon/client.go b/internal/daemon/client.go index aa7318a..8a93239 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -26,8 +26,8 @@ type Client interface { // MarkReviewAddressed marks a review as addressed MarkReviewAddressed(reviewID int64) error - // AddResponse adds a response to a job - AddResponse(jobID int64, responder, response string) error + // AddComment adds a comment to a job + AddComment(jobID int64, commenter, comment string) error // EnqueueReview enqueues a review job and returns the job ID EnqueueReview(repoPath, gitRef, agentName string) (int64, error) @@ -41,8 +41,8 @@ type Client interface { // FindPendingJobForRef finds a queued or running job for any git ref FindPendingJobForRef(repoPath, gitRef string) (*storage.ReviewJob, error) - // GetResponsesForJob fetches responses for a job - GetResponsesForJob(jobID int64) ([]storage.Response, error) + // GetCommentsForJob fetches comments for a job + GetCommentsForJob(jobID int64) ([]storage.Response, error) } // DefaultPollInterval is the default polling interval for WaitForReview. @@ -150,14 +150,14 @@ func (c *HTTPClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (c *HTTPClient) AddResponse(jobID int64, responder, response string) error { +func (c *HTTPClient) AddComment(jobID int64, commenter, comment string) error { reqBody, _ := json.Marshal(map[string]interface{}{ "job_id": jobID, - "responder": responder, - "response": response, + "commenter": commenter, + "comment": comment, }) - resp, err := c.httpClient.Post(c.addr+"/api/respond", "application/json", bytes.NewReader(reqBody)) + resp, err := c.httpClient.Post(c.addr+"/api/comment", "application/json", bytes.NewReader(reqBody)) if err != nil { return err } @@ -165,7 +165,7 @@ func (c *HTTPClient) AddResponse(jobID int64, responder, response string) error if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("add response: %s: %s", resp.Status, body) + return fmt.Errorf("add comment: %s: %s", resp.Status, body) } return nil @@ -373,8 +373,8 @@ func (c *HTTPClient) FindPendingJobForRef(repoPath, gitRef string) (*storage.Rev return nil, nil } -func (c *HTTPClient) GetResponsesForJob(jobID int64) ([]storage.Response, error) { - resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/responses?job_id=%d", c.addr, jobID)) +func (c *HTTPClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) { + resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/comments?job_id=%d", c.addr, jobID)) if err != nil { return nil, err } diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index 65d08da..fafbc8c 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -15,11 +15,11 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) -func TestHTTPClientAddResponse(t *testing.T) { +func TestHTTPClientAddComment(t *testing.T) { var received map[string]interface{} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/respond" || r.Method != http.MethodPost { + if r.URL.Path != "/api/comment" || r.Method != http.MethodPost { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) return @@ -32,18 +32,18 @@ func TestHTTPClientAddResponse(t *testing.T) { defer server.Close() client := NewHTTPClient(server.URL) - if err := client.AddResponse(42, "test-agent", "Fixed the issue"); err != nil { - t.Fatalf("AddResponse failed: %v", err) + if err := client.AddComment(42, "test-agent", "Fixed the issue"); err != nil { + t.Fatalf("AddComment failed: %v", err) } if received["job_id"].(float64) != 42 { t.Errorf("expected job_id 42, got %v", received["job_id"]) } - if received["responder"] != "test-agent" { - t.Errorf("expected responder test-agent, got %v", received["responder"]) + if received["commenter"] != "test-agent" { + t.Errorf("expected commenter test-agent, got %v", received["commenter"]) } - if received["response"] != "Fixed the issue" { - t.Errorf("expected response to match, got %v", received["response"]) + if received["comment"] != "Fixed the issue" { + t.Errorf("expected comment to match, got %v", received["comment"]) } } diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 993b933..5d1db97 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -69,8 +69,8 @@ func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server { mux.HandleFunc("/api/repos", s.handleListRepos) mux.HandleFunc("/api/review", s.handleGetReview) mux.HandleFunc("/api/review/address", s.handleAddressReview) - mux.HandleFunc("/api/respond", s.handleAddResponse) - mux.HandleFunc("/api/responses", s.handleListResponses) + mux.HandleFunc("/api/comment", s.handleAddComment) + mux.HandleFunc("/api/comments", s.handleListComments) mux.HandleFunc("/api/status", s.handleStatus) mux.HandleFunc("/api/stream/events", s.handleStreamEvents) mux.HandleFunc("/api/sync/now", s.handleSyncNow) @@ -707,27 +707,27 @@ func (s *Server) handleGetReview(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, review) } -type AddResponseRequest struct { +type AddCommentRequest struct { SHA string `json:"sha,omitempty"` // Legacy: link to commit by SHA JobID int64 `json:"job_id,omitempty"` // Preferred: link to job - Responder string `json:"responder"` - Response string `json:"response"` + Commenter string `json:"commenter"` + Comment string `json:"comment"` } -func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { +func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { writeError(w, http.StatusMethodNotAllowed, "method not allowed") return } - var req AddResponseRequest + var req AddCommentRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeError(w, http.StatusBadRequest, "invalid request body") return } - if req.Responder == "" || req.Response == "" { - writeError(w, http.StatusBadRequest, "responder and response are required") + if req.Commenter == "" || req.Comment == "" { + writeError(w, http.StatusBadRequest, "commenter and comment are required") return } @@ -742,13 +742,13 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { if req.JobID != 0 { // Link to job (preferred method) - resp, err = s.db.AddResponseToJob(req.JobID, req.Responder, req.Response) + resp, err = s.db.AddCommentToJob(req.JobID, req.Commenter, req.Comment) if err != nil { if errors.Is(err, sql.ErrNoRows) { writeError(w, http.StatusNotFound, "job not found") return } - writeError(w, http.StatusInternalServerError, fmt.Sprintf("add response: %v", err)) + writeError(w, http.StatusInternalServerError, fmt.Sprintf("add comment: %v", err)) return } } else { @@ -759,9 +759,9 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { return } - resp, err = s.db.AddResponse(commit.ID, req.Responder, req.Response) + resp, err = s.db.AddComment(commit.ID, req.Commenter, req.Comment) if err != nil { - writeError(w, http.StatusInternalServerError, fmt.Sprintf("add response: %v", err)) + writeError(w, http.StatusInternalServerError, fmt.Sprintf("add comment: %v", err)) return } } @@ -769,7 +769,7 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, resp) } -func (s *Server) handleListResponses(w http.ResponseWriter, r *http.Request) { +func (s *Server) handleListComments(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { writeError(w, http.StatusMethodNotAllowed, "method not allowed") return @@ -785,13 +785,13 @@ func (s *Server) handleListResponses(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "invalid job_id") return } - responses, err = s.db.GetResponsesForJob(jobID) + responses, err = s.db.GetCommentsForJob(jobID) if err != nil { writeError(w, http.StatusInternalServerError, fmt.Sprintf("get responses: %v", err)) return } } else if sha := r.URL.Query().Get("sha"); sha != "" { - responses, err = s.db.GetResponsesForCommitSHA(sha) + responses, err = s.db.GetCommentsForCommitSHA(sha) if err != nil { writeError(w, http.StatusNotFound, "commit not found") return diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 6963219..9b4eba5 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -370,7 +370,7 @@ func (b *Builder) writePreviousReviews(sb *strings.Builder, contexts []ReviewCon // Include responses to this review if len(ctx.Responses) > 0 { - sb.WriteString("\nResponses to this review:\n") + sb.WriteString("\nComments on this review:\n") for _, resp := range ctx.Responses { sb.WriteString(fmt.Sprintf("- %s: %q\n", resp.Responder, resp.Response)) } @@ -411,11 +411,11 @@ func (b *Builder) writePreviousAttemptsForGitRef(sb *strings.Builder, gitRef str sb.WriteString(review.Output) sb.WriteString("\n") - // Fetch and include responses for this review + // Fetch and include comments for this review if review.JobID > 0 { - responses, err := b.db.GetResponsesForJob(review.JobID) + responses, err := b.db.GetCommentsForJob(review.JobID) if err == nil && len(responses) > 0 { - sb.WriteString("\nResponses to this review:\n") + sb.WriteString("\nComments on this review:\n") for _, resp := range responses { sb.WriteString(fmt.Sprintf("- %s: %q\n", resp.Responder, resp.Response)) } @@ -442,9 +442,9 @@ func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([] if err == nil { ctx.Review = review - // Also fetch responses for this review's job + // Also fetch comments for this review's job if review.JobID > 0 { - responses, err := b.db.GetResponsesForJob(review.JobID) + responses, err := b.db.GetCommentsForJob(review.JobID) if err == nil { ctx.Responses = responses } diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index 18be53b..2ce907d 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -212,14 +212,14 @@ func TestBuildPromptWithPreviousReviewsAndResponses(t *testing.T) { db.ClaimJob("test-worker") db.CompleteJob(job.ID, "test", "prompt", "Found potential memory leak in connection pool") - // Add responses to the previous review - _, err = db.AddResponseToJob(job.ID, "alice", "Known issue, will fix in next sprint") + // Add comments to the previous review + _, err = db.AddCommentToJob(job.ID, "alice", "Known issue, will fix in next sprint") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } - _, err = db.AddResponseToJob(job.ID, "bob", "Added to tech debt backlog") + _, err = db.AddCommentToJob(job.ID, "bob", "Added to tech debt backlog") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Also add commits 4 and 5 to DB @@ -244,25 +244,25 @@ func TestBuildPromptWithPreviousReviewsAndResponses(t *testing.T) { t.Error("Prompt should contain the previous review text") } - // Should contain responses to the previous review - if !strings.Contains(prompt, "Responses to this review:") { - t.Error("Prompt should contain responses section for previous review") + // Should contain comments on the previous review + if !strings.Contains(prompt, "Comments on this review:") { + t.Error("Prompt should contain comments section for previous review") } if !strings.Contains(prompt, "alice") { - t.Error("Prompt should contain responder 'alice'") + t.Error("Prompt should contain commenter 'alice'") } if !strings.Contains(prompt, "Known issue, will fix in next sprint") { - t.Error("Prompt should contain alice's response text") + t.Error("Prompt should contain alice's comment text") } if !strings.Contains(prompt, "bob") { - t.Error("Prompt should contain responder 'bob'") + t.Error("Prompt should contain commenter 'bob'") } if !strings.Contains(prompt, "Added to tech debt backlog") { - t.Error("Prompt should contain bob's response text") + t.Error("Prompt should contain bob's comment text") } } @@ -586,9 +586,9 @@ func TestBuildPromptWithPreviousAttemptsAndResponses(t *testing.T) { db.CompleteJob(job.ID, "test", "prompt", "Found issue: missing null check") // Add a response to the previous review - _, err = db.AddResponseToJob(job.ID, "developer", "This is intentional, the value is never null here") + _, err = db.AddCommentToJob(job.ID, "developer", "This is intentional, the value is never null here") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Build prompt for a new review of the same commit @@ -607,17 +607,17 @@ func TestBuildPromptWithPreviousAttemptsAndResponses(t *testing.T) { t.Error("Prompt should contain the previous review text") } - // Should contain the response - if !strings.Contains(prompt, "Responses to this review:") { - t.Error("Prompt should contain responses section") + // Should contain the comment + if !strings.Contains(prompt, "Comments on this review:") { + t.Error("Prompt should contain comments section") } if !strings.Contains(prompt, "This is intentional") { - t.Error("Prompt should contain the response text") + t.Error("Prompt should contain the comment text") } if !strings.Contains(prompt, "developer") { - t.Error("Prompt should contain the responder name") + t.Error("Prompt should contain the commenter name") } } diff --git a/internal/prompt/templates.go b/internal/prompt/templates.go index d170a23..7d1c049 100644 --- a/internal/prompt/templates.go +++ b/internal/prompt/templates.go @@ -43,6 +43,9 @@ func GetSystemPrompt(agentName string, promptType string) string { return SystemPromptRange case "address": return SystemPromptAddress + case "run": + // No default run preamble - return empty so raw prompts are used + return "" default: return SystemPromptSingle } diff --git a/internal/prompt/templates_test.go b/internal/prompt/templates_test.go index 39e078b..7675ed6 100644 --- a/internal/prompt/templates_test.go +++ b/internal/prompt/templates_test.go @@ -38,3 +38,16 @@ func TestGeminiRunTemplateLoads(t *testing.T) { t.Errorf("Expected Gemini run template content, got: %s", result[:min(100, len(result))]) } } + +func TestNonGeminiRunReturnsEmpty(t *testing.T) { + // Non-Gemini agents without a run template should return empty string, + // NOT the review prompt. This ensures roborev run uses raw prompts + // without review-style preambles for agents that don't have one. + agents := []string{"claude-code", "claude", "unknown-agent"} + for _, agent := range agents { + result := GetSystemPrompt(agent, "run") + if result != "" { + t.Errorf("GetSystemPrompt(%q, \"run\") = %q, want empty string", agent, result[:min(50, len(result))]) + } + } +} diff --git a/internal/skills/claude/roborev-address/SKILL.md b/internal/skills/claude/roborev-address/SKILL.md index 664a10e..e7a5a8a 100644 --- a/internal/skills/claude/roborev-address/SKILL.md +++ b/internal/skills/claude/roborev-address/SKILL.md @@ -41,10 +41,10 @@ Parse the findings from the output (severity, file paths, line numbers), then: After fixing, **record what was done** by executing: ```bash -roborev respond --job "" +roborev comment --job "" ``` -This records your response in roborev so the review shows it was addressed. +This records your comment in roborev so the review shows it was addressed. Then ask the user if they want to commit the changes. @@ -56,5 +56,5 @@ Agent: 1. Executes `roborev show --job 1019` 2. Sees verdict is Fail with 2 findings 3. Reads files, fixes the issues, runs tests -4. Executes `roborev respond --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` -5. Asks: "I've addressed both findings and recorded the response. Tests pass. Would you like me to commit these changes?" +4. Executes `roborev comment --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` +5. Asks: "I've addressed both findings and recorded a comment. Tests pass. Would you like me to commit these changes?" diff --git a/internal/skills/claude/roborev-respond/SKILL.md b/internal/skills/claude/roborev-respond/SKILL.md index 9272567..8f34b57 100644 --- a/internal/skills/claude/roborev-respond/SKILL.md +++ b/internal/skills/claude/roborev-respond/SKILL.md @@ -1,11 +1,11 @@ --- name: roborev:respond -description: Add a response or note to a roborev code review to document how findings were addressed +description: Add a comment to a roborev code review and mark it as addressed --- # roborev:respond -Record a response to a roborev code review. +Record a comment on a roborev code review and mark it as addressed. ## Usage @@ -15,7 +15,7 @@ Record a response to a roborev code review. ## IMPORTANT -This skill requires you to **execute a bash command** to record the response in roborev. The task is not complete until you run the `roborev respond` command and see confirmation output. +This skill requires you to **execute bash commands** to record the comment and mark the review addressed. The task is not complete until you run both commands and see confirmation output. ## Instructions @@ -23,14 +23,14 @@ When the user invokes `/roborev:respond [message]`: 1. **If a message is provided**, immediately execute: ```bash - roborev respond --job "" + roborev comment --job "" && roborev address ``` -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their response. +2. **If no message is provided**, ask the user what they'd like to say, then execute the commands with their comment. -3. **Verify success** - the command will output confirmation. If it fails, report the error. +3. **Verify success** - both commands will output confirmation. If either fails, report the error. -The response is recorded in roborev's database and will appear when viewing the review with `roborev show`. +The comment is recorded in roborev's database and the review is marked as addressed. View results with `roborev show`. ## Examples @@ -40,9 +40,9 @@ User: `/roborev:respond 1019 Fixed all issues` Agent action: ```bash -roborev respond --job 1019 "Fixed all issues" +roborev comment --job 1019 "Fixed all issues" && roborev address 1019 ``` -Then confirm: "Response recorded for review #1019." +Then confirm: "Comment recorded and review #1019 marked as addressed." --- @@ -50,12 +50,12 @@ Then confirm: "Response recorded for review #1019." User: `/roborev:respond 1019` -Agent: "What would you like to say in response to review #1019?" +Agent: "What would you like to say about review #1019?" User: "The null check was a false positive" Agent action: ```bash -roborev respond --job 1019 "The null check was a false positive" +roborev comment --job 1019 "The null check was a false positive" && roborev address 1019 ``` -Then confirm: "Response recorded for review #1019." +Then confirm: "Comment recorded and review #1019 marked as addressed." diff --git a/internal/skills/codex/roborev-address/SKILL.md b/internal/skills/codex/roborev-address/SKILL.md index 8380998..a05a25e 100644 --- a/internal/skills/codex/roborev-address/SKILL.md +++ b/internal/skills/codex/roborev-address/SKILL.md @@ -41,10 +41,10 @@ Parse the findings from the output (severity, file paths, line numbers), then: After fixing, **record what was done** by executing: ```bash -roborev respond --job "" +roborev comment --job "" ``` -This records your response in roborev so the review shows it was addressed. +This records your comment in roborev so the review shows it was addressed. Then ask the user if they want to commit the changes. @@ -56,5 +56,5 @@ Agent: 1. Executes `roborev show --job 1019` 2. Sees verdict is Fail with 2 findings 3. Reads files, fixes the issues, runs tests -4. Executes `roborev respond --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` -5. Asks: "I've addressed both findings and recorded the response. Tests pass. Would you like me to commit these changes?" +4. Executes `roborev comment --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` +5. Asks: "I've addressed both findings and recorded a comment. Tests pass. Would you like me to commit these changes?" diff --git a/internal/skills/codex/roborev-respond/SKILL.md b/internal/skills/codex/roborev-respond/SKILL.md index 367b621..3854e5e 100644 --- a/internal/skills/codex/roborev-respond/SKILL.md +++ b/internal/skills/codex/roborev-respond/SKILL.md @@ -1,11 +1,11 @@ --- name: roborev:respond -description: Add a response or note to a roborev code review to document how findings were addressed +description: Add a comment to a roborev code review and mark it as addressed --- # roborev:respond -Record a response to a roborev code review. +Record a comment on a roborev code review and mark it as addressed. ## Usage @@ -15,7 +15,7 @@ $roborev:respond [message] ## IMPORTANT -This skill requires you to **execute a bash command** to record the response in roborev. The task is not complete until you run the `roborev respond` command and see confirmation output. +This skill requires you to **execute bash commands** to record the comment and mark the review addressed. The task is not complete until you run both commands and see confirmation output. ## Instructions @@ -23,14 +23,14 @@ When the user invokes `$roborev:respond [message]`: 1. **If a message is provided**, immediately execute: ```bash - roborev respond --job "" + roborev comment --job "" && roborev address ``` -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their response. +2. **If no message is provided**, ask the user what they'd like to say, then execute the commands with their comment. -3. **Verify success** - the command will output confirmation. If it fails, report the error. +3. **Verify success** - both commands will output confirmation. If either fails, report the error. -The response is recorded in roborev's database and will appear when viewing the review with `roborev show`. +The comment is recorded in roborev's database and the review is marked as addressed. View results with `roborev show`. ## Examples @@ -40,9 +40,9 @@ User: `$roborev:respond 1019 Fixed all issues` Agent action: ```bash -roborev respond --job 1019 "Fixed all issues" +roborev comment --job 1019 "Fixed all issues" && roborev address 1019 ``` -Then confirm: "Response recorded for review #1019." +Then confirm: "Comment recorded and review #1019 marked as addressed." --- @@ -50,12 +50,12 @@ Then confirm: "Response recorded for review #1019." User: `$roborev:respond 1019` -Agent: "What would you like to say in response to review #1019?" +Agent: "What would you like to say about review #1019?" User: "The null check was a false positive" Agent action: ```bash -roborev respond --job 1019 "The null check was a false positive" +roborev comment --job 1019 "The null check was a false positive" && roborev address 1019 ``` -Then confirm: "Response recorded for review #1019." +Then confirm: "Comment recorded and review #1019 marked as addressed." diff --git a/internal/storage/db.go b/internal/storage/db.go index 61b797f..96d567f 100644 --- a/internal/storage/db.go +++ b/internal/storage/db.go @@ -618,7 +618,7 @@ func (db *DB) migrateSyncColumns() error { return fmt.Errorf("create idx_responses_uuid: %w", err) } - // Create index for GetResponsesToSync query pattern (source_machine_id + synced_at) + // Create index for GetCommentsToSync query pattern (source_machine_id + synced_at) _, err = db.Exec(`CREATE INDEX IF NOT EXISTS idx_responses_sync ON responses(source_machine_id, synced_at)`) if err != nil { return fmt.Errorf("create idx_responses_sync: %w", err) diff --git a/internal/storage/db_test.go b/internal/storage/db_test.go index f572f16..bf54d9c 100644 --- a/internal/storage/db_test.go +++ b/internal/storage/db_test.go @@ -311,24 +311,24 @@ func TestResponseOperations(t *testing.T) { repo, _ := db.GetOrCreateRepo("/tmp/test-repo") commit, _ := db.GetOrCreateCommit(repo.ID, "resp123", "Author", "Subject", time.Now()) - // Add response - resp, err := db.AddResponse(commit.ID, "test-user", "LGTM!") + // Add comment + resp, err := db.AddComment(commit.ID, "test-user", "LGTM!") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } if resp.Response != "LGTM!" { - t.Errorf("Expected response 'LGTM!', got '%s'", resp.Response) + t.Errorf("Expected comment 'LGTM!', got '%s'", resp.Response) } - // Get responses - responses, err := db.GetResponsesForCommit(commit.ID) + // Get comments + comments, err := db.GetCommentsForCommit(commit.ID) if err != nil { - t.Fatalf("GetResponsesForCommit failed: %v", err) + t.Fatalf("GetCommentsForCommit failed: %v", err) } - if len(responses) != 1 { - t.Errorf("Expected 1 response, got %d", len(responses)) + if len(comments) != 1 { + t.Errorf("Expected 1 comment, got %d", len(comments)) } } diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index 228f488..f13ebbf 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -940,7 +940,7 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int query += " WHERE " + strings.Join(conditions, " AND ") } - query += " ORDER BY j.enqueued_at DESC" + query += " ORDER BY j.id DESC" if limit > 0 { query += " LIMIT ?" diff --git a/internal/storage/repos_test.go b/internal/storage/repos_test.go index 1b1543a..16613f1 100644 --- a/internal/storage/repos_test.go +++ b/internal/storage/repos_test.go @@ -523,8 +523,8 @@ func TestDeleteRepo(t *testing.T) { db.ClaimJob("worker-1") db.CompleteJob(job.ID, "codex", "prompt", "output") - // Add a response - db.AddResponseToJob(job.ID, "user", "comment") + // Add a comment + db.AddCommentToJob(job.ID, "user", "comment") err := db.DeleteRepo(repo.ID, true) if err != nil { @@ -720,13 +720,13 @@ func TestDeleteRepoCascadeDeletesLegacyCommitResponses(t *testing.T) { repo, _ := db.GetOrCreateRepo("/tmp/delete-legacy-resp-test") commit, _ := db.GetOrCreateCommit(repo.ID, "legacy-resp-commit", "A", "S", time.Now()) - // Add legacy commit-based response (not job-based) - _, err := db.AddResponse(commit.ID, "reviewer", "Legacy comment on commit") + // Add legacy commit-based comment (not job-based) + _, err := db.AddComment(commit.ID, "reviewer", "Legacy comment on commit") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } - // Verify response exists + // Verify comment exists var beforeCount int db.QueryRow(`SELECT COUNT(*) FROM responses WHERE commit_id = ?`, commit.ID).Scan(&beforeCount) if beforeCount != 1 { diff --git a/internal/storage/reviews.go b/internal/storage/reviews.go index eae80a8..f767b50 100644 --- a/internal/storage/reviews.go +++ b/internal/storage/reviews.go @@ -252,8 +252,8 @@ func (db *DB) GetReviewByID(reviewID int64) (*Review, error) { return &r, nil } -// AddResponse adds a response to a commit (legacy - use AddResponseToJob for new code) -func (db *DB) AddResponse(commitID int64, responder, response string) (*Response, error) { +// AddComment adds a comment to a commit (legacy - use AddCommentToJob for new code) +func (db *DB) AddComment(commitID int64, responder, response string) (*Response, error) { uuid := GenerateUUID() machineID, _ := db.GetMachineID() now := time.Now() @@ -277,8 +277,8 @@ func (db *DB) AddResponse(commitID int64, responder, response string) (*Response }, nil } -// AddResponseToJob adds a response linked to a job/review -func (db *DB) AddResponseToJob(jobID int64, responder, response string) (*Response, error) { +// AddCommentToJob adds a comment linked to a job/review +func (db *DB) AddCommentToJob(jobID int64, responder, response string) (*Response, error) { // Verify job exists first to return proper 404 instead of FK violation or orphaned row var exists int err := db.QueryRow(`SELECT 1 FROM review_jobs WHERE id = ?`, jobID).Scan(&exists) @@ -312,8 +312,8 @@ func (db *DB) AddResponseToJob(jobID int64, responder, response string) (*Respon }, nil } -// GetResponsesForCommit returns all responses for a commit -func (db *DB) GetResponsesForCommit(commitID int64) ([]Response, error) { +// GetCommentsForCommit returns all comments for a commit +func (db *DB) GetCommentsForCommit(commitID int64) ([]Response, error) { rows, err := db.Query(` SELECT id, commit_id, job_id, responder, response, created_at FROM responses @@ -346,8 +346,8 @@ func (db *DB) GetResponsesForCommit(commitID int64) ([]Response, error) { return responses, rows.Err() } -// GetResponsesForJob returns all responses linked to a job -func (db *DB) GetResponsesForJob(jobID int64) ([]Response, error) { +// GetCommentsForJob returns all comments linked to a job +func (db *DB) GetCommentsForJob(jobID int64) ([]Response, error) { rows, err := db.Query(` SELECT id, commit_id, job_id, responder, response, created_at FROM responses @@ -380,11 +380,11 @@ func (db *DB) GetResponsesForJob(jobID int64) ([]Response, error) { return responses, rows.Err() } -// GetResponsesForCommitSHA returns all responses for a commit by SHA -func (db *DB) GetResponsesForCommitSHA(sha string) ([]Response, error) { +// GetCommentsForCommitSHA returns all comments for a commit by SHA +func (db *DB) GetCommentsForCommitSHA(sha string) ([]Response, error) { commit, err := db.GetCommitBySHA(sha) if err != nil { return nil, err } - return db.GetResponsesForCommit(commit.ID) + return db.GetCommentsForCommit(commit.ID) } diff --git a/internal/storage/sync.go b/internal/storage/sync.go index 964f106..7a9e401 100644 --- a/internal/storage/sync.go +++ b/internal/storage/sync.go @@ -452,9 +452,9 @@ type SyncableResponse struct { CreatedAt time.Time } -// GetResponsesToSync returns responses created locally that need to be pushed. -// Only returns responses whose parent job has already been synced. -func (db *DB) GetResponsesToSync(machineID string, limit int) ([]SyncableResponse, error) { +// GetCommentsToSync returns comments created locally that need to be pushed. +// Only returns comments whose parent job has already been synced. +func (db *DB) GetCommentsToSync(machineID string, limit int) ([]SyncableResponse, error) { rows, err := db.Query(` SELECT r.id, r.uuid, r.job_id, j.uuid, @@ -497,15 +497,15 @@ func (db *DB) GetResponsesToSync(machineID string, limit int) ([]SyncableRespons return responses, rows.Err() } -// MarkResponseSynced updates the synced_at timestamp for a response -func (db *DB) MarkResponseSynced(responseID int64) error { +// MarkCommentSynced updates the synced_at timestamp for a comment +func (db *DB) MarkCommentSynced(responseID int64) error { now := time.Now().UTC().Format(time.RFC3339) _, err := db.Exec(`UPDATE responses SET synced_at = ? WHERE id = ?`, now, responseID) return err } -// MarkResponsesSynced updates the synced_at timestamp for multiple responses -func (db *DB) MarkResponsesSynced(responseIDs []int64) error { +// MarkCommentsSynced updates the synced_at timestamp for multiple comments +func (db *DB) MarkCommentsSynced(responseIDs []int64) error { if len(responseIDs) == 0 { return nil } diff --git a/internal/storage/sync_test.go b/internal/storage/sync_test.go index a9742f6..03bf5e3 100644 --- a/internal/storage/sync_test.go +++ b/internal/storage/sync_test.go @@ -1476,7 +1476,7 @@ func TestGetReviewsToSync_TimestampComparison(t *testing.T) { }) } -func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { +func TestGetCommentsToSync_LegacyCommentsExcluded(t *testing.T) { // This test verifies that legacy responses with job_id IS NULL (tied only to commit_id) // are excluded from sync since they cannot be synced via job_uuid. dbPath := filepath.Join(t.TempDir(), "test.db") @@ -1521,9 +1521,9 @@ func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { t.Fatalf("MarkJobSynced failed: %v", err) } - jobResp, err := db.AddResponseToJob(job.ID, "human", "This is a job response") + jobResp, err := db.AddCommentToJob(job.ID, "human", "This is a job response") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Create a legacy commit-only response by directly inserting with job_id IS NULL @@ -1537,9 +1537,9 @@ func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { legacyRespID, _ := result.LastInsertId() // Get responses to sync - should only include the job-based response - responses, err := db.GetResponsesToSync(machineID, 100) + responses, err := db.GetCommentsToSync(machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } foundJobResp := false @@ -1702,9 +1702,9 @@ func TestClearAllSyncedAt(t *testing.T) { job := h.createCompletedJob("clear-test-sha") // Add a response - _, err := h.db.AddResponseToJob(job.ID, "user", "test response") + _, err := h.db.AddCommentToJob(job.ID, "user", "test response") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Mark everything as synced @@ -1756,9 +1756,9 @@ func TestClearAllSyncedAt(t *testing.T) { t.Errorf("Expected 1 review to sync after clear, got %d", len(reviews)) } - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response to sync after clear, got %d", len(responses)) @@ -1774,9 +1774,9 @@ func TestBatchMarkSynced(t *testing.T) { for i := 0; i < 5; i++ { job := h.createCompletedJob(fmt.Sprintf("batch-test-sha-%d", i)) jobs = append(jobs, job) - _, err := h.db.AddResponseToJob(job.ID, "user", fmt.Sprintf("response %d", i)) + _, err := h.db.AddCommentToJob(job.ID, "user", fmt.Sprintf("response %d", i)) if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } } @@ -1835,11 +1835,11 @@ func TestBatchMarkSynced(t *testing.T) { } }) - t.Run("MarkResponsesSynced marks multiple responses", func(t *testing.T) { + t.Run("MarkCommentsSynced marks multiple comments", func(t *testing.T) { // Get responses for synced jobs - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 3 { t.Fatalf("Expected 3 responses to sync (jobs 0-2 synced), got %d", len(responses)) @@ -1850,14 +1850,14 @@ func TestBatchMarkSynced(t *testing.T) { for i, r := range responses { responseIDs[i] = r.ID } - if err := h.db.MarkResponsesSynced(responseIDs); err != nil { - t.Fatalf("MarkResponsesSynced failed: %v", err) + if err := h.db.MarkCommentsSynced(responseIDs); err != nil { + t.Fatalf("MarkCommentsSynced failed: %v", err) } // Verify no responses left to sync (for synced jobs) - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses to sync after batch mark, got %d", len(responses)) @@ -1872,8 +1872,8 @@ func TestBatchMarkSynced(t *testing.T) { if err := h.db.MarkReviewsSynced([]int64{}); err != nil { t.Errorf("MarkReviewsSynced with empty slice failed: %v", err) } - if err := h.db.MarkResponsesSynced([]int64{}); err != nil { - t.Errorf("MarkResponsesSynced with empty slice failed: %v", err) + if err := h.db.MarkCommentsSynced([]int64{}); err != nil { + t.Errorf("MarkCommentsSynced with empty slice failed: %v", err) } }) } @@ -1960,24 +1960,24 @@ func TestGetReviewsToSync_RequiresJobSynced(t *testing.T) { } } -// TestGetResponsesToSync_RequiresJobSynced verifies that responses are only +// TestGetCommentsToSync_RequiresJobSynced verifies that responses are only // returned when their parent job has been synced (j.synced_at IS NOT NULL). -func TestGetResponsesToSync_RequiresJobSynced(t *testing.T) { +func TestGetCommentsToSync_RequiresJobSynced(t *testing.T) { h := newSyncTestHelper(t) // Create a completed job (not synced yet) job := h.createCompletedJob("response-sync-sha") // Add a response to the job - _, err := h.db.AddResponseToJob(job.ID, "test-user", "test response") + _, err := h.db.AddCommentToJob(job.ID, "test-user", "test response") if err != nil { t.Fatalf("Failed to add response: %v", err) } // Before job is synced, GetResponsesToSync should return nothing - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses before job is synced, got %d", len(responses)) @@ -1989,9 +1989,9 @@ func TestGetResponsesToSync_RequiresJobSynced(t *testing.T) { } // Now GetResponsesToSync should return the response - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response after job is synced, got %d", len(responses)) @@ -2064,7 +2064,7 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { job := h.createCompletedJob("workflow-sha-" + string(rune('a'+i))) createdJobs = append(createdJobs, job) // Add a response - _, err := h.db.AddResponseToJob(job.ID, "user", "response") + _, err := h.db.AddCommentToJob(job.ID, "user", "response") if err != nil { t.Fatalf("Failed to add response %d: %v", i, err) } @@ -2087,9 +2087,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 0 reviews to sync (jobs not synced), got %d", len(reviews)) } - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses to sync (jobs not synced), got %d", len(responses)) @@ -2117,9 +2117,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 1 review to sync, got %d", len(reviews)) } - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response to sync, got %d", len(responses)) @@ -2149,9 +2149,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 3 reviews to sync, got %d", len(reviews)) } - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 3 { t.Errorf("Expected 3 responses to sync, got %d", len(responses)) diff --git a/internal/storage/syncworker.go b/internal/storage/syncworker.go index c5e23e3..0b0acba 100644 --- a/internal/storage/syncworker.go +++ b/internal/storage/syncworker.go @@ -639,10 +639,10 @@ func (w *SyncWorker) pushChangesWithStats(ctx context.Context, pool *PgPool) (pu } } - // Push responses - batch operation - responses, err := w.db.GetResponsesToSync(machineID, syncBatchSize) + // Push comments - batch operation + responses, err := w.db.GetCommentsToSync(machineID, syncBatchSize) if err != nil { - return stats, fmt.Errorf("get responses to sync: %w", err) + return stats, fmt.Errorf("get comments to sync: %w", err) } if len(responses) > 0 { @@ -660,8 +660,8 @@ func (w *SyncWorker) pushChangesWithStats(ctx context.Context, pool *PgPool) (pu } } if len(syncedResponseIDs) > 0 { - if err := w.db.MarkResponsesSynced(syncedResponseIDs); err != nil { - log.Printf("Sync: failed to mark responses synced: %v", err) + if err := w.db.MarkCommentsSynced(syncedResponseIDs); err != nil { + log.Printf("Sync: failed to mark comments synced: %v", err) } } } diff --git a/internal/update/update.go b/internal/update/update.go index 1459725..b13bf06 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -561,9 +561,9 @@ func extractBaseSemver(v string) string { return v } -// gitDescribePattern matches git describe format: v0.16.1-2-gabcdef -// The -N-gHASH suffix indicates N commits after the tag -var gitDescribePattern = regexp.MustCompile(`-\d+-g[0-9a-f]+$`) +// gitDescribePattern matches git describe format: v0.16.1-2-gabcdef or v0.16.1-2-gabcdef-dirty +// The -N-gHASH suffix indicates N commits after the tag, with optional -dirty suffix +var gitDescribePattern = regexp.MustCompile(`-\d+-g[0-9a-f]+(-dirty)?$`) // isDevBuildVersion returns true if the version is a dev build. // Dev builds are either: diff --git a/internal/update/update_test.go b/internal/update/update_test.go index abff1a3..b0ff22f 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -274,6 +274,11 @@ func TestIsDevBuildVersion(t *testing.T) { {"0.4.0-5-gabcdef", true}, {"1.2.3-100-gdeadbeef", true}, + // Git describe with -dirty suffix - ARE dev builds + {"0.16.1-2-g75d300a-dirty", true}, + {"v0.16.1-2-g75d300a-dirty", true}, + {"0.4.0-5-gabcdef-dirty", true}, + // Pure hash/dev - ARE dev builds {"dev", true}, {"abc1234", true},