Fix link modal not shown after access upgrade

[toger5]

We dont show the modal since there was a mistake in the isRoomJoinable function. It used a state variable instead of reacomputing the join rule with room.getJoinRule() The state is a captured variable that from before the link share click -> does not update at that time.

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[AndrewFerr]

I confirmed locally that this works.

Non-code requests:

• Could the commit message get a line break between after the first line? Without it, the commit title includes the description (and results in a very long title in git log).
• Could this be rebased on top of develop instead of merging with it? The merge makes the commit history a bit more confusing.

[toger5]

Could the commit message get a line break between after the first line? Without it, the commit title includes the description (and results in a very long title in git log).

That is interesting. I was displayed correctly in the github commit list. I tried with an actual empty newline inbetween... Maybe that works. Its still shown in my gitlog however. (Is there even a way to show only part of the message in git log)

Could this be rebased on top of develop instead of merging with it? The merge makes the commit history a bit more confusing.

Sure! We do still merge squash + merge in the react sdk? or did that change?

[dbkr]

aren't you now shadowing joinRule? If it's the same thing, shouldn't this be a callback that depends on the value of joinRule?

[AndrewFerr]

AIUI not shadowing the joinRule is what caused the bug this PR aims to fix.

[toger5]

yes we need this to be computed on the fly.
This method is called from a callback that gets created when we show the first modal (asking to change permission rules)

[dbkr]

right, but if it's a different variable, it should generally have a different name rather than shadowing a variable in a higher scope. Wouldn't having it as a dependency make react do the right thing?

[toger5]

right, but if it's a different variable, it should generally have a different name rather than shadowing a variable in a higher scope.

That is 100% correct. I see your point now.

I think having the stateVar here does not work because of this

    const shareClick = useCallback(() => {
        if (isRoomJoinable()) {
            showLinkModal();
        } else {
            // the room needs to be set to public or knock to generate a link
            Modal.createDialog(JoinRuleDialog, {
                room,
                // If the user cannot invite the Knocking is not given as an option.
                canInvite,
            }).finished.then(() => {
                if (isRoomJoinable()) showLinkModal();
            });
        }
    }, [isRoomJoinable, showLinkModal, room, canInvite]);

this line if (isRoomJoinable()) showLinkModal(); would not update the state variable even if it is a dependency since it would capture the variable while it is awaiting.

[dbkr]

Thank you :)