feat(frontend): add reusable useUser hook and header auth UX#943
feat(frontend): add reusable useUser hook and header auth UX#943skypank-coder wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughA new ChangesUser Auth State and Header Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. 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: 1
🧹 Nitpick comments (1)
application/frontend/src/hooks/useUser.ts (1)
16-39: Useasync/awaitinstead of Promise chaining in this new TS hook.This new frontend TypeScript code uses raw
.then/.catch/.finally; please rewrite toasync/awaitfor consistency with project standards.
As per coding guidelines, "Prefer async/await over raw Promise chains or callbacks in new TypeScript frontend code."Suggested refactor
- useEffect(() => { - let active = true; - fetch(`${apiUrl}/user`, { method: 'GET' }) - .then((res) => { - if (res.status === 200) { - return res.text(); - } - return null; // 401 or anything else => treated as not logged in - }) - .then((value) => { - if (active) { - setUser(value && value.trim() !== '' ? value : null); - } - }) - .catch(() => { - if (active) { - setUser(null); // network error => treat as anonymous, do NOT redirect - } - }) - .finally(() => { - if (active) { - setLoading(false); - } - }); - return () => { - active = false; - }; - }, [apiUrl]); + useEffect(() => { + let active = true; + const loadUser = async () => { + try { + const res = await fetch(`${apiUrl}/user`, { method: 'GET' }); + if (res.status === 200) { + const value = await res.text(); + if (active) setUser(value.trim() !== '' ? value : null); + return; + } + if (active) setUser(null); + } catch { + if (active) setUser(null); + } finally { + if (active) setLoading(false); + } + }; + void loadUser(); + return () => { + active = false; + }; + }, [apiUrl]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/frontend/src/hooks/useUser.ts` around lines 16 - 39, The useEffect hook in the useUser function uses Promise chaining with .then/.catch/.finally methods, which should be converted to async/await syntax for consistency with project standards. Create an async function inside the useEffect that performs the fetch call to the user endpoint, use try/catch blocks instead of .catch(), and replace the .then() chaining with await statements. Ensure the active flag check is maintained at each operation (after fetch, after setting user state, and in the cleanup) to prevent state updates on unmounted components, and keep the setLoading(false) call in a finally block or after the try/catch completes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/frontend/src/hooks/useUser.ts`:
- Around line 20-24: In the useUser hook, the current logic treats all non-200
HTTP responses as anonymous by returning null. Instead of collapsing all non-200
responses together, you should differentiate between them: return res.text() for
status 200, return null specifically for status 401 (unauthorized/anonymous),
and for all other status codes (like 5xx errors), throw an error or handle them
as actual failures rather than masking them as normal logged-out state. Apply
this same correction to both occurrences mentioned in the diff (lines 20-24 and
lines 30-33).
---
Nitpick comments:
In `@application/frontend/src/hooks/useUser.ts`:
- Around line 16-39: The useEffect hook in the useUser function uses Promise
chaining with .then/.catch/.finally methods, which should be converted to
async/await syntax for consistency with project standards. Create an async
function inside the useEffect that performs the fetch call to the user endpoint,
use try/catch blocks instead of .catch(), and replace the .then() chaining with
await statements. Ensure the active flag check is maintained at each operation
(after fetch, after setting user state, and in the cleanup) to prevent state
updates on unmounted components, and keep the setLoading(false) call in a
finally block or after the try/catch completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8e57915e-65e6-459b-95f0-ef4cd0130eb7
📒 Files selected for processing (3)
application/frontend/src/hooks/index.tsapplication/frontend/src/hooks/useUser.tsapplication/frontend/src/scaffolding/Header/Header.tsx
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Direction looks good — useUser treating 401 as anonymous (no redirect) is what we want.
One ask before merge: gate the header Login/Logout behind capabilities.login, same pattern as capabilities.myopencre. We want to roll out auth UI per-environment; prod should be able to hide it with login: false until we're ready.
useUser can stay as-is — just wrap the header auth blocks in capabilities.login && …. Default login: false when /api/capabilities is missing/fails is fine for now.
Capabilities endpoint wiring can be a small follow-up if you prefer, but please don't merge always-on header auth without the gate.
|
@northdpole Pushed updates addressing both review comments . |
2 participants
Scoped to non-MyOpenCRE auth UX per @Pa04rth 's steer (b): a reusable useUser hook + header login/user/logout state, following the existing chatbot auth pattern. Part of the Login MVP (@northdpole 's priority #1).
useUser probes /rest/v1/user and treats 401 as anonymous — no forced redirect, so public pages stay open to anonymous users (consistent with the public-reads-stay-open / v2 direction @ outlined).
Header shows the logged-in user + Logout, or a Login button when anonymous.
Frontend-only. No backend changes. No myopencre/capabilities coupling (left to Prateek).
Verified manually in dev: NO_LOGIN=1 shows logged-in state; unauthenticated shows Login with no redirect on load; login/logout flows work.
Closes #942 .