Uh oh!
There was an error while loading. Please reload this page.
fix: auth refresh - #697
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements SSE heartbeat mechanisms across two API controllers to prevent connection timeouts, adds automatic QR code refresh every 60 seconds in two frontend login components, and updates the eID wallet identifier to reflect organizational branding. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant API as Auth API
participant SSE as SSE Connection
Client->>API: POST /login (fetch offer URI)
API-->>Client: offer_uri, session_id
Client->>SSE: new EventSource(session_id)
activate SSE
SSE-->>API: connection established
activate API
loop Every 30 seconds
API->>SSE: ": heartbeat\n\n"
SSE-->>Client: heartbeat received
end
Note over Client: 60-second auto-refresh timer started
par Waiting for auth
API->>SSE: authentication result
SSE-->>Client: onmessage event
Client->>Client: handle auth/error
and Auto-refresh timeout
Client->>Client: 60s elapsed
Client->>API: POST /login (re-fetch offer)
end
Client->>SSE: close()
deactivate SSE
deactivate API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platforms/blabsy/src/components/login/login-main.tsx (2)
28-34: Add error handling for JSON parsing.
JSON.parse(e.data)can throw if the server sends malformed data. Wrap in try/catch to prevent unhandled exceptions that could break the message handler.🛠️ Suggested fix
eventSource.onmessage = async (e): Promise<void> => { - const data = JSON.parse(e.data as string) as {- token?: string;- error?: boolean;- message?: string;- type?: string;- };+ let data: {+ token?: string;+ error?: boolean;+ message?: string;+ type?: string;+ };+ try {+ data = JSON.parse(e.data as string);+ } catch (parseError) {+ console.error('Failed to parse SSE message:', parseError);+ return;+ } if (data.error && data.type === 'version_mismatch') {
107-140: Verify useEffect doesn't re-run unnecessarily.The useEffect depends on
getOfferData(memoized), which depends onwatchEventStream(memoized), which depends onsignInWithCustomTokenfromuseAuth(). However,signInWithCustomTokenin the auth context is not memoized with useCallback and is created fresh on every AuthContextProvider render. This causeswatchEventStreamto be recreated, which in turn causesgetOfferDatato be recreated, triggering the useEffect to run on every AuthContextProvider render cycle.While the cleanup logic prevents resource leaks, this causes unnecessary network requests for the QR code and creates multiple SSE connections. Wrap
signInWithCustomTokenwithuseCallbackinauth-context.tsx(line 136).
🤖 Fix all issues with AI agents
In `@platforms/blabsy/src/components/login/login-main.tsx`:
- Around line 68-77: Wrap the axios.get call in a try/catch around the block
that calls setQr and assigns eventSourceRef.current so network errors don't
leave the old/closed EventSource in place; on error, log/report the error and
ensure eventSourceRef.current remains closed or is cleaned up. After receiving
data.uri, extract the session value from new
URL(data.uri).searchParams.get('session') and explicitly check for null/empty
before calling watchEventStream; only call watchEventStream(session) and assign
eventSourceRef.current when session is present, otherwise handle the missing
session (error log or user-visible fallback) instead of using the unsafe `as
string` assertion.
In `@platforms/pictique/src/routes/`(auth)/auth/+page.svelte:
- Around line 125-128: Wrap the apiClient.get('/api/auth/offer') call in a
try/catch and only assign qrData and call watchEventStream after a successful
response; on error set a local error state or fallback (so the component doesn't
remain in an inconsistent state). After setting qrData, parse the URL and
validate that new URL(qrData).searchParams.get('session') is non-null and
non-empty before passing it to watchEventStream (do not use an unconditional "as
string" cast); if the session param is missing, handle it by setting an error
state or aborting the eventSource creation instead of calling watchEventStream
with a null value. Ensure references to apiClient.get, qrData, and
watchEventStream/eventSource are updated accordingly.
- Around line 83-93: Wrap the JSON.parse call inside the
newEventSource.onmessage handler in a try/catch to guard against malformed
server data: inside the onmessage callback (newEventSource.onmessage) attempt to
parse e.data with JSON.parse in a try block, and in the catch set/append a
user-facing errorMessage (or log the error) and call newEventSource.close() to
stop the stream and prevent an unhandled exception; ensure subsequent usage of
the parsed variable only occurs if parsing succeeded so the existing
version_mismatch check (data.error && data.type === 'version_mismatch') remains
valid.
🧹 Nitpick comments (1)
platforms/pictique-api/src/controllers/AuthController.ts (1)
20-58: Consider extracting SSE handling to a shared utility.The
sseStreammethod is nearly identical to the one inblabsy-w3ds-auth-api/src/controllers/AuthController.ts. For maintainability, consider extracting the SSE setup, heartbeat, and cleanup logic into a shared utility that both controllers can use.
Comment on lines
68
to
77
| const { data } = await axios.get<{ uri: string }>( | ||
| new URL( | ||
| '/api/auth/offer', | ||
| process.env.NEXT_PUBLIC_BASE_URL | ||
| ).toString() | ||
| ); | ||
| setQr(data.uri); | ||
| watchEventStream( | ||
| eventSourceRef.current = watchEventStream( | ||
| new URL(data.uri).searchParams.get('session') as string | ||
| ); |
Contributor
There was a problem hiding this comment.
Add error handling and validate session parameter.
Two concerns consistent with the Svelte implementation:
- The
axios.getcall isn't wrapped in try/catch - if it fails,setQrwon't be called andeventSourceRef.currentremains with the old (closed) value. - The
as stringassertion on line 76 is unsafe -searchParams.get('session')can returnnull.
🛠️ Suggested fix
const getOfferData = useCallback(async (): Promise<void> => {
// Clean up existing SSE connection
if (eventSourceRef.current) {
eventSourceRef.current.close();
}
// Clean up existing refresh timer
if (refreshTimerRef.current) {
clearTimeout(refreshTimerRef.current);
}
- const { data } = await axios.get<{ uri: string }>(- new URL(- '/api/auth/offer',- process.env.NEXT_PUBLIC_BASE_URL- ).toString()- );- setQr(data.uri);- eventSourceRef.current = watchEventStream(- new URL(data.uri).searchParams.get('session') as string- );+ try {+ const { data } = await axios.get<{ uri: string }>(+ new URL(+ '/api/auth/offer',+ process.env.NEXT_PUBLIC_BASE_URL+ ).toString()+ );+ setQr(data.uri);++ const session = new URL(data.uri).searchParams.get('session');+ if (!session) {+ console.error('Session parameter missing from offer URI');+ return;+ }+ eventSourceRef.current = watchEventStream(session);+ } catch (error) {+ console.error('Failed to fetch offer:', error);+ return;+ }
// Set up auto-refresh after 60 seconds📝 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.
Suggested change
| const{ data }=awaitaxios.get<{uri: string}>( | |
| newURL( | |
| '/api/auth/offer', | |
| process.env.NEXT_PUBLIC_BASE_URL | |
| ).toString() | |
| ); | |
| setQr(data.uri); | |
| watchEventStream( | |
| eventSourceRef.current=watchEventStream( | |
| newURL(data.uri).searchParams.get('session')asstring | |
| ); | |
| constgetOfferData=useCallback(async(): Promise<void>=>{ | |
| // Clean up existing SSE connection | |
| if(eventSourceRef.current){ | |
| eventSourceRef.current.close(); | |
| } | |
| // Clean up existing refresh timer | |
| if(refreshTimerRef.current){ | |
| clearTimeout(refreshTimerRef.current); | |
| } | |
| try{ | |
| const{ data }=awaitaxios.get<{uri: string}>( | |
| newURL( | |
| '/api/auth/offer', | |
| process.env.NEXT_PUBLIC_BASE_URL | |
| ).toString() | |
| ); | |
| setQr(data.uri); | |
| constsession=newURL(data.uri).searchParams.get('session'); | |
| if(!session){ | |
| console.error('Session parameter missing from offer URI'); | |
| return; | |
| } | |
| eventSourceRef.current=watchEventStream(session); | |
| }catch(error){ | |
| console.error('Failed to fetch offer:',error); | |
| return; | |
| } | |
| // Set up auto-refresh after 60 seconds |
🤖 Prompt for AI Agents
In `@platforms/blabsy/src/components/login/login-main.tsx` around lines 68 - 77,
Wrap the axios.get call in a try/catch around the block that calls setQr and
assigns eventSourceRef.current so network errors don't leave the old/closed
EventSource in place; on error, log/report the error and ensure
eventSourceRef.current remains closed or is cleaned up. After receiving
data.uri, extract the session value from new
URL(data.uri).searchParams.get('session') and explicitly check for null/empty
before calling watchEventStream; only call watchEventStream(session) and assign
eventSourceRef.current when session is present, otherwise handle the missing
session (error log or user-visible fallback) instead of using the unsafe `as
string` assertion.
Comment on lines
+83
to
+93
| newEventSource.onmessage = (e) => { | ||
| const data = JSON.parse(e.data as string); | ||
| // Check for error messages (version mismatch) | ||
| if (data.error && data.type === 'version_mismatch') { | ||
| errorMessage = | ||
| data.message || | ||
| 'Your eID Wallet app version is outdated. Please update to continue.'; | ||
| newEventSource.close(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Add error handling for JSON parsing.
JSON.parse(e.data) can throw if the server sends malformed data. Wrap in try/catch to prevent unhandled exceptions.
🛠️ Suggested fix
newEventSource.onmessage = (e) => {
- const data = JSON.parse(e.data as string);+ let data;+ try {+ data = JSON.parse(e.data as string);+ } catch (parseError) {+ console.error('Failed to parse SSE message:', parseError);+ return;+ }
// Check for error messages (version mismatch)📝 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.
Suggested change
| newEventSource.onmessage = (e) => { | |
| const data = JSON.parse(e.data as string); | |
| // Check for error messages (version mismatch) | |
| if (data.error && data.type === 'version_mismatch') { | |
| errorMessage = | |
| data.message || | |
| 'Your eID Wallet app version is outdated. Please update to continue.'; | |
| newEventSource.close(); | |
| return; | |
| } | |
| newEventSource.onmessage = (e) => { | |
| let data; | |
| try { | |
| data = JSON.parse(e.data as string); | |
| } catch (parseError) { | |
| console.error('Failed to parse SSE message:', parseError); | |
| return; | |
| } | |
| // Check for error messages (version mismatch) | |
| if (data.error && data.type === 'version_mismatch') { | |
| errorMessage = | |
| data.message || | |
| 'Your eID Wallet app version is outdated. Please update to continue.'; | |
| newEventSource.close(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@platforms/pictique/src/routes/`(auth)/auth/+page.svelte around lines 83 - 93,
Wrap the JSON.parse call inside the newEventSource.onmessage handler in a
try/catch to guard against malformed server data: inside the onmessage callback
(newEventSource.onmessage) attempt to parse e.data with JSON.parse in a try
block, and in the catch set/append a user-facing errorMessage (or log the error)
and call newEventSource.close() to stop the stream and prevent an unhandled
exception; ensure subsequent usage of the parsed variable only occurs if parsing
succeeded so the existing version_mismatch check (data.error && data.type ===
'version_mismatch') remains valid.
Comment on lines
+125
to
+128
| const { data } = await apiClient.get('/api/auth/offer'); | ||
| qrData = data.uri; | ||
| eventSource = watchEventStream(new URL(qrData).searchParams.get('session') as string); |
Contributor
There was a problem hiding this comment.
Add error handling and validate session parameter.
Two concerns:
- The
apiClient.getcall isn't wrapped in try/catch - if it fails, the component will be left in an inconsistent state with no QR code. - The
as stringassertion on line 128 is unsafe -searchParams.get('session')can returnnullif the session parameter is missing from the URI.
🛠️ Suggested fix
async function fetchOfferAndSetupSSE() {
// Clean up existing SSE connection
if (eventSource) {
eventSource.close();
eventSource = null;
}
// Clean up existing refresh timer
if (refreshTimer) {
clearTimeout(refreshTimer);
refreshTimer = null;
}
- const { data } = await apiClient.get('/api/auth/offer');- qrData = data.uri;-- eventSource = watchEventStream(new URL(qrData).searchParams.get('session') as string);+ try {+ const { data } = await apiClient.get('/api/auth/offer');+ qrData = data.uri;++ const session = new URL(qrData).searchParams.get('session');+ if (!session) {+ console.error('Session parameter missing from offer URI');+ return;+ }+ eventSource = watchEventStream(session);+ } catch (error) {+ console.error('Failed to fetch offer:', error);+ return;+ }
// Set up auto-refresh after 60 seconds🤖 Prompt for AI Agents
In `@platforms/pictique/src/routes/`(auth)/auth/+page.svelte around lines 125 -
128, Wrap the apiClient.get('/api/auth/offer') call in a try/catch and only
assign qrData and call watchEventStream after a successful response; on error
set a local error state or fallback (so the component doesn't remain in an
inconsistent state). After setting qrData, parse the URL and validate that new
URL(qrData).searchParams.get('session') is non-null and non-empty before passing
it to watchEventStream (do not use an unconditional "as string" cast); if the
session param is missing, handle it by setting an error state or aborting the
eventSource creation instead of calling watchEventStream with a null value.
Ensure references to apiClient.get, qrData, and watchEventStream/eventSource are
updated accordingly.
coodos
approved these changes
Jan 21, 2026
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of change
adds heartbeat to keep auth sse connection alive and refreshes qr code every 60 seconds on pictique and blabsy
Issue Number
Closes#236
Type of change
How the change has been tested
Partial Manual
Change checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.