Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove auth flag #3039

Merged
merged 4 commits into from
Feb 13, 2024
Merged

fix: Remove auth flag #3039

merged 4 commits into from
Feb 13, 2024

Conversation

fzavalia
Copy link
Contributor

@fzavalia fzavalia commented Feb 8, 2024

No description provided.

Copy link

vercel bot commented Feb 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
builder ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 13, 2024 8:57am

@coveralls
Copy link

coveralls commented Feb 8, 2024

Pull Request Test Coverage Report for Build 7883949999

Details

  • -5 of 7 (28.57%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 46.27%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/modules/land/utils.ts 0 1 0.0%
src/modules/identity/sagas.ts 2 6 33.33%
Files with Coverage Reduction New Missed Lines %
src/modules/identity/sagas.ts 3 19.33%
Totals Coverage Status
Change from base Build 7874179296: 0.06%
Covered Lines: 5267
Relevant Lines: 10301

💛 - Coveralls

Base automatically changed from fix/remove-sso to master February 8, 2024 13:46
yield put(loginRequest(providerType))
}

yield put(clearAssetPacks())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep this clearAssetPacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auth dapp flow returned before doing so, but it might have been a bug from before

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right! with the new auth dapp flow, this is no longer required because when the user is redirected again to the builder, the state is cleared

Copy link
Contributor Author

@fzavalia fzavalia Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the user might change account via the wallet, which event will end up calling the saga. If the changed wallet already has an identity, it should clear the assetpacks and whatever it did before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! this is the way 👍

@@ -173,7 +173,7 @@ export function locateNextLand(landTiles: Record<string, LandTile>, currentLandI
const nextIndex = (((index + 1) % landIds.length) + landIds.length) % landIds.length

const nextLandId = landIds[nextIndex]
return landTiles[nextLandId]!.land
return landTiles[nextLandId].land
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could landTiles[nextLandId] be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but the result would be the same with or without the !.
This change was required by the linter.

@fzavalia fzavalia merged commit c5702cb into master Feb 13, 2024
7 checks passed
@fzavalia fzavalia deleted the fix/remove-auth-flag branch February 13, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants