Skip to content

fix: remove async from void onPress callback (SonarCloud bug) #174

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

Open
alichherawalla wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: remove async from void onPress callback (SonarCloud bug) #174

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e927a74
fix: remove async from onPress callback to satisfy void return type
alichherawalla Mar 14, 2026
489ebc8
fix: add .catch() to handle removeServer promise rejection
alichherawalla Mar 14, 2026
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
6 changes: 4 additions & 2 deletions src/screens/RemoteServersScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,11 @@ export const RemoteServersScreen: React.FC = () => {
{
text: 'Delete',
style: 'destructive',
onPress: async () => {
onPress: () => {
if (activeServerId === server.id) setActiveServerId(null);
await remoteServerManager.removeServer(server.id);
remoteServerManager.removeServer(server.id).catch(error =>
setAlertState(showAlert('Deletion Failed', error instanceof Error ? error.message : 'An unknown error occurred.'))
);
Comment on lines +109 to +111
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 14, 2026

Choose a reason for hiding this comment

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

medium

There's a potential race condition here. If the component unmounts after the delete operation starts but before it completes (e.g., the user navigates away quickly), setAlertState could be called on an unmounted component. This would result in a React warning about memory leaks.

To prevent this, you can use a useRef to track if the component is mounted.

First, add this to your component:

const isMounted = React.useRef(true);
React.useEffect(() => {
  return () => {
    isMounted.current = false;
  };
}, []);

Then, check this ref in your .catch block before calling setAlertState:

remoteServerManager.removeServer(server.id).catch(error => {
  if (isMounted.current) {
    setAlertState(showAlert('Deletion Failed', error instanceof Error ? error.message : 'An unknown error occurred.'));
  }
});

},
Comment on lines +107 to 112
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 14, 2026

Choose a reason for hiding this comment

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

high

While removing async/await fixes the SonarCloud issue with the onPress function signature, it introduces a potential for silent failures. The removeServer function is async and can reject (e.g., if deleting the API key from the keychain fails). By not handling the returned promise, any rejection will be swallowed, and the user won't know the deletion failed.

This violates a general project rule: "Use .catch() to handle promise rejections instead."

To fix this, you should add a .catch() block to handle potential errors and inform the user, for example by showing another alert.

          onPress: () => {
            if (activeServerId === server.id) setActiveServerId(null);
            remoteServerManager.removeServer(server.id).catch(error =>
              setAlertState(showAlert('Deletion Failed', error instanceof Error ? error.message : 'An unknown error occurred.'))
            );
          },
References
  1. Avoid using the void operator for unawaited promises to adhere to the project's no-void ESLint rule. Use .catch() to handle promise rejections instead.

Copy link
Copy Markdown
Owner Author

@alichherawalla alichherawalla Mar 14, 2026

Choose a reason for hiding this comment

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

Good catch — added .catch() with an error alert so the user sees a "Deletion Failed" message if removeServer rejects. Pushed in 489ebc8.

},
]
Expand Down
Loading