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

Fix infinite loading on item pages and optimize menu resolver usage #3677

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alexandrevryghem
Copy link
Member

@alexandrevryghem alexandrevryghem commented Nov 20, 2024

References

Description

Fixed the issue where switching between item pages could cause an infinite loading screen. This issue was caused by requests made in dsoEditMenuResolver that set the useCachedVersionIfAvailable to false. The problem arose due to a change in the timing of when a request is actually sent to the backend. Since Angular 16, an asapScheduler causes requests to sometimes happen sooner. This broke this logic, which prevents cached data (when the cache is disabled) and stale data from being returned immediately when the service call is made.

The bitstream and relationship pages also had an additional issue that could cause an infinite loading screen. When navigating to these pages, you could sometimes see the error no elements in sequence in the console. This occurred when taking the first value of an observable that does not emit. This issue has been fixed.

Finally, the menu resolver was being resolved in too many places. It should only be resolved on pages that use the DsoEditMenuComponent. Therefore, it has been restricted to resolve only for the simple item page, the full item page, and the community and collection pages. All other places, such as the edit item/collection/community tabs and the browse tab, no longer need to resolve it. As a result, the edit item metadata tab, for example, now requires five fewer requests.

Instructions for Reviewers

List of changes in this PR:

  • Fixed the edge case where sending a request with useCachedVersionIfAvailable === false could cause an infinite loading screen:
    Previously, when a cached version was already present in the store, the RemoteDataBuildService#buildList would always first emit the cached value. This was then followed by a ResponsePending, which in turn was followed by the new response.
    Because we don't want to return the old cached value when useCachedVersionIfAvailable is false, a skipWhile was added to filter out all completed responses until a non-completed response was emitted.

    Due to recent changes, requests are sent earlier, which can cause the RemoteDataBuildService#buildList to already have the new completed response ready. The issue is that the skipWhile mistakenly interprets this as the old cached value (because no non-completed response was emitted), causing the observable to never emit.

    The fix is to update the skipWhile logic to not rely on the RemoteData's state to skip responses. Instead, it should compare the lastUpdated timestamp with the time before the request was sent. This ensures that all responses emitted by the RemoteDataBuildService#buildList are skipped unless they are new.

  • Fixed the no elements in sequence issue for the ItemBitstreamsComponent and ItemRelationshipsComponent. This was resolved by replacing the first() operators with take(1) to prevent such an error from being thrown.

  • Removed the dsoEditMenuResolver from the browse routes since the browse pages were separated from the community and collection pages in Refactored community & collection pages #2722, making the menu no longer necessary.

  • Moved the dsoEditMenuResolver to resolve only for simple/full item pages rather than for every item, collection, or community route. This prevents routes like edit metadata from resolving the menu.

  • Updated the correction types call in DSOEditMenuResolverService to always use the cache when such a request is present. However, if this was done to ensure it is re-fetched in certain situations, we should refactor it to mark the data as stale instead.

Guidance for how to test or review this PR:

  • Verify that switching between tabs does not accidentally cause an infinite load. The speed of switching between tabs should have no impact, as this bug is caused by how quickly a response is processed.
  • Verify that the DSO edit menus still work correctly and that tabs that don't use them no longer resolve them.

Checklist

  • My PR is created against the main branch of code (unless it is a backport or is fixing an issue specific to an older branch).
  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using npm run lint
  • My PR doesn't introduce circular dependencies (verified via npm run check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • My PR aligns with Accessibility guidelines if it makes changes to the user interface.
  • My PR uses i18n (internationalization) keys instead of hardcoded English text, to allow for translations.
  • My PR includes details on how to test it. I've provided clear instructions to reviewers on how to successfully test this fix or feature.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@alexandrevryghem alexandrevryghem added bug high priority performance / caching Related to performance, caching or embedded objects error handling How errors are handled from REST API claimed: Atmire Atmire team is working on this issue & will contribute back port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release labels Nov 20, 2024
@alexandrevryghem alexandrevryghem added this to the 9.0 milestone Nov 20, 2024
@alexandrevryghem alexandrevryghem self-assigned this Nov 20, 2024
…nuResolver on pages who use the DsoEditMenuComponent
@alexandrevryghem alexandrevryghem force-pushed the w2p-120109_fix-findByHref-and-findListByHref-skipping-their-response branch from 910459d to 5c9f494 Compare November 20, 2024 13:46
@alanorth
Copy link
Contributor

This is missing a port to dspace-7_x tag. Is that because is is related to Angular 16?

@alexandrevryghem
Copy link
Member Author

@alanorth: The BaseDataService fix can be backported to dspace-7_x without any negative side effects. I originally didn't include it because the issue doesn't occur there, as the change that causes the break isn't present yet. However, the menu resolver route optimization could be backported, and we could also include the BaseDataService fix. This way, they would all work consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug claimed: Atmire Atmire team is working on this issue & will contribute back error handling How errors are handled from REST API high priority performance / caching Related to performance, caching or embedded objects port to dspace-8_x This PR needs to be ported to `dspace-8_x` branch for next bug-fix release
Projects
Status: 🙋 Needs Reviewers Assigned
Development

Successfully merging this pull request may close these issues.

Infinite Load Times When Editing an Item or Browsing DSpace Item page sometimes fails to load
2 participants