fix(date-range): unify 'all' period semantics between CLI and dashboard#221
Conversation
`getDateRange` was duplicated across `src/cli.ts` and `src/dashboard.tsx`
with conflicting semantics for `'all'`. The CLI intentionally bounded
`'all'` to the last 6 months (justified inline: keeps Codex/Cursor parses
responsive on sparse multi-year history). The dashboard returned
`new Date(0)` instead, so the same `--period all` flag silently meant
two different windows depending on which entry point you hit.
`Period`, `PERIODS`, `PERIOD_LABELS`, and `toPeriod` were duplicated as
well, and `cli-date.ts` already existed for date helpers
(`parseDateRangeFlags`) so the consolidation lives there.
Both call sites now go through a single `getDateRange(period: string)`
in `cli-date.ts` that returns `{ range, label }`. The dashboard wraps it
as `getPeriodRange(period: Period)` to keep the strict `Period` type at
the React boundary while letting the CLI continue to accept extras like
`'yesterday'`.
`PERIOD_LABELS.all` becomes `'6 Months'` (short, for the dashboard tab
strip; the previous `'All Time'` was misleading and the long-form
`'Last 6 months'` from `getDateRange().label` already drives CLI output).
Changes:
- src/cli-date.ts: add `Period`, `PERIODS`, `PERIOD_LABELS`, `toPeriod`,
`getDateRange`. Pull the existing 6-month rationale into a named
`ALL_TIME_MONTHS` constant.
- src/cli.ts: drop the local copies and import from cli-date.
- src/dashboard.tsx: drop the local copies, route through
`getPeriodRange`, alias the shared `getDateRange` import to
`getDateRangeShared` to avoid shadowing the wrapper.
- tests/cli-date.test.ts: 13 cases covering `'all'` regression guard
(must never silently fall back to `Date(0)`), CLI/dashboard agreement,
end-of-month clamping tolerance, `'yesterday'` support, and
unknown-input fallback.
- README.md, CHANGELOG.md: surface the bound and point heavy users at
`--from`/`--to` for unbounded windows.
The CLI flag `--period all` continues to be accepted; only the dashboard
window changes to match what the CLI was already doing. No public API
or schema change.
Refs getagentseal#93
6c6c2be to
3dc3e32
Compare
iamtoruk
left a comment
There was a problem hiding this comment.
Deep review across CLI/menubar impact, data accuracy, security, and code quality.
Impact
CLI: No behavioral change. The CLI already capped 'all' to 6 months.
macOS menubar / GNOME extension: No change. Both call codeburn status --period all which was already bounded to 6 months. Their local tab labels ('All') are separate constants and unaffected.
Dashboard only: Intentional fix. new Date(0) -> 6-month cap.
Security
No vulnerabilities found. toPeriod whitelists inputs with safe fallback, getDateRange has a default branch for defense in depth, the ISO regex is anchored (no ReDoS), no dynamic property access with user input, no prototype pollution vectors.
Minor note: cli-date.ts importing toDateString from daily-cache.ts pulls in transitive deps on crypto, fs, os. Not exploitable, but if this module is ever used in a browser context it would break. Consider inlining toDateString (it's a 1-line formatter) or extracting it to a tiny shared module.
Data accuracy bug: end-of-month overflow
The 'all' case computes:
const start = new Date(now.getFullYear(), now.getMonth() - ALL_TIME_MONTHS, now.getDate())JS rolls overflow days into the next month. On Aug 31: new Date(2026, 2, 31) becomes Mar 3 -- silently excluding Feb 28, Mar 1, Mar 2 (3 days of data). On Oct 31: skips Apr 30. On May 31: skips Nov 30.
The data isn't deleted (it's in the cache), but the date range excludes it. The test accepts a 5-7 month tolerance which masks this.
Fix: Use day 1 of the target month:
const start = new Date(now.getFullYear(), now.getMonth() - ALL_TIME_MONTHS, 1)This always produces a valid date and gives users slightly more than 6 months (safer direction for a cost tracker).
Code quality nits
-
Unnecessary alias:
getDateRange as getDateRangeSharedin dashboard.tsx adds noise. The wrapper is calledgetPeriodRange-- there's no name conflict. Just importgetDateRangedirectly. -
Tautological test: The "CLI and dashboard agree" test calls
getDateRange('all')twice from the same function and asserts equality. It tests idempotency, not cross-module agreement. Consider renaming it to match what it actually verifies, or making it an integration test that imports throughdashboard.tsx's wrapper. -
gnome/prefs.jsstill says "All Time" (line 29): After this PR the canonical label is "6 Months". Not blocking, but a follow-up for consistency.
Verdict
The consolidation is well-executed and the fix is correct. The PR description is excellent (alternatives considered, backward compat notes, test plan). The one material issue is the end-of-month overflow -- please fix before merge. The rest are minor suggestions.
|
Thanks for the deep review. You were right about the end-of-month overflow: using the current day when subtracting 6 months can roll into the next month and silently exclude a few days. I pushed a follow-up fix in 9a258a8:
Local verification:
GitHub checks are green too. Could you take another look when you have a chance? |
There was a problem hiding this comment.
clean refactoring, solid tests, well-documented rationale.
One thing to address before merge: the label unification is incomplete across all surfaces. Please ensure "All Time" / "All" is updated to "6 Months" in all of these:
- TUI dashboard (
src/dashboard.tsx) -PERIOD_LABELS.alland the hotkey hint bar (line ~611 hardcodes "all time") - macOS menubar (
mac/Sources/CodeBurnMenubar/AppStore.swift) -Periodenumcase all = "All"(line 331) - macOS menubar phrase (
mac/Sources/CodeBurnMenubar/Views/MenuBarContent.swift) —-periodPhrasereturns"all time"for.all - GNOME indicator (
gnome/indicator.js) - line 21 still says{ id: 'all', label: 'All' } - GNOME prefs (
gnome/prefs.js) - you got this one already
Otherwise users will see "All" or "All Time" on some surfaces while the behavior is actually capped to 6 months —-misleading for a cost tracker where accuracy matters.
PR #221 unified the period logic but missed the TUI hotkey bar, GNOME indicator popup, and macOS menubar app. All surfaces now consistently show '6 Months' instead of 'All' or 'all time'.
Summary
getDateRangewas duplicated acrosssrc/cli.tsandsrc/dashboard.tsxwith conflicting semantics for'all':'all'to the last 6 months (justified inline: keeps Codex/Cursor parses responsive on sparse multi-year history).new Date(0)instead, so the same--period allflag silently meant two different windows depending on which entry point you hit.Period,PERIODS,PERIOD_LABELS, andtoPeriodwere duplicated as well.cli-date.tsalready existed for date helpers (parseDateRangeFlags), so the consolidation lives there.After this PR
'all'consistently means Last 6 months in both CLI and dashboard.'All Time'to'6 Months'(short form for tight UI; long-form'Last 6 months'fromgetDateRange().labelcontinues to drive CLI text output).--from/--toremains the escape hatch for users who need an unbounded historical window.src/cli-date.ts.Changes
src/cli-date.ts: addPeriod,PERIODS,PERIOD_LABELS,toPeriod,getDateRange. Pull the existing 6-month rationale into a namedALL_TIME_MONTHSconstant.src/cli.ts: drop the local copies and import fromcli-date.src/dashboard.tsx: drop the local copies, route through a thingetPeriodRange(period: Period)wrapper to keep the strictPeriodtype at the React boundary while letting the CLI continue to accept extras like'yesterday'. Aliased the sharedgetDateRangeimport togetDateRangeSharedto avoid shadowing the wrapper.tests/cli-date.test.ts: 13 new cases covering:'all'regression guard (must never silently fall back toDate(0))'all''yesterday'supportREADME.md,CHANGELOG.md: surface the bound and point heavy users at--from/--to.Backward compatibility
The CLI flag
--period allcontinues to be accepted. Only the dashboard window changes to match what the CLI was already doing. No public API or schema change.Alternatives considered
allto6months: rejected for backward compatibility. Users have shell aliases and menubar configs passing--period all. Cappingallseamlessly + documenting--from/--toin the changelog felt less disruptive.--period all: rejected as overkill for a tactical bug fix. Open to revisit if maintainers prefer.Notes for reviewer
'all'is touched. The provider filter'all'(e.g.dashboard.tsx:672,cli.ts--providerdefault) is unrelated and unchanged.'all'should remain bounded to 6 months. This PR intentionally preserves the CLI's existing bounded behavior and fixes the dashboard drift. Users who need true historical ranges can use--from/--to.Test plan
Refs #93