add sso#17
Conversation
📝 WalkthroughWalkthroughAdds dual SSO (public GitHub and internal Microsoft Entra ID) end-to-end: auth modules, HTTP clients, pages/routes, server APIs, an API Gateway Lambda authorizer, CDK authorizer construct and wiring, in-memory SSO value store, SSR bundle reorder script, dependency upgrades, and docs for auth/authorizer behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Header as Header.tsx
participant Webapp as Webapp (Route)
participant AuthAPI as /api/auth/*
participant AuthServer as BetterAuth Handler
participant OAuth as OAuth Provider
participant Authorizer as API Gateway Authorizer
User->>Browser: Click SSO link
Browser->>Header: navigate to /sso/public or /sso/internal
Header->>Webapp: load SSO page component
Webapp->>AuthAPI: sign-in request (/api/auth/public or /api/auth/internal)
AuthAPI->>AuthServer: delegate to authPublic/authInternal.handler
AuthServer->>OAuth: redirect / exchange code
OAuth->>AuthServer: callback with code / profile
AuthServer->>Browser: set JWT cookie and/or session
Browser->>Webapp: authenticated request to /api/sso/* (includes cookie)
Webapp->>Authorizer: API request evaluated (Authorizer reads cookie/header, JWKS)
Authorizer-->>Webapp: allow/deny (policy)
Webapp->>Browser: return protected data or 401
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 9-10: The dev script uses POSIX env assignment so it fails on
Windows; update the "webapp:dev" npm script to use cross-env (e.g. run via
cross-env NODE_OPTIONS=... vite dev --port 3000) and add cross-env to
devDependencies so the script is cross-platform; locate the "webapp:dev" entry
in package.json and replace the POSIX assignment with the cross-env form and
add/install the cross-env package in package.json.
In `@src/webapp/lib/auth.ts`:
- Line 5: The code currently falls back to a hardcoded secret via the constant
`secret` (from `BETTER_AUTH_SECRET ??
'dev-secret-min-32-chars-long-for-local'`), which is unsafe; change this so that
if `process.env.BETTER_AUTH_SECRET` is missing and the environment is not
development (e.g. `process.env.NODE_ENV !== 'development'`), the module throws
an explicit error and prevents startup; otherwise allow the dev fallback only in
development. Ensure references to `secret` and `BETTER_AUTH_SECRET` remain
consistent across functions that use them.
In `@src/webapp/routes/api.auth`.$.tsx:
- Around line 8-21: The current code logs OAuth callback and full get-session
response bodies (inside the block using pathname, authInternal.handler,
response.clone(), and JSON.stringify) which can leak sensitive data; modify it
to remove or gate these logs behind an explicit debug flag (e.g., use a
process.env.DEBUG_AUTH or a centralized config flag) so the console.log that
prints '[auth/internal] OAuth callback...' and the JSON.stringify(body) call
only run when the debug flag is enabled; keep the try/catch but ensure no
sensitive payload is logged in production.
🧹 Nitpick comments (2)
src/webapp/lib/auth.ts (2)
32-36: Empty string fallbacks for OAuth credentials will cause opaque failures.When
GITHUB_CLIENT_IDorGITHUB_CLIENT_SECRETare not set, defaulting to''will result in authentication failures without clear indication of the root cause. Consider validating these values on startup.🔧 Proposed fix to validate credentials
Add validation at the top of the file:
function requireEnv(name: string): string { const value = process.env[name]; if (!value && process.env.NODE_ENV === 'production') { throw new Error(`Missing required environment variable: ${name}`); } return value ?? ''; }Then use it for credentials:
socialProviders: { github: { - clientId: process.env.GITHUB_CLIENT_ID ?? '', - clientSecret: process.env.GITHUB_CLIENT_SECRET ?? '', + clientId: requireEnv('GITHUB_CLIENT_ID'), + clientSecret: requireEnv('GITHUB_CLIENT_SECRET'), }, },
65-69: Handle undefinedprofile.rolesexplicitly.If the Microsoft Entra ID profile doesn't include
roles,JSON.stringify(undefined)returns the string"undefined", which will fail JSON parsing and silently result in an empty array. This is technically handled by thetry/catchincustomSession, but an explicit check would be clearer.✨ Proposed improvement
mapProfileToUser: (profile) => { return { - roles: JSON.stringify(profile.roles), + roles: JSON.stringify(profile.roles ?? []), }; },
| "webapp:dev": "NODE_OPTIONS='--max-http-header-size=32768' vite dev --port 3000", | ||
| "webapp:build": "node scripts/generate-robots-txt.ts && vite build && node scripts/fix-ssr-bundle-order.mjs", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n package.json | head -100Repository: JohannesKonings/tanstack-aws
Length of output: 4033
🌐 Web query:
NODE_OPTIONS environment variable Windows command prompt PowerShell POSIX syntax compatibility
💡 Result:
NODE_OPTIONS itself is cross-platform, but the shell syntax to set env vars is not (POSIX vs Windows shells). Node reads NODE_OPTIONS as a space-separated list of CLI flags; if a flag value contains spaces, quote/escape it. [1]
POSIX shells (bash/zsh/sh)
Set for a single command:
NODE_OPTIONS="--max-old-space-size=4096" node app.jsWindows Command Prompt (cmd.exe)
Set for the current session:
set "NODE_OPTIONS=--max-old-space-size=4096"
node app.jsSingle line:
set "NODE_OPTIONS=--max-old-space-size=4096" && node app.js[2]
Windows PowerShell
Set for the current session:
$env:NODE_OPTIONS="--max-old-space-size=4096"
node app.js[2]
Making POSIX-style VAR=... command work on Windows (npm scripts)
Use cross-env so you can write POSIX-style env assignments in scripts and have them work on Windows too:
"scripts": {
"start": "cross-env NODE_OPTIONS=--max-old-space-size=4096 node app.js"
}[3]
Key point: NODE_OPTIONS=... node ... is POSIX syntax and won’t run in cmd.exe/PowerShell unless you use a helper like cross-env (or run in WSL/Git Bash). [3]
Sources: [1] Node.js CLI docs (NODE_OPTIONS) (nodejs.org), [2] Windows cmd/PowerShell examples (stackoverflow.com), [3] cross-env README (npmjs.com)
Citations:
- 1: https://nodejs.org/api/cli.html?utm_source=openai
- 2: https://stackoverflow.com/questions/56742334/how-to-use-the-node-options-environment-variable-to-set-the-max-old-space-size-g?utm_source=openai
- 3: https://www.npmjs.com/package/cross-env/v/7.0.3?utm_source=openai
Dev script is not cross-platform.
NODE_OPTIONS='--max-http-header-size=32768' uses POSIX syntax and fails on Windows (cmd.exe/PowerShell). Use cross-env so the script works across all platforms.
♻️ Proposed fix
- "webapp:dev": "NODE_OPTIONS='--max-http-header-size=32768' vite dev --port 3000",
+ "webapp:dev": "cross-env NODE_OPTIONS=--max-http-header-size=32768 vite dev --port 3000", "devDependencies": {
+ "cross-env": "^7.0.3",
"@aws-cdk/mixins-preview": "2.236.0-alpha.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.
| "webapp:dev": "NODE_OPTIONS='--max-http-header-size=32768' vite dev --port 3000", | |
| "webapp:build": "node scripts/generate-robots-txt.ts && vite build && node scripts/fix-ssr-bundle-order.mjs", | |
| "webapp:dev": "cross-env NODE_OPTIONS=--max-http-header-size=32768 vite dev --port 3000", | |
| "webapp:build": "node scripts/generate-robots-txt.ts && vite build && node scripts/fix-ssr-bundle-order.mjs", |
🤖 Prompt for AI Agents
In `@package.json` around lines 9 - 10, The dev script uses POSIX env assignment
so it fails on Windows; update the "webapp:dev" npm script to use cross-env
(e.g. run via cross-env NODE_OPTIONS=... vite dev --port 3000) and add cross-env
to devDependencies so the script is cross-platform; locate the "webapp:dev"
entry in package.json and replace the POSIX assignment with the cross-env form
and add/install the cross-env package in package.json.
| import { customSession } from 'better-auth/plugins'; | ||
| import { tanstackStartCookies } from 'better-auth/tanstack-start'; | ||
|
|
||
| const secret = process.env.BETTER_AUTH_SECRET ?? 'dev-secret-min-32-chars-long-for-local'; |
There was a problem hiding this comment.
Hardcoded fallback secret poses a security risk in production.
If BETTER_AUTH_SECRET is not set, the fallback 'dev-secret-min-32-chars-long-for-local' will be used. This is predictable and could be exploited if accidentally deployed without proper configuration. Consider failing explicitly when the secret is missing in non-development environments.
🛡️ Proposed fix to fail fast in production
-const secret = process.env.BETTER_AUTH_SECRET ?? 'dev-secret-min-32-chars-long-for-local';
+const secret = process.env.BETTER_AUTH_SECRET ?? (
+ process.env.NODE_ENV === 'production'
+ ? (() => { throw new Error('BETTER_AUTH_SECRET must be set in production'); })()
+ : 'dev-secret-min-32-chars-long-for-local'
+);📝 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 secret = process.env.BETTER_AUTH_SECRET ?? 'dev-secret-min-32-chars-long-for-local'; | |
| const secret = process.env.BETTER_AUTH_SECRET ?? ( | |
| process.env.NODE_ENV === 'production' | |
| ? (() => { throw new Error('BETTER_AUTH_SECRET must be set in production'); })() | |
| : 'dev-secret-min-32-chars-long-for-local' | |
| ); |
🤖 Prompt for AI Agents
In `@src/webapp/lib/auth.ts` at line 5, The code currently falls back to a
hardcoded secret via the constant `secret` (from `BETTER_AUTH_SECRET ??
'dev-secret-min-32-chars-long-for-local'`), which is unsafe; change this so that
if `process.env.BETTER_AUTH_SECRET` is missing and the environment is not
development (e.g. `process.env.NODE_ENV !== 'development'`), the module throws
an explicit error and prevents startup; otherwise allow the dev fallback only in
development. Ensure references to `secret` and `BETTER_AUTH_SECRET` remain
consistent across functions that use them.
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 (1)
lib/constructs/WebappDistribution.ts (1)
59-66:⚠️ Potential issue | 🟠 Major
DnsValidatedCertificateis deprecated—migrate toCertificatewith proper region handling for CloudFront.
DnsValidatedCertificateis deprecated in AWS CDK v2. The replacement isCertificatewithCertificateValidation.fromDns(). However, this requires careful handling of the CloudFront requirement: ACM certificates must be inus-east-1, and unlikeDnsValidatedCertificate, the standardCertificateconstruct doesn't support aregionparameter.Choose one approach:
- Deploy the entire stack to
us-east-1(simplest for CloudFront), or- Use cross-region references: Keep the certificate in the current region and enable
crossRegionReferencesat the stack level for cross-region reference support.Also note:
DnsValidatedCertificate.cleanupRoute53Recordshas no equivalent inCertificate—DNS validation records may persist after stack deletion.♻️ Example replacement (if stack is in us-east-1 or cross-region refs enabled)
+import { CertificateValidation } from 'aws-cdk-lib/aws-certificatemanager'; - const certificate = new DnsValidatedCertificate(this, 'Cert', { - domainName: domainName, - hostedZone, - transparencyLoggingEnabled: true, - cleanupRoute53Records: true, - region: 'us-east-1', - }); + const certificate = new Certificate(this, 'Cert', { + domainName: domainName, + validation: CertificateValidation.fromDns(hostedZone), + transparencyLoggingEnabled: true, + });
🤖 Fix all issues with AI agents
In `@src/lambda/api-gateway-authorizer.ts`:
- Around line 148-150: The code inconsistently strips trailing slashes only when
calling getJwks but passes the raw baseUrl to jwtVerify, causing issuer/audience
mismatches; fix by normalizing baseUrl once (e.g., const normalizedBaseUrl =
baseUrl.replace(/\/$/, '')) and use normalizedBaseUrl for both
getJwks('internal', normalizedBaseUrl) and jwtVerify(token, jwks, { issuer:
normalizedBaseUrl, audience: normalizedBaseUrl }) so getJwks, jwtVerify, token
and issuer/audience use the same canonical URL.
In `@src/webapp/routes/sso/public.tsx`:
- Around line 17-24: The useEffect block that calls authClientPublic.token()
(inside the component relying on session) lacks error handling; update the token
retrieval in that effect to catch and handle failures from
authClientPublic.token() (e.g., with .catch or try/catch if you convert to
async) and surface the problem by logging the error (console.error or existing
logger) and/or triggering user feedback (toast or UI state) instead of silently
ignoring errors; ensure you still only set document.cookie using
SSO_PUBLIC_JWT_COOKIE when res.data?.token exists.
- Around line 134-151: handleApiSubmit currently parses the response without
checking HTTP status, so non-2xx responses (401/403/500) can produce confusing
results; update handleApiSubmit to check res.ok after fetch and handle errors:
if !res.ok, read error text/JSON if available and surface it (throw or set an
error state) instead of proceeding to parse into {value}, ensure setLoading is
cleared in the finally block as already done, and keep the successful path to
setApiValue, setApiInput, and call loadApiValue; reference the handleApiSubmit
function and related setters (setApiValue, setApiInput, setLoading,
loadApiValue) when making the change.
🧹 Nitpick comments (11)
src/webapp/lib/sso-value.ts (1)
25-30: Input validators pass through data without sanitization.The
.inputValidator((data: string) => data)simply returns the input without any validation. Consider adding length limits or basic sanitization if this data is displayed elsewhere.♻️ Proposed validation enhancement
export const setSsoPublicValue = createServerFn({ method: 'POST' }) - .inputValidator((data: string) => data) + .inputValidator((data: string) => { + if (typeof data !== 'string') throw new Error('Value must be a string'); + return data.slice(0, 1000); // Limit length + }) .handler(async ({ data }): Promise<string> => { setStoredValue('public', data); return data; });Also applies to: 36-41
lib/constructs/WebappApi.ts (1)
43-56: Inconsistent indentation in deployOptions.The
accessLogDestinationandaccessLogFormatproperties have extra leading spaces, and there's inconsistent indentation throughout this block.🧹 Proposed formatting fix
deployOptions: { dataTraceEnabled: true, loggingLevel: MethodLoggingLevel.INFO, metricsEnabled: true, throttlingBurstLimit: 500, throttlingRateLimit: 1000, tracingEnabled: true, - accessLogDestination: new LogGroupLogDestination(logGroupAccessLogs), - accessLogFormat: AccessLogFormat.jsonWithStandardFields({ - caller: false, - httpMethod: true, - ip: true, - protocol: true, - requestTime: true, - resourcePath: true, - responseLength: true, - status: true, - user: true, - }), - - }, + accessLogDestination: new LogGroupLogDestination(logGroupAccessLogs), + accessLogFormat: AccessLogFormat.jsonWithStandardFields({ + caller: false, + httpMethod: true, + ip: true, + protocol: true, + requestTime: true, + resourcePath: true, + responseLength: true, + status: true, + user: true, + }), + },src/webapp/routes/api.sso.public.value.tsx (1)
8-13: Missing error handling for JSON parsing.
request.json()throws if the body is not valid JSON. Consider wrapping in try-catch to return a proper error response.🛡️ Proposed error handling
POST: async ({ request }) => { - const body = (await request.json()) as { value?: string }; - const value = typeof body?.value === 'string' ? body.value : ''; - setStoredValue('public', value); - return Response.json({ value }); + try { + const body = (await request.json()) as { value?: string }; + const value = typeof body?.value === 'string' ? body.value : ''; + setStoredValue('public', value); + return Response.json({ value }); + } catch { + return new Response(JSON.stringify({ error: 'Invalid JSON body' }), { + status: 400, + headers: { 'Content-Type': 'application/json' }, + }); + } },src/webapp/routes/api.sso.internal.value.tsx (1)
8-13: Missing error handling for JSON parsing.Same issue as the public value route -
request.json()can throw on invalid JSON. Apply consistent error handling across both routes.lib/constructs/Webapp.ts (1)
85-85: Remove unnecessary non-null assertion.
distributionApiGwis always defined (assigned on Line 65), so the!is unnecessary.🧹 Proposed fix
new WebappAssetsDeployment(this, 'WebappAssetsDeploymentApiGw', { assetsBucket: assetsBucket.assetsBucket, - distribution: distributionApiGw!.distribution, + distribution: distributionApiGw.distribution, });lib/constructs/WebappApiAuthorizer.ts (1)
46-57: Consider usingStack.of(this)for consistency.While
Stack.of(scope)works, usingStack.of(this)is more idiomatic in CDK constructs sincethisis already the construct that owns the function.♻️ Suggested change
if (ssmParameterName) { - const stack = Stack.of(scope); + const stack = Stack.of(this); const parameterName = ssmParameterName.startsWith('/') ? ssmParameterName : `/${ssmParameterName}`;src/webapp/routes/sso/public.tsx (1)
121-132: Redundant state update before refetch.
setServerValue(serverInput)on line 126 is immediately overwritten byloadServerValue()on line 128. Either remove the optimistic update or remove the refetch - doing both is redundant and can cause visual flicker.♻️ Option A: Keep only optimistic update (faster UX)
const handleServerSubmit = async (e: React.FormEvent) => { e.preventDefault(); setLoading('server'); try { await setSsoPublicValue({ data: serverInput }); setServerValue(serverInput); setServerInput(''); - await loadServerValue(); } finally { setLoading(null); } };♻️ Option B: Keep only refetch (authoritative server state)
const handleServerSubmit = async (e: React.FormEvent) => { e.preventDefault(); setLoading('server'); try { await setSsoPublicValue({ data: serverInput }); - setServerValue(serverInput); setServerInput(''); await loadServerValue(); } finally { setLoading(null); } };src/lambda/api-gateway-authorizer.ts (2)
3-5: Consider importing cookie constants from the route files.The cookie names are duplicated here and in
src/webapp/routes/sso/public.tsx(line 7) andsrc/webapp/routes/sso/internal.tsx(line 7). If they drift, authorization will silently fail.You could create a shared constants file to ensure consistency:
// src/shared/auth-constants.ts export const SSO_PUBLIC_JWT_COOKIE = '__auth_jwt_public'; export const SSO_INTERNAL_JWT_COOKIE = '__auth_jwt_internal';Then import in both the Lambda and the route files.
191-199: Unuseddeny()function.The
deny()function is defined but never called - the handler throws errors instead of returning deny policies. Consider removing it to reduce dead code, or using it for explicit denials if that's the intended design.src/webapp/routes/sso/internal.tsx (2)
100-208: Significant code duplication withSsoPublicValueSection.
SsoInternalValueSectionis nearly identical toSsoPublicValueSectioninpublic.tsx(~100 lines). Consider extracting a sharedSsoValueSectioncomponent that accepts the server functions and API path as props.♻️ Sketch of shared component
// src/webapp/components/SsoValueSection.tsx type SsoValueSectionProps = { getValueFn: () => Promise<string>; setValueFn: (params: { data: string }) => Promise<string>; apiPath: string; }; export function SsoValueSection({ getValueFn, setValueFn, apiPath }: SsoValueSectionProps) { // ... shared implementation } // Usage in public.tsx: <SsoValueSection getValueFn={getSsoPublicValue} setValueFn={setSsoPublicValue} apiPath="/api/sso/public/value" />
73-80: Type casting for roles could be cleaner.The repeated
(session as { roles: string[] }).rolesis verbose. Consider extracting to a variable:♻️ Cleaner roles access
const roles = 'roles' in session && Array.isArray((session as { roles?: string[] }).roles) ? (session as { roles: string[] }).roles : []; // Then in JSX: {roles.length > 0 && ( <p className="text-sm text-gray-300"> <span className="font-medium text-white">Entra ID roles:</span>{' '} {roles.join(', ')} </p> )}
| try { | ||
| const jwks = getJwks('internal', baseUrl.replace(/\/$/, '')); | ||
| const { payload } = await jwtVerify(token, jwks, { issuer: baseUrl, audience: baseUrl }); |
There was a problem hiding this comment.
Inconsistent issuer/audience handling may cause JWT verification failures.
For the internal path, the JWKS URL strips trailing slashes (line 149), but the issuer/audience check uses baseUrl directly (line 150). If baseUrl has a trailing slash, the JWT (signed with no trailing slash) will fail verification due to issuer mismatch.
🐛 Proposed fix
try {
- const jwks = getJwks('internal', baseUrl.replace(/\/$/, ''));
- const { payload } = await jwtVerify(token, jwks, { issuer: baseUrl, audience: baseUrl });
+ const base = baseUrl.replace(/\/$/, '');
+ const jwks = getJwks('internal', base);
+ const { payload } = await jwtVerify(token, jwks, { issuer: base, audience: base });📝 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 { | |
| const jwks = getJwks('internal', baseUrl.replace(/\/$/, '')); | |
| const { payload } = await jwtVerify(token, jwks, { issuer: baseUrl, audience: baseUrl }); | |
| try { | |
| const base = baseUrl.replace(/\/$/, ''); | |
| const jwks = getJwks('internal', base); | |
| const { payload } = await jwtVerify(token, jwks, { issuer: base, audience: base }); |
🤖 Prompt for AI Agents
In `@src/lambda/api-gateway-authorizer.ts` around lines 148 - 150, The code
inconsistently strips trailing slashes only when calling getJwks but passes the
raw baseUrl to jwtVerify, causing issuer/audience mismatches; fix by normalizing
baseUrl once (e.g., const normalizedBaseUrl = baseUrl.replace(/\/$/, '')) and
use normalizedBaseUrl for both getJwks('internal', normalizedBaseUrl) and
jwtVerify(token, jwks, { issuer: normalizedBaseUrl, audience: normalizedBaseUrl
}) so getJwks, jwtVerify, token and issuer/audience use the same canonical URL.
| useEffect(() => { | ||
| if (!session) return; | ||
| authClientPublic.token().then((res) => { | ||
| if (res.data?.token) { | ||
| document.cookie = `${SSO_PUBLIC_JWT_COOKIE}=${res.data.token}; path=/; max-age=900; samesite=lax`; | ||
| } | ||
| }); | ||
| }, [session]); |
There was a problem hiding this comment.
Add error handling for token retrieval.
The token() call may fail (network error, expired session, etc.), but errors are silently ignored. Consider adding error handling to provide user feedback or at least log the issue.
🛡️ Proposed fix
useEffect(() => {
if (!session) return;
- authClientPublic.token().then((res) => {
- if (res.data?.token) {
- document.cookie = `${SSO_PUBLIC_JWT_COOKIE}=${res.data.token}; path=/; max-age=900; samesite=lax`;
- }
- });
+ authClientPublic.token()
+ .then((res) => {
+ if (res.data?.token) {
+ document.cookie = `${SSO_PUBLIC_JWT_COOKIE}=${res.data.token}; path=/; max-age=900; samesite=lax`;
+ }
+ })
+ .catch((err) => {
+ console.error('Failed to retrieve JWT token:', err);
+ });
}, [session]);📝 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.
| useEffect(() => { | |
| if (!session) return; | |
| authClientPublic.token().then((res) => { | |
| if (res.data?.token) { | |
| document.cookie = `${SSO_PUBLIC_JWT_COOKIE}=${res.data.token}; path=/; max-age=900; samesite=lax`; | |
| } | |
| }); | |
| }, [session]); | |
| useEffect(() => { | |
| if (!session) return; | |
| authClientPublic.token() | |
| .then((res) => { | |
| if (res.data?.token) { | |
| document.cookie = `${SSO_PUBLIC_JWT_COOKIE}=${res.data.token}; path=/; max-age=900; samesite=lax`; | |
| } | |
| }) | |
| .catch((err) => { | |
| console.error('Failed to retrieve JWT token:', err); | |
| }); | |
| }, [session]); |
🤖 Prompt for AI Agents
In `@src/webapp/routes/sso/public.tsx` around lines 17 - 24, The useEffect block
that calls authClientPublic.token() (inside the component relying on session)
lacks error handling; update the token retrieval in that effect to catch and
handle failures from authClientPublic.token() (e.g., with .catch or try/catch if
you convert to async) and surface the problem by logging the error
(console.error or existing logger) and/or triggering user feedback (toast or UI
state) instead of silently ignoring errors; ensure you still only set
document.cookie using SSO_PUBLIC_JWT_COOKIE when res.data?.token exists.
| const handleApiSubmit = async (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| setLoading('api'); | ||
| try { | ||
| const res = await fetch('/api/sso/public/value', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ value: apiInput }), | ||
| credentials: 'include', // Include cookies in the request | ||
| }); | ||
| const data = (await res.json()) as { value?: string }; | ||
| setApiValue(typeof data?.value === 'string' ? data.value : ''); | ||
| setApiInput(''); | ||
| await loadApiValue(); | ||
| } finally { | ||
| setLoading(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing error handling for API calls.
The fetch calls don't check res.ok before parsing JSON. A 401/403/500 response will still attempt to parse JSON and may produce confusing results.
🛡️ Proposed fix
const handleApiSubmit = async (e: React.FormEvent) => {
e.preventDefault();
setLoading('api');
try {
const res = await fetch('/api/sso/public/value', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ value: apiInput }),
credentials: 'include',
});
+ if (!res.ok) {
+ throw new Error(`API error: ${res.status}`);
+ }
const data = (await res.json()) as { value?: string };
setApiValue(typeof data?.value === 'string' ? data.value : '');
setApiInput('');
- await loadApiValue();
+ } catch (err) {
+ console.error('Failed to update value:', err);
} finally {
setLoading(null);
}
};📝 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 handleApiSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| setLoading('api'); | |
| try { | |
| const res = await fetch('/api/sso/public/value', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ value: apiInput }), | |
| credentials: 'include', // Include cookies in the request | |
| }); | |
| const data = (await res.json()) as { value?: string }; | |
| setApiValue(typeof data?.value === 'string' ? data.value : ''); | |
| setApiInput(''); | |
| await loadApiValue(); | |
| } finally { | |
| setLoading(null); | |
| } | |
| }; | |
| const handleApiSubmit = async (e: React.FormEvent) => { | |
| e.preventDefault(); | |
| setLoading('api'); | |
| try { | |
| const res = await fetch('/api/sso/public/value', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ value: apiInput }), | |
| credentials: 'include', | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`API error: ${res.status}`); | |
| } | |
| const data = (await res.json()) as { value?: string }; | |
| setApiValue(typeof data?.value === 'string' ? data.value : ''); | |
| setApiInput(''); | |
| } catch (err) { | |
| console.error('Failed to update value:', err); | |
| } finally { | |
| setLoading(null); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/webapp/routes/sso/public.tsx` around lines 134 - 151, handleApiSubmit
currently parses the response without checking HTTP status, so non-2xx responses
(401/403/500) can produce confusing results; update handleApiSubmit to check
res.ok after fetch and handle errors: if !res.ok, read error text/JSON if
available and surface it (throw or set an error state) instead of proceeding to
parse into {value}, ensure setLoading is cleared in the finally block as already
done, and keep the successful path to setApiValue, setApiInput, and call
loadApiValue; reference the handleApiSubmit function and related setters
(setApiValue, setApiInput, setLoading, loadApiValue) when making the change.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores