fix: address PR 793 review comments#796
Merged
Merged
Conversation
The admin UI uses i18next, but several user-management views were still
calling toLocaleDateString("ja-JP") / toLocaleString("ja-JP") directly.
When the admin language is switched to English, dates and counts kept
rendering in Japanese format, defeating the i18n setup.
- admin/src/lib/dateUtils.ts: add getActiveLocale() and formatNumber(),
make formatDate() honour the active i18n locale.
- UsersContent.tsx / UserCard.tsx: route the three remaining hardcoded
ja-JP call sites through these helpers (table row pageCount, mobile
card pageCount, and the delete-impact lastAiUsageAt date).
- dateUtils.test.ts: cover ja/en locale switching and formatNumber.
- UsersContent.test.tsx: keep the mock backwards compatible with the
existing "1,234" assertion under the test setup's forced ja locale.
Addresses devin-ai PR #793 review.
Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughLocale-aware formatting added: ChangesLocale-aware Date and Number Formatting
Graceful Shutdown Consolidation
Tiptap Heading Clamp Behavior
Docs / JSDoc Clarification
Sequence Diagram(s)sequenceDiagram
participant UI
participant DateUtils
participant i18n
UI->>DateUtils: render -> formatDate/formatNumber(value)
DateUtils->>i18n: read language
i18n-->>DateUtils: language (e.g., "ja" / "en")
DateUtils->>DateUtils: derive getActiveLocale()
DateUtils-->>UI: formatted string (locale-aware)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 18 minutes and 46 seconds. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic localization for date and number formatting by replacing hardcoded "ja-JP" strings with a new getActiveLocale utility and adding corresponding tests. The review feedback suggests improving consistency in UsersContent.tsx by using the formatDate helper instead of a manual toLocaleDateString call, which would also permit the removal of the getActiveLocale import in that file.
| } from "@zedi/ui"; | ||
| import type { UserAdmin, UserRole, UserStatus } from "@/api/admin"; | ||
| import { formatDate } from "@/lib/dateUtils"; | ||
| import { formatDate, formatNumber, getActiveLocale } from "@/lib/dateUtils"; |
There was a problem hiding this comment.
getActiveLocale は formatDate 内部で使用されており、コンポーネント側で直接呼び出す必要がなくなるため、インポートから削除できます。
Suggested change
| import { formatDate, formatNumber, getActiveLocale } from "@/lib/dateUtils"; | |
| import { formatDate, formatNumber } from "@/lib/dateUtils"; |
Comment on lines
388
to
390
| date: new Date(confirm.deleteTarget.impact.lastAiUsageAt).toLocaleDateString( | ||
| "ja-JP", | ||
| getActiveLocale(), | ||
| ), |
There was a problem hiding this comment.
既に formatDate ユーティリティが定義されているため、ここでもそれを利用することでコードの重複を避け、日付形式（0埋めなど）の統一性を保つことができます。また、formatDate は内部で getActiveLocale() を呼び出しており、不正な日付入力に対するハンドリングも含まれているため、より安全です。
date: formatDate(confirm.deleteTarget.impact.lastAiUsageAt),
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
admin/src/pages/users/UsersContent.tsx (1)
20-20: ⚡ Quick winUse
formatDatefor delete-impact date to keep date output consistent.This path currently skips the shared formatting options/invalid-date handling used elsewhere.
Proposed refactor
-import { formatDate, formatNumber, getActiveLocale } from "@/lib/dateUtils"; +import { formatDate, formatNumber } from "@/lib/dateUtils"; ... - date: new Date(confirm.deleteTarget.impact.lastAiUsageAt).toLocaleDateString( - getActiveLocale(), - ), + date: formatDate(confirm.deleteTarget.impact.lastAiUsageAt),Also applies to: 388-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin/src/pages/users/UsersContent.tsx` at line 20, The delete-impact date rendering in UsersContent currently bypasses shared date formatting and invalid-date handling by using getActiveLocale/formatNumber; change both occurrences (around the import and the other instance at lines ~388-390) to call the shared formatDate helper instead, passing the same date value and any existing options so invalid-date handling is preserved; update the JSX where the delete-impact value is displayed to use formatDate(dateValue) (or formatDate(dateValue, options) if options are used) and remove the ad-hoc locale/number formatting logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@admin/src/pages/users/UsersContent.tsx`:
- Line 20: The delete-impact date rendering in UsersContent currently bypasses
shared date formatting and invalid-date handling by using
getActiveLocale/formatNumber; change both occurrences (around the import and the
other instance at lines ~388-390) to call the shared formatDate helper instead,
passing the same date value and any existing options so invalid-date handling is
preserved; update the JSX where the delete-impact value is displayed to use
formatDate(dateValue) (or formatDate(dateValue, options) if options are used)
and remove the ad-hoc locale/number formatting logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20627fc2-5639-41a5-8ed6-52e5bfe289bd
📒 Files selected for processing (5)
admin/src/lib/dateUtils.test.tsadmin/src/lib/dateUtils.tsadmin/src/pages/users/UserCard.tsxadmin/src/pages/users/UsersContent.test.tsxadmin/src/pages/users/UsersContent.tsx
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-authored-by: Cursor <cursoragent@cursor.com>
otomatty
changed the title
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.
概要
PR #793 の未対応レビューコメントに対する develop 向け修正です。Admin のロケール固定表示、メンバー管理 read-only TSDoc、HeadingLevelClamp の不要処理、Hocuspocus shutdown のエラーハンドリングをまとめて対応します。
変更点
admin/: 日付・数値整形を i18n の現在言語に応じたロケールへ変更し、ja/enのテストを追加src/:NoteMembersManageSectionのreadOnlyTSDoc を実装済み UI 契約に同期src/:HeadingLevelClampで doc 変更のない transaction をスキップし、必要になるまでstate.trを作らないように変更server/hocuspocus/:SIGTERM/SIGINTの shutdown 処理を共通化し、失敗時にログ出力してexit(1)するように変更変更の種類
テスト方法
bunx prettier --check admin/src/lib/dateUtils.ts admin/src/lib/dateUtils.test.ts admin/src/pages/users/UsersContent.tsx admin/src/pages/users/UsersContent.test.tsx admin/src/pages/users/UserCard.tsxbunx eslint admin/src/lib/dateUtils.ts admin/src/lib/dateUtils.test.ts admin/src/pages/users/UsersContent.tsx admin/src/pages/users/UsersContent.test.tsx admin/src/pages/users/UserCard.tsxbunx vitest run --config admin/vitest.config.ts admin/src/lib/dateUtils.test.ts admin/src/pages/users/UsersContent.test.tsxbunx prettier --check src/pages/NoteMembers/NoteMembersManageSection.tsx src/components/editor/TiptapEditor/headingLevelClampExtension.ts server/hocuspocus/src/index.tsbunx eslint src/pages/NoteMembers/NoteMembersManageSection.tsx src/components/editor/TiptapEditor/headingLevelClampExtension.ts server/hocuspocus/src/index.tsbunx vitest run src/components/editor/TiptapEditor/headingLevelClampExtension.test.ts src/pages/NoteMembers/NoteMembersManageSection.test.tsxcd server/hocuspocus && bun run buildチェックリスト
スクリーンショット（UI 変更がある場合）
表示フォーマット・TSDoc・内部処理の修正のため未添付です。
関連 Issue
Related to #793
Summary by CodeRabbit
New Features
Documentation
Tests
Chores