fix: Leaving editable tab in error state #37981
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
hetunandu
requested review from
alex-golovanov,
ankitakinger and
albinAppsmith
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12177428603. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/client/src/IDE/Components/EditableName/EditableName.tsx (2)
75-86: Consider error state cleanup timingThe current implementation clears validation errors after setting the name. Consider clearing the error state before setting the new name to prevent potential flash of error state during state updates.
} else { // Exit edit mode and revert name + setValidationError(null); setEditableName(name); - setValidationError(null); exitEditing(); }
Line range hint
143-156: Improve focus management implementationThe current setTimeout-based focus management is fragile and could lead to race conditions. Consider using a more robust approach such as:
- MutationObserver to detect when focus retention completes
- RAF (requestAnimationFrame) for more reliable timing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/IDE/Components/EditableName/EditableName.test.tsx(2 hunks)app/client/src/IDE/Components/EditableName/EditableName.tsx(1 hunks)
| @@ -151,7 +151,7 @@ describe("EditableName", () => { | |||
|
|
|||
| await userEvent.click(document.body); | |||
|
|
|||
| expect(getByRole("tooltip").textContent).toEqual(validationError); | |||
| expect(getByRole("tooltip").textContent).toEqual(""); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for missing scenarios
While the current changes correctly verify the tooltip behavior, consider adding tests for:
- The case where the name hasn't changed (should call exitWithoutSaving)
- The transition timing of error states to ensure proper cleanup
Example test structure:
test("exits without saving when name hasn't changed", () => {
const { exitEditing, getByRole, onNameSave } = setup({
isEditing: true,
});
const inputElement = getByRole("textbox");
// Set same name as original
fireEvent.change(inputElement, {
target: { value: name },
});
fireEvent.keyUp(inputElement, KEY_CONFIG.ENTER);
expect(exitEditing).toHaveBeenCalled();
expect(onNameSave).not.toHaveBeenCalled();
});Also applies to: 190-190
|
Deploy-Preview-URL: https://ce-37981.dp.appsmith.com |
|
/build-deploy-preview |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12179088518. |
|
Deploy-Preview-URL: https://ce-37981.dp.appsmith.com |
| @@ -29,6 +30,10 @@ interface EditableTextProps { | |||
| inputTestId?: string; | |||
| } | |||
|
|
|||
| export const Text = styled(ADSText)` | |||
There was a problem hiding this comment.
I'd move this to styles.
Description
fixes the editable tab handling of error states. Earlier the tab would stay in error state (with tooltip active) when you leave (focus out or press esc) the input with an invalid value.
Now it would revert the context back to the old name and let go of the error state.
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/12179074325
Commit: 0ba7f5f
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 05 Dec 2024 14:27:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
EditableNamecomponent, improving user experience during name editing.Bug Fixes
Documentation