feat: implement rerun_all_submissions CLI#54
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a new CLI command rerun-all-submissions for the admin CLI tool that allows administrators to rerun all latest submissions for each user-question combination. The implementation includes a Terminal User Interface (TUI) with progress tracking and a dry-run mode for previewing operations.
Changes:
- Added
rerun-all-submissionsCLI command with dry-run support and TUI progress visualization using charmbracelet libraries - Extended AdminCLIConfig to include SqlRunner configuration required for submission execution
- Refactored test utilities to create a reusable TestContext helper that manages test dependencies (EntClient, EventService, SqlRunner)
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/models.go | Added SqlRunner configuration field to AdminCLIConfig with validation |
| go.mod | Added charmbracelet TUI libraries (bubbles, bubbletea, lipgloss) and their dependencies |
| go.sum | Checksums for new charmbracelet library dependencies |
| cmd/admin-cli/cli.go | Initialize EventService and SqlRunner in main, pass to CLI context |
| cmd/admin-cli/commands.go | Added newRerunAllSubmissionsCommand with dry-run flag |
| cli/cli.go | Updated Context to include SubmissionService, changed NewContext signature |
| cli/rerun_all_submissions.go | Core implementation: fetches latest submissions, TUI progress display, sequential execution |
| cli/utils_test.go | New test helper providing reusable TestContext for CLI tests |
| cli/seed_users_test.go | Refactored to use new TestContext helper |
| cli/promote_test.go | Refactored to use new TestContext helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *Context) getLatestSubmissions(ctx context.Context) ([]LatestSubmission, error) { | ||
| // Get all submissions ordered by submitted_at descending | ||
| allSubmissions, err := c.entClient.Submission.Query(). | ||
| WithQuestion(). | ||
| WithUser(). | ||
| Order(ent.Desc(entsubmission.FieldSubmittedAt)). | ||
| All(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The getLatestSubmissions function loads all submissions into memory at once with .All(ctx), which could cause memory issues for systems with a large number of submissions. Consider implementing pagination or a more memory-efficient approach using database-level grouping (e.g., DISTINCT ON in PostgreSQL or a subquery with window functions) to retrieve only the latest submission per user-question pair.
| func (m *rerunModel) processNext() tea.Cmd { | ||
| return func() tea.Msg { | ||
| m.mu.Lock() | ||
| index := m.currentIndex | ||
| completed := m.completed | ||
| failed := m.failed | ||
| m.mu.Unlock() | ||
|
|
||
| if index >= len(m.submissions) { | ||
| return rerunProgressMsg{ | ||
| Index: index, | ||
| Completed: completed, | ||
| Failed: failed, | ||
| Status: "All done!", | ||
| Done: true, | ||
| } | ||
| } | ||
|
|
||
| sub := m.submissions[index] | ||
| statusMsg := fmt.Sprintf("Processing submission %d/%d (User %d, Question %d)...", | ||
| index+1, len(m.submissions), sub.UserID, sub.QuestionID) | ||
|
|
||
| // In dry run mode, just simulate without actually executing | ||
| if m.dryRun { | ||
| // Simulate a small delay to show progress | ||
| time.Sleep(50 * time.Millisecond) | ||
| completed++ | ||
| return rerunProgressMsg{ | ||
| Index: index + 1, | ||
| Completed: completed, | ||
| Failed: failed, | ||
| Status: statusMsg + " (dry run)", | ||
| Done: false, | ||
| } | ||
| } | ||
|
|
||
| // Rerun the submission | ||
| ctx := context.Background() | ||
| _, err := m.submissionService.SubmitAnswer(ctx, submission.SubmitAnswerInput{ | ||
| SubmitterID: sub.UserID, | ||
| QuestionID: sub.QuestionID, | ||
| Answer: sub.Code, | ||
| }) | ||
| if err != nil { | ||
| failed++ | ||
| return rerunProgressMsg{ | ||
| Index: index + 1, | ||
| Completed: completed, | ||
| Failed: failed, | ||
| Status: fmt.Sprintf("Failed: %v", err), | ||
| Err: err, | ||
| Done: false, | ||
| } | ||
| } | ||
|
|
||
| completed++ | ||
| return rerunProgressMsg{ | ||
| Index: index + 1, | ||
| Completed: completed, | ||
| Failed: failed, | ||
| Status: statusMsg + " ✓", | ||
| Done: false, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Submissions are being processed sequentially in the TUI, which could be very slow for a large number of submissions. Consider implementing concurrent processing with a worker pool pattern (e.g., 5-10 concurrent workers) to speed up the rerun operation. This would require careful synchronization of the progress UI updates and proper error handling.
| for _, sub := range allSubmissions { | ||
| // Get user and question IDs from edges | ||
| user, err := sub.Edges.UserOrErr() | ||
| if err != nil { | ||
| continue // Skip if user not loaded | ||
| } | ||
| question, err := sub.Edges.QuestionOrErr() | ||
| if err != nil { | ||
| continue // Skip if question not loaded | ||
| } |
There was a problem hiding this comment.
The code silently skips submissions when edges fail to load (user or question not found), continuing to the next submission without logging or tracking which submissions were skipped. This could lead to incomplete reruns without the operator knowing. Consider either returning an error if critical data is missing, or at minimum tracking and reporting skipped submissions to the user.
| if err := clictx.RerunAllSubmissions(ctx, dryRun); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| }, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The return statement is redundant since the previous if statement already returns on error. This line will only execute when err is nil, so returning nil is unnecessary.
| if err := clictx.RerunAllSubmissions(ctx, dryRun); err != nil { | |
| return err | |
| } | |
| return nil | |
| }, | |
| } | |
| } | |
| return clictx.RerunAllSubmissions(ctx, dryRun) | |
| }, | |
| } | |
| } |
| case rerunProgressMsg: | ||
| m.mu.Lock() | ||
| m.currentIndex = msg.Index | ||
| m.completed = msg.Completed | ||
| m.failed = msg.Failed | ||
| m.status = msg.Status | ||
| m.err = msg.Err | ||
| m.done = msg.Done | ||
| m.mu.Unlock() | ||
|
|
||
| if m.done { | ||
| return m, tea.Quit | ||
| } | ||
|
|
||
| // Continue processing | ||
| return m, m.processNext() |
There was a problem hiding this comment.
The mutex is held while calling processNext() which spawns a goroutine that will try to acquire the same mutex. While this specific case works because processNext() unlocks before the goroutine tries to lock again, it creates a subtle race condition risk. The pattern of holding a lock while scheduling async work that needs the same lock is error-prone. Consider restructuring to unlock before scheduling the next command.
| } | ||
|
|
||
| // Rerun the submission | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Creating a new background context here ignores the parent context, which means cancellation signals and deadlines from the parent won't propagate. This could lead to goroutines continuing to run even after the user quits the application. Consider passing the context through the message or storing it in the model, or using context.WithCancel to allow proper cancellation handling.
| ctx := context.Background() | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() |
No description provided.