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

First pass at a session handler #1850

Merged
merged 18 commits into from
Nov 9, 2023
Merged

First pass at a session handler #1850

merged 18 commits into from
Nov 9, 2023

Conversation

estrattonbailey
Copy link
Member

No description provided.

@@ -35,7 +35,7 @@ type LegacySchema = {
description: string
avatar: string
}
onboarding: {
onboarding?: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Migration was failing bc we've removed these mobx models now. We should be careful going forward to make sure we can resume from a partial migration. So far so good, and in the next release with these changes, it will be a full migration, so probably not a big deal...

Comment on lines +12 to +13
// displayName: z.string().optional(),
// aviUrl: z.string().optional(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking that anything NOT returned from session response should be loaded with RQ

Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to have this information in logged out screens and in the account selector

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can fetch every time

handle: z.string().optional(),
displayName: z.string().optional(),
aviUrl: z.string().optional(),
handle: z.string(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Handle is returned from session, and the resumeSession method actually requires it in types but still works without it

Comment on lines +85 to +87
const next = persisted.get('onboarding').step
// TODO we've introduced a footgun
if (state.step !== next) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hehe this was cyclically updating in the background. Wonder if we can protect against this better at a higher level?

Copy link
Collaborator

Choose a reason for hiding this comment

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

mm that was on me, I think the only way to solve it is to avoid fires in response to this event -- the type: set needs a silent: true param

Comment on lines 349 to 356
await login({
service: serviceUrl,
identifier: fullIdent,
password,
})
await store.session.login({
service: serviceUrl,
identifier: fullIdent,
Copy link
Member Author

Choose a reason for hiding this comment

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

Just dual-booting rn

@@ -303,10 +307,11 @@ export const SettingsScreen = withAuthRequired(
accessibilityLabel={`Switch to ${account.handle}`}
accessibilityHint="Switches the account you are logged in to">
<View style={styles.avi}>
<UserAvatar size={40} avatar={account.aviUrl} />
{/*<UserAvatar size={40} avatar={account.aviUrl} />*/}
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to get RQ in here

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

This is looking like the right approach to me

Comment on lines +12 to +13
// displayName: z.string().optional(),
// aviUrl: z.string().optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We want to have this information in logged out screens and in the account selector

src/App.web.tsx Show resolved Hide resolved
[upsertAccount],
)

const logout = React.useCallback<ApiContext['logout']>(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should send the delete session request, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a method like this? I don't see one. Atm no, we just clear the tokens from memory and storage

Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +85 to +87
const next = persisted.get('onboarding').step
// TODO we've introduced a footgun
if (state.step !== next) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mm that was on me, I think the only way to solve it is to avoid fires in response to this event -- the type: set needs a silent: true param

Comment on lines +12 to +13
// displayName: z.string().optional(),
// aviUrl: z.string().optional(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can fetch every time

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

seems good overall

src/App.web.tsx Show resolved Hide resolved
src/state/session/index.tsx Outdated Show resolved Hide resolved
)

React.useEffect(() => {
persisted.write('session', {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note you probably don't want to do this after first render. So maybe:

  • Keep a wrapper that sets a dirty flag in a ref
  • In the Effect, if the dirty flag is true, persist and unset the flag.

* origin:
  Fix tab alignment on the web (#1857)
  Show tabs when swiping feeds (#1856)
  Sync top/bottom bar disappearance to the scroll (#1855)
  Hotfix internationalization on mobile (#1854)
  Internationalization & localization (#1822)
  Hide/show header and footer without re-renders, take two (#1849)
@estrattonbailey estrattonbailey marked this pull request as ready for review November 9, 2023 22:24
Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Going to go ahead and merge so others can build off this. More will come in followup PRs. Good work @estrattonbailey

@pfrazee pfrazee merged commit 625cbc4 into main Nov 9, 2023
4 checks passed
@pfrazee pfrazee deleted the eric/session-hook branch November 9, 2023 23:14
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.

3 participants