Skip to content

chore: Shadow PR for external contribution 36882 #36969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ankitakinger wants to merge 2 commits into from
Closed

chore: Shadow PR for external contribution 36882 #36969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb04f62
fix: empty space is clickable when user edits app name or organisatio…
Sai6347 Oct 15, 2024
9a75b07
Merge branch 'fix/bug-empty-space-is-clickable-when-user-edits-app-na…
ankitakinger Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 49 additions & 5 deletions app/client/packages/design-system/ads-old/src/EditableTextSubComponent/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const TextContainer = styled.div<{
}>`
display: flex;
align-items: center;
// justify-content: space-evenly;
.bp3-editable-text.bp3-editable-text-editing::before,
.bp3-editable-text.bp3-disabled::before {
display: none;
Expand All @@ -80,7 +81,6 @@ const TextContainer = styled.div<{
color: var(--ads-editable-text-subcomponent-default-text-color);
overflow: hidden;
text-overflow: ellipsis;
${(props) => (props.isEditing ? "display: none" : "display: block")};
width: fit-content !important;
min-width: auto !important;
line-height: inherit !important;
Expand Down Expand Up @@ -138,6 +138,7 @@ export const EditableTextSubComponent = React.forwardRef(
const [value, setValue] = useState(defaultValue);
const [lastValidValue, setLastValidValue] = useState(defaultValue);
const [changeStarted, setChangeStarted] = useState<boolean>(false);
const [isCancelled, setIsCancelled] = useState<boolean>(false);

useEffect(() => {
if (isError) {
Expand Down Expand Up @@ -187,6 +188,8 @@ export const EditableTextSubComponent = React.forwardRef(

if (finalVal && finalVal !== defaultValue) {
onBlur && onBlur(finalVal);
setLastValidValue(finalVal);
setValue(finalVal);
}

setIsEditing(false);
Expand Down Expand Up @@ -215,17 +218,31 @@ export const EditableTextSubComponent = React.forwardRef(
const error = errorMessage ? errorMessage : false;

if (!error && finalVal !== "") {
setLastValidValue(finalVal);
onTextChanged && onTextChanged(finalVal);
}

setValue(finalVal);
setIsInvalid(error);
setChangeStarted(true);
},
[inputValidation, onTextChanged],
[inputValidation, onTextChanged, valueTransform],
);

const onCancel = useCallback(() => {
onBlur && onBlur(lastValidValue);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)

setIsEditing(false);
setIsInvalid(false);
setSavingState(SavingState.NOT_STARTED);
setValue(lastValidValue);
setIsCancelled(true);
}, [lastValidValue, onBlur]);

useEffect(() => {
if (isCancelled) {
setValue(lastValidValue);
}
}, [isCancelled, lastValidValue]);

Comment on lines +231 to +245
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)

const iconName =
!isEditing &&
savingState === SavingState.NOT_STARTED &&
Expand All @@ -251,15 +268,42 @@ export const EditableTextSubComponent = React.forwardRef(
className={props.className}
disabled={!isEditing}
isEditing={isEditing}
onCancel={onConfirm}
onChange={onInputchange}
onConfirm={onConfirm}
placeholder={props.placeholder || defaultValue}
ref={ref}
selectAllOnFocus
value={value}
/>

{value && !props.hideEditIcon && isEditing ? (
<>
<span
className="pl-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onConfirm(value);
}}
>
<Icon className="cursor-pointer" name="check-line" size="md" />
</span>
<span
className="px-1"
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
onCancel();
}}
>
<Icon
className="cursor-pointer"
name="close-circle-line"
size="md"
/>
</span>
</>
) : null}

{savingState === SavingState.STARTED ? (
<Spinner size="md" />
) : value && !props.hideEditIcon && iconName ? (
Expand Down
5 changes: 1 addition & 4 deletions app/client/src/pages/Applications/ApplicationCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export function ApplicationCard(props: ApplicationCardProps) {
const [isDeleting, setIsDeleting] = useState(false);
const [isForkApplicationModalopen, setForkApplicationModalOpen] =
useState(false);
const [lastUpdatedValue, setLastUpdatedValue] = useState("");
const lastUpdatedValue = "";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. If tracking the last updated value is no longer necessary, remove all references to lastUpdatedValue in the component.
  2. If tracking is still required, revert this change and keep lastUpdatedValue as 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) {
🔗 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:

  1. Reinstating lastUpdatedValue as a state variable.
  2. Adding an onTextChanged handler to EditableText to track changes.
  3. Updating the logic in handleMenuOnClose to 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 dispatch = useDispatch();

const applicationId = props.application?.id;
Expand Down Expand Up @@ -371,9 +371,6 @@ export function ApplicationCard(props: ApplicationCardProps) {
name: value,
});
}}
onTextChanged={(value: string) => {
setLastUpdatedValue(value);
}}
placeholder={"Edit text input"}
savingState={
isSavingName ? SavingState.STARTED : SavingState.NOT_STARTED
Expand Down