refactor(browser): split BrowserStore into per-domain mixins (B4)#554
Merged
Conversation
📝 WalkthroughWalkthroughBrowserStore refactors from a monolithic 1200+-line class into nine focused mixins (Profiles, Windows, History, Bookmarks, Pins, Capabilities, DriveSession, SitePermissions, Push) plus BaseStore composition. Each mixin provides database-backed CRUD and query operations for its domain while the store coordinates these behaviors. ChangesStore mixin composition refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if not user_id or not profile_id: | ||
| raise ValueError("user_id and profile_id required") | ||
| assert self._db is not None | ||
| like = f"%{query}%" |
There was a problem hiding this comment.
WARNING: The search query is used to build a LIKE pattern with wildcards on both sides. This allows the user to use % and _ as wildcards in their search, which may be unintended if literal matching is expected. Consider escaping the user's input or documenting the behavior.
| await self._db.execute( | ||
| "INSERT OR REPLACE INTO site_permissions " | ||
| "(user_id, profile_id, host_pattern, permission, state, granted_at) " | ||
| "VALUES (?, ?, ?, ?, ?, ?)", |
There was a problem hiding this comment.
WARNING: The host_pattern is not validated when setting a site permission. An invalid pattern (e.g., containing multiple asterisks) may lead to unexpected matching behavior. Consider validating that the pattern is either '', or starts with '.' followed by a string that does not contain any asterisks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tinyagentos/routes/desktop_browser/store_mixins/push.py (1)
152-162: 💤 Low valueConsider adding validation for consistency.
This method skips validation unlike other methods in the mixin. While it's a read-only operation (so invalid inputs just return
False), validatingkindagainst_PUSH_MUTE_KINDSwould catch caller bugs early—especially sinceset_push_mutevalidates it.🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/push.py` around lines 152 - 162, The is_push_muted method lacks the same validation other mixin methods perform; add a check at the start of is_push_muted to validate the kind argument against _PUSH_MUTE_KINDS (same way set_push_mute does) and raise a ValueError (or assert) on invalid kinds, so callers get immediate feedback about bad inputs before the DB query; reference the is_push_muted function and the _PUSH_MUTE_KINDS constant when making the change.
🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/bookmarks.py`:
- Around line 29-57: In list_bookmarks ensure limit is validated and enforced
before running the SQL: check that the limit parameter is an int > 0 (and
optionally clamp to a max like 1000) and raise ValueError for non-positive
values; then use the validated/clamped value in the SQL parameter passed to
self._db.execute so callers cannot pass a negative value that turns into an
unbounded SQLite LIMIT.
In `@tinyagentos/routes/desktop_browser/store_mixins/drive_sessions.py`:
- Line 79: Validate idle_timeout_s at the start of any method that uses it for
cutoff arithmetic (e.g., prune_expired_drive_sessions and any other method that
computes cutoff using idle_timeout_s): add a guard that requires idle_timeout_s
> 0 and return early (no-op) or raise a clear error if the value is non-positive
to avoid producing a future cutoff and accidentally pruning active sessions;
ensure the check is applied before computing cutoff = now - idle_timeout_s so
all timeout-based logic is protected.
In `@tinyagentos/routes/desktop_browser/store_mixins/history.py`:
- Around line 31-46: The method performing the history lookup currently forwards
the `limit` directly into the SQL LIMIT, so pass a negative or zero `limit` can
produce an unbounded query; update the function that has parameters (query: str,
limit: int = 8) to validate `limit` before calling self._db.execute: if limit <=
0 raise a ValueError (e.g., "limit must be positive"), and optionally clamp
overly large values to a safe maximum before building the `like` and calling
self._db.execute (refer to the `limit` parameter and the `_db.execute` call in
this function).
In `@tinyagentos/routes/desktop_browser/store_mixins/pins.py`:
- Around line 92-139: The read methods skip the same required-id validation that
add_pin/delete_pin perform; update list_pins_for_tab and count_pins_for_tab to
call the same validation helpers for user/profile/tab (e.g.,
self._require_user_id(user_id), self._require_profile_id(profile_id),
self._require_tab_id(tab_id)) at the top of each method, and update
list_pins_for_user to call self._require_user_id(user_id) before DB access,
leaving the rest of the logic unchanged.
- Around line 36-90: In add_pin_if_under_cap, validate the max_pins argument up
front and raise a clear error for non-positive values (e.g., if max_pins <= 0:
raise ValueError("max_pins must be a positive integer")), so callers fail fast
instead of silently returning "at_cap"/"duplicate"; add this guard near the
existing user_id/profile_id/tab_id/agent_id checks before using max_pins in the
INSERT SELECT.
In `@tinyagentos/routes/desktop_browser/store_mixins/push.py`:
- Around line 113-122: In set_push_mute validate that user_id and agent_id are
non-empty strings (same style used in delete_push_subscription) before any DB
operations: check user_id and agent_id and raise ValueError with a clear message
if either is missing/empty, then proceed to validate kind against
_PUSH_MUTE_KINDS and perform the insert/delete; reference the set_push_mute
method and mirror the parameter validation used elsewhere in this mixin to
prevent inserting rows with empty identifiers.
---
Nitpick comments:
In `@tinyagentos/routes/desktop_browser/store_mixins/push.py`:
- Around line 152-162: The is_push_muted method lacks the same validation other
mixin methods perform; add a check at the start of is_push_muted to validate the
kind argument against _PUSH_MUTE_KINDS (same way set_push_mute does) and raise a
ValueError (or assert) on invalid kinds, so callers get immediate feedback about
bad inputs before the DB query; reference the is_push_muted function and the
_PUSH_MUTE_KINDS constant when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 789b6a3b-c503-49dd-93ac-adb1cb14d7e9
📒 Files selected for processing (11)
tinyagentos/routes/desktop_browser/store.pytinyagentos/routes/desktop_browser/store_mixins/__init__.pytinyagentos/routes/desktop_browser/store_mixins/bookmarks.pytinyagentos/routes/desktop_browser/store_mixins/capabilities.pytinyagentos/routes/desktop_browser/store_mixins/drive_sessions.pytinyagentos/routes/desktop_browser/store_mixins/history.pytinyagentos/routes/desktop_browser/store_mixins/pins.pytinyagentos/routes/desktop_browser/store_mixins/profiles.pytinyagentos/routes/desktop_browser/store_mixins/push.pytinyagentos/routes/desktop_browser/store_mixins/site_permissions.pytinyagentos/routes/desktop_browser/store_mixins/windows.py
Comment on lines
+29
to
+57
| async def list_bookmarks( | ||
| self, | ||
| *, | ||
| user_id: str, | ||
| profile_id: str, | ||
| query: str | None = None, | ||
| limit: int = 100, | ||
| ) -> list[dict]: | ||
| """Return bookmarks for (user, profile), optionally substring-filtered.""" | ||
| if not user_id or not profile_id: | ||
| raise ValueError("user_id and profile_id required") | ||
| assert self._db is not None | ||
| if query: | ||
| like = f"%{query}%" | ||
| cursor = await self._db.execute( | ||
| "SELECT bookmark_id, folder_path, url, title, created_at " | ||
| "FROM bookmarks " | ||
| "WHERE user_id = ? AND profile_id = ? " | ||
| " AND (url LIKE ? OR title LIKE ?) " | ||
| "ORDER BY created_at DESC LIMIT ?", | ||
| (user_id, profile_id, like, like, limit), | ||
| ) | ||
| else: | ||
| cursor = await self._db.execute( | ||
| "SELECT bookmark_id, folder_path, url, title, created_at " | ||
| "FROM bookmarks " | ||
| "WHERE user_id = ? AND profile_id = ? " | ||
| "ORDER BY created_at DESC LIMIT ?", | ||
| (user_id, profile_id, limit), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does SQLite interpret a negative LIMIT clause?
💡 Result:
In SQLite, when a LIMIT clause evaluates to a negative value, it is interpreted as "no limit" [1][2]. This means there is no upper bound on the number of rows returned by the query [1]. Consequently, the database will return all rows that meet the other criteria of the statement [1][2]. This behavior applies to both SELECT statements [1] and, when the SQLITE_ENABLE_UPDATE_DELETE_LIMIT compile-time option is enabled, DELETE statements [2]. In contrast, if an OFFSET clause evaluates to a negative value, SQLite treats it as if it were zero [1][2].
Citations:
Reject non-positive limit values in list_bookmarks
limit is passed directly into LIMIT ?; in SQLite, a negative LIMIT means “no limit”, so a bad caller can turn this into an unbounded scan. Validate limit > 0 (and optionally clamp to a reasonable max) before executing the query.
🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/bookmarks.py` around lines 29
- 57, In list_bookmarks ensure limit is validated and enforced before running
the SQL: check that the limit parameter is an int > 0 (and optionally clamp to a
max like 1000) and raise ValueError for non-positive values; then use the
validated/clamped value in the SQL parameter passed to self._db.execute so
callers cannot pass a negative value that turns into an unbounded SQLite LIMIT.
| profile_id: str, | ||
| tab_id: str, | ||
| agent_id: str, | ||
| idle_timeout_s: float = 30.0, |
There was a problem hiding this comment.
Validate idle_timeout_s before time arithmetic to prevent accidental mass pruning.
A negative timeout makes cutoff a future timestamp, so prune_expired_drive_sessions can delete active sessions unexpectedly. Add an explicit non-negative guard (ideally strict > 0) at method entry for both timeout-based methods.
Proposed fix
async def is_driving(
@@
- idle_timeout_s: float = 30.0,
+ idle_timeout_s: float = 30.0,
) -> bool:
"""True iff a row exists AND (now - last_op_at) < idle_timeout_s."""
+ if idle_timeout_s <= 0:
+ raise ValueError("idle_timeout_s must be > 0")
assert self._db is not None
@@
async def prune_expired_drive_sessions(
@@
- idle_timeout_s: float = 30.0,
+ idle_timeout_s: float = 30.0,
) -> int:
"""Atomically delete rows idle for >= idle_timeout_s. Returns rowcount."""
+ if idle_timeout_s <= 0:
+ raise ValueError("idle_timeout_s must be > 0")
assert self._db is not NoneAlso applies to: 98-105
🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/drive_sessions.py` at line
79, Validate idle_timeout_s at the start of any method that uses it for cutoff
arithmetic (e.g., prune_expired_drive_sessions and any other method that
computes cutoff using idle_timeout_s): add a guard that requires idle_timeout_s
> 0 and return early (no-op) or raise a clear error if the value is non-positive
to avoid producing a future cutoff and accidentally pruning active sessions;
ensure the check is applied before computing cutoff = now - idle_timeout_s so
all timeout-based logic is protected.
Comment on lines
+31
to
+46
| query: str, | ||
| limit: int = 8, | ||
| ) -> list[dict]: | ||
| """Substring match on url + title for the (user, profile). Most-recent first.""" | ||
| if not user_id or not profile_id: | ||
| raise ValueError("user_id and profile_id required") | ||
| assert self._db is not None | ||
| like = f"%{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 ?) " | ||
| "ORDER BY visited_at DESC " | ||
| "LIMIT ?", | ||
| (user_id, profile_id, like, like, limit), |
There was a problem hiding this comment.
Reject non-positive history limits.
limit is passed straight into LIMIT ?, so a caller can send a negative value and bypass the cap entirely. That turns this into an unbounded history query instead of the small bounded lookup the method advertises.
Suggested fix
async def search_history(
self,
*,
user_id: str,
profile_id: str,
query: str,
limit: int = 8,
) -> list[dict]:
"""Substring match on url + title for the (user, profile). Most-recent first."""
if not user_id or not profile_id:
raise ValueError("user_id and profile_id required")
+ if limit <= 0:
+ raise ValueError("limit must be positive")
assert self._db is not None
like = f"%{query}%"
cursor = await self._db.execute(🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/history.py` around lines 31 -
46, The method performing the history lookup currently forwards the `limit`
directly into the SQL LIMIT, so pass a negative or zero `limit` can produce an
unbounded query; update the function that has parameters (query: str, limit: int
= 8) to validate `limit` before calling self._db.execute: if limit <= 0 raise a
ValueError (e.g., "limit must be positive"), and optionally clamp overly large
values to a safe maximum before building the `like` and calling self._db.execute
(refer to the `limit` parameter and the `_db.execute` call in this function).
Comment on lines
+36
to
+90
| async def add_pin_if_under_cap( | ||
| self, | ||
| *, | ||
| user_id: str, | ||
| profile_id: str, | ||
| tab_id: str, | ||
| agent_id: str, | ||
| max_pins: int, | ||
| ) -> str: | ||
| """Atomic INSERT-with-cap. Returns one of: | ||
| - "added" — pin newly created | ||
| - "duplicate" — pin already existed for this tuple | ||
| - "at_cap" — tab already at max_pins, cannot add new pin | ||
|
|
||
| The cap check and INSERT happen in one SQL statement so two concurrent | ||
| calls cannot both pass an N=3 check and produce N=5. Tested at | ||
| concurrency to lock the invariant in. | ||
| """ | ||
| if not user_id: | ||
| raise ValueError("user_id is required") | ||
| if not profile_id: | ||
| raise ValueError("profile_id is required") | ||
| if not tab_id: | ||
| raise ValueError("tab_id is required") | ||
| if not agent_id: | ||
| raise ValueError("agent_id is required") | ||
| assert self._db is not None | ||
| pinned_at = datetime.now(timezone.utc).isoformat() | ||
| cursor = await self._db.execute( | ||
| """ | ||
| INSERT OR IGNORE INTO agent_pins | ||
| (user_id, profile_id, tab_id, agent_id, pinned_at) | ||
| SELECT ?, ?, ?, ?, ? | ||
| WHERE ( | ||
| SELECT COUNT(*) FROM agent_pins | ||
| WHERE user_id = ? AND profile_id = ? AND tab_id = ? | ||
| ) < ? | ||
| """, | ||
| ( | ||
| user_id, profile_id, tab_id, agent_id, pinned_at, | ||
| user_id, profile_id, tab_id, max_pins, | ||
| ), | ||
| ) | ||
| await self._db.commit() | ||
| if cursor.rowcount > 0: | ||
| return "added" | ||
| # rowcount==0 — either duplicate (PK violation, swallowed by IGNORE) | ||
| # or at-cap (WHERE clause failed). Disambiguate with a single SELECT. | ||
| check = await self._db.execute( | ||
| "SELECT 1 FROM agent_pins " | ||
| "WHERE user_id = ? AND profile_id = ? AND tab_id = ? AND agent_id = ?", | ||
| (user_id, profile_id, tab_id, agent_id), | ||
| ) | ||
| existing = await check.fetchone() | ||
| return "duplicate" if existing else "at_cap" |
There was a problem hiding this comment.
Validate max_pins before the insert.
max_pins <= 0 currently falls through to "at_cap"/"duplicate" instead of failing fast, which hides configuration bugs and can make a tab look permanently full. Add an upfront guard here so callers get a clear error for impossible caps.
Suggested fix
async def add_pin_if_under_cap(
self,
*,
user_id: str,
profile_id: str,
tab_id: str,
agent_id: str,
max_pins: int,
) -> str:
@@
if not agent_id:
raise ValueError("agent_id is required")
+ if max_pins < 1:
+ raise ValueError("max_pins must be >= 1")
assert self._db is not None🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/pins.py` around lines 36 -
90, In add_pin_if_under_cap, validate the max_pins argument up front and raise a
clear error for non-positive values (e.g., if max_pins <= 0: raise
ValueError("max_pins must be a positive integer")), so callers fail fast instead
of silently returning "at_cap"/"duplicate"; add this guard near the existing
user_id/profile_id/tab_id/agent_id checks before using max_pins in the INSERT
SELECT.
Comment on lines
+92
to
+139
| async def list_pins_for_tab( | ||
| self, | ||
| *, | ||
| user_id: str, | ||
| profile_id: str, | ||
| tab_id: str, | ||
| ) -> list[dict]: | ||
| """Returns list of {agent_id, pinned_at} ordered by pinned_at ASC.""" | ||
| assert self._db is not None | ||
| cursor = await self._db.execute( | ||
| "SELECT agent_id, pinned_at FROM agent_pins " | ||
| "WHERE user_id = ? AND profile_id = ? AND tab_id = ? " | ||
| "ORDER BY pinned_at ASC", | ||
| (user_id, profile_id, tab_id), | ||
| ) | ||
| rows = await cursor.fetchall() | ||
| return [{"agent_id": r[0], "pinned_at": r[1]} for r in rows] | ||
|
|
||
| async def list_pins_for_user(self, *, user_id: str) -> list[dict]: | ||
| """Returns list of {profile_id, tab_id, agent_id, pinned_at}.""" | ||
| assert self._db is not None | ||
| cursor = await self._db.execute( | ||
| "SELECT profile_id, tab_id, agent_id, pinned_at FROM agent_pins " | ||
| "WHERE user_id = ? ORDER BY pinned_at ASC", | ||
| (user_id,), | ||
| ) | ||
| rows = await cursor.fetchall() | ||
| return [ | ||
| {"profile_id": r[0], "tab_id": r[1], "agent_id": r[2], "pinned_at": r[3]} | ||
| for r in rows | ||
| ] | ||
|
|
||
| async def count_pins_for_tab( | ||
| self, | ||
| *, | ||
| user_id: str, | ||
| profile_id: str, | ||
| tab_id: str, | ||
| ) -> int: | ||
| """Returns the number of pins on the (user, profile, tab) tuple.""" | ||
| assert self._db is not None | ||
| cursor = await self._db.execute( | ||
| "SELECT COUNT(*) FROM agent_pins " | ||
| "WHERE user_id = ? AND profile_id = ? AND tab_id = ?", | ||
| (user_id, profile_id, tab_id), | ||
| ) | ||
| row = await cursor.fetchone() | ||
| return row[0] if row else 0 |
There was a problem hiding this comment.
Keep the read-side API consistent with the write-side validation.
list_pins_for_tab, list_pins_for_user, and count_pins_for_tab skip the required-id checks that add_pin and delete_pin already enforce. Right now malformed calls degrade to empty results or 0, which masks upstream bugs and makes the pins API inconsistent.
Suggested fix
async def list_pins_for_tab(
self,
*,
user_id: str,
profile_id: str,
tab_id: str,
) -> list[dict]:
"""Returns list of {agent_id, pinned_at} ordered by pinned_at ASC."""
+ if not user_id:
+ raise ValueError("user_id is required")
+ if not profile_id:
+ raise ValueError("profile_id is required")
+ if not tab_id:
+ raise ValueError("tab_id is required")
assert self._db is not None
@@
async def list_pins_for_user(self, *, user_id: str) -> list[dict]:
"""Returns list of {profile_id, tab_id, agent_id, pinned_at}."""
+ if not user_id:
+ raise ValueError("user_id is required")
assert self._db is not None
@@
async def count_pins_for_tab(
self,
*,
user_id: str,
profile_id: str,
tab_id: str,
) -> int:
"""Returns the number of pins on the (user, profile, tab) tuple."""
+ if not user_id:
+ raise ValueError("user_id is required")
+ if not profile_id:
+ raise ValueError("profile_id is required")
+ if not tab_id:
+ raise ValueError("tab_id is required")
assert self._db is not None🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/pins.py` around lines 92 -
139, The read methods skip the same required-id validation that
add_pin/delete_pin perform; update list_pins_for_tab and count_pins_for_tab to
call the same validation helpers for user/profile/tab (e.g.,
self._require_user_id(user_id), self._require_profile_id(profile_id),
self._require_tab_id(tab_id)) at the top of each method, and update
list_pins_for_user to call self._require_user_id(user_id) before DB access,
leaving the rest of the logic unchanged.
Comment on lines
+113
to
+122
| async def set_push_mute( | ||
| self, user_id: str, agent_id: str, kind: str, muted: bool | ||
| ) -> None: | ||
| """Set or clear a per-(agent, kind) mute. | ||
|
|
||
| muted=True inserts (or replaces), muted=False deletes. | ||
| Raises ValueError if kind is not in _PUSH_MUTE_KINDS. | ||
| """ | ||
| if kind not in _PUSH_MUTE_KINDS: | ||
| raise ValueError(f"invalid kind: {kind}") |
There was a problem hiding this comment.
Missing validation for user_id and agent_id.
Other methods in this mixin validate required string parameters before database operations (e.g., delete_push_subscription validates both IDs). This method only validates kind but not user_id or agent_id, which could lead to inserting rows with empty identifiers.
Proposed fix
async def set_push_mute(
self, user_id: str, agent_id: str, kind: str, muted: bool
) -> None:
"""Set or clear a per-(agent, kind) mute.
muted=True inserts (or replaces), muted=False deletes.
Raises ValueError if kind is not in _PUSH_MUTE_KINDS.
"""
+ if not user_id:
+ raise ValueError("user_id is required")
+ if not agent_id:
+ raise ValueError("agent_id is required")
if kind not in _PUSH_MUTE_KINDS:
raise ValueError(f"invalid kind: {kind}")🤖 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 `@tinyagentos/routes/desktop_browser/store_mixins/push.py` around lines 113 -
122, In set_push_mute validate that user_id and agent_id are non-empty strings
(same style used in delete_push_subscription) before any DB operations: check
user_id and agent_id and raise ValueError with a clear message if either is
missing/empty, then proceed to validate kind against _PUSH_MUTE_KINDS and
perform the insert/delete; reference the set_push_mute method and mirror the
parameter validation used elsewhere in this mixin to prevent inserting rows with
empty identifiers.
|
Test comment |
|
Test |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
|
|
test |
2 similar comments
|
test |
|
test |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
|
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
WARNING
|
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
WARNING
Files Reviewed (10 files)
|
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
WARNING
Files Reviewed (10 files)
Reviewed by nemotron-3-super-120b-a12b-20230311:free · 1,417,124 tokens |
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.
Modularity plan B4. Splits the 1204-line
BrowserStoregod-class inroutes/desktop_browser/store.pyinto 9 per-domain mixins understore_mixins/(profiles, windows, history, bookmarks, pins, capabilities, drive_sessions, site_permissions, push). Methods copied verbatim;BrowserStorenow composes the mixins withBaseStoreLAST in the bases (so mixin methods take precedence and BaseStore provides__init__/helpers via MRO).SCHEMA = BROWSER_SCHEMAand theBrowserCookieStorere-export preserved.store.py 1204 → 37 lines. All 45 methods confirmed still callable on
BrowserStore; no circular import. Full desktop_browser suite (560 tests) passes.Summary by CodeRabbit
The internal structure of the browser store has been restructured into modular components handling bookmarks, history, profiles, windows, agent permissions, push notifications, and site settings. No changes to user-facing functionality.