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

feat(2627): Generating V2 invite links #2665

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

islathehut
Copy link
Collaborator

@islathehut islathehut commented Dec 5, 2024

Pull Request Checklist

  • I have linked this PR to a related GitHub issue.
  • I have added a description of the change (and Github issue number, if any) to the root CHANGELOG.md.

(Optional) Mobile checklist

Please ensure you completed the following checks if you did any changes to the mobile package:

  • I have run e2e tests for mobile
  • I have updated base screenshots for visual regression tests

@islathehut
Copy link
Collaborator Author

#2627

useEffect(() => {
if (invitationReady) {
LOGGER.info(`Generating invitation URL using generated LFA code`)
setInvitationLink(inviteLink)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm running into a case where opening the "Add Members" dialog leads to a warning that the invite link is invalid even though the connection manager should be reporting that it is valid.

2024-12-10T22:38:07.162Z INFO desktop:main:main Event: app.activate
2024-12-10T22:38:17.414Z INFO desktop:renderer:Invite Creating invite
2024-12-10T22:38:17.421Z INFO desktop:renderer:Invite Generating invite code
2024-12-10T22:38:17.422Z INFO state-manager:connection:invite:createInvite Creating LFA invite code
2024-12-10T22:38:17.422Z INFO state-manager:connection:invite:createInvite Getting existing long lived invite code
2024-12-10T22:38:17.422Z INFO state-manager:connection:invite:createInvite Validating existing long lived invite code
2024-12-10T22:38:17.424Z INFO desktop:renderer:Invite Done generating invite code
2024-12-10T22:38:17.426Z INFO backend:SocketService Validating long lived LFA invite with ID bUFPcCxDXW3F7yi or creating a new one
2024-12-10T22:38:17.426Z INFO backend:ConnectionsManagerService socketService - validateOrCreateLongLivedLfaInvite
2024-12-10T22:38:17.426Z INFO backend:auth:inviteService Validating LFA invite with ID bUFPcCxDXW3F7yi
2024-12-10T22:38:17.426Z INFO backend:ConnectionsManagerService Invite is a valid long lived LFA invite code!
2024-12-10T22:38:17.431Z INFO desktop:renderer:Invite Creating invite
2024-12-10T22:38:17.435Z INFO desktop:renderer:Invite Generating invitation URL using generated LFA code
2024-12-10T22:38:17.436Z INFO desktop:renderer:Invite Creating invite
2024-12-10T22:38:17.438Z WARN state-manager:connection:invite:createInvite Existing invalid was missing or undefined and we failed to generate a new one - your sig chain may not be configured!

LOGGER.info('Generating invite code')
dispatch(connection.actions.createInvite({}))
LOGGER.info('Done generating invite code')
setInvitationReady(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be misleading because the invitation code is not guaranteed to be done generating immediately after the dispatch right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like if it takes a while to validate or create the invite seed then we would just display an invite link without it which doesn't feel ideal to me. I think we should refactor to disable the copy to clipboard button while a selector validationInProgress is true and then set that selector to true when the saga starts and false after the invite seed is retrieved.


export const Invite: FC = () => {
const invitationLink = useSelector(connection.selectors.invitationUrl)
LOGGER.info('Creating invite')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a misleading log entry because the actual createInvite dispatch is in a useEffect that only runs when the component mounts. It should be removed in favor of letting the functions actually creating an invite log that

socket: Socket,
action: PayloadAction<ReturnType<typeof connectionActions.createInvite>['payload']>
): Generator {
logger.info('Creating LFA invite code')
Copy link
Collaborator

Choose a reason for hiding this comment

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

logging is a little excessive in this saga

yield* putResolve(connectionActions.setLongLivedInvite(lfaInviteData.newInvite))
} else if (!lfaInviteData?.valid && lfaInviteData?.newInvite == null) {
logger.warn(
`Existing invalid was missing or undefined and we failed to generate a new one - your sig chain may not be configured!`
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo?

Suggested change
`Existing invalid was missing or undefined and we failed to generate a new one - your sig chain may not be configured!`
`Existing invite was missing or undefined and we failed to generate a new one - your sig chain may not be configured!`

logger.info('Getting existing long lived invite code')
const existingLongLivedInvite: InviteResult | undefined = yield* select(connectionSelectors.longLivedInvite)
logger.info('Validating existing long lived invite code')
const lfaInviteData: { valid: boolean; newInvite?: InviteResult } | undefined = yield* apply(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const lfaInviteData: { valid: boolean; newInvite?: InviteResult } | undefined = yield* apply(
const lfaInviteData: { isValid: boolean; newInvite?: InviteResult } | undefined = yield* apply(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the actual response structure? Can we define a type to get linting here?

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.

2 participants