[codex] decouple Codex usage sync from CC statusline#264
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Codex usage synchronization logic by moving it from CCInfoTimerService into a dedicated CodexUsageSyncService. This new service handles periodic fetching and syncing of Codex usage data to the server. Feedback includes several improvements for the new service: ensuring the service context is properly propagated to background sync operations, adding missing imports for error handling and OS checks, restoring platform restrictions (macOS and Linux only), and conditionally starting the service only when a valid token is configured to avoid unnecessary background activity.
| import ( | ||
| "context" | ||
| "fmt" | ||
| "log/slog" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/malamtime/cli/model" | ||
| ) |
There was a problem hiding this comment.
Add missing imports for errors, os, and runtime to support context propagation, OS-specific checks, and better error handling.
| import ( | |
| "context" | |
| "fmt" | |
| "log/slog" | |
| "sync" | |
| "time" | |
| "github.com/malamtime/cli/model" | |
| ) | |
| import ( | |
| "context" | |
| "errors" | |
| "fmt" | |
| "log/slog" | |
| "os" | |
| "runtime" | |
| "sync" | |
| "time" | |
| "github.com/malamtime/cli/model" | |
| ) |
| s.sync() | ||
|
|
||
| for { | ||
| select { | ||
| case <-s.ticker.C: | ||
| s.sync() | ||
| case <-s.stopChan: | ||
| return | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Propagate the service context to the sync function to ensure that background operations can be cancelled during daemon shutdown.
| s.sync() | |
| for { | |
| select { | |
| case <-s.ticker.C: | |
| s.sync() | |
| case <-s.stopChan: | |
| return | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| s.sync(ctx) | |
| for { | |
| select { | |
| case <-s.ticker.C: | |
| s.sync(ctx) | |
| case <-s.stopChan: | |
| return | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } |
| func (s *CodexUsageSyncService) sync() { | ||
| if s.config.Token == "" { | ||
| return | ||
| } | ||
|
|
||
| if err := syncCodexUsage(context.Background(), s.config); err != nil { | ||
| slog.Warn("Failed to sync codex usage", slog.Any("err", err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the sync method to accept a context and use it when calling syncCodexUsage. Also, avoid logging a warning if the Codex authentication file is missing, as this is a common scenario for users who do not use Codex.
func (s *CodexUsageSyncService) sync(ctx context.Context) {
if s.config.Token == "" {
return
}
if err := syncCodexUsage(ctx, s.config); err != nil {
if !errors.Is(err, os.ErrNotExist) {
slog.Warn("Failed to sync codex usage", slog.Any("err", err))
}
}
}| func syncCodexUsage(ctx context.Context, config model.ShellTimeConfig) error { | ||
| if config.Token == "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Restore the OS check to restrict Codex usage syncing to macOS and Linux, maintaining parity with the previous implementation in CCInfoTimerService.
| func syncCodexUsage(ctx context.Context, config model.ShellTimeConfig) error { | |
| if config.Token == "" { | |
| return nil | |
| } | |
| func syncCodexUsage(ctx context.Context, config model.ShellTimeConfig) error { | |
| if runtime.GOOS != "darwin" && runtime.GOOS != "linux" { | |
| return nil | |
| } | |
| if config.Token == "" { | |
| return nil | |
| } |
| codexUsageSyncService := daemon.NewCodexUsageSyncService(cfg) | ||
| if err := codexUsageSyncService.Start(ctx); err != nil { | ||
| slog.Error("Failed to start Codex usage sync service", slog.Any("err", err)) | ||
| } else { | ||
| slog.Info("Codex usage sync service started") | ||
| defer codexUsageSyncService.Stop() | ||
| } |
There was a problem hiding this comment.
Only start the CodexUsageSyncService if a ShellTime token is configured. This avoids unnecessary background activity and misleading log messages when the service cannot perform any syncing.
| codexUsageSyncService := daemon.NewCodexUsageSyncService(cfg) | |
| if err := codexUsageSyncService.Start(ctx); err != nil { | |
| slog.Error("Failed to start Codex usage sync service", slog.Any("err", err)) | |
| } else { | |
| slog.Info("Codex usage sync service started") | |
| defer codexUsageSyncService.Stop() | |
| } | |
| if cfg.Token != "" { | |
| codexUsageSyncService := daemon.NewCodexUsageSyncService(cfg) | |
| if err := codexUsageSyncService.Start(ctx); err != nil { | |
| slog.Error("Failed to start Codex usage sync service", slog.Any("err", err)) | |
| } else { | |
| slog.Info("Codex usage sync service started") | |
| defer codexUsageSyncService.Stop() | |
| } | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77cd44be2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return err | ||
| } | ||
|
|
||
| return sendCodexUsageToServer(runCtx, config, usage) |
There was a problem hiding this comment.
syncCodexUsage reuses runCtx for both fetchCodexUsageFunc and sendCodexUsageToServer, so any slow fetch consumes the same 10s deadline and can leave the POST with little or no time to run. In practice, when the usage API call is slow (but still successful), the subsequent upload is canceled and usage never reaches /api/v1/codex-usage; this is a regression from the previous flow where upload had a separate timeout window.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@claude review it |
|
Claude finished @AnnatarHe's task in 1m 25s —— View job Code Review: Decouple Codex Usage Sync from CC Statusline
Good refactoring overall — extracting the Codex usage sync into its own service with proper lifecycle management, sentinel errors, and a startup guard via 1.
|
1 participant
Uh oh!
There was an error while loading. Please reload this page.