feat(browser): PR 9 follow-up — BookmarksBar + SitePermissionsPanel#313
Conversation
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (7 files)
Reviewed by grok-code-fast-1:optimized:free · 147,783 tokens |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ]); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("A Very Long Title Th…")); |
There was a problem hiding this comment.
CRITICAL: Test expectation mismatch - the truncate function slices to 20 characters and appends "…", resulting in "A Very Long Title Tha…", but the test expects "A Very Long Title Th…" (missing 'a'). This will cause the test to fail.
| await waitFor(() => screen.getByText("A Very Long Title Th…")); | |
| expect(screen.getByText("A Very Long Title Tha…")); |
|
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 (2)
📝 WalkthroughWalkthroughThis PR adds two new browser management features: a ChangesBookmarksBar Feature
SitePermissionsPanel Feature
Sequence DiagramssequenceDiagram
participant User
participant BookmarksBar as BookmarksBar<br/>Component
participant Store as useBrowserStore
participant API as bookmarks-api
participant Events as EventTarget
rect rgba(100, 150, 255, 0.5)
Note over User,Events: Navigation Flow
User->>BookmarksBar: Click chip
BookmarksBar->>Store: navigateTab(windowId, tabId, url)
Store->>Store: Update active tab URL
end
rect rgba(100, 200, 100, 0.5)
Note over User,Events: Removal Flow
User->>BookmarksBar: Right-click chip
BookmarksBar->>BookmarksBar: Show context menu at cursor
User->>BookmarksBar: Click "Remove bookmark"
BookmarksBar->>API: removeBookmark(profileId, bookmarkId)
API-->>BookmarksBar: success/failure
BookmarksBar->>BookmarksBar: Update local bookmarks state
BookmarksBar->>Events: Dispatch taos-browser:bookmark-changed<br/>(source: "bookmarks-bar")
end
sequenceDiagram
participant User
participant SitePermsPanel as SitePermissionsPanel<br/>Component
participant API as site-permissions-api
rect rgba(100, 150, 255, 0.5)
Note over User,API: Load and Display
User->>SitePermsPanel: Modal opens
SitePermsPanel->>API: listSitePermissions(profileId)
API-->>SitePermsPanel: grants[]
SitePermsPanel->>SitePermsPanel: Render permission table
end
rect rgba(200, 100, 100, 0.5)
Note over User,API: Revoke Permission
User->>SitePermsPanel: Click revoke button
SitePermsPanel->>API: revokeSitePermission(profileId,<br/>hostPattern, permission)
API-->>SitePermsPanel: true/false
alt Success
SitePermsPanel->>SitePermsPanel: Reload grants
else Failure
SitePermsPanel->>SitePermsPanel: Show inline error
end
end
rect rgba(150, 150, 150, 0.5)
Note over User,API: Close
User->>SitePermsPanel: Press Escape or click backdrop
SitePermsPanel->>SitePermsPanel: Call onClose callback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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. 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: 2
🧹 Nitpick comments (3)
desktop/src/apps/BrowserApp/BookmarksBar.tsx (2)
51-73: ⚡ Quick winWrap
loadinuseCallbackand add it to bothuseEffectdependency arrays
loadcloses overprofileIdbut is re-created on every render and omitted from both effects' dependency arrays. This triggersreact-hooks/exhaustive-depslint errors and creates a narrow race: ifprofileIdchanges and the component re-renders (from another state update) before the second effect re-registers, thebookmark-changedhandler still holds the previousloadclosure — meaning a concurrent event could trigger a fetch with the staleprofileId.♻️ Proposed fix
- async function load() { - const seq = ++loadSeqRef.current; - const list = await listBookmarks(profileId); - if (seq !== loadSeqRef.current) return; - setBookmarks(list); - } + const load = useCallback(async () => { + const seq = ++loadSeqRef.current; + const list = await listBookmarks(profileId); + if (seq !== loadSeqRef.current) return; + setBookmarks(list); + }, [profileId]); useEffect(() => { load(); - }, [profileId]); + }, [load]); useEffect(() => { const handler = (e: Event) => { const ce = e as CustomEvent<{ profileId: string; url: string; bookmarkId: string | null; source?: string }>; if (ce.detail.profileId !== profileId) return; if (ce.detail.source === "bookmarks-bar") return; load(); }; window.addEventListener("taos-browser:bookmark-changed", handler); return () => window.removeEventListener("taos-browser:bookmark-changed", handler); - }, [profileId]); + }, [load, profileId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/BrowserApp/BookmarksBar.tsx` around lines 51 - 73, The load function closes over profileId and should be memoized so effects and the event handler always call the current fetch; wrap load in useCallback with profileId in its dependency array (keeping the existing loadSeqRef, listBookmarks and setBookmarks usage inside), then add that memoized load to both useEffect dependency arrays (the initial load effect and the bookmark-changed event effect) so the event handler is re-registered with the latest load closure and cleanup still removes the previous listener.
140-149: 💤 Low value
faviconUrl(bm.url)is called twice per chip on each renderCache the result in the
.map()callback to avoid the redundantnew URL(...)parse.♻️ Proposed fix
- {bookmarks.map((bm) => ( + {bookmarks.map((bm) => { + const favicon = faviconUrl(bm.url); + return ( <button key={bm.bookmark_id} ... > - {faviconUrl(bm.url) && ( + {favicon && ( <img - src={faviconUrl(bm.url)} + src={favicon} alt="" ... /> )} <span>{truncate(bm.title)}</span> </button> - ))} + ); + })}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/BrowserApp/BookmarksBar.tsx` around lines 140 - 149, The code calls faviconUrl(bm.url) twice per bookmark chip which repeats URL parsing; inside the .map callback that renders each bookmark (where bm is used), call faviconUrl(bm.url) once, store it in a local constant (e.g., const favicon = faviconUrl(bm.url)) and then use that constant for both the conditional and the img src to avoid redundant new URL(...) parses and improve performance.desktop/src/apps/BrowserApp/BookmarksBar.test.tsx (1)
1-119: ⚡ Quick winConsider adding tests for the
taos-browser:bookmark-changedevent listenerThe
bookmark-changedlistener has three branches (wrongprofileId→ skip;source === "bookmarks-bar"→ skip; valid event → reload) that are not covered. This is the same guard path that prevents double-reloads when the bar removes a bookmark itself.Example skeleton:
it("reloads on bookmark-changed event from another source", async () => { vi.mocked(bookmarksApi.listBookmarks) .mockResolvedValueOnce([makeBookmark()]) // initial load .mockResolvedValueOnce([makeBookmark(), makeBookmark({ bookmark_id: "bm-2", title: "New" })]); render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); await waitFor(() => screen.getByText("Example Site")); act(() => { window.dispatchEvent(new CustomEvent("taos-browser:bookmark-changed", { detail: { profileId: PROFILE_ID, url: "https://new.com", bookmarkId: "bm-2" }, })); }); await waitFor(() => screen.getByText("New")); }); it("ignores bookmark-changed events with source bookmarks-bar", async () => { vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([makeBookmark()]); render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); await waitFor(() => screen.getByText("Example Site")); act(() => { window.dispatchEvent(new CustomEvent("taos-browser:bookmark-changed", { detail: { profileId: PROFILE_ID, url: "https://example.com", bookmarkId: null, source: "bookmarks-bar" }, })); }); // listBookmarks should still only have been called once (the initial load) expect(bookmarksApi.listBookmarks).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/apps/BrowserApp/BookmarksBar.test.tsx` around lines 1 - 119, Add tests exercising the component's "taos-browser:bookmark-changed" listener to cover the three guard branches: when the event has a mismatched profileId (should be ignored), when event.detail.source === "bookmarks-bar" (should be ignored to avoid double-reload), and when a valid external change occurs (should call listBookmarks again and update UI). Use vi.mocked(bookmarksApi.listBookmarks).mockResolvedValueOnce(...) to return an initial list then a changed list for the reload case, render <BookmarksBar ... /> to attach the listener, and dispatch window.dispatchEvent(new CustomEvent("taos-browser:bookmark-changed", { detail: { profileId: PROFILE_ID|other, bookmarkId, url, source } })) inside act() to trigger the branches; assert that bookmarksApi.listBookmarks call count and DOM updates match expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/apps/BrowserApp/SitePermissionsPanel.tsx`:
- Around line 53-64: The handleRevoke flow allows duplicate clicks; add a
pending flag to block re-entry and disable the UI until the request finishes:
introduce a state like isRevoking (or a map keyed by grant.host_pattern for
per-row tracking) used in handleRevoke and the revoke button disabled prop, set
the flag true before calling revokeSitePermission(profileId, grant.host_pattern,
grant.permission) and clear it in both the success path (after await load()) and
the catch path, and ensure error handling still calls setError; apply the same
pattern to the other revoke handler referenced at lines 133-137.
In `@desktop/src/lib/browser-site-permissions-api.ts`:
- Around line 20-31: The function listSitePermissions currently returns an empty
array on non-OK responses and exceptions, hiding server/auth errors; change it
to surface failures by throwing an Error when resp.ok is false (include
resp.status and resp.statusText or response body error) and by rethrowing or
throwing a descriptive Error in the catch block so the caller can distinguish
"no permissions" from "load error"; update the code around the resp.ok check in
listSitePermissions and the catch handler to throw meaningful errors (and ensure
callers handle Promise rejections).
---
Nitpick comments:
In `@desktop/src/apps/BrowserApp/BookmarksBar.test.tsx`:
- Around line 1-119: Add tests exercising the component's
"taos-browser:bookmark-changed" listener to cover the three guard branches: when
the event has a mismatched profileId (should be ignored), when
event.detail.source === "bookmarks-bar" (should be ignored to avoid
double-reload), and when a valid external change occurs (should call
listBookmarks again and update UI). Use
vi.mocked(bookmarksApi.listBookmarks).mockResolvedValueOnce(...) to return an
initial list then a changed list for the reload case, render <BookmarksBar ...
/> to attach the listener, and dispatch window.dispatchEvent(new
CustomEvent("taos-browser:bookmark-changed", { detail: { profileId:
PROFILE_ID|other, bookmarkId, url, source } })) inside act() to trigger the
branches; assert that bookmarksApi.listBookmarks call count and DOM updates
match expectations.
In `@desktop/src/apps/BrowserApp/BookmarksBar.tsx`:
- Around line 51-73: The load function closes over profileId and should be
memoized so effects and the event handler always call the current fetch; wrap
load in useCallback with profileId in its dependency array (keeping the existing
loadSeqRef, listBookmarks and setBookmarks usage inside), then add that memoized
load to both useEffect dependency arrays (the initial load effect and the
bookmark-changed event effect) so the event handler is re-registered with the
latest load closure and cleanup still removes the previous listener.
- Around line 140-149: The code calls faviconUrl(bm.url) twice per bookmark chip
which repeats URL parsing; inside the .map callback that renders each bookmark
(where bm is used), call faviconUrl(bm.url) once, store it in a local constant
(e.g., const favicon = faviconUrl(bm.url)) and then use that constant for both
the conditional and the img src to avoid redundant new URL(...) parses and
improve performance.
🪄 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: 24658784-dd7a-4687-912d-9306e1ae08ca
📒 Files selected for processing (7)
desktop/src/apps/BrowserApp/BookmarksBar.test.tsxdesktop/src/apps/BrowserApp/BookmarksBar.tsxdesktop/src/apps/BrowserApp/BrowserApp.tsxdesktop/src/apps/BrowserApp/SettingsPanel.tsxdesktop/src/apps/BrowserApp/SitePermissionsPanel.test.tsxdesktop/src/apps/BrowserApp/SitePermissionsPanel.tsxdesktop/src/lib/browser-site-permissions-api.ts
- listSitePermissions now throws on non-2xx / network failure instead of returning []. The panel had explicit error UX that was unreachable because the wrapper collapsed all failures into 'no permissions yet'. Reads now surface real failures so the user sees the retry message. - handleRevoke now guards against duplicate clicks while a request is in flight via a revokingKey state. The button is disabled across all rows during revoke so a fast double-click can't fire two DELETEs for the same row or interleave revokes for different rows. Note: CR/Kilo's CRITICAL on BookmarksBar.test.tsx:117 was a false positive — both reviewers miscounted. truncate(20) does slice(0,20) which yields chars 0-19 = 'A Very Long Title Th' (20 chars), then appends '…'. Test correctly expects 'A Very Long Title Th…'.
|
@coderabbitai @kilo-code-bot heads up — pushed ✅ Major (CR): ✅ Minor (CR): ❌ CRITICAL (CR + Kilo, BookmarksBar.test.tsx:117): this is a false positive — both of you miscounted by one character. The test is correct as written. The test expects exactly Please re-review. |
|
Tip For best results, initiate chat on the files or code changes.
Fix 1 (Major — Fix 2 (Minor — Fix 3 (Truncation test — false positive): You're right, I apologize for the noise. Manually re-checking: That matches the test expectation exactly. The review comment had an off-by-one error on my end — the test and implementation are both correct. The 789/789 passing suite is the ground truth here. ✏️ Learnings added
|
1 participant
Summary
taos-browser:bookmark-changedevent (skips self-dispatched events to avoid double re-fetch).browser-site-permissions-api.tswrappers callingGET/DELETE /api/desktop/browser/site-permissions.Test Plan
vitest run— 789 tests pass across 108 files (6 new BookmarksBar tests, 8 new SitePermissionsPanel tests)tsc --noEmit— 0 errorsSummary by CodeRabbit
Release Notes