Normalize stored timestamps to UTC and tighten date-filter UX#23
Merged
Conversation
- Store published_date, discovered_date, last_scanned as UTC at write time so lexicographic comparison in --since/--before works across feeds with non-UTC offsets. - Add migration 000003 to rewrite existing rows via strftime to UTC. - Route --since/--before parse errors through printError/markError. - Reject --since > --before with a friendly message. - Cover new behavior with storage + CLI tests, including a legacy-row migration test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds UTC timestamp normalization to the CLI and database layers. A new CLI helper validates and orders ChangesUTC Timestamp Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storage/database_test.go`:
- Around line 527-528: Replace the raw SQL string usages in the tests with
squirrel query builders: instead of calling db.conn.QueryRowContext(ctx, "SELECT
published_date FROM articles WHERE url = ?", url).Scan(&stored) (and the similar
raw queries around the other ranges), build the query using
squirrel.Select("published_date").From("articles").Where(squirrel.Eq{"url":
url}).RunWith(db.conn).QueryRowContext(ctx) (or use ToSql/QueryRowContext
pattern consistent with your test helpers), then Scan(&stored); apply the same
transformation for the other occurrences (the selects/inserts in lines 549-550
and 590-622) so all queries use squirrel builders rather than raw SQL strings,
keeping db.conn, ctx and Scan usage intact.
In `@internal/storage/migrations/000003_normalize_dates_utc.up.sql`:
- Around line 8-18: The UPDATEs that normalize timestamps (on
articles.published_date, articles.discovered_date and blogs.last_scanned) should
be hardened so strftime() failures don't silently set values to NULL; change
each SET to preserve the original value when strftime(...) yields NULL (e.g.,
wrap with COALESCE(strftime(...), <original_column>)) so unparseable legacy
timestamps are retained for investigation rather than overwritten.
- Around line 9-17: The migration currently normalizes timestamps with
strftime('%Y-%m-%dT%H:%M:%fZ') which produces fractional-second shapes that can
differ from Go's time.RFC3339Nano and break lexicographic comparisons; change
the three UPDATEs to a consistent shape by removing fractional seconds (use
strftime('%Y-%m-%dT%H:%M:%SZ')) for published_date, discovered_date and
last_scanned so all stored strings are identical-length RFC3339 UTC timestamps,
avoiding mixed forms like ...00Z vs ...00.000Z.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9b9778c-e8be-44b7-aaa3-24f55f170c5f
📒 Files selected for processing (6)
internal/cli/commands.gointernal/cli/commands_test.gointernal/storage/database.gointernal/storage/database_test.gointernal/storage/migrations/000003_normalize_dates_utc.down.sqlinternal/storage/migrations/000003_normalize_dates_utc.up.sql
- Migration: wrap strftime() in COALESCE so unparseable legacy timestamps are preserved instead of silently nulled, and drop fractional seconds for a uniform fixed-width RFC3339 shape. - App writes: store timestamps via a second-precision UTC layout so the lexicographic comparison in ListArticles is stable across all rows. Parsing still uses RFC3339Nano for legacy/fractional values. - Tests: replace raw SQL in storage tests with squirrel, replay the actual migration file rather than inline SQL, and cover the COALESCE preservation behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Follow-ups to #21:
published_date,discovered_date, andlast_scannedare now stored as UTC at write time, so the lexicographic string comparison used by--since/--beforeis correct for feeds that supply non-UTC offsets.strftime('%Y-%m-%dT%H:%M:%fZ', …)so prior data is normalized in place.--since/--beforeparse errors now flow throughprintError/markError, matching the rest of thearticlescommand.--since > --beforewith a friendly message instead of silently returning an empty list.--beforesemantics remain exclusive, as documented.Test plan
golangci-lint runpassesgotestsum -- ./...passes (132 tests)TestAddArticleStoresPublishedDateInUTC/TestUpdateBlogLastScannedStoresInUTC— JST input is persisted with aZsuffix.TestDateFilterRespectsTimezoneEquivalence— a JST 2024-01-15 02:00 article (UTC 2024-01-14 17:00) is correctly excluded by--since 2024-01-15.TestMigrationNormalizesLegacyTimestampsToUTC— legacy+09:00rows inserted raw are rewritten to UTC by the migration SQL.TestParseDateFilter/TestParseDateRange— empty inputs, valid/invalid formats, only-one-side, equal bounds, and thesince > beforerejection.Summary by CodeRabbit
New Features
--sinceand--beforeflags supporting YYYY-MM-DD formatBug Fixes