feat(timer): implement periodic task time saving with 10-second intervals #153
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…vals Add functionality to automatically save elapsed time every 10 seconds during timer sessions, improving data persistence for task time tracking. The timer now tracks time since last save and persists duration records at regular intervals rather than only at session completion.
Contributor
Summary of ChangesHello @ahmet-cetinkaya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the application's timer functionality by implementing periodic time saving. Previously, time was only saved at the session's completion, risking data loss if the app crashed. Now, elapsed time is automatically saved every 10 seconds, ensuring continuous preservation of productivity data and a more robust user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Reviewer's GuideImplements periodic autosaving of task timer progress every 10 seconds in marathon and task detail views by tracking unsaved elapsed time, wiring timer callbacks, and persisting incremental durations instead of only full sessions. Sequence diagram for periodic autosave in marathon timersequenceDiagram
actor User
participant MarathonPageState
participant AppTimer
participant AsyncErrorHandler
participant Mediator
User->>MarathonPageState: tap_start_timer()
MarathonPageState->>AppTimer: onTimerStart()
AppTimer-->>MarathonPageState: onTimerStart callback
loop every_second_while_running
AppTimer->>MarathonPageState: onTick(displayTime)
MarathonPageState->>MarathonPageState: _handleTimerTick(displayTime)
MarathonPageState->>MarathonPageState: _timeSinceLastSave += 1_second
alt timeSinceLastSave >= 10_seconds
MarathonPageState->>MarathonPageState: _saveElapsedTime(_timeSinceLastSave)
MarathonPageState->>AsyncErrorHandler: executeVoid(AddTaskTimeRecordCommand)
AsyncErrorHandler->>Mediator: send(AddTaskTimeRecordCommand)
Mediator-->>AsyncErrorHandler: command_completed
AsyncErrorHandler-->>MarathonPageState: result
MarathonPageState->>MarathonPageState: _timeSinceLastSave = Duration_zero
end
end
User->>AppTimer: stop_timer()
AppTimer-->>MarathonPageState: onTimerStop(totalElapsed)
MarathonPageState->>MarathonPageState: _handleTimerStop(totalElapsed)
alt unsaved_time_remaining
MarathonPageState->>MarathonPageState: _saveElapsedTime(_timeSinceLastSave)
MarathonPageState->>AsyncErrorHandler: executeVoid(AddTaskTimeRecordCommand)
AsyncErrorHandler->>Mediator: send(AddTaskTimeRecordCommand)
Mediator-->>AsyncErrorHandler: command_completed
AsyncErrorHandler-->>MarathonPageState: result
MarathonPageState->>MarathonPageState: _timeSinceLastSave = Duration_zero
end
Sequence diagram for periodic autosave in task details timersequenceDiagram
actor User
participant TaskDetailsContentState
participant AppTimer
participant Mediator
participant TasksService
User->>TaskDetailsContentState: open_task_details()
TaskDetailsContentState->>AppTimer: create_AppTimer(isMiniLayout=true, onTick, onTimerStop, onWorkSessionComplete)
User->>AppTimer: start_timer()
loop every_second_while_running
AppTimer->>TaskDetailsContentState: onTick(displayTime)
TaskDetailsContentState->>TaskDetailsContentState: _handleTimerTick(displayTime)
TaskDetailsContentState->>TaskDetailsContentState: _timeSinceLastSave += 1_second
alt timeSinceLastSave >= 10_seconds
TaskDetailsContentState->>TaskDetailsContentState: _saveTaskTime(_timeSinceLastSave)
TaskDetailsContentState->>Mediator: send(AddTaskTimeRecordCommand)
Mediator-->>TaskDetailsContentState: command_completed
TaskDetailsContentState->>TasksService: notifyTaskUpdated(taskId)
TaskDetailsContentState->>TaskDetailsContentState: _timeSinceLastSave = Duration_zero
end
end
User->>AppTimer: stop_timer_or_work_session_completes
AppTimer-->>TaskDetailsContentState: onTimerStop(totalElapsed) or onWorkSessionComplete(totalElapsed)
TaskDetailsContentState->>TaskDetailsContentState: _onTaskTimerStop(totalElapsed)
alt unsaved_time_remaining
TaskDetailsContentState->>TaskDetailsContentState: _saveTaskTime(_timeSinceLastSave)
TaskDetailsContentState->>Mediator: send(AddTaskTimeRecordCommand)
Mediator-->>TaskDetailsContentState: command_completed
TaskDetailsContentState->>TasksService: notifyTaskUpdated(taskId)
TaskDetailsContentState->>TaskDetailsContentState: _timeSinceLastSave = Duration_zero
end
Updated class diagram for AppTimer and timer consumersclassDiagram
class AppTimer {
+Function Duration onTick
+VoidCallback onTimerStart
+Function Duration onTimerStop
+Function Duration onWorkSessionComplete
+bool isMiniLayout
-Timer _timer
-bool _isRunning
-bool _isAlarmPlaying
-TimerMode _timerMode
-Duration _elapsedTime
-Duration _sessionTotalElapsed
+_startTimer()
+_stopTimer()
+_resetTimer()
}
class MarathonPageState {
-TaskListItem _selectedTask
-bool _isTimerRunning
-bool _isDimmed
-Timer _dimmingTimer
-Duration _timeSinceLastSave
+bool wantKeepAlive
+_onTimerStart()
+_handleTimerTick(Duration displayTime)
+_saveElapsedTime(Duration elapsed)
+_handleTimerStop(Duration totalElapsed)
+_startDimmingTimer()
+_stopDimmingTimer()
}
class TaskDetailsContentState {
-Task _task
-Set~String~ _visibleOptionalFields
-Duration _timeSinceLastSave
+_handleTimerTick(Duration displayTime)
+_saveTaskTime(Duration elapsed)
+_onTaskTimerStop(Duration totalElapsed)
}
class AddTaskTimeRecordCommand {
+String taskId
+int duration
+DateTime customDateTime
}
class Mediator {
+send(AddTaskTimeRecordCommand command)
}
class TasksService {
+notifyTaskUpdated(String taskId)
}
MarathonPageState --> AppTimer : uses
TaskDetailsContentState --> AppTimer : uses
MarathonPageState --> AddTaskTimeRecordCommand : creates
TaskDetailsContentState --> AddTaskTimeRecordCommand : creates
MarathonPageState --> Mediator : sends_command_via_AsyncErrorHandler
TaskDetailsContentState --> Mediator : sends_command
TaskDetailsContentState --> TasksService : notifyTaskUpdated
AppTimer --> MarathonPageState : callback_onTick
AppTimer --> MarathonPageState : callback_onTimerStop
AppTimer --> MarathonPageState : callback_onWorkSessionComplete
AppTimer --> TaskDetailsContentState : callback_onTick
AppTimer --> TaskDetailsContentState : callback_onTimerStop
AppTimer --> TaskDetailsContentState : callback_onWorkSessionComplete
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new periodic save logic in both
MarathonPageandTaskDetailsContentassumes each tick represents one real second (_timeSinceLastSave += const Duration(seconds: 1)), butAppTimeradvances time by 1 minute per second inkDebugMode; consider basing the accumulation on the actual increment (e.g., by passing the increment or total elapsed into the callback) so autosave intervals and durations remain accurate in all modes. - In Pomodoro mode,
onWorkSessionCompletecurrently receives_sessionTotalElapsed, which is only reset in_startTimerand will accumulate across multiple work/break cycles; if the intent is to persist each Pomodoro work block as a separate entry, you may want to track and pass a per-work-session elapsed duration instead of the full session total. - The 10-second autosave interval is hard-coded in multiple places (e.g.,
_timeSinceLastSave.inSeconds >= 10); consider extracting this into a shared constant to avoid drift if the interval needs to change later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new periodic save logic in both `MarathonPage` and `TaskDetailsContent` assumes each tick represents one real second (`_timeSinceLastSave += const Duration(seconds: 1)`), but `AppTimer` advances time by 1 minute per second in `kDebugMode`; consider basing the accumulation on the actual increment (e.g., by passing the increment or total elapsed into the callback) so autosave intervals and durations remain accurate in all modes.
- In Pomodoro mode, `onWorkSessionComplete` currently receives `_sessionTotalElapsed`, which is only reset in `_startTimer` and will accumulate across multiple work/break cycles; if the intent is to persist each Pomodoro work block as a separate entry, you may want to track and pass a per-work-session elapsed duration instead of the full session total.
- The 10-second autosave interval is hard-coded in multiple places (e.g., `_timeSinceLastSave.inSeconds >= 10`); consider extracting this into a shared constant to avoid drift if the interval needs to change later.
## Individual Comments
### Comment 1
<location> `src/lib/presentation/ui/features/tasks/pages/marathon_page.dart:133-135` </location>
<code_context>
- // Timer event handlers
- void _onTaskTimerStop(Duration totalElapsed) {
+ void _handleTimerTick(Duration displayTime) {
+ _timeSinceLastSave += const Duration(seconds: 1);
+ if (_timeSinceLastSave.inSeconds >= 10) {
+ _saveTaskTime(_timeSinceLastSave);
+ _timeSinceLastSave = Duration.zero;
</code_context>
<issue_to_address>
**issue (bug_risk):** Timer tick handler assumes 1s real-time increments, which diverges from AppTimer’s accelerated debug timing.
In debug builds, `AppTimer` advances by 1 minute per tick (`kDebugMode ? Duration(minutes: 1) : Duration(seconds: 1)`), but `_handleTimerTick` always adds `Duration(seconds: 1)` and uses a fixed 10-second threshold. This causes persisted time to be much smaller than the logical elapsed time in debug, and the effective chunk size to vary by build mode. Prefer either deriving the delta from `displayTime` or exposing the timer’s tick duration from `AppTimer` so this code doesn’t hard-code 1-second steps.
</issue_to_address>
### Comment 2
<location> `src/lib/presentation/ui/features/tasks/pages/marathon_page.dart:135-144` </location>
<code_context>
_startDimmingTimer();
}
- Future<void> _handleTimerStop(Duration totalElapsed) async {
- _stopDimmingTimer();
+ void _handleTimerTick(Duration displayTime) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using the same stop handler for `onTimerStop` and `onWorkSessionComplete` also stops dimming between Pomodoro work/break cycles.
Here `_handleTimerStop` (which calls `_stopDimmingTimer`) is used for both `onTimerStop` and `onWorkSessionComplete`. In Pomodoro mode, `onWorkSessionComplete` fires when work ends but the timer immediately continues into a break. If dimming should persist across work→break transitions, these two callbacks need different handlers: one to handle work-segment completion (e.g., flush accumulated time) and another to handle a true timer stop that also cancels dimming.
Suggested implementation:
```
setState(() {
_isTimerRunning = true;
_timeSinceLastSave = Duration.zero;
});
_startDimmingTimer();
}
void _handleTimerTick(Duration displayTime) {
_timeSinceLastSave += const Duration(seconds: 1);
if (_timeSinceLastSave.inSeconds >= 10) {
_saveElapsedTime(_timeSinceLastSave);
_timeSinceLastSave = Duration.zero;
}
}
/// Called when a work session completes (e.g., Pomodoro work → break transition).
/// Flushes any accumulated elapsed time but intentionally does NOT stop dimming,
/// so the dimming persists into the break segment.
Future<void> _handleWorkSessionComplete(Duration totalElapsed) async {
if (_timeSinceLastSave > Duration.zero) {
await _saveElapsedTime(_timeSinceLastSave);
_timeSinceLastSave = Duration.zero;
}
}
/// Called when the timer actually stops (user stops / session ends).
/// Flushes accumulated elapsed time and stops the dimming timer.
Future<void> _handleTimerStop(Duration totalElapsed) async {
_stopDimmingTimer();
if (_timeSinceLastSave > Duration.zero) {
await _saveElapsedTime(_timeSinceLastSave);
_timeSinceLastSave = Duration.zero;
}
}
Future<void> _saveElapsedTime(Duration elapsed) async {
if (_selectedTask == null) return;
```
To fully implement the separation of concerns between work-session completion and true timer stop, you should also:
1. Update the timer configuration where the callbacks are wired (likely where the Pomodoro/marathon timer is created/initialized in this same file):
- Set `onTimerStop` (or equivalent) to `_handleTimerStop`.
- Set `onWorkSessionComplete` (or equivalent) to `_handleWorkSessionComplete`.
- Keep the tick handler (`onTick` / `onTimerTick`) pointing to `_handleTimerTick`.
2. Ensure any previous use of `_handleTimerStop` as a shared handler for both events is replaced so that only true stop events (not work→break transitions) call `_handleTimerStop`. This will prevent dimming from being stopped between Pomodoro work/break cycles.
</issue_to_address>
### Comment 3
<location> `src/lib/presentation/ui/features/tasks/pages/marathon_page.dart:136-128` </location>
<code_context>
+ void _handleTimerTick(Duration displayTime) {
+ _timeSinceLastSave += const Duration(seconds: 1);
+ if (_timeSinceLastSave.inSeconds >= 10) {
+ _saveElapsedTime(_timeSinceLastSave);
+ _timeSinceLastSave = Duration.zero;
+ }
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** Unsaved accumulated time is attributed to whichever task is currently selected, which can shift when changing tasks mid-session.
`_saveElapsedTime` uses `_selectedTask` at save time, not when the elapsed chunk was earned. If the user switches tasks while the timer is running, any pending `_timeSinceLastSave` is logged against the new task, not the one that was active. If this isn’t intentional, consider flushing pending time before updating `_selectedTask`, or tracking elapsed time per active-task interval instead of globally.
</issue_to_address>
### Comment 4
<location> `src/lib/presentation/ui/features/tasks/components/timer.dart:29-37` </location>
<code_context>
- final Function(Duration)? onTimerStop; // For data persistence - receives total elapsed duration
- final bool isMiniLayout; // Use compact layout for detail tables
+ final Function(Duration)? onTimerStop;
+ final Function(Duration)? onWorkSessionComplete;
+ final bool isMiniLayout;
const AppTimer({
super.key,
this.onTick,
this.onTimerStart,
this.onTimerStop,
+ this.onWorkSessionComplete,
this.isMiniLayout = false,
});
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `onWorkSessionComplete` receives the total session elapsed time, not the duration of the just-finished work segment.
In `_handlePomodoroTimerCompletion`, `onWorkSessionComplete` is called with `_sessionTotalElapsed`, which is only reset in `_startTimer`. This means it accumulates time across both work and break segments, so callers can’t derive the duration of the just-finished work session. If the intent is to persist per-work-session data, consider passing the work-segment duration instead, or providing both total-session and current-segment durations.
</issue_to_address>
### Comment 5
<location> `src/lib/presentation/ui/features/tasks/components/timer.dart:310-319` </location>
<code_context>
_timer = Timer.periodic(const Duration(seconds: 1), (timer) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `onTick` provides a logical time value while consumers assume a fixed 1s per tick, which breaks under debug acceleration.
Because `elapsedIncrement` is 1 minute in debug and 1 second in release, any consumer that increments its own counter by a hard-coded 1s per tick will drift from the actual elapsed time. Since `MarathonPage` and `TaskDetailsContent` both do this, consider documenting that `onTick` reflects logical elapsed time and either (a) passing the increment to consumers, or (b) exposing an API to read the effective increment or accumulated elapsed time so callers don’t assume a fixed delta.
Suggested implementation:
```
if (mounted) {
_updateSystemTrayTimer();
if (_tickingEnabled) _startTicking();
if (_keepScreenAwake) {
_wakelockService.enable();
}
// NOTE: onTick observers receive a *logical* elapsed time increment.
// In debug this is 1 minute per tick, in release it is 1 second per tick.
// Callers MUST NOT assume a fixed 1s delta; instead, they should use the
// provided elapsedIncrement to update any local counters.
_timer = Timer.periodic(const Duration(seconds: 1), (timer) {
if (!mounted) return;
final elapsedIncrement = kDebugMode
? const Duration(minutes: 1)
: const Duration(seconds: 1);
setState(() {
if (_timerMode == TimerMode.stopwatch) {
```
To fully implement the recommendation of passing the logical increment to consumers and avoiding hard-coded 1s assumptions, you should also:
1. Update the `onTick` API in this timer widget:
- Change its type from a no-arg callback to accepting the elapsed increment, for example:
- from `final VoidCallback? onTick;`
- to `final void Function(Duration elapsedIncrement)? onTick;`
- Update every invocation inside `timer.dart` from `widget.onTick?.call();` (or similar) to `widget.onTick?.call(elapsedIncrement);` so consumers receive the logical delta on each tick.
- Add documentation to the `onTick` field (or constructor parameter) explaining that:
- It represents **logical elapsed time**, not wall-clock time.
- In debug, the increment is 1 minute per tick; in release, 1 second per tick.
- Callers must use the `elapsedIncrement` parameter instead of assuming a fixed 1 second per tick.
2. Update all consumers (such as `MarathonPage` and `TaskDetailsContent`) that currently:
- Implement an `onTick` callback with no parameters, and
- Increment their own counters by a hard-coded `1` second per tick.
They should instead:
- Accept a `Duration elapsedIncrement` parameter, and
- Increment their timers by `elapsedIncrement` (or use an accumulated value exposed by the timer widget, if you introduce one).
3. Optionally, if other parts of the codebase need to know the effective increment without subscribing to `onTick`, expose a read-only getter on the timer widget or its `State`, e.g.:
- `Duration get logicalTickIncrement => kDebugMode ? const Duration(minutes: 1) : const Duration(seconds: 1);`
and then use that instead of hard-coding `Duration(seconds: 1)` anywhere callers need the increment.
These adjustments will ensure that caller logic stays aligned with the logical elapsed time in both debug and release modes, and will prevent drift caused by assuming a fixed 1-second tick.
</issue_to_address>
### Comment 6
<location> `src/lib/presentation/ui/features/tasks/components/task_details_content.dart:1404-1406` </location>
<code_context>
),
child: AppTimer(
isMiniLayout: true,
+ onTick: _handleTimerTick,
onTimerStop: _onTaskTimerStop,
+ onWorkSessionComplete: _onTaskTimerStop,
),
),
</code_context>
<issue_to_address>
**issue (bug_risk):** Task details timer persistence has the same 1s-per-tick assumption and potential Pomodoro phase ambiguity as the marathon page.
`_handleTimerTick` always adds `Duration(seconds: 1)` to `_timeSinceLastSave` and doesn’t distinguish work vs break. With `onWorkSessionComplete` also calling `_onTaskTimerStop`, this can (1) desync persisted duration from actual elapsed time in debug, and (2) persist break time to the task if the timer runs during breaks. To align with the timer semantics, base the increment on the real elapsed delta (as in `MarathonPage`) and, if needed, only count work-phase time toward the persisted value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/lib/presentation/ui/features/tasks/pages/marathon_page.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/tasks/pages/marathon_page.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/tasks/components/task_details_content.dart
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces a periodic auto-save mechanism for task timers, saving accumulated time every 10 seconds instead of only when the timer stops. This involved adding a _timeSinceLastSave state variable, implementing _handleTimerTick and _saveTaskTime (or _saveElapsedTime) methods in TaskDetailsContentState and _MarathonPageState, and updating the AppTimer widget to expose onTick and onWorkSessionComplete callbacks. Additionally, many redundant comments were removed from the AppTimer widget. The review comments suggest extracting the hardcoded 10-second periodic save interval into a shared named constant, kPeriodicSaveIntervalSeconds, for improved readability and maintainability.
src/lib/presentation/ui/features/tasks/components/task_details_content.dart
Outdated
Show resolved
Hide resolved
src/lib/presentation/ui/features/tasks/pages/marathon_page.dart
Outdated
Show resolved
Hide resolved
- Extract kPeriodicSaveIntervalSeconds constant to remove magic numbers - Fix debug mode timing by using actual elapsed increments from timer - Separate work session complete from timer stop handlers to prevent dimming issues - Flush pending elapsed time before task switching to ensure correct attribution - Update onTick API to pass elapsed increment instead of display time - Add comprehensive documentation for timer callback behavior
- Add _currentWorkSessionElapsed to track work segment duration separately - Update _toggleWorkBreak to pass actual work session duration instead of total session - Reset work session duration appropriately when starting/stopping work sessions - Fixes issue where break time was included in work session duration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
💪 What
🤔 Why
👀 Usage
The periodic time saving works automatically in the background when using the timer feature:
👩🔬 How to validate
Summary by Sourcery
Implement periodic autosaving of task timer progress across marathon and task detail views to improve time tracking reliability.
New Features:
Enhancements: