refactor([migrate-two-remaining-local-formatters]): consolidate EventDetail/TaskItem formatters into shared utils#475
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates two previously-local UI formatting helpers (event date/time rendering in the calendar event detail panel, and duration estimate rendering in CoS task items) into shared utilities under client/src/utils/formatters.js, reducing name-shadowing and duplication risk across the client.
Changes:
- Added a new shared
formatEventDateTime(dateStr, { allDay })formatter and migratedEventDetail.jsxto use it. - Extended shared
formatDurationMin(minutes, { approximate })to optionally prefix estimated durations with~, and migratedTaskItem.jsxto use the shared helper. - Added
client/src/utils/formatters.test.jscoverage for the new/extended formatter behavior; updated PLAN/changelog entries.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| PLAN.md | Updates the planning tracker to reflect completion and a follow-on formatter migration item. |
| client/src/utils/formatters.test.js | Adds Vitest coverage for formatEventDateTime and { approximate } behavior in formatDurationMin. |
| client/src/utils/formatters.js | Extends formatDurationMin with an options arg; adds new formatEventDateTime export. |
| client/src/components/cos/tabs/TaskItem.jsx | Removes the local duration formatter and uses shared formatDurationMin(..., { approximate: true }). |
| client/src/components/calendar/EventDetail.jsx | Replaces the local formatDateTime with shared formatEventDateTime. |
| .changelog/NEXT.md | Records the formatter consolidation under “Changed”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+271
to
+275
| export function formatEventDateTime(dateStr, options = {}) { | ||
| if (!dateStr) return ''; | ||
| const { allDay = false } = options ?? {}; | ||
| const date = new Date(dateStr); | ||
| if (Number.isNaN(date.getTime())) return ''; |
Owner
Author
There was a problem hiding this comment.
Addressed in d6afe74 — removed the empty-string guard so formatEventDateTime passes any input straight to new Date(...), behavior-identical to the original local formatter (epoch for null, Invalid Date for malformed). The PLAN item required preserving visual semantics exactly, and both call sites always pass a real event time, so this path is unreachable in practice. Kept the options ?? {} null-options crash-guard.
Comment on lines
241
to
249
| * Format a duration in minutes as a human-readable string | ||
| * @param {number} minutes - Duration in minutes | ||
| * @returns {string} Formatted duration (e.g., "30m", "1h 30m", "2h") | ||
| * @param {object} [options] | ||
| * @param {boolean} [options.approximate=false] - Prefix the result with `~` | ||
| * to signal an estimate (e.g., "~1h 30m") for predicted/averaged durations. | ||
| * @returns {string} Formatted duration (e.g., "30m", "1h 30m", "2h", "~2h") | ||
| */ | ||
| export function formatDurationMin(minutes) { | ||
| export function formatDurationMin(minutes, options = {}) { | ||
| if (minutes == null) return ''; |
Comment on lines
+33
to
+57
| describe('formatEventDateTime', () => { | ||
| // Local-time ISO (no trailing Z) so parsing is deterministic relative to | ||
| // the test runtime's timezone. | ||
| const sample = '2026-04-01T13:30:00'; | ||
|
|
||
| it('renders a timed event with short weekday + time', () => { | ||
| expect(formatEventDateTime(sample)).toBe( | ||
| new Date(sample).toLocaleString([], { weekday: 'short', month: 'short', day: 'numeric', hour: 'numeric', minute: '2-digit' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('renders an all-day event as a full weekday + year date', () => { | ||
| expect(formatEventDateTime(sample, { allDay: true })).toBe( | ||
| new Date(sample).toLocaleDateString([], { weekday: 'long', month: 'long', day: 'numeric', year: 'numeric' }) | ||
| ); | ||
| }); | ||
|
|
||
| it('all-day and timed renderings differ', () => { | ||
| expect(formatEventDateTime(sample, { allDay: true })).not.toBe(formatEventDateTime(sample)); | ||
| }); | ||
|
|
||
| it('tolerates a null options argument', () => { | ||
| expect(formatEventDateTime(sample, null)).toBe(formatEventDateTime(sample)); | ||
| }); | ||
| }); |
…Detail/TaskItem formatters into shared utils
Replace EventDetail's local formatDateTime (which shadowed the shared
export with a weekday-led, event-specific shape) with a new shared
formatEventDateTime(dateStr, { allDay }) — the all-day branch reuses
the existing formatDateFull. Replace TaskItem's local formatDurationMin
(every output ~-prefixed) by extending shared formatDurationMin with an
{ approximate } option. Visual semantics preserved exactly; add
formatters.test.js covering both.
…nd log to changelog
…eview — tolerate null options arg
Both reviewers flagged that `{ ... } = {}` only defaults on undefined, so
an explicit null second arg throws. Normalize via `options ?? {}` in both
formatDurationMin and formatEventDateTime, add null-options test coverage.
Defer gemini's formatAttachmentSize→formatBytes consolidation to PLAN.md
(out of scope; differs on the falsy→'' vs '0 B' edge case).
… — keep formatEventDateTime behavior-identical
Copilot noted the empty-string guard for falsy/invalid input diverged
from the original local formatDateTime (which rendered epoch/Invalid Date).
The PLAN item required preserving visual semantics exactly, so drop the
cosmetic guard and pass any input straight to new Date(...) like the
original. Keep the options ?? {} null-options crash-guard. Both call
sites always pass a real event time, so the degenerate path is unreachable.
…iew — JSDoc nullable param + lock passthrough - formatDurationMin JSDoc: param is number|null|undefined (nullish → ''). - Add a test asserting formatEventDateTime passes malformed input straight through (raw toLocaleString), locking the deliberate no-guard behavior.
atomantic
force-pushed
the
claim/migrate-two-remaining-local-formatters
branch
from
4ce391e to
d9174bf
Compare
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
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
Resolves the PLAN.md item
[migrate-two-remaining-local-formatters]— two local format definitions that shadowed/duplicated the sharedclient/src/utils/formatters.jshelpers.EventDetail.jsxdefined a localformatDateTime(dateStr, isAllDay)that shadowed the sharedformatDateTimeexport but produced a different, weekday-led shape. Replaced with a new sharedformatEventDateTime(dateStr, { allDay }):Sat, Apr 1, 1:30 PM)formatDateFull(byte-identical options)TaskItem.jsxdefined a localformatDurationMin(mins)identical to the shared one except every output carried a~estimate prefix. Extended the sharedformatDurationMin(minutes, { approximate })with an opt-in~prefix; the one estimate call site passes{ approximate: true }.Why
Removes the confusing name-shadow of the shared
formatDateTimeand the drift risk of two near-identicalformatDurationMincopies.Behavior
formatEventDateTimeis a behavior-identical migration of the original local formatter: any input is passed straight tonew Date(...), so valid event times format exactly as before, and the degenerate path (malformed/empty →Invalid Date/epoch) is preserved too. Both call sites always pass a real event time, so that degenerate path is unreachable in practice. No UI change.Safety
formatDateTime(2 external callers) is untouched —formatEventDateTimeis a new export.formatDurationMin's new{ approximate }option defaults tofalse, so its existing caller (ReviewTab) is unaffected; both helpers tolerate an explicitnulloptions arg.client/src/utils/formatters.test.jscoversformatDurationMin(default /{ approximate }/ null-options) andformatEventDateTime(timed / all-day / null-options, plus a test that locks the deliberate malformed-input passthrough).TaskItem's localformatAttachmentSizeto sharedformatBytes) deferred to PLAN.md as out-of-scope for this item.