add clear history/storage for web widgets #2383
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/app/view/webview/webview.tsx (1)
414-428: Verify unintended whitespace changes.These lines appear to contain only whitespace changes in the diff. If these are unintentional formatting changes, consider removing them to keep the PR focused on the feature implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
emain/emain.ts(1 hunks)emain/preload.ts(1 hunks)frontend/app/view/webview/webview.tsx(3 hunks)frontend/types/custom.d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/webview/webview.tsx (2)
frontend/app/store/global.ts (2)
globalStore(783-783)getApi(772-772)frontend/util/util.ts (1)
fireAndForget(411-411)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
emain/emain.ts (1)
376-376: Consider scope of clearStorageData().The
clearStorageData()call without arguments clears all storage types (appcache, cookies, filesystem, indexdb, localstorage, shadercache, websql, serviceworkers, cachestorage). Verify this is the intended behavior, as it may clear more than expected.If more granular control is needed, consider specifying which storage types to clear:
await wc.session.clearStorageData({ storages: ['cookies', 'localstorage', 'indexdb', 'websql'] });Confirm that clearing all storage types is the desired behavior for this feature.
frontend/types/custom.d.ts (1)
106-106: LGTM!The type definition correctly declares the new API method with an appropriate signature matching the implementation.
emain/preload.ts (1)
56-56: LGTM!The API method is correctly exposed via contextBridge and properly uses
ipcRenderer.invokefor the async IPC call, consistent with other methods in this file.frontend/app/view/webview/webview.tsx (1)
498-504: LGTM!The
clearHistory()method correctly invokes the webview'sclearHistoryAPI with appropriate error handling.
| electron.ipcMain.handle("clear-webview-storage", async (event, webContentsId: number) => { | ||
| try { | ||
| const wc = electron.webContents.fromId(webContentsId); | ||
| if (wc && wc.session) { | ||
| await wc.session.clearStorageData(); | ||
| console.log("Cleared cookies and storage for webContentsId:", webContentsId); | ||
| } | ||
| } catch (e) { | ||
| console.error("Failed to clear cookies and storage:", e); | ||
| throw e; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Validate webContentsId parameter.
The handler doesn't validate the webContentsId parameter before using it. If a malformed or negative value is passed, electron.webContents.fromId() will return undefined, but the code should explicitly validate the input.
Consider adding parameter validation:
electron.ipcMain.handle("clear-webview-storage", async (event, webContentsId: number) => {
try {
+ if (!webContentsId || typeof webContentsId !== 'number' || webContentsId <= 0) {
+ throw new Error("Invalid webContentsId");
+ }
const wc = electron.webContents.fromId(webContentsId);
if (wc && wc.session) {
await wc.session.clearStorageData();
console.log("Cleared cookies and storage for webContentsId:", webContentsId);
+ } else {
+ console.warn("WebContents not found for webContentsId:", webContentsId);
}
} catch (e) {
console.error("Failed to clear cookies and storage:", e);
throw e;
}
});📝 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.
| electron.ipcMain.handle("clear-webview-storage", async (event, webContentsId: number) => { | |
| try { | |
| const wc = electron.webContents.fromId(webContentsId); | |
| if (wc && wc.session) { | |
| await wc.session.clearStorageData(); | |
| console.log("Cleared cookies and storage for webContentsId:", webContentsId); | |
| } | |
| } catch (e) { | |
| console.error("Failed to clear cookies and storage:", e); | |
| throw e; | |
| } | |
| }); | |
| electron.ipcMain.handle("clear-webview-storage", async (event, webContentsId: number) => { | |
| try { | |
| if (!webContentsId || typeof webContentsId !== 'number' || webContentsId <= 0) { | |
| throw new Error("Invalid webContentsId"); | |
| } | |
| const wc = electron.webContents.fromId(webContentsId); | |
| if (wc && wc.session) { | |
| await wc.session.clearStorageData(); | |
| console.log("Cleared cookies and storage for webContentsId:", webContentsId); | |
| } else { | |
| console.warn("WebContents not found for webContentsId:", webContentsId); | |
| } | |
| } catch (e) { | |
| console.error("Failed to clear cookies and storage:", e); | |
| throw e; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In emain/emain.ts around lines 372 to 383, the IPC handler uses webContentsId
without validating it; add explicit parameter validation (ensure webContentsId
is a number, finite, and a positive integer) before calling
electron.webContents.fromId(), and return or throw a descriptive error when
validation fails; after validating, continue to resolve the webContents and
check wc exists and has a session before clearing storage, and include brief
error messages when validation or lookup fails so malformed or negative values
are handled safely.
| async clearCookiesAndStorage() { | ||
| try { | ||
| const webContentsId = this.webviewRef.current?.getWebContentsId(); | ||
| if (webContentsId) { | ||
| await getApi().clearWebviewStorage(webContentsId); | ||
| } | ||
| } catch (e) { | ||
| console.error("Failed to clear cookies and storage", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add user feedback for clearCookiesAndStorage operation.
The clearCookiesAndStorage() method clears storage for the current webview instance but the menu label says "All Web Widgets". This creates a discrepancy between the UI text and the actual behavior. Additionally, users receive no feedback when the operation completes or fails.
Consider one of these solutions:
Option 1: Update the menu label to reflect the actual behavior:
- label: "Clear Cookies and Storage (All Web Widgets)",
+ label: "Clear Cookies and Storage (This Widget)",Option 2: If clearing all web widgets is intended, you'll need to iterate over all webview instances and clear each one's storage. In that case, you should also add user feedback:
async clearCookiesAndStorage() {
try {
const webContentsId = this.webviewRef.current?.getWebContentsId();
if (webContentsId) {
await getApi().clearWebviewStorage(webContentsId);
// Add success notification
console.log("Successfully cleared cookies and storage");
}
} catch (e) {
console.error("Failed to clear cookies and storage", e);
// Consider showing user-facing error notification
}
}🤖 Prompt for AI Agents
frontend/app/view/webview/webview.tsx lines 506-515: the clearCookiesAndStorage
method behavior (clears current webview only) mismatches the menu label "All Web
Widgets" and provides no user feedback; either change the menu label to indicate
it only clears the current widget, or implement the intended "clear all"
behavior by enumerating all webview instances (or retrieving all webContentsIds
from the managing component/state) and calling getApi().clearWebviewStorage for
each id, awaiting all operations and collecting errors, then surface a
user-facing notification on success (e.g., "Cleared cookies and storage for N
widgets") and show an error notification if any failures occur while still
attempting to clear others; ensure console.error remains for debug logging.
| { | ||
| type: "separator", | ||
| }, | ||
| { | ||
| label: "Clear History", | ||
| click: () => this.clearHistory(), | ||
| }, | ||
| { | ||
| label: "Clear Cookies and Storage (All Web Widgets)", | ||
| click: () => fireAndForget(() => this.clearCookiesAndStorage()), | ||
| }, |
There was a problem hiding this comment.
Verify clearCookiesAndStorage scope matches label.
The menu label states "Clear Cookies and Storage (All Web Widgets)" but the implementation only clears storage for the current webview instance (via this.webviewRef.current?.getWebContentsId()). This is inconsistent.
Please clarify the intended behavior:
- If it should clear only the current widget, update the label to remove "(All Web Widgets)"
- If it should clear all widgets, the implementation needs to be modified to iterate over all webview instances
Related to the comment on lines 506-515.
1 participant
No description provided.