feat(x2a): RepoAuthentication renders summary of required auth tokens#2596
Merged
eloycoto merged 2 commits intoredhat-developer:mainfrom Mar 26, 2026
Merged
Conversation
Changed Packages
|
Review Summary by QodoRepoAuthentication displays provider summary with per-provider retry
WalkthroughsDescription• Renders provider authentication summary table with status indicators • Implements per-provider retry mechanism for failed authentications • Tracks individual provider statuses (pending, authenticated, error) • Displays OAuth scopes and access levels (Read/Write vs Read-only) • Adds comprehensive test coverage for error handling and retry flows Diagramflowchart LR
CSV["CSV Content"] -->|parse| Providers["Distinct Providers<br/>Target + Source"]
Providers -->|auto-auth| AuthFlow["Initial Authentication<br/>All Providers"]
AuthFlow -->|success| Table["Provider Table<br/>Status + Scope"]
AuthFlow -->|failure| ErrorState["Error State<br/>Per-Provider"]
ErrorState -->|retry click| RetryFlow["Per-Provider Retry"]
RetryFlow -->|success| Table
Table -->|all authenticated| Complete["Set Secrets<br/>onChange"]
File Changes1. workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx
|
Code Review by Qodo
1. Pending rows after failure
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+194
to
+268
| useEffect(() => { | ||
| if (!csvContent || suppressDialog || isDone) { | ||
| if (!csvContent || initialAuthFailed || isDone || parseError) { | ||
| return undefined; | ||
| } | ||
|
|
||
| setError(undefined); | ||
| const targets = distinctTargetProviders; | ||
| const sources = distinctSourceProviders; | ||
| const allDistinctProviders = [...targets, ...sources]; | ||
|
|
||
| let projectsToCreate; | ||
| try { | ||
| projectsToCreate = parseCsvContent(csvContent); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : 'Unknown error'); | ||
| if (allDistinctProviders.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const allTargetProviders: ScmProvider[] = projectsToCreate.map(project => | ||
| resolveScmProvider(project.targetRepoUrl, hostProviderMap), | ||
| ); | ||
| const allSourceProviders: ScmProvider[] = projectsToCreate.map(project => | ||
| resolveScmProvider(project.sourceRepoUrl, hostProviderMap), | ||
| ); | ||
| const distinctTargetProviders = allTargetProviders.filter( | ||
| (p, i, arr) => arr.findIndex(q => q.name === p.name) === i, | ||
| ); | ||
| const distinctSourceProviders = allSourceProviders.filter( | ||
| (p, i, arr) => | ||
| arr.findIndex(q => q.name === p.name) === i && | ||
| !distinctTargetProviders.some(t => t.name === p.name), | ||
| ); | ||
| const allDistinctProviders = [ | ||
| ...distinctTargetProviders, | ||
| ...distinctSourceProviders, | ||
| ]; | ||
|
|
||
| let cancelled = false; | ||
| const authCsvSnapshot = csvContent; | ||
|
|
||
| const doAuthAsync = async () => { | ||
| const providerTokens = new Map<string, string>(); | ||
|
|
||
| const authenticateProvider = async ( | ||
| provider: ScmProvider, | ||
| readOnly: boolean, | ||
| ) => { | ||
| try { | ||
| const tokens = await repoAuthentication.authenticate([ | ||
| const tokens = await repoAuthRef.current.authenticate([ | ||
| provider.getAuthTokenDescriptor(readOnly), | ||
| ]); | ||
| if (cancelled) { | ||
| return; | ||
| } | ||
| providerTokens.set( | ||
| providerTokensRef.current.set( | ||
| `${SCAFFOLDER_SECRET_PREFIX}${provider.name}`, | ||
| tokens[0].token, | ||
| ); | ||
| setProviderStatuses(prev => | ||
| new Map(prev).set(provider.name, 'authenticated'), | ||
| ); | ||
| } catch (e) { | ||
| if (cancelled) { | ||
| return; | ||
| } | ||
| setError(e instanceof Error ? e.message : 'Unknown error'); | ||
| setSuppressDialog(true); | ||
| setProviderStatuses(prev => | ||
| new Map(prev).set(provider.name, 'error'), | ||
| ); | ||
| setProviderErrors(prev => | ||
| new Map(prev).set( | ||
| provider.name, | ||
| e instanceof Error ? e.message : 'Unknown error', | ||
| ), | ||
| ); | ||
| setInitialAuthFailed(true); | ||
| } | ||
| }; | ||
|
|
||
| await Promise.all([ | ||
| ...distinctTargetProviders.map(p => authenticateProvider(p, false)), | ||
| ...distinctSourceProviders.map(p => authenticateProvider(p, true)), | ||
| ...targets.map(p => authenticateProvider(p, false)), | ||
| ...sources.map(p => authenticateProvider(p, true)), | ||
| ]); | ||
|
|
||
| if ( | ||
| cancelled || | ||
| authCsvSnapshot !== prevCsvRef.current || | ||
| providerTokens.size !== allDistinctProviders.length | ||
| ) { | ||
| if ( | ||
| !cancelled && | ||
| authCsvSnapshot === prevCsvRef.current && | ||
| providerTokens.size !== allDistinctProviders.length | ||
| ) { | ||
| onChange(undefined); | ||
| } | ||
| if (cancelled || authCsvSnapshot !== prevCsvRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| onChange('authenticated'); | ||
| setDone(true); | ||
| setSecrets({ | ||
| ...secretsRef.current, | ||
| ...Object.fromEntries(providerTokens), | ||
| }); | ||
| if (providerTokensRef.current.size !== allDistinctProviders.length) { | ||
| onChangeRef.current(undefined); | ||
| } | ||
| }; | ||
|
|
||
| doAuthAsync(); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [ | ||
| csvContent, | ||
| hostProviderMap, | ||
| repoAuthentication, | ||
| setSecrets, | ||
| suppressDialog, | ||
| isDone, | ||
| onChange, | ||
| ]); | ||
| // csvContent change is the meaningful trigger for re-authentication. | ||
| // distinctTarget/SourceProviders are derived from it and captured above. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [csvContent, initialAuthFailed, isDone, parseError]); |
There was a problem hiding this comment.
1. Pending rows after failure 🐞 Bug ⛯ Reliability
When any provider authentication fails, the component sets initialAuthFailed=true, which is also a dependency of the auto-auth effect; this triggers effect cleanup (cancelled=true) while other provider auth calls may still be in-flight, preventing those providers from ever setting status/error. Those providers remain in 'pending' and do not render a Retry button, which can block completion indefinitely.
Agent Prompt
### Issue description
Auto-auth cancels other providers’ in-flight authentication when one provider fails because `initialAuthFailed` is in the effect dependency array and is set inside the effect, triggering cleanup (`cancelled=true`). This can strand other providers in `pending` with no Retry button.
### Issue Context
- The initial auto-auth effect sets `initialAuthFailed` on first failure.
- `initialAuthFailed` is also a dependency, so the effect re-runs and previous cleanup runs.
- Pending rows never show Retry; completion requires all rows authenticated.
### Fix Focus Areas
- Ensure changing `initialAuthFailed` does **not** trigger cleanup for the ongoing auth run. Options:
- Remove `initialAuthFailed` from the auto-auth effect dependency array (and keep the early-return guard via a ref), so the effect lifetime is tied only to `csvContent`/unmount.
- Alternatively, delay setting `initialAuthFailed` until after all provider auth attempts settle, or keep a per-run `authRunId` ref and ignore stale completions without flipping `cancelled`.
- Consider explicitly marking any still-pending providers as `error` if you intend to stop auto-auth after the first failure.
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[194-268]
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[321-373]
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[270-280]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+282
to
+312
| const retryProvider = useCallback( | ||
| async (provider: ScmProvider, readOnly: boolean) => { | ||
| setProviderStatuses(prev => new Map(prev).set(provider.name, 'pending')); | ||
| setProviderErrors(prev => { | ||
| const next = new Map(prev); | ||
| next.delete(provider.name); | ||
| return next; | ||
| }); | ||
|
|
||
| try { | ||
| const tokens = await repoAuthRef.current.authenticate([ | ||
| provider.getAuthTokenDescriptor(readOnly), | ||
| ]); | ||
|
|
||
| providerTokensRef.current.set( | ||
| `${SCAFFOLDER_SECRET_PREFIX}${provider.name}`, | ||
| tokens[0].token, | ||
| ); | ||
| setProviderStatuses(prev => | ||
| new Map(prev).set(provider.name, 'authenticated'), | ||
| ); | ||
| } catch (e) { | ||
| setProviderStatuses(prev => new Map(prev).set(provider.name, 'error')); | ||
| setProviderErrors(prev => | ||
| new Map(prev).set( | ||
| provider.name, | ||
| e instanceof Error ? e.message : 'Unknown error', | ||
| ), | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
2. Retry staleness after csv 🐞 Bug ⛯ Reliability
retryProvider applies async results (token + status) without checking whether csvContent changed or the component unmounted, so a retry started for an old CSV can update the new provider status/token map and potentially trigger setSecrets/onChange for the wrong state.
Agent Prompt
### Issue description
`retryProvider` can update status/errors/tokens after the CSV has changed (or after unmount), because it awaits an async authenticate call and then unconditionally writes to refs/state.
### Issue Context
- CSV change resets `providerTokensRef.current` and status/error maps.
- `retryProvider` has no `cancelled` flag or `csvContent` snapshot check.
- Completion/secrets are derived from `providerRows` statuses and `providerTokensRef.current`.
### Fix Focus Areas
- Add a per-run staleness guard for retries, similar to the auto-auth effect:
- Capture a snapshot (e.g., `const retryCsvSnapshot = prevCsvRef.current`) when the retry starts.
- Before writing status/tokens, verify `retryCsvSnapshot === prevCsvRef.current` and that the component is still mounted.
- Alternatively, maintain an incrementing `authGenerationRef` bumped on CSV change/unmount; store the generation at retry start and ignore results if it no longer matches.
- Ensure error/status updates are skipped when stale.
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[282-312]
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[182-192]
- workspaces/x2a/plugins/x2a/src/scaffolder/RepoAuthentication.tsx[270-280]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
mareklibra
force-pushed
the
FLPATH-3464.RepoAuthenticationSummary
branch
from
4a8bee6 to
94339bf
Compare
Signed-off-by: Marek Libra <marek.libra@gmail.com>
mareklibra
force-pushed
the
FLPATH-3464.RepoAuthenticationSummary
branch
from
94339bf to
a5623a0
Compare
Review Summary by QodoRepoAuthentication renders summary table of required auth tokens
WalkthroughsDescription• Refactored RepoAuthentication to display provider summary table • Added useProviderAuth hook for centralized auth state management • Renders per-provider status, scope, and access level information • Implements per-provider retry buttons with error handling • Comprehensive test coverage for auth flows and edge cases Diagramflowchart LR
CSV["CSV Content"] -->|parsed| useProviderAuth["useProviderAuth Hook"]
useProviderAuth -->|extracts providers| ProviderRows["Provider Rows"]
ProviderRows -->|displays| Table["Summary Table"]
Table -->|shows| Status["Status + Scope + Access"]
useProviderAuth -->|manages| Auth["Auth State"]
Auth -->|on success| Secrets["Store Secrets"]
Auth -->|on error| Retry["Per-Provider Retry"]
File Changes1. workspaces/x2a/plugins/x2a/src/scaffolder/useProviderAuth.ts
|
Code Review by Qodo
1. Retry staleness after CSV
|
eloycoto
requested changes
Mar 24, 2026
| return undefined; | ||
| } | ||
|
|
||
| let cancelled = false; |
Contributor
There was a problem hiding this comment.
could you explain this cancelled thing? I read the code multiple times, and I'm still not sure what it does to be honest, because if one is cancelled the behaviour is just weird, no? can we make this code a bit simpler and with a few comments?
Member
Author
There was a problem hiding this comment.
I can add more comments here. I agree this is not a simple reading, it requires non trivial knowledge of React, especially the part around refs.
The highlighted part is a corner case. If the component gets unmounted during the async flows, it will not set stale data.
Considering the component is on a scaffolder wizard page only, this is a rare case. Practically, it prevents transitive setting stale scaffolder secrets and annoying error logs in the console.
This hook is already result of an attempt to simplify the flow via removing and consolidating the logic. The RepoAuthentication component became more presentational this way.
It used to be a bit simpler with former just a single button "Try all again". But the recent multiple "Retry" look better to the user since we have the explicit list of providers to authenticate to.
Anyway, adding additional supported providers should be very simple and that's what is the most important probably. The tests seem to be comprehensive, so I would not be afraid of future extensions here.
|
eloycoto
approved these changes
Mar 26, 2026
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.
Fixes: FLPATH-3464
Within the CSV import flow, the RepoAuthentication widget shows summary of the required SCM provider tokens.
Screencast.From.2026-03-24.12-52-02.mp4