[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 You can disable this status message by setting the
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.
| const getSecretsManager = (): SecretsManager => { | ||
| if (!SecretsManager.isInitialized()) { | ||
| return null as any; | ||
| } | ||
| return SecretsManager.getInstance(); | ||
| }; |
There was a problem hiding this comment.
Returning null as any causes silent runtime crashes.
When SecretsManager.isInitialized() returns false, this function returns null cast to SecretsManager. All exported functions then call methods on this null value (e.g., getSecretsManager().onProvidersChange(callback)), causing NPEs at runtime with confusing error messages.
Consider throwing an explicit error to fail fast with a clear message:
🐛 Proposed fix
const getSecretsManager = (): SecretsManager => {
if (!SecretsManager.isInitialized()) {
- return null as any;
+ throw new Error("SecretsManager is not initialized. Call initSecretsManager() first.");
}
return SecretsManager.getInstance();
};📝 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.
| const getSecretsManager = (): SecretsManager => { | |
| if (!SecretsManager.isInitialized()) { | |
| return null as any; | |
| } | |
| return SecretsManager.getInstance(); | |
| }; | |
| const getSecretsManager = (): SecretsManager => { | |
| if (!SecretsManager.isInitialized()) { | |
| throw new Error("SecretsManager is not initialized. Call initSecretsManager() first."); | |
| } | |
| return SecretsManager.getInstance(); | |
| }; |
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/index.ts` around lines 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.
src/lib/secretsManager/index.ts
Outdated
| const registry = new FileBasedProviderRegistry(secretsStorage); | ||
|
|
||
| await SecretsManager.initialize(registry); | ||
| console.log("!!!debug", "secretsManager initialized"); |
There was a problem hiding this comment.
Remove debug logging artifact.
The "!!!debug" prefix indicates this was temporary debugging output and should be removed before merging.
🧹 Proposed fix
await SecretsManager.initialize(registry);
- console.log("!!!debug", "secretsManager initialized");📝 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.
| console.log("!!!debug", "secretsManager initialized"); | |
| await SecretsManager.initialize(registry); |
🤖 Prompt for AI Agents
In `@src/lib/secretsManager/index.ts` at 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.
| 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>
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.