fix: resolve permission errors on first time admin registration #584
Conversation
WalkthroughAdded a skeleton-based loading fallback to the dashboard ResourceGuard and updated the registration hook to dispatch Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RegisterHook as Register Hook
participant AuthAPI as Auth Service
participant Redux
participant Router as Navigation
User->>RegisterHook: Submit signup
RegisterHook->>AuthAPI: signup(...)
AuthAPI-->>RegisterHook: success
rect rgb(220,240,220)
Note over RegisterHook,Redux: New post-signup auth init
RegisterHook->>Redux: dispatch(initializeAuth())
Redux->>AuthAPI: initializeAuth flow
AuthAPI-->>Redux: initialized / error
Redux-->>RegisterHook: unwrap result
end
alt initializeAuth succeeds
RegisterHook->>RegisterHook: wait 200ms
RegisterHook->>Router: navigate('/dashboard')
else initializeAuth fails
RegisterHook->>RegisterHook: console.warn(...)
RegisterHook->>Router: navigate('/dashboard')
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: Review and find the bugs or see if existing bugs are resolved. Also please suggest a PR title and update description |
🧩 Analysis chain🏁 Script executed: Length of output: 4750 @zhravan: I've reviewed the PR and found several bugs and issues that need attention: 🐛 Bugs Identified1. Type Safety Issue - CriticalIn await (dispatch(initializeAuth() as any) as any).unwrap();Double Fix: await dispatch(initializeAuth()).unwrap();If this doesn't work, check the return type of 2. Incomplete Error Handling - CriticalLines 77-79: If Fix: try {
await dispatch(initializeAuth()).unwrap();
await new Promise((resolve) => setTimeout(resolve, 200));
router.push('/dashboard');
} catch (authError) {
console.error('Auth initialization after registration failed:', authError);
toast.error('Registration successful. Please log in.');
router.push('/auth');
}3. Race Condition Band-Aid - High PriorityLine 80: The arbitrary 200ms delay suggests a race condition that's being masked rather than properly fixed. What if 200ms isn't enough? What happens on slower connections? Better approach: Wait for the auth state to actually be ready using proper state subscription or a loading flag. 4. Console Statement in Production CodeLine 78: 5. Temporary Code Still PresentThe TODO comment in 🎯 Suggested PR Title📝 Suggested PR Description#### Issue
First-time admin registration was causing permission errors when accessing the dashboard due to uninitialized auth state.
---
#### Description
This PR fixes permission errors that occur during first-time admin registration by:
1. Initializing the auth state immediately after successful registration
2. Adding a loading fallback UI to the dashboard's ResourceGuard to handle the loading state gracefully
3. Redirecting newly registered admins directly to the dashboard instead of the login page
---
#### Scope of Change
- [x] View (UI/UX) - Added loading skeleton to dashboard
- [x] API - Changed post-registration flow
- [ ] CLI
- [ ] Infra / Deployment
- [ ] Docs
- [ ] Other (specify): ________
---
#### Root Cause
After admin registration, the auth state wasn't properly initialized before redirecting, causing the ResourceGuard to deny access to the dashboard.
---
#### Additional Notes for Reviewers
⚠️ This PR has several issues that need to be addressed:
1. Type casting issues need to be fixed
2. Error handling for auth initialization failures is incomplete
3. The 200ms delay suggests a race condition that should be properly resolved
4. Console.warn statement should be removed
5. TODO comment needs a tracking issue
---
#### Developer Checklist
- [ ] Add valid/relevant title for the PR
- [ ] Self-review done
- [ ] Manual dev testing done
- [ ] No secrets exposed
- [ ] No merge conflicts
- [ ] Docs added/updated (if applicable)
- [x] Removed debug prints / secrets / sensitive data (console.warn still present)
- [ ] Unit / Integration tests passing
- [ ] Follows all standards defined in **Nixopus Docs**🔍 RecommendationThis PR needs additional work before merging. The current implementation might mask the permission errors with the loading fallback but doesn't properly handle authentication initialization failures. Consider refactoring to properly await auth state readiness rather than using arbitrary delays. Would you like me to help create a more robust implementation that properly handles these edge cases? ✅ Actions performedFull review triggered. |
![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 (3)
view/app/dashboard/page.tsx (1)
100-118: Loading fallback wiring forResourceGuardlooks good; consider deduplicating skeleton layoutThe new
loadingFallbackgives a clear, branded loading state while permissions/auth are resolving and is consistent with the main layout. Since a very similar skeleton grid exists inMonitoringSectionfor!systemStats, you might later consider extracting a shared “dashboard skeleton” component so both places stay visually in sync and easier to tweak in one spot. The TODO to remove this can then become a TODO to consolidate or gate it instead of dropping it entirely.view/app/register/hooks/use-register.ts (2)
12-14: Removeanycasts aroundinitializeAuthdispatch and rely on typed thunk dispatchYou’re already using
useAppDispatchwith a typedAppDispatch, so the explicitas anycasts aroundinitializeAuthdefeat the type-safety and aren’t needed. Redux Toolkit’s async thunks are designed to be dispatched directly and then.unwrap()ped.You can simplify this to the idiomatic pattern:
- const dispatch = useAppDispatch(); + const dispatch = useAppDispatch(); … - try { - await (dispatch(initializeAuth() as any) as any).unwrap(); - } catch (authError) { + try { + await dispatch(initializeAuth()).unwrap(); + } catch (authError) { console.warn('Auth initialization after registration failed:', authError); }This keeps the code cleaner and preserves compile-time checks on the thunk’s payload and reject types.
Please confirm this matches your configured
AppDispatch/ Redux Toolkit types forinitializeAuthin your project setup.Also applies to: 41-41, 76-80
76-85: Clarify behavior when auth initialization fails and reconsider the fixed 200 ms delayRight now, if
initializeAuththrows or rejects, you log a warning but still wait 200 ms and push to/dashboard. That can drop users onto a dashboard whereResourceGuardmay still treat them as unauthenticated/unauthorized, which is close to the original “permission error after registration” symptom—just with a console warning.Two follow‑ups to consider:
Failure path UX:
OninitializeAuthfailure, consider redirecting to the auth flow (e.g./auth) or showing a toast and staying on the register/auth page instead of forcing/dashboard. That gives the user a clearer recovery path and avoids routing them into a guarded screen that is likely to fail again.Arbitrary 200 ms timeout:
If the goal is to avoid a brief flicker between registration completion and dashboard load, it may be better to rely solely on the thunk resolution (which you already await) or tie any delay to an actual loading state, rather than a fixed timeout. If you keep the delay for UX reasons, a short inline comment explaining why it’s needed will help future maintainers avoid “cleaning it up” and reintroducing the bug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
view/app/dashboard/page.tsx(1 hunks)view/app/register/hooks/use-register.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
view/app/dashboard/page.tsx (1)
view/components/rbac/PermissionGuard.tsx (1)
ResourceGuard(78-95)
view/app/register/hooks/use-register.ts (2)
view/redux/hooks.ts (1)
useAppDispatch(4-4)view/redux/features/users/authSlice.ts (1)
initializeAuth(29-69)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
view/app/dashboard/page.tsx (1)
104-104: Track or remove the TODO comment.The TODO comment suggests this is temporary code for post-production testing. Ensure this is tracked in an issue or removed if the fallback is intended to be permanent.
Do you want me to open a tracking issue for this TODO, or should it be removed if the loading fallback is meant to be permanent?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
view/app/dashboard/page.tsx(1 hunks)view/app/register/hooks/use-register.ts(3 hunks)
🔇 Additional comments (1)
view/app/register/hooks/use-register.ts (1)
82-82: Verify implementation patterns and test the proposed alternatives.After examining the codebase, the review comment identifies a valid concern. The 200ms delay occurs after
await dispatch(initializeAuth()).unwrap()completes but beforerouter.push('/dashboard').Key findings:
initializeAuth is properly awaited (line 77): The async thunk completes and Redux state updates (token, user, isAuthenticated, isInitialized) are applied.
ResourceGuard has async operations: The dashboard page wraps content in
<ResourceGuard resource="dashboard" action="read">, which uses auseRBAC()hook that returns anisLoadingflag. This indicates the guard performs async permission checking.Race condition context: The 200ms delay sits between Redux state update completion and navigation. However, it executes before the dashboard component renders, so it may not effectively wait for ResourceGuard's async permission checks, which happen after mount.
Inconsistent pattern: In
view/app/auth/hooks/use-auth.ts(line 61),initializeAuth()is dispatched WITHOUT await or unwrap, then immediately navigates. This suggests the 200ms delay in use-register.ts may be overly defensive.The review comment's suggestions are sound. Instead of an arbitrary timeout, synchronization should rely on:
- Redux
isInitializedflag becoming true- Async RBAC permission checks completing before or during navigation
- Explicit state readiness checks rather than timing assumptions
However, without reproducing the original race condition, the actual impact of the 200ms delay cannot be definitively measured.
| try { | ||
| await (dispatch(initializeAuth() as any) as any).unwrap(); | ||
| } catch (authError) { | ||
| console.warn('Auth initialization after registration failed:', authError); | ||
| } | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| router.push('/dashboard'); |
There was a problem hiding this comment.
Fix error handling to prevent permission errors on auth failure.
If initializeAuth() fails, the user is still redirected to /dashboard, which can cause the permission errors this PR aims to fix. The error is logged but not handled.
Apply this diff to properly handle auth initialization failure:
toast.success(t('auth.register.success'));
try {
- await (dispatch(initializeAuth() as any) as any).unwrap();
+ await dispatch(initializeAuth()).unwrap();
+ await new Promise((resolve) => setTimeout(resolve, 200));
+ router.push('/dashboard');
} catch (authError) {
- console.warn('Auth initialization after registration failed:', authError);
+ toast.error('Failed to initialize authentication. Please try logging in.');
+ router.push('/auth');
+ return;
}
-
- await new Promise((resolve) => setTimeout(resolve, 200));
-
- router.push('/dashboard');
}This ensures that only successful auth initialization leads to the dashboard, while failures redirect to the auth page.
📝 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.
| try { | |
| await (dispatch(initializeAuth() as any) as any).unwrap(); | |
| } catch (authError) { | |
| console.warn('Auth initialization after registration failed:', authError); | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, 200)); | |
| router.push('/dashboard'); | |
| try { | |
| await dispatch(initializeAuth()).unwrap(); | |
| await new Promise((resolve) => setTimeout(resolve, 200)); | |
| router.push('/dashboard'); | |
| } catch (authError) { | |
| toast.error('Failed to initialize authentication. Please try logging in.'); | |
| router.push('/auth'); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In view/app/register/hooks/use-register.ts around lines 76 to 84, the code logs
but ignores initializeAuth() failures and unconditionally redirects to
'/dashboard'; change the flow so only a successful initializeAuth leads to the
dashboard: keep the await unwrap inside try, on success await the 200ms delay
and router.push('/dashboard'), and inside the catch stop further processing and
redirect to the auth page (e.g., router.push('/auth' or '/login')) so failed
auth initialization does not send users to the dashboard.
| router.push('/auth'); | ||
|
|
||
| try { | ||
| await (dispatch(initializeAuth() as any) as any).unwrap(); |
There was a problem hiding this comment.
Remove type casts and fix initializeAuth typing.
The double as any cast bypasses TypeScript's type safety and prevents catching type errors at compile time. initializeAuth() should return a properly typed async thunk.
Apply this diff to remove the casts:
- await (dispatch(initializeAuth() as any) as any).unwrap();
+ await dispatch(initializeAuth()).unwrap();If TypeScript errors occur after removing the casts, ensure initializeAuth is properly defined as an async thunk using createAsyncThunk from @reduxjs/toolkit.
📝 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.
| await (dispatch(initializeAuth() as any) as any).unwrap(); | |
| await dispatch(initializeAuth()).unwrap(); |
🤖 Prompt for AI Agents
In view/app/register/hooks/use-register.ts around line 77, remove the double "as
any" casts on the dispatched initializeAuth call and make the types flow
correctly: change the call to await dispatch(initializeAuth()).unwrap() (no
casts) and then fix typing errors by ensuring initializeAuth is created with
createAsyncThunk from @reduxjs/toolkit and its payload/return types are
declared, and that your store's dispatch type (e.g., AppDispatch) is the
correctly-typed dispatch used in this hook so unwrap() and the await operate
with proper types.
| try { | ||
| await (dispatch(initializeAuth() as any) as any).unwrap(); | ||
| } catch (authError) { | ||
| console.warn('Auth initialization after registration failed:', authError); |
There was a problem hiding this comment.
Remove console.warn from production code.
Console statements should be removed from production code, as noted in the developer checklist.
This will be addressed by the error handling fix in the previous comment, which replaces the console.warn with a toast notification and proper navigation.
🤖 Prompt for AI Agents
In view/app/register/hooks/use-register.ts around line 79, remove the
console.warn call and replace it with production-safe error handling: show the
user-facing toast notification (e.g., toast.error with a clear message), perform
the proper navigation flow as described in the previous comment, and, if
necessary, record the error via the app's logger (not console) for diagnostics;
ensure no console.* calls remain.
Issue
Link to related issue(s):
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.