chore: Shadow PR for external contribution 36882#36969
chore: Shadow PR for external contribution 36882#36969ankitakinger wants to merge 2 commits intoreleasefrom
Conversation
…me-or-organization-name-9643' of github.com:Sai6347/appsmith into chore/external-contribution-36882
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
IDE Pod
|
/build-deploy-preview skip-tests=true |
github-actions
bot
added
the
skip-changelog
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11403133429. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
Line range hint
324-332: Update logic inhandleMenuOnCloseaffected bylastUpdatedValuechangeThe condition
lastUpdatedValue && props.application.name !== lastUpdatedValueinhandleMenuOnClosewill always be false due tolastUpdatedValuebeing an empty string constant. This breaks the update logic for the application name.To resolve this:
- Remove the condition and associated update logic if it's no longer needed.
- If the update functionality is still required, implement an alternative method to track changes in the application name.
app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx (1)
73-73: Remove unnecessary commented-out codeThe commented-out CSS line
// justify-content: space-evenly;can be removed to keep the code clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx (5 hunks)
- app/client/src/pages/Applications/ApplicationCard.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx (4)
141-141: Declaration ofisCancelledstate variableThe introduction of
isCancelledstate is appropriate for tracking the cancellation state.
191-192: UpdatinglastValidValueinonConfirmGood job updating
lastValidValueafter confirmation. This ensures the component retains the latest valid input.
228-228: Include all dependencies inuseCallbackIncluding
valueTransformin the dependency array ensures the callback stays updated with the latest transformation logic.
278-305: Add confirm and cancel icons for better user interactionThe addition of confirm and cancel icons enhances the user experience during editing.
| const [isForkApplicationModalopen, setForkApplicationModalOpen] = | ||
| useState(false); | ||
| const [lastUpdatedValue, setLastUpdatedValue] = useState(""); | ||
| const lastUpdatedValue = ""; |
There was a problem hiding this comment.
Potential loss of functionality due to state removal
The conversion of lastUpdatedValue from a state variable to a constant empty string may lead to unexpected behavior. This change removes the ability to track updates to the application name during editing.
Consider the following options:
- If tracking the last updated value is no longer necessary, remove all references to
lastUpdatedValuein the component. - If tracking is still required, revert this change and keep
lastUpdatedValueas a state variable.
💡 Codebase verification
Residual References to lastUpdatedValue Detected
The lastUpdatedValue variable is still referenced in ApplicationCard.tsx, indicating that its removal may be incomplete. This could lead to potential bugs or inconsistent behavior.
- File:
app/client/src/pages/Applications/ApplicationCard.tsx- Line 120:
const lastUpdatedValue = ""; - Line 123:
if (lastUpdatedValue && props.application.name !== lastUpdatedValue) {
- Line 120:
🔗 Analysis chain
Verify intended behavior for application name updates
The changes to lastUpdatedValue and the lack of an onTextChanged handler for EditableText suggest that real-time tracking of application name changes has been removed. This is a significant change in the component's behavior.
Please confirm if this is the intended behavior. If not, consider:
- Reinstating
lastUpdatedValueas a state variable. - Adding an
onTextChangedhandler toEditableTextto track changes. - Updating the logic in
handleMenuOnCloseto properly handle name updates.
To verify the current behavior, you can run the following script:
Also applies to: 324-332
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to lastUpdatedValue or onTextChanged
rg 'lastUpdatedValue|onTextChanged' app/client/src/pages/Applications/ApplicationCard.tsx
Length of output: 235
| ); | ||
|
|
||
| const onCancel = useCallback(() => { | ||
| onBlur && onBlur(lastValidValue); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use optional chaining for optional function calls
Replace onBlur && onBlur(lastValidValue); with onBlur?.(lastValidValue); for cleaner and more concise code.
Apply this diff:
- onBlur && onBlur(lastValidValue);
+ onBlur?.(lastValidValue);📝 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.
| onBlur && onBlur(lastValidValue); | |
| onBlur?.(lastValidValue); |
🧰 Tools
🪛 Biome
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const onCancel = useCallback(() => { | ||
| onBlur && onBlur(lastValidValue); | ||
| setIsEditing(false); | ||
| setIsInvalid(false); | ||
| setSavingState(SavingState.NOT_STARTED); | ||
| setValue(lastValidValue); | ||
| setIsCancelled(true); | ||
| }, [lastValidValue, onBlur]); | ||
|
|
||
| useEffect(() => { | ||
| if (isCancelled) { | ||
| setValue(lastValidValue); | ||
| } | ||
| }, [isCancelled, lastValidValue]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify cancellation logic by removing redundant state
The isCancelled state and its associated useEffect are unnecessary. Since setValue(lastValidValue) is already called in onCancel, you can remove isCancelled and the useEffect to simplify the code.
Apply this diff to simplify the code:
- const [isCancelled, setIsCancelled] = useState<boolean>(false);
const onCancel = useCallback(() => {
onBlur && onBlur(lastValidValue);
setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
- setIsCancelled(true);
}, [lastValidValue, onBlur]);
- useEffect(() => {
- if (isCancelled) {
- setValue(lastValidValue);
- }
- }, [isCancelled, lastValidValue]);📝 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 onCancel = useCallback(() => { | |
| onBlur && onBlur(lastValidValue); | |
| setIsEditing(false); | |
| setIsInvalid(false); | |
| setSavingState(SavingState.NOT_STARTED); | |
| setValue(lastValidValue); | |
| setIsCancelled(true); | |
| }, [lastValidValue, onBlur]); | |
| useEffect(() => { | |
| if (isCancelled) { | |
| setValue(lastValidValue); | |
| } | |
| }, [isCancelled, lastValidValue]); | |
| const onCancel = useCallback(() => { | |
| onBlur && onBlur(lastValidValue); | |
| setIsEditing(false); | |
| setIsInvalid(false); | |
| setSavingState(SavingState.NOT_STARTED); | |
| setValue(lastValidValue); | |
| }, [lastValidValue, onBlur]); |
🧰 Tools
🪛 Biome
[error] 232-232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
Deploy-Preview-URL: https://ce-36969.dp.appsmith.com |
github-actions
bot
added
Bug
2 participants
Description
Shadow PR for external contribution 36882
Corresponding EE PR:
Fixes #9643
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
EditableTextSubComponentwith cancel and confirm options.Bug Fixes
ApplicationCard, simplifying state management.Refactor
ApplicationCardcomponent.