feat: factor weeklyProgressWorkDays into pace calculation#1357
feat: factor weeklyProgressWorkDays into pace calculation#1357pstanton237 wants to merge 6 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 11, 2026, 5:29 PM ET / 21:29 UTC. Summary Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Review detailsBest possible solution: Retry the Codex review after fixing the execution failure. Do we have a high-confidence way to reproduce the issue? Unclear. The review failed before ClawSweeper could establish a reproduction path. Is this the best way to solve the issue? Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction. AGENTS.md: unclear because the file could not be read completely. Codex review notes: model internal, reasoning high; reviewed against 7c279bca0477. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebdb2d2217
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // (workDays=5 → Mon-Fri are work days) | ||
| var dayCursor = windowStart | ||
| while dayCursor < resetsAt { | ||
| let dayEnd = min(dayCursor.addingTimeInterval(daySeconds), resetsAt) |
There was a problem hiding this comment.
Split workday pace at local day boundaries
When the weekly reset is not at local midnight, this loop walks 24-hour slices anchored to windowStart and labels the entire slice by the slice's starting weekday. For example, with a Sunday 20:00 local reset and 5 workdays, Monday 00:00–20:00 is counted as Sunday/non-work, so expected usage stays at 0 for most of Monday and the whole workday curve is shifted by the reset time. Since resetsAt can be any Date, the workday-aware calculation should split intervals on calendar day boundaries before classifying them.
Useful? React with 👍 / 👎.
|
Thanks for pointing me here. I do not want to duplicate your PR, so I took a quick pass over this branch instead. I ran One small consistency edge case I noticed: Tiny fix would be to align the valid range in Happy to send a small patch or paste the exact diff here if useful. The broader maintainer decision still seems to be whether the existing Display setting should now also affect pace semantics. If yes, the setting copy / PR description probably just needs to say that clearly. |
|
Thanks for catching that! Just pushed a fix — |
de889d2 to
1810fb9
Compare
The existing weeklyProgressWorkDays setting only affected visual markers on the progress bar. This change threads the work-days value into UsagePace.weekly() so that the expected usage curve distributes 100% across configured work days only. Non-work days contribute zero expected usage, producing a flat curve on weekends. Users who work Mon-Fri and consume 100% by Friday now see "on pace" instead of a misleading ~29% deficit from the 7-day linear model. Fixes steipete#1356
Address Codex bot review feedback: when the weekly reset time is not at midnight, the previous 24-hour-slice approach could misclassify hours near day boundaries. Now uses calendar.startOfDay to split intervals at local midnight, ensuring each slice is classified by its actual calendar weekday regardless of reset time offset.
Align the valid workDays range (>= 2) with the UI picker (Off/4/5/7) and marker path (2...7) to prevent inconsistent behavior from stale or manual values.
1810fb9 to
6e1b7c5
Compare
|
Rebased and reviewed this contribution, fixed the pre-workday zero-usage edge case, and preserved the contributor commits in #1451. Repository CI would not start for this fork head even after a synchronize event and close/reopen, so the maintainer-hosted draft is now the reviewable continuation. Thanks @pstanton237! |
Live app proof (Mon 22:00 KST, Settings > Display > Work Days = 5)
Same usage, same time — the 5-day model distributes expected usage across configured work days only.
Summary
Threads the existing
weeklyProgressWorkDayssetting into the pace calculation so the expected usage curve distributes 100% across only the configured work days. Non-work days contribute zero expected usage, making the pace indicator accurate for users who only work on weekdays.workDaysparameter toUsagePace.weekly()workdayAwareExpected()which walks calendar day boundaries and classifies each slice by its actual calendar weekday (Mon=1..Sun=7, workday if isoWeekday <= workDays)weeklyProgressWorkDaysfrom settings through all call sitesworkDaysis nil, 7, or window is not 10080 minutesBefore / After
After-fix proof (test run output)
Verified on macOS 15.4, Swift 6.1,
swift test --filter UsagePaceTests(2026-06-08).Compatibility note
This PR intentionally reuses the existing
weeklyProgressWorkDayssetting (Off/4/5/7 in Display preferences). Users who previously set this value for visual markers will now also get workday-aware pace. This is the natural intent of the setting — a user who configured "5 days" wants their quota framed around a 5-day work week, both visually and in pace calculation. If maintainers prefer a separate opt-in, this can be split, but we believe the unified setting is the correct UX.When Codex historical pace is active (
historicalTrackingEnabled), it takes precedence over the workday model because it already incorporates actual usage patterns. The workday model serves as the baseline for users without historical data or for non-Codex providers.Test plan
workday aware pace shows on track for five day user on friday— verifies 7-day model reports deficit, 5-day model reports on-paceworkday aware pace shows on track midweek— Wed 18:00 with 60% used ≈ on pace for 5-day modelworkday aware pace falls back to linear when workDays is nil or 7workday aware pace ignores non weekly windows— session windows unaffectedUsagePaceTestsandHistoricalUsagePaceTestspass unchangedFixes #1356
Follow-up to #1096 (visual segmentation)