Handle join_call links in webapp

[bgardner8008]

Summary

Problem: Call links (/call link) don't work when clicked from within webapp - join_call parameter is read once at initialization and only checked on channel changes. These links only work when pasted into browser or clicked externally, causing new page load. See Jira ticket for more explanation.

Ticket Link

https://mattermost.atlassian.net/browse/MM-67348

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.84%. Comparing base (03814a7) to head (6e6d5b8).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
webapp/src/index.tsx	0.00%	52 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
- Coverage   23.99%   23.84%   -0.16%     
==========================================
  Files         237      237              
  Lines       13532    13578      +46     
  Branches     1611     1619       +8     
==========================================
- Hits         3247     3237      -10     
- Misses       9978    10035      +57     
+ Partials      307      306       -1     

Files with missing lines	Coverage Δ
webapp/src/index.tsx	0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cpoile]

Thanks Bill! Just a couple questions:

• could we add tests (sorry, but important for this kind of thing I think)
• it's been awhile, so I'm not sure if it applies, but does this play nicely with Desktop?

[hmhealey]

I've got some concerns about this way of doing this that I laid out below. Most of them are more about doing things in a more React-friendly way that I think would be easier to maintain in the long run (more encapsulated, easier to test, etc), but they seem to clash with how Calls generally interacts with the Redux store in somewhat non-standard ways. Ideally, I think we'd change that, but that definitely balloons the scope of these changes quite a bit.

My comments on handleLinkClick are focused on more things that could affect the user or other parts of the app, but I'm not entirely certain if they're actually things that can occur or not.

Let me know if you'd like to chat about any of this since it might be easier to hash this out synchronously. I'd like to help figure out what the right approach here is to balance maybe improving some of this without bogging you down with it

[hmhealey]

I'm not a huge fan of this approach because of how this is fighting with the web app's routing so much. Having it be a separate click handler that takes precedence over other click handlers (because of the true argument at the end of addEventListener) means that this may interfere with places where we block click events like in permalink previews.

Also I'm not sure if this will actually happen, but the // If clicking link in same channel, prevent navigation and join directly block seems like it might break some other logic in the app because I think that might prevent the web app from redirecting from /TEAM/channels/CHANNEL_ID to /TEAM/channels/CHANNEL_NAME (which is also what causes the query parameter to be removed). I'm pretty sure there's other logic in the app which would expect the page URL to be /TEAM/channels/CHANNEL_NAME, so if that's prevented, it's possible that other logic might have issues

[bgardner8008]

Yes, it does seem like this should come from the react navigation rather than a click handler, but react was stripping the params.

[hmhealey]

I'm not sure if we care to support every case, but this means that if someone adds ?join_call=true to a "canonical" channel link (/TEAM/channels/CHANNEL_NAME) or the different URLs supported for DMs/GMs (like /TEAM/messages/@USER and some other hidden ones), then this won't support joining a call off those links

[bgardner8008]

Good points about the other possible call links, I was unaware.

[hmhealey]

I'm not the biggest fan of this approach since it

Broadly speaking, I think there might be a better approach here where we handle this by using registry.registerRootComponent to mount an invisible component which uses React Router's useLocation hook to watch for the query parameter to be added. That approach is a lot more self-contained, takes advantage of the React lifecycle, and has the nice benefit of handling both the initial load case and whenever the URL changes.

function JoinCallWatcher() {
    const location = useLocation();

    useEffect(() => {
        const params = new URLSearchParams(location.search);

        console.log('This is logged whenever anything in the URL changes', location, params);
    }, [location]);

    const joinCall = new URLSearchParams(location.search).get('join_call');
    useEffect(() => {
        console.log('This is logged on initial mount and whenever that parameter changes, but not on other URL changes', joinCall);
    }, [joinCall]);

    return null;
}

Ideally, you'd dispatch a Redux action at that point which could get the store state and see if it should join or change calls. The dispatched action could have access to the current channel ID and any of the other Calls plugin state like the channel of a current call. Even if we did that though, you'd still need to add handling for the channel ID not changing until after the join_call parameter is removed (like pendingJoinChannelId does currently in your code).

That may however be easier said than done with the way functions like connectToCall and a lot of ./utils is currently written in way that isn't Redux-friendly because they pass around the Redux store singleton we get from the web app instead of being written as Thunk actions which can access the store from React much more easily. I'd be glad to help look into doing that more, but I'm fully aware doing that might be breaking some of Calls' code conventions even if those aren't general best practices

[hmhealey]

Also, regarding the testing, encapsulating this in a component would make it easier to test this since we could interact with just that component and mock things like useLocation directly without worrying about the rest of this file

[bgardner8008]

This joinCallWatcher approach looks great to me, way less hacky than the PR. For this and the above comments, let me get up to speed a bit more (I'm a noob at react) and we can schedule a call to discuss later. Thanks.