Check Tool: 'back' link takes user to referrer page , when arriving via deep link. #584
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
WalkthroughThe changes in this pull request primarily enhance the functionality of the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/controllers/deepLinkController.js (1)
37-39: Note HTTP spec uses "referer" spellingThe code correctly uses "referer" when checking headers (as per HTTP spec) but uses "referrer" when storing in session. Whilst this works, it might be worth maintaining consistency with either spelling throughout the codebase.
Consider using consistent spelling:
if (req.headers.referer) { - sessionData.referrer = req.headers.referer + sessionData.referer = req.headers.referer }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/controllers/deepLinkController.js (1 hunks)
- src/controllers/pageController.js (2 hunks)
- test/unit/PageController.test.js (1 hunks)
- test/unit/deepLinkController.test.js (1 hunks)
🔇 Additional comments (6)
src/controllers/pageController.js (1)
12-24: Verify coverage of all required navigation pathsThe implementation handles the upload method step as specified in the PR objectives. However, let's verify if all the required paths are covered.
✅ Verification successful
Based on the search results, I can now generate a final response as we have a clear picture of all the navigation paths in the application.
Back navigation implementation correctly covers the required paths
The implementation in
wizardBackLinkfunction appropriately handles the two key scenarios:
- For tree dataset: returns to referrer from
/check/geometry-type- For non-tree datasets: returns to referrer from
/check/upload-methodThe search results confirm these are the only two points where deep-link back navigation needs special handling, as they represent the diverging paths in the wizard flow:
- Tree dataset path:
/check/dataset->/check/geometry-type- Non-tree dataset path:
/check/dataset->/check/upload-methodThe implementation aligns with the PR objectives and covers all the required navigation paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential navigation paths in the codebase echo "Searching for route definitions and navigation paths..." rg -g '!node_modules' -g '*.{js,jsx}' "'/check/.*'" -A 5Length of output: 13658
test/unit/deepLinkController.test.js (2)
7-7: LGTM: Addition of headers object to mock requestThe addition of the empty headers object to the mock request is necessary to support testing the new referer header functionality.
Line range hint
28-52: Additional test cases needed for referer header functionalityThe test suite should include cases that verify the new referer header validation functionality. Consider adding test cases for:
- Request with valid referer header
- Request with missing referer header
- Request with invalid referer header format
Here's a script to verify the implementation of referer header handling in the controller:
src/controllers/deepLinkController.js (1)
36-40: Verify referrer usage across the applicationThe changes store the referrer in the session, but we should verify how this integrates with the rest of the application.
Let's check how this referrer value is used:
✅ Verification successful
Let me gather more specific information about how the deep link session key is accessed.
Based on the search results, I can now provide a final response about the referrer usage:
The referrer implementation is properly integrated within the application
The referrer storage and usage is well-structured:
- The referrer is stored in the session under the
check-tool-deep-linkkey indeepLinkController.js- It's retrieved in
pageController.jsalong with other deep link information- The value is used appropriately for back-link navigation, as evidenced by the test cases
The implementation follows a consistent pattern where:
deepLinkControllercaptures and stores the referrerpageControllerretrieves and uses it for navigation purposes- The functionality is properly tested in the test suite
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of referrer/referer in the codebase echo "Searching for referrer usage:" rg -i "referr?er" --type js # Specifically look for session access to this value echo "\nSearching for session access patterns:" ast-grep --pattern 'sessionModel.get($PATTERN)'Length of output: 1059
Script:
#!/bin/bash # Search for checkToolDeepLinkSessionKey usage and its value echo "Searching for deep link session key usage:" rg "checkToolDeepLinkSessionKey" -B 2 -A 2 # Look for sessionModel.get patterns specifically with the deep link key echo "\nSearching for deep link session access:" ast-grep --pattern 'sessionModel.get($KEY)' # Look for any cleanup or removal of the session data echo "\nSearching for session cleanup:" rg "sessionModel\.(unset|delete|remove|clear)" --type jsLength of output: 1851
test/unit/PageController.test.js (2)
58-85: 🛠️ Refactor suggestionConsider additional test cases for edge cases and security
Whilst the current test coverage is good, consider adding these scenarios:
- Invalid or malformed referrer URLs
- Security implications of using referrer headers
Add these test cases:
it('sanitises malformed referrer URLs', () => { const pageController = new PageController({ route: '/upload-method' }) const req = makeReq({ deepLinkData: { referrer: 'javascript:alert(1)' } }) pageController.locals(req, {}, vi.fn()) expect(req.form.options.lastPage).toBe(undefined) }) it('handles external referrer URLs securely', () => { const pageController = new PageController({ route: '/upload-method' }) const req = makeReq({ deepLinkData: { referrer: 'https://malicious-site.com' } }) pageController.locals(req, {}, vi.fn()) expect(req.form.options.lastPage).toBe(undefined) })Let's verify the current security measures:
#!/bin/bash # Description: Check for URL validation in the codebase # Expected: Find URL validation logic for referrer headers # Look for URL validation patterns rg -A 5 'referrer|referer'
43-86: Verify test coverage aligns with acceptance criteriaThe test suite effectively covers the core functionality, but let's verify it matches all acceptance criteria from issue #557.
✅ Verification successful
Let me gather more context about the acceptance criteria mentioned in issue #557 to properly verify the test coverage.
Based on the gathered context, I can now provide a final response as we have sufficient information about both the implementation and the acceptance criteria.
Test coverage adequately verifies acceptance criteria
The test suite effectively covers the acceptance criteria from issue #557. Specifically:
- The tests verify that clicking the back button on the 'upload method' step takes users back to their referrer page when deep linking is active
- The test cases cover all key scenarios:
- Arriving at the deep link entry point (verifies return to referrer)
- Arriving at other steps (verifies normal wizard flow)
- Handling cases with and without deep link session information
- Preserving existing back link behaviour when needed
The implementation in
pageController.jsaligns with the tests, showing proper handling of thewizardBackLinklogic for both deep linking and standard navigation paths.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify implementation matches acceptance criteria # Expected: Find related controller logic and route definitions # Look for upload method route definitions rg -A 5 "upload-method" # Look for back link logic in controllers ast-grep --pattern 'backLink = $$$'Length of output: 10344
Script:
#!/bin/bash # Look for issue #557 details gh issue view 557 # Look for wizard back link implementation rg -A 5 "wizardBackLink" # Look for deep link related code ast-grep --pattern 'deepLink'Length of output: 1842
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/controllers/deepLinkController.js (2 hunks)
- test/unit/PageController.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/PageController.test.js
🔇 Additional comments (1)
src/controllers/deepLinkController.js (1)
58-60: LGTM! Verify session data handlingThe session data handling changes look good. The integration with
maybeSetReferreris clean and maintains the existing flow.Let's verify the session data handling across the codebase:
✅ Verification successful
Let me gather more information about the
maybeSetReferrerfunction to ensure it's properly integrated.
Session data handling and referrer integration verified successfully
The session data handling is consistent across the codebase:
- The
checkToolDeepLinkSessionKeyis properly defined and used in bothpageController.jsanddeepLinkController.js- The
maybeSetReferrerfunction correctly validates and sets the referrer URL in the session data- The referrer is appropriately used in
wizardBackLinkfunction for navigation purposes- Test coverage confirms the expected behaviour
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent session data handling # Look for other places where checkToolDeepLinkSessionKey is used rg -A 3 "checkToolDeepLinkSessionKey"Length of output: 1307
Script:
#!/bin/bash # Check the implementation of maybeSetReferrer rg -A 5 "maybeSetReferrer" # Also check for any referrer-related session handling rg -A 3 "referrer"Length of output: 3456
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/controllers/pageController.js (1)
36-52: Consider enhancing error handling specificityThe error handling is good, but could be more specific about what went wrong. Consider adding more context to the log message.
Consider this enhancement:
} catch (e) { logger.warn('PageController.locals(): error setting back link', { type: types.App, - errorMessage: e.message + errorMessage: e.message, + currentUrl: req?.originalUrl, + sessionId: req?.sessionID, + hasDeepLinkInfo: !!req?.sessionModel?.get(this.checkToolDeepLinkSessionKey) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/controllers/pageController.js (2 hunks)
🔇 Additional comments (3)
src/controllers/pageController.js (3)
2-3: LGTM! Appropriate logging imports added.The addition of logging utilities will help with monitoring and debugging the back link functionality.
6-25: 🛠️ Refactor suggestionConsider additional edge cases and validation
Whilst the implementation handles the core functionality, there are a few areas for improvement:
- The function assumes
currentUrlwill always match exact strings - consider using URL parsing to handle potential query parameters or trailing slashes- No validation for the structure of
deepLinkInfobeyond checking for 'referrer'Consider this enhancement:
+const VALID_ROUTES = { + GEOMETRY_TYPE: '/check/geometry-type', + UPLOAD_METHOD: '/check/upload-method' +}; + function wizardBackLink (currentUrl, deepLinkInfo) { + // Normalise the URL by removing trailing slash and query params + const normalisedUrl = new URL(currentUrl, 'http://base').pathname.replace(/\/$/, '') + if (deepLinkInfo && 'referrer' in deepLinkInfo) { const { referrer, dataset } = deepLinkInfo - if (dataset === 'tree' && currentUrl === '/check/geometry-type') { + if (dataset === 'tree' && normalisedUrl === VALID_ROUTES.GEOMETRY_TYPE) { return referrer } - if (dataset !== 'tree' && currentUrl === '/check/upload-method') { + if (dataset !== 'tree' && normalisedUrl === VALID_ROUTES.UPLOAD_METHOD) { return referrer } } return undefined }Likely invalid or redundant comment.
38-41: Verify referrer handling across the applicationLet's ensure the referrer is being properly set and maintained across the application flow.
✅ Verification successful
Let me gather more specific information about the referrer handling implementation.
Referrer handling implementation is properly structured and verified
The implementation shows a well-structured approach to handling referrers:
- The
DeepLinkControllercorrectly captures and validates the referrer URL from request headers- The
wizardBackLinkfunction properly handles deep link scenarios for both tree and non-tree datasets- The referrer is appropriately stored in and retrieved from the session
- Test coverage confirms the expected behaviour for deep link entry points
The code changes align with the PR objective of fixing back navigation for deep-linked users, with proper referrer handling across the application flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all places where referrer might be set or used echo "Checking referrer usage across the codebase:" rg -l 'referrer' --type js echo "\nChecking deep link session key usage:" rg -l 'check-tool-deep-link' --type js echo "\nChecking related controller implementations:" rg -l 'DeepLinkController' --type jsLength of output: 607
Script:
#!/bin/bash # Check referrer handling implementation in PageController echo "Checking referrer handling in PageController:" rg -A 5 -B 5 'referrer' src/controllers/pageController.js echo "\nChecking referrer handling in DeepLinkController:" rg -A 5 -B 5 'referrer' src/controllers/deepLinkController.js echo "\nChecking wizardBackLink implementation:" ast-grep --pattern 'function wizardBackLink($$$) { $$$ }' echo "\nChecking related test cases:" rg -A 5 -B 5 'referrer' test/unit/PageController.test.jsLength of output: 3619
|
Correct solution would require a sane API from the wizard library; what it currently exposes is does not make things like this easy. I'm for not opening a ticket unless someone feels really strongly about this and argues for it in planning. |
|
👋 could we please make sure there's a product and/or design review prior to merging and pushing to production? What's the edge-case situation? |
What type of PR is this? (check all applicable)
Description
Sets the "back" link on page user arrives at via a deep link to the linked from page (using the
refererheader when available).Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Go to a task list of any dataset and click the "check service" link, for example
Then click the "Back" link. It should take you back to the task list page.
Also worth checking: the "Back" links on further steps do not take you to the task list page.
Note: there is a limitation/bug where going "back" from the upload type will always take you to the
/datasetstep. This is a limitation in the wizard library (this behaviour is already present in production).Added/updated tests?
Summary by CodeRabbit
New Features
Bug Fixes
Tests