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.

Sign up for GitHub

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

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Attempt to fix issues seemingly caused by global loading state not being observed. #11392

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
704a31d
Attempt to fix issues seemingly caused by global loading state not be…
rtibbles Oct 11, 2023
ab65f72
Adds loaders to all class route page components or nearest relevant c…
rtibbles Oct 12, 2023
50e1295
Remove KCircularLoader to conform to loader standards.
rtibbles Oct 13, 2023
782aa60
Revert addition of loading state that waited for long timeout.
rtibbles Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion kolibri/plugins/coach/assets/src/views/CoachImmersivePage.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
:loading="loading"
:appearanceOverrides="appearanceOverrides"
>
<div class="coach-main">
<KCircularLoader
v-if="coreLoading"
Copy link
Member

@MisRob MisRob Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Some time ago, we attempted to move away from having global loading state being managed by these root pages but instead let it be managed by the child pages as such architecture showed up not being very flexible and on some cases preventing us from updating pages in a reasonable manner. I'm concerned that by introducing this again here will lead to same issues. I didn't dig into detail into all pages at this point, but here's one example where I see potential confusion https://github.com/learningequality/kolibri/blob/release-v0.16.x/kolibri/plugins/coach/assets/src/views/plan/QuizEditDetailsPage.vue#L93-L96

I didn't review in detail yet and can think about it more. Just looking at it very high-level, I think that this type of change may easily end up having unexpected impact and seems to go against the new pattern we've tried to adopt in the whole codebase to address problems caused by this approach. For now, I wanted to ask what was the reason for introducing it in this manner and if you've already considered alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

I know it'd be lots of work to do it from child pages. On the other hand, it seems that we'd need to have a look at them anyways to cleanup conflicting or overlapping loading states so that we don't run again into issues we've been cleaning up lately after the core base refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this because when the refactor happened to remove the global loading state from CoreBase, it wasn't then re-added to each individual page in coach, which for many of the pages were reliant on the global loading state preventing their components from being shown until state has been set.

As a result, all of these components make unguarded reads from their module state - or are not set up to reactively update if the module state changes while they are displayed. This is the cause of the bug that Peter has reported here.

I agree that we should be moving in the direction you describe, but this is a bandaid to account for the incomplete refactor, and I think not doing it is far more likely to leave additional regressions undiscovered until after release.

The code you are pointing to has not changed for 4 years (long predating the refactor) so I think the chance of this change causing issues for the loading state there is minimal.

Copy link
Member

Choose a reason for hiding this comment

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

I definitely agree it's caused by incomplete refactor and no problem to keep addressing this outside of the scope of this particular PR. My only suggestion would be to have an issue explaining why this change happened and in what direction we want to move on.

The code you are pointing to has not changed for 4 years (long predating the refactor) so I think the chance of this change causing issues for the loading state there is minimal.

I would rather say that we don't know, but one thing we can do to be more sure is to test loading states thoroughly.

One last high-level question, before I look into details, why is circular loader used rather than the linear one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only PRs that I am aware of that have made changes to the loading state since the refactor are this one: #11218 and this one: #11201

Neither of which affect pages that use the CoachImmersivePage, so I think the chance of a conflict between this change and previous loading state work. Unless I've missed a PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Member

@MisRob MisRob Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I can also raise the issue of global vs local loaders in a KDS tactical if that'd be helpful. I heard more devs "complaining" about it :)

Copy link
Member Author

@rtibbles rtibbles Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am fine with mostly doing away with global loaders (although I think having the linear loader consistently be turned on until route navigation has completed at least would be fairly easy to do globally, without having to mess around with anything in each route) - but getting rid of them completely would mean that we need to rearchitect all our pages to handle their loading states internally (which is probably best achieved by moving their loading state into setup in the component).

Copy link
Member

Choose a reason for hiding this comment

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

although I think having the linear loader consistently be turned on until route navigation has completed at least would be fairly easy to do globally, without having to mess around with anything in each route

Yes, I see value in this. Can't think of any issues in this particular case. I assume it should be pretty brief (after we deal with issues like #11219) so in that case it may even make more sense to stay with the subtler global linear loader.

Copy link
Member

Choose a reason for hiding this comment

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

By "it" I meant route navigation, if we stop doing API requests there

type="indeterminate"
:delay="true"
/>
<div v-else class="coach-main">
<slot></slot>
</div>
</ImmersivePage>
Expand Down Expand Up @@ -103,6 +108,7 @@
},
computed: {
...mapState({
coreLoading: state => state.core.loading,
error: state => state.core.error,
}),
},
Expand Down