chore: Fix appsmith store hydration from localstorage in view mode #39375
Conversation
WalkthroughThe pull request updates two core engine modules. In the Changes
Sequence Diagram(s)sequenceDiagram
participant CE as AppEditorEngine.completeChore
participant CU as CurrentUser
participant L as Logger
CE->>CU: Check currentUser?.email
alt Email exists
CE->>CE: Proceed with chore completion
else Email missing
CE->>L: Log error for missing branch condition
end
sequenceDiagram
participant LAD as AppEngine.loadAppData
participant App as Application Data
participant S as selectGitApplicationCurrentBranch
participant PSA as getPersistentAppStore
LAD->>App: Fetch current application
LAD->>S: Retrieve current branch
alt Branch found
LAD->>PSA: Update persistent store with branch from selector
else No branch found
LAD->>PSA: Update persistent store with branch from payload
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![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/entities/Engine/index.ts (1)
116-125: Consider adding error handling for the currentBranch selection.The currentBranch selection could fail, but there's no error handling in place.
const currentBranch: string | undefined = yield select( selectGitApplicationCurrentBranch, + ).catch(error => { + log.error("Error selecting current branch:", error); + return undefined; + });app/client/src/entities/Engine/AppEditorEngine.ts (1)
305-315: Good addition of error logging!The error logging provides better visibility into branch persistence failures. However, consider structuring the error message for better readability.
- log.error( - `There was an error setting the latest git branch in local - userEmail: ${!!currentUser?.email}, applicationId: ${currentApplication?.baseId}, branch: ${currentBranch}`, - ); + log.error( + "Failed to set latest git branch in local storage:", + { + hasUserEmail: !!currentUser?.email, + applicationId: currentApplication?.baseId, + branch: currentBranch, + } + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/entities/Engine/AppEditorEngine.ts(2 hunks)app/client/src/entities/Engine/index.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/entities/Engine/index.ts (2)
2-2: LGTM! New imports align with the PR objective.The addition of
selectGitApplicationCurrentBranchandgetPersistentAppStoreimports supports the store hydration fix.Also applies to: 22-23
121-125:❓ Verification inconclusive
Verify the store hydration behavior.
The store update using
getPersistentAppStoreneeds verification to ensure it correctly handles all scenarios:
- When both branch and currentBranch are undefined
- When switching between branches
🏁 Script executed:
#!/bin/bash # Search for all getPersistentAppStore usages to verify consistent behavior rg "getPersistentAppStore" -A 3Length of output: 894
Action Required: Verify Store Hydration for Branch Fallbacks
Please ensure that the store update using
getPersistentAppStoreworks as expected under these scenarios:
- Undefined Branches: Confirm that when both
branchandcurrentBranchare undefined, the function gracefully returns a valid persistent store.- Branch Switching: Validate that providing a specific branch (via
branch || currentBranch) correctly computes and uses the intended store name, ensuring consistency when switching between branches.Double-check that the implementation in
app/client/src/constants/AppConstants.ts(which definesgetPersistentAppStore) aligns with these expectations.
ashit-rath
added
ok-to-test
Description
Context - https://theappsmith.slack.com/archives/C0134BAVDB4/p1740027639910039?thread_ts=1739904050.960349&cid=C0134BAVDB4
The app store wasn't getting hydrated from local storage in view mode because of the changes in the PR #39255
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.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13429852595
Commit: d9c5c62
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 20 Feb 2025 08:36:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?