Skip to content

Conversation

dhth
Copy link
Owner

@dhth dhth commented Aug 17, 2025

Summary by CodeRabbit

  • Improvements

    • Centralized start/end time parsing and duration validation for task logs.
    • Unified Ctrl+C behavior: quits immediately from all views.
    • Streamlined time-entry flows and more consistent error messaging for invalid times.
  • Documentation

    • Help text updated: “q/esc” now labeled “Go back or quit.”
  • Tests

    • Added unit tests for the new time parsing/validation and adjusted UI tests to match the simplified validation and messaging.

Copy link

coderabbitai bot commented Aug 17, 2025

Walkthrough

Centralizes 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

Cohort / File(s) Summary of changes
Time parsing & validation (new)
internal/types/duration.go
Adds ParseTaskLogTimes and IsTaskLogDurationValid; trims inputs, parses with time.ParseInLocation(timeFormat, ..., time.Local), validates ordering and minimum 60s duration, and defines internal sentinel error vars.
UI: integrate centralized time logic
internal/ui/handle.go, internal/ui/view.go
Replaces manual begin/end parsing and in-file duration checks with calls to types.ParseTaskLogTimes and types.IsTaskLogDurationValid; simplifies error messaging to generic "Error: {err}"; removes now-duplicated helpers and specific error constants.
UI: quit handling
internal/ui/update.go
Unifies ctrl+C quitting into a top-level KeyMsg check in Update, removing inner ctrl+C cases.
UI: help text
internal/ui/help.go
Changes q/esc label from "Go back" to "Go back or quit".
Tests: new & modified
internal/types/duration_test.go, internal/ui/view_test.go
Adds internal/types table-driven tests for ParseTaskLogTimes (valid and invalid cases). Narrows internal/ui/view_test.go scenarios, removing many prior success/failure cases and adjusting expectations to the centralized parsing behavior.

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
Loading
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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch streamline-task-log-duration-validation

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 items

The 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 help

Current 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 exit
internal/types/duration.go (2)

1-56: Add package-level docs and function comments for exported API

These 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 cases

With 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 handler

You’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

📥 Commits

Reviewing files that changed from the base of the PR and between d75892e and 3d7d7f3.

⛔ 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 — LGTM

The “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 — LGTM

Catching 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 — LGTM

Removing 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 — LGTM

Switching 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 consistent

Please confirm that both format identifiers exist and align between parsing and display:

  • Ensure timeOnlyFormat is declared (e.g. in internal/ui) and used by m.activeTLBeginTS.Format(...).
  • Ensure timeFormat is declared (e.g. in internal/types) and used by types.GetTaskLogTimes for parsing.
internal/ui/handle.go (2)

96-99: LGTM: Centralized duration validation and consistent error message

Using 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 flows

By 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

Copy link

@coderabbitai coderabbitai bot left a 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 seconds

Using 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 cases

Current 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 precision

This 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 returning

Currently, 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 flow

Same 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 brevity

A 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3d7d7f3 and d868cf6.

⛔ 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.ParseTaskLogTimes

This 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 path

Delegating 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.

@dhth dhth merged commit a29a5ce into main Aug 17, 2025
15 checks passed
@dhth dhth deleted the streamline-task-log-duration-validation branch August 17, 2025 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant