chore: Prefetch module apis in service worker#34003
Conversation
WalkthroughThe change involves extensive updates to various service worker utilities and configurations in the project. Key alterations include the addition of new constants, interfaces, and functions to manage service worker operations, updates to the page load API and service worker paths, and adaptations to the build configurations to accommodate file extension changes. Integration and unit tests were also updated to cover new functionalities and dependencies. Changes
Poem
Warning Review ran into problemsProblems (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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9392808373. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9393111316. |
|
Deploy-Preview-URL: https://ce-34003.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9395277246. |
|
Deploy-Preview-URL: https://ce-34003.dp.appsmith.com |
There was a problem hiding this comment.
TODO: Revert this file with new test cases.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9410863760. |
|
Deploy-Preview-URL: https://ce-34003.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9411847411. |
|
Deploy-Preview-URL: https://ce-34003.dp.appsmith.com |
dvj1988
force-pushed
the
chore/pre-fetch-modules-api
branch
from
931fae5 to
700da97
Compare
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9445780339. |
dvj1988
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts (1 hunks)
- app/client/src/ce/utils/serviceWorkerUtils.ts (1 hunks)
- app/client/src/serviceWorker.ts (3 hunks)
Additional context used
Biome
app/client/src/ce/utils/serviceWorkerUtils.ts
[error] 53-53: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 162-162: This constructor is unnecessary. (lint/complexity/noUselessConstructor)
Unsafe fix: Remove the unnecessary constructor.
[error] 29-29: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
Additional comments not posted (1)
app/client/src/serviceWorker.ts (1)
92-94: Verify route registration logic.Ensure that the route registration for HTML documents is correctly scoped and does not unintentionally capture unwanted requests.
Verification successful
The context around the
new Routeregistration shows that the route is scoped to handle requests wheresameOriginis true and the request destination is"document". This indicates that the route is specifically targeting HTML document requests and should not unintentionally capture other types of requests.
- The route registration logic appears to be correctly scoped to handle only HTML document requests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify route registration logic for HTML documents. # Test: Search for route registration logic. Expect: Correct scoping of routes. rg --type python $'new Route'Length of output: 61
Script:
#!/bin/bash # Description: Verify route registration logic for HTML documents. # Test: Search for route registration logic. Expect: Correct scoping of routes. rg --type ts 'new Route'Length of output: 101
Script:
#!/bin/bash # Description: Extract and review the context around the route registration logic for HTML documents. # Extract 10 lines before and after the match to review the context. rg --type ts -C 10 'new Route' app/client/src/serviceWorker.tsLength of output: 646
| describe("getRequestKey", () => { | ||
| it("should return the correct request key", () => { | ||
| const request = new Request("https://app.appsmith.com", { | ||
| method: "GET", | ||
| }); | ||
| request.headers.append("branchname", "main"); | ||
| const key = prefetchApiService.getRequestKey(request); | ||
| expect(key).toBe("GET:https://app.appsmith.com/:branchname:main"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("aqcuireFetchMutex", () => { | ||
| it("should acquire a new mutex if not present", async () => { | ||
| const request = new Request("https://app.appsmith.com", { | ||
| method: "GET", | ||
| }); | ||
| const acquireSpy = jest.spyOn(Mutex.prototype, "acquire"); | ||
| await prefetchApiService.aqcuireFetchMutex(request); | ||
| expect(acquireSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should reuse existing mutex if present", async () => { | ||
| const request = new Request("https://app.appsmith.com", { | ||
| method: "GET", | ||
| }); | ||
| const mutex = new Mutex(); | ||
| prefetchApiService.prefetchFetchMutexMap.set( | ||
| prefetchApiService.getRequestKey(request), | ||
| mutex, | ||
| ); | ||
| const acquireSpy = jest.spyOn(mutex, "acquire"); | ||
| await prefetchApiService.aqcuireFetchMutex(request); | ||
| expect(acquireSpy).toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Improve mutex handling in tests.
Refactor the mutex handling in tests to ensure that it is correctly tested and mocked:
- const acquireSpy = jest.spyOn(Mutex.prototype, "acquire");
- await prefetchApiService.aqcuireFetchMutex(request);
- expect(acquireSpy).toHaveBeenCalled();
+ const mutexMock = new Mutex();
+ jest.spyOn(prefetchApiService.prefetchFetchMutexMap, 'get').mockReturnValue(mutexMock);
+ const acquireSpy = jest.spyOn(mutexMock, "acquire");
+ await prefetchApiService.aqcuireFetchMutex(request);
+ expect(acquireSpy).toHaveBeenCalledTimes(1);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("getRequestKey", () => { | |
| it("should return the correct request key", () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| request.headers.append("branchname", "main"); | |
| const key = prefetchApiService.getRequestKey(request); | |
| expect(key).toBe("GET:https://app.appsmith.com/:branchname:main"); | |
| }); | |
| }); | |
| describe("aqcuireFetchMutex", () => { | |
| it("should acquire a new mutex if not present", async () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| const acquireSpy = jest.spyOn(Mutex.prototype, "acquire"); | |
| await prefetchApiService.aqcuireFetchMutex(request); | |
| expect(acquireSpy).toHaveBeenCalled(); | |
| }); | |
| it("should reuse existing mutex if present", async () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| const mutex = new Mutex(); | |
| prefetchApiService.prefetchFetchMutexMap.set( | |
| prefetchApiService.getRequestKey(request), | |
| mutex, | |
| ); | |
| const acquireSpy = jest.spyOn(mutex, "acquire"); | |
| await prefetchApiService.aqcuireFetchMutex(request); | |
| expect(acquireSpy).toHaveBeenCalled(); | |
| }); | |
| }); | |
| describe("getRequestKey", () => { | |
| it("should return the correct request key", () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| request.headers.append("branchname", "main"); | |
| const key = prefetchApiService.getRequestKey(request); | |
| expect(key).toBe("GET:https://app.appsmith.com/:branchname:main"); | |
| }); | |
| }); | |
| describe("aqcuireFetchMutex", () => { | |
| it("should acquire a new mutex if not present", async () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| const mutexMock = new Mutex(); | |
| jest.spyOn(prefetchApiService.prefetchFetchMutexMap, 'get').mockReturnValue(mutexMock); | |
| const acquireSpy = jest.spyOn(mutexMock, "acquire"); | |
| await prefetchApiService.aqcuireFetchMutex(request); | |
| expect(acquireSpy).toHaveBeenCalledTimes(1); | |
| }); | |
| it("should reuse existing mutex if present", async () => { | |
| const request = new Request("https://app.appsmith.com", { | |
| method: "GET", | |
| }); | |
| const mutex = new Mutex(); | |
| prefetchApiService.prefetchFetchMutexMap.set( | |
| prefetchApiService.getRequestKey(request), | |
| mutex, | |
| ); | |
| const acquireSpy = jest.spyOn(mutex, "acquire"); | |
| await prefetchApiService.aqcuireFetchMutex(request); | |
| expect(acquireSpy).toHaveBeenCalled(); | |
| }); | |
| }); |
| (global as any).fetch = jest.fn() as jest.Mock; | ||
| (global as any).caches = { | ||
| open: jest.fn(), | ||
| delete: jest.fn(), | ||
| }; |
There was a problem hiding this comment.
Mock global objects properly for isolation.
Ensure that global objects are mocked in a way that isolates tests from external changes:
- (global as any).fetch = jest.fn() as jest.Mock;
- (global as any).caches = {
- open: jest.fn(),
- delete: jest.fn(),
- };
+ jest.mock('node-fetch', () => ({
+ fetch: jest.fn(),
+ }));
+ jest.mock('caches', () => ({
+ open: jest.fn(),
+ delete: jest.fn(),
+ }));Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts (1 hunks)
- app/client/src/ce/utils/serviceWorkerUtils.ts (1 hunks)
- app/client/src/serviceWorker.ts (3 hunks)
Additional context used
Biome
app/client/src/ce/utils/serviceWorkerUtils.ts
[error] 53-53: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 162-162: This constructor is unnecessary. (lint/complexity/noUselessConstructor)
Unsafe fix: Remove the unnecessary constructor.
[error] 29-29: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
Additional comments not posted (5)
app/client/src/serviceWorker.ts (4)
10-14: Update import paths to reflect TypeScript conversion.
51-67: Enhanced prefetching logic for HTML route handling.Ensure that error handling is robust in the prefetching logic, as silent failures might lead to debugging difficulties.
92-94: Good use of route conditions to ensure that only document-type requests from the same origin are handled.
100-103: Efficient caching strategy for API requests.Consider logging or handling cases where the network fetch is necessary because the cache is missed.
app/client/src/ce/utils/serviceWorkerUtils.test.ts (1)
22-619: Comprehensive test coverage for new utility functions.Consider adding negative test cases for scenarios where exceptions might occur, to ensure robust error handling.
dvj1988
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9459326015. |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts (1 hunks)
- app/client/src/ce/utils/serviceWorkerUtils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/utils/serviceWorkerUtils.test.ts
Additional context used
Biome
app/client/src/ce/utils/serviceWorkerUtils.ts
[error] 52-52: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
[error] 161-161: This constructor is unnecessary. (lint/complexity/noUselessConstructor)
Unsafe fix: Remove the unnecessary constructor.
[error] 28-28: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
| /** | ||
| * returns the value in the query string for a key | ||
| */ | ||
| export const getSearchQuery = (search = "", key: string) => { |
There was a problem hiding this comment.
Adjust the default parameter to follow the last required parameter.
- export const getSearchQuery = (search = "", key: string) => {
+ export const getSearchQuery = (key: string, search = "") => {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getSearchQuery = (search = "", key: string) => { | |
| export const getSearchQuery = (key: string, search = "") => { |
Tools
Biome
[error] 52-52: This default parameter should follow the last required parameter or should be a required parameter. (lint/style/useDefaultParameterLast)
The last required parameter is here:
A default parameter that precedes a required parameter cannot be omitted at call site.
Unsafe fix: Turn the parameter into a required parameter.
| // Mutex to lock the fetch and cache operation | ||
| prefetchFetchMutexMap = new Map<string, Mutex>(); | ||
|
|
||
| constructor() {} |
There was a problem hiding this comment.
Remove the unnecessary constructor to simplify the class definition.
- constructor() {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor() {} |
Tools
Biome
[error] 161-161: This constructor is unnecessary. (lint/complexity/noUselessConstructor)
Unsafe fix: Remove the unnecessary constructor.
|
|
||
| type TApplicationParamsOrNull = TApplicationParams | null; | ||
|
|
||
| export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/"); |
There was a problem hiding this comment.
Consider using a regular expression literal for better performance and readability.
- export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/");
+ export const cachedApiUrlRegex = /\/api\/v1\/consolidated-api\//;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const cachedApiUrlRegex = new RegExp("/api/v1/consolidated-api/"); | |
| export const cachedApiUrlRegex = /\/api\/v1\/consolidated-api\//; |
Tools
Biome
[error] 28-28: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
dvj1988
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9499546811. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (1)
- app/client/package.json (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/package.json
dvj1988
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9512917544. |
|
Deploy-Preview-URL: https://ce-34003.dp.appsmith.com |
3 participants
Description
This PR has the following changes
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9511154598
Commit: 785197e
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
node-fetchto support new service worker functionalities.Refactor
viewandeditendpoint URL construction inConsolidatedPageLoadApifor better code maintainability.Chores
package.jsonfor better development experience and compatibility.