Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds two related features to the StudySync application: (1) future-date Study Goal planning — allowing users to create and view goals scheduled for upcoming days — and (2) recurring Tasks with a weekly/bi-weekly schedule encoded as a plain-text pattern (e.g. "1:1,3,5" = every week on Mon/Wed/Fri).
Changes:
- Recurring Tasks: A new
recurring_patterncolumn is added to thetaskstable (with migration), a recurrence pattern field and UI controls are added toTask/TaskUpdate/TaskManagementPanel, and arecurringTaskAppliesTohelper is exposed inTaskService. - Future Study Goal Planning: The Add Goal dialog in
StudyPlannerPanelnow has a date picker supporting future dates;StudyService.getStudyGoalsForFutureDateis introduced; bothCalendarViewPanelandDailyViewPanelgain future-date goal display and inline add-goal dialogs. - Calendar UI: Future days are now styled distinctly (green dashed border) and made clickable, previously restricted to past/today only.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
schema.sql |
Adds recurring_pattern column to tasks table and an ALTER TABLE IF NOT EXISTS migration for existing databases |
Task.java |
Adds recurringPattern field, constructors, getters/setters, isRecurring(), getRecurringSummary(), and DB query methods for recurring tasks |
TaskUpdate.java |
Adds recurringPattern to the record and a convenience constructor that preserves existing pattern |
TaskService.java |
Preserves recurringPattern through applyTaskUpdates; adds getRecurringTasks, getActiveRecurringTasks, and recurringTaskAppliesTo |
TaskManagementPanel.java |
Adds recurring task UI controls (checkbox, interval spinner, day checkboxes) to the task form |
StudyService.java |
Adds getStudyGoalsForFutureDate; getTodayGoals now processes delayed goals first |
StudyPlannerPanel.java |
Adds date picker and quick-buttons to the Add Goal dialog enabling future planning |
DailyViewPanel.java |
Routes future-date goal queries to getStudyGoalsForFutureDate |
CalendarViewPanel.java |
Makes future dates clickable, applies green dashed style, adds inline Add Goal dialog for plannable dates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Convenience constructor without recurring pattern (preserves existing). */ | ||
| public TaskUpdate(String title, String description, String category, TaskPriority priority, LocalDate deadline) { | ||
| this(title, description, category, priority, deadline, null); | ||
| } |
There was a problem hiding this comment.
The TaskUpdate convenience constructor (without recurringPattern) passes null as recurringPattern, and the docstring says it "preserves existing". However, this means there is no way for a caller to explicitly clear a task's recurringPattern (set it back to null) through TaskUpdate. The UI in TaskManagementPanel.saveTask() sends null for recurringPattern when the "Recurring task" checkbox is unchecked — but applyTaskUpdates in TaskService treats null as "preserve old value", so removing a recurring schedule from an existing task is broken. Consider using a special sentinel value (e.g. an empty string), or Optional<String>, to allow callers to distinguish "not set by caller" from "intentionally cleared".
src/main/java/com/studysync/presentation/ui/components/TaskManagementPanel.java
Outdated
Show resolved
Hide resolved
| public List<StudyGoal> getTodayGoals() { | ||
| // Ensure delayed goals are properly processed before retrieving | ||
| processAllDelayedGoals(); | ||
| return StudyGoal.findByDate(LocalDate.now()); |
There was a problem hiding this comment.
The @Transactional(readOnly = true) annotation was removed from getTodayGoals(). The method now calls processAllDelayedGoals(), which performs write operations (updates and deletes), so readOnly = true is indeed no longer appropriate. However, the class-level @Transactional is a write transaction by default, so that is fine. This change is intentional, but it is worth noting that getTodayGoals() now unconditionally runs a full scan and update of all goals on every call — which can be expensive if called frequently (e.g. on every UI refresh tick). The same pattern exists in getStudyGoalsForDate() which was already present before this PR. Consider adding a time-based guard (e.g. only process once per day) rather than invoking processAllDelayedGoals() on every retrieval call.
| for (int i = 0; i < dayNums.length; i++) { | ||
| int dayIdx = Integer.parseInt(dayNums[i].trim()) - 1; | ||
| if (dayIdx >= 0 && dayIdx < 7) sb.append(dayNames[dayIdx]); | ||
| if (i < dayNums.length - 1) sb.append(", "); |
There was a problem hiding this comment.
In getRecurringSummary(), the separator ", " is appended based on array position (i < dayNums.length - 1) regardless of whether the day name itself was added to the string (the if (dayIdx >= 0 && dayIdx < 7) guard can suppress it). If a malformed pattern contains an out-of-range day number followed by a valid one (e.g. "1:0,3"), the result will be "Every week on , Wed" — with a leading orphan separator. Tie the separator append to the same dayIdx validity guard to avoid this.
| for (int i = 0; i < dayNums.length; i++) { | |
| int dayIdx = Integer.parseInt(dayNums[i].trim()) - 1; | |
| if (dayIdx >= 0 && dayIdx < 7) sb.append(dayNames[dayIdx]); | |
| if (i < dayNums.length - 1) sb.append(", "); | |
| boolean firstDayAppended = false; | |
| for (int i = 0; i < dayNums.length; i++) { | |
| int dayIdx = Integer.parseInt(dayNums[i].trim()) - 1; | |
| if (dayIdx >= 0 && dayIdx < 7) { | |
| if (firstDayAppended) { | |
| sb.append(", "); | |
| } | |
| sb.append(dayNames[dayIdx]); | |
| firstDayAppended = true; | |
| } |
| * @return list of study goals planned for that date | ||
| */ | ||
| @Transactional(readOnly = true) | ||
| public List<StudyGoal> getStudyGoalsForFutureDate(LocalDate date) { |
There was a problem hiding this comment.
The docstring for getStudyGoalsForFutureDate says the date parameter must be "a future date", but the method does not validate this precondition. If a past or present date is accidentally passed, the method will silently return results using the "no delay processing" path, which could show incorrect data (missing delayed goals for that date). Consider adding an assertion or a runtime check, or rename the method to something more general like getStudyGoalsForDateWithoutDelayProcessing to better reflect that the caller is responsible for ensuring a future date is passed.
| * @return list of study goals planned for that date | |
| */ | |
| @Transactional(readOnly = true) | |
| public List<StudyGoal> getStudyGoalsForFutureDate(LocalDate date) { | |
| * @return list of study goals planned for that date | |
| * @throws ValidationException if {@code date} is not strictly in the future | |
| */ | |
| @Transactional(readOnly = true) | |
| public List<StudyGoal> getStudyGoalsForFutureDate(LocalDate date) { | |
| if (date == null) { | |
| throw new ValidationException("date must not be null"); | |
| } | |
| LocalDate today = LocalDate.now(); | |
| if (!date.isAfter(today)) { | |
| throw new ValidationException( | |
| "getStudyGoalsForFutureDate requires a future date. " + | |
| "Use getStudyGoalsForDate or getTodayGoals for past or present dates." | |
| ); | |
| } |
| TaskPriority priority, LocalDate deadline, TaskStatus status, int points) { | ||
| this(id, title, description, category, priority, deadline, status, points, null); | ||
| } |
There was a problem hiding this comment.
The 8-argument constructor now delegates to the 9-argument constructor with null for recurringPattern. This is referenced in TaskService.addTask — when the incoming task has a null priority, a replacement task is created using the 8-argument constructor, silently dropping the recurring pattern from the original task. To preserve the recurring pattern in that code path, either the 8-argument constructor should not be used at all in addTask (pass recurringPattern explicitly), or this constructor should not silently default it to null. Note that this factory method Task.create(...) also silently creates non-recurring tasks.
| } | ||
|
|
||
| return new Task(task.getId(), newTitle, newDescription, newCategory, newPriority, newDeadline, task.getStatus(), task.getPoints()); | ||
| String newRecurringPattern = update.recurringPattern() != null ? update.recurringPattern() : task.getRecurringPattern(); |
There was a problem hiding this comment.
In applyTaskUpdates, the logic update.recurringPattern() != null ? update.recurringPattern() : task.getRecurringPattern() makes it impossible to explicitly clear a recurring pattern when editing a task. When the user unchecks the "Recurring task" checkbox in the UI, recurringPattern is set to null and passed via TaskUpdate. Since null is the sentinel for "no change," applyTaskUpdates will preserve the old pattern instead of clearing it. A recurring task will therefore remain recurring even after the user has deselected the checkbox and saved.
To support clearing the pattern, a distinct sentinel is needed — for example, an empty string "" could mean "clear the pattern", while null means "keep existing". Alternatively, the UI could use a dedicated Optional<String> field or a marker constant to signal intent to clear.
| String newRecurringPattern = update.recurringPattern() != null ? update.recurringPattern() : task.getRecurringPattern(); | |
| String newRecurringPattern = task.getRecurringPattern(); | |
| if (update.recurringPattern() != null) { | |
| // Convention: | |
| // null -> keep existing pattern (no change) | |
| // "" -> clear existing pattern | |
| // other -> set new pattern | |
| if (update.recurringPattern().isEmpty()) { | |
| newRecurringPattern = null; | |
| } else { | |
| newRecurringPattern = update.recurringPattern(); | |
| } | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Add time-based guard so processAllDelayedGoals runs at most once per day - Fix getRecurringSummary orphan separator on malformed patterns - Add null/future-date validation to getStudyGoalsForFutureDate - Preserve recurringPattern in addTask null-priority fallback path - Use empty-string sentinel to allow clearing recurring pattern on edit - Update README and ARCHITECTURE with recurring tasks, future goal planning, schema migration strategy, and delay processing guard
No description provided.