tiagobento commented on code in PR #2087:
URL:
https://github.com/apache/incubator-kie-tools/pull/2087#discussion_r1434370671
##########
packages/online-editor/src/importFromUrl/NewWorkspaceFromUrlPage.tsx:
##########
@@ -96,6 +98,25 @@ export function NewWorkspaceFromUrlPage() {
);
const { clonableUrl, selectedGitRefName, gitServerRefsPromise } =
clonableUrlObject;
+ if (
+ gitServerRefsPromise.error?.toString().includes("Unauthorized") &&
+ isCertainlyGit(importableUrl.type) &&
+ importableUrl.url &&
+ !authInfo
+ ) {
+ const { compatible } = getCompatibleAuthSessionWithUrlDomain({
+ authProviders,
+ authSessions,
+ authSessionStatus,
+ urlDomain: new URL(importableUrl.url).hostname,
+ });
+ setAuthInfo({
+ username: (compatible as GitAuthSession[])[0].login,
+ uuid: (compatible as GitAuthSession[])[0].uuid,
+ password: (compatible as GitAuthSession[])[0].token,
+ });
+ }
Review Comment:
Using an `if` statement directly on the component's function implementation
is not something we really do, since it will potentially lead to very strange
behaviors, as this `if` statement will be executed every time
`NewWorkspaceFromUrlPage` re-renders.
That's one of the reasons why we have the general rule of always
encapsulating code inside hooks, like `useEffect`.
More than that, this code waits for an error to happen, checks for the error
string (which is very weak in terms of reliability) and tries to isolate a
scenario so that `authInfo` can be set.
Instead of trying to find a way to really "ignore" everything else going
around the fix, we always need to try and understand how the current code is
working, and how the current abstractions can be adapted so that the bug can be
fixed.
We have a very similar code to this one on the effect marked with `//
Startup the page. Only import if those are set.`.
##########
packages/online-editor/src/importFromUrl/NewWorkspaceFromUrlPage.tsx:
##########
@@ -76,7 +76,9 @@ export function NewWorkspaceFromUrlPage() {
const authProviders = useAuthProviders();
const { authSessions, authSessionStatus } = useAuthSessions();
- const { authSession, gitConfig, authInfo } =
useAuthSession(queryParamAuthSessionId);
+ const authSessionInfo = useAuthSession(queryParamAuthSessionId);
+ const { authSession, gitConfig } = authSessionInfo;
+ const [authInfo, setAuthInfo] = useState<AuthInfo |
undefined>(authSessionInfo.authInfo);
Review Comment:
Creating this state introduces an ambiguity, as `authInfo` is already
determined by the `queryParamAuthSessionId`. We should be setting
`queryParamAuthSessionId` correctly and re-rendering the page instead of
introducing a new state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
