feat(tasks): add reusable task sorting - #666
Conversation
|
doistbot
left a comment
There was a problem hiding this comment.
Adds a reusable sortTasks utility with comprehensive sorting logic, tests, and proper string-union conventions.
Few things worth tightening:
- Floating due times ignore
task.due.timezone: When a task has a floating datetime (noZ/offset) and itsdue.timezonediffers from the account timezone, the sort uses the wrong frame of reference. Convert the wall-clock time fromtask.due.timezone(falling back to the account timezone when null) into a common frame before computing theDateSortValue. childOrderskipped for same-project tasks withoutprojectOrder: WhenTaskSortContextis absent and two tasks share a project with equal priority/dates, manual order (childOrder) is not applied—tasks stay in input order instead. Usetask.projectIdequality to applychildOrderwithin a project while keeping the existing guard for cross-project tasks without ranks.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (7)
src/utils/task-sorting.test.ts:4:
makeTaskre-lists everyTaskfield, duplicating the shared fixtures insrc/test-utils/test-defaults.ts.TASK_WITH_OPTIONALS_AS_NULLalready matches these defaults (null section/parent/deadline/duration/due, empty labels, priority 1), so this can be{ ...TASK_WITH_OPTIONALS_AS_NULL, ...overrides }plus overrides forid/content/url.sanitization.test.tsalready spreadsDEFAULT_TASKthis way; reusing the fixture keeps the Task shape in one place.src/utils/task-sorting.ts:189: The "null sorts last" guard is duplicated verbatim in
compareDate,compareOptionalNumber, andcompareOptionalText. Extract a singlecompareNullable(a, b, compare)helper and pass each field-specific comparator into it, so the same 4-line branch isn't maintained three times.src/utils/task-sorting.ts:337:
directionis recomputed inside the sort comparator on every comparison (O(n log n) times), but it only depends onoptions.sortOrderandsortedBy, both fixed per call. Compute it once before.sort(...)and capture it in the closure.src/utils/task-sorting.ts:238:
assigneeNameis invoked for every task even when the result is never read — only theASSIGNEEbranch ofcomparePrimaryusesassignee. For any othersortedBy(including the default hierarchies) this runs the user-supplied callback n times for nothing. Resolve it lazily, e.g. only whensortedBy === 'ASSIGNEE'.src/utils/task-sorting.test.ts:234:
projectOrderandworkspaceOrdercontain the exact same keys and values, so this loop cannot tell which rank source eachsortedByactually reads. A swap incomparePrimary(WORKSPACE readingprojectOrder, or PROJECT readingworkspaceOrder) would still pass. Give the two maps different orders and assert that PROJECT and WORKSPACE produce different results.src/utils/task-sorting.test.ts:213: The three fields in this loop all order the tasks first/second/third in lockstep (due Jan/Feb/Mar, deadline Apr/May/Jun, addedAt Jan 1/2/3), so a copy-paste bug where DUE_DATE, DEADLINE, or ADDED_DATE compares the wrong attribute is undetectable. Use fixtures where at least one field disagrees on order, or assert a distinct expected order per field.
src/utils/task-sorting.test.ts:432:
expect(result[0]).toBe(result[2])pins the fact thatsortTasksreturns the same object references, which is not part of the documented contract (only that it returns a new array). A future defensive copy of task objects would fail here for no behavioral reason. The stable-order behavior is already covered by thetoEqualassertion; drop the identity check.
c0a5ee9 to
993aa7e
Compare
|
I am keeping this as one PR. The sorter is one atomic module, and most of the review load is focused edge-case coverage. Splitting the implementation from its tests would create an artificial boundary. This PR is already the second layer of a stack. |
993aa7e to
249abd4
Compare
249abd4 to
034c38e
Compare
doistbot
left a comment
There was a problem hiding this comment.
This PR adds a pure, non-mutating sortTasks utility that implements Todoist's default and named ordering (date, timezone, priority, manual) using consumer-supplied context for project/workspace ranks, assignee names, timezone, and locale.
Few things worth tightening:
- When sorting fixed datetimes, preserve the original instant rather than reducing it to account-local wall-clock fields. During a DST fall-back hour, two distinct UTC times can map to the same local time in the wrong order, so keep an epoch-based key for timed values (handling date-only tasks separately) to maintain chronological correctness.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (9)
src/utils/task-sorting.ts:73:
isValidDatePartsandcomponentsAsUtcTimestamphand-roll UTC timestamp construction withnew Date(0)plussetUTCFullYear/setUTCHours.Date.UTC(year, month - 1, day, hour, minute, second, millisecond)does the same in one call, and reading backgetUTCFullYear/getUTCMonth/getUTCDatestill validates the calendar. Inputs always come from the(\d{4})regex, so the 0-99 two-digit-year quirk does not apply.src/utils/task-sorting.test.ts:12: The task URL is rebuilt from the base URI literal instead of reusing
getTaskUrl(overrides.id)from./url-helpers. Other fixtures (todoist-api.tasks.test.ts,todoist-api.move-tasks.test.ts) construct URLs viagetTaskUrl; hardcoding the domain here duplicates that source of truth.src/utils/task-sorting.ts:138:
parseDateTimeComponentsbuilds a throwawayDateSortValueonly to reusedateSortValue's validation, then discards it. Callers (parseFloatingDateTime,parseZonedFloatingDateTime) re-derive the value from the returned components, so the value is effectively constructed twice and the validity logic is split from the rest of the parsing. Extract the date-part and time-range checks into a small predicate shared bydateSortValueandparseDateTimeComponentsinstead.src/utils/task-sorting.ts:250: The four-way decision in
parseTaskDate(date-only vs. explicit-offset fixed vs. zoned floating vs. plain floating) is a nested ternary chain that is easy to misread. Split it into an early-returnif/elsesequence (or a smallswitch) so each datetime format gets a distinct, self-explanatory branch.src/utils/task-sorting.ts:65:
DATE_TIME_FORMATTERSis a process-lifetime, module-levelMapkeyed by consumer-supplied timezone strings — bothcontext.timezone(line 411) and each task'sdue.timezone(line 257) — and it is never evicted or size-bounded.due.timezoneis typed as plainstring(not validated IANA), so in a long-running SDK consumer distinct values accumulateIntl.DateTimeFormatinstances indefinitely. Bound the cache (LRU/max size) or cache only the account timezone and let per-task source timezones be transient.src/utils/task-sorting.ts:227:
parseFixedDateTimerunsparseDateTimeComponentspurely to validate and then discards the result; that call extracts every field, builds aDateSortValue, and range-validates it.Date.parseanddateSortValueAtthen re-parse and re-validate the same string. Use a regex-only format check (e.g.DATE_TIME_PATTERN.test(...)) so each fixed datetime is parsed once instead of twice.src/utils/task-sorting.ts:424:
comparePrimary({ sortedBy, a, b, collator })allocates a new object literal on every sort comparison — O(n log n) allocations that could be zero. ChangecomparePrimaryto accept the four values as positional parameters instead of a destructured object, e.g.comparePrimary(sortedBy, a, b, collator). The call site is the sort comparator, so this runs for every pairwise comparison.src/utils/task-sorting.ts:175:
componentsAtcallspart()six times, each doing a full.find()scan of theformatToPartsarray. A single loop overpartsthat populates a lookup object (orMap) would do one pass instead of six. This function is called up to 5 times per zoned floating-date task (4 iterations intimestampForWallClock+ 1 indateSortValueAt).src/utils/task-sorting.ts:410:
Intl.Collatoris constructed for everysortTaskscall, but it is only read forALPHABETICALLYandASSIGNEEsorting. Default, priority, date, project, and workspace sorts pay this setup cost unnecessarily. Construct it only for the two text-basedsortedByvalues (or lazily on first use).
034c38e to
146f13f
Compare
Co-authored-by: Craig Carlyle <craigcarlyle@me.com>
Every other exported shape in src/ is declared with export type, so bring TaskSortOptions and TaskSortContext in line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2010062 to
85871de
Compare
|
🎉 This PR is included in version 14.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
sortTasksutility that uses the existing SDK sorting types.Example
Validation
todoist-cli#479locally against a packed build of this SDK stack. Its full test suite passed.Context
This is the second PR in the stack and depends on #665. Consumers must fetch all pages before using it for globally correct sorting.
Related CLI work:
Stack created with GitHub Stacks CLI • Give Feedback 💬