-
Notifications
You must be signed in to change notification settings - Fork 44
fix: 日をまたいだ際に日時の表示を更新するように修正 #4750
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
base: master
Are you sure you want to change the base?
Conversation
|
Preview (prod) → https://4750-prod.traq-preview.trapti.tech/ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4750 +/- ##
==========================================
- Coverage 13.54% 13.49% -0.05%
==========================================
Files 704 705 +1
Lines 31691 31800 +109
Branches 676 676
==========================================
- Hits 4293 4292 -1
- Misses 27386 27496 +110
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds automatic date display updates when the day changes for message headers in the application.
- Introduces a new
useDateRepresentationcomposable that automatically updates date display when crossing midnight - Replaces the static computed date property with a reactive date that updates via scheduled timeouts
- Implements proper cleanup of timers on component unmount to prevent memory leaks
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <script lang="ts"> | ||
| import { ref, onMounted, onUnmounted, watchEffect } from 'vue' | ||
| import { getDateRepresentation } from '/@/lib/basic/date' | ||
| const useDateRepresentation = (updatedAt: string) => { |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useDateRepresentation function is defined in a separate script block but should be moved to a composables directory (e.g., /@/composables/useDateRepresentation.ts) for better code organization and reusability across components.
| import { ref, onMounted, onUnmounted, watchEffect } from 'vue' | ||
| import { getDateRepresentation } from '/@/lib/basic/date' | ||
| const useDateRepresentation = (updatedAt: string) => { | ||
| const dateTimeout = ref<ReturnType<typeof setTimeout>>() | ||
| const date = ref<string>('') | ||
| watchEffect(() => { | ||
| date.value = getDateRepresentation(updatedAt) | ||
| }) | ||
| const subscribeDateChange = () => { | ||
| const now = new Date() | ||
| const tomorrow = new Date() | ||
| tomorrow.setDate(now.getDate() + 1) | ||
| tomorrow.setHours(0, 0, 0, 0) | ||
| dateTimeout.value = setTimeout(() => { | ||
| date.value = getDateRepresentation(updatedAt) | ||
| subscribeDateChange() | ||
| }, tomorrow.getTime() - now.getTime()) | ||
| } | ||
| onMounted(() => subscribeDateChange()) | ||
| onUnmounted(() => clearTimeout(dateTimeout.value)) |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each MessageHeader component creates its own timer, which could be inefficient with many messages. Consider using a global timer that broadcasts date changes to all components instead of creating individual timers per component.
| import { ref, onMounted, onUnmounted, watchEffect } from 'vue' | |
| import { getDateRepresentation } from '/@/lib/basic/date' | |
| const useDateRepresentation = (updatedAt: string) => { | |
| const dateTimeout = ref<ReturnType<typeof setTimeout>>() | |
| const date = ref<string>('') | |
| watchEffect(() => { | |
| date.value = getDateRepresentation(updatedAt) | |
| }) | |
| const subscribeDateChange = () => { | |
| const now = new Date() | |
| const tomorrow = new Date() | |
| tomorrow.setDate(now.getDate() + 1) | |
| tomorrow.setHours(0, 0, 0, 0) | |
| dateTimeout.value = setTimeout(() => { | |
| date.value = getDateRepresentation(updatedAt) | |
| subscribeDateChange() | |
| }, tomorrow.getTime() - now.getTime()) | |
| } | |
| onMounted(() => subscribeDateChange()) | |
| onUnmounted(() => clearTimeout(dateTimeout.value)) | |
| import { ref, onMounted, onUnmounted, watchEffect, computed } from 'vue' | |
| import { getDateRepresentation } from '/@/lib/basic/date' | |
| // Global midnight tick composable | |
| const midnightTick = ref(new Date()) | |
| let midnightTimeout: ReturnType<typeof setTimeout> | null = null | |
| function scheduleNextMidnightTick() { | |
| const now = new Date() | |
| const tomorrow = new Date() | |
| tomorrow.setDate(now.getDate() + 1) | |
| tomorrow.setHours(0, 0, 0, 0) | |
| const msUntilMidnight = tomorrow.getTime() - now.getTime() | |
| midnightTimeout = setTimeout(() => { | |
| midnightTick.value = new Date() | |
| scheduleNextMidnightTick() | |
| }, msUntilMidnight) | |
| } | |
| if (typeof window !== 'undefined' && !midnightTimeout) { | |
| scheduleNextMidnightTick() | |
| } | |
| const useMidnightTick = () => midnightTick | |
| const useDateRepresentation = (updatedAt: string) => { | |
| const tick = useMidnightTick() | |
| const date = ref<string>(getDateRepresentation(updatedAt)) | |
| watchEffect(() => { | |
| // Recompute date when tick or updatedAt changes | |
| date.value = getDateRepresentation(updatedAt) | |
| }) |
| }, tomorrow.getTime() - now.getTime()) | ||
| } | ||
| onMounted(() => subscribeDateChange()) | ||
| onUnmounted(() => clearTimeout(dateTimeout.value)) |
Copilot
AI
Oct 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout cleanup doesn't check if dateTimeout.value is defined. If the component unmounts before subscribeDateChange is called, this could attempt to clear an undefined timeout.
| onUnmounted(() => clearTimeout(dateTimeout.value)) | |
| onUnmounted(() => { | |
| if (dateTimeout.value !== undefined) { | |
| clearTimeout(dateTimeout.value) | |
| } | |
| }) |
Pugma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらコメントをつけました!
ご確認をお願いします 🙏
| import { ref, onMounted, onUnmounted, watchEffect } from 'vue' | ||
| import { getDateRepresentation } from '/@/lib/basic/date' | ||
| const useDateRepresentation = (updatedAt: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここは copilot のコメントのとおりだと思います
/src/composables 以下にこのような再利用可能な関数をおいているところがあるので、そちらに移してしまいましょう
また、 (画面で見えない部分を含む) 読み込んでいる範囲すべての投稿が現在時刻を参照するようになってしまうので、それも処理が重くなる原因として copilot からコメントがついてます
これは、先の部分で触れたのと同じように composable 以下に移して、それを各投稿のコンポーネントから参照するようにすることでまとめて解決できると思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(横槍ごめんなさい :sorry:) composables にまとめたところで setTimeout のインスタンスが大量に生成されるのは回避できない気がします
(パフォーマンスに問題があるようなら effect scope とかを使うとよさそうです)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
composables にまとめたうえで、この部分から各投稿の時間表示を更新するイベントを発火できればいいのかなって思っていたんだけど、それが effect scope になるのかな
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ~ Copilot がそういう変更を提示してくれているんですね、よく読んでいませんでしたすみません :pray-nya:
でもなんか間違ってる気がするので、やはり effect scope あたりを使って実装するのが無難そうです (他にシンプルな実装の仕方があるならそれでもよさそうですけれど)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`<script lang="ts">
import { useDateRepresentation } from '/@/composables/useDateRepresentation'
export default {
setup() {
const date = useDateRepresentation(updatedAt)
return { date }
}
}
</script>`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
↑
ありがとうございます。わかりました。copilotの言うとおりに、composablesにuseDateRepresentation.tsとしてまとめました。これを参照する方法を調べたのですが、MessageHeader.vueの元の部分のところを
上のように変えるというので良いのでしょうか?
また、copilotが最後に指摘している
onUnmounted(() => clearTimeout(dateTimeout.value))
という文言がuseDateRepresentationに見当たらず、どうすればよいか分かりません。
お手数をおかけしまして申し訳ありません。よろしくお願いします。
概要
日をまたいだ際に日時の表示を更新する処理を追加しました。
なぜこの PR を入れたいのか
close #4606
動作確認の手順
日付が変わるのを待ち、日時の表示が変わることを確認する。(タイムゾーンをいじると楽かもしれないです。)
UI 変更部分のスクリーンショット
before
after
PR を出す前の確認事項
見てほしいところ・聞きたいことなど
MessageHeaderごとに更新の処理が走るので、少し非効率な気がします。