feat: show guided tour on first connection #1404
Conversation
WalkthroughThe pull request introduces enhancements to user management and access control across multiple frontend and backend files. The changes focus on expanding user metadata, implementing accessible domain retrieval, and refining the first-time user connection experience. The modifications include updating the user interface type definitions, adding new store management for first-time connections, and modifying the sidebar component to handle guided tour logic based on accessible domains. Changes
Sequence DiagramsequenceDiagram
participant User
participant Backend
participant Frontend
participant Store
User->>Backend: Login Request
Backend->>Backend: Retrieve User Details
Backend->>Frontend: Return User Data with Accessible Domains
Frontend->>Store: Update firstTimeConnection
Frontend->>Frontend: Determine Guided Tour Visibility
Poem
🪧 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 (
|
nas-tabchiche
changed the title
nas-tabchiche
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/src/lib/utils/stores.ts (1)
5-5: LGTM! Consider documenting the store's purpose.The implementation of the persisted store for tracking first-time connection is correct. Consider adding a brief comment explaining its purpose and persistence behavior.
import type { Driver } from 'driver.js'; +// Persisted store to track and remember if this is user's first time connecting export const firstTimeConnection = persisted('firstTimeConnection', true);Also applies to: 19-19
frontend/src/lib/utils/types.ts (1)
12-17: Consider using more specific types instead ofRecord<string, any>.While the additions to the User interface are good, using
Record<string, any>for user_groups, roles, and permissions is too permissive and could lead to type-safety issues.Consider defining specific interfaces for these properties:
interface UserGroup { id: string; name: string; // add other relevant fields } interface Role { id: string; name: string; // add other relevant fields } interface Permission { id: string; name: string; // add other relevant fields } export interface User { // ...existing fields user_groups: UserGroup[]; roles: Role[]; permissions: Permission[]; // ...other fields }backend/iam/views.py (1)
73-75: Consider caching the accessible domains query.The
get_accessible_foldersquery might be expensive. Consider caching the result, especially if it's accessed frequently.from django.core.cache import cache # In get method cache_key = f'user_{request.user.id}_accessible_domains' accessible_domains = cache.get(cache_key) if accessible_domains is None: accessible_domains = RoleAssignment.get_accessible_folders( Folder.get_root_folder(), request.user, Folder.ContentType.DOMAIN ) cache.set(cache_key, accessible_domains, timeout=3600) # Cache for 1 hour
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/iam/views.py(3 hunks)frontend/src/lib/components/SideBar/SideBar.svelte(2 hunks)frontend/src/lib/utils/stores.ts(2 hunks)frontend/src/lib/utils/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.12)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/src/lib/components/SideBar/SideBar.svelte (1)
192-195: Consider edge cases in tour trigger logic.Setting
$firstTimeConnectionto false immediately after triggering the tour might cause issues if the user closes or interrupts the tour before completion.Consider tracking tour completion instead:
if ($firstTimeConnection) { triggerVisit(); - $firstTimeConnection = false; // This will prevent the tour from showing up again on page reload + // Listen for tour completion before setting firstTimeConnection to false + $driverInstance?.onDestroyStarted(() => { + if ($driverInstance?.isLastStep()) { + $firstTimeConnection = false; + } + }); }
| const user = $page.data?.user; | ||
| $firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use reactive statement for store value updates.
Direct mutation of store values outside of reactive statements can lead to reactivity issues.
-$firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0;
+$: $firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0;📝 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.
| const user = $page.data?.user; | |
| $firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0; | |
| const user = $page.data?.user; | |
| $: $firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0; |
| @@ -82,6 +85,7 @@ def get(self, request) -> Response: | |||
| "permissions": request.user.permissions, | |||
| "is_third_party": request.user.is_third_party, | |||
| "is_admin": request.user.is_admin(), | |||
| "accessible_domains": [str(f) for f in accessible_domains], | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for string conversion.
The list comprehension for converting folders to strings might fail if any folder object is invalid.
-"accessible_domains": [str(f) for f in accessible_domains],
+"accessible_domains": [str(f) for f in accessible_domains if f is not None],📝 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.
| "accessible_domains": [str(f) for f in accessible_domains], | |
| "accessible_domains": [str(f) for f in accessible_domains if f is not None], |
nas-tabchiche
requested review from
ab-smith,
eric-intuitem and
Mohamed-Hacene
…ded-tour-if-there-are-no-accessible-domains
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/lib/components/SideBar/SideBar.svelte (2)
181-188: Add error handling for driver initialization.The driver initialization should be wrapped in a try-catch block to handle potential runtime errors gracefully.
function triggerVisit() { - const translatedSteps = steps; - const driverObj = driver({ - showProgress: true, - steps: translatedSteps - }); - $driverInstance = driverObj; - driverObj.drive(); + try { + const translatedSteps = steps; + const driverObj = driver({ + showProgress: true, + steps: translatedSteps + }); + $driverInstance = driverObj; + driverObj.drive(); + } catch (error) { + console.error('Failed to initialize guided tour:', error); + // Consider showing a user-friendly error message + } }
201-203: Add null check for user properties and optimize reactive dependencies.The reactive statement assumes
useranduser.is_adminare always defined. Also, consider moving this statement before thefirstTimeConnectionupdate to optimize reactivity chain.-$: displayGuidedTour = $firstTimeConnection && user.is_admin; +$: displayGuidedTour = $firstTimeConnection && user?.is_admin === true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/SideBar/SideBar.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
frontend/src/lib/components/SideBar/SideBar.svelte (4)
8-15: LGTM! Clean import organization and prop typing.The imports are well-organized and the
openprop is properly typed as a boolean.
190-195: LGTM! Clean mount logic implementation.The mount logic correctly checks the display condition before triggering the tour and properly updates the store afterward.
199-199: Use reactive statement for store value updates.Direct mutation of store values outside of reactive statements can lead to reactivity issues.
-$: $firstTimeConnection = $firstTimeConnection && user.accessible_domains.length === 0; +$: $firstTimeConnection = $firstTimeConnection && user?.accessible_domains?.length === 0;
17-17: Add null check for user data access.The optional chaining on
datais good, but accessing properties on potentially undefineduserlater in the code could cause runtime errors.
Mohamed-Hacene
deleted the
CA-818-suggest-loading-dummy-data-and-or-guided-tour-if-there-are-no-accessible-domains
branch
Summary by CodeRabbit
Release Notes
New Features
Backend Updates
Frontend Updates