Skip to content

Conversation

@sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Nov 28, 2025

Description

Tip

Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

Fixes #Issue Number
or
Fixes Issue URL

Warning

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.Git"

🔍 Cypress test results

Warning

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19814283152
Commit: dcc8e7b
Cypress dashboard.
Tags: @tag.Git
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.


Mon, 01 Dec 2025 07:08:41 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Chores
    • Added analytics tracking for static URL workflows: new events for enabling/disabling static URLs and for application/page slug updates.
    • Events now capture previous and new slug values, application/page identifiers, workspace context, and Git connection status.
    • Analytics are emitted after successful changes to ensure accurate usage telemetry.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

Walkthrough

Adds four static URL analytics events, a new Spring utility to emit those events with application/page metadata, and integrates event emission into static URL service flows to send analytics after persistence.

Changes

Cohort / File(s) Summary
Analytics Event Definitions
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/AnalyticsEvents.java
Adds enum constants: STATIC_URL_ENABLED, STATIC_URL_DISABLED, STATIC_URL_APP_SLUG_UPDATED, STATIC_URL_PAGE_SLUG_UPDATED (grouped under // Static URL events).
Analytics Utility
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
New Spring component that fetches current user, builds application- and page-level analytics property maps (ids, workspace, slug, previousSlug, git status, names), logs debug info, and emits events via AnalyticsService. Methods return the original domain object (Mono<Application> / Mono<NewPage>).
Service Integration
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
Adds protected final StaticUrlAnalyticsUtils staticUrlAnalyticsUtils and invokes its send* methods after persistence for: auto-generate (enable), set application slug (app slug updated), delete/disable (disabled), and page slug updates (page slug updated). Captures previous slug values from relevant existing entities before updates.
sequenceDiagram
    participant Client
    participant StaticUrlServiceImpl
    participant DB as Database
    participant AnalyticsUtil as StaticUrlAnalyticsUtils
    participant SessionUser as SessionUserService
    participant AnalyticsService

    Client->>StaticUrlServiceImpl: request static URL operation (enable/disable/update)
    StaticUrlServiceImpl->>DB: persist changes (save application/page)
    DB-->>StaticUrlServiceImpl: return persisted entity
    StaticUrlServiceImpl->>AnalyticsUtil: send*StaticUrlEvent(event, entity, slug[, previousSlug])
    AnalyticsUtil->>SessionUser: getCurrentUser()
    SessionUser-->>AnalyticsUtil: current user context
    AnalyticsUtil->>AnalyticsService: sendEvent(event, properties)
    AnalyticsService-->>AnalyticsUtil: acknowledge
    AnalyticsUtil-->>StaticUrlServiceImpl: return entity
    StaticUrlServiceImpl-->>Client: return result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay attention to:
    • completeness and null-safety of analytics property maps in StaticUrlAnalyticsUtils
    • correct detection of Git connection (GitUtils usage)
    • capturing previous slug values from unpublished/persisted/fresh-fetched entities
    • reactive chaining to ensure events are emitted only after successful persistence and errors propagate correctly
    • any consumers that might rely on enum ordering/serialization of AnalyticsEvents

Poem

✨ Slugs set sail, then softly told,
Events dispatched when changes hold,
A util hums the metadata tune,
Pages, apps — a tracking boon,
Logs whisper under analytics' moon.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning PR description is incomplete—it uses the template structure but lacks actual content. No issue reference, motivation, context, or dependencies are provided. Provide a meaningful description: reference the issue, explain the purpose of adding static URL analytics, list any dependencies, and clarify the scope of changes. Fill in or remove template placeholders.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding analytics tracking for static URL operations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/static-url-analytics

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Nov 28, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f9ee62 and 791891e.

📒 Files selected for processing (3)
  • app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/AnalyticsEvents.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-30T07:15:20.287Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/utils/helpers.tsx:1124-1136
Timestamp: 2025-10-30T07:15:20.287Z
Learning: In app/client/src/utils/helpers.tsx, within the getUpdateRouteForSlugPath function, the pageSlug parameter extracted from the route includes the trailing hyphen (e.g., "home-page-" not "home-page"). Additionally, when the static slug conversion branch is executed, params.basePageId is guaranteed to be defined because the code path requires successful page data fetch from the server as a precondition.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
  • GitUtils (18-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: server-unit-tests / server-unit-tests
  • GitHub Check: server-spotless / spotless-check
🔇 Additional comments (5)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/AnalyticsEvents.java (1)

99-103: LGTM!

New analytics events are well-organized with a descriptive comment and follow the existing naming conventions. The default constructor will produce lowercase event names (e.g., static_url_enabled), which is consistent with other events in this enum.

app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (1)

70-71: LGTM!

The StaticUrlAnalyticsUtils dependency is properly injected via constructor (Lombok's @RequiredArgsConstructor).

app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (3)

26-33: LGTM!

Clean class structure with proper annotations and dependency injection.


95-114: LGTM!

Analytics properties are well-structured with proper null handling using defaultIfNull. The inclusion of git connection status provides useful context.


124-145: LGTM!

Page analytics properties are appropriately structured with defensive null checking for nested page name access.

Comment on lines 45 to 57
return sessionUserService.getCurrentUser().flatMap(user -> {
Map<String, Object> analyticsProps = buildApplicationAnalyticsProps(application, uniqueSlug);

log.debug(
"Sending static URL analytics event: {} for applicationId: {}, slug: {}",
event.getEventName(),
application.getId(),
uniqueSlug);

return analyticsService
.sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
.thenReturn(application);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Empty user Mono could cause unexpected behavior.

If sessionUserService.getCurrentUser() returns an empty Mono, the flatMap won't execute and an empty Mono<Application> is returned. This could cause issues upstream where the caller expects the application to be returned. Consider using switchIfEmpty or defaultIfEmpty to handle this gracefully.

-        return sessionUserService.getCurrentUser().flatMap(user -> {
+        return sessionUserService.getCurrentUser()
+                .flatMap(user -> {
             Map<String, Object> analyticsProps = buildApplicationAnalyticsProps(application, uniqueSlug);

             log.debug(
                     "Sending static URL analytics event: {} for applicationId: {}, slug: {}",
                     event.getEventName(),
                     application.getId(),
                     uniqueSlug);

             return analyticsService
                     .sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
                     .thenReturn(application);
-        });
+        })
+        .defaultIfEmpty(application);
📝 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.

Suggested change
return sessionUserService.getCurrentUser().flatMap(user -> {
Map<String, Object> analyticsProps = buildApplicationAnalyticsProps(application, uniqueSlug);
log.debug(
"Sending static URL analytics event: {} for applicationId: {}, slug: {}",
event.getEventName(),
application.getId(),
uniqueSlug);
return analyticsService
.sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
.thenReturn(application);
});
return sessionUserService.getCurrentUser()
.flatMap(user -> {
Map<String, Object> analyticsProps = buildApplicationAnalyticsProps(application, uniqueSlug);
log.debug(
"Sending static URL analytics event: {} for applicationId: {}, slug: {}",
event.getEventName(),
application.getId(),
uniqueSlug);
return analyticsService
.sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
.thenReturn(application);
})
.defaultIfEmpty(application);
🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
around lines 45 to 57, handle the case where sessionUserService.getCurrentUser()
returns an empty Mono by ensuring the method still returns the application;
change the reactive chain so that after the flatMap you append a
switchIfEmpty(Mono.just(application)) (or equivalent) to skip sending analytics
when there is no user but still return the application upstream, and optionally
add a debug log in the switchIfEmpty branch to indicate analytics was skipped.

Comment on lines 72 to 85
return sessionUserService.getCurrentUser().flatMap(user -> {
Map<String, Object> analyticsProps = buildPageAnalyticsProps(page, applicationId, uniqueSlug);

log.debug(
"Sending static URL analytics event: {} for pageId: {}, applicationId: {}, slug: {}",
event.getEventName(),
page.getId(),
applicationId,
uniqueSlug);

return analyticsService
.sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
.thenReturn(page);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same empty user concern as above.

Apply similar defaultIfEmpty(page) handling to ensure the page is always returned even if no user is present.

🤖 Prompt for AI Agents
In
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
around lines 72 to 85, the call to
sessionUserService.getCurrentUser().flatMap(...) can produce an empty Mono when
no user exists causing the method to return empty; append .defaultIfEmpty(page)
after the flatMap chain so the page is always returned when the user is absent
(i.e., sessionUserService.getCurrentUser().flatMap(...).defaultIfEmpty(page));
no other behavior changes required.

return applicationService
.save(appFromDB)
.flatMap(savedApp -> staticUrlAnalyticsUtils.sendApplicationStaticUrlEvent(
AnalyticsEvents.STATIC_URL_APP_SLUG_UPDATED, savedApp, normalizedSlug));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you capture what was in the previous state, like what was the old stug for page and application?

@github-actions
Copy link

Failed server tests

  • com.appsmith.server.helpers.UserUtilsTest#makeInstanceAdministrator_WhenUserAlreadyAdmin_MaintainsPermissionsSuccessfully

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (4)

271-273: Analytics failure could still break the primary flow.

The flatMap chain means an analytics failure propagates and fails the entire autoGenerateStaticUrl operation. Add .onErrorResume() to swallow analytics errors.


423-429: Analytics failure could still break the slug update.

Same concern as above—wrap with .onErrorResume() to prevent analytics issues from failing the save.


549-551: Analytics failure could block the delete response.

Apply .onErrorResume() to ensure static URL deletion succeeds even if analytics fails.


716-723: Analytics failure could block page slug updates.

Both branches use flatMap for analytics. Apply .onErrorResume() to both paths.

Also applies to: 748-755

app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (2)

59-72: Empty user Mono could cause unexpected behavior.

If sessionUserService.getCurrentUser() returns empty, the flatMap won't execute and an empty Mono<Application> is returned. Add .defaultIfEmpty(application) after the flatMap.


102-116: Same empty user concern for page events.

Apply .defaultIfEmpty(page) to ensure the page is returned even when no user session exists.

🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (1)

735-741: Redundant previous slug capture.

The previous slug is already captured at lines 702-706 via capturedPreviousSlug. This second fetch from pageFromDB re-reads the same value since no modification has occurred yet. Consider reusing capturedPreviousSlug instead.

                                return pageService.findById(pageId, null).flatMap(pageFromDB -> {
-                                    // Capture previous slug from fresh DB fetch for analytics
-                                    String prevSlugFromDB = null;
-                                    if (pageFromDB.getUnpublishedPage() != null) {
-                                        prevSlugFromDB =
-                                                pageFromDB.getUnpublishedPage().getUniqueSlug();
-                                    }
-                                    final String capturedPrevSlugFromDB = prevSlugFromDB;
-
                                     PageDTO editPage = pageFromDB.getUnpublishedPage();
                                     if (editPage != null) {
                                         editPage.setUniqueSlug(normalizedPageSlug);
                                     }

                                     return pageService
                                             .save(pageFromDB)
                                             .flatMap(savedPage -> staticUrlAnalyticsUtils.sendPageStaticUrlEvent(
                                                     AnalyticsEvents.STATIC_URL_PAGE_SLUG_UPDATED,
                                                     savedPage,
                                                     applicationId,
                                                     normalizedPageSlug,
-                                                    capturedPrevSlugFromDB));
+                                                    capturedPreviousSlug));
                                 });
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 791891e and 22a62f0.

📒 Files selected for processing (2)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (8 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-01-30T10:47:35.449Z
Learnt from: ApekshaBhosale
Repo: appsmithorg/appsmith PR: 38925
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java:0-0
Timestamp: 2025-01-30T10:47:35.449Z
Learning: In reactive programming with Spring WebFlux, avoid using `block()` as it can lead to thread pool exhaustion. Instead, maintain the reactive chain using operators like `flatMap()` and handle empty cases with `defaultIfEmpty()`. When adding tags or metrics, ensure they are applied within the reactive chain.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-10-30T07:15:20.287Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/utils/helpers.tsx:1124-1136
Timestamp: 2025-10-30T07:15:20.287Z
Learning: In app/client/src/utils/helpers.tsx, within the getUpdateRouteForSlugPath function, the pageSlug parameter extracted from the route includes the trailing hyphen (e.g., "home-page-" not "home-page"). Additionally, when the static slug conversion branch is executed, params.basePageId is guaranteed to be defined because the code path requires successful page data fetch from the server as a precondition.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-10-28T03:30:58.299Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/sagas/InitSagas.ts:260-271
Timestamp: 2025-10-28T03:30:58.299Z
Learning: In app/client/src/sagas/InitSagas.ts, when constructing consolidatedApiParams for static page URLs, the code intentionally reuses applicationId and defaultPageId fields to pass staticApplicationSlug and staticPageSlug values respectively. This is by design, even though ConsolidatedApiParams type has dedicated staticApplicationSlug and staticPageSlug fields.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-10-28T09:17:22.519Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359
Timestamp: 2025-10-28T09:17:22.519Z
Learning: In the Appsmith codebase, slug normalization for static URLs (application slugs and page slugs) handles lowercase conversion on the backend. The frontend helper `filterAccentedAndSpecialCharacters` in `app/client/src/pages/AppIDE/components/AppSettings/utils.ts` intentionally preserves the user's input casing to avoid modifying the entered value too drastically, as the backend will normalize to lowercase during persistence.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-06-18T10:23:51.839Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 40966
File: app/server/reactive-caching/src/main/java/com/appsmith/caching/aspects/DistributedLockAspect.java:139-139
Timestamp: 2025-06-18T10:23:51.839Z
Learning: In reactive code, using `defaultIfEmpty()` on a Mono ensures it will never be empty, which prevents `block()` from returning null. For example, `instanceIdProvider.getInstanceId().defaultIfEmpty("unknown")` guarantees that the Mono will always emit a value, making subsequent `block()` calls safe from null pointer exceptions.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
📚 Learning: 2024-10-08T15:32:34.115Z
Learnt from: sondermanish
Repo: appsmithorg/appsmith PR: 30521
File: app/server/appsmith-server/src/main/java/com/appsmith/server/exports/exportable/ExportServiceCEImpl.java:0-0
Timestamp: 2024-10-08T15:32:34.115Z
Learning: The `sanitizeEntities` method in `ExportServiceCEImpl.java` should return `Mono.empty()` instead of `Mono.empty().then()` for simplicity and clarity.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
📚 Learning: 2024-10-04T05:51:15.138Z
Learnt from: abhvsn
Repo: appsmithorg/appsmith PR: 36596
File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java:475-475
Timestamp: 2024-10-04T05:51:15.138Z
Learning: In `app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java`, the variable `newsletterSignedUpUserEmail` is always expected to be non-null because the user's email is always present in the user object, so an additional null check is unnecessary.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
  • GitUtils (18-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: server-spotless / spotless-check
  • GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (3)

4-4: LGTM on the new import and field injection.

The AnalyticsEvents import and StaticUrlAnalyticsUtils field are appropriately added for the analytics integration.

Also applies to: 70-71


415-420: Good: Previous slug capture for analytics.

Capturing the previous slug before modification addresses the reviewer feedback to track the old state.


701-706: Good: Previous slug capture implemented.

This addresses the request to track the old slug state for analytics.

app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlAnalyticsUtils.java (3)

26-32: LGTM on class structure.

Standard Spring component with appropriate dependencies. Using @RequiredArgsConstructor with final fields is clean.


127-152: LGTM on application analytics properties.

Proper use of defaultIfNull for null-safety and GitUtils.isArtifactConnectedToGit for git status. Conditional inclusion of previousSlug is clean.


163-190: LGTM on page analytics properties.

Null-safe extraction of page name and proper use of defaultIfNull. The absence of isGitConnected is reasonable since page-level events don't directly carry git metadata.

@github-actions
Copy link

Failed server tests

  • com.appsmith.server.helpers.UserUtilsTest#makeInstanceAdministrator_WhenUserAlreadyAdmin_MaintainsPermissionsSuccessfully

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (2)

563-571: STATIC_URL_DISABLED analytics is correctly chained after full cleanup

Hooking STATIC_URL_DISABLED after both the application and all pages are saved, and guarding it with onErrorResume, keeps static URL teardown reliable while still recording the event. If you later need to know which slug was disabled in analytics, you could capture the pre-disable slug before disableStaticUrlSettings(application) and extend the event payload/schema to include it, but that’s an optional enhancement.


719-785: Page slug analytics now carry old/new values for both clear and set flows

The updatePageSlug changes correctly:

  • Capture applicationId once from the resolved Application.
  • Capture the previous slug from the in-memory page when clearing, and from a fresh DB fetch when setting, so STATIC_URL_PAGE_SLUG_UPDATED events get both newSlug (or null on clear) and a reliable previousSlug.
  • Use onErrorResume on both analytics calls so slug updates/clears are never blocked by analytics failures.

Using TextUtils.makeSlug to derive normalizedPageSlug is also consistent with the backend-driven slug normalization behavior we use elsewhere for static URLs. Based on learnings, this keeps slug casing and normalization semantics aligned across flows.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22a62f0 and dcc8e7b.

📒 Files selected for processing (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-30T07:15:20.287Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/utils/helpers.tsx:1124-1136
Timestamp: 2025-10-30T07:15:20.287Z
Learning: In app/client/src/utils/helpers.tsx, within the getUpdateRouteForSlugPath function, the pageSlug parameter extracted from the route includes the trailing hyphen (e.g., "home-page-" not "home-page"). Additionally, when the static slug conversion branch is executed, params.basePageId is guaranteed to be defined because the code path requires successful page data fetch from the server as a precondition.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-10-28T03:30:58.299Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/sagas/InitSagas.ts:260-271
Timestamp: 2025-10-28T03:30:58.299Z
Learning: In app/client/src/sagas/InitSagas.ts, when constructing consolidatedApiParams for static page URLs, the code intentionally reuses applicationId and defaultPageId fields to pass staticApplicationSlug and staticPageSlug values respectively. This is by design, even though ConsolidatedApiParams type has dedicated staticApplicationSlug and staticPageSlug fields.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
📚 Learning: 2025-10-28T09:17:22.519Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359
Timestamp: 2025-10-28T09:17:22.519Z
Learning: In the Appsmith codebase, slug normalization for static URLs (application slugs and page slugs) handles lowercase conversion on the backend. The frontend helper `filterAccentedAndSpecialCharacters` in `app/client/src/pages/AppIDE/components/AppSettings/utils.ts` intentionally preserves the user's input casing to avoid modifying the entered value too drastically, as the backend will normalize to lowercase during persistence.

Applied to files:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: server-spotless / spotless-check
  • GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/staticurl/StaticUrlServiceImpl.java (3)

4-70: Analytics dependencies are wired in cleanly

Importing AnalyticsEvents and injecting StaticUrlAnalyticsUtils via the existing @RequiredArgsConstructor pattern aligns with the usages below and keeps this service’s dependencies consistent; no changes needed here.


271-282: STATIC_URL_ENABLED analytics is sequenced correctly and made non-blocking

Emitting STATIC_URL_ENABLED only after generateUniquePageSlugsForApplication completes, and swallowing failures with onErrorResume, ensures analytics can’t break autoGenerateStaticUrl while still logging issues. Just confirm that sendApplicationStaticUrlEvent returns the same Application instance (savedApp); if not, consider wrapping it with thenReturn(savedApp) so this method’s return value is unambiguous.


424-443: Previous application slug capture for update analytics looks correct

Capturing previousSlug from appFromDB.getStaticUrlSettings().getUniqueSlug() before mutating, and then sending STATIC_URL_APP_SLUG_UPDATED with both normalizedSlug and capturedPreviousSlug, gives analytics the expected before/after context without changing the core save flow. The onErrorResume fallback also keeps slug updates from failing due to analytics issues.

@github-actions
Copy link

Failed server tests

  • com.appsmith.server.helpers.UserUtilsTest#makeInstanceAdministrator_WhenUserAlreadyAdmin_MaintainsPermissionsSuccessfully

@sondermanish sondermanish enabled auto-merge (squash) December 1, 2025 06:32
@sondermanish sondermanish self-assigned this Dec 1, 2025
@ashit-rath ashit-rath disabled auto-merge December 1, 2025 07:06
@sondermanish sondermanish added the ok-to-test Required label for CI label Dec 1, 2025
@ashit-rath ashit-rath merged commit 7e119f9 into release Dec 1, 2025
34 of 36 checks passed
@ashit-rath ashit-rath deleted the chore/static-url-analytics branch December 1, 2025 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants