-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add capture date display and user goals documentation #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Show verification/crawl timestamps in citation UX: "Verified [date]" for documents, "Retrieved [date+time]" for URLs. Add user goals progressive disclosure documentation to AGENTS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Refactor IIFE in CitationDrawer to computed value for readability - Add JSDoc documenting timezone behavior (display=local, tooltip=UTC) - Improve CaptureTimestamp tests to assert rendered content via DOM queries - Add edge case tests: leap day, future dates, old dates, date-only strings - Add formatCaptureDate to canonical locations table in CLAUDE.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
89c49c4 to
9a65a35
Compare
Code Review: Capture Date Display FeatureOverall Assessment✅ High quality implementation with excellent documentation, comprehensive test coverage, and thoughtful UX design. The code follows the project's conventions and the progressive disclosure documentation in AGENTS.md is particularly valuable. Strengths1. Excellent Documentation
2. Comprehensive Test Coverage
3. Type Safety
4. Clean Code Structure
Issues & Recommendations🔴 Critical: Inconsistent Date Type HandlingLocation: verifiedAt?: Date; // Line 89
crawledAt?: Date | string | null; // Line 124Problem:
Impact:
Recommendation: // Make types consistent
verifiedAt?: Date | string | null;
crawledAt?: Date | string | null;Or add a comment explaining why they differ. 🟡 Minor: Hardcoded LocaleLocation: new Intl.DateTimeFormat("en-US", dateFormatOptions).format(parsed);Issue: Hardcoded Recommendation: // Use browser locale for date formatting
new Intl.DateTimeFormat(undefined, dateFormatOptions).format(parsed);
// or
new Intl.DateTimeFormat(navigator.language, dateFormatOptions).format(parsed);Trade-off: Using user locale may make snapshots less stable, but improves UX for international users. 🟡 Minor: Missing Test for Timezone Edge CasesLocation: Missing coverage:
Recommendation: it("display uses local timezone, tooltip uses UTC", () => {
const result = formatCaptureDate("2026-01-15T23:30:00Z");
expect(result\!.tooltip).toContain("23:30"); // UTC time preserved
// Display time depends on TZ, but we can verify it's formatted
expect(result\!.display).toMatch(/Jan\s+(15|16)/); // Could be 15 or 16 depending on TZ
});🟢 Nitpick: Component Placement ConsistencyLocation: The Current structure:
This is probably fine since fallback has no image, but worth noting for future consistency. 🟢 Nitpick: Test Timeout Magic NumberLocation: await new Promise((r) => setTimeout(r, 50));Consider extracting this to a named constant: const POPOVER_SETTLE_TIME_MS = 50;
await new Promise((r) => setTimeout(r, POPOVER_SETTLE_TIME_MS));Security & Performance✅ No security concerns - only formatting display values, no XSS vectors Compliance with CLAUDE.md✅ Canonical location registered in CLAUDE.md table Recommended Actions
ConclusionThis is a well-crafted PR that adds valuable UX context to citations. The documentation is exemplary and will help future maintainers. The only critical issue is the type inconsistency in Recommendation: Approve with minor changes (fix type consistency, consider locale improvements) |
✅ Playwright Test ReportStatus: Tests passed 📊 Download Report & Snapshots (see Artifacts section) What's in the Visual SnapshotsThe gallery includes visual snapshots for:
Run ID: 21898075186 |
Summary
formatCaptureDateutility and register it in CLAUDE.md canonical locationsChanges
Capture Date Display (
src/react/dateUtils.ts,CitationComponent.tsx,CitationDrawer.tsx)formatCaptureDate()utility usingIntl.DateTimeFormat— same-year dates omit the year, URL citations include time, tooltip provides full ISO 8601 for audit hoverCaptureTimestampsub-component inserted into all three popover states (success, partial/miss, fallback) between the proof image and verification logcrawledAtUser Goals Documentation (
AGENTS.md)Documents WHY users interact with citations at each progressive disclosure level:
Tests (
src/__tests__/dateUtils.test.ts,CaptureTimestamp.test.tsx)formatCaptureDate: null/invalid handling, year logic, showTime, edge cases (leap day, future dates, 1970, date-only strings)CaptureTimestamp: asserts rendered DOM content via popover interaction — verifies labels, date format, title attributes, crawledAt preferenceTest plan
bun test— 1052 tests pass (0 failures)npm run build— ESM, CJS, DTS, CSS all succeednpm run size— ~340 bytes added to react bundle (pre-existing size limit overage, not caused by this PR)npm run test:ct— Playwright snapshot tests (expect diffs on popovers with date fields in verification fixtures)