feat(browser): frontend chrome rebuild — multi-window + tab model (PR 4/10)#303
Merged
Conversation
…oping Whole-branch Opus review caught 1 Critical + 3 Important issues: - AddressBar (Critical): Cmd+L dispatches taos-browser:focus-address but no listener existed. Now AddressBar subscribes to the event with windowId match + focuses the input. Cmd+L works again. - BrowserApp (Important): closed browser windows left orphan entries in browser-store and persisted server rows that grew unboundedly. Now the BrowserApp unmount cleanup calls removeWindow + the backend deleteWindow. - browser-store moveTab (Important): Math.min(0, fromTabs.length-1) always returns 0 — should clamp by the original closing index matching closeTab's next-tab-by-index semantics. Test added for the active-tab-out-of-multi-tab-source case that was uncovered. - BrowserApp (Important): keyboard hook always received hasFocus=true, causing every shortcut to fire N× with N browser windows open. Now reads focused state from process-store so only the focused window's shortcuts fire.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThe PR refactors BrowserApp from a monolithic component to a modular, store-driven architecture. It removes the old ChangesBrowserApp v2 Frontend Refactor
Frontend-Backend Integration
Backend Services
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 49 seconds.Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| function resolveFinalUrl(input: string): string { | ||
| if (input.startsWith("@") || input.startsWith("!")) { | ||
| // No-op for PR 4 — PR 5/6 will replace this branch | ||
| return input; |
There was a problem hiding this comment.
WARNING: Potential security vulnerability in stub implementation
The @ and ! prefix handling is currently a stub (PR 4), returning the input unvalidated. This could allow javascript: URLs to be navigated to if a user types '@javascript:alert(1)', as it bypasses the search query logic and navigates directly.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (11 files)
Fix these issues in Kilo Cloud Reviewed by grok-code-fast-1:optimized:free · 154,261 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
desktop/src/apps/BrowserApp/MoveTabMenu.test.tsx (1)
16-77: ⚡ Quick winConsider adding a test for the "New window" path once the
queueMicrotaskrace is fixed.The three existing cases are solid. The "New window" click in
handleNewWindowis the only untested branch — and it's precisely where the race condition identified inMoveTabMenu.tsxmanifests. A test that assertsmoveTabis called with the new window ID after the store entry appears would both document the fix and guard against regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/MoveTabMenu.test.tsx` around lines 16 - 77, Add a test that exercises the "New window" path of MoveTabMenu by spying on useBrowserStore.getState().moveTab, rendering MoveTabMenu with a single source window, clicking the "New window" menu item (which triggers MoveTabMenu.handleNewWindow), then waiting for the store to contain the newly created window ID and asserting moveTab was called with ("fromWindowId", tabId, newWindowId) and that onClose was invoked; use waitFor/act to allow the queueMicrotask-created store entry to settle before asserting so the microtask race is avoided.tinyagentos/routes/desktop_browser/store.py (1)
172-172: ⚡ Quick winLIKE metacharacters in
queryare not escaped — searches with%or_behave unexpectedly.
f"%{query}%"passes the raw user string into the LIKE pattern. A%symbol in a LIKE pattern matches any sequence of zero or more characters, and_matches any single character. So a user typing%in the address bar getslike = "%%%"which matches every row (up tolimit). The ESCAPE clause is needed to treat these literally.Fix both
search_history(line 172) andlist_bookmarks(line 221):🔧 Proposed fix
+def _escape_like(s: str) -> str: + return s.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") async def search_history(self, *, user_id, profile_id, query, limit=8): ... - like = f"%{query}%" + like = f"%{_escape_like(query)}%" cursor = await self._db.execute( "SELECT url, title, visited_at " "FROM history " "WHERE user_id = ? AND profile_id = ? " - " AND (url LIKE ? OR title LIKE ?) " + " AND (url LIKE ? OR title LIKE ?) ESCAPE '\\' " "ORDER BY visited_at DESC LIMIT ?", (user_id, profile_id, like, like, limit), )Apply the same change to the
if query:branch inlist_bookmarks.Also applies to: 221-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/store.py` at line 172, search_history builds the LIKE pattern from raw query which lets user '%' or '_' act as wildcards; escape any existing backslashes then percent and underscore in query (e.g., escape \ then replace % -> \% and _ -> \_) before creating like = f"%{escaped}%", and add an ESCAPE '\\' clause to the SQL so the backslash-escaped characters are treated literally; apply the same change to the list_bookmarks branch that builds its like pattern as well, using the same escaped variable and ESCAPE '\\' in the query.desktop/src/apps/BrowserApp/TabRenderer.tsx (1)
108-110: ⚖️ Poor tradeoffPer-tab zoom via
scale()clips content rather than reflowing it.
transform: scale(N)on aninset-0absolute iframe visually magnifies from the top-left corner; the iframe still renders for the full unscaled viewport. Atzoom > 1the bottom-right portion of every page becomes unreachable (clipped by the parent'soverflow-hidden) with no way for the user to scroll to it. Users expect browser zoom to reflow the page at a new pixel density, not to crop it.Consider tracking this as a known PR-4 limitation and deferring a proper implementation (e.g., applying a
zoomproperty inside the iframe's document, or a viewport meta-tag approach) to the next PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/TabRenderer.tsx` around lines 108 - 110, The per-tab zoom currently uses CSS transform in TabRenderer (transform: tab.zoom !== 1 ? `scale(${tab.zoom})`), which clips iframe content; remove/disable this transform-based scaling so the iframe is not cropped, add a TODO and reference PR-4 to defer a proper implementation, and instead persist the desired zoom value (tab.zoom) for later application inside the iframe (e.g., via a postMessage to the iframe content to set document.body.style.zoom or inject a viewport meta approach). Update TabRenderer to stop applying transform scaling when tab.zoom !== 1, add a concise comment noting PR-4 and the intended in-iframe fix, and ensure any UI/state still exposes tab.zoom for the follow-up change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/BrowserApp/AddressBar.tsx`:
- Around line 84-92: commitNavigation calls navigateTab with raw stub inputs
like "@john" or "!foo" which causes browsers to attempt a relative navigation;
update commitNavigation to detect stub inputs (e.g., strings starting with "@"
or "!") after trimming and early-return (but still clear suggestions and reset
selected index) instead of calling navigateTab; reference the commitNavigation
function and leave resolveFinalUrl unchanged for now and then remove the
now-unreachable stub branch in resolveFinalUrl (the branch handling "@" / "!")
as a follow-up.
In `@desktop/src/apps/BrowserApp/AddressSuggest.tsx`:
- Around line 52-72: The suggestions list currently renders each item as <button
role="option"> which breaks ARIA listbox semantics; change the element returned
in AddressSuggest (the mapped suggestion item) from a button to a
non-interactive container (e.g. a div) with tabIndex={-1} and keep the existing
props: role="option", aria-selected, data-suggest-index, onMouseEnter={() =>
onHighlight(i)} and onClick={() => onSelect(s)}; remove button-specific
attributes like type and rely on the parent AddressBar keyboard handling (keep
Icon, span title/url markup and className logic intact) so the input remains the
single tab stop and listbox option semantics are preserved.
In `@desktop/src/apps/BrowserApp/Chrome.tsx`:
- Around line 84-88: The profile chip div currently uses role="status" which
creates an unnecessary live region; remove the role="status" attribute from the
div (the element with className "flex items-center gap-1.5 px-2 py-0.5
rounded-full bg-shell-bg-deep border border-shell-border-subtle text-xs" and
aria-label={`Profile: ${win.profileId}`}) so it becomes a static, display-only
badge; if you later add a decorative icon consider replacing with role="img" and
a descriptive aria-label, but do not leave role="status".
In `@desktop/src/apps/BrowserApp/FindInPage.tsx`:
- Around line 45-65: The broadcastQuery function currently calls
iframe.contentWindow?.postMessage(..., "*") which leaks search queries to any
cross-origin iframe; change the call to use window.location.origin as the
targetOrigin (i.e., pass the app's origin instead of "*") so messages are only
delivered to same-origin/proxied frames, update the comment to reflect that
cross-origin frames will be silently ignored, and keep the existing try/catch
around iframe.contentWindow?.postMessage to swallow messaging errors for
unreachable frames.
In `@desktop/src/apps/BrowserApp/keyboard.ts`:
- Around line 74-92: The zoom handlers for "+"/"=" and "-"/"_" should clamp and
round values before calling store.setTabZoom: compute newZoom = currentZoom ±
ZOOM_STEP (or 1.0 for "0"), clamp newZoom into a safe range (e.g. minZoom =
0.25, maxZoom = 5.0) and then round to two decimal places to avoid
floating-point accumulation (use a rounding helper or
Number(parseFloat(...).toFixed(2))). Update the calls in the switch cases that
reference activeTab, ZOOM_STEP and store.setTabZoom to use the
clamped-and-rounded newZoom while keeping e.preventDefault() and the same
activeTab/windowId arguments.
In `@desktop/src/apps/BrowserApp/MoveTabMenu.tsx`:
- Around line 53-63: handleNewWindow currently uses queueMicrotask so moveTab
runs before the new BrowserApp has mounted and created its window entry, causing
moveTab to silently no-op; instead, after calling openWindow("browser",
browserApp.defaultSize) subscribe to the browser store (or use the same store
selector used by moveTab) and wait until windows[newWindowId] exists, then call
moveTab(fromWindowId, tabId, newWindowId), unsubscribe the listener, and finally
call onClose; reference handleNewWindow, openWindow, moveTab,
BrowserApp.useEffect/createWindow and the browser store's windows map when
implementing the polling/subscription and cleanup.
In `@desktop/src/apps/BrowserApp/TabOverview.tsx`:
- Line 51: The grid containers wrapping TabCard items are missing
role="tablist", leaving each element using role="tab" and aria-selected
orphaned; update the two grid wrapper divs in TabOverview.tsx (the elements with
className "grid grid-cols-2 gap-2" and the second similar grid at the later
section) to include role="tablist" so the TabCard components (which render
role="tab" / aria-selected) are proper children of a tablist and accessible to
screen readers.
In `@desktop/src/apps/BrowserApp/TabRenderer.tsx`:
- Line 100: The iframe sandbox in TabRenderer.tsx currently includes
"allow-scripts allow-same-origin", which nullifies the sandbox when the proxied
content is same-origin; update the TabRenderer iframe sandbox attribute to
remove "allow-same-origin" (keep only safe tokens like "allow-scripts
allow-forms allow-popups allow-downloads" or whatever minimal set is required)
so framed pages cannot escape to the parent, and if you truly need
same-origin/script execution for proxied content, instead serve the proxy from
an isolated subdomain (e.g., browser-sandbox) so you can safely drop
allow-same-origin; adjust any proxy routing for /api/desktop/browser/proxy
accordingly and document the change in TabRenderer.
In `@desktop/src/apps/BrowserApp/TabStrip.tsx`:
- Around line 116-123: The tab wrapper div in TabStrip.tsx is missing the
Tailwind "group" class so group-hover variants on the close button never
trigger; update the className array used for the TabItem wrapper (the element
composed from widthClass, the "h-[28px] px-2 ..." string, and isActive branch)
to include "group" (e.g., add "group" to that array) so group-hover:opacity-100
on the close button can reveal it on tab hover.
In `@desktop/src/apps/BrowserApp/WindowChooser.tsx`:
- Around line 80-108: The ul currently rendered in WindowChooser.tsx uses
role="list" but contains buttons with role="option"; update the parent element
to use role="listbox" so the option ownership is correct for assistive tech.
Locate the JSX rendering the list (the ul that maps over windowList and renders
items via summarizeWindow) and change its role to "listbox"; keep existing
button elements (role="option", aria-selected) and the onClick handlers
(onSelect, onClose) intact so tests querying getAllByRole("option") and
selection behavior remain unchanged.
In `@tests/routes/desktop_browser/test_suggest.py`:
- Around line 117-122: The test currently uses a non-strict assertion (assert
len(suggestions) <= 5) which won't catch cases where limit isn't enforced;
update the assertion to assert len(suggestions) == 5 to verify the limit=5
parameter is applied, keeping the rest of the request (client.get to
"/api/desktop/browser/suggest" with params
{"profile_id":"personal","q":"example","limit":5}) and the suggestions variable
unchanged; also ensure the test setup indeed inserts 15 distinct matching
entries so the equality assertion is meaningful.
In `@tinyagentos/routes/desktop_browser/suggest.py`:
- Around line 23-24: Suppress the false-positive Ruff B008 on the FastAPI
dependency by either adding fastapi.Depends to the ruff config's
extend-immutable-calls (e.g., add "fastapi.Depends" to
tool.ruff.lint.flake8-bugbear.extend-immutable-calls) or by silencing the
warning inline where Depends(get_current_user) is used; additionally, enforce a
hard cap on the limit parameter (e.g., clamp or validate limit in the route
handler or dependency) so callers cannot pass extremely large values that cause
list_bookmarks or search_history to allocate huge result sets (use the limit
parameter name and the route handler that calls list_bookmarks/search_history to
apply the clamp/validation).
In `@tinyagentos/routes/desktop_browser/windows.py`:
- Line 33: Add a Ruff B008 suppression for FastAPI dependency calls: update the
route handler parameter annotations that use Depends(get_current_user) (and the
similar Depends(...) usages at the other two occurrences) to include "# noqa:
B008" on those lines, or alternatively add "B008" to the project-level Ruff
extend-ignore; target the function/method signatures that reference Depends and
get_current_user to locate the exact places to change.
---
Nitpick comments:
In `@desktop/src/apps/BrowserApp/MoveTabMenu.test.tsx`:
- Around line 16-77: Add a test that exercises the "New window" path of
MoveTabMenu by spying on useBrowserStore.getState().moveTab, rendering
MoveTabMenu with a single source window, clicking the "New window" menu item
(which triggers MoveTabMenu.handleNewWindow), then waiting for the store to
contain the newly created window ID and asserting moveTab was called with
("fromWindowId", tabId, newWindowId) and that onClose was invoked; use
waitFor/act to allow the queueMicrotask-created store entry to settle before
asserting so the microtask race is avoided.
In `@desktop/src/apps/BrowserApp/TabRenderer.tsx`:
- Around line 108-110: The per-tab zoom currently uses CSS transform in
TabRenderer (transform: tab.zoom !== 1 ? `scale(${tab.zoom})`), which clips
iframe content; remove/disable this transform-based scaling so the iframe is not
cropped, add a TODO and reference PR-4 to defer a proper implementation, and
instead persist the desired zoom value (tab.zoom) for later application inside
the iframe (e.g., via a postMessage to the iframe content to set
document.body.style.zoom or inject a viewport meta approach). Update TabRenderer
to stop applying transform scaling when tab.zoom !== 1, add a concise comment
noting PR-4 and the intended in-iframe fix, and ensure any UI/state still
exposes tab.zoom for the follow-up change.
In `@tinyagentos/routes/desktop_browser/store.py`:
- Line 172: search_history builds the LIKE pattern from raw query which lets
user '%' or '_' act as wildcards; escape any existing backslashes then percent
and underscore in query (e.g., escape \ then replace % -> \% and _ -> \_) before
creating like = f"%{escaped}%", and add an ESCAPE '\\' clause to the SQL so the
backslash-escaped characters are treated literally; apply the same change to the
list_bookmarks branch that builds its like pattern as well, using the same
escaped variable and ESCAPE '\\' in the query.
🪄 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 Plus
Run ID: e250490a-7cc8-4275-8123-f8bd320926e3
📒 Files selected for processing (40)
desktop/src/apps/BrowserApp.initialUrl.test.tsxdesktop/src/apps/BrowserApp.tsxdesktop/src/apps/BrowserApp/AddressBar.test.tsxdesktop/src/apps/BrowserApp/AddressBar.tsxdesktop/src/apps/BrowserApp/AddressSuggest.test.tsxdesktop/src/apps/BrowserApp/AddressSuggest.tsxdesktop/src/apps/BrowserApp/BrowserApp.test.tsxdesktop/src/apps/BrowserApp/BrowserApp.tsxdesktop/src/apps/BrowserApp/Chrome.test.tsxdesktop/src/apps/BrowserApp/Chrome.tsxdesktop/src/apps/BrowserApp/FindInPage.test.tsxdesktop/src/apps/BrowserApp/FindInPage.tsxdesktop/src/apps/BrowserApp/MoveTabMenu.test.tsxdesktop/src/apps/BrowserApp/MoveTabMenu.tsxdesktop/src/apps/BrowserApp/TabOverview.test.tsxdesktop/src/apps/BrowserApp/TabOverview.tsxdesktop/src/apps/BrowserApp/TabRenderer.test.tsxdesktop/src/apps/BrowserApp/TabRenderer.tsxdesktop/src/apps/BrowserApp/TabStrip.test.tsxdesktop/src/apps/BrowserApp/TabStrip.tsxdesktop/src/apps/BrowserApp/WindowChooser.test.tsxdesktop/src/apps/BrowserApp/WindowChooser.tsxdesktop/src/apps/BrowserApp/index.tsdesktop/src/apps/BrowserApp/keyboard.test.tsdesktop/src/apps/BrowserApp/keyboard.tsdesktop/src/apps/BrowserApp/types.tsdesktop/src/hooks/use-session-persistence.tsdesktop/src/lib/browser-suggest-api.test.tsdesktop/src/lib/browser-suggest-api.tsdesktop/src/lib/browser-windows-api.test.tsdesktop/src/lib/browser-windows-api.tsdesktop/src/stores/browser-store.test.tsdesktop/src/stores/browser-store.tstests/routes/desktop_browser/test_suggest.pytests/routes/desktop_browser/test_windows.pytinyagentos/routes/desktop.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/store.pytinyagentos/routes/desktop_browser/suggest.pytinyagentos/routes/desktop_browser/windows.py
💤 Files with no reviewable changes (2)
- desktop/src/apps/BrowserApp.initialUrl.test.tsx
- desktop/src/apps/BrowserApp.tsx
Comment on lines
+74
to
+92
| case "+": | ||
| case "=": | ||
| if (activeTab) { | ||
| e.preventDefault(); | ||
| store.setTabZoom(windowId, activeTab.id, activeTab.zoom + ZOOM_STEP); | ||
| } | ||
| break; | ||
| case "-": | ||
| case "_": | ||
| if (activeTab) { | ||
| e.preventDefault(); | ||
| store.setTabZoom(windowId, activeTab.id, activeTab.zoom - ZOOM_STEP); | ||
| } | ||
| break; | ||
| case "0": | ||
| if (activeTab) { | ||
| e.preventDefault(); | ||
| store.setTabZoom(windowId, activeTab.id, 1.0); | ||
| } |
There was a problem hiding this comment.
Zoom has no bounds clamping and accumulates floating-point error
Two issues in the same block:
-
No clamping:
zoom - ZOOM_STEPcan reach 0 or go negative after enough presses;zoom + ZOOM_STEPis unbounded. A negative zoom would likely produce a broken CSStransform: scale(...). Should clamp to a sensible range (e.g.[0.25, 5.0]). -
FP accumulation:
1.0 + 0.1 = 1.1, but1.1 + 0.1 = 1.2000000000000002. If the zoom value is displayed as a percentage label (e.g. "120%"), this will surface as "120.00000000002%". Rounding to two decimal places before passing to the store fixes this.
🔧 Proposed fix
+const MIN_ZOOM = 0.25;
+const MAX_ZOOM = 5.0;
+
+function clampZoom(z: number): number {
+ return Math.round(Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, z)) * 100) / 100;
+} case "+":
case "=":
if (activeTab) {
e.preventDefault();
- store.setTabZoom(windowId, activeTab.id, activeTab.zoom + ZOOM_STEP);
+ store.setTabZoom(windowId, activeTab.id, clampZoom(activeTab.zoom + ZOOM_STEP));
}
break;
case "-":
case "_":
if (activeTab) {
e.preventDefault();
- store.setTabZoom(windowId, activeTab.id, activeTab.zoom - ZOOM_STEP);
+ store.setTabZoom(windowId, activeTab.id, clampZoom(activeTab.zoom - ZOOM_STEP));
}
break;📝 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.
Suggested change
| case "+": | |
| case "=": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, activeTab.zoom + ZOOM_STEP); | |
| } | |
| break; | |
| case "-": | |
| case "_": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, activeTab.zoom - ZOOM_STEP); | |
| } | |
| break; | |
| case "0": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, 1.0); | |
| } | |
| const MIN_ZOOM = 0.25; | |
| const MAX_ZOOM = 5.0; | |
| function clampZoom(z: number): number { | |
| return Math.round(Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, z)) * 100) / 100; | |
| } | |
| case "+": | |
| case "=": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, clampZoom(activeTab.zoom + ZOOM_STEP)); | |
| } | |
| break; | |
| case "-": | |
| case "_": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, clampZoom(activeTab.zoom - ZOOM_STEP)); | |
| } | |
| break; | |
| case "0": | |
| if (activeTab) { | |
| e.preventDefault(); | |
| store.setTabZoom(windowId, activeTab.id, 1.0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/BrowserApp/keyboard.ts` around lines 74 - 92, The zoom
handlers for "+"/"=" and "-"/"_" should clamp and round values before calling
store.setTabZoom: compute newZoom = currentZoom ± ZOOM_STEP (or 1.0 for "0"),
clamp newZoom into a safe range (e.g. minZoom = 0.25, maxZoom = 5.0) and then
round to two decimal places to avoid floating-point accumulation (use a rounding
helper or Number(parseFloat(...).toFixed(2))). Update the calls in the switch
cases that reference activeTab, ZOOM_STEP and store.setTabZoom to use the
clamped-and-rounded newZoom while keeping e.preventDefault() and the same
activeTab/windowId arguments.
…inor)
Major:
- TabRenderer: drop iframe sandbox `allow-same-origin`. Combined with
`allow-scripts` on a same-origin proxy, it was a sandbox bypass.
Isolated-subdomain proxy hosting deferred to HTTPS+DNS Foundations
brainstorm (already documented in PR 2's spec amendment).
- TabStrip: add missing `group` Tailwind class so close-button
`group-hover:opacity-100` actually fires (button was permanently
invisible — tests didn't catch it because RTL queries by aria-label
regardless of visibility).
- MoveTabMenu: replace queueMicrotask race with a real store
subscription. queueMicrotask fired before the new BrowserApp's
mount-effect could call createWindow, so moveTab was a silent no-op.
Minor (security + a11y + lint):
- AddressBar: @/! prefixes are now true no-ops (don't call navigateTab
with raw input). Defends against `@javascript:...` constructions.
- AddressSuggest: <button role="option"> → <div role="option"
tabIndex={-1}>. Buttons inside listboxes confuse keyboard semantics.
- Chrome: remove role="status" from profile chip — live region would
spuriously announce when PR 5 wires switching.
- FindInPage: postMessage targetOrigin "*" → window.location.origin.
Stops search-query leakage to genuinely cross-origin embedded pages.
- keyboard.ts zoom: round to 1 decimal to stop 0.30000000000000004
drift from repeated +0.1 / -0.1.
- TabOverview: add role="tablist" to the grid containers so
role="tab" children have a proper owner.
- WindowChooser: <ul role="list"> → role="listbox" so role="option"
buttons have a valid owner.
- test_suggest: <= 5 → == 5 for limit-cap assertion.
- suggest.py + windows.py: # noqa: B008 on Depends() defaults.
- suggest.py: cap limit to [1, 50] to prevent abuse.
This was referenced May 4, 2026
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.
Fourth of ten PRs implementing BrowserApp v2 per the design doc.
First user-visible UI deliverable of BrowserApp v2. Replaces the 503-line
BrowserApp.tsxmonolith with a multi-window, multi-tab browser shell using the compact unified URL bar (Q8 Layout A — Safari 15+ style).Summary
Backend
windows.py—/api/desktop/browser/windowsGET/PUT/DELETE for browser-window state persistencesuggest.py—/api/desktop/browser/suggestlocal-only address-bar autocomplete (history + bookmarks per profile, no online suggest API)BrowserStoreextended:upsert_window/list_windows/delete_window/add_history/search_history/add_bookmark/list_bookmarks. All multi-user keyed.tinyagentos/routes/desktop.py:browser_proxy+_rewrite_html_urlsdeleted (PR 3's/api/desktop/browser/proxyis the only proxy now)Frontend store
browser-storeZustand — one entry per window: tabs, activeTab, profile, recentlyClosed (max 50), discard stateFrontend chrome (compact unified bar — Q8 Layout A)
Chrome.tsx— back/forward/refresh + profile chip (display only; PR 5 wires the dropdown)TabStrip.tsx— pinned tabs (favicon-only) + inactive tabs (favicon + title) + active tab (wider, hosts AddressBar in PR 5+)AddressBar.tsx+AddressSuggest.tsx— URL input with separate input value vs tab URL, Enter commits, Esc reverts, debounced 150ms suggest popover with keyboard navTabRenderer.tsx— iframe pool withdisplay:noneswitching for live tabs + snapshot card for discarded tabs + 60s discard scheduler (10-min idle, hard cap 12 live tabs)BrowserApp/BrowserApp.tsx— top-level container; auto-creates window in store on mount, idempotentMobile (Q6 — single instance + window chooser)
useIsMobile(600)switches layoutTabOverview.tsx— full-screen grid of tab cards (Pinned section + Open section + New tab button)WindowChooser.tsx— sheet listing all browser windows for the user with current markerCross-cutting
FindInPage.tsx— Cmd+F overlay, broadcasts to iframe via postMessage (copilot.js will respond in PR 6)keyboard.ts— Cmd+T (new tab), Cmd+W (close), Cmd+L (focus address), Cmd+F (find), Cmd+0/+/- (zoom)MoveTabMenu.tsx— right-click move-to-window (DOM-portal drag-and-drop with iframe state preservation deferred per plan)useSessionPersistenceextended for browser windows (debounced 2s save, mirrors existing pattern)What this does not land
Test Plan
pytest tests/routes/desktop_browser/test_windows.py -v— 6 testspytest tests/routes/desktop_browser/test_suggest.py -v— 8 testspytest tests/routes/desktop_browser/— 143 tests, all greennpx vitest run— 381 frontend tests, all greenpytest tests/routes/ tests/test_secrets.py— 535 tests, no regressionsSpec
docs/superpowers/specs/2026-05-03-browser-app-v2-design.md§5 (frontend), Q6 (mobile), Q8 Layout A (compact chrome).Cumulative shipping arc:
Notes for reviewers
@and!prefixes are stubs in PR 4. PR 6 will wire@<agent>to agent suggestions; PR 5 will wire!<profile>to inline profile switching.copilot.jsresponds to the iframe postMessage. Match count shows nothing.scrollYis on the Tab type but never written — placeholder for the discard-then-reload restore. Will be wired when PR 6's copilot.js can postMessage scroll position back.e03e9a5: Cmd+L focus event, BrowserApp unmount cleanup (no orphan windows),moveTabindex-clamping bug, and per-window keyboard-handler scoping (was firing N× with N windows open).Summary by CodeRabbit
New Features