TypeError: Cannot read property 'set_active_channel' of undefined

[kollivier]

DESCRIPTION

In edit_channel/new_channel/views.js, one or more of the API calls to retrieve the channel list is failing, but we are not catching errors in those calls, so we only get this error later on when set_active_channel is called on the undefined channel list. We should probably first add error handlers to the promise calls to gather more information about the problem.

CATEGORY

BUG

DETAILS

https://sentry.io/learningequality/studio/issues/655655845/

TypeError: Cannot read property 'set_active_channel' of undefined
  at n.set_active_channel (/static/js/bundles/channel_edit-e04e5108459d75209c16.js:13:164565)
  at n.set_details (/static/js/bundles/channel_edit-e04e5108459d75209c16.js:13:165657)
  at n.toggle_panel (/static/js/bundles/channel_edit-e04e5108459d75209c16.js:13:165294)
  at z (/static/js/bundles/common-e04e5108459d75209c16.js:1:32734)
  at n.<anonymous> (/static/js/bundles/common-e04e5108459d75209c16.js:1:32936)
...
(8 additional frame(s) were not displayed)

[kollivier]

This report is also possibly related, as they're both triggering from the same page and deal with invalid data that could be caused by a failed initialization of the channel list.

Sentry issue: STUDIO-11

[jayoshih]

Sorry, I know exactly what this is. When you select a channel right after the page loads, some of the other lists the channel might be in might not have finished loading. It is essentially trying to read from a list that isn't there. I can look into this more

[kollivier]

Marking this high priority just because it is probably the most frequent error report we get on Sentry, and silencing it would significantly reduce the noise in the error reporter.

[jayoshih]

@kollivier Will look into this tomorrow- thanks for flagging!

[kollivier]

@jayoshih Actually, sorry, I never updated this ticket. This problem isn't totally resolved, but we significantly reduced the number of Sentry reports by checking if the channel lists have been propagated before calling methods on them.

There are still edge cases though, like if you load the channels page and click the New Channel button really quickly, you can get an error if the My Channels list is not yet loaded. In general, what we need is to improve the code to handle scenarios where the page is still fetching data it needs to render.

[kollivier]

PR for reference: #1070

[jayoshih]

Sounds good! Since the errors are slightly fewer, I'm thinking this would actually be a good place to use vue instead. If we can tell which channel is selected, we should be able to render it as selected easily with the state. It also works out well since the channel list is fairly self-contained

[kollivier]

Yes, I think Vue would be a good choice here (and possibly everywhere :)!

Note though that the problem here is that we were trying to call set_active_channel even before we had received the channel list from the API call. So the key is to have some sort of "loading channel list, please wait..." UI and only actually try to render the list once the various get_channel_list API calls have returned. I think it might be good to work out a design pattern for this case (i.e. view requires results of API call before it can render properly), as several views have similar issues.

[jayoshih]

Based on the implementation, there are three ways I can think of for handling this issue:

1. Have the user wait until the list is loaded. The reason the current implementation is failing is because a user might click a channel on the edit page for instance, but the public channel list hasn't loaded yet. The program tries to call set_active_channel, but all four lists need to be loaded for it to work. This is where my main hesitation is because the public channel list is only going to grow, which means the loading time could be quite substantial. This option is ideal, however, if we want to get something out fast.

2. Have a two-way calling system such that clicking a channel calls set_active_channel on all available lists, and lists check what the active channel is once they're loaded. I did something similar with the topic tree loading at a certain node, but the code got really complicated and hacky since I ended up having to store the current node on the list view and then check it from the list item view. This option is ideal if we want a balance between time and a good user experience.

3. Rewrite code in vue In a vue implementation, I could just set a condition in the template like :class={'active': isInNodePath}, so if a channel has been selected before the list has been loaded, it would be able to immediately check the state (vue has a two-way data binding system where the templates update when the data updates automatically). Out of the three options, however, this seems like the most time consuming. This option is ideal if we want to make something sustainable and simpler in the long-term.

I am slightly leaning towards the third strategy, but I understand that it is also valuable to get this out in a timely manner, so I'm open to the other two options

[kollivier]

I think it depends on how often we see this page changing.

If it isn't likely to change often, we can probably live with its issues for a while. In that case, doing 1 first to quiet Sentry some more, and then doing 3 when this view bubbles up to the top of the Vue TODO views :), probably makes the most sense.

If, on the other hand, we see this page getting touched by a lot of changes we're going to be making, we're probably better off biting the bullet and moving to Vue now, so that we can have a clean and solid foundation to work from. In this case I think doing the work upfront would ultimately save us time.

[jayoshih]

I think I might have some time next week or the following week to make the migration to vue!