Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an admin POST endpoint to purge inactive Mattermost users, server-side reactivate/cleanup logic with Hive activity lookups and batch deactivation, makes Mattermost bootstrap resilient to per-channel failures, removes Remeda usages replacing deep clones with JSON cloning, enhances profile-metadata merging/tokens handling, and adjusts wallet asset fallback logic. Changes
Sequence DiagramsequenceDiagram
participant Client as API Client
participant Endpoint as cleanup-inactive Endpoint
participant Server as App Server
participant MmAPI as Mattermost Admin API
participant HiveAPI as Hive Bridge API
Client->>Endpoint: POST /api/mattermost/admin/cleanup-inactive (inactiveDays?)
Endpoint->>Endpoint: read auth token from cookies
Endpoint->>MmAPI: GET /users/me (validate super-admin)
MmAPI-->>Endpoint: current user
Endpoint->>Server: call cleanupInactiveMattermostUsers(inactiveDays)
Server->>MmAPI: list team members / users (paged)
MmAPI-->>Server: team members (with delete_at)
Server->>HiveAPI: hiveGetProfiles(usernames batch)
HiveAPI-->>Server: profiles (last activity timestamps)
Server->>Server: compute inactive users (cutoff)
Server->>MmAPI: deactivate users (admin API) / reactivate if needed
MmAPI-->>Server: deactivation/reactivation results
Server-->>Endpoint: {deactivated, checked, skipped, errors}
Endpoint-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
feruzm
added
the
patch
feruzm
added
patch
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/server/mattermost.ts (3)
807-813: Redundant user lookup on each deactivation — O(2N) API calls where O(N) suffices.
deactivateMattermostUserAsAdmin(account.name)internally callsfindMattermostUserto resolve the username → user ID, but you already resolved these IDs in the batch at Line 778. Consider extracting a lower-level helper that deactivates by user ID directly, avoiding one extra Mattermost API round-trip per deactivation.♻️ Proposed approach
Add a by-ID deactivation helper and use it in the cleanup loop:
+async function deactivateMattermostUserById(userId: string): Promise<void> { + await mmFetch(`/users/${userId}`, { + method: "DELETE", + headers: getAdminHeaders() + }); +}Then in the cleanup loop, build a name→id map from the already-fetched
usersarray and use it:+ const userIdByName = new Map(users.map((u) => [u.username, u.id])); // ... - await deactivateMattermostUserAsAdmin(account.name); + const uid = userIdByName.get(account.name); + if (uid) { + await deactivateMattermostUserById(uid); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/mattermost.ts` around lines 807 - 813, The loop calls deactivateMattermostUserAsAdmin(account.name) which re-resolves username→ID via findMattermostUser causing redundant API calls; add a new lower-level helper (e.g., deactivateMattermostUserById or deactivateUserById) that accepts a Mattermost user ID and performs the deactivation, build a name→id map from the already-fetched users array (the batch created around Line 778), then in the cleanup loop use map[account.name] to get the ID and call the new by-ID helper (fall back to existing deactivateMattermostUserAsAdmin only if the ID is missing) so each user is deactivated with a single API request.
16-21: Server and clientMattermostUserinterfaces are diverging.The server-side
MattermostUsernow includesdelete_at, while the client-side definition inapps/web/src/features/chat/mattermost-api.tshas neither. This is likely intentional (admin vs. user context), but worth noting to avoid accidental cross-imports. Consider adding a brief doc comment to clarify the server-only scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/mattermost.ts` around lines 16 - 21, The server-side MattermostUser interface now includes email and delete_at, which diverges from the client-side definition; add a short doc comment above the MattermostUser interface declaration that states this is a server-only/admin context type (not to be imported by client code) and briefly list the extra fields (email, delete_at) to prevent accidental cross-imports; update the comment near the MattermostUser symbol to make the scope explicit and mention where the client-side type lives so reviewers/maintainers can quickly see the intended separation.
768-823: Sequential deactivation could be slow for large cleanup runs.Each deactivation is awaited individually inside a
for…ofloop. For a team with many inactive users, this could take a long time. Consider parallelizing with bounded concurrency (e.g., batches of 5–10Promise.allSettledcalls), similar to the pattern already used indeleteMattermostDmPostsByUserAsAdmin(Line 504–519).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/mattermost.ts` around lines 768 - 823, The sequential deactivation loop is slow; replace the inner for...of that calls deactivateMattermostUserAsAdmin(account.name) with a bounded-concurrency batching approach (e.g., split accounts toDeactivate into chunks of 5–10 and use Promise.allSettled per chunk) so multiple deactivations run in parallel; after each batch, inspect results to increment deactivated and errors counters accordingly (increment deactivated for fulfilled promises, errors for rejected) while preserving skipped/checked logic and keeping hiveGetProfiles, deactivateMattermostUserAsAdmin, checked, deactivated, and errors as the referenced symbols to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/server/mattermost.ts`:
- Around line 807-813: The loop calls
deactivateMattermostUserAsAdmin(account.name) which re-resolves username→ID via
findMattermostUser causing redundant API calls; add a new lower-level helper
(e.g., deactivateMattermostUserById or deactivateUserById) that accepts a
Mattermost user ID and performs the deactivation, build a name→id map from the
already-fetched users array (the batch created around Line 778), then in the
cleanup loop use map[account.name] to get the ID and call the new by-ID helper
(fall back to existing deactivateMattermostUserAsAdmin only if the ID is
missing) so each user is deactivated with a single API request.
- Around line 16-21: The server-side MattermostUser interface now includes email
and delete_at, which diverges from the client-side definition; add a short doc
comment above the MattermostUser interface declaration that states this is a
server-only/admin context type (not to be imported by client code) and briefly
list the extra fields (email, delete_at) to prevent accidental cross-imports;
update the comment near the MattermostUser symbol to make the scope explicit and
mention where the client-side type lives so reviewers/maintainers can quickly
see the intended separation.
- Around line 768-823: The sequential deactivation loop is slow; replace the
inner for...of that calls deactivateMattermostUserAsAdmin(account.name) with a
bounded-concurrency batching approach (e.g., split accounts toDeactivate into
chunks of 5–10 and use Promise.allSettled per chunk) so multiple deactivations
run in parallel; after each batch, inspect results to increment deactivated and
errors counters accordingly (increment deactivated for fulfilled promises,
errors for rejected) while preserving skipped/checked logic and keeping
hiveGetProfiles, deactivateMattermostUserAsAdmin, checked, deactivated, and
errors as the referenced symbols to update.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation