Fix minor issues in SDKs#13
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR updates CommonJS build outputs to ChangesCJS Build Output Rename
OIDC Discovery Fallback and Nuxt Route Removal
Sample and Template Updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/react/vite-react-template/src/Dashboard.jsx (1)
1-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd missing copyright header.
Static analysis flags this new file as missing the required WSO2 Apache 2.0 copyright header enforced by the
@thunderid/copyright-headerrule.🤖 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 `@templates/react/vite-react-template/src/Dashboard.jsx` around lines 1 - 10, The new Dashboard component is missing the required WSO2 Apache 2.0 copyright header. Add the standard copyright notice at the top of the Dashboard.jsx file before the existing imports, matching the format used by other files covered by the `@thunderid/copyright-header` rule.Source: Linters/SAST tools
🤖 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 `@packages/javascript/src/ThunderIDJavaScriptClient.ts`:
- Around line 356-365: The discovery handling in ThunderIDJavaScriptClient still
throws raw JSON/endpoint-resolution errors for successful HTTP responses instead
of preserving the malformed response on the baseUrl fallback path. Update the
response-handling logic around the response.json() and
authHelper.resolveEndpoints call so failures from parsing/resolving discovery
metadata are caught and routed to the existing baseUrl fallback, and only raise
ThunderIDAuthException if the fallback also fails. While doing so, remove the
unsafe any usage in the catch and ensure ThunderIDAuthException receives a
string message rather than passing the raw error object.
In `@samples/express/quickstart/.env.example`:
- Around line 5-6: The .env.example entry for NODE_TLS_REJECT_UNAUTHORIZED is
too risky to ship as a copy-paste default; update the quickstart example to
avoid encouraging global TLS disablement. Remove NODE_TLS_REJECT_UNAUTHORIZED=0
from the sample env file and document it as an explicit dev-only runtime flag,
or replace it with NODE_EXTRA_CA_CERTS for the self-signed cert; if you keep any
warning text, make it a prominent DANGER note.
- Line 3: The Express quickstart env example includes a stray
NEXT_PUBLIC_THUNDERID_APPLICATION_ID entry that is not used by
samples/express/quickstart/index.js. Update the .env.example contents to match
the Express configuration keys used by the app (THUNDERID_BASE_URL,
THUNDERID_CLIENT_ID, THUNDERID_CLIENT_SECRET) or remove the unused variable so
the example stays consistent with the actual setup.
In `@samples/nextjs/quickstart/.env.example`:
- Around line 6-7: The .env.example entry for NODE_TLS_REJECT_UNAUTHORIZED=0 in
the Next.js quickstart sample should be made safer or more clearly marked as
dangerous. Update the comment near the NODE_TLS_REJECT_UNAUTHORIZED setting to
use a stronger warning (for example a DANGER-style prefix) and/or replace it
with a safer local-dev approach such as NODE_EXTRA_CA_CERTS or a runtime flag so
it is less likely to be copy-pasted into production.
In `@samples/node/quickstart/.env.example`:
- Around line 4-5: The .env.example entry for NODE_TLS_REJECT_UNAUTHORIZED=0 in
the quickstart sample is too easy to copy into production and globally disables
TLS verification. Update the example around the NODE_TLS_REJECT_UNAUTHORIZED
setting to use safer guidance, such as making it clearly local-only with a
stronger warning or replacing it with a more explicit development-only
alternative in the sample config.
In `@samples/nuxt/quickstart/.env.example`:
- Around line 5-6: The .env.example entry for NODE_TLS_REJECT_UNAUTHORIZED=0 is
too easy to copy into production, so update the example in the quickstart
environment file to avoid normalizing global TLS disablement. Either replace it
with a safer local-only alternative or make the warning in the comment much
stronger and more explicit that it must never be used in production, while
keeping the guidance near the NODE_TLS_REJECT_UNAUTHORIZED example.
In `@templates/react/vite-react-template/src/Home.jsx`:
- Around line 1-9: The Home component currently always renders SignInButton, so
authenticated users never get the signed-in experience on the root route. Update
Home in Home.jsx to restore the SignedIn/SignedOut split using the existing auth
components, or otherwise add a dashboard link/redirect for signed-in users so /
still provides dashboard access. Keep the fix localized to the Home component
and preserve the current unauthenticated sign-in behavior.
---
Outside diff comments:
In `@templates/react/vite-react-template/src/Dashboard.jsx`:
- Around line 1-10: The new Dashboard component is missing the required WSO2
Apache 2.0 copyright header. Add the standard copyright notice at the top of the
Dashboard.jsx file before the existing imports, matching the format used by
other files covered by the `@thunderid/copyright-header` rule.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 852eee0f-a1f3-47b3-b73c-343502ac19d4
📒 Files selected for processing (19)
packages/browser/package.jsonpackages/browser/rolldown.config.jspackages/express/package.jsonpackages/javascript/src/ThunderIDJavaScriptClient.tspackages/nuxt/src/module.tspackages/react-router/package.jsonpackages/react-router/rolldown.config.jspackages/tanstack-router/package.jsonpackages/tanstack-router/rolldown.config.jssamples/express/quickstart/.env.examplesamples/nextjs/quickstart/.env.examplesamples/node/quickstart/.env.examplesamples/nuxt/quickstart/.env.examplesamples/react/quickstart/src/App.jsxsamples/react/quickstart/src/main.jsxtemplates/react/vite-react-template/src/App.jsxtemplates/react/vite-react-template/src/Dashboard.jsxtemplates/react/vite-react-template/src/Home.jsxtemplates/react/vite-react-template/src/main.jsx
💤 Files with no reviewable changes (3)
- samples/react/quickstart/src/main.jsx
- packages/nuxt/src/module.ts
- templates/react/vite-react-template/src/main.jsx
| import { SignInButton } from '@thunderid/react' | ||
|
|
||
| export default function Home() { | ||
| return ( | ||
| <main> | ||
| <SignedOut> | ||
| <h1>Welcome</h1> | ||
| <SignInButton /> | ||
| </SignedOut> | ||
| <SignedIn> | ||
| <User>{(user) => <h1>Hello, {user.given_name || user.username}.</h1>}</User> | ||
| <SignOutButton /> | ||
| </SignedIn> | ||
| <SignInButton /> | ||
| </main> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
@thunderid/react SignInButton component behavior when user is already signed in
💡 Result:
In the @thunderid/react SDK, the SignInButton component is designed to be used in conjunction with conditional rendering components like SignedIn and SignedOut [1][2]. When a user is already signed in, the SignInButton component is not rendered because developers typically wrap it inside the SignedOut component [1][3][4]. The SignedOut component automatically hides its children when an authentication session is detected [1][4]. If you were to manually render the SignInButton while a user is already signed in, the SDK's internal authentication state management would recognize the active session, and clicking the button would typically result in no action, or it would simply redirect the user back to the application or your configured post-login redirect URI, as the authentication flow is already satisfied [4]. To implement this correctly, use the provided conditional rendering components as shown in the official patterns: import { SignedIn, SignedOut, SignInButton, SignOutButton } from '@thunderid/react'; function App { return ( <> Sign In Sign Out </>); } By following this pattern, the SignInButton is physically removed from the DOM when the user has an active session [1][4].
Citations:
- 1: https://thunderid.dev/docs/next/guides/getting-started/connect-your-application/react/
- 2: https://thunderid.dev/docs/next/sdks/react/overview/
- 3: https://thunderid.dev/docs/next/guides/getting-started/connect-your-application/nextjs/
- 4: https://medium.com/@nisargmakwana22/i-tried-self-hosted-auth-for-the-first-time-heres-what-i-learned-e0cb779016de
🏁 Script executed:
git ls-files | rg '^(templates/react/vite-react-template|templates/react|.*react.*template).*'Repository: thunder-id/javascript-sdks
Length of output: 656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files templates/react/vite-react-template/src templates/react/vite-react-template/README.md templates/react/vite-react-template/package.json
printf '\n== main.jsx ==\n'
cat -n templates/react/vite-react-template/src/main.jsx
printf '\n== App.jsx ==\n'
cat -n templates/react/vite-react-template/src/App.jsx
printf '\n== Home.jsx ==\n'
cat -n templates/react/vite-react-template/src/Home.jsx
printf '\n== Dashboard.jsx ==\n'
cat -n templates/react/vite-react-template/src/Dashboard.jsx
printf '\n== README.md (relevant lines) ==\n'
rg -n "dashboard|route|react-router|SignedIn|SignedOut|SignInButton|SignOutButton" templates/react/vite-react-template/README.mdRepository: thunder-id/javascript-sdks
Length of output: 2305
Restore the signed-in Home state
templates/react/vite-react-template/src/Home.jsx now always shows SignInButton, so authenticated users no longer get any dashboard affordance on /. Bring back the SignedIn/SignedOut split or add a link/redirect to /dashboard.
🧰 Tools
🪛 ESLint
[error] 1-10: Missing WSO2 Apache 2.0 copyright header
(@thunderid/copyright-header)
[error] 1-1: Unable to resolve path to module '@thunderid/react'.
(import-x/no-unresolved)
🤖 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 `@templates/react/vite-react-template/src/Home.jsx` around lines 1 - 9, The
Home component currently always renders SignInButton, so authenticated users
never get the signed-in experience on the root route. Update Home in Home.jsx to
restore the SignedIn/SignedOut split using the existing auth components, or
otherwise add a dashboard link/redirect for signed-in users so / still provides
dashboard access. Keep the fix localized to the Home component and preserve the
current unauthenticated sign-in behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@templates/react/vite-react-template/src/Dashboard.jsx`:
- Line 1: The new Dashboard.jsx file is missing the repository’s standard Apache
copyright banner, which will cause lint to fail. Add the required Apache header
comment at the top of the file, above the existing UserDropdown import, matching
the template used by other files in this repository.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5b62bcb-f827-453f-a60c-cd07bfbec1b5
📒 Files selected for processing (21)
packages/browser/package.jsonpackages/browser/rolldown.config.jspackages/express/package.jsonpackages/javascript/src/ThunderIDJavaScriptClient.tspackages/nuxt/src/module.tspackages/react-router/package.jsonpackages/react-router/rolldown.config.jspackages/tanstack-router/package.jsonpackages/tanstack-router/rolldown.config.jssamples/express/quickstart/.env.examplesamples/nextjs/quickstart/.env.examplesamples/node/quickstart/.env.examplesamples/nuxt/quickstart/.env.examplesamples/react/quickstart/src/App.jsxsamples/react/quickstart/src/main.jsxtemplates/nextjs/nextjs-template/app/page.tsxtemplates/react/vite-react-template/src/App.jsxtemplates/react/vite-react-template/src/Dashboard.jsxtemplates/react/vite-react-template/src/Home.jsxtemplates/react/vite-react-template/src/main.jsxtemplates/vue/vite-vue-template/src/App.vue
💤 Files with no reviewable changes (3)
- templates/react/vite-react-template/src/main.jsx
- samples/react/quickstart/src/main.jsx
- packages/nuxt/src/module.ts
✅ Files skipped from review due to trivial changes (3)
- packages/browser/rolldown.config.js
- packages/tanstack-router/package.json
- packages/tanstack-router/rolldown.config.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-router/rolldown.config.js
- packages/browser/package.json
- samples/react/quickstart/src/App.jsx
- packages/express/package.json
- packages/react-router/package.json
| @@ -0,0 +1,9 @@ | |||
| import { UserDropdown } from '@thunderid/react' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the standard Apache header.
This new file is missing the repository’s required copyright banner, so the lint step will keep failing until it is added.
🧰 Tools
🪛 ESLint
[error] 1-10: Missing WSO2 Apache 2.0 copyright header
(@thunderid/copyright-header)
[error] 1-1: Unable to resolve path to module '@thunderid/react'.
(import-x/no-unresolved)
🤖 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 `@templates/react/vite-react-template/src/Dashboard.jsx` at line 1, The new
Dashboard.jsx file is missing the repository’s standard Apache copyright banner,
which will cause lint to fail. Add the required Apache header comment at the top
of the file, above the existing UserDropdown import, matching the template used
by other files in this repository.
Source: Linters/SAST tools
2 participants
Purpose
$subject
Approach
N/A
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
.cjsbuilds.