fix: unify timestamp format to ISO 8601 for correct timezone display#194
Merged
fix: unify timestamp format to ISO 8601 for correct timezone display#194
Conversation
…ne display
timeFormatUTC() was stripping the Z suffix from ISO 8601 strings, causing
dayjs() to interpret them as local time. This made .tz('Asia/Tokyo')
a no-op, displaying UTC values as if they were JST.
- Remove timeFormatUTC and pass ISO 8601 strings through to DB as-is
- Fix timeFormatTz to use dayjs.utc() for correct parsing
- Update all display code to use dayjs.utc() instead of dayjs()
- Add CLAUDE.md section on datetime/timezone conventions
- Add tests for timeFormatTz
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughアプリ全体で日時解析をUTC基準に統一し、その後目的のタイムゾーンへ変換するように変更しました。ドキュメントに「日時・タイムゾーンの原則」を追加し、 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
AGENTS.md is auto-generated by opensrc, so it should not be tracked. Its content (source code reference docs) is now in CLAUDE.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
batch/helper/timeformat.test.ts (1)
5-15: 旧フォーマットの回帰テストも追加したいです。移行期間は
YYYY-MM-DD HH:mm:ss形式の既存データが残り得るので、それも UTC として解釈できることをここで固定しておくと安心です。💡 追加イメージ
describe('timeFormatTz', () => { it('converts ISO 8601 UTC to Asia/Tokyo', () => { expect(timeFormatTz('2026-03-16T02:56:35Z', 'Asia/Tokyo')).toBe( '2026-03-16 11:56:35', ) }) + + it('treats legacy UTC strings without Z as UTC', () => { + expect(timeFormatTz('2026-03-16 02:56:35', 'Asia/Tokyo')).toBe( + '2026-03-16 11:56:35', + ) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/helper/timeformat.test.ts` around lines 5 - 15, 既存のISOテストに加えて旧フォーマット 'YYYY-MM-DD HH:mm:ss' を UTC として解釈する回帰テストを追加してください: batch/helper/timeformat.test.ts にある timeFormatTz を使い、例えば timeFormatTz('2026-03-16 02:56:35', 'Asia/Tokyo') が '2026-03-16 11:56:35' を返すケースと timeFormatTz('2026-03-16 02:56:35', 'US/Pacific') が '2026-03-15 19:56:35' を返すケースを追加して、旧フォーマットを明示的に UTC として扱うことを固定化してください.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/`$orgSlug/analysis/reviews/index.tsx:
- Around line 68-72: sinceDate is computed by subtracting months in local time
then converting to UTC, which makes the day boundary depend on the environment
timezone; change the computation order so the subtraction happens in UTC. Update
the expression that builds sinceDate to call utc() before subtract (e.g. use
dayjs().utc().subtract(periodMonths, 'month').startOf('day').toISOString()) and
ensure the revised sinceDate is what you pass into getQueueHistoryRawData,
getWipCycleRawData, and getPRSizeDistribution.
---
Nitpick comments:
In `@batch/helper/timeformat.test.ts`:
- Around line 5-15: 既存のISOテストに加えて旧フォーマット 'YYYY-MM-DD HH:mm:ss' を UTC
として解釈する回帰テストを追加してください: batch/helper/timeformat.test.ts にある timeFormatTz を使い、例えば
timeFormatTz('2026-03-16 02:56:35', 'Asia/Tokyo') が '2026-03-16 11:56:35'
を返すケースと timeFormatTz('2026-03-16 02:56:35', 'US/Pacific') が '2026-03-15
19:56:35' を返すケースを追加して、旧フォーマットを明示的に UTC として扱うことを固定化してください.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 677f3239-7031-4129-aa45-ae20635e4f35
📒 Files selected for processing (11)
CLAUDE.mdapp/libs/business-hours.tsapp/routes/$orgSlug/+components/pr-block.tsxapp/routes/$orgSlug/analysis/feedbacks/_index/index.tsxapp/routes/$orgSlug/analysis/reviews/index.tsxapp/routes/$orgSlug/workload/$login/index.tsxapp/routes/$orgSlug/workload/+components/team-stacks-chart.tsxbatch/db/mutations.tsbatch/github/review-response.tsbatch/helper/timeformat.test.tsbatch/helper/timeformat.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ency dayjs().subtract().utc() subtracts in local time then converts, making the day boundary environment-dependent. dayjs.utc().subtract() is correct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…amps - batch/bizlogic/cycletime.ts: all diff calculations - throughput queries (merged, deployed, ongoing): hour diff calculations - size-badge-popover, feedback-columns: .fromNow() display - data-management: refreshRequestedAt display - repository-item: pushedAt display - Update all cycletime test fixtures to ISO 8601 format Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Grep-based CI check that detects dayjs(variable) calls that should use dayjs.utc(). Legitimate local-date usages (Date objects, formatted strings) are allowlisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract diffInDays(start, end, businessDaysOnly) to business-hours.ts - Use it in throughput merged/deployed/ongoing queries (removes duplication) - Change new Date().toISOString() to dayjs.utc().toISOString() in ongoing - Remove CI dayjs grep check (CLAUDE.md conventions + code review is enough) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
batch/github/review-response.test.ts (1)
50-56: 固定日付によるテストは意図が伝わりにくい可能性があります。
2025-01-01という固定日付では「90日より古い」というテストの意図がコードから読み取りにくいです。相対日付を使用すると、テストの意図が明確になり、将来の保守性が向上します。♻️ 相対日付を使用した改善案
+import dayjs from 'dayjs' +import utc from 'dayjs/plugin/utc' + +dayjs.extend(utc) + // ... it('filters out comments older than 90 days', () => { + const oldDate = dayjs.utc().subtract(91, 'days').toISOString() const result = analyzeReviewResponse([ - comment('alice', '2025-01-01T10:00:00Z'), - comment('bob', '2025-01-01T12:00:00Z'), + comment('alice', oldDate), + comment('bob', dayjs.utc(oldDate).add(2, 'hours').toISOString()), ]) expect(result).toEqual([]) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@batch/github/review-response.test.ts` around lines 50 - 56, The test using a fixed date makes the "older than 90 days" intent unclear; update the test around analyzeReviewResponse and the test helper comment(...) to use relative dates (e.g., compute Date.now() or mock current time and create timestamps like now minus 91 days and now minus 89 days) so the expectation remains stable over time; alternatively mock the global Date/Date.now in the test and generate comment timestamps based on that mocked "now" to clearly express "older than 90 days" vs "within 90 days."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@batch/github/review-response.test.ts`:
- Around line 50-56: The test using a fixed date makes the "older than 90 days"
intent unclear; update the test around analyzeReviewResponse and the test helper
comment(...) to use relative dates (e.g., compute Date.now() or mock current
time and create timestamps like now minus 91 days and now minus 89 days) so the
expectation remains stable over time; alternatively mock the global
Date/Date.now in the test and generate comment timestamps based on that mocked
"now" to clearly express "older than 90 days" vs "within 90 days."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d131d4d-6fb4-48bc-b012-dee0e871cde3
📒 Files selected for processing (21)
.gitignoreCLAUDE.mdapp/components/size-badge-popover.tsxapp/libs/business-hours.tsapp/routes/$orgSlug/analysis/feedbacks/_index/+components/feedback-columns.tsxapp/routes/$orgSlug/analysis/feedbacks/_index/index.tsxapp/routes/$orgSlug/analysis/reviews/index.tsxapp/routes/$orgSlug/settings/data-management/index.tsxapp/routes/$orgSlug/settings/repositories.add/+components/repository-item.tsxapp/routes/$orgSlug/throughput/deployed/+functions/queries.server.tsapp/routes/$orgSlug/throughput/merged/+functions/queries.server.tsapp/routes/$orgSlug/throughput/ongoing/+functions/queries.server.tsbatch/bizlogic/cycletime.tsbatch/bizlogic/cycletime_codingTime.test.tsbatch/bizlogic/cycletime_deployTime.test.tsbatch/bizlogic/cycletime_pickupTime.test.tsbatch/bizlogic/cycletime_reviewTime.test.tsbatch/bizlogic/cycletime_totalTime.test.tsbatch/db/mutations.tsbatch/github/review-response.test.tsbatch/github/review-response.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- app/routes/$orgSlug/analysis/feedbacks/_index/index.tsx
- batch/github/review-response.ts
- app/routes/$orgSlug/analysis/reviews/index.tsx
Fixed timestamps would eventually expire past the 90-day filter window. Now uses vi.useFakeTimers() with relative daysAgo() helper. Co-Authored-By: Claude Opus 4.6 (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
timeFormatUTC()was stripping the Z suffix from ISO 8601 strings (2026-03-16T02:56:35Z→2026-03-16 02:56:35), causingdayjs()to interpret them as local time. This made.tz('Asia/Tokyo')a no-op — UTC values were displayed as if they were JSTtimeFormatUTCand pass ISO 8601 strings through to DB as-istimeFormatTzand all display code to usedayjs.utc()for correct parsingChanges
batch/db/mutations.tstimeFormatUTC()calls from upsert functionsbatch/helper/timeformat.tstimeFormatUTC, fixtimeFormatTzto usedayjs.utc()app/routes/$orgSlug/workload/$login/index.tsxdayjs()→dayjs.utc()app/routes/$orgSlug/+components/pr-block.tsxdayjs()→dayjs.utc()app/routes/$orgSlug/workload/+components/team-stacks-chart.tsxdayjs()→dayjs.utc()app/libs/business-hours.tsdayjs()→dayjs.utc()app/routes/$orgSlug/analysis/reviews/index.tsx.utc()to date range filterapp/routes/$orgSlug/analysis/feedbacks/_index/index.tsx.utc()to date range filterbatch/github/review-response.tsdayjs()→dayjs.utc()batch/helper/timeformat.test.tstimeFormatTzCLAUDE.mdTest plan
pnpm validatepasses (lint, format, typecheck, build, 200 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
バグ修正
テスト
ドキュメント
雑務