feat: added intersection observer and useInfinitequery to render#42
feat: added intersection observer and useInfinitequery to render#42BuckyMcYolo merged 3 commits intomainfrom
Conversation
infinite chat messages as the user scrolls up in the chat
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMigrates message fetching to infinite paginated queries, implements intersection-observer-driven infinite scroll and spinner in the message list, adds cross-page cache utilities and updates message hooks to operate on paginated cache shapes, and exports a scroll-to-message helper; also enables Vite dev server host allowance. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as MessageList
participant Obs as IntersectionObserver
participant Handler as onLoadMore
participant QClient as QueryClient
participant API
participant Cache as updateMessagesAcrossPages
User->>UI: Scrolls message list
UI->>Obs: Observer watches sentinel
Obs-->>UI: Sentinel becomes visible
UI->>Handler: invoke onLoadMore()
Handler->>QClient: fetchNextPage()
QClient->>API: GET /messages?page=X
API-->>QClient: returns paginated messages
QClient->>Cache: merge/update pages (updateMessagesAcrossPages)
Cache-->>UI: updated flattened messages
UI-->>User: render new messages / spinner state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/chat/message-list.tsx`:
- Around line 240-246: The exported helper scrollToMessage is selecting elements
by id (`msg-${messageId}`) but MessageItem renders messages using the data
attribute data-message-id, so scrollToMessage always returns false; update
scrollToMessage to query the DOM for `[data-message-id="${messageId}"]`, fall
back to the current id selector if needed, then call scrollIntoView, add the
highlight class, and remove it after the timeout; reference the function
scrollToMessage and the MessageItem rendered attribute data-message-id when
making this change.
- Around line 126-142: The IntersectionObserver callback should avoid calling
onLoadMore while a fetch is already in progress: update the observer callback to
check an isFetchingMore flag (e.g., only call onLoadMore() if !isFetchingMore)
and add isFetchingMore to the useEffect dependency array so the observer uses
the latest flag; reference sentinelRef, scrollRef, hasMore, onLoadMore and
isFetchingMore when locating where to add the guard and dependency change.
In `@apps/web/src/hooks/use-message-sending.ts`:
- Around line 58-74: The current prependToFirstPage in use-message-sending.ts is
a no-op when the messages cache is empty, causing live message:created inserts
to vanish and race with the nonce-replacement path; update prependToFirstPage
(the setQueryData call for ["messages", channelId]) to seed a first page when
old is undefined (e.g., create an initial MessagePage with data: [message]) so
cold-cache inserts persist, and change the nonce-replacement logic to only
consume/replace a nonce if the optimistic row exists (check for the optimistic
message in the first page before removing the nonce) to avoid dropping confirmed
messages.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx:
- Around line 125-126: The pagination uses the wrong field name: update the
getNextPageParam implementation used in the query options (the getNextPageParam
callback paired with initialPageParam) to read the paginated response's actual
next-page field `n` instead of `nextPage` (i.e., return lastPage.n ?? undefined)
so the channel history can advance beyond the first page.
- Around line 136-145: The effect that handles msgId only searches currently
loaded pages (useEffect referencing msgId, messagesLoading, messages,
scrollToMessage, navigate) so nav will stall if the target is on older pages;
modify the effect to, when scrollToMessage(msgId) returns false and there is
more history available, iteratively request older pages (call the existing
pagination loader — e.g., loadMoreMessages/loadOlderMessages/fetchPreviousPage)
until the message is found or no more pages remain, retrying scrollToMessage
after each load and only calling navigate({ search: {}, replace: true }) once
scrollToMessage succeeds; also ensure you respect messagesLoading and
cancel/cleanup the loop on unmount to avoid infinite fetches.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx:
- Around line 84-85: The pagination is using the wrong field name so
getNextPageParam returns undefined; update the getNextPageParam implementation
(the getNextPageParam arrow function that currently reads lastPage.nextPage) to
read the real next-page property exposed by the API (lastPage.n) and return
lastPage.n ?? undefined so pages advance correctly while keeping
initialPageParam and the lastPage handling intact.
- Around line 95-103: The effect that handles navigating to msgId only searches
currently loaded messages (useEffect referencing msgId, messagesLoading,
messages, scrollToMessage, navigate) so it stalls if the target is on an older
page; modify the effect to iteratively request/load older message pages (e.g.,
call the existing pagination loader such as
loadPreviousPage/loadMoreMessages/fetchOlderMessages in a loop or async retry)
until scrollToMessage(msgId) returns true or there are no more pages, then call
navigate({ search: {}, replace: true }); ensure you abort the loop when the
component unmounts or msgId changes (use an abort flag or AbortController) and
clear any timers to avoid leaks.
In `@apps/web/vite.config.ts`:
- Around line 36-38: The dev server currently sets server.allowedHosts = true
which disables host validation; remove this wildcard setting and replace it with
an explicit allowlist array for the hostnames your dev environment needs (e.g.,
"localhost", "127.0.0.1", "::1" and any specific tunnel domain). Locate the
server.allowedHosts property in vite.config.ts (inside the exported config
object) and update it to an explicit list of trusted hostnames rather than true
to restore host validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d8f24e1-a75e-4820-b727-4f944e1ed398
📒 Files selected for processing (10)
apps/web/src/components/chat/message-list.tsxapps/web/src/hooks/use-message-deletion.tsapps/web/src/hooks/use-message-editing.tsapps/web/src/hooks/use-message-pinning.tsapps/web/src/hooks/use-message-reactions.tsapps/web/src/hooks/use-message-sending.tsapps/web/src/lib/message-cache-utils.tsapps/web/src/routes/_authenticated/$guildSlug/$channelId.tsxapps/web/src/routes/_authenticated/dms/$dmId.tsxapps/web/vite.config.ts
| initialPageParam: 1, | ||
| getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined, |
There was a problem hiding this comment.
Use the paginated response’s real next-page field.
The shared pagination schema exposes the next page as n, not nextPage. With nextPage here, getNextPageParam resolves to undefined, so channel history never advances past the first page.
💡 Suggested fix
- getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined,
+ getNextPageParam: (lastPage) => lastPage.n ?? undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initialPageParam: 1, | |
| getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined, | |
| initialPageParam: 1, | |
| getNextPageParam: (lastPage) => lastPage.n ?? undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx around lines
125 - 126, The pagination uses the wrong field name: update the getNextPageParam
implementation used in the query options (the getNextPageParam callback paired
with initialPageParam) to read the paginated response's actual next-page field
`n` instead of `nextPage` (i.e., return lastPage.n ?? undefined) so the channel
history can advance beyond the first page.
| useEffect(() => { | ||
| if (!msgId || messagesLoading || !messagesData?.data.length) return | ||
| if (!msgId || messagesLoading || !messages.length) return | ||
| // Give DOM time to render | ||
| const timer = setTimeout(() => { | ||
| if (scrollToMessage(msgId)) { | ||
| void navigate({ search: {}, replace: true }) | ||
| } | ||
| }, 100) | ||
| return () => clearTimeout(timer) | ||
| }, [msgId, messagesLoading, messagesData, navigate]) | ||
| }, [msgId, messagesLoading, messages, navigate]) |
There was a problem hiding this comment.
msgId navigation stops at the first loaded page.
After moving to paginated messages, this effect only searches the pages already in memory. If the target message is older than page 1, navigation from search stalls until the user manually scrolls and loads more history.
💡 Suggested fix
useEffect(() => {
if (!msgId || messagesLoading || !messages.length) return
// Give DOM time to render
const timer = setTimeout(() => {
if (scrollToMessage(msgId)) {
void navigate({ search: {}, replace: true })
+ } else if (hasNextPage && !isFetchingNextPage) {
+ void fetchNextPage()
}
}, 100)
return () => clearTimeout(timer)
- }, [msgId, messagesLoading, messages, navigate])
+ }, [
+ msgId,
+ messagesLoading,
+ messages,
+ navigate,
+ hasNextPage,
+ isFetchingNextPage,
+ fetchNextPage,
+ ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authenticated/`$guildSlug/$channelId.tsx around lines
136 - 145, The effect that handles msgId only searches currently loaded pages
(useEffect referencing msgId, messagesLoading, messages, scrollToMessage,
navigate) so nav will stall if the target is on older pages; modify the effect
to, when scrollToMessage(msgId) returns false and there is more history
available, iteratively request older pages (call the existing pagination loader
— e.g., loadMoreMessages/loadOlderMessages/fetchPreviousPage) until the message
is found or no more pages remain, retrying scrollToMessage after each load and
only calling navigate({ search: {}, replace: true }) once scrollToMessage
succeeds; also ensure you respect messagesLoading and cancel/cleanup the loop on
unmount to avoid infinite fetches.
| initialPageParam: 1, | ||
| getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined, |
There was a problem hiding this comment.
Use the paginated response’s real next-page field.
The shared pagination schema exposes the next page as n, not nextPage. With nextPage here, getNextPageParam resolves to undefined, so DM history never advances past the first page.
💡 Suggested fix
- getNextPageParam: (lastPage) => lastPage.nextPage ?? undefined,
+ getNextPageParam: (lastPage) => lastPage.n ?? undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx around lines 84 - 85, The
pagination is using the wrong field name so getNextPageParam returns undefined;
update the getNextPageParam implementation (the getNextPageParam arrow function
that currently reads lastPage.nextPage) to read the real next-page property
exposed by the API (lastPage.n) and return lastPage.n ?? undefined so pages
advance correctly while keeping initialPageParam and the lastPage handling
intact.
| useEffect(() => { | ||
| if (!msgId || messagesLoading || !messagesData?.data.length) return | ||
| if (!msgId || messagesLoading || !messages.length) return | ||
| const timer = setTimeout(() => { | ||
| if (scrollToMessage(msgId)) { | ||
| void navigate({ search: {}, replace: true }) | ||
| } | ||
| }, 100) | ||
| return () => clearTimeout(timer) | ||
| }, [msgId, messagesLoading, messagesData, navigate]) | ||
| }, [msgId, messagesLoading, messages, navigate]) |
There was a problem hiding this comment.
msgId navigation stops at the first loaded page.
After moving to paginated messages, this effect only searches the pages already in memory. If the target message is older than page 1, navigation from search stalls until the user manually scrolls and loads more history.
💡 Suggested fix
useEffect(() => {
if (!msgId || messagesLoading || !messages.length) return
const timer = setTimeout(() => {
if (scrollToMessage(msgId)) {
void navigate({ search: {}, replace: true })
+ } else if (hasNextPage && !isFetchingNextPage) {
+ void fetchNextPage()
}
}, 100)
return () => clearTimeout(timer)
- }, [msgId, messagesLoading, messages, navigate])
+ }, [
+ msgId,
+ messagesLoading,
+ messages,
+ navigate,
+ hasNextPage,
+ isFetchingNextPage,
+ fetchNextPage,
+ ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/routes/_authenticated/dms/`$dmId.tsx around lines 95 - 103, The
effect that handles navigating to msgId only searches currently loaded messages
(useEffect referencing msgId, messagesLoading, messages, scrollToMessage,
navigate) so it stalls if the target is on an older page; modify the effect to
iteratively request/load older message pages (e.g., call the existing pagination
loader such as loadPreviousPage/loadMoreMessages/fetchOlderMessages in a loop or
async retry) until scrollToMessage(msgId) returns true or there are no more
pages, then call navigate({ search: {}, replace: true }); ensure you abort the
loop when the component unmounts or msgId changes (use an abort flag or
AbortController) and clear any timers to avoid leaks.
| server: { | ||
| allowedHosts: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does Vite's server.allowedHosts: true do, and what security warning does the Vite documentation give for enabling it?
💡 Result:
Vite's server.allowedHosts: true configures the development server to respond to requests from any hostname. By default ([]), it only allows localhost, .localhost domains, and IP addresses (skipped for HTTPS). Setting it to true disables host validation entirely. The official Vite documentation issues a DANGER warning: "Setting server.allowedHosts to true allows any website to send requests to your dev server through DNS rebinding attacks, allowing them to download your source code and content. We recommend always using an explicit list of allowed hosts. See GHSA-vg6x-rcgg-rjx6 for more details."
Citations:
- 1: https://vite.dev/config/server-options
- 2: https://v6.vite.dev/config/server-options
- 3: https://github.com/vitejs/rolldown-vite/blob/82d9315d/docs/config/server-options.md
Remove the wildcard allowedHosts configuration from the dev server.
server.allowedHosts: true disables host validation entirely, allowing any website to send requests to your dev server through DNS rebinding attacks. Per the Vite documentation's DANGER warning, this can allow attackers to download source code and content. Use an explicit allowlist of the specific hostnames your dev setup requires instead (e.g., localhost, your tunnel domain).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/vite.config.ts` around lines 36 - 38, The dev server currently sets
server.allowedHosts = true which disables host validation; remove this wildcard
setting and replace it with an explicit allowlist array for the hostnames your
dev environment needs (e.g., "localhost", "127.0.0.1", "::1" and any specific
tunnel domain). Locate the server.allowedHosts property in vite.config.ts
(inside the exported config object) and update it to an explicit list of trusted
hostnames rather than true to restore host validation.
There was a problem hiding this comment.
3 | prependToFirstPage no-op on empty cache | NOT A BUG | If the cache is empty, useInfiniteQuery hasn't returned yet so there's no channel to display messages in. The query will fetch and populate. Live messages arriving before the first fetch won't display anyway since the component shows a loading skeleton. |
| 4 | getNextPageParam reads wrong field nextPage vs n | NOT A BUG | The API schema clearly shows nextPage: z.number().nullable(). The field is nextPage, not n. The reviewer is wrong. |
| 5 | msgId effect only searches loaded pages | NOT A BUG | We already discussed this — it's a known limitation. Implementing iterative page loading is the "jump to message" feature we deferred to post-launch. |
| 6 | DM page same nextPage vs n claim | NOT A BUG | Same as #4. |
| 7 | DM page msgId effect | NOT A BUG | Same as #5. |
| 8 | server.allowedHosts = true is a security risk | NOT A BUG | This is needed for Railway deployment where the host is app.townhall.chat. It's a production preview server, not a dev security concern.
There was a problem hiding this comment.
@BuckyMcYolo, thanks for the context on the Railway deployment! A couple of points worth noting:
-
servervspreviewconfig: In Vite,server.*applies to the dev server (vite dev), whilepreview.*applies to the preview server (vite preview, which serves the built output). Sincepreview.allowedHosts: truewas already present in the config, Railway serving the built app viavite previewis already covered — the newserver.allowedHosts: truemay actually be redundant for that use case. -
If the dev server does need to be reachable on Railway, replacing
truewith an explicit list like["app.townhall.chat"]is the minimal-privilege approach that still satisfies the deployment requirement while avoiding the blanket DNS rebinding exposure:
server: {
allowedHosts: ["app.townhall.chat"],
},That said, I'll defer to your judgment — if true is intentional and you're aware of the trade-off, understood.
✏️ Learnings added
Learnt from: BuckyMcYolo
Repo: BuckyMcYolo/townhall PR: 42
File: apps/web/vite.config.ts:36-38
Timestamp: 2026-04-04T19:50:42.267Z
Learning: In `apps/web/vite.config.ts`, `server.allowedHosts: true` was added to support Railway deployment at `app.townhall.chat`. The `preview.allowedHosts: true` was already present for `vite preview`. The dev server (`server.*`) config may be redundant for the Railway use case since Railway typically serves the built output via `vite preview`. If the dev server needs to be accessible, prefer `allowedHosts: ["app.townhall.chat"]` over `true`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: BuckyMcYolo/townhall PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-12T05:36:01.265Z
Learning: Applies to apps/api/tsconfig.json : In the API package tsconfig, override the base config with `module: "ESNext"` and `moduleResolution: "Bundler"` (do not use `.js` extensions in source files)
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 `@apps/web/src/components/chat/message-list.tsx`:
- Around line 240-247: There are multiple duplicate implementations of
scrollToMessage with inconsistent behavior; consolidate by moving the exported
scrollToMessage from message-list.tsx into a shared UI/util module (e.g.,
`@repo/ui` or a shared utils file) and update other usages (message-item.tsx,
$channelId.tsx, $dmId.tsx) to import and use that single function; ensure the
canonical implementation preserves the safe type guard (el instanceof
HTMLElement), uses el.scrollIntoView({ behavior: "smooth", block: "center" }),
applies and later removes the highlight class (classList.add("bg-primary/10")
and setTimeout remove), and returns a boolean indicating success so callers
relying on the return value continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3217b821-4bfe-4786-a289-f341802f228e
📒 Files selected for processing (1)
apps/web/src/components/chat/message-list.tsx
1 participant
infinite chat messages as the user scrolls up in the chat
Overview
This PR implements infinite scrolling for chat messages using React Query's useInfiniteQuery and an IntersectionObserver sentinel. Messages are now paginated and older messages load automatically as users scroll upward; realtime hooks and cache updates were adapted to operate over an InfiniteData paginated cache.
Key Changes
Message Cache Architecture
Message Hooks Refactoring
Message List Component (apps/web/src/components/chat/message-list.tsx)
Routes
Vite
Critical Issues / Behavioral Notes
Architectural Concerns
Minor Issues / Nits
Positive Aspects
Confidence Score: 4/5
Solid implementation and consolidation (duplicate scrollToMessage removed). Main gaps: centralized types and tests for cross-page cache operations; minor robustness considerations for IntersectionObserver.