chore: make branchName a query param in consolidated api#38851
chore: make branchName a query param in consolidated api#38851
Conversation
|
Warning Rate limit exceeded@dvj1988 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a Changes
Possibly related PRs
Suggested Reviewers
Poem
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12985758035. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts (1)
4-20: Consider refactoring static-only class to functions.While the implementation is clean, consider refactoring to standalone functions for better maintainability:
-export class ConsolidatedApiUtils { - private static BASE_URL = "v1/consolidated-api"; - private static VIEW_URL = `${this.BASE_URL}/view`; - private static EDIT_URL = `${this.BASE_URL}/edit`; - - static getViewUrl = (requestParams: ConsolidatedApiParams) => { - const queryParams = getQueryStringfromObject(requestParams); - return `${this.VIEW_URL}${queryParams}`; - }; - - static getEditUrl = (requestParams: ConsolidatedApiParams) => { - const queryParams = getQueryStringfromObject(requestParams); - return `${this.EDIT_URL}${queryParams}`; - }; -} +const BASE_URL = "v1/consolidated-api"; +const VIEW_URL = `${BASE_URL}/view`; +const EDIT_URL = `${BASE_URL}/edit`; + +export const getViewUrl = (requestParams: ConsolidatedApiParams) => { + const queryParams = getQueryStringfromObject(requestParams); + return `${VIEW_URL}${queryParams}`; +}; + +export const getEditUrl = (requestParams: ConsolidatedApiParams) => { + const queryParams = getQueryStringfromObject(requestParams); + return `${EDIT_URL}${queryParams}`; +};🧰 Tools
🪛 Biome (1.9.4)
[error] 4-20: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/client/src/actions/pageActions.tsx(2 hunks)app/client/src/api/services/ConsolidatedPageLoadApi/api.ts(1 hunks)app/client/src/api/services/ConsolidatedPageLoadApi/types.ts(1 hunks)app/client/src/api/services/ConsolidatedPageLoadApi/url.ts(1 hunks)app/client/src/ce/sagas/PageSagas.tsx(2 hunks)app/client/src/ce/utils/serviceWorkerUtils.ts(2 hunks)app/client/src/pages/AppViewer/index.tsx(1 hunks)app/client/src/sagas/InitSagas.ts(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts
[error] 4-20: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (15)
app/client/src/api/services/ConsolidatedPageLoadApi/types.ts (1)
1-5: LGTM! Clean interface definition.The
ConsolidatedApiParamsinterface properly defines the optional parameters needed for the consolidated API.app/client/src/api/services/ConsolidatedPageLoadApi/api.ts (2)
6-11: LGTM! Clean implementation of view endpoint.The implementation properly uses the new parameter interface and URL utilities.
14-19: LGTM! Clean implementation of edit endpoint.The implementation mirrors the view endpoint pattern consistently.
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (2)
51-53: LGTM! Clean conversion of headers to query parameters.The edit endpoint properly moves headers to query parameters with appropriate default values.
84-86: LGTM! Consistent parameter handling in view endpoint.The view endpoint mirrors the edit endpoint's parameter handling pattern.
app/client/src/ce/utils/serviceWorkerUtils.ts (3)
13-13: LGTM! Good addition of ConsolidatedApiUtils import.The import aligns with the goal of centralizing URL construction logic.
114-120: LGTM! Clean refactor of edit mode URL construction.The change properly delegates URL construction to ConsolidatedApiUtils, removing direct URL manipulation.
127-133: LGTM! Clean refactor of view mode URL construction.Similar to edit mode, the change properly delegates URL construction to ConsolidatedApiUtils.
app/client/src/pages/AppViewer/index.tsx (1)
169-169: LGTM! Proper addition of branch parameter.The change correctly propagates the branch information to the resource fetching action.
app/client/src/sagas/InitSagas.ts (2)
224-224: LGTM! Clean interface update for branch support.The function signature is properly updated to include the optional branch parameter.
Also applies to: 232-233
238-238: LGTM! Proper integration of branch parameter.The branch parameter is correctly mapped to branchName in the params object.
app/client/src/actions/pageActions.tsx (2)
76-76: LGTM! Clean interface update.The interface is properly updated to include the branch property.
302-302: LGTM! Proper action creator update.The action creator correctly includes the branch parameter in its payload.
Also applies to: 309-309
app/client/src/ce/sagas/PageSagas.tsx (2)
155-155: LGTM!Clean import of required lodash utilities.
404-406: Verify branch parameter handling.The code correctly handles the branch parameter by:
- Destructuring it from the payload
- Using
pickByto conditionally include it in the params- Renaming it to
branchNamefor API consistencyHowever, we should verify that all callers of this saga are updated to handle the new branch parameter.
Failed server tests
|
dvj1988
force-pushed
the
chore/remove-branchname-header-consolidated-api
branch
from
edec5f9 to
029a844
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12987175265. |
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12987586768. |
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12988780319. |
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12989322601. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts (2)
5-8: Consider using a namespace or object with functions instead of a static-only class.While the implementation is clean, we can improve it by following TypeScript best practices.
Consider this alternative approach:
-export class ConsolidatedApiUtils { - private static BASE_URL = "v1/consolidated-api"; - private static VIEW_URL = `${this.BASE_URL}/view`; - private static EDIT_URL = `${this.BASE_URL}/edit`; +export const ConsolidatedApiUtils = { + BASE_URL: "v1/consolidated-api", + VIEW_URL: "v1/consolidated-api/view", + EDIT_URL: "v1/consolidated-api/edit",🧰 Tools
🪛 Biome (1.9.4)
[error] 5-25: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
10-24: Consider reducing code duplication in URL generation methods.Both methods share identical parameter handling logic.
Consider extracting the common logic:
+ private static createUrlWithParams(baseUrl: string, params: ConsolidatedApiParams) { + const queryParamsObject = pickBy(params, identity); + const queryParams = new URLSearchParams(queryParamsObject); + return `${baseUrl}?${queryParams}`; + } + - static getViewUrl = (requestParams: ConsolidatedApiParams) => { - // Remove undefined values from the requestParams object - const queryParamsObject = pickBy(requestParams, identity); - const queryParams = new URLSearchParams(queryParamsObject); - - return `${this.VIEW_URL}?${queryParams}`; - }; + static getViewUrl = (params: ConsolidatedApiParams) => { + return this.createUrlWithParams(this.VIEW_URL, params); + }; - static getEditUrl = (requestParams: ConsolidatedApiParams) => { - // Remove undefined values from the requestParams object - const queryParamsObject = pickBy(requestParams, identity); - const queryParams = new URLSearchParams(queryParamsObject); - - return `${this.EDIT_URL}?${queryParams}`; - }; + static getEditUrl = (params: ConsolidatedApiParams) => { + return this.createUrlWithParams(this.EDIT_URL, params); + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts(1 hunks)app/client/src/ce/utils/serviceWorkerUtils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/utils/serviceWorkerUtils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts
[error] 5-25: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts (1)
1-3: LGTM! Clean imports using specific lodash utilities.The imports are well-organized and use specific lodash utility imports instead of importing the entire library.
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts (2)
5-8: Consider converting static class to pure functions.The class contains only static members which can be simplified to pure functions for better maintainability and reduced complexity.
Consider this alternative implementation:
-export class ConsolidatedApiUtils { - private static BASE_URL = "v1/consolidated-api"; - private static VIEW_URL = `${this.BASE_URL}/view`; - private static EDIT_URL = `${this.BASE_URL}/edit`; +const BASE_URL = "v1/consolidated-api"; +const VIEW_URL = `${BASE_URL}/view`; +const EDIT_URL = `${BASE_URL}/edit`;🧰 Tools
🪛 Biome (1.9.4)
[error] 5-25: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
10-24: Extract common URL generation logic to reduce duplication.Both methods share identical parameter processing logic. Consider extracting it to a shared utility function.
Here's a suggested implementation:
+const generateUrlWithParams = (baseUrl: string, params: ConsolidatedApiParams) => { + const queryParamsObject = pickBy(params, identity); + const queryParamsString = new URLSearchParams(queryParamsObject).toString(); + return `${baseUrl}?${queryParamsString}`; +}; -static getViewUrl = (requestParams: ConsolidatedApiParams) => { - const queryParamsObject = pickBy(requestParams, identity); - const queryParamsString = new URLSearchParams(queryParamsObject).toString(); - return `${this.VIEW_URL}?${queryParamsString}`; -}; +static getViewUrl = (requestParams: ConsolidatedApiParams) => + generateUrlWithParams(this.VIEW_URL, requestParams); -static getEditUrl = (requestParams: ConsolidatedApiParams) => { - const queryParamsObject = pickBy(requestParams, identity); - const queryParamsString = new URLSearchParams(queryParamsObject).toString(); - return `${this.EDIT_URL}?${queryParamsString}`; -}; +static getEditUrl = (requestParams: ConsolidatedApiParams) => + generateUrlWithParams(this.EDIT_URL, requestParams);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/api/services/ConsolidatedPageLoadApi/types.ts(1 hunks)app/client/src/api/services/ConsolidatedPageLoadApi/url.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/api/services/ConsolidatedPageLoadApi/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts
[error] 5-25: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (2)
app/client/src/api/services/ConsolidatedPageLoadApi/url.ts (2)
1-3: LGTM! Good use of specific lodash imports.The selective imports from lodash will help with tree-shaking and bundle size optimization.
10-24: Headers are still being added through API interceptorsThe consolidated API endpoints are still using the core API instance which adds headers through interceptors.
#!/bin/bash # Check the API interceptor implementation rg -l "apiRequestInterceptor" app/client/src/api cat $(rg -l "apiRequestInterceptor" app/client/src/api)
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13005560927. |
…ader-consolidated-api
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/ce/utils/serviceWorkerUtils.test.ts (1)
473-482: Simplify request key test case.The test case can be made more concise by combining the URL and method declarations.
- const url = - "https://app.appsmith.com/api/v1/consolidated-api/edit?defaultPageId=page123&applicationId=app123&branchName=main"; - const method = "GET"; - const request = new Request(url, { - method, - }); + const request = new Request( + "https://app.appsmith.com/api/v1/consolidated-api/edit?defaultPageId=page123&applicationId=app123&branchName=main", + { method: "GET" } + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/ce/utils/serviceWorkerUtils.test.ts(6 hunks)app/client/src/ce/utils/serviceWorkerUtils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/utils/serviceWorkerUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (5)
app/client/src/ce/utils/serviceWorkerUtils.test.ts (5)
353-353: LGTM! URL construction for EDIT mode with applicationId.The test correctly verifies the consolidated API URL construction with branch name as a query parameter.
371-371: LGTM! URL construction for PUBLISHED mode with applicationId.The test correctly verifies the consolidated API URL construction with branch name as a query parameter.
388-388: LGTM! URL construction for EDIT mode without applicationId.The test correctly verifies the consolidated API URL construction with branch name as a query parameter.
405-405: LGTM! URL construction for PUBLISHED mode without applicationId.The test correctly verifies the consolidated API URL construction with branch name as a query parameter.
437-437: LGTM! URL construction in getPrefetchRequests.The test correctly verifies the consolidated API URL construction with branch name as a query parameter.
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13005767971. |
dvj1988
changed the title
dvj1988
changed the title
|
Deploy-Preview-URL: https://ce-38851.dp.appsmith.com |
|
Have you tried manually testing this? |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13007859503. |
@brayn003 Yes I have on both CE and EE. CE DP: https://ce-38851.dp.appsmith.com/ |
@brayn003 I am cancelling this run because the DP is already up to date with the latest changes. |
|
LGTM |
…#38851) ## Description - Move `branchName` header to `branchName` query param for consolidated api - Update the service worker prefetching logic to reflect the new url signature changes for consolidated api. ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13005671168> > Commit: 45b23c1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13005671168&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 28 Jan 2025 08:54:43 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added support for branch-specific data retrieval across multiple components. - Enhanced API endpoint configuration to dynamically handle branch parameters. - **Improvements** - Refactored URL generation and parameter handling for more flexible API calls. - Updated server-side controllers to support branch-related query parameters. - **Technical Updates** - Modified several API and saga functions to include optional branch information. - Improved type safety and parameter management in consolidated API services. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…#38851) ## Description - Move `branchName` header to `branchName` query param for consolidated api - Update the service worker prefetching logic to reflect the new url signature changes for consolidated api. ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13005671168> > Commit: 45b23c1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13005671168&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 28 Jan 2025 08:54:43 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added support for branch-specific data retrieval across multiple components. - Enhanced API endpoint configuration to dynamically handle branch parameters. - **Improvements** - Refactored URL generation and parameter handling for more flexible API calls. - Updated server-side controllers to support branch-related query parameters. - **Technical Updates** - Modified several API and saga functions to include optional branch information. - Improved type safety and parameter management in consolidated API services. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
branchNameheader tobranchNamequery param for consolidated apiAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13005671168
Commit: 45b23c1
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 28 Jan 2025 08:54:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates