Bug: Improve back navigation logic in ItemComponent#5170
Bug: Improve back navigation logic in ItemComponent#5170milanmajchrak wants to merge 1 commit intoDSpace:mainfrom
Conversation
- Add session storage support to remember the previous URL when navigating to an item page - Implement generic session storage helper methods in RouteService for storing and retrieving URLs - Update navigation logic to fall back to session storage when the current route doesn't provide a valid previous URL - Add /home to allowed previous routes for back navigation - Add helper method pickAllowedPrevious for cleaner URL validation - Add tests for home URL back navigation and session fallback Co-authored-by: amadulhaxxani <amadulhaxxani@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves “Back to results” navigation on item pages by persisting an allowed previous URL in session storage, so the back button remains correct across refreshes and different navigation entry points (including /home).
Changes:
- Added generic session storage URL helpers to
RouteService. - Updated
ItemComponentto validate/store previous URLs and fall back to session storage when route history isn’t usable; expanded allowed previous routes to include/home. - Updated/added unit tests and stubs to cover the revised behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/core/services/route.service.ts | Adds sessionStorage helpers for storing/retrieving URLs. |
| src/app/item-page/simple/item-types/shared/item.component.ts | Stores/uses a validated previous URL with a session fallback; allows /home. |
| src/app/item-page/simple/item-types/shared/item.component.spec.ts | Adds tests for /home back navigation and “prefer route over stale session” behavior. |
| src/app/core/testing/route-service.stub.ts | Extends the RouteService stub with session helper methods for tests. |
| src/app/item-page/simple/item-types/publication/publication.component.spec.ts | Updates RouteService mocks to include new session helper methods. |
| src/app/item-page/simple/item-types/untyped-item/untyped-item.component.spec.ts | Updates RouteService mocks to include new session helper methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Store a URL in session storage for later retrieval | ||
| * Generic method that can be used by any component | ||
| * @param key The session storage key | ||
| * @param url The URL to store | ||
| */ | ||
| public storeUrlInSession(key: string, url: string): void { | ||
| if (typeof window !== 'undefined' && hasValue(window.sessionStorage)) { | ||
| // Only write if the value is different to avoid unnecessary writes | ||
| const currentValue = window.sessionStorage.getItem(key); | ||
| if (currentValue !== url) { | ||
| window.sessionStorage.setItem(key, url); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Retrieve a URL from session storage | ||
| * Generic method that can be used by any component | ||
| * @param key The session storage key | ||
| */ | ||
| public getUrlFromSession(key: string): string | null { | ||
| if (typeof window !== 'undefined' && hasValue(window.sessionStorage)) { | ||
| return window.sessionStorage.getItem(key); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
storeUrlInSession / getUrlFromSession add new public behavior to RouteService, but there are no unit tests covering them (there is an existing route.service.spec.ts covering other methods). Add tests for: successful write/read, no write when value unchanged, and graceful handling when sessionStorage is unavailable/throws.
| /** | ||
| * The function used to return to list from the item. | ||
| * Uses stored previous URL if available, otherwise falls back to browser history. | ||
| */ | ||
| back = () => { | ||
| this.routeService.getPreviousUrl().pipe( | ||
| take(1), | ||
| ).subscribe( | ||
| (url => { | ||
| this.router.navigateByUrl(url); | ||
| }), | ||
| ); | ||
| this.router.navigateByUrl(this.storedPreviousUrl); | ||
| }; |
There was a problem hiding this comment.
back() now always calls router.navigateByUrl(this.storedPreviousUrl), but storedPreviousUrl is only assigned inside the showBackButton$ observable. To avoid navigating with undefined (e.g., if back() is invoked before the observable emits), initialize storedPreviousUrl or make it optional and add a guard/fallback (e.g., browser history back). Also, the JSDoc says it falls back to browser history, but no such fallback is implemented—either implement it or update the comment to match the behavior.
| public storeUrlInSession(key: string, url: string): void { | ||
| if (typeof window !== 'undefined' && hasValue(window.sessionStorage)) { | ||
| // Only write if the value is different to avoid unnecessary writes | ||
| const currentValue = window.sessionStorage.getItem(key); | ||
| if (currentValue !== url) { | ||
| window.sessionStorage.setItem(key, url); | ||
| } | ||
| } |
There was a problem hiding this comment.
Accessing window.sessionStorage / getItem / setItem can throw (e.g., storage blocked, Safari private mode, quota exceeded). Wrap the sessionStorage interactions in a try/catch (and consider handling the case where even reading window.sessionStorage throws) so navigation logic can’t break the app at runtime.
| public getUrlFromSession(key: string): string | null { | ||
| if (typeof window !== 'undefined' && hasValue(window.sessionStorage)) { | ||
| return window.sessionStorage.getItem(key); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Same concern as storeUrlInSession: sessionStorage.getItem (or even accessing window.sessionStorage) may throw in some environments. Add a try/catch and return null on failure to keep this helper safe to call from components.
| clearUrlFromSession: (key: string) => { | ||
| // no-op for tests | ||
| }, |
There was a problem hiding this comment.
clearUrlFromSession is present in the testing stub but there’s no corresponding method on RouteService (and it isn’t used anywhere else). Consider removing it to keep the stub aligned with the real service surface area and reduce confusion.
| clearUrlFromSession: (key: string) => { | |
| // no-op for tests | |
| }, |
|
QA testing performed (no code review). Works as expected:
|
alegontarz
moved this from 🙋 Needs Reviewers Assigned
to 👀 Under Review
in DSpace 10.0 Release
References
Original author: @amadulhaxxani
Description
Improves the "Back to results" button logic on item pages by adding session storage as a fallback for the previous URL. This ensures the back button works correctly across page refreshes and prevents stale navigation when users visit items from different contexts (e.g. homepage vs. collection browse).
Instructions for Reviewers
List of changes in this PR:
storeUrlInSession()andgetUrlFromSession()generic helper methods toRouteServicefor persisting URLs in session storageItemComponentto store the previous URL in session storage and fall back to it when the route-based previous URL is not a valid navigation target/hometo the list of allowed previous routes so the back button appears when navigating from the homepagepickAllowedPrevious()helper method for cleaner logicback()method to use the stored URL directly instead of re-subscribing to the observableroute-service.stub.ts,publication.component.spec.ts,untyped-item.component.spec.ts, anditem.component.spec.tsHow to test:
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.