-
Notifications
You must be signed in to change notification settings - Fork 8
streamline task log duration validation #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughCentralizes task-log time parsing and validation into new internal/types functions, updates UI to use them (including unified top-level ctrl+C quit handling), adjusts help text, refactors duration display/error messaging, and adds unit tests for the new parsing logic while reducing some existing UI tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as UI (handle/view)
participant Types as internal/types
User->>UI: Provide beginStr, endStr
UI->>Types: ParseTaskLogTimes(beginStr, endStr)
Types-->>UI: (beginTS, endTS) or error
alt parsed successfully
UI->>Types: (optional) IsTaskLogDurationValid(beginTS, endTS)
Types-->>UI: nil or error
alt valid
UI->>UI: compute/display duration, proceed
else invalid
UI->>User: Show "Error: {err}"
end
else parse error
UI->>User: Show "Error: {err}"
end
sequenceDiagram
participant Sys as Terminal
participant App as Update()
Sys-->>App: KeyMsg(ctrl+C)
App->>App: top-level ctrl+C check
alt ctrl+C detected
App-->>Sys: tea.Quit (quit immediately)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
internal/ui/help.go (1)
26-34
: Fix count: header says “6 views” but lists 7 itemsThe header claims there are 6 views, but the list includes 7 bullet points (including Help View).
Apply this diff to correct the count:
- "hours" has 6 views: + "hours" has 7 views:internal/ui/view.go (1)
281-283
: Nit: minor copy tweak for consistency with helpCurrent text: “Press (q/<ctrl+c>/ to exit)”. Consider slightly clearer phrasing:
Apply this diff for readability:
- Press (q/<ctrl+c>/<esc> to exit) + Press q/<esc> or <ctrl+c> to exitinternal/types/duration.go (2)
1-56
: Add package-level docs and function comments for exported APIThese functions are exported (even within internal/), so brief comments improve discoverability and editor tooling.
Proposed doc comments:
// ParseTaskLogTimes parses begin and end timestamps using timeFormat in the local timezone, // validates the resulting duration, and returns the parsed times or a validation error. // ValidateTaskLogDuration returns an error if end is before begin or the duration is under one minute.
1-56
: Add focused unit tests for parsing/validation edge casesWith logic centralized here, unit tests will keep the UI tests lean and still preserve coverage across edge cases.
Here’s a starter test file you can drop in as internal/types/duration_test.go:
package types import ( "testing" ) func TestGetTaskLogTimes(t *testing.T) { tests := []struct { name string begin string end string wantErr bool errSubstr string }{ {"empty begin", "", "2025/08/08 00:10", true, "begin time is empty"}, {"empty end", "2025/08/08 00:00", "", true, "end time is empty"}, {"invalid begin", "not-a-time", "2025/08/08 00:10", true, "begin time is invalid"}, {"invalid end", "2025/08/08 00:00", "not-a-time", true, "end time is invalid"}, {"end before begin", "2025/08/08 01:00", "2025/08/08 00:59", true, "before begin time"}, {"less than a minute", "2025/08/08 00:00", "2025/08/08 00:00", true, "at least a minute"}, {"exactly one minute", "2025/08/08 00:00", "2025/08/08 00:01", false, ""}, {"more than one minute", "2025/08/08 00:00", "2025/08/08 00:02", false, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { _, _, err := GetTaskLogTimes(tt.begin, tt.end) if tt.wantErr && err == nil { t.Fatalf("expected error, got nil") } if !tt.wantErr && err != nil { t.Fatalf("unexpected error: %v", err) } if tt.wantErr && tt.errSubstr != "" && (err == nil || !contains(err.Error(), tt.errSubstr)) { t.Fatalf("error %v does not contain expected substring %q", err, tt.errSubstr) } }) } } // contains is a tiny helper to avoid importing strings in the test for such a small need. func contains(s, sub string) bool { return len(sub) == 0 || (len(s) >= len(sub) && indexOf(s, sub) >= 0) } func indexOf(s, sub string) int { outer: for i := 0; i+len(sub) <= len(s); i++ { for j := range sub { if s[i+j] != sub[j] { continue outer } } return i } return -1 }internal/ui/handle.go (1)
154-156
: Likely bug: clearing the wrong input slice in editActiveTLView escape handlerYou’re clearing m.taskInputs[entryBeginTS] instead of the TL input field. This likely leaves stale TL input state behind.
Replace the line with the TL input:
m.tLInputs[entryBeginTS].SetValue("")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (3)
internal/ui/__snapshots__/TestFinishActiveTLViewWhereEndTimeBeforeBeginTime_1.snap
is excluded by!**/*.snap
internal/ui/__snapshots__/TestFinishActiveTLViewWhereNoTimeTracked_1.snap
is excluded by!**/*.snap
internal/ui/__snapshots__/TestHelpView_1.snap
is excluded by!**/*.snap
📒 Files selected for processing (6)
internal/types/duration.go
(1 hunks)internal/ui/handle.go
(3 hunks)internal/ui/help.go
(1 hunks)internal/ui/update.go
(1 hunks)internal/ui/view.go
(1 hunks)internal/ui/view_test.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/ui/view.go (1)
internal/types/duration.go (1)
GetTaskLogTimes
(18-44)
internal/ui/handle.go (1)
internal/types/duration.go (2)
GetTaskLogTimes
(18-44)IsTaskLogDurationValid
(46-55)
🔇 Additional comments (7)
internal/ui/help.go (1)
43-46
: Help hint update aligns with unified quit behavior — LGTMThe “q/ Go back or quit” wording matches the new top-level Ctrl+C handling in Update and clarifies behavior. No further action needed.
internal/ui/update.go (2)
30-33
: Top-level Ctrl+C handling simplifies and centralizes quit logic — LGTMCatching ctrl+c up-front and returning tea.Quit removes duplication and avoids missing paths. This matches the help text and makes behavior consistent.
36-40
: Insufficient dimensions: rely on top-level Ctrl+C, keep q/esc local — LGTMRemoving ctrl+c handling here (now caught at the top-level) is correct. Returning early for q/esc maintains the intended “exit immediately” experience when the UI can’t render.
internal/ui/view.go (2)
372-375
: Use centralized parsing/validation and standardize errors — LGTMSwitching to types.GetTaskLogTimes consolidates parsing/validation and standardizes error messages. Returning a single “Error: …” string keeps the UI consistent.
49-51
: Verify time format constants are defined and consistentPlease confirm that both format identifiers exist and align between parsing and display:
- Ensure
timeOnlyFormat
is declared (e.g. ininternal/ui
) and used bym.activeTLBeginTS.Format(...)
.- Ensure
timeFormat
is declared (e.g. ininternal/types
) and used bytypes.GetTaskLogTimes
for parsing.internal/ui/handle.go (2)
96-99
: LGTM: Centralized duration validation and consistent error messageUsing types.IsTaskLogDurationValid(begin, now) here is correct and aligns with the new centralized validation flow. The error message is surfaced to the user.
70-92
: Confirm product decision on allowing end timestamps in the future for finish/create/edit flowsBy delegating to types.GetTaskLogTimes and types.IsTaskLogDurationValid, the UI no longer prevents end > now. This enables:
- Finishing tracking with an end time in the future.
- Creating or editing a TL with an end time in the future.
This may create inconsistent UX (e.g., “finishing now” but storing a future end), and could impact reporting.
If the intent is to disallow future end-times in these contexts, add an explicit check against m.timeProvider.Now() before proceeding (or extend types validation with a context-aware variant). Please confirm intended behavior.
Also applies to: 108-144
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
internal/ui/view.go (1)
377-383
: Optional: avoid float conversions when deriving secondsUsing float Seconds() then casting to int can be avoided.
Apply this minimal change:
- totalSeconds := int(dur.Seconds()) + totalSeconds := int(dur / time.Second)And ensure this import exists at the top of the file:
import "time"internal/types/duration_test.go (1)
10-118
: Solid, table-driven coverage for ParseTaskLogTimes; consider a couple of casesCurrent tests look good and exercise valid/invalid paths with sentinel error checks. Two optional enhancements:
- Add a success case with leading/trailing whitespace around valid times (if you decide to TrimSpace before parsing; see my suggestion on duration.go).
- Optionally assert parsed times match expected values (not just non-zero) in one or two representative cases to guard against timezone/format regressions.
If you accept trimming inputs prior to parsing, add a case like:
{ name: "valid times with extra whitespace", beginStr: " 2025/08/08 00:00 ", endStr: " 2025/08/08 00:01 ", }internal/ui/handle.go (1)
436-446
: Minor behavior check: finishing flow sets default endTS to Now() (with seconds), inputs use minute precisionThis is fine functionally, but note the slight precision mismatch: inputs are minute-granular (timeFormat), while the default end timestamp here includes seconds. This can cause small deltas when users submit without editing. If you want strict minute granularity end-to-end, consider truncating Now() to minutes in this path as well.
Would you like a follow-up patch to normalize to minute precision consistently for begin/end in this flow?
♻️ Duplicate comments (3)
internal/ui/handle.go (2)
71-74
: Surface parsing/validation errors to the user instead of silently returningCurrently, errors from ParseTaskLogTimes are swallowed; the user receives no feedback.
Apply this diff:
beginTS, endTS, err := types.ParseTaskLogTimes(m.tLInputs[entryBeginTS].Value(), m.tLInputs[entryEndTS].Value()) if err != nil { - return nil + m.message = errMsg(fmt.Sprintf("Error: %s", err.Error())) + return nil }
108-111
: Also surface ParseTaskLogTimes errors for create/edit TL flowSame swallowing issue as the finish flow: user gets no feedback on invalid inputs.
Proposed fix:
beginTS, endTS, err := types.ParseTaskLogTimes(m.tLInputs[entryBeginTS].Value(), m.tLInputs[entryEndTS].Value()) if err != nil { - return nil + m.message = errMsg(fmt.Sprintf("Error: %s", err.Error())) + return nil }internal/types/duration.go (1)
38-41
: Inline the temporary error for brevityA small readability improvement; no behavior change.
- durationErr := IsTaskLogDurationValid(beginTS, endTS) - if durationErr != nil { - return zero, zero, durationErr - } + if err := IsTaskLogDurationValid(beginTS, endTS); err != nil { + return zero, zero, err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
internal/ui/__snapshots__/TestFinishActiveTLViewWhereNoTimeTracked_1.snap
is excluded by!**/*.snap
📒 Files selected for processing (4)
internal/types/duration.go
(1 hunks)internal/types/duration_test.go
(1 hunks)internal/ui/handle.go
(3 hunks)internal/ui/view.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
internal/types/duration_test.go (1)
internal/types/duration.go (1)
ParseTaskLogTimes
(18-44)
internal/ui/view.go (1)
internal/types/duration.go (1)
ParseTaskLogTimes
(18-44)
internal/ui/handle.go (1)
internal/types/duration.go (2)
ParseTaskLogTimes
(18-44)IsTaskLogDurationValid
(46-55)
🔇 Additional comments (3)
internal/ui/view.go (1)
371-387
: Good consolidation: UI now delegates parsing/validation to types.ParseTaskLogTimesThis removes duplicated logic in the view and keeps duration validity rules centralized. Error surfacing via "Error: ..." is consistent with the rest of the UI.
internal/ui/handle.go (1)
95-101
: LGTM: consistent error surfacing for quick-finish pathDelegating duration validation to types.IsTaskLogDurationValid and prefixing the error with "Error: " brings this path in line with the centralized validation and consistent UX.
internal/types/duration.go (1)
46-55
: Confirm intent: no check for “end is in the future”IsTaskLogDurationValid purposefully allows end > now. This is probably desirable for manual entries, but for “finish active TL” flows it might be surprising to allow future end times. If that’s intentional, all good. If not, consider a caller-specific check in the UI path handling “finish active TL.”
If you’d like, I can prepare a narrowly-scoped check (only in the finish-active path) that warns when end > now without changing the generic validation logic here.
Summary by CodeRabbit
Improvements
Documentation
Tests