feat(browser): PR 5 — profile switcher, discard policy, Reader mode#304
Conversation
…e-cascade warning
…view + URL-bar toggle
… nav reset (review feedback)
…ofile in use elsewhere
|
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 (5)
📝 WalkthroughWalkthroughAdds reader-mode extraction/UI, profile CRUD and switching (with per-profile tab snapshots), configurable settings and discard heuristics, iframe live-exclusion detection, backend extract and profile endpoints, and associated tests across frontend, store, and server code. ChangesReader Mode
Profile Management
Settings and Tab Discard / Live-Exclusion
Router Registration & Misc Backend
Sequence Diagram(s)sequenceDiagram
participant AddressBar as AddressBar (Client)
participant BrowserStore as Browser Store
participant Backend as /api/desktop/browser/extract
participant ReaderMode as ReaderMode UI
AddressBar->>BrowserStore: read activeTab, readerAvailable?
Note right of AddressBar: on input focus\nwhen readerAvailable === undefined
AddressBar->>Backend: GET /api/.../extract?profile_id=&url=
Backend-->>AddressBar: ExtractResult (title, html, text, word_count)
AddressBar->>BrowserStore: setTabReader(readerAvailable: true/false, readerExtract?)
BrowserStore-->>ReaderMode: updated tab state (readerAvailable/readerExtract)
ReaderMode-->>AddressBar: renders toggle/button (readerAvailable true)
AddressBar->>BrowserStore: setTabReader(readerActive: toggle)
BrowserStore-->>ReaderMode: show/hide reader UI
Note: rectangles use default styling; diagram focuses on high-level flow. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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)
|
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (issues in diff but lines not resolvable)Issues found in changed code that cannot receive inline comments due to line resolution issues:
Files Reviewed (33 files)
Reviewed by grok-code-fast-1:optimized:free · 318,422 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: 11
🧹 Nitpick comments (3)
tinyagentos/routes/desktop_browser/store.py (1)
156-170: ⚡ Quick winAddress Ruff S608 on the f-string SQL construction.
The current
setslist only ever holds the two hardcoded literals"name = ?"and"color = ?"— no user input reaches the query template, so there is no real injection risk. However, Ruff S608 triggers on the f-string pattern and will fail CI if the rule is treated as an error. The simplest fix is a targeted# noqasuppression; alternatively, since there are only two optional fields, the four possible queries can be expressed explicitly, which also removes the need for the diagnostic.🛠️ Option A — inline suppression
cursor = await self._db.execute( - f"UPDATE profiles SET {', '.join(sets)} " - f"WHERE user_id = ? AND profile_id = ?", + f"UPDATE profiles SET {', '.join(sets)} " # noqa: S608 + "WHERE user_id = ? AND profile_id = ?", params, )🛠️ Option B — eliminate the f-string entirely
- # Build dynamic SET clause - sets: list[str] = [] - params: list[object] = [] - if name is not None: - sets.append("name = ?") - params.append(name) - if color is not None: - sets.append("color = ?") - params.append(color) - params.extend([user_id, profile_id]) - cursor = await self._db.execute( - f"UPDATE profiles SET {', '.join(sets)} " - f"WHERE user_id = ? AND profile_id = ?", - params, - ) + if name is not None and color is not None: + sql = "UPDATE profiles SET name = ?, color = ? WHERE user_id = ? AND profile_id = ?" + p: list[object] = [name, color, user_id, profile_id] + elif name is not None: + sql = "UPDATE profiles SET name = ? WHERE user_id = ? AND profile_id = ?" + p = [name, user_id, profile_id] + else: + sql = "UPDATE profiles SET color = ? WHERE user_id = ? AND profile_id = ?" + p = [color, user_id, profile_id] + cursor = await self._db.execute(sql, p)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/store.py` around lines 156 - 170, The f-string used to build the UPDATE SQL in the method that constructs sets and params (the sets list, params list, and the self._db.execute call building f"UPDATE profiles SET {', '.join(sets)} ...") triggers Ruff S608; fix by adding a targeted inline suppression on that f-string (append " # noqa: S608") or, alternatively, eliminate the f-string entirely by composing the query with plain string concatenation/format-free join (e.g., base = "UPDATE profiles SET " + ", ".join(sets) + " WHERE user_id = ? AND profile_id = ?" and pass params to self._db.execute) so the code no longer trips S608 while preserving the current parameterized behavior.desktop/src/apps/BrowserApp/ProfileManager.test.tsx (1)
202-294: ⚡ Quick winTest renders component twice, creating potential flakiness.
The test renders
ProfileManagerat line 225, then re-configures the mock (lines 240-266), then renders again at line 269 without unmounting the first. This leaves two instances in the DOM, requiringgetAllByText("Work")[0]and similar array indexing to select the correct elements. Consider unmounting the first render or restructuring to avoid the double-render pattern.♻️ Suggested fix: remove first render or unmount before re-render
- // Render ProfileManager as if the user in window-a opened it (active = personal) - render( - <ProfileManager - activeProfileId="personal" - onClose={() => {}} - />, - ); - - // Wait for the initial profile list (which initially has personal + work) - // But we've already set up fetch to return only personal, so wait for Work to appear - // via the initial load mock — we need to override the initial load too. - // Let's wait for the list to settle: the first GET returns personal only (no work). - // Because the fetch is mocked to return personal-only for ALL GETs, the list - // will show only Personal. We won't see Work in the UI. - // To properly test the flow, we need Work in the initial list. - // Re-mock: first GET returns both, DELETE succeeds, second GET returns only personal. global.fetch = vi.fn() .mockImplementationOnce(() => // ... mock sequence ); - // Re-render with correct mock sequence - const { unmount } = render( + const { unmount } = render( <ProfileManager activeProfileId="personal" onClose={() => {}} />, ); - await waitFor(() => screen.getAllByText("Work")[0]); + await waitFor(() => screen.getByText("Work"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/ProfileManager.test.tsx` around lines 202 - 294, The test currently renders ProfileManager twice (two render(...) calls) causing duplicate DOM instances and flakiness; remove the first render or call the first render's unmount() before setting up the second mock sequence and re-rendering so only one ProfileManager instance exists; specifically adjust the initial render call that precedes the mockImplementationOnce sequence (and its associated render) to be removed or unmounted, keep the later render(...) that uses the three-step fetch mock, and ensure you only interact with elements from that single mounted ProfileManager (references: ProfileManager component, render(), unmount(), and the fetch mock setup).desktop/src/apps/BrowserApp/ProfileSwitcher.tsx (1)
32-35: 💤 Low valueConsider adding error handling for profile loading.
If
listProfiles()fails,profilesremainsnulland the UI shows "Loading…" indefinitely. A brief error state would improve UX, though this is acceptable for MVP.♻️ Optional: Add error state
const [profiles, setProfiles] = useState<Profile[] | null>(null); +const [loadError, setLoadError] = useState(false); const [creating, setCreating] = useState(false); ... useEffect(() => { - listProfiles().then(setProfiles); + listProfiles() + .then(setProfiles) + .catch(() => setLoadError(true)); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/ProfileSwitcher.tsx` around lines 32 - 35, The profile loading hook in ProfileSwitcher currently calls listProfiles() inside useEffect and only sets profiles via setProfiles, so failures leave profiles null and the UI stuck on "Loading…". Update the effect to handle errors: call listProfiles().then(setProfiles).catch(err => { setProfiles([]); setError(err) }) and add a local error state (e.g., const [error, setError] = useState<Error|null>(null)) in the ProfileSwitcher component; then render a brief error message or retry button when error is non-null (instead of the perpetual loading state) so the UI recovers when listProfiles() fails.
🤖 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/Chrome.tsx`:
- Around line 95-98: The profile manager modal's open state (managerOpen /
setManagerOpen) isn't being closed when other overlays open; update the
overlay-toggle handlers (the onClick that calls setSwitcherOpen and
setSettingsOpen, and the similar handler at the other location) to also call
setManagerOpen(false) so the manager is closed before opening the switcher or
settings. Ensure both handlers explicitly close the manager (e.g.,
setManagerOpen(false)) prior to toggling their respective panels to prevent
overlapping modals and focus conflicts.
In `@desktop/src/apps/BrowserApp/ProfileManager.tsx`:
- Around line 48-51: The async handlers reload, handleRename, handleDelete, and
handleCreate currently call
listProfiles/createProfile/renameProfile/deleteProfile and await results without
try/catch, so wrap the await-containing body of each function in a try/catch; on
error call setError(error?.message || String(error)) and optionally clear any
success state (e.g., keep existing setProfiles/update UI only on success), and
rethrow or return gracefully as needed. Ensure you still handle the existing
null/false return branches after the try block (e.g., only run the current
setProfiles/follow-up logic when API call succeeds) so thrown exceptions are
surfaced to the UI via setError.
- Around line 119-121: The confirmation overlay uses absolute inset-0 but the
card container in ProfileManager.tsx (the <div> with className "bg-shell-surface
rounded-md shadow-xl border border-shell-border w-[420px] max-w-full
max-h-[80vh] flex flex-col" and onClick={(e) => e.stopPropagation()}) is not
positioned, so the overlay anchors to the viewport; update that container to be
positioned (add the "relative" class) so the confirmation overlay (around lines
~270–274) will be sized and rounded relative to the card instead of the full
screen.
- Around line 57-58: The error banner can persist because setError(null) is
never called at the start of the handlers; update handleRename, handleDelete,
and handleCreate to call setError(null) immediately as the first statement in
each function so any previous error is cleared before attempting the new
operation (refer to the handleRename, handleDelete, handleCreate function
declarations and the setError state setter).
- Around line 57-69: The rename handler (handleRename) can be re-entered when
setEditingId(null) unmounts the input and triggers onBlur, causing a double call
to renameProfile; add an in-flight re-entry guard (e.g., a ref or state like
isRenamingRef or isRenaming) checked at the top of handleRename and set before
awaiting renameProfile and cleared in a finally block to prevent duplicate
requests, update both handleRename and the corresponding onBlur handler to
respect this guard so the blur-triggered call no-ops if a rename is already in
progress.
- Around line 113-118: The dialog element in ProfileManager (the <div
role="dialog" aria-label="Manage profiles" ... onClick={onClose}>) is missing
aria-modal="true"; update that div to include aria-modal="true" so assistive
technologies treat the rest of the document as inert while the modal is open and
preserve proper dialog semantics.
In `@desktop/src/stores/browser-settings-store.test.ts`:
- Around line 1-6: Add the missing type import for SearchEngine to the test
imports so the TypeScript-only assertion "as SearchEngine" compiles under tsc
--noEmit; update the import line that currently imports useBrowserSettingsStore,
searchUrlFor, and SEARCH_ENGINES from "./browser-settings-store" to also import
the SearchEngine type (using a type import if desired) to resolve the "Cannot
find name 'SearchEngine'" error.
In `@tests/routes/desktop_browser/test_profile_routes.py`:
- Around line 51-59: The test_post_appends_suffix_on_collision currently only
captures the second POST response and allows either "personal-2" or
"personal-3"; change it to capture both POST responses and assert
deterministically that the first POST returns profile_id "personal-2" and the
second returns "personal-3" (referencing the test name
test_post_appends_suffix_on_collision and the "profile_id" field from the JSON
bodies of the two POSTs to /api/desktop/browser/profiles) so the collision
handling is strictly verified.
In `@tinyagentos/routes/desktop_browser/profile.py`:
- Around line 200-212: The pre-check in profile deletion is racy: remove the
separate snapshot checks in routes/desktop_browser/profile.py (the code that
calls browser_store.list_profiles and the len(profiles) guard) and instead
enforce the "cannot delete last profile" invariant atomically inside the store
layer by adding or updating a transactional store method (e.g.
browser_store.delete_profile or a new browser_store.delete_profile_if_not_last)
to check the remaining profile count and raise LastProfileError within the same
DB transaction that performs the delete; update the route to call that atomic
store method and handle LastProfileError as before.
- Around line 45-67: The bootstrap currently does a read-then-write causing
races; wrap the claim-and-seed in a single DB transaction so only the
transaction that actually inserts the init marker seeds defaults. Use store._db
transaction semantics (begin/commit/rollback or a transaction context) and
perform "INSERT OR IGNORE INTO profile_init (user_id, initialized_at) VALUES (?,
?)" inside that transaction, then check whether the insert affected a row (e.g.,
cursor.rowcount or SELECT changes()/re-query profile_init for user_id) — only
call store.add_profile for entries in _DEFAULTS when the insert indicates we won
the init claim; finally commit the transaction (or rollback on error). Ensure
you reference store._db, profile_init, add_profile, and _DEFAULTS while making
these changes.
- Around line 154-161: The rename route currently forwards name unchanged to
store.update_profile so blank or whitespace-only names can slip through; trim
the incoming name (e.g., name = name.strip() if name is not None), validate that
the trimmed name is non-empty and raise a ValueError if it is blank, then pass
the normalized name to store.update_profile (reference: rename_profile,
update_profile, create_profile for consistent behavior). Also catch ValueError
raised during validation or by update_profile in the route and convert it into
an HTTP 400 response (HTTPException or your framework's equivalent) so clients
receive a proper bad-request error instead of a 500.
---
Nitpick comments:
In `@desktop/src/apps/BrowserApp/ProfileManager.test.tsx`:
- Around line 202-294: The test currently renders ProfileManager twice (two
render(...) calls) causing duplicate DOM instances and flakiness; remove the
first render or call the first render's unmount() before setting up the second
mock sequence and re-rendering so only one ProfileManager instance exists;
specifically adjust the initial render call that precedes the
mockImplementationOnce sequence (and its associated render) to be removed or
unmounted, keep the later render(...) that uses the three-step fetch mock, and
ensure you only interact with elements from that single mounted ProfileManager
(references: ProfileManager component, render(), unmount(), and the fetch mock
setup).
In `@desktop/src/apps/BrowserApp/ProfileSwitcher.tsx`:
- Around line 32-35: The profile loading hook in ProfileSwitcher currently calls
listProfiles() inside useEffect and only sets profiles via setProfiles, so
failures leave profiles null and the UI stuck on "Loading…". Update the effect
to handle errors: call listProfiles().then(setProfiles).catch(err => {
setProfiles([]); setError(err) }) and add a local error state (e.g., const
[error, setError] = useState<Error|null>(null)) in the ProfileSwitcher
component; then render a brief error message or retry button when error is
non-null (instead of the perpetual loading state) so the UI recovers when
listProfiles() fails.
In `@tinyagentos/routes/desktop_browser/store.py`:
- Around line 156-170: The f-string used to build the UPDATE SQL in the method
that constructs sets and params (the sets list, params list, and the
self._db.execute call building f"UPDATE profiles SET {', '.join(sets)} ...")
triggers Ruff S608; fix by adding a targeted inline suppression on that f-string
(append " # noqa: S608") or, alternatively, eliminate the f-string entirely by
composing the query with plain string concatenation/format-free join (e.g., base
= "UPDATE profiles SET " + ", ".join(sets) + " WHERE user_id = ? AND profile_id
= ?" and pass params to self._db.execute) so the code no longer trips S608 while
preserving the current parameterized behavior.
🪄 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: 62996908-ee5b-4c01-9ea1-9de26214f8bb
📒 Files selected for processing (33)
desktop/src/apps/BrowserApp/AddressBar.test.tsxdesktop/src/apps/BrowserApp/AddressBar.tsxdesktop/src/apps/BrowserApp/Chrome.tsxdesktop/src/apps/BrowserApp/ProfileManager.test.tsxdesktop/src/apps/BrowserApp/ProfileManager.tsxdesktop/src/apps/BrowserApp/ProfileSwitcher.test.tsxdesktop/src/apps/BrowserApp/ProfileSwitcher.tsxdesktop/src/apps/BrowserApp/ReaderMode.test.tsxdesktop/src/apps/BrowserApp/ReaderMode.tsxdesktop/src/apps/BrowserApp/SettingsPanel.test.tsxdesktop/src/apps/BrowserApp/SettingsPanel.tsxdesktop/src/apps/BrowserApp/TabRenderer.test.tsxdesktop/src/apps/BrowserApp/TabRenderer.tsxdesktop/src/apps/BrowserApp/live-exclusion.test.tsdesktop/src/apps/BrowserApp/live-exclusion.tsdesktop/src/apps/BrowserApp/types.tsdesktop/src/lib/browser-extract-api.test.tsdesktop/src/lib/browser-extract-api.tsdesktop/src/lib/browser-profile-api.test.tsdesktop/src/lib/browser-profile-api.tsdesktop/src/stores/browser-settings-store.test.tsdesktop/src/stores/browser-settings-store.tsdesktop/src/stores/browser-store.test.tsdesktop/src/stores/browser-store.tspyproject.tomltests/routes/desktop_browser/test_extract.pytests/routes/desktop_browser/test_profile_routes.pytinyagentos/routes/desktop_browser/__init__.pytinyagentos/routes/desktop_browser/extract.pytinyagentos/routes/desktop_browser/profile.pytinyagentos/routes/desktop_browser/profile_routes.pytinyagentos/routes/desktop_browser/schema.pytinyagentos/routes/desktop_browser/store.py
| async def test_post_appends_suffix_on_collision(self, client): | ||
| await client.post( | ||
| "/api/desktop/browser/profiles", json={"name": "Personal", "color": "#000000"}, | ||
| ) | ||
| body = (await client.post( | ||
| "/api/desktop/browser/profiles", json={"name": "Personal", "color": "#111111"}, | ||
| )).json() | ||
| # Original "personal" exists from defaults; first POST gets "personal-2" | ||
| assert body["profile_id"] in {"personal-2", "personal-3"} |
There was a problem hiding this comment.
Tighten this collision assertion.
These requests are sequential, so after the defaults already own personal, the first POST here should take personal-2 and the second should deterministically return personal-3. Allowing either suffix makes the test pass even if collision handling regresses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routes/desktop_browser/test_profile_routes.py` around lines 51 - 59,
The test_post_appends_suffix_on_collision currently only captures the second
POST response and allows either "personal-2" or "personal-3"; change it to
capture both POST responses and assert deterministically that the first POST
returns profile_id "personal-2" and the second returns "personal-3" (referencing
the test name test_post_appends_suffix_on_collision and the "profile_id" field
from the JSON bodies of the two POSTs to /api/desktop/browser/profiles) so the
collision handling is strictly verified.
…ileManager UX bugs
|
@coderabbitai full review — please re-review commit 74c3f85 which addresses the previous round (especially the atomic SQL changes for default-profile seeding race and last-profile delete race in tinyagentos/routes/desktop_browser/profile.py + store.py). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 31856 🏁 Script executed: Length of output: 65108
✅ Atomic SQL fixes — confirmed correct
|
| Area | Status |
|---|---|
Default-profile seeding race (claim_profile_init) |
✅ Fixed |
Last-profile delete race (delete_profile COUNT subquery) |
✅ Fixed |
create_profile concurrent slug collision |
🔴 Still present |
profile_id unused in extract_endpoint |
🟡 Minor |
ProfileManager undifferentiated delete error |
🟡 Minor |
live-exclusion instanceof vs cast |
🟡 Minor |
Chrome hardcoded profile colours |
🟡 Minor |
The two primary race conditions you flagged are solidly fixed. The create_profile slug collision is the most actionable remaining item.
…hip color + polish
|
@coderabbitai full review — please re-review commit cf22778, which addresses your previous round (slug-collision retry in create_profile, chip color fetch in Chrome.tsx, instanceof scoping in live-exclusion.ts, generic delete error message, and reserved-but-documented profile_id in extract_endpoint). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 49948
✅ All previous issues resolved1.
|
| Previous issue | Status |
|---|---|
create_profile concurrent slug collision |
✅ Fixed — retry loop + add_profile returns bool |
Chrome hardcoded profile colours |
✅ Fixed — live fetch from backend |
live-exclusion instanceof vs cast |
✅ Fixed — cross-realm instanceof pattern |
ProfileManager undifferentiated delete error |
✅ Fixed — generic message |
profile_id unused in extract_endpoint |
✅ Fixed — reserved + documented |
No new issues found across the reviewed files. The PR is in good shape. 🎉
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tinyagentos/routes/desktop_browser/profile.py (2)
85-87: 💤 Low valueConsider moving imports to the top of the file.
Placing
import reand theBrowserCookieStoreimport mid-file after function definitions is non-standard. If this is intentional for PR organization, consider consolidating imports at the top in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/profile.py` around lines 85 - 87, The imports "re" and "BrowserCookieStore" are placed mid-file; move them to the module import block at the top alongside other imports to follow standard style. Locate the occurrences of "import re" and "from tinyagentos.routes.desktop_browser.store import BrowserCookieStore" and relocate them to the top of tinyagentos/routes/desktop_browser/profile.py, ensuring they are grouped with existing imports and that any functions or classes (e.g., the profile route handlers) still reference these symbols without changing behavior.
117-149: 💤 Low valueSlug-collision retry logic is correct.
Using
INSERT OR IGNOREas the authoritative guard with application-level retry properly handles the race betweenlist_profilesandadd_profile. The 100-attempt cap is a reasonable safety bound.Minor: rename unused loop variable
attempt→_attemptto signal intent (per Ruff B007).♻️ Optional: rename unused loop variable
- for attempt in range(max_attempts): + for _attempt in range(max_attempts):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/profile.py` around lines 117 - 149, The unused loop variable `attempt` in the retry loop should be renamed to `_attempt` to signal it's intentionally unused and satisfy lint rule Ruff B007; locate the loop `for attempt in range(max_attempts):` and change the variable name to `_attempt` (no other logic changes).desktop/src/apps/BrowserApp/ProfileManager.test.tsx (1)
290-382: ⚡ Quick winRemove the unused first render to clean up test structure.
The test renders
ProfileManagertwice: once at lines 313-318 with a mock that's immediately overwritten, and again at lines 357-362 with the correct mock sequence. The first render is never unmounted and creates duplicate DOM elements, requiringgetAllBy*with index access patterns ([0],[deleteBtns.length - 1]).Remove lines 296-325 (the first mock setup, first render, and explanatory comments) since the second render with the proper mock sequence is the actual test.
♻️ Proposed cleanup
it("Test A: recovers orphan windows in other windows after delete", async () => { // Set up two windows: window-a on personal, window-b on work useBrowserStore.getState().createWindow("window-a", "personal"); useBrowserStore.getState().createWindow("window-b", "work"); - // Mock: DELETE succeeds, then GET list returns only personal (work was removed) - global.fetch = vi.fn().mockImplementation((url: string, opts: any) => { - if (opts?.method === "DELETE") { - return Promise.resolve({ ok: true, status: 204 }); - } - // GET /profiles — return only personal after delete - return Promise.resolve({ - ok: true, - status: 200, - json: async () => ({ - profiles: [ - { profile_id: "personal", name: "Personal", color: "#6c8df0", created_at: 0 }, - ], - }), - }); - }); - - // Render ProfileManager as if the user in window-a opened it (active = personal) - render( - <ProfileManager - activeProfileId="personal" - onClose={() => {}} - />, - ); - - // Wait for the initial profile list (which initially has personal + work) - // But we've already set up fetch to return only personal, so wait for Work to appear - // via the initial load mock — we need to override the initial load too. - // Let's wait for the list to settle: the first GET returns personal only (no work). - // Because the fetch is mocked to return personal-only for ALL GETs, the list - // will show only Personal. We won't see Work in the UI. - // To properly test the flow, we need Work in the initial list. - // Re-mock: first GET returns both, DELETE succeeds, second GET returns only personal. global.fetch = vi.fn() .mockImplementationOnce(() => Promise.resolve({ ... }), ) ...; - // Re-render with correct mock sequence - const { unmount } = render( + render( <ProfileManager activeProfileId="personal" onClose={() => {}} />, ); - await waitFor(() => screen.getAllByText("Work")[0]); + await waitFor(() => screen.getByText("Work")); // Click delete on Work - const deleteBtns = screen.getAllByLabelText(/delete profile work/i); - fireEvent.click(deleteBtns[deleteBtns.length - 1]); + const deleteBtn = screen.getByLabelText(/delete profile work/i); + fireEvent.click(deleteBtn); // Confirm the deletion - await waitFor(() => screen.getAllByText(/this also clears all saved cookies/i)[0]); - const confirmBtns = screen.getAllByLabelText(/confirm delete/i); - fireEvent.click(confirmBtns[confirmBtns.length - 1]); + await waitFor(() => screen.getByText(/this also clears all saved cookies/i)); + fireEvent.click(screen.getByLabelText(/confirm delete/i)); // After delete completes, window-b should have been switched to personal await waitFor(() => { const windowB = useBrowserStore.getState().windows["window-b"]; expect(windowB?.profileId).toBe("personal"); }); - - unmount(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/ProfileManager.test.tsx` around lines 290 - 382, Test renders ProfileManager twice causing duplicate DOM and reliance on indexed queries; remove the first (unused) render and its preceding mock so the test only sets up the intended mock sequence and single render. Specifically, delete the initial global.fetch mock block and the first render(...) call that uses ProfileManager (the render before the re-mock and re-render), leaving only the mockImplementationOnce chain, the subsequent render of ProfileManager, and the assertions that follow; ensure useBrowserStore.createWindow calls remain to set up windows "window-a" and "window-b".desktop/src/apps/BrowserApp/Chrome.test.tsx (1)
91-96: 💤 Low valuePrefer
screenqueries over rawdocument.querySelectorfor better failure diagnostics.When
dotisnull, the assertion produces"expected undefined to be 'rgb(...)'"with no indication the element was absent. Usingscreen.getByLabelTextto locate the chip first surfaces a clear "unable to find element" message instead.♻️ Suggested refactor
- await waitFor(() => { - const dot = document.querySelector( - "[aria-label='Profile: personal'] span[aria-hidden='true']", - ) as HTMLElement | null; - expect(dot?.style.backgroundColor).toBe("rgb(171, 205, 239)"); // `#abcdef` in rgb - }); + await waitFor(() => { + const chip = screen.getByLabelText(/profile: personal/i); + const dot = chip.querySelector("span[aria-hidden='true']") as HTMLElement | null; + expect(dot).not.toBeNull(); + expect(dot!.style.backgroundColor).toBe("rgb(171, 205, 239)"); // `#abcdef` in rgb + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/BrowserApp/Chrome.test.tsx` around lines 91 - 96, Replace the raw document.querySelector call in the test's waitFor block with Testing Library's screen query to improve diagnostics: use screen.getByLabelText("Profile: personal") to retrieve the chip element (this will throw a helpful error if missing), then locate the inner span (e.g., via chip.querySelector("span[aria-hidden='true']") or within(chip).getByRole if appropriate) and assert its style.backgroundColor equals "rgb(171, 205, 239)"; update the test that currently calls waitFor and document.querySelector to use screen.getByLabelText and the chip variable instead (keep the waitFor around the assertion if asynchronous).tinyagentos/routes/desktop_browser/extract.py (2)
63-65: 💤 Low valueSilent
except: passhides lxml parse failures (Ruff S110).When
lxml_html.fromstringraises, the error is silently eaten andtextstays empty.word_countwill be 0 and the caller gets back a suspiciously empty result with no indication of what went wrong. A single_logger.debugcall preserves the "never raises" contract while aiding diagnosis.♻️ Proposed fix
except Exception: - # Empty / malformed summary — fall through with empty text - pass + _logger.debug("lxml text_content failed for extracted summary; returning empty text")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/extract.py` around lines 63 - 65, The bare except in the HTML parsing block swallows lxml_html.fromstring errors leaving text empty and word_count 0; change the except in the extract logic to capture the exception (e.g., except Exception as e) and emit a debug log via _logger.debug including the exception message/stack (and the URL or snippet context if available) so failures are recorded while still falling through to the empty text behavior; update references in the same block that compute text/word_count to remain unchanged.
53-55: PassurltoDocumentconstructor to absolutize relative links in extracted summary HTML.The
extract_readablefunction receivesurlas a parameter but doesn't forward it toDocument. Thereadability-lxmllibrary'sDocumentclass accepts aurlkeyword argument and uses it internally to callmake_links_absolute, convertinghref="/path"andsrc="./img.jpg"into full URLs. Without this, relative links and images in the summary HTML will be broken.♻️ One-line fix
- doc = Document(decoded) + doc = Document(decoded, url=url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/desktop_browser/extract.py` around lines 53 - 55, The Document is constructed without the source URL so relative links in summary_html stay relative; change the call that creates the Document (the Document(...) invocation inside extract_readable) to pass the received url argument (e.g., Document(decoded, url=url)) so that Document.make_links_absolute runs and doc.summary(html_partial=True) returns absolute href/src values; keep the rest of the logic (title via doc.short_title() or doc.title()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tinyagentos/routes/desktop_browser/extract.py`:
- Around line 107-111: The code currently creates an httpx.AsyncClient with
follow_redirects=True which allows silent 3xx hops to bypass SSRF checks; change
the client to use follow_redirects=False in the httpx.AsyncClient call (where
response = await http.get(url) is invoked) and either reject 3xx responses or
implement manual hop handling: on 3xx inspect the Location header, call
validate_url_or_raise(Location) for each redirect target, then perform a new GET
to the validated Location. Ensure validate_url_or_raise is used before any
secondary fetch so every redirect hop is re-validated.
---
Nitpick comments:
In `@desktop/src/apps/BrowserApp/Chrome.test.tsx`:
- Around line 91-96: Replace the raw document.querySelector call in the test's
waitFor block with Testing Library's screen query to improve diagnostics: use
screen.getByLabelText("Profile: personal") to retrieve the chip element (this
will throw a helpful error if missing), then locate the inner span (e.g., via
chip.querySelector("span[aria-hidden='true']") or within(chip).getByRole if
appropriate) and assert its style.backgroundColor equals "rgb(171, 205, 239)";
update the test that currently calls waitFor and document.querySelector to use
screen.getByLabelText and the chip variable instead (keep the waitFor around the
assertion if asynchronous).
In `@desktop/src/apps/BrowserApp/ProfileManager.test.tsx`:
- Around line 290-382: Test renders ProfileManager twice causing duplicate DOM
and reliance on indexed queries; remove the first (unused) render and its
preceding mock so the test only sets up the intended mock sequence and single
render. Specifically, delete the initial global.fetch mock block and the first
render(...) call that uses ProfileManager (the render before the re-mock and
re-render), leaving only the mockImplementationOnce chain, the subsequent render
of ProfileManager, and the assertions that follow; ensure
useBrowserStore.createWindow calls remain to set up windows "window-a" and
"window-b".
In `@tinyagentos/routes/desktop_browser/extract.py`:
- Around line 63-65: The bare except in the HTML parsing block swallows
lxml_html.fromstring errors leaving text empty and word_count 0; change the
except in the extract logic to capture the exception (e.g., except Exception as
e) and emit a debug log via _logger.debug including the exception message/stack
(and the URL or snippet context if available) so failures are recorded while
still falling through to the empty text behavior; update references in the same
block that compute text/word_count to remain unchanged.
- Around line 53-55: The Document is constructed without the source URL so
relative links in summary_html stay relative; change the call that creates the
Document (the Document(...) invocation inside extract_readable) to pass the
received url argument (e.g., Document(decoded, url=url)) so that
Document.make_links_absolute runs and doc.summary(html_partial=True) returns
absolute href/src values; keep the rest of the logic (title via
doc.short_title() or doc.title()) unchanged.
In `@tinyagentos/routes/desktop_browser/profile.py`:
- Around line 85-87: The imports "re" and "BrowserCookieStore" are placed
mid-file; move them to the module import block at the top alongside other
imports to follow standard style. Locate the occurrences of "import re" and
"from tinyagentos.routes.desktop_browser.store import BrowserCookieStore" and
relocate them to the top of tinyagentos/routes/desktop_browser/profile.py,
ensuring they are grouped with existing imports and that any functions or
classes (e.g., the profile route handlers) still reference these symbols without
changing behavior.
- Around line 117-149: The unused loop variable `attempt` in the retry loop
should be renamed to `_attempt` to signal it's intentionally unused and satisfy
lint rule Ruff B007; locate the loop `for attempt in range(max_attempts):` and
change the variable name to `_attempt` (no other logic changes).
🪄 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: 7bb36690-e02a-4f49-a6ef-378e7eda0bfd
📒 Files selected for processing (11)
desktop/src/apps/BrowserApp/Chrome.test.tsxdesktop/src/apps/BrowserApp/Chrome.tsxdesktop/src/apps/BrowserApp/ProfileManager.test.tsxdesktop/src/apps/BrowserApp/ProfileManager.tsxdesktop/src/apps/BrowserApp/live-exclusion.tsdesktop/src/stores/browser-settings-store.test.tstests/routes/desktop_browser/test_profile_routes.pytinyagentos/routes/desktop_browser/extract.pytinyagentos/routes/desktop_browser/profile.pytinyagentos/routes/desktop_browser/profile_routes.pytinyagentos/routes/desktop_browser/store.py
🚧 Files skipped from review as they are similar to previous changes (6)
- desktop/src/apps/BrowserApp/live-exclusion.ts
- desktop/src/stores/browser-settings-store.test.ts
- desktop/src/apps/BrowserApp/Chrome.tsx
- desktop/src/apps/BrowserApp/ProfileManager.tsx
- tinyagentos/routes/desktop_browser/profile_routes.py
- tests/routes/desktop_browser/test_profile_routes.py
…-review - extract.py: walk redirects manually with follow_redirects=False; re-run validate_url_or_raise on each hop Location header (mirrors proxy.py pattern); return 403 on SSRF block, 502 on chain > 5 hops - extract.py: pass url= to Document() so readability-lxml absolutizes relative links - extract.py: replace silent except: pass with _logger.debug for visibility - profile.py: move mid-file `import re` and BrowserCookieStore import to top - profile.py: rename loop variable attempt -> _attempt (Ruff B007) - ProfileManager.test.tsx: remove orphan double-render in Test A; switch getAllBy* with index to getBy* now that there is a single tree - Chrome.test.tsx: replace document.querySelector with screen.getByLabelText for better failure diagnostics in the chip-color test - tests/test_extract.py: add 4 SSRF redirect regression tests covering RFC1918, AWS IMDS, chain-too-long, and normal redirect success cases
|
@coderabbitai full review — please re-review commit f3117c7. Critical fix: extract.py SSRF redirect bypass closed by switching follow_redirects=False + per-hop validate_url_or_raise loop (mirroring proxy.py). Plus Document(url=url) for absolute links, _logger.debug for lxml fallback, profile.py import cleanup, _attempt rename, ProfileManager.test.tsx double-render cleanup, Chrome.test.tsx screen.getByLabelText. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 31471
✅ All stated fixes verified1. SSRF redirect bypass —
|
| Item | Status |
|---|---|
SSRF redirect bypass (follow_redirects=False + per-hop validate) |
✅ Fixed |
Document(url=url) for absolute links |
✅ Fixed |
_logger.debug for lxml fallback |
✅ Fixed |
profile.py import cleanup |
✅ Fixed |
_attempt rename |
✅ Fixed |
ProfileManager.test.tsx double-render cleanup |
✅ Fixed |
Chrome.test.tsx screen.getByLabelText |
✅ Fixed |
urlparse dead import in extract.py |
🟡 New / Minor |
url vs fetch_url in extract_readable call |
🟡 New / Minor |
The critical SSRF fix is solid. Only two low-severity items remain; the fetch_url one is the more actionable of the two.
1 participant
Summary
Three quality-of-life upgrades that turn PR 4's working browser into a daily driver:
/api/desktop/browser/extract) plus a URL-bar toggle that appears when an article has >200 words. Article view renders DOMPurify-sanitised HTML with font-size and serif/sans-serif controls. Lazy: extract fires on address-bar focus, not on every navigation. Stale-fetch guard discards a result if the user navigated away mid-fetch.Per-(window, profile) tab snapshots ride along: switching profiles saves the current tab set under the old profile and restores any prior snapshot for the new profile.
Files
profile.py,profile_routes.py,store.py), Readability extract endpoint (extract.py).ProfileSwitcher.tsx,ProfileManager.tsx,SettingsPanel.tsx,ReaderMode.tsx,live-exclusion.ts,browser-settings-store.ts, plusbrowser-extract-api.tsandbrowser-profile-api.tsclients.Chrome.tsx(gear icon + chip),AddressBar.tsx(Reader toggle + lazy extract + search-engine wired through),TabRenderer.tsx(live-exclusion in scheduler, ReaderMode renders over hidden iframe),browser-store.ts(switchProfile,setTabReader, reader-state reset onnavigateTab/goBack/goForward), Tab type extensions.Test plan
Deferred (follow-ups)
Spec / plan references
Summary by CodeRabbit
New Features
Tests