feat: helper icon for static url documentation and added analytics #41407
Conversation
WalkthroughAdds a docs link and telemetry for the Static URL feature: a new exported docs-link text constant, an analytics event type for STATIC_URL_DOCS_CLICK, and a tooltip-wrapped help button in App Settings that logs the event and opens the docs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/ce/constants/messages.ts (1)
1952-1952: Docs link text constant looks goodThe new
STATIC_URL_DOCS_LINK_TEXThelper 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"hardeningThe Tooltip + Link setup around the Static URL label is a nice UX improvement and the reuse of
STATIC_URL_DOCS_LINK_TEXTkeeps copy centralized.Two small refinements to consider:
- Line 114–123:
staticUrlTooltipContentis 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 dropuseMemoand inline the JSX, or at least avoid an empty dependency array so the content re-renders when surrounding props/state change.- Line 116: The
Linkusestarget="_blank". Please verify whether the@appsmith/adsLinkcomponent 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
📒 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: brayn003
Repo: appsmithorg/appsmith PR: 40462
File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40
Timestamp: 2025-04-28T16:16:02.155Z
Learning: 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-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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsxapp/client/src/ce/constants/messages.ts
📚 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsxapp/client/src/ce/constants/messages.ts
📚 Learning: 2025-10-30T07:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: 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
There was a problem hiding this comment.
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.The
useMemohere 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
_blanksafely, 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
📒 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: brayn003
Repo: appsmithorg/appsmith PR: 40462
File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40
Timestamp: 2025-04-28T16:16:02.155Z
Learning: 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-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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 Learning: 2025-10-30T07:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: 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.
There was a problem hiding this comment.
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 distinctionImporting
AnalyticsUtiland loggingSTATIC_URL_PAGE_SLUG_CHANGEDwith{ pageId, oldSlug: page.uniqueSlug, newSlug: staticPageSlug }is consistent with the newSTATIC_URL_EVENTStype and with how static slugs are modeled (uniqueSlugas the static URL slug). The guards insaveStaticPageSlugensure 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 a
statusfield / 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 taxonomyAdding
STATIC_URL_EVENTSto theEventNameunion 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
📒 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-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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx
📚 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/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsx
📚 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/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
vsvamsi1
changed the title
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
|
@vsvamsi1 Also let's put the analytics in the saga after the api succeeds |
There was a problem hiding this comment.
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, andsaveIsShownalso readcanManagePages(anddispatchfor two of them) but don’t include these in theiruseCallbackdependency arrays. While these values are effectively stable in most flows, it’s still a potential source of stale closures and is inconsistent with the updatedsavePageNamehook.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
📒 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-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/client/src/ce/sagas/PageSagas.tsxapp/client/src/ce/sagas/ApplicationSagas.tsx
📚 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/client/src/ce/sagas/PageSagas.tsxapp/client/src/pages/AppIDE/components/AppSettings/components/PageSettings.tsxapp/client/src/ce/sagas/ApplicationSagas.tsx
📚 Learning: 2025-11-05T12:56:49.249Z
Learnt from: vsvamsi1
Repo: appsmithorg/appsmith PR: 41363
File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/PagePersistance_Spec.ts:0-0
Timestamp: 2025-11-05T12:56:49.249Z
Learning: 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.tsxapp/client/src/ce/sagas/ApplicationSagas.tsx
📚 Learning: 2025-11-05T12:58:32.486Z
Learnt from: vsvamsi1
Repo: appsmithorg/appsmith PR: 41363
File: app/client/cypress/e2e/Regression/ClientSide/StaticUrl/SlugPersistance_Spec.ts:32-225
Timestamp: 2025-11-05T12:58:32.486Z
Learning: 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.tsxapp/client/src/ce/sagas/ApplicationSagas.tsx
📚 Learning: 2024-10-24T08:38:20.429Z
Learnt from: ankitakinger
Repo: appsmithorg/appsmith PR: 37056
File: app/client/src/pages/Editor/JSEditor/JSObjectNameEditor/JSObjectNameEditor.tsx:22-25
Timestamp: 2024-10-24T08:38:20.429Z
Learning: "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-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: 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: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: 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: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:28-34
Timestamp: 2024-12-10T10:52:38.244Z
Learning: 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-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/client/src/ce/sagas/ApplicationSagas.tsx
📚 Learning: 2024-12-10T10:52:57.789Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/deleteBranchSaga.ts:38-45
Timestamp: 2024-12-10T10:52:57.789Z
Learning: 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 stateIncluding
canManagePagesanddispatchin theuseCallbackdependency 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
| const currentApplication: ApplicationPayload | undefined = yield select( | ||
| getCurrentApplication, | ||
| ); | ||
|
|
||
| if (!currentApplication) { | ||
| throw new Error("No current application found"); | ||
| } | ||
| if (!currentApplication) { | ||
| throw new Error("No current application found"); | ||
| } | ||
|
|
||
| const applicationId = currentApplication.id; | ||
| const applicationId = currentApplication.id; |
There was a problem hiding this comment.
Consider moving currentApplication validation inside the try block.
The validation logic is currently outside the try-catch block, meaning if currentApplication is null, the thrown error won't be caught by the saga's error handler. This differs from the pattern in validateAppSlugSaga (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.
| const currentApplication: ApplicationPayload | undefined = yield select( | |
| getCurrentApplication, | |
| ); | |
| if (!currentApplication) { | |
| throw new Error("No current application found"); | |
| } | |
| if (!currentApplication) { | |
| throw new Error("No current application found"); | |
| } | |
| const applicationId = currentApplication.id; | |
| const applicationId = currentApplication.id; | |
| 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; | |
| const { onSuccess, slug } = action.payload; |
🤖 Prompt for AI Agents
In app/client/src/ce/sagas/ApplicationSagas.tsx around lines 1188 to 1196, the
check that throws when currentApplication is missing is outside the saga's
try-catch and thus won't be handled; move the validation and the extraction of
applicationId into the existing try block so the thrown error is caught by the
saga error handler (mirror the pattern used in validateAppSlugSaga lines
~1262-1268), i.e., remove the external throw, perform the select then inside try
do if (!currentApplication) throw new Error("No current application found") and
then set const applicationId = currentApplication.id so failures dispatch the
error action and run error analytics.
| throw new Error("No current application found"); | ||
| } | ||
| if (!currentApplication) { | ||
| throw new Error("No current application found"); |
There was a problem hiding this comment.
Why did we take it out of the try block? If the error is thrown the action will not trigger ReduxActionTypes.PERSIST_APP_SLUG_ERROR and there would a suspended state
| throw new Error("No current application found"); | ||
| } | ||
| if (!currentApplication) { | ||
| throw new Error("No current application found"); |
| ]); | ||
|
|
||
| const cancelSlugChange = useCallback(() => { | ||
| AnalyticsUtil.logEvent("STATIC_URL_CANCEL_CLICK", { |
There was a problem hiding this comment.
I don't think we need it at this level of granularity
| const appSlugSuggestion = useSelector(getAppSlugSuggestion); | ||
| const isAppSlugSaving = useSelector(getIsPersistingAppSlug); | ||
|
|
||
| const staticUrlTooltipContent = useMemo( |
There was a problem hiding this comment.
It's perfectly fine not to memoize this
|
|
||
| const handleStaticUrlToggle = useCallback( | ||
| (isEnabled: boolean) => { | ||
| AnalyticsUtil.logEvent("STATIC_URL_TOGGLE_CLICK", { |
There was a problem hiding this comment.
We just need success or failure of settings change, modal opening/closing is not a significant event. Let's remove this
| throw new Error("No current application found"); | ||
| } | ||
| if (!currentApplication) { | ||
| throw new Error("No current application found"); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (1)
49-50: Consider moving to constants file.While the hardcoded URL works fine here, consider moving
STATIC_URL_DOCS_URLto the constants/messages file for consistency with other static URLs in the codebase.Apply this diff to move it to the constants file:
In
app/client/src/ce/constants/messages.ts:+export const STATIC_URL_DOCS_URL = "https://docs.appsmith.com/build-apps/how-to-guides/configure-static-app-urls"; export const STATIC_URL_DOCS_LINK_TEXT = () => "Learn more in docs";In this file:
+import { + ..., + STATIC_URL_DOCS_LINK_TEXT, + STATIC_URL_DOCS_URL, +} from "ee/constants/messages"; ... -const STATIC_URL_DOCS_URL = - "https://docs.appsmith.com/build-apps/how-to-guides/configure-static-app-urls";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/utils/analyticsUtilTypes.ts(2 hunks)app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx(6 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/PageSettings.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 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/client/src/ce/constants/messages.tsapp/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 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/client/src/ce/constants/messages.tsapp/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 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/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
📚 Learning: 2025-04-28T16:16:02.155Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 40462
File: app/client/src/git/components/ImportOverrideModal/ImportOverrideModalView.tsx:34-40
Timestamp: 2025-04-28T16:16:02.155Z
Learning: 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:17:49.646Z
Learnt from: ashit-rath
Repo: appsmithorg/appsmith PR: 41312
File: app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx:817-883
Timestamp: 2025-10-30T07:17:49.646Z
Learning: 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). (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (4)
app/client/src/ce/constants/messages.ts (1)
1952-1952: LGTM!The constant follows the established pattern in this file and provides a clear, user-friendly message for the documentation link.
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx (3)
212-219: LGTM!The analytics event and docs navigation are properly implemented. This is a UI interaction event, so logging it at the component level is appropriate (unlike API success events which should be in sagas).
480-491: LGTM!Good UX pattern for providing contextual help. The tooltip and icon button integration follows design system conventions.
304-311: LGTM!The dependency array correctly includes all values used in the
modalOldSlugcomputation, preventing potential stale closure issues.
app/client/src/pages/AppIDE/components/AppSettings/components/GeneralSettings.tsx
Outdated
Show resolved
Hide resolved
vsvamsi1
added
ok-to-test
tomjose92
left a comment
tomjose92
left a comment
There was a problem hiding this comment.
Reviewed the changes and the docs URL is correct. LGTM
3 participants
Description
Fixes #
Issue Numberor
Fixes
Issue URLWarning
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19812434606
Commit: 4019290
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 01 Dec 2025 06:14:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.