Skip to content

Resolve 4 correctness bugs (IRL-21, 22, 23, 24)#34

Merged
alecvdp merged 6 commits intomainfrom
alecvdpoel/irl-correctness-bug-bundle
Apr 16, 2026
Merged

Resolve 4 correctness bugs (IRL-21, 22, 23, 24)#34
alecvdp merged 6 commits intomainfrom
alecvdpoel/irl-correctness-bug-bundle

Conversation

@alecvdp
Copy link
Copy Markdown
Owner

@alecvdp alecvdp commented Apr 16, 2026

Summary

Bundles fixes for four correctness bugs filed in this morning's review:

  • IRL-23 — Drag-and-drop in TasksWidget was reading from initialTasks (server-rendered prop) rather than optimisticTasks. With more optimistic updates landing over time, this would silently clobber unsaved state.
  • IRL-21syncRecurringTasks() was a findMany + Promise.all(update) with no concurrency guard. Two simultaneous page loads could both decide a task was due and reset it twice. Now a single atomic updateMany.
  • IRL-22 — Date handling was inconsistent: server parsed YYYY-MM-DD as local time (depended on whether the server ran in PT vs UTC); client compared in browser-local TZ. Now: calendar dates are always stored as UTC noon (unambiguous, readable as same calendar day in any TZ from UTC-12 to UTC+12), recurrence math uses setUTCDate so DST doesn't drift the anchor, and the diff helper is shared. Bonus: EventsWidget.getDaysUntil was using Math.abs and labeling overdue items as "In N days" — now correctly says "Yesterday" / "N days ago".
  • IRL-24 — Pastebin accepted any URL-shaped string as <a href>, including data:, javascript:, file:. Now an http/https allowlist enforced both server-side (rejected at the boundary) and defensively in the renderer.

Two new shared helpers: src/lib/dates.ts and src/lib/url.ts. Net –34 lines after deduping.

Test plan

  • npm run lint — clean
  • npm run build — green
  • Manually create a recurring task, complete it, advance time past nextResetAt, refresh — task reopens once and stays reopened
  • Create a task with a due date near midnight local TZ — label says "today" / "tomorrow" as expected
  • Drag a task between buckets — the move sticks and doesn't revert any in-flight optimistic edit
  • Try pasting javascript:alert(1) into Pastebin — link is rejected (no DB row)
  • Existing pastebin links continue to render and click through correctly

🤖 Generated with Claude Code


Open with Devin

…ing, URL allowlist

- IRL-23: TasksWidget drag-and-drop now reads from optimisticTasks instead
  of the server-rendered initialTasks prop, so optimistic edits aren't
  silently overwritten by a subsequent drag.
- IRL-21: syncRecurringTasks() collapsed from findMany + Promise.all(update)
  into a single atomic prisma.task.updateMany so concurrent page loads can't
  reset a recurring task twice.
- IRL-22: New src/lib/dates.ts with shared parseCalendarDate, addCalendarDays,
  daysUntilCalendarDate, etc. Server now parses YYYY-MM-DD as UTC noon
  (was: local time, depended on server TZ). Recurrence math uses setUTCDate
  so DST doesn't drift the anchor. EventsWidget.getDaysUntil now correctly
  labels past dates ("Yesterday" / "N days ago" instead of "In N days").
- IRL-24: New src/lib/url.ts enforces an http/https allowlist for pasted
  links. addLink rejects non-http schemes (data:, javascript:, file:, etc.)
  before they reach the DB; PastebinWidget re-normalizes defensively when
  rendering hrefs.

Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 16, 2026 16:45
@linear
Copy link
Copy Markdown

linear Bot commented Apr 16, 2026

Adds vitest as a devDependency with `npm test` / `npm run test:watch` scripts,
and a CI workflow that runs lint + tests + build on every PR and push to main.

Initial test coverage targets the highest-value pure-logic surfaces:

- src/lib/dates.ts (15 tests) — UTC anchoring, DST transitions, round-trip
  with formatCalendarDateForInput, addCalendarDays/Months across DST, and
  the Math.round-vs-floor case in daysUntilCalendarDate that would otherwise
  collapse a 23-hour DST diff to 0 days.
- src/lib/url.ts (7 tests) — http/https allowlist, javascript:/data:/file:
  rejection, bare-hostname https:// prepending.
- src/lib/tasks.ts (8 tests) — assignee/priority/bucket/recurrence
  normalizers, including the legacy "WIFE" → "SHARED" fallback.

CI uses a throwaway SQLite db (file:./prisma/ci.db) and runs `prisma migrate
deploy` before the build — IRL-13 (build broken from unapplied migrations)
would have been caught the moment it was pushed with this in place.

Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR bundles fixes for four correctness/security issues across tasks, events, and pastebin links by centralizing calendar-date math, hardening URL handling, and correcting optimistic state usage in drag-and-drop.

Changes:

  • Add shared helpers for calendar-date parsing/formatting and day-diff calculations, then adopt them in widgets and task actions.
  • Make recurring task resets atomic via a single updateMany to avoid double-resets under concurrent requests.
  • Harden pastebin link handling by normalizing/allowlisting http/https URLs server-side and defensively re-normalizing in the renderer; fix drag-and-drop to use optimisticTasks.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/lib/url.ts Introduces URL normalization + protocol allowlist for external links
src/lib/dates.ts Adds shared “calendar date” parsing/formatting and day-diff helpers
src/components/TasksWidget.tsx Uses shared date helpers; fixes drag-and-drop to read from optimistic state
src/components/PastebinWidget.tsx Uses URL normalizer for safe href rendering
src/components/EventsWidget.tsx Uses shared day-diff helper and fixes overdue labeling
src/app/actions/tasks.ts Uses shared date helpers; makes recurring reset atomic via updateMany
src/app/actions/links.ts Enforces URL allowlist at the server action boundary before persisting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib/url.ts
Comment on lines +11 to +16
// Default to https:// when the user typed a bare hostname like "example.com".
// Any other scheme (including javascript:, data:, etc.) gets parsed as-is and
// then rejected unless it's http or https.
const candidate = /^[a-z][a-z0-9+.-]*:/i.test(trimmed)
? trimmed
: `https://${trimmed}`;
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizeExternalUrl treats any something: prefix as a URL scheme. That causes bare hostnames with ports (e.g. localhost:3000, example.com:8443) to be interpreted as a non-http(s) protocol and rejected. Consider changing the scheme detection so host:port is still normalized to https://host:port (e.g., require :// for scheme detection, or attempt https:// prefix when parsing fails and the input doesn't start with http:/https:).

Suggested change
// Default to https:// when the user typed a bare hostname like "example.com".
// Any other scheme (including javascript:, data:, etc.) gets parsed as-is and
// then rejected unless it's http or https.
const candidate = /^[a-z][a-z0-9+.-]*:/i.test(trimmed)
? trimmed
: `https://${trimmed}`;
// Default to https:// when the user typed a bare hostname like "example.com"
// or "localhost:3000". Only treat inputs with an explicit "://" scheme as
// already absolute URLs (plus plain http:/https: prefixes), so host:port
// values are normalized instead of being mistaken for custom protocols.
const candidate =
/^https?:/i.test(trimmed) || /^[a-z][a-z0-9+.-]*:\/\//i.test(trimmed)
? trimmed
: `https://${trimmed}`;

Copilot uses AI. Check for mistakes.
Comment thread src/app/actions/links.ts
Comment on lines +31 to +34
if (!rawUrl || !title) return;

const url = normalizeExternalUrl(rawUrl);
if (!url) return;
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addLink silently returns when the URL fails normalization (normalizeExternalUrl(rawUrl) returns null). In the current client flow, that looks like a successful submit (the form resets) even though no DB row was created. Consider returning a success/error result (or throwing a validation error that the client can surface) so the UI can avoid clearing the form and can show an “invalid URL” message.

Suggested change
if (!rawUrl || !title) return;
const url = normalizeExternalUrl(rawUrl);
if (!url) return;
if (!rawUrl || !title) {
throw new Error("URL and title are required");
}
const url = normalizeExternalUrl(rawUrl);
if (!url) {
throw new Error("Invalid URL");
}

Copilot uses AI. Check for mistakes.
Comment on lines 61 to 67
{initialLinks.map(link => (
<div key={link.id} className={`${styles.linkCard} ${link.isPinned ? styles.pinned : ""}`}>
<a
href={formatUrl(link.url)}
href={safeHref(link.url)}
target="_blank"
rel="noopener noreferrer"
className={styles.linkArea}
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safeHref falls back to "#" when a stored URL can’t be normalized, but the link still renders with target="_blank". Clicking opens a new tab to the same page (with #) which is a confusing failure mode. Consider rendering non-clickable content (or removing target/adding onClick preventDefault) when normalization fails.

Copilot uses AI. Check for mistakes.
devin-ai-integration[bot]

This comment was marked as resolved.

IRL-26: Vitest unit tests + GitHub Actions CI
devin-ai-integration[bot]

This comment was marked as resolved.

alecvdp and others added 3 commits April 16, 2026 10:41
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@alecvdp alecvdp merged commit 2bea4eb into main Apr 16, 2026
2 of 3 checks passed
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 11 additional findings in Devin Review.

Open in Devin Review

const diffTime = Math.abs(eventDate.getTime() - today.getTime());
const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24));

const diffDays = daysUntilCalendarDate(date);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 daysUntilCalendarDate gives wrong day count for event dates that aren't UTC-noon calendar dates

The new daysUntilCalendarDate function (src/lib/dates.ts:57) extracts the target's calendar day using UTC methods (getUTCFullYear, getUTCMonth, getUTCDate). This is correct for task due dates, which are now pinned to UTC noon via parseCalendarDate. However, in EventsWidget.tsx:90, it is called on event dates, which are stored as local-time datetimes (src/app/actions/events.ts:24-26 creates them via new Date(\${dateStr}T${timeStr}`)with noZ` suffix — local time parsing).

For evening events in negative-UTC-offset timezones (e.g., US Eastern/Pacific), the UTC date rolls to the next calendar day. Example: an event at 10pm EDT on April 17 is stored as 2026-04-18T02:00:00Z; getUTCDate() returns 18, so the countdown shows "Tomorrow" instead of "Today".

This also creates a visible inconsistency: the date box on EventsWidget.tsx:129 uses getDate() (local time → correct day), while the countdown uses UTC (→ wrong day).

The old code was correct: it zeroed local time on both dates via setHours(0,0,0,0).

Prompt for agents
The daysUntilCalendarDate function in src/lib/dates.ts is designed for calendar dates pinned to UTC noon (task due dates). It reads the target's calendar day via getUTCDate() etc. But EventsWidget.tsx:90 calls it on event dates, which are local-time datetimes (created in src/app/actions/events.ts:24-26 as new Date(dateStr + T + timeStr) without a Z suffix). For evening events in negative-UTC-offset timezones, getUTCDate() returns the next day, making the countdown off by one.

Two possible approaches:
1. Revert EventsWidget's getDaysUntil to use local-time comparison (setHours(0,0,0,0) on both dates), since event dates are local-time concepts, not UTC-noon calendar dates.
2. Add a separate helper (e.g. daysUntilLocalDate) that uses getFullYear/getMonth/getDate on both sides, and use it in EventsWidget. Keep daysUntilCalendarDate for task due dates in TasksWidget.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants