Remove trusted_issuer and enable runtime SDK overrides via sdk config block#3078
Open
JayaShakthi97 wants to merge 1 commit into
Open
Remove trusted_issuer and enable runtime SDK overrides via sdk config block#3078JayaShakthi97 wants to merge 1 commit into
JayaShakthi97 wants to merge 1 commit into
Conversation
JayaShakthi97
requested review from
DonOmalVindula,
ayeshajay,
brionmario,
jeradrutnam and
rajithacharith
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughRemoves trusted-issuer types and context helpers, switches console runtime and Helm wiring to an ChangesTrusted-Issuer Config Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
docs/content/guides/guides/trusted-issuer.mdxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/apps/console/src/__tests__/AppWithDecorators.test.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/apps/console/src/hocs/__tests__/withConfig.test.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
JayaShakthi97
force-pushed
the
feature/console-sdk-config-override
branch
from
18cfc1c to
1853771
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/apps/console/src/hocs/withConfig.tsx (1)
27-27: ⚡ Quick winTrusted-issuer getters remain only in test mocks;
withConfigno longer uses them.
frontend/apps/console/src/hocs/withConfig.tsxonly destructuresgetClientUrlandconfigfromuseConfig, sogetTrustedIssuerUrl/getTrustedIssuerClientId/getTrustedIssuerScopes/isTrustedIssuerGenericOidcaren’t referenced there. The identifiers still appear in__tests__useConfigmocks (e.g.,frontend/apps/console/src/__tests__/AppWithDecorators.test.tsxand other*/__tests__/*.test.tsx), so those mocks are likely stale and can be removed/trimmed if unused.🤖 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 `@frontend/apps/console/src/hocs/withConfig.tsx` at line 27, withConfig.tsx now only uses useConfig's getClientUrl and config, so the trusted-issuer getters (getTrustedIssuerUrl, getTrustedIssuerClientId, getTrustedIssuerScopes, isTrustedIssuerGenericOidc) in the useConfig test mocks are stale; update the tests by removing or trimming those unused mock properties (e.g., in __tests__/AppWithDecorators.test.tsx and other */__tests__/*.test.tsx) so mocks only provide getClientUrl and config (and any other actually used keys), ensuring mock shapes match what withConfig.tsx and useConfig() expect.
🤖 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 `@docs/content/guides/guides/trusted-issuer.mdx`:
- Line 59: Replace the hardcoded brand literal "ThunderID-specific" inside the
fenced code block comment with the placeholder "{{ProductName}}-specific" so the
docs remain brand-agnostic; locate the fenced block containing the comment text
"and allow CORS-safe uncredentialed token requests." and update the comment
string "ThunderID-specific" to "{{ProductName}}-specific".
- Around line 144-155: The docs reference a Helm key
configuration.consoleClient.sdk (and fields like consoleClient.sdk.baseUrl)
which doesn’t match the current chart/runtime that exposes consoleClient.path,
clientId, scopes and renders only brand/client/server in
window.__THUNDERID_RUNTIME_CONFIG__; update the guide to match the actual chart
contract by replacing references to configuration.consoleClient.sdk and its
nested fields with the correct keys (consoleClient.path, consoleClient.clientId,
consoleClient.scopes, etc.) and show the exact runtime shape
(window.__THUNDERID_RUNTIME_CONFIG__ with brand/client/server and the
consoleClient keys), or alternatively update the chart to emit the sdk subtree
if you intend to keep the doc as-is—ensure all mentions (including the other
location noted) are made consistent.
In `@frontend/apps/console/src/hocs/__tests__/withConfig.test.tsx`:
- Around line 187-192: The test name claims merging but only replaces
preferences because sdkDefaults has no preferences; update the test to exercise
actual merge by ensuring sdkDefaults includes sibling keys (e.g., add a default
preferences object with another key like timeout or theme in the sdkDefaults
fixture) and then set mockConfig.sdk = {preferences: {resolveFromMeta: true}}
and assert that capturedProviderProps.preferences equals the merged object
containing both resolveFromMeta and the default sibling(s) (referencing
sdkDefaults, mockConfig.sdk, WithConfigComponent, and
capturedProviderProps.preferences), or alternatively rename the test to "sets
preferences" if you intend not to test merge behavior.
In `@frontend/packages/contexts/src/Config/types.ts`:
- Around line 162-165: The doc comment in ThunderIDProvider types incorrectly
says baseUrl, clientId, afterSignInUrl and scopes are "set from env vars";
update the comment to accurately reflect how values are sourced: state that only
baseUrl and clientId are pulled from env vars (via withConfig.tsx),
afterSignInUrl is derived from getClientUrl() with env as a fallback, and scopes
are not provided by env by default and only apply when supplied via config.sdk;
reference ThunderIDProvider, withConfig.tsx, getClientUrl(), and config.sdk so
the maintainer can locate and correct the comment accordingly.
---
Nitpick comments:
In `@frontend/apps/console/src/hocs/withConfig.tsx`:
- Line 27: withConfig.tsx now only uses useConfig's getClientUrl and config, so
the trusted-issuer getters (getTrustedIssuerUrl, getTrustedIssuerClientId,
getTrustedIssuerScopes, isTrustedIssuerGenericOidc) in the useConfig test mocks
are stale; update the tests by removing or trimming those unused mock properties
(e.g., in __tests__/AppWithDecorators.test.tsx and other */__tests__/*.test.tsx)
so mocks only provide getClientUrl and config (and any other actually used
keys), ensuring mock shapes match what withConfig.tsx and useConfig() expect.
🪄 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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b31ac2f4-bdf4-49c6-9fa9-7ad016f04189
📒 Files selected for processing (10)
docs/content/guides/guides/trusted-issuer.mdxfrontend/apps/console/src/hocs/__tests__/withConfig.test.tsxfrontend/apps/console/src/hocs/withConfig.tsxfrontend/apps/console/src/layouts/DashboardLayout.tsxfrontend/packages/contexts/src/Config/ConfigContext.tsxfrontend/packages/contexts/src/Config/ConfigProvider.tsxfrontend/packages/contexts/src/Config/__tests__/ConfigProvider.test.tsxfrontend/packages/contexts/src/Config/types.tsfrontend/packages/contexts/src/index.tsinstall/helm/conf/apps/console/config.js
💤 Files with no reviewable changes (4)
- frontend/packages/contexts/src/Config/tests/ConfigProvider.test.tsx
- install/helm/conf/apps/console/config.js
- frontend/packages/contexts/src/Config/ConfigContext.tsx
- frontend/packages/contexts/src/Config/ConfigProvider.tsx
| type: "generic", | ||
| signInOptions: {resource: "https://<this-thunderid-instance-url>"}, | ||
| // For generic OIDC providers: suppress ThunderID-specific bootstrap calls | ||
| // and allow CORS-safe uncredentialed token requests. |
Contributor
There was a problem hiding this comment.
Replace hardcoded ThunderID in docs code snippet commentary.
Line 59 uses a hardcoded brand literal (ThunderID-specific) in a fenced block comment. Replace it with {{ProductName}}-specific so docs remain brand-agnostic.
As per coding guidelines: “docs/**: Scan for hardcoded occurrences of Thunder or ThunderID… flag every occurrence as a major issue… .mdx fenced code blocks… Product name → {{ProductName}} placeholder.”
🤖 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 `@docs/content/guides/guides/trusted-issuer.mdx` at line 59, Replace the
hardcoded brand literal "ThunderID-specific" inside the fenced code block
comment with the placeholder "{{ProductName}}-specific" so the docs remain
brand-agnostic; locate the fenced block containing the comment text "and allow
CORS-safe uncredentialed token requests." and update the comment string
"ThunderID-specific" to "{{ProductName}}-specific".
Comment on lines
+187
to
192
| it('merges config.sdk.preferences, preserving unspecified sibling keys', () => { | ||
| mockConfig.sdk = {preferences: {resolveFromMeta: true}}; | ||
|
|
||
| render(<WithConfigComponent />); | ||
| expect(capturedProviderProps.preferences).toEqual({ | ||
| resolveFromMeta: false, | ||
| theme: {inheritFromBranding: false}, | ||
| }); | ||
| expect(capturedProviderProps.preferences).toEqual({resolveFromMeta: true}); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test name claims merge behavior it doesn't exercise.
sdkDefaults contains no preferences, so there are no default sibling keys to preserve — the result is simply config.sdk.preferences verbatim, identical to the earlier "sets preferences" test. As written it gives false confidence that nested merge preserves siblings (which ties directly to the deep-vs-shallow merge question). Either rename it to reflect that it just sets preferences, or restructure to actually assert sibling preservation against a known default.
🤖 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 `@frontend/apps/console/src/hocs/__tests__/withConfig.test.tsx` around lines
187 - 192, The test name claims merging but only replaces preferences because
sdkDefaults has no preferences; update the test to exercise actual merge by
ensuring sdkDefaults includes sibling keys (e.g., add a default preferences
object with another key like timeout or theme in the sdkDefaults fixture) and
then set mockConfig.sdk = {preferences: {resolveFromMeta: true}} and assert that
capturedProviderProps.preferences equals the merged object containing both
resolveFromMeta and the default sibling(s) (referencing sdkDefaults,
mockConfig.sdk, WithConfigComponent, and capturedProviderProps.preferences), or
alternatively rename the test to "sets preferences" if you intend not to test
merge behavior.
Comment on lines
162
to
+165
| * Accepts any valid ThunderIDProvider prop. Values are deep-merged on top of | ||
| * the defaults derived from the application config, so only fields that need | ||
| * to differ from the computed defaults must be specified. | ||
| * `config.sdk` takes the highest precedence — it overrides both the defaults | ||
| * derived from `trusted_issuer` and the identity-related props (baseUrl, | ||
| * clientId, afterSignInUrl, scopes) resolved from the server/client config. | ||
| * the application defaults, so only fields that need to differ must be specified. | ||
| * `config.sdk` takes the highest precedence — it overrides the identity-related | ||
| * props (baseUrl, clientId, afterSignInUrl, scopes) set from env vars. |
Contributor
There was a problem hiding this comment.
Doc comment overstates which props are "set from env vars".
Cross-referencing withConfig.tsx: only baseUrl/clientId come from env vars. afterSignInUrl is derived from getClientUrl() (env is only the fallback), and scopes is not set by default at all — it only exists when supplied via config.sdk. Listing afterSignInUrl and scopes as env-derived props may send operators looking for a non-existent scopes env var.
📝 Suggested wording
- * `config.sdk` takes the highest precedence — it overrides the identity-related
- * props (baseUrl, clientId, afterSignInUrl, scopes) set from env vars.
+ * `config.sdk` takes the highest precedence — it overrides the identity-related
+ * props derived elsewhere: `baseUrl`/`clientId` (from env vars), `afterSignInUrl`
+ * (from the resolved client URL, with an env-var fallback), and `scopes`.📝 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
| * Accepts any valid ThunderIDProvider prop. Values are deep-merged on top of | |
| * the defaults derived from the application config, so only fields that need | |
| * to differ from the computed defaults must be specified. | |
| * `config.sdk` takes the highest precedence — it overrides both the defaults | |
| * derived from `trusted_issuer` and the identity-related props (baseUrl, | |
| * clientId, afterSignInUrl, scopes) resolved from the server/client config. | |
| * the application defaults, so only fields that need to differ must be specified. | |
| * `config.sdk` takes the highest precedence — it overrides the identity-related | |
| * props (baseUrl, clientId, afterSignInUrl, scopes) set from env vars. | |
| * Accepts any valid ThunderIDProvider prop. Values are deep-merged on top of | |
| * the application defaults, so only fields that need to differ must be specified. | |
| * `config.sdk` takes the highest precedence — it overrides the identity-related | |
| * props derived elsewhere: `baseUrl`/`clientId` (from env vars), `afterSignInUrl` | |
| * (from the resolved client URL, with an env-var fallback), and `scopes`. |
🤖 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 `@frontend/packages/contexts/src/Config/types.ts` around lines 162 - 165, The
doc comment in ThunderIDProvider types incorrectly says baseUrl, clientId,
afterSignInUrl and scopes are "set from env vars"; update the comment to
accurately reflect how values are sourced: state that only baseUrl and clientId
are pulled from env vars (via withConfig.tsx), afterSignInUrl is derived from
getClientUrl() with env as a fallback, and scopes are not provided by env by
default and only apply when supplied via config.sdk; reference
ThunderIDProvider, withConfig.tsx, getClientUrl(), and config.sdk so the
maintainer can locate and correct the comment accordingly.
The trusted_issuer block in config.js let operators point the console at an external auth server, but it duplicated what the sdk block (introduced in thunder-id#2991) already provides. Operators can now supply baseUrl, clientId, scopes, signInOptions, preferences, and sendCookiesInRequests directly via sdk without needing a separate first-class config key. Removed: - TrustedIssuerConfig type and trusted_issuer field from ProductConfig - getTrustedIssuerUrl, getTrustedIssuerClientId, getTrustedIssuerScopes, isTrustedIssuerGenericOidc from ConfigContextType and ConfigProvider - All trusted_issuer-driven conditional logic in withConfig.tsx - Generic-OIDC custom sign-out path in DashboardLayout.tsx - trusted_issuer Helm template block in install/helm/conf/apps/console/config.js - All related tests Updated docs/content/guides/guides/trusted-issuer.mdx to describe the equivalent sdk block configuration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JayaShakthi97
force-pushed
the
feature/console-sdk-config-override
branch
from
1853771 to
7091694
Compare
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Purpose
Removes the
trusted_issuerfirst-class config key from the console in favour of thesdkblock introduced in #2991. Operators can configurebaseUrl,clientId,scopes,signInOptions,preferences, andsendCookiesInRequestsdirectly in thesdkblock without needing a separate config key.Approach
Removed
TrustedIssuerConfigand all related types, context methods, and conditional logic:TrustedIssuerConfigtype andtrusted_issuerfield fromProductConfiggetTrustedIssuerUrl,getTrustedIssuerClientId,getTrustedIssuerScopes,isTrustedIssuerGenericOidcfromConfigContextTypeandConfigProvidertrusted_issuer-driven conditional logic inwithConfig.tsxDashboardLayout.tsxtrusted_issuerHelm template block ininstall/helm/conf/apps/console/config.jsUpdated
docs/content/guides/guides/trusted-issuer.mdxto document the equivalentsdkblock configuration pattern.Related Issues
Related PRs
sdkblockChecklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Documentation
Refactor
Tests