[DB-29] added IPC methods for secretsManager#277
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR introduces a functional API layer for secrets management by creating a new index.ts file that exports 10 public functions (initSecretsManager, subscribeToProvidersChange, setSecretProviderConfig, getSecretValue, etc.) wrapping an underlying SecretsManager singleton. The main/events.js file is refactored to import and use these functions instead of directly instantiating concrete classes. IPC channel handlers are updated to follow a "secretsManager:\*" naming convention and delegate to the corresponding exported functions. The public getSecretsManager() helper is removed from secretsManager.ts, and a type adjustment is made in SecretsManagerEncryptedStorage to treat stored data as a Record map structure. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/secretsManager/index.ts`:
- Around line 7-12: The current getSecretsManager function returns null when
SecretsManager.isInitialized() is false, causing NPEs elsewhere; change
getSecretsManager to throw a clear, descriptive Error (e.g., "SecretsManager not
initialized - call init first") instead of returning null so callers like
getSecretsManager().onProvidersChange(...) fail fast; update any docs/comments
if needed and keep the checks around SecretsManager.isInitialized(),
SecretsManager.getInstance(), and calls to onProvidersChange/getSecret etc. to
reflect this contract.
- Line 23: Remove the temporary debug logging call console.log("!!!debug",
"secretsManager initialized") from src/lib/secretsManager/index.ts; either
delete that line or replace it with the project's proper logger (e.g.,
processLogger.debug or similar) consistent with surrounding logging conventions
so no stray debug output remains.
In `@src/main/events.js`:
- Around line 289-296: subscribeToProvidersChange's unsubscribe function is
being discarded, causing duplicate subscriptions; capture the unsubscribe
returned by subscribeToProvidersChange inside the ipcMain.handle for
"secretsManager:subscribeToProvidersChange" and return it to the renderer so it
can call it to remove the listener (i.e., const unsubscribe =
subscribeToProvidersChange(...); return () => unsubscribe()). Alternatively, if
you prefer a singleton subscription, store the unsubscribe in a module-scoped
variable (e.g., currentProvidersUnsub) and reuse/replace it when ipcMain.handle
is invoked to prevent multiple subscriptions; references:
ipcMain.handle("secretsManager:subscribeToProvidersChange"),
subscribeToProvidersChange, webAppWindow.webContents.send, and the
"secretsManager:providersChanged" channel.
| ipcMain.handle("secretsManager:subscribeToProvidersChange", () => { | ||
| subscribeToProvidersChange((providers) => { | ||
| webAppWindow?.webContents.send( | ||
| "secretsManager:providersChanged", | ||
| providers | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Unsubscribe callback is discarded, risking duplicate subscriptions and memory leaks.
subscribeToProvidersChange() returns an unsubscribe function, but it's not being returned or stored. If the renderer calls this IPC handler multiple times, each call creates a new subscription without a way to clean up the previous ones.
Consider either:
- Tracking subscription state to prevent duplicates, or
- Returning the unsubscribe mechanism to the renderer
🔧 Proposed fix (option 1: singleton subscription)
+let providersChangeUnsubscribe = null;
+
ipcMain.handle("secretsManager:subscribeToProvidersChange", () => {
+ // Prevent duplicate subscriptions
+ if (providersChangeUnsubscribe) {
+ return;
+ }
- subscribeToProvidersChange((providers) => {
+ providersChangeUnsubscribe = subscribeToProvidersChange((providers) => {
webAppWindow?.webContents.send(
"secretsManager:providersChanged",
providers
);
});
});🤖 Prompt for AI Agents
In `@src/main/events.js` around lines 289 - 296, subscribeToProvidersChange's
unsubscribe function is being discarded, causing duplicate subscriptions;
capture the unsubscribe returned by subscribeToProvidersChange inside the
ipcMain.handle for "secretsManager:subscribeToProvidersChange" and return it to
the renderer so it can call it to remove the listener (i.e., const unsubscribe =
subscribeToProvidersChange(...); return () => unsubscribe()). Alternatively, if
you prefer a singleton subscription, store the unsubscribe in a module-scoped
variable (e.g., currentProvidersUnsub) and reuse/replace it when ipcMain.handle
is invoked to prevent multiple subscriptions; references:
ipcMain.handle("secretsManager:subscribeToProvidersChange"),
subscribeToProvidersChange, webAppWindow.webContents.send, and the
"secretsManager:providersChanged" channel.
* handled secret manager errors * fix: return types * added initialization error handling * fix: test return * [DB-21] added variable fetching flow and awsSecretsManagerProvider (#266) * convert into generic types * fix: types * fix: working types --------- Co-authored-by: Sahil Gupta <sahil865gupta@gmail.com>
…op-app into secrets/storage-listener
…stly-desktop-app into ipc-layer
…op-app into ipc-layer
* [DB-48] added persistence to secret values * handled editing variables using secret IDs * handled errors on secrets fetch * added reset call on initialization * fix: storing userId in the file * fix: handle invalid userId * rmove unused import * fix: setSecrets write
1 participant
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.