chore: Don't duplicate the URL parsing Regexes #34788

sharat87 · 2024-07-08T11:03:50Z

This code duplication meant that we missed updating the regexes to the ones that support UUIDs, and so the caching functionality broke.

This PR removes the duplication and imports the regex constants from a single place.

/test sanity

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9850040595
Commit: d8261a1
Cypress dashboard.
Tags: @tag.Sanity
Spec:

Tue, 09 Jul 2024 03:00:41 UTC

Summary by CodeRabbit

Refactor
- Improved maintainability by importing path constants from a centralized location.
- Replaced hardcoded pageId and applicationId with variables for better code clarity and flexibility.

coderabbitai · 2024-07-08T11:03:58Z

Walkthrough

This update optimizes the serviceWorkerUtils.ts file by refactoring hardcoded path constants, replacing them with imports from @appsmith/constants/routes/appRoutes. It also replaces explicit values for pageId and applicationId with corresponding variables throughout the test suite, enhancing maintainability and readability.

Changes

File	Change Summary
`app/client/src/ce/utils/serviceWorkerUtils.ts`	Replaced hardcoded path constants with imports from `@appsmith/constants/routes/appRoutes`. Updated `pageId` and `applicationId` to be variable-based.

Poem

In the code of apps a change we weave,
With imports new, hardcodes leave.
Paths and IDs now with grace,
From constants all in their place.
Cleaner scripts, a future bright,
CodeRabbit hops with sheer delight!
🐰✨

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?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

dvj1988 · 2024-07-09T03:10:12Z

/build-deploy-preview skip-tests=true

github-actions · 2024-07-09T03:10:38Z

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9850343078.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 34788.
recreate: .

coderabbitai

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9766775 and d8261a1.

Files selected for processing (2)

app/client/src/ce/utils/serviceWorkerUtils.test.ts (12 hunks)
app/client/src/ce/utils/serviceWorkerUtils.ts (1 hunks)

Files skipped from review due to trivial changes (2)

app/client/src/ce/utils/serviceWorkerUtils.test.ts
app/client/src/ce/utils/serviceWorkerUtils.ts

dvj1988 · 2024-07-09T03:19:09Z

@sharat87 Can you also create a EE test PR also for these change? Last time when I tried importing the regexes from appRoutes there were some issues in the build. I would like to do a manual test before we merge.

github-actions · 2024-07-09T03:24:17Z

Deploy-Preview-URL: https://ce-34788.dp.appsmith.com

riodeuno · 2024-07-09T05:40:46Z

app/client/src/ce/utils/serviceWorkerUtils.ts

@@ -2,13 +2,14 @@ import { Mutex } from "async-mutex";
 import { APP_MODE } from "entities/App";


@dvj1988 @sharat87 Is it a good idea to tie the service worker utilities to our main app's code? I'm wondering if it effects the chucks or the build process? Basically, I might be wrong, but I believe, servicer worker registration and code in the wild seems to be separated from the main codebase.

@riodeuno, I suspected that too. I think it's quite likely as well. But from the code, it looks like we're mostly importing constant strings, both into application code and service worker code, so I think this is okay. I'd rather constants are imported from a single source for both. It'll mean they'll effectively be downloaded twice, once for application and once for service worker, but I think this is okay. Especially since we need the constant value in service worker code, so might as well import from a common source.

I don't know enough to be sure about the above though, so I'd invite more opinions on this. 🙏

Agreed.
We can run yarn analyze and see what it looks like.
Looking at it without analyzing it --
The only thing that comes to mind is that the serviceworker js file is created at build time, once generated there shouldn't be any ramifications on the chunks for our codebase. Based on this, this change is fine.

It could be that service worker utils are chunked along with our main app codebase. We're talking about ~250 LoC. The trade-off here is code structure that allows for better maintainability and fewer side effects vs saving the minified version of ~250 lines of unminified code from being downloaded in the chunk. I'm happy to trade in favour of better code structure.

@dvj1988 I too am looking for your opinion on this.

Actually, scratch that. I'm fairly certain that serviceworkerutils would not get into our main codebase chunks. As we would not be importing this utils file for anything in our codebase. Approving this PR.

I think the constants that are imported are duplicated in both the service worker bundle as well as the main bundle. There is a minor duplication of code in the bundle but as you guys have mentioned it better to have a single source of truth.

This is from https://ce-34788.dp.appsmith.com/pageService.js

This is from https://ce-34788.dp.appsmith.com/static/js/main.46d3445c.js

I believe the reason why we have duplication is because, the two bundles are generated as 2 different entry points in weback and the chunks generated in one entry point is not reused in another.

Also if you check the service worker we can see chunk details of the main bundle being injected into the service worker. So that means the service worker is bundled after the main bundle chunks are created.

sharat87 · 2024-07-09T07:29:52Z

Thanks @dvj1988 for the heads-up. Indeed there was a failure, and I fixed it in this EE PR: https://github.com/appsmithorg/appsmith-ee/pull/4565.

dvj1988 · 2024-07-09T08:52:08Z

Thanks @dvj1988 for the heads-up. Indeed there was a failure, and I fixed it in this EE PR: appsmithorg/appsmith-ee#4565.

Please give me time to manually test both ce and ee DPs today.

dvj1988

LGTM.

This code duplication meant that we missed updating the regexes to the ones that support UUIDs, and so the caching functionality broke. This PR removes the duplication and imports the regex constants from a single place. /test sanity  > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9850040595> > Commit: d8261a1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9850040595&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 09 Jul 2024 03:00:41 UTC   ## Summary by CodeRabbit - **Refactor** - Improved maintainability by importing path constants from a centralized location. - Replaced hardcoded `pageId` and `applicationId` with variables for better code clarity and flexibility.

chore: Don't duplicate the URL parsing Regexes

91a9fe0

github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Jul 8, 2024

chore: Fix tests

d8261a1

sharat87 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Jul 8, 2024

sharat87 marked this pull request as ready for review July 9, 2024 03:06

sharat87 requested review from dvj1988, riodeuno, hetunandu and abhvsn July 9, 2024 03:06

coderabbitai bot reviewed Jul 9, 2024

View reviewed changes

riodeuno reviewed Jul 9, 2024

View reviewed changes

sharat87 requested a review from riodeuno July 9, 2024 07:46

riodeuno approved these changes Jul 9, 2024

View reviewed changes

dvj1988 approved these changes Jul 9, 2024

View reviewed changes

sharat87 merged commit d64ddee into release Jul 9, 2024
73 of 75 checks passed

sharat87 deleted the sharat87-patch-4 branch July 9, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Don't duplicate the URL parsing Regexes #34788

chore: Don't duplicate the URL parsing Regexes #34788

sharat87 commented Jul 8, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

dvj1988 commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

coderabbitai bot left a comment

dvj1988 commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

riodeuno Jul 9, 2024 •

edited

Loading

sharat87 Jul 9, 2024

riodeuno Jul 9, 2024

riodeuno Jul 9, 2024 •

edited

Loading

dvj1988 Jul 9, 2024

sharat87 commented Jul 9, 2024

dvj1988 commented Jul 9, 2024 •

edited

Loading

dvj1988 left a comment

		@@ -2,13 +2,14 @@ import { Mutex } from "async-mutex";
		import { APP_MODE } from "entities/App";

chore: Don't duplicate the URL parsing Regexes #34788

chore: Don't duplicate the URL parsing Regexes #34788

Conversation

sharat87 commented Jul 8, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented Jul 8, 2024 • edited Loading

Walkthrough

Changes

Poem

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

dvj1988 commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

dvj1988 commented Jul 9, 2024

github-actions bot commented Jul 9, 2024

riodeuno Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

sharat87 Jul 9, 2024

Choose a reason for hiding this comment

riodeuno Jul 9, 2024

Choose a reason for hiding this comment

riodeuno Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

dvj1988 Jul 9, 2024

Choose a reason for hiding this comment

sharat87 commented Jul 9, 2024

dvj1988 commented Jul 9, 2024 • edited Loading

dvj1988 left a comment

Choose a reason for hiding this comment

sharat87 commented Jul 8, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jul 8, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

riodeuno Jul 9, 2024 •

edited

Loading

riodeuno Jul 9, 2024 •

edited

Loading

dvj1988 commented Jul 9, 2024 •

edited

Loading