fix: remove async from void onPress callback (SonarCloud bug)#174
fix: remove async from void onPress callback (SonarCloud bug)#174alichherawalla wants to merge 2 commits into
Conversation
SonarCloud flagged a Promise-returning function passed to onPress which expects () => void. Dropping async/await since removeServer is fire-and-forget in this context. Authored-by: GLM <work@wednesday.is>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a SonarCloud quality gate issue by refactoring a server deletion callback. The change ensures that an Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a SonarCloud bug by removing async/await from an onPress callback, which expects a void returning function. However, this change introduces an unhandled promise, which could lead to silent failures if the server deletion process fails. I've kept the review comment with a suggestion to properly handle promise rejections by adding a .catch() block, which aligns with the project's general rules and improves error handling.
| onPress: () => { | ||
| if (activeServerId === server.id) setActiveServerId(null); | ||
| await remoteServerManager.removeServer(server.id); | ||
| remoteServerManager.removeServer(server.id); | ||
| }, |
There was a problem hiding this comment.
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
- Avoid using the
voidoperator for unawaited promises to adhere to the project'sno-voidESLint rule. Use.catch()to handle promise rejections instead.
There was a problem hiding this comment.
Good catch — added .catch() with an error alert so the user sees a "Deletion Failed" message if removeServer rejects. Pushed in 489ebc8.
Greptile SummaryThis PR removes Key observations:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant RemoteServersScreen
participant CustomAlert
participant RemoteServerStore
participant RemoteServerManager
participant KeyChain
User->>RemoteServersScreen: Tap "Delete"
RemoteServersScreen->>CustomAlert: showAlert (confirm dialog)
User->>CustomAlert: Tap "Delete" (confirm)
CustomAlert->>RemoteServersScreen: onPress()
RemoteServersScreen->>RemoteServerStore: setActiveServerId(null) [if active]
RemoteServersScreen->>RemoteServerManager: removeServer(id) [fire-and-forget]
Note over RemoteServersScreen,RemoteServerManager: Promise returned but not awaited
RemoteServerManager->>RemoteServerManager: providerRegistry.unregisterProvider(id)
RemoteServerManager->>KeyChain: removeApiKey(id)
alt KeyChain success
KeyChain-->>RemoteServerManager: resolved
RemoteServerManager->>RemoteServerStore: removeServer(id)
else KeyChain failure
KeyChain-->>RemoteServerManager: rejected
Note over RemoteServerManager: Unhandled rejection — store.removeServer never called
end
Last reviewed commit: e927a74 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onPress: () => { | ||
| if (activeServerId === server.id) setActiveServerId(null); | ||
| await remoteServerManager.removeServer(server.id); | ||
| remoteServerManager.removeServer(server.id); |
There was a problem hiding this comment.
Unhandled promise rejection risk
remoteServerManager.removeServer is async and internally awaits removeApiKey. If removeApiKey rejects (e.g. Keychain error), the returned Promise will be silently dropped here, producing an unhandled promise rejection. The rest of the codebase already uses a consistent .catch(() => {}) pattern for fire-and-forget async calls (e.g. lines 44 and 88), so this should follow the same convention.
| remoteServerManager.removeServer(server.id); | |
| remoteServerManager.removeServer(server.id).catch(() => {}); |
There was a problem hiding this comment.
Addressed — went with Gemini's suggestion of showing an error alert rather than a silent .catch(() => {}) since this is a user-initiated action and they should know if deletion failed. Pushed in 489ebc8.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 86.46% 86.44% -0.02%
==========================================
Files 214 214
Lines 10515 10516 +1
Branches 2818 2819 +1
==========================================
- Hits 9092 9091 -1
- Misses 772 773 +1
- Partials 651 652 +1
🚀 New features to boost your workflow:
|
Addresses Gemini and Greptile review comments — shows error alert on deletion failure instead of silently swallowing the rejection. Authored-by: GLM <work@wednesday.is>
|
/gemini review |
|
There was a problem hiding this comment.
Code Review
This pull request correctly resolves the SonarCloud issue by removing the async keyword from the onPress callback, ensuring it returns void as expected. You've also improved robustness by adding error handling for the server deletion process, which now informs the user of failures via an alert. I have one suggestion to prevent a potential race condition that could cause a React warning if the component unmounts during the deletion.
| remoteServerManager.removeServer(server.id).catch(error => | ||
| setAlertState(showAlert('Deletion Failed', error instanceof Error ? error.message : 'An unknown error occurred.')) | ||
| ); |
There was a problem hiding this comment.
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.'));
}
});1 participant
Summary
async/awaitfrom the delete serveronPresscallback inRemoteServersScreen() => voidwas expectedremoveServeris fire-and-forget in this context, so dropping async is safeTest plan
🤖 Generated with Claude Code