chore: workspace save ticker #697
Conversation
WalkthroughThe ticker system is refactored to support configurable event types and intervals. The main.go file introduces a new workspace-save ticker running in parallel with the default ticker. The ticker.go file adds flexibility by extracting event type configuration into a field, providing a backward-compatible Changes
Sequence DiagramsequenceDiagram
participant Main as main.go
participant DefaultTicker as Default Ticker<br/>(WorkspaceTick)
participant SaveTicker as Workspace-Save Ticker<br/>(WorkspaceSave)
participant Producer as Kafka Producer
par Default Ticker Flow
loop Every interval
DefaultTicker->>Producer: ProduceEvent(eventType: "workspace_tick")
end
and Workspace-Save Ticker Flow
loop Every 1 hour
SaveTicker->>Producer: ProduceEvent(eventType: "workspace_save")
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes involve API refactoring with new public constructors, introduction of configurable event types, and parallel execution of multiple tickers. While the individual changes are straightforward, understanding the interaction between the two tickers and verifying backward compatibility requires careful analysis across multiple files. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 DB Package Test Coveragepkg/db coverage: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/ticker/ticker_test.go (1)
32-98: Consider adding test coverage for the newNewconstructor.While the existing tests are updated correctly, consider adding tests to verify:
- The
Newconstructor with a customeventTypeemits events with that type (notWorkspaceTickEventType)- The
Newconstructor respects the customintervalparameter- Both
NewDefaultandNewconstructors produce functionally correct tickersExample test to add:
func TestEmitTickForWorkspace_CustomEventType(t *testing.T) { mockProducer := new(MockEventProducer) customEventType := "custom.event.type" ticker := New(mockProducer, time.Minute, customEventType) workspaceID := "test-workspace" // Verify custom event type is used, not WorkspaceTickEventType mockProducer.On("ProduceEvent", customEventType, workspaceID, nil).Return(nil) err := ticker.emitTickForWorkspace(context.Background(), workspaceID) assert.NoError(t, err) mockProducer.AssertExpectations(t) }apps/workspace-engine/main.go (1)
157-163: Improve error log messages to distinguish between tickers.Both ticker goroutines log the generic message "Ticker error", making it difficult to determine which ticker encountered an issue during troubleshooting. Consider adding context to differentiate them.
Apply this diff to improve error logging:
workspaceSaveTicker := ticker.New(producer, 1*time.Hour, string(handler.WorkspaceSave)) go func() { log.Info("Workspace save ticker started") if err := workspaceSaveTicker.Run(ctx); err != nil { - log.Error("Ticker error", "error", err) + log.Error("Workspace save ticker error", "error", err) } }()Similarly, update line 153 for consistency:
- log.Error("Ticker error", "error", err) + log.Error("Workspace ticker error", "error", err)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/workspace-engine/main.go(2 hunks)apps/workspace-engine/pkg/ticker/ticker.go(2 hunks)apps/workspace-engine/pkg/ticker/ticker_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/ticker/ticker_test.goapps/workspace-engine/main.goapps/workspace-engine/pkg/ticker/ticker.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/ticker/ticker_test.go
🧬 Code graph analysis (3)
apps/workspace-engine/pkg/ticker/ticker_test.go (1)
apps/workspace-engine/pkg/ticker/ticker.go (1)
WorkspaceTickEventType(23-23)
apps/workspace-engine/main.go (2)
apps/workspace-engine/pkg/ticker/ticker.go (2)
NewDefault(36-45)New(48-54)apps/workspace-engine/pkg/events/handler/handler.go (1)
WorkspaceSave(79-79)
apps/workspace-engine/pkg/ticker/ticker.go (1)
apps/workspace-engine/pkg/kafka/producer.go (1)
EventProducer(12-16)
🔇 Additional comments (3)
apps/workspace-engine/pkg/ticker/ticker.go (1)
35-45: LGTM!The
NewDefaultconstructor correctly maintains backward compatibility by using the default event type and reading the interval from the environment with proper validation.apps/workspace-engine/main.go (2)
12-12: LGTM!The import is necessary to access the
WorkspaceSaveevent type constant used in the new ticker configuration.
149-149: LGTM!The change to
NewDefaultcorrectly maintains backward compatibility with the previous behavior while adapting to the refactored ticker API.
| // New creates a new ticker with the configured interval and event type | ||
| func New(producer kafka.EventProducer, interval time.Duration, eventType string) *Ticker { | ||
| return &Ticker{ | ||
| producer: producer, | ||
| interval: interval, | ||
| eventType: eventType, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input validation to prevent runtime issues.
The constructor should validate that interval is positive and eventType is non-empty to prevent runtime panics (from time.NewTicker) and invalid event production.
Apply this diff to add validation:
// New creates a new ticker with the configured interval and event type
func New(producer kafka.EventProducer, interval time.Duration, eventType string) *Ticker {
+ if interval <= 0 {
+ log.Warn("Invalid interval provided, using default", "interval", interval)
+ interval = DefaultTickInterval
+ }
+ if eventType == "" {
+ log.Warn("Empty event type provided, using default")
+ eventType = WorkspaceTickEventType
+ }
+
return &Ticker{
producer: producer,
interval: interval,
eventType: eventType,
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/ticker/ticker.go around lines 47 to 54, validate
inputs in New: ensure interval > 0 and eventType is not empty; if invalid,
return an error (change signature to New(producer kafka.EventProducer, interval
time.Duration, eventType string) (*Ticker, error)), add appropriate fmt.Errorf
messages for each check (or a combined validation error), and only
construct/return the Ticker when validations pass; remember to add the fmt
import and update all call sites to handle the (Ticker, error) return.
Summary by CodeRabbit
New Features
Refactor