Error handling of UrlResolver and fix VC profile inaccessible #7704
Conversation
…re rerouting to restricted on VC profile and settings
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Component as VCProfilePage/VCSettingsPageLayout
participant Hook as useRestrictedRedirect
participant Priv as Privilege Checker
Component->>Hook: Call useRestrictedRedirect(data, error, skip)
alt skip is true
Hook->>Hook: Exit early (bypass redirect)
else skip is false
Hook->>Priv: Validate user privileges
Priv-->>Hook: Return validation result
Hook->>Component: Trigger redirect if necessary
end
sequenceDiagram
participant Provider as UrlResolverProvider
participant GraphQL as UrlResolver Query
Provider->>GraphQL: Execute urlResolver query
GraphQL-->>Provider: Return data (including isError, errorMessage)
alt Not loading and isError is true
Provider->>Provider: Throw NotFoundError(errorMessage)
else
Provider->>Provider: Proceed normally
end
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/routing/urlResolver/UrlResolverProvider.tsx (1)
137-139: Consider adding type narrowing for better type safety.While the error handling logic is correct, we can improve type safety by adding a type guard.
- if (!urlResolverLoading && urlResolverData?.urlResolver.isError) { + if (!urlResolverLoading && urlResolverData && urlResolverData.urlResolver.isError) { - throw new NotFoundError(urlResolverData?.urlResolver.errorMessage); + throw new NotFoundError(urlResolverData.urlResolver.errorMessage); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/core/apollo/generated/apollo-helpers.tsis excluded by!**/generated/**src/core/apollo/generated/apollo-hooks.tsis excluded by!**/generated/**src/core/apollo/generated/graphql-schema.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
src/core/routing/useRestrictedRedirect.tsx(3 hunks)src/domain/community/virtualContributor/vcProfilePage/VCProfilePage.tsx(1 hunks)src/domain/community/virtualContributorAdmin/layout/VCSettingsPageLayout.tsx(1 hunks)src/main/routing/urlResolver/UrlResolverProvider.tsx(2 hunks)src/main/routing/urlResolver/UrlResolvers.graphql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/**/*.{ts,tsx,js}`: Review the React.js/TypeScript/JavaS...
src/**/*.{ts,tsx,js}: Review the React.js/TypeScript/JavaScript code for best practices and potential bugs.
Ensure that the code adheres to TypeScript's typing system and modern standards.
Use the following code guide documentation in the codebase: 'docs/code-guidelines.md'.
Ensure sufficient error handling and logging is present, but note:
- We have a global error handler in place. So local
try/catchblocks are not required unless they provide value beyond what the global error handler already covers.- Avoid flagging the absence of local error handling in cases where the global error handler suffices.
- Avoid flagging the absence of logging in cases where the global logger is already in place (graphql usage).
Inform about the recommendation of not using non-null assertion,
except on GraphQL queries where the variable is asserted non-null and followed by a
skip:with the same variable.
Examples:
- Not Required: there's a skip
{
variables: { templateId: templateId! },
skip: !templateId,
}Check for common security vulnerabilities such as:
- SQL Injection
- XSS (Cross-Site Scripting)
- CSRF (Cross-Site Request Forgery)
- Insecure dependencies
- Sensitive data exposure
src/domain/community/virtualContributor/vcProfilePage/VCProfilePage.tsxsrc/domain/community/virtualContributorAdmin/layout/VCSettingsPageLayout.tsxsrc/core/routing/useRestrictedRedirect.tsxsrc/main/routing/urlResolver/UrlResolverProvider.tsx
`src/**/*.{graphql,gql}`: Review the GraphQL schema and quer...
src/**/*.{graphql,gql}: Review the GraphQL schema and queries for best practices, potential bugs, and adherence to the project's GraphQL standards.
Ensure that the schema is well-defined and queries are optimized.
Check for security vulnerabilities.
src/main/routing/urlResolver/UrlResolvers.graphql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (9)
src/main/routing/urlResolver/UrlResolverProvider.tsx (2)
3-3: LGTM!The import statement is well-organized and follows the project's path aliasing convention.
130-135: LGTM!The GraphQL query implementation follows the project guidelines correctly:
- Non-null assertion is properly used with a corresponding skip condition
- Skip condition prevents unnecessary query execution
src/main/routing/urlResolver/UrlResolvers.graphql (1)
49-50: LGTM!The new GraphQL fields are well-named and properly integrated:
isErrorprovides clear error state indicationerrorMessageprovides detailed error information
These additions support the improved error handling for VC profile accessibility.src/core/routing/useRestrictedRedirect.tsx (4)
11-11: LGTM! Interface update is well-defined.The optional
skipparameter is correctly typed as a boolean in theRestrictedRedirectQueryResponseinterface.
27-27: LGTM! Hook signature update is correct.The
skipparameter is properly destructured with a default value offalse, maintaining backward compatibility.
40-42: LGTM! Early return optimization is well-implemented.The early return when
skipis true prevents unnecessary redirect logic execution.
51-51:⚠️ Potential issueAdd skip to useEffect dependencies.
The
skipparameter should be included in the dependencies array since it's used within the effect.Apply this diff to fix the dependencies:
- }, [data, error, skip]); + }, [data, error, skip, navigate, redirectUrl, navigateOptions, readPrivileges, requiredPrivilege]);Likely an incorrect or invalid review comment.
src/domain/community/virtualContributorAdmin/layout/VCSettingsPageLayout.tsx (1)
35-41: LGTM! Hook usage is correctly updated.The implementation:
- Properly combines loading states to prevent premature redirects
- Correctly reads privileges from the virtual contributor data
- Appropriately requires Update privilege for settings page
src/domain/community/virtualContributor/vcProfilePage/VCProfilePage.tsx (1)
50-53: LGTM! Hook usage is correctly updated.The implementation:
- Properly combines loading states to prevent premature redirects
- Correctly reads privileges from the virtual contributor data
- Appropriately uses default Read privilege for profile page
…kem-io/client-web into client-7701-vc-profile-access
The second one leads to access to all VC profiles from all members. The logic should be improved on the server in the future. The fix was suggested by Neil.
Summary by CodeRabbit
New Features
Bug Fixes