Virtual Scrolling 2.1 #3577
No reviewers
Labels
No labels
BE Request
Backlog
Better behavior
Blocked by deps
Bug
Code Review
Discussion
Doing
Feature Request / Enhancement
In the shining bright future maybe
It's complicated
MS Edge
Mememoon (or other niche browsers)
Missing API
Missing feature
Missing l10n/i18n
NL1
NL2
NL4
Need to verify on develop
RELEASE BLOCKER
Reassign or Close
Refactor
Regression
Safari
To Do
User story
Waiting on godot
accessibility
better documentation
chore
confirmed
easy ticket
incident
l10n update
mastoapi
mobile
needs design
needs-info
needs-review
stupid
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
pleroma/pleroma-fe!3577
Loading…
Reference in a new issue
No description provided.
Delete branch "chat-view-update"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
useConversation2e4ea899fe9a5519d9b3@ -162,2 +185,4 @@}},isSuspendable() {return !this.replying && this.mediaPlaying.size === 0fix
@ -12,4 +19,0 @@getScrollPosition,isBottomedOut,isScrollable,} from './chat_layout_utils.js'probably remove this entire file now
/oc review
opencode-agent:job=87:generation=1
Automated review of
develop@7e147527→chat-view-update@664260e(28 files, +1293/−1170), triggered by/oc reviewon comment 118642.Verification performed at head
664260e: full unit suite in browser mode (chromium headless): 41 files, 486 passed, 2 skipped — green;biome checkclean;stylelintclean.Overall: the decomposition is a genuine readability win (
chat_view.js725 → 256 lines) and theinvertDirection/compensation comments inuseVirtualScrolling.jsare excellent. That said, I found one functional regression I'd treat as a blocker, one crash-on-error-path bug, and a handful of smaller issues.Blocking / high
1. Cross-chat message contamination —
usePleromaChat.js(onChatUpdate/addMessages)The old
chat_view.jshad a sanity check inaddMessagesthat dropped messages whosechat_iddidn't match the current chat (if (!this.isConversation && message.chat_id !== this.chat.id) return). That check exists because the chat view registers a subscriber without astream, so it lands inglobalSubscriptionsandstores/streaming.jsonMessagedispatches everypleroma:chat_update(for all chats) to it. The composable port dropped the filter:onChatUpdatefeedschatUpdate.lastMessagestraight intoaddMessages. While this chat is open, a message arriving in any other chat will be appended to this chat's list, skewingminId/maxId,newMessagesCount, andmarkAsRead(readChat({ lastReadId: maxId })can be called with a foreign id). Please restore thechat_idcheck (inaddMessagesoronChatUpdate).2. Retry path crashes —
usePleromaChat.jsdoSendMessagemessageRetriers.set(...)is called on therefitself, not itsMap— should bemessageRetriers.value.set(...). On a retryable send error (5xx), the old code scheduled an exponential-backoff retry (this.messageRetriers[key] = setTimeout(...)); the new code throwsTypeError: messageRetriers.set is not a functioninside the catch block, so the retry is never scheduled and the exception escapes the error handler. The new tests only cover happy paths, so this isn't caught.3.
formPlaceholdernever updates —chat_view.jsThe computed reads
chatRecipientwithout.value: the ref object is always truthy,chatRecipient.screen_name_uiis alwaysundefined, and the computed builds no reactive dependency, so it evaluates once and never again. Result: the post-form placeholder is permanently"Message "(I verifiedt('chats.message_user', { nickname: undefined })interpolates to"Message ") instead of''until the recipient loads and then"Message @nick". Fix:chatRecipient.value?.screen_name_ui(withchatRecipient.valuein the condition).Medium
4.
streamingEnabledis constantfalse—usePleromaChat.jsmastoUserSocketStatus === WSConnectionStatus.JOINEDis missing.value, andmastoUserSocketStatusis not a member ofstores/streaming.jsat all (the store exposesstate; cf.useConversation.js:27, which at least uses.value). So thefetchChatstreaming short-circuit is dead code. Practical impact is limited today becauseonStreamConnectstops the poller, but any future reliance onstreamingEnabledwill misbehave. Worth fixing both sites againststate.5.
ResizeObservernever disconnected —useClientRectSize.jsonUnmounted(() => resizeObserver.disconnect)evaluates the method without calling it (missing parens), so observers are never released. Consumers: one per chat view (post form), one per embedded conversation in timelines (conversation.jspreviously disconnected correctly), plus the App-level body scroller. Also nothing unobserves the previous element when theelementref changes.Small / nits
useVirtualScrolling.js:oldRealis computed fromnewValandnewRealfromoldVal— swapped names. The comparison is symmetric so behavior is fine, but it's very confusing to read.chat_view.js:watch(isValid)throwingTypeErrorroutes a prop-contract violation to the global error handler in production; aconsole.error/report without the throw may be friendlier.usePleromaChat.js: the internalerrorref set onactivate()failure is never returned (onlyfetchErroris), so activate failures surface only aschatReady === false. Also neither the retrysetTimeouts noridempotencyKeyIndexare cleared inclear()/deactivate(), so a scheduled retry can fire after switching/closing a chat and re-enterdoSendMessageon cleared state.usePleromaChat.jsdeleteChatMessage:maxBy(messages, 'id').idthrows when the deleted message was the last one (pre-existing behavior, but it moved here — a good moment to guard it).chat_message.js:suspendableStateChangeemitsthis.status.idwhileheightChangeemitschatItem.id; the keys diverge if a chat message ever wraps aretweeted_status.timeline.js: infinite scroll now fires onshouldLoadBottomedges only — if the user rests inside the load margin while new statuses stream in, the watcher won't re-fire (the old scroll handler re-checked continuously). Also the load margin changed from ~750px tofontSize * 25(~350–400px) — intentional?chat_view.vue/chat_view.js:ref="messageList"/ref="pendingMessageList"are unused in setup;faChevronLeftis still imported/registered after the go-back button removal;startFetching'sisFirstFetchparameter is unused.useWindowScroll.js: returning barewindow.scrollBy/window.scrollTohappens to work because they're own properties ofwindow(I verified detached calls don't throw in chromium), but wrapping them ((...args) => window.scrollBy(...args)) would be more defensive across engines.Tests / coverage
chat_view.spec.js(128 lines) was deleted;usePleromaChat.spec.jscoversaddMessages/markAsRead/deleteChatMessagehappy paths, but nothing covers the send-error path (where #2 hides) or the conversation/chat mode fork in the view. I'd love to keep some view-level coverage for that fork.Nice touches: the
fake-id prefix is now applied consistently (buildFakeMessage,fake-date-, and the context-change filter inuseVirtualScrollingall line up), and the separation of pending messages into their own list instance is clean.Published by opencode-fix-bot · marker:
opencode-agent:job=87:generation=1Intentional. IMO old margin was way too big.
I'm gonna be real. I really don't want to deal with pleroma chats. Nobody does. That's why i'm doing Chats 2.0 with normal threads. I might add more tests to it on a rainy day.
/oc review
opencode-agent:job=88:generation=1
Automated re-review of
develop@7e147527→chat-view-update@cdf7c2a2(29 files, +1311/−1174, 61 commits), triggered by/oc reviewon comment 118650. Follow-up to the earlier automated review at head664260e: the branch gained 14 commits since, and this pass verifies those fixes against the new head and looks for anything they introduced.Verification performed at head
cdf7c2a2: full unit suite in browser mode (chromium headless): 41 files, 486 passed, 2 skipped — green;biome checkclean;stylelintclean.Previously reported findings — all fixed ✅
chat_idsanity check is restored inaddMessages(but see new finding A below).messageRetriers.value.set(...)is correct now, and pending retry timers are cleared inclear(), so they can't fire into cleared state after switching/closing a chat.formPlaceholderreactivity →.valueis in the dependency chain now (but see new finding B below).streamingEnabled→ derived fromstatewith.valuein bothusePleromaChat.jsanduseConversation.js.ResizeObserverleak →disconnect()is actually invoked; activate failures surface viafetchError;deleteChatMessageguardsmaxBy/minBywith?.;suspendableStateChangenow emitschatItem.id, consistent withheightChange; unusedfaChevronLeft,ref="messageList"/ref="pendingMessageList"and theisFirstFetchparam are gone; the timeline load watcher re-checks while scrolling ([shouldLoadBottom, scrollY], debounced 100 ms), addressing the "won't re-fire while resting in the margin" concern.New findings (introduced by the fixes)
A.
addMessagesguard aborts the whole batch —usePleromaChat.jsline ~233The comment says foreign messages should be skipped, but
returninside theforloop drops every remaining message in the batch once one foreign message is seen. All three current call sites pass homogeneous batches (chatMessagesis fetched by chat id,onChatUpdatepasses a singlelastMessage, send confirmation is a single message), so nothing is lost today — but this is exactly the latent-trap shape of the original #1: the first time a caller passes a mixed batch, messages silently disappear.continuemakes the guard do what the comment promises.B.
formPlaceholderfix is incomplete —chat_view.jslines ~208–216The reactivity bug is fixed, but the condition still tests the ref object rather than
chatRecipient.value, so theelsebranch is dead code. Consequence: while the chat is loading — or after a failedgetOrCreateChat— the form placeholder renders"Message "(trailing space,nickname: undefined) instead of''; at base it was''until the recipient resolved. The form is visible in that state (only the submit button is disabled viachatReady). Fix:if (chatRecipient.value).Small
timeline.js: the debounced watcher callback isn't cancelled on unmount, so a pending 100 ms debounce can firefetchOlderStatuses()once after the timeline is gone. Harmless today, butdebounce.cancelinonUnmounted/onScopeDisposewould be tidy.usePleromaChat.js:handleMessageErroris invoked with{ chatId, ... }but only destructuresidempotencyKey/isRetry— harmless dead argument.Not re-raised, per discussion: the load-margin change (intentional, comment 118645) and additional send-error / chat-fork test coverage (declined, comment 118647).
Overall: every blocker and medium from the previous review is properly fixed and the suite is green. Finding A (
return→continue) is a two-character change worth making before merge; B is minor polish.Published by opencode-fix-bot · marker:
opencode-agent:job=88:generation=1