Merged
Conversation
Electron 既存の Cmd+/- ズームと同じ webContents.setZoomLevel() を使用。 zoom-changed イベントの subscription でキーボード操作とボタン操作を リアルタイム同期。ツールバーに [-] [%] [+] ボタンを配置。
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdded browser zoom controls and subscriptions, a global new-window handler that routes target="_blank"/Cmd+click opens into workspace tabs, a URL copy button, and new tRPC procedures/events to coordinate zoom and new-window events across renderer and main processes. Changes
Sequence DiagramsequenceDiagram
participant User
participant Webview as WebContents (guest)
participant BrowserManager as Main BrowserManager
participant tRPC as tRPC Router/Subscriptions
participant GlobalHandler as Dashboard Hook / Tabs Store
User->>Webview: Cmd/Ctrl+Click or anchor target="_blank"
Webview-->>BrowserManager: window.open -> webContents.window-open handler (main)
BrowserManager->>tRPC: emit unscoped "new-window" {paneId, url}
tRPC-->>GlobalHandler: onAnyNewWindow subscription event
GlobalHandler->>TabsStore: resolve paneId -> tab, call addBrowserTab(workspaceId, url)
TabsStore-->>UI: new browser tab opened in workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/browser/browser-manager.ts (1)
32-63:⚠️ Potential issue | 🟠 MajorMake
register()safe to call more than once for the same pane.The cleanup guard only runs when
webContentsIdchanges. A secondregister(paneId, sameId)call adds another console/context-menu/zoom handler, then overwrites the stored cleanup closure, sounregister()can no longer remove the older listeners.♻️ Suggested fix
register(paneId: string, webContentsId: number): void { // Clean up previous listeners if re-registering with a new webContentsId const prevId = this.paneWebContentsIds.get(paneId); - if (prevId != null && prevId !== webContentsId) { + if (prevId != null) { for (const map of [ this.consoleListeners, this.contextMenuListeners, this.zoomListeners, ]) { const cleanup = map.get(paneId); if (cleanup) { cleanup(); map.delete(paneId); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/browser/browser-manager.ts` around lines 32 - 63, The register method currently only removes old listeners when the webContentsId changes, which allows duplicate handlers if register(paneId, sameId) is called; modify register (in browser-manager.ts) to always run the existing cleanup for the given paneId before installing new listeners: for each of this.consoleListeners, this.contextMenuListeners, this.zoomListeners check map.get(paneId) and if present call cleanup() and map.delete(paneId) regardless of prevId equality, then set this.paneWebContentsIds.set(paneId, webContentsId) and proceed to create new handlers (setupConsoleCapture, setupContextMenu, setupZoomListener) so that unregister() can reliably remove the latest listeners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/browser/browser.ts`:
- Around line 148-163: The onZoomChanged subscription currently only emits
future zoom changes, so new subscribers see the default zoom (100%) until the
next event; modify the subscription in onZoomChanged to immediately emit the
current zoom by calling browserManager.getWebContents(input.paneId) and, if
present, emit.next({ zoomLevel: wc.getZoomLevel() }) before registering the
handler, and still register the handler that emits on subsequent zoom-changed
events; also ensure this is paired with a registration-time emit in
browserManager.register() if subscriptions may be created before registration so
the initial state is available.
In `@apps/desktop/src/main/lib/browser/browser-manager.ts`:
- Around line 236-240: The current setupZoomListener uses the "zoom-changed"
event which only fires for wheel/pinch and is pre-change; replace it with a
post-change check that captures keyboard and programmatic zoom: in
setupZoomListener(paneId, wc) store a lastZoom value (per pane), remove or stop
using the "zoom-changed" handler, and add a wc.on('resize', ...) listener that
calls wc.getZoomLevel(), compares to lastZoom, updates lastZoom, and emits
`zoom-changed:${paneId}` only when the level changed; also ensure you remove the
'resize' listener when the webContents is destroyed to avoid leaks.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx`:
- Around line 48-49: The optimistic update to the zoom UI is applied before
confirming the server mutation result, so when browser.applyZoom rejects (the
server returns { success: false } from browser.ts) the toolbar stays showing an
incorrect percent; modify the mutation usage around
electronTrpc.browser.setZoomLevel.useMutation() (and the applyZoom caller) to
perform the optimistic UI update only while storing the previous zoomLevel and
then rollback on mutation failure—either by using the mutation's
onError/onSettled callbacks to restore the prior zoomLevel when the response is
unsuccessful or by awaiting the mutation and only committing the new zoomLevel
if the returned payload.success is true; update the setZoomLevel usage and the
applyZoom handler (references: setZoomLevel, applyZoom, zoomLevel state)
accordingly, and ensure the same fix is applied to the other optimistic update
site noted (lines ~83-90).
- Around line 129-150: The zoom buttons currently only show symbols/text and
rely on tooltips, so update each button in BrowserPane.tsx (the buttons that
call applyZoom and use zoomLevel/zoomPercent/ZOOM_STEP/ZOOM_MIN/ZOOM_MAX) to
include explicit aria-label attributes (e.g., aria-label="Zoom out",
aria-label="Reset zoom to 100%", aria-label="Zoom in") for the zoom-out,
zoom-reset, and zoom-in controls; ensure the labels describe the action and, if
helpful, include target percent (e.g., "Zoom to 100%") and apply the same
aria-label changes to the other group of buttons around the code referenced at
the second location (the controls at lines 158-165).
In `@README.md`:
- Line 43: The changelog row for "ブラウザタブのズーム表示・操作" contains a future added-date
"2026-03-29"; update that date to the correct current or past date (e.g.,
"2026-03-28") in README.md so the "追加日" is not in the future, ensuring the table
cell with the date string is replaced accordingly.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/browser/browser-manager.ts`:
- Around line 32-63: The register method currently only removes old listeners
when the webContentsId changes, which allows duplicate handlers if
register(paneId, sameId) is called; modify register (in browser-manager.ts) to
always run the existing cleanup for the given paneId before installing new
listeners: for each of this.consoleListeners, this.contextMenuListeners,
this.zoomListeners check map.get(paneId) and if present call cleanup() and
map.delete(paneId) regardless of prevId equality, then set
this.paneWebContentsIds.set(paneId, webContentsId) and proceed to create new
handlers (setupConsoleCapture, setupContextMenu, setupZoomListener) so that
unregister() can reliably remove the latest listeners.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7753df4-c8d9-449e-9a50-9d6caf1155fc
📒 Files selected for processing (4)
README.mdapps/desktop/src/lib/trpc/routers/browser/browser.tsapps/desktop/src/main/lib/browser/browser-manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx
- ズーム: Cmd+/- のポーリング同期（300ms）で倍率表示がリアルタイム連動 - 新タブ: target="_blank" / Cmd+click を新しいブラウザタブで開くように変更 グローバル subscription で非表示タブからのイベントも正しく処理 - URLコピー: ツールバーにリンクボタン追加、クリックでクリップボードにコピー
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx (1)
217-220: Consider handling clipboard write failures.
navigator.clipboard.writeText()returns a Promise that can reject if the clipboard is unavailable or permissions are denied. Users would see no feedback in such cases.♻️ Suggested improvement
onClick={() => { - navigator.clipboard.writeText(currentUrl); - toast.success("URL copied"); + navigator.clipboard.writeText(currentUrl).then( + () => toast.success("URL copied"), + () => toast.error("Failed to copy URL"), + ); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx` around lines 217 - 220, The onClick handler using navigator.clipboard.writeText(currentUrl) doesn't handle rejections or the absence of the clipboard API; make the handler async (or handle the returned Promise) in the BrowserToolbar component so you await navigator.clipboard?.writeText(currentUrl) (or call .then/.catch), and on success call toast.success("URL copied") but on failure call toast.error with a helpful message (e.g., "Failed to copy URL" plus error.message) and handle the case where navigator.clipboard is undefined by immediately showing an error toast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx`:
- Around line 217-220: The onClick handler using
navigator.clipboard.writeText(currentUrl) doesn't handle rejections or the
absence of the clipboard API; make the handler async (or handle the returned
Promise) in the BrowserToolbar component so you await
navigator.clipboard?.writeText(currentUrl) (or call .then/.catch), and on
success call toast.success("URL copied") but on failure call toast.error with a
helpful message (e.g., "Failed to copy URL" plus error.message) and handle the
case where navigator.clipboard is undefined by immediately showing an error
toast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 615273c1-6b35-4117-b301-817a57a92fb0
📒 Files selected for processing (7)
README.mdapps/desktop/src/lib/trpc/routers/browser/browser.tsapps/desktop/src/main/lib/browser/browser-manager.tsapps/desktop/src/renderer/hooks/useBrowserNewWindowHandler.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts
✅ Files skipped from review due to trivial changes (2)
- README.md
- apps/desktop/src/main/lib/browser/browser-manager.ts
4 tasks
5 tasks
MocA-Love
added a commit
that referenced
this pull request
Apr 24, 2026
upstream 取り込み PR #10: delete dialog checkbox (superset-sh#3681) + hook 補完
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
ブラウザタブの操作性を総合的に強化しました。
1. ズーム倍率表示・操作ボタン
[-][100%][+]ボタンを配置webContents.setZoomLevel()を使用2. target="_blank" / Cmd+click を新しいブラウザタブで開く
setWindowOpenHandlerでキャッチした URL をaddBrowserTab()で新タブとして開くonAnyNewWindow）をDashboardLayoutに配置し、webview が非表示（パーキング中）でもイベントを確実にハンドルwindow.open()を呼び、同じ経路で処理3. URL コピーボタン
変更ファイル
browser.ts(tRPC)setZoomLevelmutation、onZoomChangedポーリング subscription、onAnyNewWindowsubscription 追加browser-manager.tsnew-windowイベント emit 追加、不要な zoom listener 削除BrowserPane.tsxBrowserToolbar.tsxusePersistentWebview.tsuseBrowserNewWindowHandler.tslayout.tsxテスト計画
Summary by CodeRabbit