chore: Client side changes to send organizationId in verify email flow#39898
chore: Client side changes to send organizationId in verify email flow#39898trishaanand merged 5 commits intoreleasefrom
Conversation
WalkthroughThis pull request introduces session token validation in the client’s initialization flow. The Changes
Sequence Diagram(s)sequenceDiagram
participant Saga as EagerPageInitSaga
participant Utils as SessionUtils
participant API as Token Validation API
participant Sentry as Sentry Logger
Saga->>Utils: Call validateSessionToken()
alt Session token exists
Utils->>API: Validate session token
alt API returns success
API-->>Utils: Success response
Utils->>Saga: Return true (token removed)
else API returns error/invalid
API-->>Utils: Error response
Utils->>Saga: Return false
end
else Session token missing
Utils->>Saga: Return false
end
alt Exception thrown in Utils
Saga->>Sentry: captureException(error)
Saga->>log: log.error(error)
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/pages/UserAuth/VerifyUser.tsx (1)
53-64: Improved form input handling with explicit fields.The refactoring from iterating over query parameters to explicitly defining inputs is a cleaner implementation. It provides better control over which parameters are sent in the form submission.
Consider storing the redirectUrl value in a variable similar to email, token, and organizationId to avoid multiple calls to queryParams.get().
const token = queryParams.get("token"); const email = queryParams.get("email"); const organizationId = queryParams.get("organizationId"); + const redirectUrl = queryParams.get("redirectUrl"); // Later in the JSX - {queryParams.get("redirectUrl") && ( + {redirectUrl && ( <input name="redirectUrl" type="hidden" - value={queryParams.get("redirectUrl") || ""} + value={redirectUrl || ""} /> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/UserAuth/VerifyUser.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/pages/UserAuth/VerifyUser.tsx (1)
22-22: Added new query parameter retrieval for organizationId.This change correctly adds extraction of the organizationId from query parameters, aligning with the PR objective to include organizationId in the verify email flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/utils/SessionUtils.ts (1)
12-19: Use the defined constant instead of string literalThe function should use the SESSION_TOKEN_PARAM constant instead of the hardcoded string.
const urlParams = new URLSearchParams(window.location.search); - const sessionToken = urlParams.get("sessionToken"); + const sessionToken = urlParams.get(SESSION_TOKEN_PARAM);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/client/src/sagas/InitSagas.ts(2 hunks)app/client/src/utils/SessionUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/client/src/sagas/InitSagas.ts (1)
app/client/src/utils/SessionUtils.ts (1)
validateSessionToken(12-42)
🔇 Additional comments (7)
app/client/src/sagas/InitSagas.ts (2)
95-95: Import validation utility from SessionUtilsGood addition of the import statement for the session token validation functionality.
466-473: Properly handle session token validation errorsThe try-catch block correctly validates the session token while ensuring errors don't block initialization. Good use of error logging and Sentry for error tracking.
app/client/src/utils/SessionUtils.ts (5)
1-4: Good module structure with clear constant definitionThe imports and constant definition are well-organized.
6-11: Well-documented function with clear JSDocThe JSDoc comments clearly explain the purpose and return value of the function.
31-39: Good URL handling with proper state managementProperly removing the session token from the URL after validation is a good security practice.
39-42: Robust error handlingThe catch block appropriately handles any errors during the validation process by returning false, ensuring the application flow isn't disrupted.
21-29:Details
✅ Verification successful
Verify the API response type conversion
The type assertion being used here suggests a potential mismatch between the API response and the expected type.
🏁 Script executed:
#!/bin/bash # Description: Check the API response type for session validation endpoint # Find the API response type definition rg -A 5 "ApiResponse.*boolean" --type tsLength of output: 1208
API Response Conversion Verified
The type conversion inSessionUtils.tsis consistent with its usage throughout the codebase (see similar patterns inJSLibrarySaga.tsandGitSyncSagas.ts). The double assertion throughunknowntoApiResponse<boolean>is intentional to bridge the untyped API response without causing type conflicts. No changes are needed here.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
#39898) ## 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="" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Tests have not run on the HEAD a1634ec yet > <hr>Tue, 25 Mar 2025 09:26:39 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a session token validation step during initialization to ensure the app handles token issues gracefully. - Enhanced error management during startup, allowing the app to continue loading smoothly even if session token validation encounters problems. - Added a dedicated utility for managing session tokens extracted from URL parameters. - Expanded the `VerifyUser` component to accept an additional `organizationId` property for improved user authentication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Albin <albin@appsmith.com>
appsmithorg#39898) ## 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="" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Tests have not run on the HEAD a1634ec yet > <hr>Tue, 25 Mar 2025 09:26:39 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a session token validation step during initialization to ensure the app handles token issues gracefully. - Enhanced error management during startup, allowing the app to continue loading smoothly even if session token validation encounters problems. - Added a dedicated utility for managing session tokens extracted from URL parameters. - Expanded the `VerifyUser` component to accept an additional `organizationId` property for improved user authentication. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Albin <albin@appsmith.com>
3 participants
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 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=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD a1634ec yet
Tue, 25 Mar 2025 09:26:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
VerifyUsercomponent to accept an additionalorganizationIdproperty for improved user authentication.