Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThese changes refactor Mattermost integration logic across three files: replacing shared query client with instance-scoped client to prevent stale subscription data in bootstrap handler, filtering default channels (town-square, off-topic) from channel responses, and restructuring personal access token management to store and validate tokens via user props. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/app/api/mattermost/bootstrap/route.ts`:
- Around line 86-97: Add a regression test that verifies per-request
subscription reads don't reuse cached data: simulate two consecutive POST
bootstrap requests that return different subscription fixtures and assert the
second request does not auto-join communities from the first response.
Specifically, in test setup target the bootstrap route logic that constructs a
new QueryClient (symbol: QueryClient) and calls fetchQuery with
getAccountSubscriptionsQueryOptions(username), and ensure the test uses distinct
mock responses for those calls so the subscriptions variable (Subscription[]) is
refreshed per-request; fail the test if stale subscription data from the first
call causes auto-joining on the second.
In `@apps/web/src/app/api/mattermost/channels/route.ts`:
- Around line 111-119: Add a regression test that ensures
MATTERMOST_DEFAULT_CHANNELS filtering doesn't break default-channel selection:
craft a payload with channels including "town-square" and "off-topic" plus a
real channel, run the same code path that computes hasCategories and
filteredChannels (referencing MATTERMOST_DEFAULT_CHANNELS, hasCategories,
filteredChannels) and assert the chats-page-client's defaultChannelId resolution
still picks the intended non-default channel; add this test alongside the
chats-page-client tests and ensure it fails if the default channels are not
removed from the payload.
In `@apps/web/src/server/mattermost.ts`:
- Around line 280-291: isTokenValid currently treats every exception as an
invalid token and ensurePersonalToken swallows all getMattermostUserWithProps
failures, causing createToken to run on transient 5xx/network errors; change
both places to only treat 401/403 (or explicit auth error from
mmFetch/getMattermostUserWithProps) as “invalid” and proceed to create a new
PAT, but rethrow other errors so they surface (returning 502 upstream).
Concretely, in isTokenValid(token) inspect the caught error for an HTTP status
(e.g., err.status or err.response?.status) and return false only for 401/403,
otherwise rethrow; similarly in ensurePersonalToken when calling
getMattermostUserWithProps, check the error status and only call createToken()
on auth errors, otherwise propagate the error.
- Around line 295-307: The patch currently overwrites the user's props by
sending a single-key props object when calling mmFetch(`/users/${userId}/patch`)
to store result.token under CHAT_PAT_PROP, which can clobber existing props;
instead, fetch or use the existing props and merge the new key into them before
PATCHing. Update the code around mmFetch(`/users/${userId}/patch`) to read the
current user props (or use the same merge pattern used elsewhere in this
module), create a new props object that copies existing props and sets
[CHAT_PAT_PROP]: result.token, and send that merged props in the body so
existing fields like ecency_left_channels, DM privacy, and bans are preserved.
🪄 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: CHILL
Plan: Pro
Run ID: 9da7771b-adbb-48b4-84dd-dfccbfc6e1c3
📒 Files selected for processing (3)
apps/web/src/app/api/mattermost/bootstrap/route.tsapps/web/src/app/api/mattermost/channels/route.tsapps/web/src/server/mattermost.ts
| // 4) Hive subscriptions — use a per-request QueryClient. | ||
| // Do NOT use the shared getQueryClient() here: React's cache() is | ||
| // scoped to Server Components, not Route Handlers. In a long-lived | ||
| // container the shared QueryClient persists across requests, serving | ||
| // stale subscription data that causes auto-joining to unsubscribed communities. | ||
| let subscriptions: Subscription[] = []; | ||
| try { | ||
| const qc = new QueryClient(); | ||
| subscriptions = | ||
| (await getQueryClient().fetchQuery( | ||
| (await qc.fetchQuery( | ||
| getAccountSubscriptionsQueryOptions(username) | ||
| )) || []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a regression test for per-request subscription reads.
Auto-join decisions now depend on fetching subscriptions fresh for each bootstrap. Please add coverage that two consecutive POST calls with different subscription fixtures don't reuse the previous response and join stale communities. As per coding guidelines, All new features in @ecency/web require tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/mattermost/bootstrap/route.ts` around lines 86 - 97, Add
a regression test that verifies per-request subscription reads don't reuse
cached data: simulate two consecutive POST bootstrap requests that return
different subscription fixtures and assert the second request does not auto-join
communities from the first response. Specifically, in test setup target the
bootstrap route logic that constructs a new QueryClient (symbol: QueryClient)
and calls fetchQuery with getAccountSubscriptionsQueryOptions(username), and
ensure the test uses distinct mock responses for those calls so the
subscriptions variable (Subscription[]) is refreshed per-request; fail the test
if stale subscription data from the first call causes auto-joining on the
second.
| // Mattermost auto-joins users to these default channels when they join | ||
| // the team. Hide them since no Hive user intentionally joined these. | ||
| const MATTERMOST_DEFAULT_CHANNELS = new Set(["town-square", "off-topic"]); | ||
|
|
||
| const hasCategories = (categoriesResponse.categories || []).length > 0; | ||
| const filteredChannels = channels.filter((channel) => { | ||
| // Filter out Mattermost team default channels | ||
| if (MATTERMOST_DEFAULT_CHANNELS.has(channel.name)) return false; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add regression coverage for the default-channel filter.
apps/web/src/app/chats/_components/chats-page-client.tsx:16-25 derives defaultChannelId from this payload. Please add a test proving that removing "town-square" and "off-topic" still supports the default-channel selection flow expected by the chats page. As per coding guidelines, All new features in @ecency/web require tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/api/mattermost/channels/route.ts` around lines 111 - 119,
Add a regression test that ensures MATTERMOST_DEFAULT_CHANNELS filtering doesn't
break default-channel selection: craft a payload with channels including
"town-square" and "off-topic" plus a real channel, run the same code path that
computes hasCategories and filteredChannels (referencing
MATTERMOST_DEFAULT_CHANNELS, hasCategories, filteredChannels) and assert the
chats-page-client's defaultChannelId resolution still picks the intended
non-default channel; add this test alongside the chats-page-client tests and
ensure it fails if the default channels are not removed from the payload.
1 participant
Summary by CodeRabbit
Bug Fixes