feat(composer): add an account switcher #9050
Conversation
SapphoSys
changed the title
SapphoSys
changed the title
|
holy shit |
src/view/com/composer/Composer.tsx
Outdated
| session.resumeSession({ | ||
| ...selectedAccount, | ||
| accessJwt: selectedAccount.accessJwt, | ||
| refreshJwt: selectedAccount.refreshJwt, | ||
| active: selectedAccount.active, | ||
| }) |
There was a problem hiding this comment.
session.resumeSession() is an async API call - not sure how I feel about doing this in a useMemo. Maybe useQuery might be more appropriate? We should probably await this
There was a problem hiding this comment.
yeah that's a side effect and should not be in useMemo
There was a problem hiding this comment.
@mozzius switched over to tanstack's useQuery if you wanna take a look :)
src/view/com/composer/Composer.tsx
Outdated
| // try a simple request to check if the session is valid | ||
| await tempAgent.getProfile({actor: account.did}) | ||
| // if it succeeds, update the selected account | ||
| setSelectedAccount(account) |
There was a problem hiding this comment.
if we use a query as mentioned above, maybe we could use setQueryData here to reuse the Agent directly so we don't need to resume session twice
There was a problem hiding this comment.
@mozzius done and done in the commits below
src/view/com/composer/Composer.tsx
Outdated
| {isWeb ? ( | ||
| <Menu.Root> | ||
| <Menu.Trigger label={_(msg`Switch account`)}> | ||
| {({props}) => ( | ||
| <Button | ||
| {...props} | ||
| disabled={otherAccounts.length === 0} |
There was a problem hiding this comment.
Would be much neater to just have a single component that is just platform split at the file level for this button+dialog
| createComposerState, | ||
| ) | ||
|
|
||
| const onSelectAccount = React.useCallback( |
There was a problem hiding this comment.
make sure to check what happens if the async code below is slowed down to trigger a race condition of resolving in the opposite order
There was a problem hiding this comment.
I quickly switched from account A to B to C, then back to A. Switching accounts has a (close to) 1 second delay (might be quicker with your connection, it varied for me), with subsequent switches being a lot faster
(this is with the useQuery & useQueryClient logic added in order to cache sessions)
I also decided to add a 1 second setTimeout promise delay just to check if the rapid fire switch requests would be out of order. I haven't noticed that being the case
There was a problem hiding this comment.
Though I could probably add a condition guard or something just in case... hm
@mozzius let me know if it'd be necessary here
There was a problem hiding this comment.
to clarify, i mean that race conditions are always possible — so it’s good to find a way to trigger one reliably (eg by simulating alternating delays) and then to write the code in a way that it’s resilient to the race
since resumeSession already has active it wouldn't hurt to put it here too
|
just wanted to say based |
| if (selectedAccount.did === currentAccount!.did) { | ||
| return defaultAgent | ||
| } | ||
| const session = new CredentialSession(new URL(selectedAccount.service)) |
There was a problem hiding this comment.
shouldn't this have a listener for session updates, any session expiry here is going to result in the session getting marked as invalid (due to stale refresh token) when you do another post (with that account), or actually switch to that account later
selectedAccount should probably be a DID, instead of passing the entire Account object in
There was a problem hiding this comment.
ya good point, i just quickly put things together to get the switcher working in the composer but now that i look at it its redundant
| // get fresh account data from session store | ||
| const account = accounts.find(acc => acc.did === accountDid) | ||
| if (!account) { | ||
| setError('Account not found') |
There was a problem hiding this comment.
i'm not 100% sure about the setError statement here tbh since the codebase only uses setError with either translated strings using msg, or to pass errors just as is (e.message, data.error, etc)
i originally wanted to use the throw statement, but wasn't sure it fit here. this feels a little hacky... hm
| currentAccount?.did, | ||
| currentAccount?.service, | ||
| currentAccount?.active, |
There was a problem hiding this comment.
is this necessary? queryClients get reset when you change account anyway
This pull request adds an account switcher to the post composer. Closes #6708 and closes #5531.
Technical details
When the user clicks on their user avatar, the account switcher is triggered. If there is only one account, the switcher is disabled.
It temporarily assumes an identity just for the composer and goes away if the modal is dismissed. The global site identity remains unaffected.
It uses the same strings as src/components/dialogs/SwitchAccount.tsx. The expired token logic is more or less the same as src/lib/hooks/useAccountSwitcher.ts, only that I temporarily spawn an agent beforehand to check if the identity is valid.
Feature tested on Web and Android. I don't have an iOS device at hand, so I was unable to test the changes there.
Screenshots
Web
Android
(long screenshot)