fix: remove sign-in popup in backstage by default and make it configurable#440
Merged
mevan-karu merged 1 commit intoopenchoreo:mainfrom Mar 19, 2026
Merged
fix: remove sign-in popup in backstage by default and make it configurable#440mevan-karu merged 1 commit intoopenchoreo:mainfrom
mevan-karu merged 1 commit intoopenchoreo:mainfrom
Conversation
mevan-karu
requested review from
jhivandb,
kaviththiranga and
sameerajayasoma
as code owners
📝 WalkthroughWalkthroughAdds a feature flag to enable a redirect-based OAuth sign-in flow, implements an AutoRedirectSignInPage that checks/fetches Backstage identity and triggers redirect/instant-popup flows, wires Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App.tsx\n(DynamicSignInPage)
participant Auto as AutoRedirectSignInPage
participant Identity as Backstage\nIdentity API
participant IDP as OAuth\nProvider/IDP
User->>App: Load sign-in page
App->>App: Read configApi.enableExperimentalRedirectFlow
alt feature enabled
App->>Auto: Render AutoRedirectSignInPage
Auto->>Identity: getBackstageIdentity(optional: true)
alt identity exists
Identity-->>Auto: Return identity
Auto->>Identity: Fetch profile
Identity-->>Auto: Return profile
Auto->>App: onSignInSuccess(UserIdentity)
App-->>User: Signed in
else no identity
Identity-->>Auto: No identity
Auto->>Identity: getBackstageIdentity(instantPopup: true)
alt instant popup returns identity
Identity-->>Auto: Return identity
Auto->>App: onSignInSuccess(UserIdentity)
App-->>User: Signed in
else fallback
Auto->>IDP: Redirect to IDP (full-page redirect)
IDP-->>User: OAuth redirect flow
end
end
else feature disabled
App-->>User: Render existing popup-based SignInPage
end
opt error occurs
Auto->>App: errorApiRef.post(error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
packages/app/src/App.tsx (1)
132-133: Consider documenting the reason for disabling exhaustive-deps.The empty dependency array with the eslint-disable comment is intentional for running the sign-in check only once on mount. While this pattern is acceptable for sign-in flows, consider adding a brief comment explaining why the effect should only run once to help future maintainers.
📝 Suggested comment
}) .catch((err: Error) => errorApi.post(err)); - // eslint-disable-next-line react-hooks/exhaustive-deps + // Run only on mount - sign-in should happen once, then the page redirects or navigates away + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/App.tsx` around lines 132 - 133, Add a brief inline comment above the eslint-disable-next-line react-hooks/exhaustive-deps in App.tsx explaining that the useEffect (which performs the sign-in check on mount) intentionally uses an empty dependency array so it only runs once on component mount and should not re-run when props/state change; reference the useEffect/sign-in check to make it clear why disabling exhaustive-deps is necessary for this flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app/src/App.tsx`:
- Around line 132-133: Add a brief inline comment above the
eslint-disable-next-line react-hooks/exhaustive-deps in App.tsx explaining that
the useEffect (which performs the sign-in check on mount) intentionally uses an
empty dependency array so it only runs once on component mount and should not
re-run when props/state change; reference the useEffect/sign-in check to make it
clear why disabling exhaustive-deps is necessary for this flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 370a5071-58f2-4d80-a592-a66d1f5197cc
📒 Files selected for processing (4)
app-config.production.yamlapp-config.yamlpackages/app/src/App.tsxpackages/app/src/apis.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/App.tsx`:
- Around line 131-135: The App component currently only renders <Progress />
after the useEffect that calls errorApi.post on auth errors, causing a permanent
loading state; update the App component to track an authError state (e.g.,
authError / setAuthError) inside the same effect (watch the promise catch that
invokes errorApi.post) and set that state when an error occurs, then switch
rendering from always returning <Progress /> to a conditional render that shows
a recoverable fallback UI (a retry button that re-runs the auth redirect logic
or a sign-in prompt) when authError is set, while keeping <Progress /> during
normal loading; reference the existing errorApi.post call and the Progress
component so you update the same effect and render branch.
- Around line 127-129: Awaiting authApi.getBackstageIdentity({ instantPopup:
true }) currently ignores the resolved identity; change the code to capture the
returned identity (e.g., const identity = await authApi.getBackstageIdentity({
instantPopup: true })) and then call onSignInSuccess(identity) so the success
path mirrors the earlier "existing identity" branch (the same pattern used
before lines handling identity and onSignInSuccess), ensuring the UI advances
from <Progress /> when instantPopup resolves without a full-page redirect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8534a880-dfdd-421c-b2da-e1bbe8553392
📒 Files selected for processing (5)
app-config.production.yamlapp-config.yamlpackages/app/src/App.tsxpackages/app/src/apis.tspackages/app/src/apis/authRefs.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app-config.production.yaml
- app-config.yaml
kaviththiranga
previously approved these changes
Mar 18, 2026
…rable Signed-off-by: Mevan <mevan.karu@gmail.com>
kaviththiranga
approved these changes
Mar 19, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/app/src/App.tsx (2)
193-199: Consider namespacing the config key underopenchoreo.features.The
enableExperimentalRedirectFlowconfig key is read from the root level, while the relatedauth.enabledsetting usesopenchoreo.features.auth.enabled. For consistency and discoverability, consider moving this toopenchoreo.features.enableExperimentalRedirectFlowor similar.♻️ Suggested change for consistency
// When redirect flow is enabled, skip the sign-in page entirely and redirect immediately const enableExperimentalRedirectFlow = - configApi.getOptionalBoolean('enableExperimentalRedirectFlow') ?? false; + configApi.getOptionalBoolean('openchoreo.features.enableExperimentalRedirectFlow') ?? false;Note: This would require corresponding updates to
app-config.yamlandapp-config.production.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/App.tsx` around lines 193 - 199, Currently the code reads enableExperimentalRedirectFlow from the root using configApi.getOptionalBoolean('enableExperimentalRedirectFlow') and returns <AutoRedirectSignInPage /> when true; change the config key to a namespaced key such as 'openchoreo.features.enableExperimentalRedirectFlow' by updating the call to configApi.getOptionalBoolean('openchoreo.features.enableExperimentalRedirectFlow') (and keeping the fallback default), update any references to enableExperimentalRedirectFlow in the component logic, and update app-config.yaml and app-config.production.yaml to define the new key under openchoreo.features so the value is discoverable and consistent with openchoreo.features.auth.enabled.
109-109: Consider adding explicit props typing.Using
anyfor props works but loses type safety. Consider using Backstage'sSignInPagePropstype for better IDE support and compile-time checks.♻️ Optional typing improvement
+import type { SignInPageProps } from '@backstage/core-plugin-api'; + -function AutoRedirectSignInPage(props: any) { +function AutoRedirectSignInPage(props: SignInPageProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/App.tsx` at line 109, AutoRedirectSignInPage currently types its parameter as any; replace it with Backstage's SignInPageProps for type safety by importing SignInPageProps and changing the function signature to use that type (e.g., function AutoRedirectSignInPage(props: SignInPageProps)). Add the appropriate import for SignInPageProps from the Backstage package that exports it and update any usages inside AutoRedirectSignInPage to match the typed props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app/src/App.tsx`:
- Around line 193-199: Currently the code reads enableExperimentalRedirectFlow
from the root using
configApi.getOptionalBoolean('enableExperimentalRedirectFlow') and returns
<AutoRedirectSignInPage /> when true; change the config key to a namespaced key
such as 'openchoreo.features.enableExperimentalRedirectFlow' by updating the
call to
configApi.getOptionalBoolean('openchoreo.features.enableExperimentalRedirectFlow')
(and keeping the fallback default), update any references to
enableExperimentalRedirectFlow in the component logic, and update
app-config.yaml and app-config.production.yaml to define the new key under
openchoreo.features so the value is discoverable and consistent with
openchoreo.features.auth.enabled.
- Line 109: AutoRedirectSignInPage currently types its parameter as any; replace
it with Backstage's SignInPageProps for type safety by importing SignInPageProps
and changing the function signature to use that type (e.g., function
AutoRedirectSignInPage(props: SignInPageProps)). Add the appropriate import for
SignInPageProps from the Backstage package that exports it and update any usages
inside AutoRedirectSignInPage to match the typed props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5704cbfb-49f5-408d-b20d-14be9509bd98
📒 Files selected for processing (5)
app-config.production.yamlapp-config.yamlpackages/app/src/App.tsxpackages/app/src/apis.tspackages/app/src/apis/authRefs.ts
✅ Files skipped from review due to trivial changes (2)
- app-config.yaml
- app-config.production.yaml
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
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.
This pull request introduces support for a redirect-based OAuth authentication flow, streamlining the sign-in experience by skipping the popup window and optionally bypassing the sign-in page entirely. The changes add configuration options for this flow, update the authentication logic to handle redirects, and ensure the app can dynamically switch between authentication modes.
Authentication flow improvements:
enableExperimentalRedirectFlowconfiguration option to bothapp-config.yamlandapp-config.production.yaml, allowing control over the redirect-based OAuth flow via environment variable or config file. [1] [2]AutoRedirectSignInPagecomponent inApp.tsxthat immediately checks for an existing session and triggers a full-page redirect if none is found, replacing the sign-in page when the redirect flow is enabled.DynamicSignInPagelogic to conditionally useAutoRedirectSignInPagebased on theenableExperimentalRedirectFlowconfig, skipping the sign-in page when enabled.Code and API updates:
configApito the OAuth2 provider factory inapis.ts, ensuring proper configuration access for authentication.App.tsxto include necessary components and APIs for the new authentication flow.Summary by CodeRabbit
New Features
Chores