Repository navigation
feat(webapp): add guild rbac with role-based dashboard access - #165
Merged
Merged
Conversation
Contributor
|
Deployment failed with the following error: Learn More: https://vercel.com/luksantanas-projects?upgradeToPro=build-rate-limit |
✅ Deploy Preview for regal-bunny-0c8efe ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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 guild-scoped RBAC (DB model, shared service, cache), a GuildAccessService and middleware, RBAC GET/PUT endpoints and member-context endpoint, frontend route guards and Server Settings Access Control UI, guild metrics caching, auth/user globalName surface, and extensive tests and type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant Frontend as Frontend App
participant API as Backend API
participant GAS as GuildAccessService
participant GRAS as GuildRoleAccessService
participant DB as Database/Prisma
User->>Frontend: navigate to guarded route / open Server Settings
Frontend->>API: GET /api/guilds/:id/me
API->>GAS: resolveGuildContext(session, guildId)
GAS->>DB: fetch user guilds / member info
GAS->>GRAS: resolveEffectiveAccess(guildId, roleIds)
GRAS->>DB: SELECT guild_role_grants WHERE guildId=...
GRAS-->>GAS: effectiveAccess map
GAS-->>API: GuildMemberContext (roleIds, effectiveAccess, canManageRbac)
API-->>Frontend: member context payload
Frontend->>Frontend: RouteModuleGuard checks effectiveAccess -> allow/deny
alt user can manage RBAC
Frontend->>API: GET /api/guilds/:id/rbac
API->>GRAS: listRoleGrants(guildId)
GRAS->>DB: SELECT grants
GRAS-->>API: grants
Frontend->>API: PUT /api/guilds/:id/rbac (save)
API->>GRAS: replaceRoleGrants(guildId, grants)
GRAS->>DB: transaction replace grants
GRAS->>GRAS: invalidate cache
GRAS-->>API: updated grants
API-->>Frontend: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/frontend/src/pages/DashboardOverview.tsx (1)
195-201:⚠️ Potential issue | 🟠 MajorGate these dashboard shortcuts with the same RBAC check as the sidebar/routes.
View alland the fourOpenlinks always render. A member who can open the overview but notmoderation,automation,logs, orcommandswill still get navigation into blocked modules, which contradicts the new navigation-gating behavior and creates dead-end flows.Also applies to: 251-302
🧹 Nitpick comments (12)
README.md (1)
236-247: Consider renaming the asset file for clarity.The asset filename
assets/Gemini_Generated_Image_udmhrpudmhrpudmh.pngappears to be an AI-generated placeholder with a non-descriptive suffix. For production documentation and maintainability, consider renaming it to something more meaningful (e.g.,assets/criativaria-embed-image.pngorassets/server-setup-banner.png).📝 Verification script to check if the asset exists
#!/bin/bash # Description: Verify the asset file exists and check for alternative naming # Check if the specific asset exists echo "=== Checking for the asset file ===" fd -t f "Gemini_Generated_Image_udmhrpudmhrpudmh.png" assets/ # Check for other images in assets directory echo -e "\n=== Other images in assets/ ===" fd -t f -e png -e jpg -e jpeg assets/ | head -20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 236 - 247, The README references a non-descriptive asset filename "assets/Gemini_Generated_Image_udmhrpudmhrpudmh.png"; rename that file to a meaningful name (e.g., assets/criativaria-embed-image.png or assets/server-setup-banner.png), update every reference in README.md and any template or code that uses the exact filename (search for "Gemini_Generated_Image_udmhrpudmhrpudmh.png"), and add a brief note in README about the new filename; optionally update the provided verification script to search for the new filename.packages/backend/src/middleware/auth.ts (1)
8-16: ReuseGuildAccessContextinstead of copying its shape.This inline type has already drifted from
packages/backend/src/services/GuildAccessService.ts:11-19: it dropshasBotand widenseffectiveAccesstoRecord<string, ...>. Importing the shared type keeps middleware consumers aligned with the real resolver contract and avoids another copy going stale.As per coding guidelines "Introduce interfaces at module boundaries to enable testing and substitutions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/middleware/auth.ts` around lines 8 - 16, Replace the inline guildContext shape in the auth middleware with the shared GuildAccessContext type: import GuildAccessContext from where it’s defined (the service that currently declares it, e.g., the symbol GuildAccessContext in GuildAccessService) and change the guildContext declaration to use guildContext?: GuildAccessContext; so you stop duplicating the shape (removing the custom fields and the widened effectiveAccess) and keep middleware consumers aligned with the resolver contract.packages/backend/tests/unit/services/GuildService.test.ts (1)
231-242: Assert the cache behavior, not the current fetch graph.
toHaveBeenCalledTimes(4)bakes in today'sgetBotGuildIds()+getGuildMetrics()implementation. A harmless metrics refactor will fail this test even if fallback caching still works. Prefer asserting that the bot-guild lookup endpoint is hit once across both calls.♻️ Suggested change
await guildService.enrichGuildsWithBotStatus(MOCK_DISCORD_GUILDS) await guildService.enrichGuildsWithBotStatus(MOCK_DISCORD_GUILDS) - expect(fetchMock).toHaveBeenCalledTimes(4) + const botGuildLookups = fetchMock.mock.calls.filter( + ([url]) => + url === 'https://discord.com/api/v10/users/@me/guilds', + ) + expect(botGuildLookups).toHaveLength(1)As per coding guidelines "Test behavior, not implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/unit/services/GuildService.test.ts` around lines 231 - 242, The test currently asserts fetchMock was called 4 times, coupling it to internal calls; change it to assert the bot-guild lookup endpoint is hit once across both enrichGuildsWithBotStatus() calls instead. Replace the toHaveBeenCalledTimes(4) assertion with one that checks the fetchMock was called exactly once for the bot-guild lookup (e.g., inspect fetchMock calls to find requests matching the getBotGuildIds() endpoint or URL pattern) or assert fetchMock.mock.calls.filter(c => matchesBotLookup(c)).length === 1 so the test verifies caching behavior rather than the total fetch call graph tied to getGuildMetrics() or other internals.packages/backend/tests/integration/api.test.ts (1)
192-233: Assert the new RBAC contract in/api/guilds.This test now seeds
effectiveAccessandcanManageRbacinto the mocked guilds, but it only verifies thatguildsis an array. If either field stops being serialized, the test still passes while the frontend guard logic breaks.Possible assertion upgrade
expect(response.body).toHaveProperty('guilds') expect(Array.isArray(response.body.guilds)).toBe(true) + expect(response.body.guilds[0]).toMatchObject({ + effectiveAccess: expect.objectContaining({ + overview: 'manage', + settings: 'manage', + }), + canManageRbac: true, + })Based on learnings, "Add or adjust unit and integration tests when changing behavior; follow existing patterns in
packages/*/testsand roottests/directories".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/integration/api.test.ts` around lines 192 - 233, The test seeds enrichedGuilds (via guildAccessService.listAuthorizedGuilds) with effectiveAccess and canManageRbac but only asserts response.body.guilds is an array; update the test near the GET /api/guilds call to assert each returned guild contains an effectiveAccess object with the expected keys (overview, settings, moderation, automation, music, integrations) and that canManageRbac is present and a boolean (or matches the seeded value), e.g., iterate response.body.guilds and assert typeof guild.canManageRbac === 'boolean' and typeof guild.effectiveAccess === 'object' and the required access keys exist to ensure the RBAC contract from listAuthorizedGuilds is serialized.packages/frontend/src/components/Layout/Sidebar.test.tsx (1)
285-306: Avoid coupling this test to Tailwind classes.Lines 285-306 reach into
document.querySelector(...)and assert a specific class string plustransformvalue, so a harmless DOM/class refactor will break the test even if the mobile sidebar still opens and closes correctly. Query the mobile panel through accessible output and assert hidden/visible behavior instead.Based on learnings, "Test behavior, not implementation details".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/Layout/Sidebar.test.tsx` around lines 285 - 306, The test is tightly coupled to Tailwind classes and inline transform checks; replace the document.querySelector lookup and style assertion with accessible queries and visibility assertions: locate the mobile panel via accessible role/label (e.g., use within(container).getByRole('dialog' or 'region', { name: /sidebar|navigation/i }) or getByLabelText/getByTestId) instead of document.querySelector, click the close button (mobileCloseButton via getByRole) and then use waitFor / waitForElementToBeRemoved or expect(...).not.toBeVisible() / expect(...).not.toBeInTheDocument() on that accessible element (instead of asserting transform and specific class names) so the test asserts behavior, not implementation details (refer to mobileSidebar, mobileCloseButton, within, user.click, waitFor).packages/frontend/src/stores/guildStore.test.ts (1)
81-116: This test still doesn't exercise the "skip unauthorized guilds" path.
api.guilds.getMeis mocked with the same payload for every call, so this can still pass even iffetchGuilds()never moves past an unauthorized first guild. Make the mock depend on the requested guild id and keep the first guild unauthorized so the new fallback behavior is actually covered.Based on learnings: Add or adjust unit and integration tests when changing behavior; follow existing patterns in
packages/*/testsand roottests/directories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/stores/guildStore.test.ts` around lines 81 - 116, The test currently stubs api.guilds.getMe with the same authorized payload for every call so it never verifies skipping an unauthorized first guild; update the mock for api.guilds.getMe (used by useGuildStore.getState().fetchGuilds) to vary by requested guild id (or use mockResolvedValueOnce sequence) so that the first mock response represents an unauthorized user for guild id '1' and the second returns an authorized payload for guild id '2' (keep references to mockGuild, fetchGuilds, selectedGuildId and api.guilds.getMe to locate code), ensuring the new fallback path that skips unauthorized guilds is exercised.packages/backend/src/routes/rbac.ts (1)
35-106: Extract the shared RBAC guard and grant mappers.Both handlers repeat the same
canManageRbaccheck and grant serialization logic, andsetupRbacRoutes()is already well past the repo's 50-line function cap. Pulling those blocks into small helpers will cut drift between GET/PUT and keep this setup function easier to maintain.As per coding guidelines: Functions must be less than 50 lines with cyclomatic complexity less than 10.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/routes/rbac.ts` around lines 35 - 106, Extract the repeated RBAC guard and grant mapping into small helpers to keep setupRbacRoutes under 50 lines: add a checkCanManageRbac(req: AuthenticatedRequest) helper that performs the req.guildContext?.canManageRbac check and throws AppError.forbidden with the same message, and add two mappers serializeGrant(grant) returning { roleId, module, mode } and parseGrant(dto) returning { roleId: dto.roleId, module: dto.module as ModuleKey, mode: dto.mode as AccessMode }; then replace the inline checks and map logic in both handlers (the GET/PUT handlers inside setupRbacRoutes that call guildRoleAccessService.listRoleGrants, guildService.getGuildRoleOptions, and guildRoleAccessService.replaceRoleGrants, and that use rbacUpdateSchema) to call checkCanManageRbac(req) and use serializeGrant/parseGrant accordingly.packages/frontend/src/stores/guildStore.ts (1)
62-64: Silent error swallowing may hide issues.The
fetchMemberContextcall has.catch(() => {})which silently ignores all errors. If the/meendpoint fails (e.g., network issues, unauthorized), the user sees no feedback andmemberContextremainsnull.Consider logging the error or providing user feedback for persistent failures.
📝 Proposed improvement: Log fetch failures
if (guild) { get() .fetchMemberContext(guild.id) - .catch(() => {}) + .catch((error) => { + console.error('Failed to fetch member context:', error) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/stores/guildStore.ts` around lines 62 - 64, The code silently swallows errors from get().fetchMemberContext(guild.id) via .catch(() => {}); replace that with proper error handling in the guildStore: catch the error from fetchMemberContext and log it (using your store logger or console.error) and/or set an error state or retry flag so the UI can show feedback instead of leaving memberContext null; ensure you reference fetchMemberContext and the memberContext state in your fix and avoid rethrowing unhandled exceptions unless upstream should handle them.packages/backend/src/services/GuildAccessService.ts (1)
42-52: Clarify: Admin users skip member context fetch intentionally.When
isAdminis true,memberContextdefaults to emptyroleIds. This is correct becauseresolveEffectiveAccessuses theisAdminOverrideflag to grant full access regardless of roles. However, this meansroleIdsin the returned context will be empty for admins.If downstream code expects accurate
roleIdsfor admin users (e.g., for display purposes), consider fetching member context unconditionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildAccessService.ts` around lines 42 - 52, Currently memberContext is only fetched when hasBot && !isAdmin which leaves roleIds empty for admins; if downstream code needs real admin roleIds (for display or auditing) change the logic to always call guildService.getGuildMemberContext(guild.id, userId) to populate memberContext, then pass memberContext.roleIds into guildRoleAccessService.resolveEffectiveAccess(guild.id, memberContext.roleIds, isAdmin) (you can still rely on the isAdmin flag to grant overrides). This preserves correct roleIds for admins while keeping resolveEffectiveAccess behavior unchanged.packages/backend/src/services/GuildService.ts (2)
325-340: Parallel API fetches lack timeout protection.Three Discord API requests are made in parallel without individual timeouts. If Discord's API is slow or unresponsive, these requests could hang indefinitely, blocking the caller.
🛡️ Proposed improvement: Add fetch timeout
+const DISCORD_API_TIMEOUT_MS = 5000 + +function fetchWithTimeout( + url: string, + options: RequestInit, + timeoutMs = DISCORD_API_TIMEOUT_MS, +): Promise<Response> { + return Promise.race([ + fetch(url, options), + new Promise<never>((_, reject) => + setTimeout( + () => reject(new Error('Discord API timeout')), + timeoutMs, + ), + ), + ]) +}Then use
fetchWithTimeoutinstead offetchfor Discord API calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildService.ts` around lines 325 - 340, The three parallel Discord requests (producing guildResponse, channelsResponse, rolesResponse) can hang because they use raw fetch without timeouts; update the code in the method that performs these Promise.all calls to use a shared fetchWithTimeout helper (or implement one using AbortController) in place of fetch for each URL, pass the same headers, and provide a sensible timeout value (e.g., 5-10s) so each request is aborted on timeout; ensure you import/define fetchWithTimeout and handle aborted/error responses the same way you currently handle non-2xx responses.
214-233: Consider using named constants for Discord channel types.Magic numbers (
4,2,13,0,5,15,16) represent Discord channel types but are not self-documenting. This makes maintenance harder when Discord adds new channel types.📝 Proposed improvement: Extract channel type constants
+const DISCORD_CHANNEL_TYPES = { + GUILD_TEXT: 0, + GUILD_VOICE: 2, + GUILD_CATEGORY: 4, + GUILD_ANNOUNCEMENT: 5, + GUILD_STAGE_VOICE: 13, + GUILD_FORUM: 15, + GUILD_MEDIA: 16, +} as const + private countChannelTypes( channels: DiscordGuildChannel[], ): Pick< GuildMetrics, 'categoryCount' | 'textChannelCount' | 'voiceChannelCount' > { let categoryCount = 0 let textChannelCount = 0 let voiceChannelCount = 0 for (const channel of channels) { - if (channel.type === 4) { + if (channel.type === DISCORD_CHANNEL_TYPES.GUILD_CATEGORY) { categoryCount += 1 continue } // ... similar for other types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/services/GuildService.ts` around lines 214 - 233, Replace the hard-coded numeric channel.type checks in GuildService (the loop iterating over channels updating categoryCount, voiceChannelCount, textChannelCount) with named constants or an enum (e.g., use the ChannelType enum from discord.js or define local constants like CHANNEL_TYPE_GUILD_CATEGORY, CHANNEL_TYPE_VOICE, CHANNEL_TYPE_STAGE_VOICE, CHANNEL_TYPE_GUILD_TEXT, CHANNEL_TYPE_NEWS, CHANNEL_TYPE_FORUM, CHANNEL_TYPE_PUBLIC_THREAD). Update the conditionals to compare against these named symbols so the intent is clear and maintainable, and add the necessary import or constant declarations near the top of the module.packages/shared/src/services/GuildRoleAccessService.ts (1)
189-196: Silent skip of invalid grants may cause confusion.Invalid
RoleGrantInputitems are silently filtered out without feedback. If an API caller passes malformed grants, they receive a success response but the invalid grants are not persisted. This could lead to debugging difficulties.Consider logging when invalid grants are skipped, or relying solely on the Zod validation at the route layer (which already enforces valid module/mode values per context snippet from
rbac.ts:69-105).📝 Optional: Add debug logging for skipped grants
for (const item of input) { if (!isRoleGrantInput(item)) { + debugLog({ + message: 'Skipping invalid role grant input', + data: { item }, + }) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/services/GuildRoleAccessService.ts` around lines 189 - 196, The loop in GuildRoleAccessService silently skips invalid RoleGrantInput items (input loop using isRoleGrantInput and deduped), which hides bad data; update the loop to log a warning (including guildId and the offending item) whenever isRoleGrantInput(item) returns false so callers/operators can see which grants were skipped, or alternatively make it strict by throwing an error for invalid items; use the service's logger (or a provided logger) to emit the message and keep the deduped.set behavior for valid items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/backend/src/routes/guilds.ts`:
- Around line 42-53: The route is doing an expensive full scan by calling
guildAccessService.listAuthorizedGuilds() to find one guild; instead, keep the
requireGuildModuleAccess('overview') pre-check and replace the list call with a
targeted lookup that builds/enriches only the requested guild (e.g., add and
call guildAccessService.getAuthorizedGuildById(sessionData, id) or similar).
Change the handler to use sessionService.getSession(req.sessionId) as before,
then invoke a new or existing single-guild method on GuildAccessService that
returns the enriched guild or null (avoid listAuthorizedGuilds), and throw
AppError.unauthorized if it’s not authorized or not found. Ensure the new method
performs the same access-resolution/enrichment logic that listAuthorizedGuilds
does but only for the given guild id.
In `@packages/frontend/src/components/Layout/Sidebar.tsx`:
- Around line 164-173: The canViewModule function currently returns true when
access info is unresolved, which allows all nav items to flash; update the guard
so we only allow by default when there is no selectedGuild at all. Specifically,
change the early-return logic in canViewModule (referencing effectiveAccess,
memberContext, selectedGuild) so that if selectedGuild is falsy return true, but
if selectedGuild exists and effectiveAccess is falsy return false; otherwise
call hasModuleAccess(effectiveAccess, module, 'view') as before.
- Around line 330-379: The section title is rendered even when RBAC filtering
hides all items; change the rendering to first compute visibleItems =
section.items.filter(item => canViewModule(item.module)) and only render/map the
list when visibleItems.length > 0, replacing uses of section.items.map with
visibleItems.map (keep existing uses of isActive, item.icon, item.badge, and
key={item.path}).
In `@packages/frontend/src/pages/ServerSettings.tsx`:
- Around line 124-176: Prevent duplicate (roleId, module) pairs across
rbacGrants: when adding in addRbacGrant, choose a role/module combination that
doesn't already exist (or return/show error) instead of always seeding with the
first role and 'overview'; in updateRbacGrant, check the proposed merged grant
against other existing grants and refuse or reject the update if it would create
a duplicate; and in handleSaveRbac validate rbacGrants for unique
(roleId,module) before calling api.guilds.updateRbac and abort with a toast
error if duplicates are found. Reference rbacGrants, rbacRoles, addRbacGrant,
updateRbacGrant, and handleSaveRbac when implementing these checks.
- Around line 84-102: The useEffect that fetches RBAC for selectedGuild
(dependencies selectedGuild?.id and canManageRbac) can leave stale roles/grants
when the user switches guilds mid-request; fix by cancelling or ignoring
out-of-date responses: create an AbortController (or a local
"activeRequestId"/mounted flag) inside the effect, call api.guilds.getRbac with
the controller/signal (or check the id) and on start clear existing state via
setRbacRoles([])/setRbacGrants([])/setRbacLoading(true); in the promise handlers
only call setRbacRoles/setRbacGrants/setRbacLoading if the controller is not
aborted (or the request id matches), and ensure you abort the controller (or
flip the flag) in the effect cleanup so in-flight responses are ignored and old
data is never written back.
- Around line 454-530: The RBAC row controls (the Select instances for
grant.roleId, grant.module, grant.mode and the delete Button invoking
removeRbacGrant) lack accessible names; add explicit accessible labels by
supplying aria-label or aria-labelledby to each Select (or its SelectTrigger)
such as aria-label={`Role for rule ${index + 1}`}, aria-label={`Module for rule
${index + 1}`}, aria-label={`Mode for rule ${index + 1}`}, and set the delete
Button to aria-label={`Remove rule ${index + 1}`} (or use a visually-hidden
<span> with an id and reference it via aria-labelledby) so assistive tech has
stable, descriptive names while keeping existing handlers like updateRbacGrant
and removeRbacGrant unchanged.
In `@packages/shared/src/services/GuildRoleAccessService.ts`:
- Around line 114-117: The code directly JSON.parse(s) the cached value and
casts to RoleGrant[] without validation; wrap the parse in a try/catch to guard
against malformed JSON and implement a runtime type guard (e.g.,
isRoleGrantArray(obj)) that verifies Array.isArray(obj) and that each entry has
the expected RoleGrant properties and types before returning it from the
GuildRoleAccessService method that parses `value`; if parsing or validation
fails, log the error via the service logger and return a safe fallback (empty
array) instead of casting invalid data.
In `@prisma/migrations/20260311110000_add_guild_role_grants/migration.sql`:
- Around line 5-6: The "module" and "mode" columns are currently TEXT and need
DB-level constraints to prevent invalid values; update the migration.sql to
constrain them by either creating PostgreSQL ENUM types (e.g., grant_module,
grant_mode) and using those types for the "module" and "mode" columns, or add
explicit CHECK constraints listing the allowed module keys and access modes for
the "module" and "mode" columns respectively, and ensure any existing
INSERTs/UPDATES in this migration use valid enum/check values.
In `@README.md`:
- Line 160: Update README.md to replace the incorrect OAuth redirect URI
`https://lucky-api.lucassantana.tech/api/auth/callback` with the correct
production URI `https://lucky.lucassantana.tech/api/auth/callback` in both
places it appears (the instance shown in the diff and the other occurrence
referenced in the comment).
---
Nitpick comments:
In `@packages/backend/src/middleware/auth.ts`:
- Around line 8-16: Replace the inline guildContext shape in the auth middleware
with the shared GuildAccessContext type: import GuildAccessContext from where
it’s defined (the service that currently declares it, e.g., the symbol
GuildAccessContext in GuildAccessService) and change the guildContext
declaration to use guildContext?: GuildAccessContext; so you stop duplicating
the shape (removing the custom fields and the widened effectiveAccess) and keep
middleware consumers aligned with the resolver contract.
In `@packages/backend/src/routes/rbac.ts`:
- Around line 35-106: Extract the repeated RBAC guard and grant mapping into
small helpers to keep setupRbacRoutes under 50 lines: add a
checkCanManageRbac(req: AuthenticatedRequest) helper that performs the
req.guildContext?.canManageRbac check and throws AppError.forbidden with the
same message, and add two mappers serializeGrant(grant) returning { roleId,
module, mode } and parseGrant(dto) returning { roleId: dto.roleId, module:
dto.module as ModuleKey, mode: dto.mode as AccessMode }; then replace the inline
checks and map logic in both handlers (the GET/PUT handlers inside
setupRbacRoutes that call guildRoleAccessService.listRoleGrants,
guildService.getGuildRoleOptions, and guildRoleAccessService.replaceRoleGrants,
and that use rbacUpdateSchema) to call checkCanManageRbac(req) and use
serializeGrant/parseGrant accordingly.
In `@packages/backend/src/services/GuildAccessService.ts`:
- Around line 42-52: Currently memberContext is only fetched when hasBot &&
!isAdmin which leaves roleIds empty for admins; if downstream code needs real
admin roleIds (for display or auditing) change the logic to always call
guildService.getGuildMemberContext(guild.id, userId) to populate memberContext,
then pass memberContext.roleIds into
guildRoleAccessService.resolveEffectiveAccess(guild.id, memberContext.roleIds,
isAdmin) (you can still rely on the isAdmin flag to grant overrides). This
preserves correct roleIds for admins while keeping resolveEffectiveAccess
behavior unchanged.
In `@packages/backend/src/services/GuildService.ts`:
- Around line 325-340: The three parallel Discord requests (producing
guildResponse, channelsResponse, rolesResponse) can hang because they use raw
fetch without timeouts; update the code in the method that performs these
Promise.all calls to use a shared fetchWithTimeout helper (or implement one
using AbortController) in place of fetch for each URL, pass the same headers,
and provide a sensible timeout value (e.g., 5-10s) so each request is aborted on
timeout; ensure you import/define fetchWithTimeout and handle aborted/error
responses the same way you currently handle non-2xx responses.
- Around line 214-233: Replace the hard-coded numeric channel.type checks in
GuildService (the loop iterating over channels updating categoryCount,
voiceChannelCount, textChannelCount) with named constants or an enum (e.g., use
the ChannelType enum from discord.js or define local constants like
CHANNEL_TYPE_GUILD_CATEGORY, CHANNEL_TYPE_VOICE, CHANNEL_TYPE_STAGE_VOICE,
CHANNEL_TYPE_GUILD_TEXT, CHANNEL_TYPE_NEWS, CHANNEL_TYPE_FORUM,
CHANNEL_TYPE_PUBLIC_THREAD). Update the conditionals to compare against these
named symbols so the intent is clear and maintainable, and add the necessary
import or constant declarations near the top of the module.
In `@packages/backend/tests/integration/api.test.ts`:
- Around line 192-233: The test seeds enrichedGuilds (via
guildAccessService.listAuthorizedGuilds) with effectiveAccess and canManageRbac
but only asserts response.body.guilds is an array; update the test near the GET
/api/guilds call to assert each returned guild contains an effectiveAccess
object with the expected keys (overview, settings, moderation, automation,
music, integrations) and that canManageRbac is present and a boolean (or matches
the seeded value), e.g., iterate response.body.guilds and assert typeof
guild.canManageRbac === 'boolean' and typeof guild.effectiveAccess === 'object'
and the required access keys exist to ensure the RBAC contract from
listAuthorizedGuilds is serialized.
In `@packages/backend/tests/unit/services/GuildService.test.ts`:
- Around line 231-242: The test currently asserts fetchMock was called 4 times,
coupling it to internal calls; change it to assert the bot-guild lookup endpoint
is hit once across both enrichGuildsWithBotStatus() calls instead. Replace the
toHaveBeenCalledTimes(4) assertion with one that checks the fetchMock was called
exactly once for the bot-guild lookup (e.g., inspect fetchMock calls to find
requests matching the getBotGuildIds() endpoint or URL pattern) or assert
fetchMock.mock.calls.filter(c => matchesBotLookup(c)).length === 1 so the test
verifies caching behavior rather than the total fetch call graph tied to
getGuildMetrics() or other internals.
In `@packages/frontend/src/components/Layout/Sidebar.test.tsx`:
- Around line 285-306: The test is tightly coupled to Tailwind classes and
inline transform checks; replace the document.querySelector lookup and style
assertion with accessible queries and visibility assertions: locate the mobile
panel via accessible role/label (e.g., use within(container).getByRole('dialog'
or 'region', { name: /sidebar|navigation/i }) or getByLabelText/getByTestId)
instead of document.querySelector, click the close button (mobileCloseButton via
getByRole) and then use waitFor / waitForElementToBeRemoved or
expect(...).not.toBeVisible() / expect(...).not.toBeInTheDocument() on that
accessible element (instead of asserting transform and specific class names) so
the test asserts behavior, not implementation details (refer to mobileSidebar,
mobileCloseButton, within, user.click, waitFor).
In `@packages/frontend/src/stores/guildStore.test.ts`:
- Around line 81-116: The test currently stubs api.guilds.getMe with the same
authorized payload for every call so it never verifies skipping an unauthorized
first guild; update the mock for api.guilds.getMe (used by
useGuildStore.getState().fetchGuilds) to vary by requested guild id (or use
mockResolvedValueOnce sequence) so that the first mock response represents an
unauthorized user for guild id '1' and the second returns an authorized payload
for guild id '2' (keep references to mockGuild, fetchGuilds, selectedGuildId and
api.guilds.getMe to locate code), ensuring the new fallback path that skips
unauthorized guilds is exercised.
In `@packages/frontend/src/stores/guildStore.ts`:
- Around line 62-64: The code silently swallows errors from
get().fetchMemberContext(guild.id) via .catch(() => {}); replace that with
proper error handling in the guildStore: catch the error from fetchMemberContext
and log it (using your store logger or console.error) and/or set an error state
or retry flag so the UI can show feedback instead of leaving memberContext null;
ensure you reference fetchMemberContext and the memberContext state in your fix
and avoid rethrowing unhandled exceptions unless upstream should handle them.
In `@packages/shared/src/services/GuildRoleAccessService.ts`:
- Around line 189-196: The loop in GuildRoleAccessService silently skips invalid
RoleGrantInput items (input loop using isRoleGrantInput and deduped), which
hides bad data; update the loop to log a warning (including guildId and the
offending item) whenever isRoleGrantInput(item) returns false so
callers/operators can see which grants were skipped, or alternatively make it
strict by throwing an error for invalid items; use the service's logger (or a
provided logger) to emit the message and keep the deduped.set behavior for valid
items.
In `@README.md`:
- Around line 236-247: The README references a non-descriptive asset filename
"assets/Gemini_Generated_Image_udmhrpudmhrpudmh.png"; rename that file to a
meaningful name (e.g., assets/criativaria-embed-image.png or
assets/server-setup-banner.png), update every reference in README.md and any
template or code that uses the exact filename (search for
"Gemini_Generated_Image_udmhrpudmhrpudmh.png"), and add a brief note in README
about the new filename; optionally update the provided verification script to
search for the new filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 608affa0-472a-430b-9eb4-1f9e2dd7c52c
📒 Files selected for processing (32)
CHANGELOG.mdREADME.mdpackages/backend/src/middleware/auth.tspackages/backend/src/middleware/guildAccess.tspackages/backend/src/routes/auth.tspackages/backend/src/routes/guilds.tspackages/backend/src/routes/index.tspackages/backend/src/routes/rbac.tspackages/backend/src/services/DiscordOAuthService.tspackages/backend/src/services/GuildAccessService.tspackages/backend/src/services/GuildService.tspackages/backend/tests/integration/api.test.tspackages/backend/tests/integration/routes/guilds.test.tspackages/backend/tests/unit/services/GuildService.test.tspackages/frontend/src/App.tsxpackages/frontend/src/components/Layout/Sidebar.test.tsxpackages/frontend/src/components/Layout/Sidebar.tsxpackages/frontend/src/components/ui/Button.tsxpackages/frontend/src/lib/rbac.tspackages/frontend/src/pages/DashboardOverview.tsxpackages/frontend/src/pages/ServerSettings.tsxpackages/frontend/src/services/api.tspackages/frontend/src/stores/guildStore.test.tspackages/frontend/src/stores/guildStore.tspackages/frontend/src/types/auth.tspackages/frontend/src/types/guild.tspackages/frontend/src/types/index.tspackages/frontend/src/types/rbac.tspackages/shared/src/services/GuildRoleAccessService.tspackages/shared/src/services/index.tsprisma/migrations/20260311110000_add_guild_role_grants/migration.sqlprisma/schema.prisma
Comment on lines
+42
to
+53
| if (!req.sessionId) { | ||
| throw AppError.unauthorized() | ||
| } | ||
|
|
||
| const sessionData = await sessionService.getSession(req.sessionId) | ||
| if (!sessionData) { | ||
| throw AppError.unauthorized('Session expired') | ||
| } | ||
|
|
||
| const guilds = | ||
| await guildAccessService.listAuthorizedGuilds(sessionData) | ||
| const guildDetails = guilds.find((guild) => guild.id === id) |
There was a problem hiding this comment.
Avoid a full authorized-guild scan in the single-guild route.
This handler already runs after requireGuildModuleAccess('overview'), but it re-fetches the session and calls guildAccessService.listAuthorizedGuilds(). listAuthorizedGuilds() builds context for every guild before returning (packages/backend/src/services/GuildAccessService.ts:78-113), so /api/guilds/:id now does O(all guilds) Discord/DB work just to find one record. A single-guild lookup/enrichment path would be much cheaper and avoids duplicating the access-resolution work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/routes/guilds.ts` around lines 42 - 53, The route is
doing an expensive full scan by calling
guildAccessService.listAuthorizedGuilds() to find one guild; instead, keep the
requireGuildModuleAccess('overview') pre-check and replace the list call with a
targeted lookup that builds/enriches only the requested guild (e.g., add and
call guildAccessService.getAuthorizedGuildById(sessionData, id) or similar).
Change the handler to use sessionService.getSession(req.sessionId) as before,
then invoke a new or existing single-guild method on GuildAccessService that
returns the enriched guild or null (avoid listAuthorizedGuilds), and throw
AppError.unauthorized if it’s not authorized or not found. Ensure the new method
performs the same access-resolution/enrichment logic that listAuthorizedGuilds
does but only for the given guild id.
Comment on lines
+164
to
+173
| const effectiveAccess = | ||
| memberContext?.effectiveAccess ?? selectedGuild?.effectiveAccess | ||
|
|
||
| const canViewModule = (module: ModuleKey) => { | ||
| if (!selectedGuild || !effectiveAccess) { | ||
| return true | ||
| } | ||
|
|
||
| return hasModuleAccess(effectiveAccess, module, 'view') | ||
| } |
There was a problem hiding this comment.
Don't default-allow nav items when access is unresolved.
If selectedGuild exists but memberContext/effectiveAccess hasn't loaded yet, this returns true and renders every module link. That breaks the PR's default-deny behavior and causes a flash of unauthorized navigation whenever access data is missing or stale.
Suggested fix
const canViewModule = (module: ModuleKey) => {
- if (!selectedGuild || !effectiveAccess) {
+ if (!selectedGuild) {
return true
}
+ if (!effectiveAccess) {
+ return false
+ }
return hasModuleAccess(effectiveAccess, module, 'view')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/components/Layout/Sidebar.tsx` around lines 164 - 173,
The canViewModule function currently returns true when access info is
unresolved, which allows all nav items to flash; update the guard so we only
allow by default when there is no selectedGuild at all. Specifically, change the
early-return logic in canViewModule (referencing effectiveAccess, memberContext,
selectedGuild) so that if selectedGuild is falsy return true, but if
selectedGuild exists and effectiveAccess is falsy return false; otherwise call
hasModuleAccess(effectiveAccess, module, 'view') as before.
Comment on lines
+330
to
+379
| {section.items | ||
| .filter((item) => | ||
| canViewModule(item.module), | ||
| ) | ||
| .map((item) => { | ||
| const active = isActive(item.path) | ||
| return ( | ||
| <Link | ||
| key={item.path} | ||
| to={item.path} | ||
| data-active={ | ||
| active ? 'true' : 'false' | ||
| } | ||
| className={cn( | ||
| 'h-[18px] w-[18px] shrink-0 transition-colors', | ||
| 'lucky-focus-visible group relative flex items-center gap-3 rounded-xl px-3 py-2.5 transition-all', | ||
| active | ||
| ? 'text-lucky-accent' | ||
| : 'text-lucky-text-tertiary group-hover:text-lucky-text-secondary', | ||
| ? 'bg-lucky-bg-active/80 text-lucky-text-primary ring-1 ring-lucky-border-strong' | ||
| : 'text-lucky-text-secondary hover:bg-lucky-bg-tertiary/70 hover:text-lucky-text-primary', | ||
| )} | ||
| /> | ||
| <span className='type-body-sm truncate'>{item.label}</span> | ||
| {item.badge !== undefined && item.badge > 0 && ( | ||
| <span className='ml-auto inline-flex min-h-[18px] min-w-[18px] items-center justify-center rounded-full bg-lucky-accent px-1 text-[10px] font-bold text-black'> | ||
| {item.badge > 99 ? '99+' : item.badge} | ||
| > | ||
| <span | ||
| className={cn( | ||
| 'absolute left-0 top-1/2 h-5 w-[3px] -translate-y-1/2 rounded-r', | ||
| active | ||
| ? 'bg-lucky-accent' | ||
| : 'bg-transparent', | ||
| )} | ||
| /> | ||
| <item.icon | ||
| className={cn( | ||
| 'h-[18px] w-[18px] shrink-0 transition-colors', | ||
| active | ||
| ? 'text-lucky-accent' | ||
| : 'text-lucky-text-tertiary group-hover:text-lucky-text-secondary', | ||
| )} | ||
| /> | ||
| <span className='type-body-sm truncate'> | ||
| {item.label} | ||
| </span> | ||
| )} | ||
| </Link> | ||
| ) | ||
| })} | ||
| {item.badge !== undefined && | ||
| item.badge > 0 && ( | ||
| <span className='ml-auto inline-flex min-h-[18px] min-w-[18px] items-center justify-center rounded-full bg-lucky-accent px-1 text-[10px] font-bold text-black'> | ||
| {item.badge > 99 | ||
| ? '99+' | ||
| : item.badge} | ||
| </span> | ||
| )} | ||
| </Link> | ||
| ) | ||
| })} |
There was a problem hiding this comment.
Skip empty nav sections after RBAC filtering.
Once access filtering hides every item in a section, the title still renders with an empty body. That leaves dead headings like Moderation or Extras for users who don't have those modules.
Suggested fix
- {navSections.map((section) => (
- <div key={section.title}>
+ {navSections.map((section) => {
+ const visibleItems = section.items.filter((item) =>
+ canViewModule(item.module),
+ )
+ if (visibleItems.length === 0) {
+ return null
+ }
+
+ return (
+ <div key={section.title}>
<p className='type-meta mb-2 px-2 text-lucky-text-tertiary'>
{section.title}
</p>
<div className='space-y-1'>
- {section.items
- .filter((item) =>
- canViewModule(item.module),
- )
- .map((item) => {
+ {visibleItems.map((item) => {
const active = isActive(item.path)
return (
<Link
@@
</Link>
)
})}
</div>
</div>
- ))}
+ )
+ })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/components/Layout/Sidebar.tsx` around lines 330 - 379,
The section title is rendered even when RBAC filtering hides all items; change
the rendering to first compute visibleItems = section.items.filter(item =>
canViewModule(item.module)) and only render/map the list when
visibleItems.length > 0, replacing uses of section.items.map with
visibleItems.map (keep existing uses of isActive, item.icon, item.badge, and
key={item.path}).
Comment on lines
+84
to
+102
| useEffect(() => { | ||
| if (!selectedGuild?.id || !canManageRbac) { | ||
| setRbacRoles([]) | ||
| setRbacGrants([]) | ||
| return | ||
| } | ||
|
|
||
| setRbacLoading(true) | ||
| api.guilds | ||
| .getRbac(selectedGuild.id) | ||
| .then((res) => { | ||
| setRbacRoles(res.data.roles) | ||
| setRbacGrants(res.data.grants) | ||
| }) | ||
| .catch(() => { | ||
| toast.error('Failed to load access control policy') | ||
| }) | ||
| .finally(() => setRbacLoading(false)) | ||
| }, [selectedGuild?.id, canManageRbac]) |
There was a problem hiding this comment.
Reset or ignore RBAC responses when the selected guild changes.
Line 91 starts a new load without clearing the previous guild's RBAC state, and Lines 165-169 always write the response back into shared local state. If the user switches servers mid-request—or the next load fails—the previous server's roles/grants can render under the new server and then be saved back incorrectly.
As per coding guidelines, "Set timeouts and handle aborts/cancellations; avoid dangling requests in API/network code".
Also applies to: 158-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/pages/ServerSettings.tsx` around lines 84 - 102, The
useEffect that fetches RBAC for selectedGuild (dependencies selectedGuild?.id
and canManageRbac) can leave stale roles/grants when the user switches guilds
mid-request; fix by cancelling or ignoring out-of-date responses: create an
AbortController (or a local "activeRequestId"/mounted flag) inside the effect,
call api.guilds.getRbac with the controller/signal (or check the id) and on
start clear existing state via
setRbacRoles([])/setRbacGrants([])/setRbacLoading(true); in the promise handlers
only call setRbacRoles/setRbacGrants/setRbacLoading if the controller is not
aborted (or the request id matches), and ensure you abort the controller (or
flip the flag) in the effect cleanup so in-flight responses are ignored and old
data is never written back.
Comment on lines
+124
to
+176
| const addRbacGrant = () => { | ||
| if (rbacRoles.length === 0) { | ||
| return | ||
| } | ||
|
|
||
| setRbacGrants((prev) => [ | ||
| ...prev, | ||
| { | ||
| roleId: rbacRoles[0].id, | ||
| module: 'overview', | ||
| mode: 'view', | ||
| }, | ||
| ]) | ||
| } | ||
|
|
||
| const updateRbacGrant = (index: number, updates: Partial<RoleGrant>) => { | ||
| setRbacGrants((prev) => | ||
| prev.map((grant, currentIndex) => | ||
| currentIndex === index | ||
| ? { | ||
| ...grant, | ||
| ...updates, | ||
| } | ||
| : grant, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| const removeRbacGrant = (index: number) => { | ||
| setRbacGrants((prev) => | ||
| prev.filter((_, currentIndex) => currentIndex !== index), | ||
| ) | ||
| } | ||
|
|
||
| const handleSaveRbac = async () => { | ||
| if (!selectedGuild?.id || !canManageRbac) { | ||
| return | ||
| } | ||
|
|
||
| setRbacSaving(true) | ||
| try { | ||
| const response = await api.guilds.updateRbac( | ||
| selectedGuild.id, | ||
| rbacGrants, | ||
| ) | ||
| setRbacGrants(response.data.grants) | ||
| toast.success('Access control policy saved') | ||
| } catch { | ||
| toast.error('Failed to save access control policy') | ||
| } finally { | ||
| setRbacSaving(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent duplicate (roleId, module) rules before submit.
The persisted model is unique on [guildId, roleId, module], but this editor allows the same pair to be added or selected multiple times. Because addRbacGrant() seeds the first role with overview, one extra click can put the form into a state the database cannot represent.
Also applies to: 449-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/pages/ServerSettings.tsx` around lines 124 - 176,
Prevent duplicate (roleId, module) pairs across rbacGrants: when adding in
addRbacGrant, choose a role/module combination that doesn't already exist (or
return/show error) instead of always seeding with the first role and 'overview';
in updateRbacGrant, check the proposed merged grant against other existing
grants and refuse or reject the update if it would create a duplicate; and in
handleSaveRbac validate rbacGrants for unique (roleId,module) before calling
api.guilds.updateRbac and abort with a toast error if duplicates are found.
Reference rbacGrants, rbacRoles, addRbacGrant, updateRbacGrant, and
handleSaveRbac when implementing these checks.
Comment on lines
+454
to
+530
| <Select | ||
| value={grant.roleId} | ||
| onValueChange={(value) => | ||
| updateRbacGrant(index, { | ||
| roleId: value, | ||
| }) | ||
| } | ||
| > | ||
| <SelectTrigger className='bg-lucky-bg-secondary border-lucky-border text-white'> | ||
| <SelectValue placeholder='Role' /> | ||
| </SelectTrigger> | ||
| <SelectContent className='bg-lucky-bg-secondary border-lucky-border'> | ||
| {rbacRoles.map((role) => ( | ||
| <SelectItem | ||
| key={role.id} | ||
| value={role.id} | ||
| > | ||
| {role.name} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
|
|
||
| <Select | ||
| value={grant.module} | ||
| onValueChange={(value) => | ||
| updateRbacGrant(index, { | ||
| module: value as RoleGrant['module'], | ||
| }) | ||
| } | ||
| > | ||
| <SelectTrigger className='bg-lucky-bg-secondary border-lucky-border text-white'> | ||
| <SelectValue placeholder='Module' /> | ||
| </SelectTrigger> | ||
| <SelectContent className='bg-lucky-bg-secondary border-lucky-border'> | ||
| {RBAC_MODULES.map((module) => ( | ||
| <SelectItem | ||
| key={module} | ||
| value={module} | ||
| > | ||
| {module} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
|
|
||
| <Select | ||
| value={grant.mode} | ||
| onValueChange={(value) => | ||
| updateRbacGrant(index, { | ||
| mode: value as RoleGrant['mode'], | ||
| }) | ||
| } | ||
| > | ||
| <SelectTrigger className='bg-lucky-bg-secondary border-lucky-border text-white'> | ||
| <SelectValue placeholder='Mode' /> | ||
| </SelectTrigger> | ||
| <SelectContent className='bg-lucky-bg-secondary border-lucky-border'> | ||
| <SelectItem value='view'> | ||
| view | ||
| </SelectItem> | ||
| <SelectItem value='manage'> | ||
| manage | ||
| </SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
|
|
||
| <Button | ||
| type='button' | ||
| variant='ghost' | ||
| className='text-lucky-text-tertiary hover:text-lucky-error' | ||
| onClick={() => | ||
| removeRbacGrant(index) | ||
| } | ||
| > | ||
| <Trash2 className='w-4 h-4' /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible names to each RBAC rule control.
Each row renders three unlabeled selects and an icon-only delete button. Once values are selected, assistive tech has no stable label for “role”, “module”, “mode”, or “remove rule”, so editing the policy becomes guesswork.
As per coding guidelines, "Provide accessible UI components using semantic HTML and ARIA attributes where necessary".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/frontend/src/pages/ServerSettings.tsx` around lines 454 - 530, The
RBAC row controls (the Select instances for grant.roleId, grant.module,
grant.mode and the delete Button invoking removeRbacGrant) lack accessible
names; add explicit accessible labels by supplying aria-label or aria-labelledby
to each Select (or its SelectTrigger) such as aria-label={`Role for rule ${index
+ 1}`}, aria-label={`Module for rule ${index + 1}`}, aria-label={`Mode for rule
${index + 1}`}, and set the delete Button to aria-label={`Remove rule ${index +
1}`} (or use a visually-hidden <span> with an id and reference it via
aria-labelledby) so assistive tech has stable, descriptive names while keeping
existing handlers like updateRbacGrant and removeRbacGrant unchanged.
Comment on lines
+114
to
+117
| } | ||
|
|
||
| const parsed = JSON.parse(value) as RoleGrant[] | ||
| return parsed |
There was a problem hiding this comment.
Unsafe JSON.parse without validation.
The cached value is parsed and directly cast to RoleGrant[] without validation. If the cache becomes corrupted or contains malformed data, this could cause runtime errors or unexpected behavior downstream.
🛡️ Proposed fix to validate cached data
const value = await redisClient.get(cacheKey(guildId))
if (!value) {
return null
}
- const parsed = JSON.parse(value) as RoleGrant[]
- return parsed
+ const parsed = JSON.parse(value) as unknown
+ if (!Array.isArray(parsed)) {
+ return null
+ }
+
+ return parsed.every(
+ (item) =>
+ typeof item === 'object' &&
+ item !== null &&
+ isModuleKey(item.module) &&
+ isAccessMode(item.mode),
+ )
+ ? (parsed as RoleGrant[])
+ : null📝 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.
Suggested change
| } | |
| const parsed = JSON.parse(value) as RoleGrant[] | |
| return parsed | |
| const value = await redisClient.get(cacheKey(guildId)) | |
| if (!value) { | |
| return null | |
| } | |
| const parsed = JSON.parse(value) as unknown | |
| if (!Array.isArray(parsed)) { | |
| return null | |
| } | |
| return parsed.every( | |
| (item) => | |
| typeof item === 'object' && | |
| item !== null && | |
| isModuleKey(item.module) && | |
| isAccessMode(item.mode), | |
| ) | |
| ? (parsed as RoleGrant[]) | |
| : null |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/services/GuildRoleAccessService.ts` around lines 114 -
117, The code directly JSON.parse(s) the cached value and casts to RoleGrant[]
without validation; wrap the parse in a try/catch to guard against malformed
JSON and implement a runtime type guard (e.g., isRoleGrantArray(obj)) that
verifies Array.isArray(obj) and that each entry has the expected RoleGrant
properties and types before returning it from the GuildRoleAccessService method
that parses `value`; if parsing or validation fails, log the error via the
service logger and return a safe fallback (empty array) instead of casting
invalid data.
Comment on lines
+5
to
+6
| "module" TEXT NOT NULL, | ||
| "mode" TEXT NOT NULL, |
There was a problem hiding this comment.
Constrain module and mode at the database layer.
Both columns are free-form TEXT. A typo or bad write can persist a grant the evaluator never matches, leaving authorization state silently wrong. For RBAC data, use enums or CHECK constraints for the supported module keys and access modes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prisma/migrations/20260311110000_add_guild_role_grants/migration.sql` around
lines 5 - 6, The "module" and "mode" columns are currently TEXT and need
DB-level constraints to prevent invalid values; update the migration.sql to
constrain them by either creating PostgreSQL ENUM types (e.g., grant_module,
grant_mode) and using those types for the "module" and "mode" columns, or add
explicit CHECK constraints listing the allowed module keys and access modes for
the "module" and "mode" columns respectively, and ensure any existing
INSERTs/UPDATES in this migration use valid enum/check values.
…c-access # Conflicts: # CHANGELOG.md
Contributor
|
Deployment failed with the following error: Learn More: https://vercel.com/luksantanas-projects?upgradeToPro=build-rate-limit |
|
Size Change: +1.42 kB (+0.49%) Total Size: 294 kB
ℹ️ View Unchanged
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
LucasSantana-Dev
force-pushed
the
feature/dashboard-rbac-access
branch
from
March 11, 2026 16:54
ac3b594 to
c4c6413
Compare
github-actions
Bot
removed
dependencies
|
|
5 tasks done
This was referenced Mar 12, 2026
This was referenced Mar 24, 2026
This was referenced Apr 12, 2026
6 tasks
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
* feat(webapp): add guild rbac with role-based dashboard access * refactor(tests): reduce duplicated setup blocks for sonar gate * refactor(backend): deduplicate guild guard route wiring * test(rbac): increase coverage for guild access and policy flows * test(shared): add coverage for guild role access service * ci(sonar): exclude shared sources from coverage gate * test(backend): expand guild service coverage * test(rbac): raise coverage for backend routes and frontend guards * test(frontend): harden rbac route coverage assertions * docs(changelog): scope rbac branch docs to access-control changes * test(rbac): tighten guard and auth route assertions * test(backend): remove any cast from route setup mock * test(backend): assert rbac guard registration order * feat(frontend): finalize discord portal url mapping * chore(discovery): add media pack v1 assets * chore(release): prepare v2.6.10 dashboard and security rollout * test(bot): cover web queue handlers for sonar gate * fix(pr165): remove out-of-scope files from rbac branch
This was referenced Jun 22, 2026
This branch was successfully deployed
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
1 participant
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.
Summary\n- add guild role-based module access (view/manage) with default deny and admin override\n- fix dashboard/server data and sidebar identity resolution (nick > globalName > username)\n- add access-control UI in server settings and route/nav gating by effective module access\n- add backend guild/member context + rbac endpoints and shared rbac evaluator service\n- persist role grants via prisma migration and update README/CHANGELOG\n\n## Verification\n- npm run lint\n- npm run type:check\n- npm run test\n- npm run test --workspace=packages/frontend\n
Summary by CodeRabbit
New Features
Improvements
Tests