Skip to content

Conversation

@JoshuaVulcan
Copy link
Collaborator

What does this PR do?

  • Due to the necessity of the Vary: Authorization headers of our tile service responses, we must rely on custom code to ensure disk-level caching for our vector tile data.

How does it look

  • N/A

Relevant link(s)

Where / how to start reviewing (optional)

  • sw-custom

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements custom vector tile caching in the service worker to work around browser limitations with Authorization headers. The changes introduce user-scoped disk caching that respects Cache-Control headers while handling authentication requirements for tile requests.

Key changes:

  • Upgrades Workbox from v4 to v7 with updated API syntax
  • Implements user-scoped tile caching with scope hash tracking via postMessage
  • Adds cache invalidation based on Cache-Control max-age headers

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
src/sw-custom.js Core implementation of user-scoped tile caching with Workbox v7, including scope management, cache key generation, and Cache-Control header respect
src/App.js Sends user scope hash to service worker on login/logout to enable user-specific cache isolation
src/Map/index.js Removes outdated comment line

JoshuaVulcan and others added 2 commits October 22, 2025 12:16
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@luixlive luixlive left a comment

Choose a reason for hiding this comment

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

Good to go 🚀

Everything looks fine. I left a bunch of questions (some out of curiosity, some just food for thought to see if we can improve the code) and a few nits, but nothing to block it.


- name: Install dependencies
run: yarn --ignore-scripts
run: yarn install
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why are we doing this change? Note that yarn install and just yarn is the same, but I wonder if there's a reason behind removing --ignore-scripts flag.

export const fetchSubjectGroups = () => dispatch => axios.get(SUBJECT_GROUPS_API_URL)
.then(response => dispatch(fetchSubjectGroupsSuccess(response)));
.then(response => dispatch(fetchSubjectGroupsSuccess(response)))
.catch(_error => dispatch(fetchSubjectGroupsError())); // Fallback to empty array on error
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The method fetchSubjectGroupsError is a bit deceiving. Makes you feel there's a special handling for errors but all we do is call the "success" action creator with an empty array. We can modernize and keep the intention clearer like this:

export const fetchSubjectGroups = () => async (dispatch) => {
  let subjectGroups;
  try {
    subjectGroups = await axios.get(SUBJECT_GROUPS_API_URL);
  } catch (_error) {
    subjectGroups = [];
  }

  dispatch(fetchSubjectGroupsSuccess(subjectGroups));
};



// set user scope for service worker caching
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I know I already made you move this, sorry 😅 But I just thought it would be nice to clean a bit our App component by hiding some of these implementations behind a hook. Not urgent, not a blocker. But if you like the idea, it would be awesome to put this in a useSetServiceWorkerScope or something. That way we would simplify this component but also it would make logic incredibly easy to test! (Even for a Cursor agent jeje)

@JoshuaVulcan JoshuaVulcan merged commit 8f1cf60 into develop Oct 24, 2025
3 checks passed
@JoshuaVulcan JoshuaVulcan deleted the ERA-12142 branch October 24, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants