fix: avoid ghost WhatsApp channels after QR cancel#44
Conversation
There was a problem hiding this comment.
Code Review
This pull request centralizes WhatsApp credential management by introducing a new utility file and refactoring existing logic to use it. It also adds a cleanup mechanism for cancelled login attempts in the WhatsApp login manager. The review feedback correctly identifies that the new cleanupCancelledWhatsAppLogin function uses synchronous file system operations, which could block the Electron main thread and cause UI unresponsiveness. It is recommended to convert this function to be asynchronous and update the corresponding call sites and unit tests to handle the resulting promises.
| export function cleanupCancelledWhatsAppLogin(accountId: string): boolean { | ||
| const accountDir = getWhatsAppAccountCredentialsDir(accountId); | ||
| if (!existsSync(accountDir)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (existsSync(getWhatsAppCredsFilePathForDir(accountDir))) { | ||
| return false; | ||
| } | ||
|
|
||
| rmSync(accountDir, { recursive: true, force: true }); | ||
|
|
||
| const credentialsDir = getWhatsAppCredentialsDir(); | ||
| if (existsSync(credentialsDir) && readdirSync(credentialsDir).length === 0) { | ||
| rmSync(credentialsDir, { recursive: true, force: true }); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The function cleanupCancelledWhatsAppLogin uses synchronous file system operations (existsSync, rmSync, readdirSync). Since this function is called from an async context, using synchronous I/O can block the main thread. It's better to use the asynchronous versions from fs/promises to avoid potential UI freezes or unresponsiveness. This also makes the file I/O style consistent within the file.
This change will require:
- Updating the imports in this file (removing sync
fsmethods and addingrmfromfs/promises). - Updating the call site in
whatsapp-login.tstoawaitthis function. - Updating the corresponding test in
tests/unit/whatsapp-credentials.test.ts.
export async function cleanupCancelledWhatsAppLogin(accountId: string): Promise<boolean> {
const accountDir = getWhatsAppAccountCredentialsDir(accountId);
if (!(await fileExists(accountDir))) {
return false;
}
if (await fileExists(getWhatsAppCredsFilePathForDir(accountDir))) {
return false;
}
await rm(accountDir, { recursive: true, force: true });
const credentialsDir = getWhatsAppCredentialsDir();
try {
// Check if parent directory is now empty and remove it.
if ((await readdir(credentialsDir)).length === 0) {
await rm(credentialsDir, { recursive: true, force: true });
}
} catch {
// Ignore errors, e.g. if credentialsDir doesn't exist anymore.
}
return true;
}
electron/utils/whatsapp-login.ts
Outdated
| if (cleanupAccountId && cleanupCancelledWhatsAppLogin(cleanupAccountId)) { | ||
| console.log(`[WhatsAppLogin] Cleaned up cancelled login state for ${cleanupAccountId}`); | ||
| } |
There was a problem hiding this comment.
To accompany the suggested change to make cleanupCancelledWhatsAppLogin asynchronous, this call should use await.
| if (cleanupAccountId && cleanupCancelledWhatsAppLogin(cleanupAccountId)) { | |
| console.log(`[WhatsAppLogin] Cleaned up cancelled login state for ${cleanupAccountId}`); | |
| } | |
| if (cleanupAccountId && (await cleanupCancelledWhatsAppLogin(cleanupAccountId))) { | |
| console.log(`[WhatsAppLogin] Cleaned up cancelled login state for ${cleanupAccountId}`); | |
| } |
| expect(cleanupCancelledWhatsAppLogin('cancelled')).toBe(true); | ||
| expect(cleanupCancelledWhatsAppLogin('existing')).toBe(false); |
There was a problem hiding this comment.
If cleanupCancelledWhatsAppLogin is converted to an async function as suggested, these assertions will need to be updated to handle the returned Promise.
| expect(cleanupCancelledWhatsAppLogin('cancelled')).toBe(true); | |
| expect(cleanupCancelledWhatsAppLogin('existing')).toBe(false); | |
| await expect(cleanupCancelledWhatsAppLogin('cancelled')).resolves.toBe(true); | |
| await expect(cleanupCancelledWhatsAppLogin('existing')).resolves.toBe(false); |
Summary:
Validation: