Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough複数のビュー・コンポーネント・シリアライザで time 要素や JSON に出力する日時表現を ISO 8601( Changes
Sequence Diagram(s)(該当なし) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
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 |
| | お知らせ作成中 | ||
| - else | ||
| time.a-meta(datetime='announcement.published_at_date_time') | ||
| time.a-meta(datetime="#{announcement.published_at.iso8601}") |
There was a problem hiding this comment.
datetime属性に文字列が渡されていたので同時に修正した。
| .page-content-header__row | ||
| .page-content-header__before-title | ||
| time.a-meta(datetime="#{@announcement.created_at.to_datetime}" pubdate='pubdate') | ||
| time.a-meta(datetime="#{@announcement.created_at.iso8601}" pubdate='pubdate') |
There was a problem hiding this comment.
created_atなどはdatetime型のフィールドなので、to_datetimeは不要な処理なのでは?と考えている。
to_datetimeが必要な理由が見つけられなかったので一旦消した。もし必要性を知っていたら教えていただきたい
他にも同様の修正を掛けた場所が複数ある
|
メモ: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ddaa008 to
fe5f29d
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
app/views/events/_events.html.slim (1)
29-31: 変更は妥当です。加えて、日時属性の自動検証を1本追加するのを推奨します。この変更自体は問題ありません。今回のような横断修正の再発防止として、代表ビューに
time[datetime]のフォーマットを確認する view/system spec を1つ置くと、手動確認の負担を下げられます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/events/_events.html.slim` around lines 29 - 31, Add a view/system spec that automatically verifies the time[datetime] attribute uses ISO8601 so regressions like the one in the events partial are caught; target the rendered partial containing the time.a-meta(datetime=event.start_at.iso8601) (or a representative events view) and assert that the time element's datetime attribute matches an ISO8601 timestamp (use a regex or DateTime.parse in the test) and that the visible text still uses the localized l(event.start_at) formatting; place the test as a lightweight view spec (e.g., spec/views/events/_events_spec.rb) or a small system spec (e.g., spec/system/event_view_spec.rb) depending on existing test conventions.
🤖 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/javascript/components/BookmarksInDashboard.jsx`:
- Around line 157-159: In BookmarksInDashboard.jsx the code uses
bookmark.updated_at.iso8601 which is undefined because the API already returns
updated_at as an ISO8601 string; change usages of bookmark.updated_at.iso8601
(e.g. in the <time dateTime={...}> and any other references) to use
bookmark.updated_at directly so the ISO8601 string from the API is passed
through (update the dateTime prop and any displayed value accordingly).
In `@app/javascript/components/Events.jsx`:
- Line 107: In Events.jsx, fix the time element to use the ISO8601 string
directly by replacing any usage of event.start_at.iso8601 with event.start_at
(e.g., update the <time className="a-meta" dateTime={...}> to use
event.start_at); additionally implement the missing /api/events endpoint so the
frontend receives events as JSON with start_at as an ISO8601 string (ensure the
controller/action or route that serves /api/events returns an array of events
including start_at strings).
In `@app/javascript/components/Product.jsx`:
- Around line 157-163: The time elements in Product.jsx are using
product.created_at.iso8601 and product.updated_at.iso8601 which are undefined
because the API returns formatted strings; update Product.jsx to use the
machine-readable fields product.created_at_date_time and
product.updated_at_date_time for the dateTime attributes (and likewise switch
any other .iso8601 uses, e.g. at line 276); on the API side add
training_ends_on_date_time to app/views/api/products/_product.json.jbuilder
(e.g. output product.user.training_ends_on.to_datetime when training_ends_on
exists) and then use product.user.training_ends_on_date_time in the front-end
where training_ends_on is rendered.
In `@app/javascript/components/RegularEvent.jsx`:
- Line 113: The time element in RegularEvent.jsx is accessing
event.start_at.iso8601 which is undefined because the API already returns
start_at as an ISO8601 string; update the component to use event.start_at
directly for both the dateTime prop and displayed time (replace references to
event.start_at.iso8601 with event.start_at) so the <time> element and any
parsing logic consume the string the API provides (check usages in
RegularEvent.jsx around the <time className="a-meta" ...> and any related
rendering/formatting functions).
In `@app/views/events/_event.html.slim`:
- Around line 25-26: datetime 属性に created_at を使っているため表示の updated_at
と不一致になっています。time.a-meta__value 要素の datetime を event.updated_at
に合わせて修正し、datetime="#{event.updated_at.iso8601}" として表示日時と参照元が一致するようにしてください(要素:
time.a-meta__value、参照フィールド: event.updated_at)。
In `@app/views/home/_colleague_trainee_recent_report.html.slim`:
- Line 19: Replace the use of Date#to_time when rendering the datetime attribute
so the app respects Rails time_zone: change the time tag building code that uses
report.reported_on.to_time (the time.a-meta(datetime=...) expression) to use
report.reported_on.in_time_zone.iso8601 instead; also apply the same fix to the
other occurrences that convert Date to time (the similar uses in the report
partial and bookmarks list) so all datetime attributes are generated with
in_time_zone.iso8601.
In `@app/views/products/_product.html.slim`:
- Around line 62-63: datetime属性に使用している時刻が表示と不一致です: time.a-meta の datetime に
product.commented_users.last.created_at.iso8601 を使っていますが表示は
product.comments.last.created_at を使っています。time.a-meta の datetime を
product.comments.last.created_at.iso8601 に切り替えて、time
要素の機械可読な時刻と画面表示(product.comments.last.created_at)を一致させてください(または表示側を
commented_users に合わせる場合は product.comments.last.created_at を
product.commented_users.last.created_at に統一してください)。
In `@app/views/regular_events/_regular_event.html.slim`:
- Around line 23-24: The time element in the partial _regular_event.html.slim
uses regular_event.created_at for the datetime attribute while rendering
regular_event.updated_at as the visible text; update the datetime attribute to
use regular_event.updated_at.iso8601 so the machine-readable datetime (datetime
attr) matches the displayed timestamp (regular_event.updated_at) in the time
element.
---
Nitpick comments:
In `@app/views/events/_events.html.slim`:
- Around line 29-31: Add a view/system spec that automatically verifies the
time[datetime] attribute uses ISO8601 so regressions like the one in the events
partial are caught; target the rendered partial containing the
time.a-meta(datetime=event.start_at.iso8601) (or a representative events view)
and assert that the time element's datetime attribute matches an ISO8601
timestamp (use a regex or DateTime.parse in the test) and that the visible text
still uses the localized l(event.start_at) formatting; place the test as a
lightweight view spec (e.g., spec/views/events/_events_spec.rb) or a small
system spec (e.g., spec/system/event_view_spec.rb) depending on existing test
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12331653-9bcf-4825-97e8-2e494d8a6001
📒 Files selected for processing (36)
app/components/products/product_component.html.slimapp/components/users/users_answer_component.html.slimapp/javascript/components/BookmarksInDashboard.jsxapp/javascript/components/Events.jsxapp/javascript/components/Product.jsxapp/javascript/components/RegularEvent.jsxapp/views/announcements/_announcement.html.slimapp/views/announcements/show.html.slimapp/views/coding_tests/coding_test_submissions/_coding_test_submission.html.slimapp/views/coding_tests/coding_test_submissions/show.html.slimapp/views/current_user/bookmarks/_list.html.slimapp/views/events/_event.html.slimapp/views/events/_events.html.slimapp/views/home/_announcement.html.slimapp/views/home/_colleague_trainee_recent_report.html.slimapp/views/movies/_movie_header.html.slimapp/views/notifications/_notification.html.slimapp/views/pages/_doc_header.html.slimapp/views/products/_product.html.slimapp/views/products/_product_header.html.slimapp/views/questions/_question_header.html.slimapp/views/regular_events/_regular_event.html.slimapp/views/reports/_product.html.slimapp/views/reports/_report.html.slimapp/views/users/_product.html.slimapp/views/users/_reports.html.slimapp/views/users/announcements/_wip_announcements.html.slimapp/views/users/comments/_comment.html.slimapp/views/users/events/_wip_events.html.slimapp/views/users/events/index.html.slimapp/views/users/pages/_wip_pages.html.slimapp/views/users/products/_wip_products.html.slimapp/views/users/questions/_wip_questions.html.slimapp/views/users/reports/_wip_reports.html.slimapp/views/watches/_watch.html.slimapp/views/works/show.html.slim
fe5f29d to
5a178d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/views/users/comments/_comment.html.slim`:
- Around line 32-33: The datetime attribute and displayed time are inconsistent:
the time tag uses commentable.created_at for datetime but renders
comment.updated_at; change one so both match—preferably update the datetime
attribute to use comment.updated_at.iso8601 (or alternatively render
commentable.created_at) so the time.a-meta element’s datetime value and the
visible timestamp (comment.updated_at) are aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30114772-08f2-4b34-8782-5540561faf8a
📒 Files selected for processing (33)
app/components/products/product_component.html.slimapp/components/users/users_answer_component.html.slimapp/javascript/components/Product.jsxapp/views/announcements/_announcement.html.slimapp/views/announcements/show.html.slimapp/views/coding_tests/coding_test_submissions/_coding_test_submission.html.slimapp/views/coding_tests/coding_test_submissions/show.html.slimapp/views/current_user/bookmarks/_list.html.slimapp/views/events/_event.html.slimapp/views/events/_events.html.slimapp/views/home/_announcement.html.slimapp/views/home/_colleague_trainee_recent_report.html.slimapp/views/movies/_movie_header.html.slimapp/views/notifications/_notification.html.slimapp/views/pages/_doc_header.html.slimapp/views/products/_product.html.slimapp/views/products/_product_header.html.slimapp/views/questions/_question_header.html.slimapp/views/regular_events/_regular_event.html.slimapp/views/reports/_product.html.slimapp/views/reports/_report.html.slimapp/views/users/_product.html.slimapp/views/users/_reports.html.slimapp/views/users/announcements/_wip_announcements.html.slimapp/views/users/comments/_comment.html.slimapp/views/users/events/_wip_events.html.slimapp/views/users/events/index.html.slimapp/views/users/pages/_wip_pages.html.slimapp/views/users/products/_wip_products.html.slimapp/views/users/questions/_wip_questions.html.slimapp/views/users/reports/_wip_reports.html.slimapp/views/watches/_watch.html.slimapp/views/works/show.html.slim
🚧 Files skipped from review as they are similar to previous changes (20)
- app/views/coding_tests/coding_test_submissions/show.html.slim
- app/views/regular_events/_regular_event.html.slim
- app/views/users/reports/_wip_reports.html.slim
- app/views/movies/_movie_header.html.slim
- app/views/announcements/show.html.slim
- app/views/reports/_product.html.slim
- app/views/events/_event.html.slim
- app/views/watches/_watch.html.slim
- app/views/home/_colleague_trainee_recent_report.html.slim
- app/views/users/events/_wip_events.html.slim
- app/components/users/users_answer_component.html.slim
- app/views/pages/_doc_header.html.slim
- app/components/products/product_component.html.slim
- app/views/coding_tests/coding_test_submissions/_coding_test_submission.html.slim
- app/views/works/show.html.slim
- app/views/questions/_question_header.html.slim
- app/views/announcements/_announcement.html.slim
- app/views/products/_product.html.slim
- app/javascript/components/Product.jsx
- app/views/home/_announcement.html.slim
a6510fe to
7e80ca1
Compare
7e80ca1 to
1a08af2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@yokomaru |
|
@s-tone-gs |
|
@s-tone-gs |
There was a problem hiding this comment.
お疲れ様です。お待たせいたしました。
in_time_zoneとto_datetimeの違いなど非常に勉強になりました!
コメントいたしましたのでご確認お願いいたします。
検証はほぼ問題なかったのですが、検証項目について2点、対応箇所について1点確認させてください!
検証項目について
※ タブというと以下のようなUIを想像するので、検証箇所はなるべく正確に記載いただくと伝わりやすいかな?と思いました!🙏

- ログインユーザー固有
同期の日報という項目が見当たらなかったのですが、最新のみんなの日報の箇所のことであっていますでしょうか?
対応箇所について
time.a-metaでgrepしたところ、datetime属性がついてないtimeタグがいくつか発見いたしました👀- こちらはそのままでも問題ないでしょうか?(この属性の必須度がどのくらいか分からっておらず質問させていただきました!🙏)
- 検討いただき不要or別のIssueでの対応の方が良いと判断されたらそれでも大丈夫です!
time.a-meta
span.a-meta__label
| 提出
= l(@product.published_at)datetime属性がついてないtimeタグについて
# git grep -n "time.a-meta" (vscodeで正規表現あり検索で`time.a-meta\n`の方がみやすいかもです)
app/components/products/product_component.html.slim:36: time.a-meta
app/components/products/product_component.html.slim:42: time.a-meta
app/components/products/product_component.html.slim:49: time.a-meta
app/components/products/product_component.html.slim:55: time.a-meta
app/components/products/product_component.html.slim:76: time.a-meta
app/views/companies/products/_product.html.slim:28: time.a-meta
app/views/companies/products/_product.html.slim:37: time.a-meta
app/views/companies/products/_product.html.slim:46: time.a-meta
app/views/companies/products/_product.html.slim:74: time.a-meta
app/views/companies/products/_product.html.slim:83: time.a-meta
app/views/companies/products/_product.html.slim:92: time.a-meta
app/views/companies/products/_product.html.slim:101: time.a-meta
app/views/events/_upcoming_event.html.slim:29: time.a-meta
app/views/external_entries/_external_entry.html.slim:18: time.a-meta
app/views/home/_recent_report.html.slim:19: time.a-meta
app/views/questions/_question.html.slim:39: time.a-meta
app/views/questions/_question.html.slim:45: time.a-meta
app/views/questions/_question_header.html.slim:45: time.a-meta
app/views/users/_inactive_users.html.slim:21: time.a-meta
| .card-list-item-meta__items | ||
| .card-list-item-meta__item | ||
| time.a-meta dateTime=notification.created_at | ||
| time.a-meta dateTime=notification.created_at.iso8601 |
There was a problem hiding this comment.
キャメルケースになっているのが気になりました!(非React化等を行なった際に残ったものだと推測しています)
この機に合わせて修正してしまっても良いのかな?と思いました💡
| time.a-meta dateTime=notification.created_at.iso8601 | |
| time.a-meta datetime=notification.created_at.iso8601 |
There was a problem hiding this comment.
ありがとうございます~!見落としでした👀
キャメルケースを排除にて修正しました!
| .card-list-item-meta__item | ||
| - if @product.user.training_ends_on | ||
| time.a-meta dateTime=@product.user.training_ends_on | ||
| time.a-meta dateTime=@product.user.training_ends_on.in_time_zone.iso8601 |
There was a problem hiding this comment.
| class: 'a-user-name' | ||
| .card-list-item-meta__item | ||
| time.a-meta datetime=answer.updated_at pubdate="pubdate" | ||
| time.a-meta datetime=answer.updated_at.iso8601 pubdate="pubdate" |
There was a problem hiding this comment.
pubdate属性は現在廃止 されているらしく、過去のPRをいくつかみたところ削除対応をされているようです。
#2789 (comment)
#1587 (comment)
ざっとみた感じこの要素が残っている箇所が今回のPRでiso対応した箇所と一致しているようなので、このPRでついでに削除してもいいのかな?とおもいました!(検討いただき別のIssueでの対応の方が良いと判断されたらそれでも大丈夫です!)
There was a problem hiding this comment.
ありがとうございます~!把握していませんでした!対応箇所も一致していますし、過去のPRでも同時に修正されているので今回もこのPR内で削除しようと思います🫡
|
@yokomaru |
|
@machida ref: #9717 (review)
|
|
@s-tone-gs 提案ありがとうございます!!ぜひ、お願いしたいです🙏 |
|
@machida 検証項目についての指摘もありがとうございます!
すみません、そもそもユーザープロフィールは現状の修正箇所外であり、検証項目には含まれないものでした💦
こちらはに関してはそうです! 修正箇所が増える関係で検証項目をいじりますので、その際に同時に修正します🙇♂️曖昧な記述で混乱させてしまって申し訳ありません |
4734e08 to
2e544ae
Compare
🚀 Review AppURL: https://bootcamp-pr-9717-fvlfu45apq-an.a.run.app
|
c3a41d9 to
4d3b156
Compare
|
|
||
| - if @display_until_next_elapsed_days && elapsed_days < @reply_deadline_days | ||
| time.a-meta | ||
| .a-meta |
There was a problem hiding this comment.
「1時間未満」や「約○○時間」という曖昧な値は機械可読な形式で指定できないので、timeタグで扱うことをやめた。
ref: https://developer.mozilla.org/ja/docs/Web/HTML/Reference/Elements/time
|
@yokomaru ref: #9717 (comment) |
APIレスポンスの際にISO8601形式にシリアライズされているため、.iso8601は呼び出せない
to_timeはDate型フィールドの変換を行う際にタイムゾーンを無視してしまうため修正した
現在は廃止されているため
fff0e2e to
08e7b62
Compare
| .a-meta | ||
| | 〜 | ||
| time datetime=@product.comments.last.created_at.iso8601 | ||
| = l @product.comments.last.created_at, format: :date_and_time |
There was a problem hiding this comment.
「〜(から)」という記号は時間情報ではないため「~」だけtimeタグの外に出した
08e7b62 to
8a99201
Compare
CEOやアクセシビリティの観点から修正するべきと判断
8a99201 to
4335246
Compare









Issue
datetime属性の形式が統一されていない #9664概要
変更確認方法
YYYY-MM-DDThh:mm:ss+09:00の形式であること新たに付与したdatetime属性の確認
近日開催のイベント欄の「開催日時」にdatetime属性が付与されていることを確認する
ログイン不要
P[n]Mの形式であることを確認するコンテンツページ
ユーザー関連
ログインユーザー固有
通知
Screenshot
変更前
変更後
Summary by CodeRabbit