[NOISSUE] remove host from the new rovodev login flow #1547
Conversation
sdzh-atlassian
requested review from
BHulovatyi,
Blastoplex,
amarg-at,
bwieger-atlassian-com,
cabella-dot,
ccallcottstevens,
dchiew-atl,
jwang19-atlassian,
marcomura,
matt-lassian,
mattcolman,
om-ukr,
sky-ocean-spirits and
teg-atlassian
as code owners
![atlassian[bot]](https://avatars.githubusercontent.com/in/14320?s=60&v=4){kind=small}
src/rovo-dev/rovoDevAuthValidator.ts
Outdated
| * and extract the cloud ID from the first one. | ||
| */ | ||
| async function fetchCloudId(host: string): Promise<string> { | ||
| async function fetchCloudId(email: string): Promise<string> { |
There was a problem hiding this comment.
🔎 Code Readability - Unused Variable
The email parameter is unused in this function and should be removed.
Details
📖 Explanation: The email parameter is passed to fetchCloudId but never used since the function only returns a hardcoded placeholder value. This creates dead code and misleading function signature.
|
How do we support the multiple site dis-ambiguation? |
| // Use a placeholder host since RovoDev doesn't require it | ||
| return { | ||
| host: rovoDevAuth.host, | ||
| host: 'unused-rovodev-host.atlassian.net', |
There was a problem hiding this comment.
? - This feels sketch
src/rovo-dev/rovoDevAuthValidator.ts
Outdated
| return data.cloudId; | ||
| // Return a placeholder cloud ID | ||
| // The actual cloud ID will be determined by the backend when needed | ||
| return 'rovodev-placeholder-cloudid'; |
There was a problem hiding this comment.
trying a return of a constant seems unusual
cabella-dot
force-pushed
the
NOISSUE-remove-host-from-login-form
branch
from
ef6dcfd to
c07b006
Compare
| if (newState.state === 'Downloading' || newState.state === 'Starting' || newState.state === 'Started') { | ||
| this._userInfo = newState.jiraSiteUserInfo; | ||
| this._jiraItemsProvider.setJiraSite(newState.jiraSiteHostname); | ||
| // this._jiraItemsProvider.setJiraSite(newState.jiraSiteHostname); |
There was a problem hiding this comment.
@sdzh-atlassian without host, we need to rethink how the issue provider works
| if (!creds) { | ||
| return undefined; | ||
| } | ||
| return { |
There was a problem hiding this comment.
🔎 Code Readability - Code Duplication
The duplicate credential conversion logic should be extracted into a helper function to reduce code duplication.
Details
📖 Explanation: The same credential mapping logic appears twice in the getCredentials method, which violates the DRY principle and makes maintenance harder.
What Is This Change?
Cleaning up
hostfrom the new rovodev authentication processImportant caveat - this also removes the
--site-url {siteUrl}flag from rovodevProcessManagerHow Has This Been Tested?
Manually, see demo:
https://www.loom.com/share/dbc0bdd7e3a94d378578a3e1fd86dab1
Basic checks:
npm run lintnpm run testAny suggestions or improvements have been posted as pull request comments.