From cfc29015846927a9d29f79966410e55b72e21caf Mon Sep 17 00:00:00 2001 From: Jack Cuthbert Date: Wed, 19 Aug 2026 15:01:28 +1000 Subject: [PATCH 1/3] fix(client): read a due time that follows a due date in the same todo Typing "next week when 3pm" now sets both the day and the time. It used to set the day and leave "3pm" in the title, describing a due date the todo did not have. --- apps/client/src/todos/lib/quick-add.test.ts | 11 ++ apps/client/src/todos/lib/quick-add.ts | 115 +++++++++++--------- docs/specs/quick-add.md | 8 ++ 3 files changed, 82 insertions(+), 52 deletions(-) diff --git a/apps/client/src/todos/lib/quick-add.test.ts b/apps/client/src/todos/lib/quick-add.test.ts index cf3dc7b..b752b7a 100644 --- a/apps/client/src/todos/lib/quick-add.test.ts +++ b/apps/client/src/todos/lib/quick-add.test.ts @@ -460,4 +460,15 @@ describe('regressions', () => { expect(result.listId).toBe('s') expect(result.summary).toBe('Buy milk') }) + + it('takes a day from one phrase and a time from another', () => { + // Reported from use 2026-08-19: "next week when 3pm" set the week and + // silently dropped the time, leaving "3pm" sitting in the summary + // describing a due the todo did not have. Only the first chrono match + // per segment was read, so the second was thrown away before the day + // and time could be taken from different phrases. + const result = parseQuickAdd('Call them next week when 3pm', [], NOW) + expect(result.due).toEqual({ date: '2026-08-21', time: '15:00' }) + expect(result.summary).toBe('Call them when') + }) }) diff --git a/apps/client/src/todos/lib/quick-add.ts b/apps/client/src/todos/lib/quick-add.ts index 4feb5c5..6ac8e74 100644 --- a/apps/client/src/todos/lib/quick-add.ts +++ b/apps/client/src/todos/lib/quick-add.ts @@ -281,58 +281,69 @@ export function parseQuickAdd( // is reported and no date text is stripped from the summary // (see `QuickAddOptions.noDates`). for (const segment of options.noDates ? [] : gapsBetween(text, tokens)) { - const [parsed] = chrono.parse(segment.text, now, { forwardDate: true }) - if (!parsed) continue - const start = parsed.start - // **"Now" is not a due date.** A todo scheduled for this instant is - // overdue as soon as it exists, which nobody means, so a match that - // resolves to the reference instant is discarded rather than written. - // - // This also removes a whole family of false positives rather than - // listing them. chrono's casual parser reads a determiner followed by - // a unit letter — "the s", "a s", "the m" — as *now*, so typing "sort - // out the shed" invented a due date of today at the current minute the - // moment "the s" was on screen, then took those words out of the - // summary. Every one of those resolves to the reference instant, so - // one rule about meaning catches them all; matching the phrases would - // have been a list that the next casual pattern escapes. - // *(added 2026-08-14, found in review.)* - if (isNow(start, now)) continue - // The same family, for the few that land a minute out rather than on - // the instant — see `isPhantomDate`. - if (isPhantomDate(parsed.text)) continue - // `isCertain('hour')` is the all-day/timed distinction - // (docs/specs/todos.md — due times): a date chrono inferred rather - // than read is not a time the user asked for. - const namesTime = start.isCertain('hour') - if (namesTime) clock ??= start.date() - // A bare time ("3pm") also resolves to a day — today's — which must - // not outrank an explicit one stated elsewhere in the line. Only a - // match that actually names a day sets it. - const namesDay = - start.isCertain('day') || - start.isCertain('weekday') || - start.isCertain('month') - if (namesDay) day ??= start.date() - // **A match that sets nothing is not a match.** chrono recognises - // spans that name no date component at all — "this week" and "this - // year" both come back with an empty `knownValues` — so neither of - // the two branches above fires. Highlighting it anyway marked the - // words and then took them out of the summary, leaving a todo that - // had lost "this week" and gained no due date. - // - // "This week" is ambiguous in any case: chrono resolves it to - // tomorrow, which is nobody's reading of it. Leaving the words in the - // title is the honest outcome. - // *(added 2026-08-17, reported from use.)* - if (!namesDay && !namesTime) continue - // Offset back into the original string, since chrono indexed the - // segment rather than the whole line. - tokens.push({ - kind: 'date', - start: segment.offset + parsed.index, - end: segment.offset + parsed.index + parsed.text.length, - }) + // **Every match in the segment, not just the first.** `day` and + // `clock` are separate for exactly this case — a line can name a day + // in one phrase and a time in another — but taking only + // `chrono.parse(...)[0]` threw the second away before either could see + // it. "next week when 3pm" set the week and dropped the time, leaving + // "3pm" in the summary reporting a due the todo did not have. The + // `??=` below still means the first day and the first time win, which + // is the rule that was always intended. + // *(fixed 2026-08-19, reported from use.)* + for (const parsed of chrono.parse(segment.text, now, { + forwardDate: true, + })) { + const start = parsed.start + // **"Now" is not a due date.** A todo scheduled for this instant is + // overdue as soon as it exists, which nobody means, so a match that + // resolves to the reference instant is discarded rather than written. + // + // This also removes a whole family of false positives rather than + // listing them. chrono's casual parser reads a determiner followed by + // a unit letter — "the s", "a s", "the m" — as *now*, so typing "sort + // out the shed" invented a due date of today at the current minute the + // moment "the s" was on screen, then took those words out of the + // summary. Every one of those resolves to the reference instant, so + // one rule about meaning catches them all; matching the phrases would + // have been a list that the next casual pattern escapes. + // *(added 2026-08-14, found in review.)* + if (isNow(start, now)) continue + // The same family, for the few that land a minute out rather than on + // the instant — see `isPhantomDate`. + if (isPhantomDate(parsed.text)) continue + // `isCertain('hour')` is the all-day/timed distinction + // (docs/specs/todos.md — due times): a date chrono inferred rather + // than read is not a time the user asked for. + const namesTime = start.isCertain('hour') + if (namesTime) clock ??= start.date() + // A bare time ("3pm") also resolves to a day — today's — which must + // not outrank an explicit one stated elsewhere in the line. Only a + // match that actually names a day sets it. + const namesDay = + start.isCertain('day') || + start.isCertain('weekday') || + start.isCertain('month') + if (namesDay) day ??= start.date() + // **A match that sets nothing is not a match.** chrono recognises + // spans that name no date component at all — "this week" and "this + // year" both come back with an empty `knownValues` — so neither of + // the two branches above fires. Highlighting it anyway marked the + // words and then took them out of the summary, leaving a todo that + // had lost "this week" and gained no due date. + // + // "This week" is ambiguous in any case: chrono resolves it to + // tomorrow, which is nobody's reading of it. Leaving the words in the + // title is the honest outcome. + // *(added 2026-08-17, reported from use.)* + if (!namesDay && !namesTime) continue + // Offset back into the original string, since chrono indexed the + // segment rather than the whole line. + tokens.push({ + kind: 'date', + start: segment.offset + parsed.index, + end: segment.offset + parsed.index + parsed.text.length, + }) + } } // A time with no day means today — chrono's own reading, and the one a diff --git a/docs/specs/quick-add.md b/docs/specs/quick-add.md index 52abfcb..e6f8e62 100644 --- a/docs/specs/quick-add.md +++ b/docs/specs/quick-add.md @@ -381,6 +381,14 @@ became a todo called "Do the thing", due never. have set. Leaving the words in the title is the honest outcome: the todo says what you typed. *(fixed 2026-08-17, reported from use.)* +**A day and a time can come from different phrases.** "Call them next +week when 3pm" sets both, because the day and the time are tracked +separately and the first of each wins. Only the first chrono match in a +segment used to be read, so the second was discarded before either could +be taken from it — the line above set the week, dropped the time, and left +"3pm" in the summary describing a due the todo did not have. +*(fixed 2026-08-19, reported from use.)* + **Offline** changes nothing. The parse is entirely client-side, and the created todo queues through the outbox like any other ([sync-and-offline](./sync-and-offline.md)). From 1f662440291af7437313e3b423ad2beff06952e3 Mon Sep 17 00:00:00 2001 From: Jack Cuthbert Date: Wed, 19 Aug 2026 15:01:52 +1000 Subject: [PATCH 2/3] feat(client): wrap long todos in quick add instead of hiding them A long line now wraps onto as many lines as it needs, so text can no longer be pushed out of sight by choosing a list or a date. Recognised words are marked with a little more room around them, the list suggestions follow the cursor, and a tall modal moves up the screen rather than pushing its own buttons out of view. Summaries are capped at 500 characters, which keeps typing quick however much text is pasted in. --- .../src/todos/lib/editable-caret.test.ts | 98 ++++ apps/client/src/todos/lib/editable-caret.ts | 158 ++++++ .../quick-add-field.module.css | 86 +++ .../todos/quick-add-field/quick-add-field.tsx | 345 ++++++++++++ .../quick-add-modal.module.css | 187 +++--- .../todos/quick-add-modal/quick-add-modal.tsx | 532 +++++++++++------- docs/specs/quick-add.md | 212 ++++++- e2e/tests/mobile.spec.ts | 14 +- e2e/tests/quick-add.spec.ts | 187 +++++- 9 files changed, 1477 insertions(+), 342 deletions(-) create mode 100644 apps/client/src/todos/lib/editable-caret.test.ts create mode 100644 apps/client/src/todos/lib/editable-caret.ts create mode 100644 apps/client/src/todos/quick-add-field/quick-add-field.module.css create mode 100644 apps/client/src/todos/quick-add-field/quick-add-field.tsx diff --git a/apps/client/src/todos/lib/editable-caret.test.ts b/apps/client/src/todos/lib/editable-caret.test.ts new file mode 100644 index 0000000..380323a --- /dev/null +++ b/apps/client/src/todos/lib/editable-caret.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, it } from 'vitest' +import type { QuickAddToken } from './quick-add' +import { tokensChanged } from './editable-caret' + +// docs/specs/quick-add.md — what a contenteditable costs. This predicate +// is the whole undo story: rewriting `innerHTML` empties the browser's +// undo stack, so the marks are redrawn only when the token set actually +// changes and left alone for every keystroke that does not move one. +// False here means the DOM is never touched, which is what keeps ⌘Z +// native for ordinary typing. +// +// The caret helpers beside this one are deliberately *not* unit tested: +// what they assert is DOM behaviour, the client has no DOM test +// environment (every test here is a `.ts` over extracted logic), and +// adding one for two functions would be a fourth test arrangement in a +// repo that already has three. They are covered in e2e/tests/quick-add, +// against a real browser. + +const token = ( + kind: QuickAddToken['kind'], + start: number, + end: number, +): QuickAddToken => ({ kind, start, end }) + +describe('tokensChanged', () => { + it('is false for the same tokens, so typing does not redraw', () => { + const text = 'Clean the gutters tomorrow at 3pm #Chores' + const before = [token('date', 18, 33), token('list', 34, 41)] + const after = [token('date', 18, 33), token('list', 34, 41)] + expect(tokensChanged(before, after, text, text)).toBe(false) + }) + + it('is false for two empty sets', () => { + expect(tokensChanged([], [], '', '')).toBe(false) + }) + + it('is true when a token appears', () => { + // Typing the "m" that completes "3pm": this is the one keystroke + // that costs an undo entry, and it has to redraw to earn it. + const before = [token('list', 34, 41)] + const after = [token('date', 18, 33), token('list', 34, 41)] + const text = 'Clean the gutters tomorrow at 3pm #Chores' + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is true when a token disappears', () => { + const text = 'Clean the gutters tomorrow at 3pm #Chores' + const before = [token('date', 18, 33), token('list', 34, 41)] + const after = [token('date', 18, 33)] + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is false when a token only slides along the line', () => { + // Typing a character *before* a token shifts its offsets without + // changing which words are marked. Found in the browser 2026-08-19: + // comparing raw offsets made this the common case rather than the + // rare one — every keystroke in the prose before a token forced a + // redraw, and with it lost that keystroke's undo entry, which is + // exactly what this predicate exists to prevent. What matters is + // which text is marked, not where it sits. + const text = 'Clean the gutters tomorrow at 3pm' + const shifted = 'Clean the Zgutters tomorrow at 3pm' + expect( + tokensChanged( + [token('date', 18, 33)], + [token('date', 19, 34)], + text, + shifted, + ), + ).toBe(false) + }) + + it('is true when a token moves onto different words', () => { + const before = [token('date', 0, 8)] + const after = [token('date', 9, 17)] + expect( + tokensChanged(before, after, 'tomorrow p1 today', 'tomorrow today p1'), + ).toBe(true) + }) + + it('is true when a token grows', () => { + // "tomorrow" becoming "tomorrow at 3pm" — same start, same kind, and + // the mark has to stretch. + const text = 'Clean the gutters tomorrow at 3pm' + const before = [token('date', 18, 26)] + const after = [token('date', 18, 33)] + expect(tokensChanged(before, after, text, text)).toBe(true) + }) + + it('is true when only the kind differs', () => { + // Same span, different meaning. Nothing in the app produces this + // today, but the mark is drawn from the token, so a set that differs + // only by kind is still a set that has changed. + const before = [token('date', 0, 2)] + const after = [token('priority', 0, 2)] + expect(tokensChanged(before, after, 'p1 milk', 'p1 milk')).toBe(true) + }) +}) diff --git a/apps/client/src/todos/lib/editable-caret.ts b/apps/client/src/todos/lib/editable-caret.ts new file mode 100644 index 0000000..53b42e5 --- /dev/null +++ b/apps/client/src/todos/lib/editable-caret.ts @@ -0,0 +1,158 @@ +import type { QuickAddToken } from './quick-add' + +// docs/specs/quick-add.md — what a contenteditable costs. +// +// The caret in a contenteditable is a DOM position: a node plus an offset +// into it. Everything else in quick add addresses text by a plain offset +// into the string — `replaceToken` returns one, the parser's tokens are +// `slice`-compatible ranges — so these two functions are the whole +// translation layer between the two worlds, and nothing above them has to +// know a Range exists. +// +// The `` this replaced had `setSelectionRange` for the same job. + +/** + * Where the caret is, as an offset into the element's plain text. + * + * `null` when the selection is somewhere else on the page, which is the + * ordinary case while a pill menu is open. + * + * Measured by asking a Range how much text precedes the caret rather than + * by walking and summing node lengths ourselves: a Range counts exactly + * what `innerText` would return over the same span, so the two agree even + * when the marks split the text into many nodes. + */ +export function caretOffset(el: HTMLElement): number | null { + const selection = document.getSelection() + if (!selection || selection.rangeCount === 0) return null + + const range = selection.getRangeAt(0) + if (!el.contains(range.endContainer)) return null + + const toCaret = range.cloneRange() + toCaret.selectNodeContents(el) + toCaret.setEnd(range.endContainer, range.endOffset) + return toCaret.toString().length +} + +/** + * Put the caret at a plain-text offset, counting through the text nodes. + * + * `>=` rather than `>` when finding the node: an offset landing exactly on + * a node's end belongs to that node, at its final position. Preferring the + * *next* node would put the caret on the far side of a mark's boundary, + * so typing after a token would land inside it. + * + * An offset past the end clamps to the last text node rather than throwing + * — the text can only have shrunk under a caret we are restoring, and the + * end of the line is the sane place to be. + */ +export function placeCaret(el: HTMLElement, offset: number): void { + const walker = document.createTreeWalker(el, NodeFilter.SHOW_TEXT) + let seen = 0 + let last: Node | null = null + + // `SHOW_TEXT` guarantees every node here is a Text, but the DOM types + // say `Node` — narrowed by checking rather than asserted, so this stays + // honest if the filter ever changes. + for (let node = walker.nextNode(); node; node = walker.nextNode()) { + const length = node.textContent?.length ?? 0 + if (seen + length >= offset) { + select(node, offset - seen) + return + } + seen += length + last = node + } + + // Nothing long enough: an empty field has no text node at all, so put + // the caret in the element itself rather than giving up. + if (last) select(last, last.textContent?.length ?? 0) + else select(el, 0) +} + +function select(node: Node, offset: number): void { + const range = document.createRange() + range.setStart(node, offset) + range.collapse(true) + const selection = document.getSelection() + selection?.removeAllRanges() + selection?.addRange(range) +} + +/** + * Must the marks be redrawn? + * + * **This is what keeps undo native.** Writing `innerHTML` empties the + * browser's undo stack — measured 2026-08-19, and the reason `⌘Z` does + * nothing at all in a naive highlighting editor. Answering `false` here + * leaves the DOM untouched, so the browser's own editing, and its undo + * history along with it, is never disturbed. + * + * **Compares the marked *words*, not their offsets.** Offsets were the + * first implementation and were wrong in use: typing anywhere before a + * token shifts every later token's `start` and `end`, so a keystroke in + * the middle of the summary counted as a change and forced a redraw. That + * made the redraw the common case rather than the rare one and cost an + * undo entry per keystroke — the exact failure this predicate exists to + * prevent. The marks are drawn around text, and text that has only slid + * along the line is still in the right DOM node. + * *(fixed 2026-08-19, found in the browser.)* + */ +export function tokensChanged( + before: readonly QuickAddToken[], + after: readonly QuickAddToken[], + beforeText: string, + afterText: string, +): boolean { + if (before.length !== after.length) return true + return before.some((token, index) => { + const other = after[index] + return ( + !other || + token.kind !== other.kind || + beforeText.slice(token.start, token.end) !== + afterText.slice(other.start, other.end) + ) + }) +} + +/** + * Where the caret is on screen, in viewport coordinates. + * + * `null` when the selection is outside `el`, or when the browser reports + * no rect at all — a collapsed range at the very start of an empty element + * gives an empty list rather than a zero-width rect. + * + * A collapsed range has no dimensions in some engines, so this expands it + * by one character where it can and measures that instead. The + * `getClientRects()[0]` path covers the ordinary case, and is preferred + * because it is the caret's own line rather than a character beside it — + * which matters on a wrapped field, where the two can be different lines. + */ +export function caretRect( + el: HTMLElement, +): { bottom: number; left: number } | null { + const selection = document.getSelection() + if (!selection || selection.rangeCount === 0) return null + + const range = selection.getRangeAt(0) + if (!el.contains(range.endContainer)) return null + + const direct = range.getClientRects()[0] + if (direct) return { bottom: direct.bottom, left: direct.left } + + // Collapsed with no rect of its own: measure the character before the + // caret, which sits on the same line, and take its trailing edge. + const probe = range.cloneRange() + const offset = range.endOffset + if (offset > 0) { + probe.setStart(range.endContainer, offset - 1) + const before = probe.getClientRects()[0] + if (before) return { bottom: before.bottom, left: before.right } + } + + // Nothing measurable — an empty field. Its own box is the best answer. + const box = el.getBoundingClientRect() + return { bottom: box.top + el.clientHeight, left: box.left } +} diff --git a/apps/client/src/todos/quick-add-field/quick-add-field.module.css b/apps/client/src/todos/quick-add-field/quick-add-field.module.css new file mode 100644 index 0000000..c8decc8 --- /dev/null +++ b/apps/client/src/todos/quick-add-field/quick-add-field.module.css @@ -0,0 +1,86 @@ +/* docs/specs/quick-add.md — the field wraps, and grows. + See quick-add-field.tsx. */ + +/* The line you type a todo into. + * + * One element, not two. It was an `` under a shadow layer holding + * the same text at the same metrics, which is what made the marks + * unpaddable: padding a token in the shadow moved that layer's text and + * nothing else. A contenteditable has no second layer to fall out of + * register with, so the marks below can be drawn as marks rather than as + * a box-shadow trick. + * + * It wraps and grows without bound — no `max-height`, no scrolling. A cap + * here would put the text back behind a scroll edge, which is the thing + * this change exists to remove; the *modal* carries the cap instead + * (quick-add-modal.module.css — `.popup`), so the pills and the buttons + * stay reachable while every line of the todo stays visible. */ +.field { + font: inherit; + font-size: var(--text-lg); + line-height: 1.5; + padding: var(--space-2); + border: 0; + /* No focus ring, matching the notes field below it and the `` this + replaced. + * + The global `:focus-visible` rule (styles/global.css) draws a 2px accent + outline on anything focusable, and exempts `input`, `select` and + `textarea` — the elements that carry a border to change instead. A + contenteditable `div` is in none of those lists, so it picked up a ring + the old field never had, around the one element in this modal that is + always focused the moment it opens. Two framed fields stacked is the + form this modal exists to replace. + *(added 2026-08-19, reported from use.)* */ + outline: none; + color: var(--ink); + caret-color: var(--ink); + /* `pre-wrap` keeps the runs of spaces someone types between tokens, and + wraps at the edge. `break-word` is the guard against a single + unbroken 200-character string — a pasted URL — widening the modal + past the viewport instead of breaking. */ + white-space: pre-wrap; + overflow-wrap: break-word; + /* An empty field is still a line tall, so the modal does not jolt when + the first character lands. */ + min-height: calc(1.5 * var(--text-lg)); +} + +.field:focus-visible { + outline: none; +} + +/* The placeholder, drawn rather than declared: `::placeholder` applies to + form controls, and this is not one. `:empty` is exactly right here — + the element holds one text node while it has any text at all, and none + when it does not. */ +.field:empty::before { + content: attr(data-placeholder); + color: var(--faint); + /* Without this the caret sits *after* the placeholder rather than at + the start of the line. */ + pointer-events: none; +} + +/* A recognised token: *marked* text, not a chip. + * + * Padded and softly rounded, which the shadow-layer arrangement could not + * do at any price — see the spec. `--radius-sm` rather than + * `--radius-full`: a pill-shaped run of words sitting in a line of prose + * reads as an object you could pick up and move, and this is your own + * text that the parser has understood, not a chip. + * + * Tinted `--accent` at the same 12% the preview pills use: the mark and + * the pill below it are two views of one token, so they share a hue. */ +.token { + color: var(--accent); + background: color-mix(in srgb, var(--accent) 12%, transparent); + padding: 2px 5px; + border-radius: var(--radius-sm); + /* A mark that wraps across two lines is drawn complete on both. Without + this the padding and the rounded corners are applied to the whole + inline box once, so the fragment on the second line loses its left + edge and the first loses its right. */ + box-decoration-break: clone; + -webkit-box-decoration-break: clone; +} diff --git a/apps/client/src/todos/quick-add-field/quick-add-field.tsx b/apps/client/src/todos/quick-add-field/quick-add-field.tsx new file mode 100644 index 0000000..a46d1b9 --- /dev/null +++ b/apps/client/src/todos/quick-add-field/quick-add-field.tsx @@ -0,0 +1,345 @@ +import { + useLayoutEffect, + useRef, + type ClipboardEvent, + type FormEvent, + type KeyboardEvent, + type ReactNode, + type RefObject, +} from 'react' +import { caretOffset, placeCaret, tokensChanged } from '../lib/editable-caret' +import type { QuickAddToken } from '../lib/quick-add' +import styles from './quick-add-field.module.css' + +/** + * The longest a summary may be. + * + * A todo's summary is a *title*; prose belongs in the notes field below, + * which is what that field is for. Without a bound the cost of every + * keystroke grows with the text — the whole line is re-parsed and the + * marks can be redrawn — which was measured at ~12ms per keystroke at + * 4,000 characters on a fast machine, so several times that on an + * ordinary one. 500 is far past any real todo and short enough that the + * per-keystroke work stays flat. + * *(added 2026-08-19, reported from use: holding ⌘V made typing crawl.)* + */ +const MAX_SUMMARY = 500 + +interface QuickAddFieldProps { + value: string + onChange: (value: string) => void + onKeyDown: (event: KeyboardEvent) => void + /** Whether the field holds focus, so the caller can close its menus. */ + onFocusChange: (focused: boolean) => void + placeholder: string + tokens: readonly QuickAddToken[] + /** Where a pill rewrite wants the caret, or null to leave it alone. */ + caretTo: RefObject + fieldRef: RefObject +} + +/** + * The line you type a todo into (docs/specs/quick-add.md — the field + * wraps, and grows). + * + * A contenteditable rather than a form control, for one reason: the + * recognised tokens are marked *inside* the text, and a padded mark is + * only possible when the marks are real elements in the element you type + * into. The `` this replaced kept its marks in a shadow layer + * underneath, which cannot pad a token without sliding that layer out of + * register with the text above it — measured at 34px across three tokens + * then, and 52px again against a wrapping `