chore: add etag in consolidated api controller #38873
Conversation
WalkthroughThis pull request introduces significant changes to Appsmith's caching strategies, service worker configuration, API response handling, and tracing setup. Key modifications include a shift from pre-caching to a more streamlined caching approach in the service worker, the addition of ETag support for API responses, and updates to the OpenTelemetry tracing endpoint. These changes collectively enhance the application's performance and observability. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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/13009113652. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13009215068. |
|
Deploy-Preview-URL: https://ce-38873.dp.appsmith.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/server/appsmith-server/src/main/resources/application.properties (2)
91-91: Document the New Relic integration setup.Consider adding a comment above this property to document:
- The purpose of the New Relic integration
- Required environment variables
- Protocol details (gRPC)
+ # New Relic OTLP/gRPC exporter endpoint for distributed tracing management.otlp.tracing.endpoint=${APPSMITH_NEW_RELIC_OTEL_EXPORTER_OTLP_ENDPOINT:https://otlp.nr-data.net:4317}/v1/traces
91-91: Review security implications of external OTLP endpoint.The configuration exposes tracing data to an external endpoint. Ensure:
- The endpoint URL is configurable per environment
- Proper authentication is in place
- Sensitive data is filtered from traces
app/client/src/serviceWorker.ts (2)
12-13: Consider removing unused manifest variable.
IfwbManifestis no longer referenced, removing it may simplify the code.
15-16: Handle promise fromcaches.delete().
Currently, the deletion result is ignored; consider awaiting it or adding a log to confirm cache removal success.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (1)
107-137: ETag logic successfully implemented.
The measured computation time is logged, and conditional checks forIf-None-Matchappear correct. Keep an eye on potential performance impacts for large response payloads.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3)
648-702: Consider making ObjectMapper a singleton field for better performance.The ObjectMapper instance is created for each method call. Consider making it a class-level field to avoid repeated instantiation.
+ private final ObjectMapper objectMapper; public ConsolidatedAPIServiceCEImpl(...) { + this.objectMapper = new ObjectMapper(); + this.objectMapper.registerModule(new JavaTimeModule()); } @NotNull public String computeConsolidatedAPIResponseEtag(...) { - ObjectMapper objectMapper = new ObjectMapper(); - objectMapper.registerModule(new JavaTimeModule());Otherwise, the implementation looks good with proper error handling, strong hashing, and tracing integration.
648-702: Consider enhancing the ETag computation implementation.While the implementation is functionally correct, consider these improvements:
- Extract empty string literals to constants
- Use Optional to simplify null checks
- Use a Map builder instead of Map.of for better maintainability
+ private static final String EMPTY_ETAG = ""; + private static final String HASH_ALGORITHM = "SHA-256"; @NotNull public String computeConsolidatedAPIResponseEtag( ConsolidatedAPIResponseDTO consolidatedAPIResponseDTO, String defaultPageId, String applicationId) { if (isBlank(defaultPageId) && isBlank(applicationId)) { - return ""; + return EMPTY_ETAG; } Span computeEtagSpan = observationHelper.createSpan(ETAG_SPAN).start(); try { - String lastDeployedAt = consolidatedAPIResponseDTO.getPages() != null - ? consolidatedAPIResponseDTO - .getPages() - .getData() - .getApplication() - .getLastDeployedAt() - .toString() - : null; + String lastDeployedAt = Optional.ofNullable(consolidatedAPIResponseDTO.getPages()) + .map(pages -> pages.getData()) + .map(data -> data.getApplication()) + .map(app -> app.getLastDeployedAt()) + .map(Object::toString) + .orElse(null); if (lastDeployedAt == null) { - return ""; + return EMPTY_ETAG; } - Object currentTheme = consolidatedAPIResponseDTO.getCurrentTheme() != null - ? consolidatedAPIResponseDTO.getCurrentTheme() - : ""; - Object themes = consolidatedAPIResponseDTO.getThemes() != null - ? consolidatedAPIResponseDTO.getThemes() - : Collections.emptyList(); + Object currentTheme = Optional.ofNullable(consolidatedAPIResponseDTO.getCurrentTheme()) + .orElse(EMPTY_ETAG); + Object themes = Optional.ofNullable(consolidatedAPIResponseDTO.getThemes()) + .orElse(Collections.emptyList()); - Map<String, Object> consolidateAPISignature = Map.of( - "userProfile", consolidatedAPIResponseDTO.getUserProfile(), - "featureFlags", consolidatedAPIResponseDTO.getFeatureFlags(), - "tenantConfig", consolidatedAPIResponseDTO.getTenantConfig(), - "productAlert", consolidatedAPIResponseDTO.getProductAlert(), - "currentTheme", currentTheme, - "themes", themes, - "lastDeployedAt", lastDeployedAt); + Map<String, Object> consolidateAPISignature = new HashMap<>(); + consolidateAPISignature.put("userProfile", consolidatedAPIResponseDTO.getUserProfile()); + consolidateAPISignature.put("featureFlags", consolidatedAPIResponseDTO.getFeatureFlags()); + consolidateAPISignature.put("tenantConfig", consolidatedAPIResponseDTO.getTenantConfig()); + consolidateAPISignature.put("productAlert", consolidatedAPIResponseDTO.getProductAlert()); + consolidateAPISignature.put("currentTheme", currentTheme); + consolidateAPISignature.put("themes", themes); + consolidateAPISignature.put("lastDeployedAt", lastDeployedAt); ObjectMapper objectMapper = new ObjectMapper(); objectMapper.registerModule(new JavaTimeModule()); String consolidateAPISignatureJSON = objectMapper.writeValueAsString(consolidateAPISignature); - MessageDigest digest = MessageDigest.getInstance("SHA-256"); + MessageDigest digest = MessageDigest.getInstance(HASH_ALGORITHM); byte[] hashBytes = digest.digest(consolidateAPISignatureJSON.getBytes(StandardCharsets.UTF_8)); String etag = Base64.getEncoder().encodeToString(hashBytes); return etag; } catch (Exception e) { log.error("Error while computing etag for ConsolidatedAPIResponseDTO", e); - return ""; + return EMPTY_ETAG; } finally { observationHelper.endSpan(computeEtagSpan, true); } }
648-702: LGTM! Well-implemented ETag computation with proper error handling.The implementation includes proper validation, error handling, and comprehensive object mapping for ETag computation.
Consider optimizing object creation and string operations.
The current implementation creates new objects for each ETag computation. Consider:
- Making the ObjectMapper a static final field
- Using a StringBuilder for string concatenation
+ private static final ObjectMapper objectMapper; + + static { + objectMapper = new ObjectMapper(); + objectMapper.registerModule(new JavaTimeModule()); + } @NotNull public String computeConsolidatedAPIResponseEtag( ConsolidatedAPIResponseDTO consolidatedAPIResponseDTO, String defaultPageId, String applicationId) { - ObjectMapper objectMapper = new ObjectMapper(); - objectMapper.registerModule(new JavaTimeModule());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/client/craco.build.config.js(2 hunks)app/client/src/serviceWorker.ts(2 hunks)app/client/start-https.sh(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ConsolidatedApiSpanNamesCE.java(1 hunks)app/server/appsmith-server/pom.xml(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java(3 hunks)app/server/appsmith-server/src/main/resources/application.properties(1 hunks)deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (61)
- GitHub Check: perform-test / ci-test / ci-test (59)
- GitHub Check: perform-test / ci-test / ci-test (58)
- GitHub Check: perform-test / ci-test / ci-test (57)
- GitHub Check: perform-test / ci-test / ci-test (56)
- GitHub Check: perform-test / ci-test / ci-test (55)
- GitHub Check: perform-test / ci-test / ci-test (54)
- GitHub Check: perform-test / ci-test / ci-test (53)
- GitHub Check: perform-test / ci-test / ci-test (52)
- GitHub Check: perform-test / ci-test / ci-test (51)
- GitHub Check: perform-test / ci-test / ci-test (50)
- GitHub Check: perform-test / ci-test / ci-test (49)
- GitHub Check: perform-test / ci-test / ci-test (48)
- GitHub Check: perform-test / ci-test / ci-test (47)
- GitHub Check: perform-test / ci-test / ci-test (46)
- GitHub Check: perform-test / ci-test / ci-test (45)
- GitHub Check: perform-test / ci-test / ci-test (44)
- GitHub Check: perform-test / ci-test / ci-test (43)
- GitHub Check: perform-test / ci-test / ci-test (42)
- GitHub Check: perform-test / ci-test / ci-test (41)
- GitHub Check: perform-test / ci-test / ci-test (40)
- GitHub Check: perform-test / ci-test / ci-test (39)
- GitHub Check: perform-test / ci-test / ci-test (38)
- GitHub Check: perform-test / ci-test / ci-test (37)
- GitHub Check: perform-test / ci-test / ci-test (36)
- GitHub Check: perform-test / ci-test / ci-test (35)
- GitHub Check: perform-test / ci-test / ci-test (34)
- GitHub Check: perform-test / ci-test / ci-test (33)
- GitHub Check: perform-test / ci-test / ci-test (32)
- GitHub Check: perform-test / ci-test / ci-test (31)
- GitHub Check: perform-test / ci-test / ci-test (30)
- GitHub Check: perform-test / ci-test / ci-test (29)
- GitHub Check: perform-test / ci-test / ci-test (28)
- GitHub Check: perform-test / ci-test / ci-test (27)
- GitHub Check: perform-test / ci-test / ci-test (26)
- GitHub Check: perform-test / ci-test / ci-test (25)
- GitHub Check: perform-test / ci-test / ci-test (24)
- GitHub Check: perform-test / ci-test / ci-test (23)
- GitHub Check: perform-test / ci-test / ci-test (22)
- GitHub Check: perform-test / ci-test / ci-test (21)
- GitHub Check: perform-test / ci-test / ci-test (20)
- GitHub Check: perform-test / ci-test / ci-test (19)
- GitHub Check: perform-test / ci-test / ci-test (18)
- GitHub Check: perform-test / ci-test / ci-test (17)
- GitHub Check: perform-test / ci-test / ci-test (16)
- GitHub Check: perform-test / ci-test / ci-test (15)
- GitHub Check: perform-test / ci-test / ci-test (14)
- GitHub Check: perform-test / ci-test / ci-test (13)
- GitHub Check: perform-test / ci-test / ci-test (12)
- GitHub Check: perform-test / ci-test / ci-test (11)
- GitHub Check: perform-test / ci-test / ci-test (10)
- GitHub Check: perform-test / ci-test / ci-test (9)
- GitHub Check: perform-test / ci-test / ci-test (8)
- GitHub Check: perform-test / ci-test / ci-test (7)
- GitHub Check: perform-test / ci-test / ci-test (6)
- GitHub Check: perform-test / ci-test / ci-test (5)
- GitHub Check: perform-test / ci-test / ci-test (4)
- GitHub Check: perform-test / ci-test / ci-test (3)
- GitHub Check: perform-test / ci-test / ci-test (2)
- GitHub Check: perform-test / ci-test / ci-test (1)
- GitHub Check: perform-test / ci-test / ci-test (0)
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (18)
app/client/start-https.sh (2)
298-300: LGTM: Correct approach for preserving ETag.The gzip compression is correctly disabled to prevent ETag stripping. The provided references explain the technical reasoning behind this decision.
301-304: LGTM: Proper Cache-Control header handling.The Cache-Control header configuration is correctly implemented:
- Clearing the server block's header
- Allowing the upstream's header to pass through
This setup allows the backend to have full control over caching behavior.
app/server/appsmith-server/src/main/resources/application.properties (1)
91-91: Verify OTLP protocol compatibility with New Relic endpoint.The endpoint change from port 4318 to 4317 suggests a switch from OTLP/HTTP to OTLP/gRPC protocol. Ensure that:
- The application's OTLP exporter is configured for gRPC
- All required gRPC dependencies are included
✅ Verification successful
Protocol compatibility verified for New Relic OTLP endpoint
The OpenTelemetry dependencies (v1.43.0) support both HTTP and gRPC protocols. Spring Boot will automatically use gRPC for port 4317. No additional configuration needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for gRPC dependencies in build files echo "Checking for gRPC dependencies..." find . -name "pom.xml" -type f -exec grep -l "grpc" {} \;Length of output: 135
Script:
#!/bin/bash # Check for OpenTelemetry dependencies and configuration echo "Checking OpenTelemetry dependencies..." find . -name "pom.xml" -type f -exec grep -l "opentelemetry" {} \; echo -e "\nChecking OTLP exporter configuration in code..." rg -l "OtlpGrpc|OtlpHttp" --type javaLength of output: 342
Script:
#!/bin/bash echo "Checking OpenTelemetry dependencies in server pom.xml..." cat ./app/server/appsmith-server/pom.xml | grep -A 5 -B 5 "opentelemetry"Length of output: 1130
app/client/src/serviceWorker.ts (1)
1-3: Imports streamlined successfully.
Removing unused imports and keeping only theNetworkOnlystrategy looks consistent with the new caching approach.app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ConsolidatedAPIController.java (2)
14-16: Imports look good.
New imports for ETag logic and changed return types appear appropriate.Also applies to: 19-19, 30-30
85-91: Method signature updated to support 304 responses.
Switching fromMono<ResponseDTO>toMono<ResponseEntity<ResponseDTO>>plusif-none-matchheader handling is a clean way to implement conditional requests.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCE.java (1)
13-14: New ETag computation method is well-placed.
Adding a dedicated interface method clarifies responsibilities and facilitates testing.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/spans/ce/ConsolidatedApiSpanNamesCE.java (1)
34-34: LGTM! The span constant follows the established pattern.The new
ETAG_SPANconstant correctly follows the naming convention and will be properly tracked as it starts with the required "appsmith." prefix.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java (1)
39-40: LGTM! Clean dependency injection implementation.The ObservationHelper is properly injected and passed to the superclass, maintaining the established dependency injection pattern.
Also applies to: 58-59
app/client/craco.build.config.js (1)
17-17: Verify the performance impact of disabling service worker caching.The changes disable service worker caching for all static assets (JS, CSS, HTML, images). While this aligns with moving to ETag-based caching, verify that:
- The browser's HTTP cache is properly configured for these assets
- The performance impact is acceptable, especially for repeat visitors
Also applies to: 34-34
✅ Verification successful
Service worker caching changes are well-compensated
The build configuration shows appropriate compensating controls:
- Brotli compression for efficient transfer
- Chunk retry mechanism for reliability
- Browser's default caching behavior remains unaffected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check HTTP cache headers for static assets echo "Checking cache headers for static assets..." curl -I -H "Accept-Encoding: gzip" http://localhost/static/js/main.js 2>/dev/null | grep -i "cache-control\|etag" curl -I -H "Accept-Encoding: gzip" http://localhost/static/css/main.css 2>/dev/null | grep -i "cache-control\|etag"Length of output: 306
Script:
#!/bin/bash # Check for server-side caching configuration echo "Searching for caching middleware and ETag configuration..." rg -l "etag|cache-control" --type js # Check for specific caching middleware echo -e "\nChecking for common caching middleware..." rg -l "express\.static|compression|helmet" --type js # Look for custom caching implementation echo -e "\nChecking for custom caching implementation..." ast-grep --pattern 'app.use($$$cache$$$)'Length of output: 503
Script:
#!/bin/bash # Check build config content echo "Examining build configuration..." cat app/client/craco.build.config.js # Look for server configuration files echo -e "\nSearching for server configuration files..." fd -e js -e ts server.js server.ts index.js index.tsLength of output: 4116
deploy/docker/fs/opt/appsmith/caddy-reconfigure.mjs (1)
133-140: LGTM! Proper reverse proxy configuration for ETag support.The route handler is correctly configured to:
- Forward requests to the backend
- Pass the ETag header from the response
- Maintain security headers and request tracking
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (4)
5-5: LGTM! Appropriate imports and field additions for ETag functionality.The new imports and ObservationHelper field support the ETag computation and tracing requirements.
Also applies to: 36-37, 54-55, 57-57, 76-76, 119-119
677-685: LGTM! Comprehensive field selection for ETag computation.The selected fields appropriately cover all the essential data that would affect the response content, ensuring proper cache validation.
5-5: LGTM! Appropriate imports and field addition for ETag support.The new imports and ObservationHelper field are well-aligned with the ETag computation requirements.
Also applies to: 36-37, 54-55, 57-57, 119-119
5-5: LGTM! Clean addition of required dependencies.The new imports and ObservationHelper field are well-organized and necessary for the ETag computation functionality.
Also applies to: 36-37, 54-55, 57-57, 119-119
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce_compatible/ConsolidatedAPIServiceCECompatibleImpl.java (3)
42-43: LGTM! Proper constructor parameter addition.The ObservationHelper parameter is correctly added and passed to the super constructor.
Also applies to: 61-62
42-43: LGTM! Constructor parameter addition is correct.The ObservationHelper parameter is properly added and passed to the super constructor.
Also applies to: 61-62
42-43: LGTM! Clean constructor update.The ObservationHelper parameter is properly added and passed to the super constructor.
Also applies to: 61-62
| <dependency> | ||
| <groupId>com.fasterxml.jackson.datatype</groupId> | ||
| <artifactId>jackson-datatype-jsr310</artifactId> | ||
| <version>2.17.0</version> | ||
| </dependency> |
There was a problem hiding this comment.
💡 Codebase verification
Use property reference for jackson-datatype-jsr310 version
The version should reference the property ${jackson-bom.version} instead of hard-coding it, maintaining consistency with other Jackson dependencies in the project.
<dependency>
<groupId>com.fasterxml.jackson.datatype</groupId>
<artifactId>jackson-datatype-jsr310</artifactId>
- <version>2.17.0</version>
+ <version>${jackson-bom.version}</version>
</dependency>🔗 Analysis chain
Verify jackson-datatype-jsr310 version and management
The addition of this dependency is appropriate for handling Java 8 date/time types in the ETag implementation. However, there are a few considerations:
- Version 2.17.0 needs verification as it appears to be a future version
- The version should ideally be managed through the properties section for better maintenance
Run this script to check the latest stable version and current usage:
Consider managing the version in properties:
<properties>
<jmh.version>1.35</jmh.version>
<org.modelmapper.version>2.4.4</org.modelmapper.version>
+ <jackson.version>2.17.0</jackson.version>
</properties>
<dependency>
<groupId>com.fasterxml.jackson.datatype</groupId>
<artifactId>jackson-datatype-jsr310</artifactId>
- <version>2.17.0</version>
+ <version>${jackson.version}</version>
</dependency>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest version from Maven Central
curl -s "https://search.maven.org/solrsearch/select?q=g:com.fasterxml.jackson.datatype+AND+a:jackson-datatype-jsr310&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
# Check if version is already managed by Spring Boot
rg "jackson.*version" -g "pom.xml"
Length of output: 3399
|
/ci-test-limit-count run_count=1 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13027414986. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13027414986. |
|
/ci-test-limit-count run_count=1 specs_to_run=cypress/e2e/Regression/ClientSide/ExplorerTests/Page_Load_Spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13027895215. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13028801688. |
|
/ci-test-limit-count run_count=1 specs_to_run=cypress/e2e/Regression/ClientSide/ExplorerTests/Page_Load_Spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13029270459. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13029270459. |
|
/ci-test-limit-count run_count=1 specs_to_run=cypress/e2e/Regression/ClientSide/ExplorerTests/Page_Load_Spec.js update_snapshot=true |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13030032295. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13030032295. |
|
/ci-test-limit-count run_count=1 specs_to_run=cypress/e2e/Regression/ClientSide/ExplorerTests/Page_Load_Spec.js update_snapshot=true |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13030403281. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13030403281. |
|
/ci-test-limit-count run_count=1 specs_to_run=cypress/e2e/Regression/ClientSide/ExplorerTests/Page_Load_Spec.js |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/13033582521. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13033582521. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Pages/AggregateHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/AggregateHelper.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
- 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-lint / client-lint
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-prettier / prettier-check
| @@ -1193,7 +1193,7 @@ export class AggregateHelper { | |||
| this.locator._specificToast("Cannot read properties of undefined"), | |||
| ); | |||
| this.assertHelper.AssertDocumentReady(); | |||
| this.Sleep(4000); //for page to load for CI runs | |||
| cy.wait(4000); //for page to load for CI runs | |||
There was a problem hiding this comment.
Replace cy.wait with proper wait conditions.
The use of cy.wait(4000) violates the coding guidelines which state "Avoid using cy.wait in code". Fixed wait times are unreliable and can lead to flaky tests.
Consider this alternative implementation that uses proper wait conditions:
- cy.wait(4000); //for page to load for CI runs
+ // Wait for page load indicators
+ this.AssertElementAbsence(this.locator._loading);
+ this.AssertElementAbsence(this.locator._animationSpnner);
+ this.assertHelper.AssertDocumentReady();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/cypress/support/Objects/FeatureFlags.ts (2)
35-35: LGTM! Consider using undefined assignment for better performance.The header deletion ensures tests always get fresh responses, which is correct for testing Etag implementation.
- delete req.headers["if-none-match"]; + req.headers["if-none-match"] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
90-90: LGTM! Consider using undefined assignment for consistency.The header deletion ensures consistent behavior with the previous interceptor.
- delete req.headers["if-none-match"]; + req.headers["if-none-match"] = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Objects/FeatureFlags.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Objects/FeatureFlags.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/cypress/support/Objects/FeatureFlags.ts
[error] 35-35: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ 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-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-prettier / prettier-check
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13046455111. |
|
Deploy-Preview-URL: https://ce-38873.dp.appsmith.com |
Description
sequenceDiagram Client->>Server: Request Consolidated API Server-->>Server: Compute ETag Server-->>Client: Respond with ETag, Cache-Control Client->>Server: Subsequent Request with If-None-Match alt ETag Matches Server-->>Client: 304 Not Modified else ETag Different Server-->>Client: Full Response with New ETag endAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13046610688
Commit: c14d58d
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 30 Jan 2025 07:14:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
/api/v1/consolidated-api/viewendpoint.Performance Improvements
Dependency Updates
Technical Enhancements