Movatterモバイル変換

vsvamsi1 added the ok-to-testRequired label for CI label

ashit-rath previously approved these changes

github-actionsbot added the EnhancementNew feature or request label

Copy link

Contributor

coderabbitaibot commentedNov 24, 2025•
edited
Loading

Walkthrough

Added a docs-link text constant and a docs tooltip/opener in GeneralSettings for Static URL; introduced new STATIC_URL analytics event types; added analytics logging across UI and sagas for static URL enable/disable/persist operations; adjusted some callback dependencies in PageSettings.

Changes

Cohort / File(s)	Summary
Constants `app/client/src/ce/constants/messages.ts`	Added exported`STATIC_URL_DOCS_LINK_TEXT` returning`"Learn more in docs"`.
UI — App Settings (Static URL) `app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx`	Added`STATIC_URL_DOCS_URL`; imported`Tooltip`; added question-mark tooltip trigger beside Static URL label and header; memoized tooltip content rendering a clickable docs link; added handler to open docs URL; added analytics calls for toggle, docs click, apply, cancel, enable/disable flows; updated effect/deps.
Analytics types `app/client/src/ce/utils/analyticsUtilTypes.ts`	Added exported`STATIC_URL_EVENTS` union and extended`EventName` to include static-URL related event literals (toggle, cancel, docs click, slug/app persist success/error, enabled/disabled success/error).
Page settings telemetry `app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx`	Broadened`savePageName` dependencies; added analytics logging on page slug persistence (success/error) with pageId and slug.
Sagas — Static URL operations `app/client/src/ce/sagas/ApplicationSagas.tsx` ,`app/client/src/ce/sagas/PageSagas.tsx`	Reworked persistAppSlugSaga/error guards and added try/catch; added fetchAppAndPages refresh after persist/enable/disable; added analytics logs for STATIC_URL_APP_SLUG_PERSIST_SUCCESS/ERROR, STATIC_URL_ENABLED_SUCCESS/ERROR, STATIC_URL_DISABLED_SUCCESS/ERROR, and page-slug persist success/error with payloads including ids and slugs.

Sequence Diagram(s)

sequenceDiagram  autonumber  participant User  participant GeneralSettings  participant Tooltip  participant Browser  participant Analytics  Note over GeneralSettings,Tooltip: UI docs/help flow (new)  User->>GeneralSettings: hover/click help icon  GeneralSettings->>Tooltip: show(staticUrlTooltipContent)  Tooltip-->>User: display tooltip ("Learn more in docs")  User->>Tooltip: click docs link  Tooltip->>GeneralSettings: onDocsClick()  GeneralSettings->>Analytics: emit STATIC_URL_DOCS_CLICK  GeneralSettings->>Browser: open STATIC_URL_DOCS_URL (new tab)

sequenceDiagram  autonumber  participant User  participant GeneralSettings  participant Sagas  participant Analytics  Note over GeneralSettings,Sagas: Toggle/enable/disable/persist flows  User->>GeneralSettings: toggle Static URL  GeneralSettings->>Analytics: emit STATIC_URL_TOGGLE_CLICK  GeneralSettings->>Sagas: dispatch enable/disable/persist actions  Sagas->>Sagas: validate currentApplication / try/catch  Sagas->>Analytics: emit STATIC_URL_*_SUCCESS or *_ERROR (with appId/slug)  Sagas->>GeneralSettings: refresh via fetchAppAndPagesSaga

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Check analytics type literals and exported union inanalyticsUtilTypes.ts.
Verify payload shapes and that no sensitive data is logged in analytics (slugs/ids).
Review dependency arrays and memoization inGeneralSettings.tsx andPageSettings.tsx to avoid stale closures.
Confirm saga error handling changes preserve previous semantics and that fetchAppAndPagesSaga refresh is safe.

Poem

A tiny question mark nudges the view,
"Learn more in docs" — one click, off you flew.
Events hum softly, slugs and toggles told,
Sagas refresh state, analytics unfold. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run`@coderabbitai generate docstrings` to improve docstring coverage.
Description check	⚠️ Warning	The PR description lacks the required issue reference (Fixes #...) and contains unresolved placeholder text, but core changes are documented with context.	Add the actual issue number or URL to the 'Fixes' section. Remove or replace placeholder text before merging.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: adding a helper icon for static URL documentation and analytics tracking.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branchtestCases

Thanks for usingCodeRabbit! 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.}

coderabbitaibot reviewed

Copy link

Contributor

coderabbitaibot 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/client/src/ce/constants/messages.ts (1)
1952-1952:Docs link text constant looks good
The newSTATIC_URL_DOCS_LINK_TEXT helper follows the existing message pattern and keeps the docs CTA copy centralized. If you find similar “learn more” phrases proliferating later, you might consider consolidating them, but nothing to change here now.
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (1)
26-40:Static URL docs tooltip wiring is solid; consider i18n updates andtarget="_blank" hardening
The Tooltip + Link setup around the Static URL label is a nice UX improvement and the reuse ofSTATIC_URL_DOCS_LINK_TEXT keeps copy centralized.
Two small refinements to consider:
Line 114–123:staticUrlTooltipContent is memoized with[], so the translated string is frozen after the first render. If you ever support runtime locale switching, this tooltip won’t update. Given the tiny cost here, you could dropuseMemo and inline the JSX, or at least avoid an empty dependency array so the content re-renders when surrounding props/state change.
Line 116: TheLink usestarget="_blank". Please verify whether the@appsmith/adsLink component automatically addsrel="noopener noreferrer" for security; if not, it’s safer to pass that explicitly.
Also applies to: 49-50, 114-123, 482-491

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and betweenb95329d andb65dcc7.

📒 Files selected for processing (2)

app/client/src/ce/constants/messages.ts (1 hunks)
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (4 hunks)

🧰 Additional context used

🧠 Learnings (5)

📚 Learning: 2025-04-28T16:16:02.155Z

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 40462File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40Timestamp: 2025-04-28T16:16:02.155ZLearning: The Appsmith team prefers not to include HTML markup in string constants. Text styling or emphasis should be handled through proper React components rather than HTML tags in strings.

Applied to files:

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-30T07:15:20.287Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/utils/helpers.tsx:1124-1136Timestamp: 2025-10-30T07:15:20.287ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
app/client/src/ce/constants/messages.ts

📚 Learning: 2025-10-28T03:30:58.299Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/sagas/InitSagas.ts:260-271Timestamp: 2025-10-28T03:30:58.299ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-28T09:17:22.519Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359Timestamp: 2025-10-28T09:17:22.519ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
app/client/src/ce/constants/messages.ts

📚 Learning: 2025-10-30T07:17:49.646Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883Timestamp: 2025-10-30T07:17:49.646ZLearning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.

Applied to files:

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

🧬 Code graph analysis (1)

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (1)

app/client/src/ce/constants/messages.ts (1)
STATIC_URL_DOCS_LINK_TEXT (1952-1952)

⏰ 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). (8)

GitHub Check: perform-test / server-build / server-unit-tests
GitHub Check: perform-test / rts-build / build
GitHub Check: perform-test / client-build / client-build
GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
GitHub Check: client-lint / client-lint
GitHub Check: client-unit-tests / client-unit-tests
GitHub Check: client-build / client-build
GitHub Check: client-prettier / prettier-check

small fix

80945c7

vsvamsi1 dismissedashit-rath’sstale review via80945c7

November 24, 2025 14:36

coderabbitaibot reviewed

Copy link

Contributor

coderabbitaibot 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 (3)

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (3)
48-49:Consider extracting to a constants file.
The docs URL is currently hardcoded in the component file. For easier maintenance and reusability, consider moving it to a constants file (e.g., alongside other documentation URLs).
113-116:Unnecessary memoization for static content.
TheuseMemo here provides no benefit sincecreateMessage(STATIC_URL_DOCS_LINK_TEXT) returns the same static string on every render. You can simplify this to a direct call or a constant.
Apply this diff to simplify:
- const staticUrlTooltipContent = useMemo(-   () => createMessage(STATIC_URL_DOCS_LINK_TEXT),-   [],- );+ const staticUrlTooltipContent = createMessage(STATIC_URL_DOCS_LINK_TEXT);
214-216:Consider adding noopener/noreferrer for defense in depth.
While modern browsers handle_blank safely, explicitly setting window features with noopener can provide additional security.
Apply this diff:
 const openStaticUrlDocs = useCallback(() => {-  window.open(STATIC_URL_DOCS_URL, "_blank");+  const newWindow = window.open(STATIC_URL_DOCS_URL, "_blank", "noopener,noreferrer");+  if (newWindow) newWindow.opener = null; }, []);

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and betweenb65dcc7 and80945c7.

📒 Files selected for processing (1)

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (5 hunks)

🧰 Additional context used

🧠 Learnings (5)

📚 Learning: 2025-04-28T16:16:02.155Z

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 40462File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40Timestamp: 2025-04-28T16:16:02.155ZLearning: The Appsmith team prefers not to include HTML markup in string constants. Text styling or emphasis should be handled through proper React components rather than HTML tags in strings.

Applied to files:

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-30T07:15:20.287Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/utils/helpers.tsx:1124-1136Timestamp: 2025-10-30T07:15:20.287ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-28T03:30:58.299Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/sagas/InitSagas.ts:260-271Timestamp: 2025-10-28T03:30:58.299ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-28T09:17:22.519Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359Timestamp: 2025-10-28T09:17:22.519ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

📚 Learning: 2025-10-30T07:17:49.646Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883Timestamp: 2025-10-30T07:17:49.646ZLearning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.

Applied to files:

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

⏰ 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). (8)

GitHub Check: perform-test / rts-build / build
GitHub Check: perform-test / client-build / client-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: client-prettier / prettier-check

🔇 Additional comments (1)

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (1)
479-490:LGTM! Clean implementation of the help button.
The Tooltip and Button integration is well-structured and follows the ADS component patterns. The icon button provides clear visual affordance for accessing documentation.

vsvamsi1 requested a review fromashit-rath

November 24, 2025 14:40

added analytics

8b98e8d

coderabbitaibot reviewed

Copy link

Contributor

coderabbitaibot 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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx (1)
57-58:Static URL slug analytics event is wired correctly; consider success/failure distinction
ImportingAnalyticsUtil and loggingSTATIC_URL_PAGE_SLUG_CHANGED with{ pageId, oldSlug: page.uniqueSlug, newSlug: staticPageSlug } is consistent with the newSTATIC_URL_EVENTS type and with how static slugs are modeled (uniqueSlug as the static URL slug). The guards insaveStaticPageSlug ensure this only fires when the slug actually changes and passes validation. Based on learnings, this aligns with the existing static slug semantics arounduniqueSlug.
If you care about differentiating successful vs failed saves in telemetry, you might later move this logging to the success path of the persist flow (or add astatus field / complementary failure event), since right now it fires optimistically even if the API call fails.
Also applies to: 296-312
app/client/src/ce/utils/analyticsUtilTypes.ts (1)
360-361:STATIC_URL_EVENTS typing cleanly integrates with existing analytics taxonomy
AddingSTATIC_URL_EVENTS to theEventName union and defining the static URL literals (includingSTATIC_URL_PAGE_SLUG_CHANGED) follows the existing pattern for grouped event types (PREMIUM_DATASOURCES_EVENTS, etc.) and will give good type coverage for the new analytics you’ve wired up.
You may want to confirm downstream analytics consumers (dashboards, funnels) are updated to recognize these new event names where relevant.
Also applies to: 476-493

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between80945c7 and8b98e8d.

📒 Files selected for processing (3)

app/client/src/ce/utils/analyticsUtilTypes.ts (2 hunks)
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (9 hunks)
app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-10-28T03:30:58.299Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/sagas/InitSagas.ts:260-271Timestamp: 2025-10-28T03:30:58.299ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx

📚 Learning: 2025-10-30T07:15:20.287Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/utils/helpers.tsx:1124-1136Timestamp: 2025-10-30T07:15:20.287ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx

📚 Learning: 2025-10-28T09:17:22.519Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359Timestamp: 2025-10-28T09:17:22.519ZLearning: 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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx

⏰ 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). (8)

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-check-cyclic-deps / check-cyclic-dependencies
GitHub Check: client-lint / client-lint
GitHub Check: client-unit-tests / client-unit-tests
GitHub Check: client-build / client-build
GitHub Check: client-prettier / prettier-check

vsvamsi1enabled auto-merge (squash)

November 25, 2025 05:52

vsvamsi1 changed the title~~feat: helper icon for static url documentation~~feat: helper icon for static url documentation and added analytics

ashit-rath reviewed

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx OutdatedShow resolvedHide resolved

Copy link

Contributor

ashit-rath commentedNov 25, 2025

@vsvamsi1 Also let's put the analytics in the saga after the api succeeds

added analytics for api saga updates

5c6b704

vsvamsi1 requested a review fromashit-rath

November 25, 2025 08:00

coderabbitaibot reviewed

Copy link

Contributor

coderabbitaibot 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: 1

♻️ Duplicate comments (2)

app/client/src/ce/sagas/ApplicationSagas.tsx (2)
1317-1325:Same validation pattern issue as in persistAppSlugSaga.
This saga has the same problem with validation logic outside the try block.
1374-1382:Same validation pattern issue as in persistAppSlugSaga.
This saga has the same problem with validation logic outside the try block.

🧹 Nitpick comments (1)

app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx (1)
290-335:Consider aligning deps of other save callbacks for consistency*
saveCustomSlug,saveStaticPageSlug, andsaveIsShown also readcanManagePages (anddispatch for two of them) but don’t include these in theiruseCallback dependency arrays. While these values are effectively stable in most flows, it’s still a potential source of stale closures and is inconsistent with the updatedsavePageName hook.
You could optionally update their dependency arrays to mirror the closure contents, e.g.:
-  }, [page.pageId, page.customSlug, customSlug]);+  }, [canManagePages, dispatch, page.pageId, page.customSlug, customSlug]);-  }, [-    page.pageId,-    page.uniqueSlug,-    staticPageSlug,-    canManagePages,-    dispatch,-    staticPageSlugError,-    isPageSlugValid,-  ]);+  }, [+    canManagePages,+    dispatch,+    page.pageId,+    page.uniqueSlug,+    staticPageSlug,+    staticPageSlugError,+    isPageSlugValid,+  ]);-    [page.pageId, isShown],+    [canManagePages, dispatch, page.pageId, isShown],
This would keep all the save handlers consistent and remove any chance of subtle stale state issues in the future.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between8b98e8d and5c6b704.

📒 Files selected for processing (5)

app/client/src/ce/sagas/ApplicationSagas.tsx (8 hunks)
app/client/src/ce/sagas/PageSagas.tsx (2 hunks)
app/client/src/ce/utils/analyticsUtilTypes.ts (2 hunks)
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (8 hunks)
app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

app/client/src/ce/utils/analyticsUtilTypes.ts
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

🧰 Additional context used

🧠 Learnings (10)

📚 Learning: 2025-10-28T03:30:58.299Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/sagas/InitSagas.ts:260-271Timestamp: 2025-10-28T03:30:58.299ZLearning: 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/client/src/ce/sagas/PageSagas.tsx
app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2025-10-30T07:15:20.287Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/utils/helpers.tsx:1124-1136Timestamp: 2025-10-30T07:15:20.287ZLearning: 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/client/src/ce/sagas/PageSagas.tsx
app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx
app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2025-11-05T12:56:49.249Z

Learnt from: vsvamsi1Repo: appsmithorg/appsmith PR: 41363File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/PagePersistance_Spec.ts:0-0Timestamp: 2025-11-05T12:56:49.249ZLearning: In the file `app/client/cypress/e2e/Regression/ClientSide/StaticUrl/PagePersistance_Spec.ts`, the test suite intentionally uses sequential tests where Tests 4-6 depend on state (`page1Slug`, `page2Slug`) set in Tests 2-3. The author (vsvamsi1) prioritizes readability by breaking the flow into different segments over strict test isolation.

Applied to files:

app/client/src/ce/sagas/PageSagas.tsx
app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2025-11-05T12:58:32.486Z

Learnt from: vsvamsi1Repo: appsmithorg/appsmith PR: 41363File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/SlugPersistance_Spec.ts:32-225Timestamp: 2025-11-05T12:58:32.486ZLearning: In the file `app/client/cypress/e2e/Regression/ClientSide/StaticUrl/SlugPersistance_Spec.ts`, the test suite intentionally uses sequential tests where Tests 2-5 depend on state from previous tests (e.g., slug values modified in earlier tests). The author (vsvamsi1) prioritizes readability by breaking the flow into different segments over strict test isolation.

Applied to files:

app/client/src/ce/sagas/PageSagas.tsx
app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2024-10-24T08:38:20.429Z

Learnt from: ankitakingerRepo: appsmithorg/appsmith PR: 37056File: app/client/src/pages/Editor/JSEditor/JSObjectNameEditor/JSObjectNameEditor.tsx:22-25Timestamp: 2024-10-24T08:38:20.429ZLearning: "constants/AppConstants" does not export "SaveActionNameParams".

Applied to files:

app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx

📚 Learning: 2025-10-30T07:17:49.646Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883Timestamp: 2025-10-30T07:17:49.646ZLearning: In app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx, the isErrorPersistingAppSlug flag is unused and should be removed. There is no corresponding selector to expose it, and no handlers set it to true.

Applied to files:

app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2024-12-10T10:52:38.873Z

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 38060File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13Timestamp: 2024-12-10T10:52:38.873ZLearning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.

Applied to files:

app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2024-12-10T10:52:38.244Z

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 38060File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:28-34Timestamp: 2024-12-10T10:52:38.244ZLearning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts`, error handling is managed outside the scope, so casting errors directly to strings is acceptable.

Applied to files:

app/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2025-10-28T09:17:22.519Z

Learnt from: ashit-rathRepo: appsmithorg/appsmith PR: 41312File: app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx:330-359Timestamp: 2025-10-28T09:17:22.519ZLearning: 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/client/src/ce/sagas/ApplicationSagas.tsx

📚 Learning: 2024-12-10T10:52:57.789Z

Learnt from: brayn003Repo: appsmithorg/appsmith PR: 38060File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45Timestamp: 2024-12-10T10:52:57.789ZLearning: In the TypeScript file `app/client/src/git/sagas/deleteBranchSaga.ts`, within the `deleteBranchSaga` function, error handling is managed outside the scope of the catch block. Therefore, casting `error` to `string` in this context is acceptable.

Applied to files:

app/client/src/ce/sagas/ApplicationSagas.tsx

🧬 Code graph analysis (1)

app/client/src/ce/sagas/ApplicationSagas.tsx (3)

app/client/src/entities/Application/types.ts (1)
ApplicationPayload (12-53)
app/client/src/ce/selectors/applicationSelectors.tsx (1)
getCurrentApplication (46-50)
app/client/src/actions/ReduxActionTypes.ts (1)
ReduxAction (9-12)

⏰ 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). (8)

GitHub Check: perform-test / rts-build / build
GitHub Check: perform-test / client-build / client-build
GitHub Check: perform-test / server-build / server-unit-tests
GitHub Check: client-prettier / prettier-check
GitHub Check: client-lint / client-lint
GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
GitHub Check: client-unit-tests / client-unit-tests
GitHub Check: client-build / client-build

🔇 Additional comments (6)

app/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx (1)
270-288:savePageName deps now correctly track permission & name state
IncludingcanManagePages anddispatch in theuseCallback dependency array matches the values captured in the closure and avoids potential stale reads if permissions or the underlying dispatch instance ever change. This makes the hook more correct and consistent with React’s hook rules.
app/client/src/ce/sagas/PageSagas.tsx (2)
1586-1590:LGTM! Analytics placed correctly after API success.
The analytics logging is appropriately positioned after the API call succeeds and the success action is dispatched, as requested in the PR comments.
1601-1606:LGTM! Error analytics logging is well-structured.
Proper error message extraction and consistent placement with the success case.
app/client/src/ce/sagas/ApplicationSagas.tsx (3)
1228-1231:LGTM! Analytics correctly placed after API success.
The analytics logging follows the requested pattern of being called after the API succeeds and application data is refreshed.
1345-1348:LGTM! Analytics logging is consistent and well-placed.
Both success and error analytics follow the correct pattern.
Also applies to: 1363-1367
1402-1404:LGTM! Analytics logging follows the correct pattern.
Consistent placement and structure with the other sagas.
Also applies to: 1420-1423

Comment on lines +1188 to +1196

		constcurrentApplication:ApplicationPayload\|undefined=yieldselect(
		getCurrentApplication,
		);

		if(!currentApplication){
		thrownewError("No current application found");
		}
		if(!currentApplication){
		thrownewError("No current application found");
		}

		constapplicationId=currentApplication.id;
		constapplicationId=currentApplication.id;

Copy link

Contributor

coderabbitaibotNov 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue |🟠 Major

Consider moving currentApplication validation inside the try block.

The validation logic is currently outside the try-catch block, meaning ifcurrentApplication is null, the thrown error won't be caught by the saga's error handler. This differs from the pattern invalidateAppSlugSaga (lines 1262-1268), which performs validation inside the try block.

This could result in:

Unhandled errors bubbling up
No error action dispatched to update UI state
Error analytics not being logged

Consider this refactor:

 export function* persistAppSlugSaga(   action: ReduxAction<{ slug: string; onSuccess?: () => void }>, ) {+  try {     const currentApplication: ApplicationPayload | undefined = yield select(       getCurrentApplication,     );      if (!currentApplication) {       throw new Error("No current application found");     }      const applicationId = currentApplication.id;-  try {     const { onSuccess, slug } = action.payload;

📝 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

	constcurrentApplication:ApplicationPayload\|undefined=yieldselect(
	getCurrentApplication,
	);

	if(!currentApplication){
	thrownewError("No current application found");
	}
	if(!currentApplication){
	thrownewError("No current application found");
	}

	constapplicationId=currentApplication.id;
	constapplicationId=currentApplication.id;
	exportfunction*persistAppSlugSaga(
	action:ReduxAction<{slug:string;onSuccess?:()=>void}>,
	){
	try{
	constcurrentApplication:ApplicationPayload\|undefined=yieldselect(
	getCurrentApplication,
	);

	if(!currentApplication){
	thrownewError("No current application found");
	}

	constapplicationId=currentApplication.id;

	const{ onSuccess, slug}=action.payload;

🤖 Prompt for AI Agents

In app/client/src/ce/sagas/ApplicationSagas.tsx around lines 1188 to 1196, thecheck that throws when currentApplication is missing is outside the saga'stry-catch and thus won't be handled; move the validation and the extraction ofapplicationId into the existing try block so the thrown error is caught by thesaga error handler (mirror the pattern used in validateAppSlugSaga lines~1262-1268), i.e., remove the external throw, perform the select then inside trydo if (!currentApplication) throw new Error("No current application found") andthen set const applicationId = currentApplication.id so failures dispatch theerror action and run error analytics.

ashit-rath requested changes

		thrownewError("No current application found");
		}
		if(!currentApplication){
		thrownewError("No current application found");

Copy link

Contributor

ashit-rathNov 25, 2025

Choose a reason for hiding this comment

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

Why did we take it out of the try block? If the error is thrown the action will not triggerReduxActionTypes.PERSIST_APP_SLUG_ERROR and there would a suspended state

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

		thrownewError("No current application found");
		}
		if(!currentApplication){
		thrownewError("No current application found");

Copy link

Contributor

ashit-rathNov 25, 2025

Choose a reason for hiding this comment

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

Same with this as well

		]);

		constcancelSlugChange=useCallback(()=>{
		AnalyticsUtil.logEvent("STATIC_URL_CANCEL_CLICK",{

Copy link

Contributor

ashit-rathNov 25, 2025

Choose a reason for hiding this comment

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

I don't think we need it at this level of granularity

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx

		constappSlugSuggestion=useSelector(getAppSlugSuggestion);
		constisAppSlugSaving=useSelector(getIsPersistingAppSlug);

		conststaticUrlTooltipContent=useMemo(

Copy link

Contributor

ashit-rathNov 25, 2025

Choose a reason for hiding this comment

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

It's perfectly fine not to memoize this

app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx


		consthandleStaticUrlToggle=useCallback(
		(isEnabled:boolean)=>{
		AnalyticsUtil.logEvent("STATIC_URL_TOGGLE_CLICK",{

Copy link

Contributor

ashit-rathNov 25, 2025

Choose a reason for hiding this comment

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

We just need success or failure of settings change, modal opening/closing is not a significant event. Let's remove this