fix: メッセージスクロール周りの改修#5004
Open
yas-ako wants to merge 20 commits into
Open
Conversation
|
Preview (prod) → https://5004-prod.traq-preview.trapti.tech/ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5004 +/- ##
==========================================
+ Coverage 62.60% 62.64% +0.03%
==========================================
Files 108 108
Lines 3097 3100 +3
Branches 630 630
==========================================
+ Hits 1939 1942 +3
Misses 1048 1048
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
from
d20401c to
d0690a0
Compare
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
from
b4077b5 to
b65804b
Compare
uni-kakurenbo
changed the title
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
4 times, most recently
from
789f4cd to
cfb4b02
Compare
uni-kakurenbo
changed the title
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
11 times, most recently
from
cdfff3d to
a3cdf7a
Compare
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
7 times, most recently
from
1dd991e to
fcbc5d7
Compare
uni-kakurenbo
force-pushed
the
fix/message-scroller-layout-shift
branch
from
fcbc5d7 to
dc48d47
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Main/MainView/MessagesScroller/composables/useMessagesFetcher.ts (1)
214-222:⚠️ Potential issue | 🟠 Major
isInitialLoadが失敗経路で解除されない可能性がありますLine 216 で
trueにした後、初回ロード処理が例外終了するとfalseに戻らず、初期ロードUIが張り付くリスクがあります。init側でfinallyによる解除を持たせると安全です。💡 提案修正（失敗時も必ず初期ロード状態を解除）
-const init = () => { +const init = async () => { resetRenderedContent() isInitialLoad.value = true - if (props.entryMessageId) { - onLoadAroundMessagesRequest(props.entryMessageId) - } else { - isReachedLatest.value = true - onLoadLatestMessagesRequest() - } + try { + if (props.entryMessageId) { + await onLoadAroundMessagesRequest(props.entryMessageId) + } else { + isReachedLatest.value = true + await onLoadLatestMessagesRequest() + } + } finally { + isInitialLoad.value = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Main/MainView/MessagesScroller/composables/useMessagesFetcher.ts` around lines 214 - 222, The init function sets isInitialLoad.value = true but never guarantees it will be reset if onLoadAroundMessagesRequest or onLoadLatestMessagesRequest throw; wrap the conditional load calls in a try/finally inside init (using the existing init, resetRenderedContent, isInitialLoad, onLoadAroundMessagesRequest and onLoadLatestMessagesRequest identifiers) and set isInitialLoad.value = false in the finally block so the initial-load UI is cleared regardless of success or failure.src/components/Main/MainView/MessageElement/composables/useElementRenderObserver.ts (1)
39-68:⚠️ Potential issue | 🟡 Minor
ResizeObserverのクリーンアップが不足しています。コンポーネントがアンマウントされた際に
resizeObserver.disconnect()が呼ばれないため、メモリリークの可能性があります。🛡️ 修正案
+import { onUnmounted, unref, watch, watchEffect } from 'vue' -import { unref, watch, watchEffect } from 'vue' // ... 既存のコード ... watch( () => route.path, () => // パス変更でunobserve // vue-routerのインスタンス再利用対策 bodyRef.value ? resizeObserver.unobserve(bodyRef.value) : undefined ) + + onUnmounted(() => { + resizeObserver.disconnect() + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Main/MainView/MessageElement/composables/useElementRenderObserver.ts` around lines 39 - 68, The ResizeObserver created as resizeObserver is never disconnected, risking a leak; ensure you call resizeObserver.disconnect() during cleanup—add a call to disconnect inside the stop() path (where you stop observing initial renders) and also register onBeforeUnmount/cleanup in the composable to call resizeObserver.disconnect() so the observer is removed when the component using useElementRenderObserver unmounts; reference resizeObserver, stop(), prevState, emit, bodyRef, isEntryMessage and messageId to keep the existing logic (entryMessageLoaded / changeHeight emits) intact while adding the disconnects.
🧹 Nitpick comments (3)
src/components/Main/MainView/MessagesScroller/MessagesScroller.vue (1)
310-313:handleScrollが初期化時にのみ評価される問題。
enableProactiveLoadingは computed ですが、handleScrollはセットアップ時に一度だけ評価されます。isMobileが実行時に変化した場合（例：ウィンドウリサイズ）、handleScrollの throttle/debounce 設定は更新されません。実際に
isMobileが変化するシナリオが稀であれば、現状でも問題ありませんが、ドキュメントコメントがあると意図が明確になります。♻️ コメント追加案
+// Note: enableProactiveLoading の値はセットアップ時に固定される +// （実行時に isMobile が変化しても handleScroll は更新されない） const handleScroll = (enableProactiveLoading.value ? throttle : debounce)( enableProactiveLoading.value ? 24 : 200, requestLoadMessages )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Main/MainView/MessagesScroller/MessagesScroller.vue` around lines 310 - 313, handleScroll is created once in setup using enableProactiveLoading (computed) so its throttle/debounce choice won't update if isMobile/enableProactiveLoading changes at runtime; either add a short doc comment above the handleScroll definition stating this one-time evaluation is intentional (because isMobile changes are rare) or make it reactive by recreating handleScroll when enableProactiveLoading changes (e.g., replace the one-off call with a computed or a watch on enableProactiveLoading that reassigns handleScroll using throttle/debounce and rebinds listeners), referencing handleScroll, enableProactiveLoading, isMobile, requestLoadMessages, throttle, and debounce.src/components/Main/MainView/MessagesScroller/composables/useMessageScroller.ts (2)
103-116: コメントの意図を確認してください。Line 103-104 のコメント
// XXX: 追加時にここは0になるについて、ids.length - prevIds.length === 0は「配列長が変わらない」ケースを示しますが、これは通常「追加」ではなく「置換」や「更新」のケースです。コメントの意図を確認し、必要であれば修正するか、XXX を解消してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Main/MainView/MessagesScroller/composables/useMessageScroller.ts` around lines 103 - 116, The inline XXX comment "// XXX: 追加時にここは0になる" is misleading because the branch `if (ids.length - prevIds.length === 0)` actually covers cases where the array length did not change (updates/replacements), not additions; update the comment to accurately describe that this block handles "no-length-change" scenarios (e.g., replacements/updates), or remove XXX if obsolete, and ensure any wording references the variables used here (`ids`, `prevIds`, `rootRef`, `state.height`, `newHeight`) so future readers understand that when lengths are equal we preserve scroll position unless already near bottom.
26-78:onChangeHeight内の冗長なnullチェック。Line 68 の
if (!rootRef.value) returnは、Line 27 で既にチェック済みのため冗長です。ただし、await defer()の後なので、その間にrootRef.valueがnullになる可能性は理論上あります。現状のままで問題ありませんが、コメントがあると意図が明確になります。♻️ コメント追加案
// 末尾付近を閲覧しているときは，末尾を保つ if (state.height - scrollBottom <= 50) { - if (!rootRef.value) return + // defer() 中にアンマウントされた可能性があるため再チェック + if (!rootRef.value) return // 複数の onChangeHeight が同フレーム内に複数回呼ばれる場合があるので， // すべての更新を待ってからスクロールする await defer() rootRef.value.scrollTo({ top: rootRef.value.scrollHeight }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Main/MainView/MessagesScroller/composables/useMessageScroller.ts` around lines 26 - 78, Keep the guard `if (!rootRef.value) return` inside onChangeHeight (after the await defer()) but add a concise comment explaining why it's necessary: note that rootRef.value was checked at the top of onChangeHeight but can become null during the await defer(), so the second null-check prevents runtime errors before calling rootRef.value.scrollTo/getBoundingClientRect; reference the onChangeHeight function, rootRef, and defer in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/components/Main/MainView/MessageElement/composables/useElementRenderObserver.ts`:
- Around line 39-68: The ResizeObserver created as resizeObserver is never
disconnected, risking a leak; ensure you call resizeObserver.disconnect() during
cleanup—add a call to disconnect inside the stop() path (where you stop
observing initial renders) and also register onBeforeUnmount/cleanup in the
composable to call resizeObserver.disconnect() so the observer is removed when
the component using useElementRenderObserver unmounts; reference resizeObserver,
stop(), prevState, emit, bodyRef, isEntryMessage and messageId to keep the
existing logic (entryMessageLoaded / changeHeight emits) intact while adding the
disconnects.
In
`@src/components/Main/MainView/MessagesScroller/composables/useMessagesFetcher.ts`:
- Around line 214-222: The init function sets isInitialLoad.value = true but
never guarantees it will be reset if onLoadAroundMessagesRequest or
onLoadLatestMessagesRequest throw; wrap the conditional load calls in a
try/finally inside init (using the existing init, resetRenderedContent,
isInitialLoad, onLoadAroundMessagesRequest and onLoadLatestMessagesRequest
identifiers) and set isInitialLoad.value = false in the finally block so the
initial-load UI is cleared regardless of success or failure.
---
Nitpick comments:
In
`@src/components/Main/MainView/MessagesScroller/composables/useMessageScroller.ts`:
- Around line 103-116: The inline XXX comment "// XXX: 追加時にここは0になる" is
misleading because the branch `if (ids.length - prevIds.length === 0)` actually
covers cases where the array length did not change (updates/replacements), not
additions; update the comment to accurately describe that this block handles
"no-length-change" scenarios (e.g., replacements/updates), or remove XXX if
obsolete, and ensure any wording references the variables used here (`ids`,
`prevIds`, `rootRef`, `state.height`, `newHeight`) so future readers understand
that when lengths are equal we preserve scroll position unless already near
bottom.
- Around line 26-78: Keep the guard `if (!rootRef.value) return` inside
onChangeHeight (after the await defer()) but add a concise comment explaining
why it's necessary: note that rootRef.value was checked at the top of
onChangeHeight but can become null during the await defer(), so the second
null-check prevents runtime errors before calling
rootRef.value.scrollTo/getBoundingClientRect; reference the onChangeHeight
function, rootRef, and defer in the comment for clarity.
In `@src/components/Main/MainView/MessagesScroller/MessagesScroller.vue`:
- Around line 310-313: handleScroll is created once in setup using
enableProactiveLoading (computed) so its throttle/debounce choice won't update
if isMobile/enableProactiveLoading changes at runtime; either add a short doc
comment above the handleScroll definition stating this one-time evaluation is
intentional (because isMobile changes are rare) or make it reactive by
recreating handleScroll when enableProactiveLoading changes (e.g., replace the
one-off call with a computed or a watch on enableProactiveLoading that reassigns
handleScroll using throttle/debounce and rebinds listeners), referencing
handleScroll, enableProactiveLoading, isMobile, requestLoadMessages, throttle,
and debounce.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8406740b-1c96-4b8b-b383-45254486cfa5
📒 Files selected for processing (17)
src/components/Main/MainView/ChannelView/ChannelViewContent/ChannelViewContentMain.vuesrc/components/Main/MainView/MessageElement/ClipElement.vuesrc/components/Main/MainView/MessageElement/MessageContents.vuesrc/components/Main/MainView/MessageElement/MessageElement.vuesrc/components/Main/MainView/MessageElement/composables/useElementRenderObserver.tssrc/components/Main/MainView/MessagesScroller/MessagesScroller.vuesrc/components/Main/MainView/MessagesScroller/MessagesSkeleton.vuesrc/components/Main/MainView/MessagesScroller/composables/useMessageScroller.tssrc/components/Main/MainView/MessagesScroller/composables/useMessageScrollerElementResizeObserver.tssrc/components/Main/MainView/MessagesScroller/composables/useMessagesFetcher.tssrc/components/UI/DeferredRender.vuesrc/components/UI/MarkdownContent.vuesrc/components/UI/SlideDown.vuesrc/composables/message/useEmbeddings.tssrc/lib/basic/timer.tssrc/styles/global.scsstests/unit/lib/basic/timer.spec.ts
💤 Files with no reviewable changes (1)
- src/components/Main/MainView/MessagesScroller/composables/useMessageScrollerElementResizeObserver.ts
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
3 participants
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 を入れたいのか
PR を出す前の確認事項
Summary by CodeRabbit
Release Notes
新機能
スタイル
テスト