pass time provider as a dependency #92
Conversation
WalkthroughIntroduces a TimeProvider interface and concrete providers (RealTimeProvider, TestTimeProvider); replaces direct time.Now() usage with provider-based calls; updates Task and TaskLogEntry UpdateListDesc signatures to accept TimeProvider; wires the provider through UI initialization and interactive/report/stats flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as cmd.Root
participant UI as RenderUI
participant Init as InitialModel
participant M as Model
participant TP as RealTimeProvider
CLI->>UI: RenderUI(db, style, RealTimeProvider{})
UI->>Init: InitialModel(db, style, RealTimeProvider{}, debug, cfg)
Init->>M: construct Model{timeProvider: RealTimeProvider{}}
M-->>UI: ready
sequenceDiagram
participant M as Model
participant TP as TimeProvider
participant T as Task
participant TL as TaskLogEntry
Note over M: During render/update
M->>TP: Now()
TP-->>M: time.Time
M->>T: UpdateListDesc(TimeProvider)
M->>TL: UpdateListDesc(TimeProvider)
T-->>M: updated desc
TL-->>M: updated desc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code Graph Analysis (2)cmd/root.go (2)
internal/ui/ui.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
internal/types/types.go (3)
63-79: TimeProvider abstraction looks good; simple and testable.The interface and the two concrete providers are straightforward and enable deterministic time in tests.
Consider adding brief doc comments to exported symbols (TimeProvider, RealTimeProvider, TestTimeProvider) to improve discoverability in GoDoc.
90-101: Pin “now” once per call to avoid subtle drift and double syscalls.Call Now() once and reuse it to keep a consistent reference time within the method and reduce overhead.
Apply:
-func (t *Task) UpdateListDesc(timeProvider TimeProvider) { +func (t *Task) UpdateListDesc(timeProvider TimeProvider) { var timeSpent string if t.SecsSpent != 0 { timeSpent = "worked on for " + HumanizeDuration(t.SecsSpent) } else { timeSpent = "no time spent" } - lastUpdated := fmt.Sprintf("last updated: %s", humanize.RelTime(t.UpdatedAt, timeProvider.Now(), "ago", "from now")) + now := timeProvider.Now() + lastUpdated := fmt.Sprintf("last updated: %s", humanize.RelTime(t.UpdatedAt, now, "ago", "from now")) t.ListDesc = fmt.Sprintf("%s %s", utils.RightPadTrim(lastUpdated, 60, true), timeSpent) }
107-131: Reuse a single “now” in TaskLogEntry.UpdateListDesc for consistency. We’ve confirmed thatgetTSRelative(ts, reference)already accepts a reference time and that the only call site ininternal/types/types.gostill usestimeProvider.Now()twice. Capturenow := timeProvider.Now()once and pass it to bothgetTSRelativeandhumanize.RelTime:func (tl *TaskLogEntry) UpdateListDesc(timeProvider TimeProvider) { timeSpentStr := HumanizeDuration(tl.SecsSpent) var timeStr string var durationMsg string - endTSRelative := getTSRelative(tl.EndTS, timeProvider.Now()) + now := timeProvider.Now() + endTSRelative := getTSRelative(tl.EndTS, now) switch endTSRelative { case tsFromToday: durationMsg = fmt.Sprintf("%s ... %s", tl.BeginTS.Format(timeOnlyFormat), tl.EndTS.Format(timeOnlyFormat)) case tsFromYesterday: durationMsg = "Yesterday" case tsFromThisWeek: durationMsg = tl.EndTS.Format(dayFormat) default: - durationMsg = humanize.RelTime(tl.EndTS, timeProvider.Now(), "ago", "from now") + durationMsg = humanize.RelTime(tl.EndTS, now, "ago", "from now") } timeStr = fmt.Sprintf("%s (%s)", utils.RightPadTrim(durationMsg, 40, true), timeSpentStr) tl.ListDesc = fmt.Sprintf("%s %s", utils.RightPadTrim(tl.TaskSummary, 60, true), timeStr) }internal/ui/log.go (1)
48-58: Good: interactive path now injects a real-time provider.This wires the provider cleanly into the interactive flow.
For testability, consider accepting a types.TimeProvider parameter in RenderTaskLog (with a default of RealTimeProvider when omitted) so non-interactive code paths and higher-level tests can also control “now” deterministically.
internal/ui/report.go (1)
52-59: Provider injection aligned with the new plumbing.Passing types.RealTimeProvider{} into initialRecordsModel keeps interactive reports consistent with the new abstraction.
Similar to logs: consider making RenderReport accept a types.TimeProvider (defaulting to RealTimeProvider) to broaden test control without reaching into internals.
internal/ui/model.go (1)
100-108: Model.timeProvider initialization is coveredAll code paths into
Modelsupply a non-nilTimeProvider:
- internal/ui/initial.go (lines 18, 56–60):
InitialModeltakestimeProvider types.TimeProviderand assigns it toModel.timeProvider.- internal/ui/ui.go (line 44):
CallsInitialModel(..., types.RealTimeProvider{}).No entry point leaves
timeProviderunset, so panics are not possible today.Optional refactor (future-proofing): in
InitialModel, guard against a nil provider:func InitialModel(db *sql.DB, style Style, timeProvider types.TimeProvider, debug bool) Model { if timeProvider == nil { timeProvider = types.RealTimeProvider{} } m := Model{ db: db, style: style, timeProvider: timeProvider, // … } // … }internal/ui/update.go (1)
429-442: Minor: compute “now” once per ctrl+t branch for clarity.You’re calling Now() twice in separate cases; computing it once reduces duplication and eliminates micro drift.
Apply:
- switch m.period { - case types.TimePeriodWeek: - now := m.timeProvider.Now() + now := m.timeProvider.Now() + switch m.period { + case types.TimePeriodWeek: weekday := now.Weekday() offset := (7 + weekday - time.Monday) % 7 startOfWeek := now.AddDate(0, 0, -int(offset)) dr.Start = time.Date(startOfWeek.Year(), startOfWeek.Month(), startOfWeek.Day(), 0, 0, 0, 0, startOfWeek.Location()) dr.NumDays = 7 default: - now := m.timeProvider.Now() nDaysBack := now.AddDate(0, 0, -1*(m.dateRange.NumDays-1)) dr.Start = time.Date(nDaysBack.Year(), nDaysBack.Month(), nDaysBack.Day(), 0, 0, 0, 0, nDaysBack.Location()) }internal/ui/ui.go (1)
9-13: Prefer using the same TimeProvider for framesDir and model wiring (remove direct time.Now usage).Unifying the time source here removes the last direct time.Now dependency in this flow and improves determinism for tests (e.g., framesDir naming). It also lets you drop the time import.
Apply:
import ( "database/sql" "errors" "fmt" "os" "path/filepath" - "time" tea "github.com/charmbracelet/bubbletea" "github.com/dhth/hours/internal/types" ) @@ logFramesCfg := logFramesConfig{ log: logFrames, } + // Use a single provider for both framesDir and UI model. + tp := types.RealTimeProvider{} if logFrames { - framesDir := filepath.Join(".frames", fmt.Sprintf("%d", time.Now().Unix())) + framesDir := filepath.Join(".frames", fmt.Sprintf("%d", tp.Now().Unix())) err := os.MkdirAll(framesDir, 0o755) if err != nil { return fmt.Errorf("%w: %s", errCouldnCreateFramesDir, err.Error()) } logFramesCfg.framesDir = framesDir } p := tea.NewProgram( InitialModel( db, style, - types.RealTimeProvider{}, + tp, debug, logFramesCfg, ), tea.WithAltScreen(), )Also applies to: 31-41, 43-52
internal/ui/handle.go (1)
1-13: Optional: Replace remaining direct time.Now usages in the UI package
To complete the time-refactor in this layer, swap these calls out for your injected/common time provider for consistency and testability:• internal/ui/ui.go:35 – generating
framesDirviatime.Now().Unix()
• internal/ui/generate.go:145 –now := time.Now().Local()inrandomTimestampReview these spots (or justify acceptable exceptions) and update to use the shared Clock/Now API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (9)
internal/types/types.go(4 hunks)internal/ui/handle.go(11 hunks)internal/ui/initial.go(3 hunks)internal/ui/log.go(1 hunks)internal/ui/model.go(2 hunks)internal/ui/report.go(1 hunks)internal/ui/stats.go(1 hunks)internal/ui/ui.go(2 hunks)internal/ui/update.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
internal/ui/log.go (1)
internal/types/types.go (2)
RealTimeProvider(67-67)RealTimeProvider(69-71)
internal/ui/initial.go (2)
internal/types/types.go (1)
TimeProvider(63-65)internal/ui/model.go (1)
Model(100-138)
internal/ui/model.go (2)
internal/types/types.go (3)
TimeProvider(63-65)DateRange(230-234)TaskStatus(201-201)internal/ui/styles.go (1)
Style(13-44)
internal/ui/report.go (1)
internal/types/types.go (2)
RealTimeProvider(67-67)RealTimeProvider(69-71)
internal/ui/ui.go (2)
internal/ui/initial.go (1)
InitialModel(18-119)internal/types/types.go (2)
RealTimeProvider(67-67)RealTimeProvider(69-71)
internal/ui/handle.go (1)
internal/ui/model.go (1)
Model(100-138)
internal/ui/stats.go (1)
internal/types/types.go (2)
RealTimeProvider(67-67)RealTimeProvider(69-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: live-tests (ubuntu-latest)
🔇 Additional comments (9)
internal/ui/model.go (1)
156-169: initialRecordsModel correctly initializes timeProviderThe
return recordsModel{…}ininitialRecordsModelincludes thetimeProvider: timeProviderassignment, so the new field is properly set for all variants. No further changes are needed.internal/ui/update.go (1)
336-341: Correct: UpdateListDesc now uses the injected time provider.This keeps list rendering fully provider-driven.
internal/ui/ui.go (1)
12-12: LGTM: Provider injected into InitialModel at composition root.Passing a concrete types.RealTimeProvider here is consistent with the new dependency model and keeps main wiring simple.
Also applies to: 43-52
internal/ui/stats.go (1)
63-69: LGTM: TimeProvider threaded into interactive stats records model.This keeps the interactive path consistent with the rest of the UI’s time abstraction.
internal/ui/initial.go (3)
18-23: LGTM: InitialModel now accepts and stores a TimeProvider.Cleanly inserted before debug, and the Model.timeProvider field is correctly initialized.
Also applies to: 56-60
125-142: LGTM: initialRecordsModel propagates TimeProvider into recordsModel.Keeps record views aligned with the same abstraction.
18-23: All call sites match the new signaturesI’ve verified across the UI package that:
- internal/ui/ui.go invokes
InitialModel(db, style, timeProvider, debug, logFramesCfg)(5 args).- internal/ui/{stats,report,log}.go invoke
initialRecordsModel(kind, db, style, timeProvider, dateRange)(5 args).UpdateListDescis always called withm.timeProvider—no zero-arg usages remain.- No test files reference the old signatures.
No further updates are needed.
internal/ui/handle.go (2)
57-60: LGTM: Replaced direct time.Now with m.timeProvider.Now across flows.This aligns validation, tracking start/stop, and quick-switch logic with the injected time source. Truncation to second is consistently applied where required.
Also applies to: 79-84, 120-128, 137-141, 393-401, 488-491, 494-499, 524-533, 533-535
699-705: LGTM: List description renderers updated to accept a TimeProvider.Passing m.timeProvider into Task and TaskLogEntry UpdateListDesc ensures consistent time formatting in list UIs.
Also applies to: 712-716, 767-771
Summary by CodeRabbit
New Features
Bug Fixes