Add OS Map Layer #758
Add OS Map Layer #758
Conversation
WalkthroughThis pull request introduces significant enhancements for managing Ordnance Survey (OS) map API tokens. The changes include a new service for token retrieval, modifications to the map initialization process to handle API tokens, and the addition of a new endpoint for token access. The build process is also updated to include static assets. Comprehensive unit tests have been added to ensure the functionality of the new features and the robustness of the token management system. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DilwoarH
force-pushed
the
753-use-os-base-layer-for-maps
branch
from
c30ff9a to
da21efa
Compare
DilwoarH
force-pushed
the
753-use-os-base-layer-for-maps
branch
from
da21efa to
c61b6f1
Compare
DilwoarH
force-pushed
the
753-use-os-base-layer-for-maps
branch
from
c61b6f1 to
53d6c28
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/services/osMapService.js (1)
1-5: Handle unsuccessful responses gracefully
If the fetch call encounters a non-OK status or parsing error,.json()may cause runtime exceptions. Consider checkingres.okor adding error handling to avoid unhandled promise rejections.src/assets/js/os-api-token.js (1)
21-36: Enhance error reporting for token fetch
Although you handle rejected fetch promises, also consider verifyingres.okbefore reading JSON. If multiple calls happen simultaneously, any callers aftermakingRequestis set to true wait for a single outcome. This is generally fine, but consider returning the same promise to avoid repeated fetch attempts.test/unit/services/osMapService.test.js (1)
1-45: Comprehensive test coverage
These tests cover success, network failure, and JSON parse errors. You may wish to add a case where the response has a non-OK status code but still returns JSON, to confirm correct error handling for a wider range of response types.src/routes/api.js (1)
42-48: Standardise HTTP error management
Returning a 500 for any retrieval error might be too broad. Consider refining response codes or structures to differentiate between various failure types.test/unit/javascript/os-api-token.test.js (1)
5-11: Consider using numeric types for token expiration fields
Currently,expires_inis stored as a string ('299'). Converting it to a numeric type helps avoid potential parsing or comparison issues if your token logic evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)src/assets/js/map.js(3 hunks)src/assets/js/os-api-token.js(1 hunks)src/routes/api.js(2 hunks)src/services/osMapService.js(1 hunks)test/unit/javascript/map.test.js(1 hunks)test/unit/javascript/os-api-token.test.js(1 hunks)test/unit/services/osMapService.test.js(1 hunks)
🔇 Additional comments (10)
src/assets/js/map.js (4)
3-3: Import module is valid
This import correctly references the new token management module.
34-34: Confirm availability of local style resource
Ensure that the JSON style file ('/public/static/OS_VTS_3857_3D.json') is hosted and accessible in all deployment environments, otherwise the map may fail to load.
37-37: Sensible default for interactivity
Using the nullish coalescing operator (opts.interactive ?? true) neatly sets a default interactive mode if none is provided.
38-53: Check 'key=null' approach
Appendingkey=nullmight cause issues if the OS endpoint strictly requires a real key. Verify that OS services accept this placeholder, or consider removing it. Also, considerresourceTypechecks if you only need transformations for specific request types (e.g. tiles).src/assets/js/os-api-token.js (1)
1-1: Verify licence compliance
This file is copied from another repository. Make certain the original licence terms permit this usage.src/routes/api.js (1)
4-4: New import structure
Successfully imports thegetOsMapAccessTokenfunction, aligning with the service-based architecture.test/unit/javascript/os-api-token.test.js (2)
23-32: Tests thoroughly cover fetch success cases
The tests forgetFreshApiTokencorrectly validate both success and error paths. No issues here.
64-75: Good approach to prevent multiple token refresh calls
Using a shared promise ensures that only one refresh attempt occurs, avoiding race conditions and redundant calls.test/unit/javascript/map.test.js (1)
33-35: Mock usage is correct and enables isolated testing
OverridinggetApiTokento return a mock token ensures the tests remain self-contained, which is beneficial for reliability.package.json (1)
18-18: Ensuring static assets are copied is a positive step
Includingcp -R src/assets/static/. public/staticin your build script ensures all static assets are bundled for deployment. Great attention to detail.
src/assets/js/map.js
Outdated
| // fetch initial token | ||
| await getApiToken() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling for token acquisition
If getApiToken() fails, the map creation flow will proceed without a valid token, potentially leading to subsequent request errors. Consider surrounding this call with a try/catch block or logging the error.
There was a problem hiding this comment.
would it be worth falling back to the maptiler base map in this instance?
There was a problem hiding this comment.
@GeorgeGoodall-GovUk Certainly! Introducing a fallback to a different base map (e.g., MapTiler) could help maintain map functionality even if the OS token acquisition fails. One approach is to wrap the token request in a try/catch. If it throws an error, you can catch it, change the map style to MapTiler’s tiles, log a warning, and continue. A simplified example might look like this:
try {
await getApiToken();
} catch (err) {
console.warn('Failed to fetch OS map token, fallback to MapTiler base map.', err);
// Replace the style with MapTiler base map or any other fallback style
options.mapStyle = 'https://api.maptiler.com/maps/basic/style.json?key=YOUR_MAPTILER_KEY';
}From there, you can pass that options.mapStyle to the maplibregl.Map constructor. This prevents the entire map from failing if the OS token is unavailable while still preserving as much user functionality as possible.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const API_ACCESS_TOKEN_URL = '/api/os/get-access-token' | ||
| let apiToken = { | ||
| access_token: '', | ||
| expires_in: 0, | ||
| issued_at: 0 | ||
| } | ||
|
|
||
| let makingRequest = false | ||
|
|
||
| export const getApiToken = () => { | ||
| const tokenCheckBuffer = 30 * 1000 | ||
| const tokenExpires = parseInt(apiToken.expires_in) * 1000 + parseInt(apiToken.issued_at) | ||
| if (Date.now() > tokenExpires - tokenCheckBuffer && !makingRequest) { | ||
| getFreshApiToken() | ||
| } | ||
| return apiToken.access_token | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Synchronous vs. asynchronous token refresh
getApiToken triggers a background refresh but returns the current token even if it is about to expire. Consider waiting for getFreshApiToken() if the expiry window is very short, to reduce the chance of using an expired token.
| url: url + '&srs=3857' | ||
| } | ||
|
|
||
| const token = getApiToken() |
There was a problem hiding this comment.
Please ensure the token is fetched by awaiting getFreshApiToken() somewhere in createMapFromServerContext() (or before it's called).
It sucks that the request transform function can't be async, so no straightforward way to refresh it here 😔
There was a problem hiding this comment.
We call await getApiToken() in the initialisation script - with calls both
There was a problem hiding this comment.
I know, unfortunately there's a race condition. getApiToken() is a synchronous function (which the initialisation code awaits; it should't). That means it returns a value immediately, possibly before the promise of getFreshApiToken() completes.
There was a problem hiding this comment.
Ah I see the problem - good catch, will update
This aligns with the map layer from the main website.
DilwoarH
force-pushed
the
753-use-os-base-layer-for-maps
branch
from
e79690a to
a747c05
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)src/assets/js/map.js(3 hunks)src/assets/js/os-api-token.js(1 hunks)src/routes/api.js(2 hunks)src/services/osMapService.js(1 hunks)test/unit/javascript/map.test.js(2 hunks)test/unit/javascript/os-api-token.test.js(1 hunks)test/unit/services/osMapService.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/services/osMapService.js
- src/routes/api.js
- test/unit/javascript/os-api-token.test.js
- package.json
- src/assets/js/os-api-token.js
- test/unit/services/osMapService.test.js
🧰 Additional context used
🪛 Gitleaks (8.21.2)
src/assets/js/map.js
414-414: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
test/unit/javascript/map.test.js
214-214: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
test/unit/javascript/map.test.js (1)
34-36: LGTM! Clean mock implementation.The mock implementation for
getApiTokenis clear and follows testing best practices.src/assets/js/map.js (2)
4-4: LGTM! Clean import statement.The import of
getApiTokenis clear and follows best practices.
41-56:⚠️ Potential issueUse async token fetch in transformRequest.
The synchronous call to
getApiTokenintransformRequestcould cause issues as the token might not be available immediately. Consider using an async approach or ensuring the token is always available before making requests.Apply this diff to handle the token more safely:
transformRequest: (url, resourceType) => { if (url.indexOf('api.os.uk') > -1) { if (!/[?&]key=/.test(url)) url += '?key=null' const requestToMake = { url: url + '&srs=3857' } - const token = getApiToken() + const token = this.token // Store token during initialisation requestToMake.headers = { Authorization: 'Bearer ' + token } return requestToMake } }Likely invalid or redundant comment.
Preview Link
https://submit-753-use-os-base--dwmoqo.herokuapp.com/organisations/local-authority:BNE/conservation-area/overview
What type of PR is this? (check all applicable)
Description
Please replace this line with a brief description of the changes made.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These changes improve the application's map functionality and authentication mechanism, ensuring more robust and efficient API interactions.