Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThe changes update the logic in the RolePage function by querying a role directly using the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RolePage
participant Database
participant RoleClient
User->>RolePage: Request role page (roleId)
RolePage->>Database: Query role & permissions for roleId
Database-->>RolePage: Return role data or null
alt Role found
RolePage->>RolePage: Filter permissions & remove soft-deleted keys
RolePage->>RoleClient: Render RoleClient with active permissions
RoleClient-->>User: Display role details
else Role not found
RolePage-->>User: Return notFound response
end
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
Can we remove everything but the fix from this PR. |
|
ah fuck, I pulled main earlier |
|
If you can reproduce the same issue for permissions can you also check that one too? |
|
they had a similar structure |
|
that one worked 🤷 |
|
dafuq and role details works? |
|
no, that’s why this fix exists |
|
ahh shit I thought /roles |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/internal/services/keys/service.go (1)
27-39: Implemented cache initialization with appropriate configuration.The cache is properly initialized with reasonable freshness (10s) and staleness (60s) durations, along with other required parameters. There's one minor issue though:
The Resource name is set to "permissions" but should probably be "keys" since this is the keys service:
- Resource: "permissions", + Resource: "keys",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/agent_build_publish.yaml(1 hunks)apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx(3 hunks)go/apps/api/run.go(2 hunks)go/internal/services/keys/service.go(1 hunks)go/internal/services/keys/verify.go(2 hunks)go/internal/services/permissions/check.go(1 hunks)go/internal/services/permissions/service.go(2 hunks)go/pkg/testutil/http.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
go/internal/services/permissions/service.go (3)
go/pkg/cache/cache.go (15) (15)
cache(20:34)New(58:96)Config(36:53)c(98:144)c(146:168)c(170:172)c(174:176)c(178:188)c(190:212)c(214:218)c(220:235)c(237:254)c(256:258)c(260:291)c(293:345)go/internal/services/keys/service.go (3) (3)
service(18:23)New(25:45)Config(12:16)go/pkg/otel/logging/interface.go (1) (1)
Logger(11:116)
go/pkg/testutil/http.go (4)
go/pkg/rbac/permissions.go (1) (1)
t(184:186)go/internal/services/keys/service.go (2) (2)
New(25:45)Config(12:16)go/internal/services/permissions/service.go (2) (2)
New(30:53)Config(24:28)go/internal/services/ratelimit/sliding_window.go (2) (2)
New(56:83)Config(50:54)
go/apps/api/run.go (3)
go/internal/services/keys/service.go (2) (2)
New(25:45)Config(12:16)go/internal/services/permissions/service.go (2) (2)
New(30:53)Config(24:28)go/apps/api/routes/services.go (2) (2)
Services(17:25)EventBuffer(13:15)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (26)
.github/workflows/agent_build_publish.yaml (1)
21-23: Reassure consistent tag formatting.
This approach correctly extracts the version fromGITHUB_REF. However, ensure all tags consistently follow therefs/tags/agent/format, or the expansion may lead to unexpected results.go/internal/services/permissions/check.go (2)
6-6: Appropriate addition of cache import.
The new import is consistent with the caching logic that follows. No issues here.
14-23: Sound caching logic integration.
Usings.cache.SWRto reduce repetitive DB queries is a good performance optimization. The error handling to distinguish cache hits and misses appears logical.go/internal/services/keys/verify.go (2)
9-9: Cache import aligns with new caching mechanism.
This import is necessary for the key-caching feature. Implementation looks fine.
23-34: Robust caching fallback logic.
TheSWRusage properly distinguishes between found rows, missing rows, and errors, providing a reliable cache solution. This should significantly reduce load on the database.go/apps/api/run.go (2)
123-123: Added Clock parameter to keys service initialization.The Clock parameter has been added to the keys service configuration, which will allow the service to properly initialize its new caching functionality. This is a good addition for consistent time handling across services.
138-145: Improved permissions service initialization with explicit error handling.This change refactors the permissions service initialization to use a named variable with proper error handling, which is a good practice. The Clock parameter is also added to support the new caching functionality.
go/pkg/testutil/http.go (2)
78-78: Added Clock parameter to keys service in test harness.The addition of the Clock parameter to the test harness keys service initialization matches the production code, ensuring consistency between production and test environments. Good synchronization of changes.
85-90: Updated permissions service initialization in test harness with Clock parameter and error handling.This change properly adds the Clock parameter to the permissions service initialization in the test harness and includes appropriate error handling. This ensures the test harness matches the updated production code.
go/internal/services/keys/service.go (4)
4-8: Added necessary imports for caching functionality.The added imports for time, cache, and clock packages are required for the new caching implementation. This is a good preparation for the following functionality changes.
15-15: Added Clock field to Config struct.The Clock parameter is essential for proper time-based cache operations. This is a good addition that will help with consistent time handling across services.
21-23: Added keyCache field with descriptive comment.The service struct now includes a keyCache field of type cache.Cache with appropriate type parameters for caching keys by their hash. The comment clarifies the mapping relationship, which is a good practice.
40-44: Updated service initialization to include the keyCache.The service struct initialization now includes the keyCache field, completing the implementation of the caching functionality.
go/internal/services/permissions/service.go (5)
4-8: Added necessary imports for caching functionality.The added imports for time, cache, and clock packages are required for the caching implementation. This is consistent with similar changes in the keys service.
17-20: Added cache field with descriptive comment.The service struct now includes a cache field for storing permissions by key ID, with a helpful comment explaining the mapping relationship.
27-27: Added Clock field to Config struct.The Clock parameter is necessary for proper time-based cache operations, ensuring consistent time handling across the application.
30-45: Modified New function to initialize cache with error handling.The New function now initializes the cache with appropriate configuration parameters and handles potential errors. The freshness and staleness durations (10s and 60s) are reasonable for this use case.
47-53: Updated service initialization to include the cache.The service struct initialization now includes the cache field, properly integrating the caching functionality into the permissions service.
apps/dashboard/app/(app)/authorization/roles/[roleId]/page.tsx (8)
3-3: Added appropriate 404 handling.The import of
notFoundfrom Next.js navigation is a good addition since it properly handles cases where resources aren't found, aligning with the PR's goal of fixing the roles 404 error.
47-62: Improved role retrieval approach.The database query is now properly structured to load a role directly by ID first, as opposed to fetching through the workspace. This change directly addresses the PR objective "fix: load role first" and should prevent 404 errors when accessing roles directly.
The relations structure (permissions, workspace, keys) is well-organized and comprehensive.
63-65: Enhanced error handling for missing resources.This check properly handles cases where a role or its associated workspace doesn't exist, returning a 404 response instead of attempting to continue processing with invalid data.
66-67: Added security check for tenant isolation.Good addition of a tenant validation check to ensure the user can only access roles within their assigned workspace, maintaining proper multi-tenancy security boundaries.
70-70: Simplified permission extraction logic.The new approach directly maps permissions from role.permissions, making the code more straightforward and easier to understand than the previous implementation.
73-77: Updated filtering logic for active permissions.The filter logic is now properly aligned with the new data structure, ensuring that only permissions that exist in the permissions array are included in the active role permissions set.
79-79: Updated sorting to use new permissions array.The sorting operation now correctly references the new permissions array, ensuring consistency with the updated data retrieval approach.
123-123: Improved key filtering for non-deleted keys.The filter now properly excludes soft-deleted keys from being passed to the client component, ensuring the UI only displays active keys.
|
I can't see available permissions right now? |
|
can we double check that |
3 participants
Summary by CodeRabbit