Speed up workspace listing and search; add FTS index and fix search d…#113
Open
clean6378-max-it wants to merge 5 commits into
Open
Speed up workspace listing and search; add FTS index and fix search d…#113clean6378-max-it wants to merge 5 commits into
clean6378-max-it wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a local SQLite FTS5 search index with background daemon-thread refresh and atomic pointer-based publication. Extends all three search sources ( ChangesSearch Index, Time-Windowed Search, and Workspace List/Tab Count Alignment
Sequence Diagram(s)sequenceDiagram
participant Browser
participant SearchRoute as /api/search
participant resolve as resolve_search_since_ms
participant GlobalSearch as search_global_storage
participant IndexCheck as index_is_usable
participant FTSIndex as _search_global_storage_via_index
participant LiveScan as _search_global_storage_live_scan
Browser->>SearchRoute: GET /api/search?q=export&all_history=1
SearchRoute->>resolve: resolve_search_since_ms(all_history=True)
resolve-->>SearchRoute: since_ms = None
SearchRoute->>GlobalSearch: search_global_storage(q, since_ms=None)
GlobalSearch->>IndexCheck: index_is_usable(workspace_path, rules)
alt FTS index enabled and ready
IndexCheck-->>GlobalSearch: True
GlobalSearch->>FTSIndex: query_composer_bubble_hits + query_composer_title_hits
FTSIndex-->>GlobalSearch: matched composers (since_ms=None → unwindowed)
else FTS index disabled or stale
IndexCheck-->>GlobalSearch: False
GlobalSearch->>LiveScan: full SQLite scan with since_ms=None filter
LiveScan-->>GlobalSearch: all composers (no timestamp filter)
end
GlobalSearch-->>SearchRoute: list[SearchResult] with timestamps
SearchRoute-->>Browser: {results: [...], allHistory: true, searchWindowDays: null}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_search_index.py (1)
135-147: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest intent and assertion are currently mismatched.
Line 135 says this verifies index usage, but Lines 138-147 only prove a hit exists; the same assertion still passes if
search_global_storagefalls back to live scan. Add an explicit assertion that live-scan fallback is not invoked.🧪 Proposed hardening
class TestSearchGlobalStorageUsesIndex: def test_search_global_storage_uses_index(self, indexed_storage): patches = _index_patches(indexed_storage["cache_dir"]) with patches[0], patches[1], patches[2]: - results = search_global_storage( - indexed_storage["ws_root"], - indexed_storage["term"], - indexed_storage["term"].lower(), - [], - ParseWarningCollector(), - since_ms=None, - ) - assert any(r["chatId"] == indexed_storage["composer_id"] for r in results) + with patch("services.search._search_global_storage_live_scan") as live_scan: + results = search_global_storage( + indexed_storage["ws_root"], + indexed_storage["term"], + indexed_storage["term"].lower(), + [], + ParseWarningCollector(), + since_ms=None, + ) + live_scan.assert_not_called() + assert any(r["chatId"] == indexed_storage["composer_id"] for r in results)🤖 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 `@tests/test_search_index.py` around lines 135 - 147, The test method test_search_global_storage_uses_index sets up patches to enable index usage but the current assertion only verifies that results were found, which could still pass if the code falls back to live scan instead of using the index. Add an explicit assertion after the results are returned that verifies the live-scan fallback mechanism was not invoked, such as checking that a specific mock or patch related to the fallback was not called, to ensure the test actually proves that search_global_storage used the index as intended.
🧹 Nitpick comments (3)
services/workspace_resolver.py (1)
112-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
workspace.jsonexception handling.Ruff flags both broad
except Exceptionhandlers here. Catch the expected read/parse/path failures explicitly so unexpected defects still surface.♻️ Proposed fix
- except Exception as e: + except (OSError, ValueError, TypeError, KeyError) as e: warn_workspace_json_read(_logger, workspace_id, e) @@ - except Exception as e: + except (OSError, ValueError, TypeError, KeyError) as e: warn_workspace_json_read(_logger, entry["name"], e)🤖 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 `@services/workspace_resolver.py` around lines 112 - 124, Replace the two broad except Exception handlers with specific exception handling. In the first except block around the warn_workspace_json_read call for the initial workspace.json read, catch the specific exceptions that read_json_file and get_workspace_folder_paths would raise (such as FileNotFoundError, JSONDecodeError, KeyError, and ValueError). In the second except block within the loop, similarly catch only the expected exceptions from read_json_file, get_workspace_folder_paths, normalize_file_path operations rather than catching all Exception types, so that unexpected errors will still propagate and surface during debugging instead of being silently caught.Source: Linters/SAST tools
tests/conftest.py (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroaden the index-test exemption matcher.
The exemption only matches module names ending with
test_search_index, so index-focused modules liketest_search_index_windowsstill getCURSOR_CHAT_BROWSER_NO_SEARCH_INDEX=1. That can silently alter what those tests actually validate.Suggested tweak
- if request.module.__name__.endswith("test_search_index"): + if request.module.__name__.endswith( + ("test_search_index", "test_search_index_windows") + ): return🤖 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 `@tests/conftest.py` around lines 30 - 31, The endswith check in the conftest.py file only matches module names ending with exactly "test_search_index", so index-focused modules like "test_search_index_windows" bypass the exemption and still get the CURSOR_CHAT_BROWSER_NO_SEARCH_INDEX environment variable applied. Broaden the condition that checks request.module.__name__ to use a pattern match (such as the `in` operator or a substring search) that will catch any module name containing "test_search_index" as part of its name, not just those ending with it exactly, so all search-index-related test modules are properly exempted.tests/test_models_wired_at_read_sites.py (1)
99-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-pin the “search does not rely on
Bubble.from_dict” contract.This test now verifies only output shape/content. It no longer guards against regressions that reintroduce
Bubble.from_dictinto/api/search. Add a spy on the symbol used by the search path and assert it is not called, while keeping the current result assertion.🤖 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 `@tests/test_models_wired_at_read_sites.py` around lines 99 - 115, The test_search_endpoint_finds_bubble_text method only validates the search results content but does not actively guard against the regression of using Bubble.from_dict in the search path. Add a spy or mock on the Bubble.from_dict method before calling the /api/search endpoint via client.get(), then assert that this spy/mock was not called during the request. Retain all existing assertions about the response status code, results length, and matching text to maintain the current validation logic.
🤖 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 `@api/search.py`:
- Around line 28-34: The `_parse_since_days_param` function accepts unbounded
integer values that can cause overflow errors when passed to timedelta
operations, resulting in 500 responses. Add bounds checking to validate that the
parsed integer falls within an acceptable range (e.g., return None or a capped
value if the number exceeds a reasonable maximum like 36500 days or similar).
Apply the same bounds validation to any similar parameter parsing functions
referenced at lines 44-47.
In `@scripts/profile_search_breakdown.py`:
- Around line 74-77: The mode label at line 74 incorrectly marks the scan as
"scoped" when the condition checks window_ids, but the actual implementation of
_index_bubble_texts_matching_query() still performs a full bubble LIKE SQL pass
and only filters composer IDs in Python afterward. Change the mode label from
"scoped" to something more accurate that reflects the actual behavior of a full
database scan with Python-level filtering (not a truly scoped database query),
so the profiler output correctly represents the query strategy being used.
In `@scripts/profile_search_window.py`:
- Line 61: The variable name `scoped_bubble_scan` in the print statement is
misleading because it suggests an actual executed strategy when it only
represents a threshold heuristic check. Rename this variable to a more accurate
name that clearly indicates it is a threshold-based heuristic or condition check
(such as `in_small_window` or `below_bubble_threshold`) to prevent
misinterpretation in profiling output.
In `@services/search_index.py`:
- Around line 245-247: When src_conn is None in the open_global_db context
manager (indicating the source database cannot be opened), the function returns
False immediately without cleaning up the temporary search_index file that was
already created at new_path. Before returning False, route this through the
existing cleanup path to remove the temporary index file, ensuring orphan files
are not left behind on the filesystem during failed background polls.
- Around line 376-407: The query_composer_bubble_hits function accepts a
since_ms parameter at line 379 but does not use it in the SQL query at lines
394-397, causing all matching bubbles to be fetched before Python-side
filtering. Push the time window filter into the FTS query by conditionally
joining the composers table and adding a WHERE clause with (updated_ms >= ? OR
updated_ms <= 0) when since_ms is not None, following the same pattern used in
the query_composer_title_hits function, to reduce unnecessary I/O.
In `@services/search.py`:
- Around line 151-164: The _quick_bubble_text function only checks for top-level
keys "text", "rawText", and "content" when extracting searchable text from
bubble KV values, but misses the "richText" key which is a documented text
source in the Bubble model. Add "richText" to the tuple of keys being checked in
the for loop to ensure rich-text-only messages are properly indexed and
discoverable in searches.
- Around line 222-230: The function `_light_composer_exclusion_text` and related
fast path implementations currently apply exclusion rules only against limited
content (title/metadata for title matches, query-matching bubbles for content
matches), allowing excluded terms in non-matching bubbles to slip through
results. Modify the exclusion checking logic to load all visible bubble and tool
text for each candidate after pruning, then apply exclusion rules against the
full conversation content rather than just title or query-specific snippets.
Reserve the matchingText field exclusively for query-specific snippets while
ensuring exclusion validation happens against the complete conversation data.
- Around line 317-328: The workspace_for_composer method is passing an empty
dictionary for the bubble_map parameter instead of the resolver's actual bubble
map, which can cause workspace assignment to diverge from path-based resolution.
Replace the bubble_map={} argument with the resolver's actual bubble map from
the context (likely self.ctx.bubble_map or similar). Apply this same fix to any
other call sites mentioned in the review that also pass an empty bubble_map
dictionary.
- Around line 965-985: The search-window filtering at the point where created_ms
is calculated uses only createdAt, which can skip recently updated sessions.
Modify the created_ms assignment to use the maximum of createdAt and the
session's database modification timestamp (session_ts or mtime). Ensure this
updated created_ms value is used both in the _timestamp_in_search_window check
and when setting the timestamp in the results.append dictionary. Additionally,
when converting bubbles elsewhere in the code, use created_ms or session_ts as a
fallback to account for updated sessions that may have been created before the
search cutoff.
In `@services/workspace_db.py`:
- Around line 416-423: The current code uses as_completed() to iterate through
futures, which returns them in completion order based on thread timing rather
than submission order. This causes non-deterministic behavior where duplicate
composerIds in the mapping dictionary can be assigned to different workspace
names across runs. To fix this, modify the loop that processes futures to
iterate in the original workspace_entries order instead of as_completed() order.
You can do this by creating a list of futures that maintains correspondence with
workspace_entries (using zip or enumerate), then iterate through this list in
order while extracting results from each completed future, ensuring that earlier
workspace_entries always take precedence when the same composerId appears in
multiple entries.
In `@services/workspace_resolver.py`:
- Around line 144-149: The code calls data.get("allComposers") without first
verifying that data is a dictionary. If json.loads() successfully parses the
JSON but returns a non-dict value (like a list or string), calling .get() will
raise an AttributeError. Add a guard check to ensure data is a dictionary before
calling .get("allComposers"), similar to the existing isinstance(c, dict) check
used for the composer object c. The guard should allow the code to safely handle
malformed payloads without breaking workspace-name inference.
In `@templates/search.html`:
- Around line 42-43: The URL parameter parsing for all_history only recognizes
the value '1', but the backend API accepts both '1' and 'true'. Update the
condition in the line where allHistory is assigned to check if the all_history
parameter equals either '1' or 'true' (instead of only checking for '1'), so
that both shared link formats correctly restore the toggle state for the element
with id all-history.
---
Outside diff comments:
In `@tests/test_search_index.py`:
- Around line 135-147: The test method test_search_global_storage_uses_index
sets up patches to enable index usage but the current assertion only verifies
that results were found, which could still pass if the code falls back to live
scan instead of using the index. Add an explicit assertion after the results are
returned that verifies the live-scan fallback mechanism was not invoked, such as
checking that a specific mock or patch related to the fallback was not called,
to ensure the test actually proves that search_global_storage used the index as
intended.
---
Nitpick comments:
In `@services/workspace_resolver.py`:
- Around line 112-124: Replace the two broad except Exception handlers with
specific exception handling. In the first except block around the
warn_workspace_json_read call for the initial workspace.json read, catch the
specific exceptions that read_json_file and get_workspace_folder_paths would
raise (such as FileNotFoundError, JSONDecodeError, KeyError, and ValueError). In
the second except block within the loop, similarly catch only the expected
exceptions from read_json_file, get_workspace_folder_paths, normalize_file_path
operations rather than catching all Exception types, so that unexpected errors
will still propagate and surface during debugging instead of being silently
caught.
In `@tests/conftest.py`:
- Around line 30-31: The endswith check in the conftest.py file only matches
module names ending with exactly "test_search_index", so index-focused modules
like "test_search_index_windows" bypass the exemption and still get the
CURSOR_CHAT_BROWSER_NO_SEARCH_INDEX environment variable applied. Broaden the
condition that checks request.module.__name__ to use a pattern match (such as
the `in` operator or a substring search) that will catch any module name
containing "test_search_index" as part of its name, not just those ending with
it exactly, so all search-index-related test modules are properly exempted.
In `@tests/test_models_wired_at_read_sites.py`:
- Around line 99-115: The test_search_endpoint_finds_bubble_text method only
validates the search results content but does not actively guard against the
regression of using Bubble.from_dict in the search path. Add a spy or mock on
the Bubble.from_dict method before calling the /api/search endpoint via
client.get(), then assert that this spy/mock was not called during the request.
Retain all existing assertions about the response status code, results length,
and matching text to maintain the current validation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 154040f4-ed3c-4763-91f5-f16f5bf18dda
📒 Files selected for processing (25)
api/search.pyapp.pyscripts/profile_search.pyscripts/profile_search_breakdown.pyscripts/profile_search_window.pyservices/search.pyservices/search_index.pyservices/summary_cache.pyservices/workspace_composer_scan.pyservices/workspace_db.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.pytemplates/search.htmltemplates/workspace.htmltests/conftest.pytests/test_api_search.pytests/test_models_wired_at_read_sites.pytests/test_parse_failure_logging.pytests/test_project_path_boundary.pytests/test_search_exclusion_filtering.pytests/test_search_helpers.pytests/test_search_index.pytests/test_search_index_windows.pytests/test_workspace_list_count_alignment.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/search.py (1)
516-530: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the full-index query out of the candidate loop.
query_composer_rows_in_window(None)runs a fullcomposerstable scan (and opens a fresh read-only connection) every time it's called. Line 528 invokes it once per missing candidate inside thefor composer_id in candidate_idsloop, so a batch of cache misses degrades to repeated full scans. Resolve it once and reuse.♻️ Proposed fix
search_pool = composers_in_window if since_ms is not None else query_composer_rows_in_window(None) + all_rows: dict[str, sqlite3.Row] | None = None for composer_id, row in search_pool.items(): raw_lower = (row["raw_json"] or "").lower() if query_lower in raw_lower: candidate_ids.add(composer_id) for composer_id in candidate_ids: composer_row: sqlite3.Row | None = ( composers_in_window.get(composer_id) if since_ms is not None else search_pool.get(composer_id) ) if composer_row is None: - composer_row = query_composer_rows_in_window(None).get(composer_id) + if all_rows is None: + all_rows = query_composer_rows_in_window(None) + composer_row = all_rows.get(composer_id) if composer_row is None: continueNote: when
since_ms is not None, this fallback also pulls in composers outside the window; confirm that's intended versus skipping them.🤖 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 `@services/search.py` around lines 516 - 530, The query_composer_rows_in_window(None) call inside the for loop over candidate_ids is inefficient because it executes a full table scan for every missing candidate. Move the query_composer_rows_in_window(None) call outside and before the for loop over candidate_ids, store the result in a variable (e.g., full_window_data), and then reuse that variable inside the loop where the fallback lookup occurs on line 528. This eliminates repeated database scans and connection overhead.
🤖 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.
Nitpick comments:
In `@services/search.py`:
- Around line 516-530: The query_composer_rows_in_window(None) call inside the
for loop over candidate_ids is inefficient because it executes a full table scan
for every missing candidate. Move the query_composer_rows_in_window(None) call
outside and before the for loop over candidate_ids, store the result in a
variable (e.g., full_window_data), and then reuse that variable inside the loop
where the fallback lookup occurs on line 528. This eliminates repeated database
scans and connection overhead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1128d68-b30b-4071-b9f0-6daa1af54714
📒 Files selected for processing (11)
api/search.pyscripts/profile_search_breakdown.pyscripts/profile_search_window.pyservices/search.pyservices/search_index.pyservices/workspace_db.pyservices/workspace_resolver.pytemplates/search.htmltests/conftest.pytests/test_models_wired_at_read_sites.pytests/test_search_index.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/conftest.py
- tests/test_search_index.py
- scripts/profile_search_window.py
- tests/test_models_wired_at_read_sites.py
- scripts/profile_search_breakdown.py
- templates/search.html
- services/search_index.py
- services/workspace_resolver.py
Collaborator
Author
|
@CodeRabbit, please review this PR in more depth again based on issue description(latest updated version) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest issue `#95` description
gh issue view 95 --repo cppalliance/cppa-cursor-browser --json title,body,comments | jq '{title, body, lastComment: .comments[-1]}'Length of output: 4635 🏁 Script executed: #!/bin/bash
# Get PR `#113` details and files changed
gh pr view 113 --repo cppalliance/cppa-cursor-browser --json title,body,files | jq '{title, body: (.body | .[0:2000]), files: [.files[].path]}'Length of output: 3403 🏁 Script executed: #!/bin/bash
# Read key new/changed files
echo "=== services/workspace_composer_scan.py ==="
cat services/workspace_composer_scan.py
echo ""
echo "=== services/search_index.py (first 200 lines) ==="
head -200 services/search_index.py
echo ""
echo "=== services/search.py (first 150 lines) ==="
head -150 services/search.pyLength of output: 15529 🏁 Script executed: #!/bin/bash
echo "=== services/workspace_db.py ==="
cat services/workspace_db.py
echo ""
echo "=== services/workspace_listing.py ==="
cat services/workspace_listing.py
echo ""
echo "=== services/workspace_resolver.py ==="
cat services/workspace_resolver.pyLength of output: 49210 🏁 Script executed: #!/bin/bash
echo "=== services/workspace_tabs.py ==="
cat services/workspace_tabs.py
echo ""
echo "=== app.py ==="
cat app.py
echo ""
echo "=== api/search.py ==="
cat api/search.pyLength of output: 42236 🏁 Script executed: #!/bin/bash
echo "=== services/search_index.py (full) ==="
cat services/search_index.py
echo ""
echo "=== services/search.py (key sections) ==="
cat services/search.pyLength of output: 60367 🏁 Script executed: #!/bin/bash
echo "=== tests/test_workspace_list_count_alignment.py ==="
cat tests/test_workspace_list_count_alignment.py
echo ""
echo "=== tests/test_search_index.py ==="
cat tests/test_search_index.py
echo ""
echo "=== tests/conftest.py ==="
cat tests/conftest.pyLength of output: 17524 🏁 Script executed: #!/bin/bash
echo "=== services/summary_cache.py (key fingerprint section) ==="
cat services/summary_cache.py
echo ""
echo "=== templates/workspace.html ==="
cat templates/workspace.html
echo ""
echo "=== templates/search.html (tail) ==="
tail -100 templates/search.htmlLength of output: 29654
Acceptance Criteria Status
Issues Found1.
|
bradjin8
requested changes
Jun 23, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
services/search_index.py (1)
270-315: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
indexed_composer_idsis populated before the composer is actually inserted, so orphan bubbles can still be indexed.Line 272 adds
composer_idto the set before thejson.loads/isinstanceguards at 275–279. Composers whose payload fails to parse or isn't a dict hitcontinue(277/279) and are never inserted into thecomposerstable, yet their id remains inindexed_composer_ids. As a result the bubble filter at 306–307 still admits bubbles for those composers, producing FTS rows with no backingcomposersrow — the exact orphan scenario the earlier fix targeted. These rows add rebuild cost and can surface bubble hits that are dropped downstream when nosince_msjoin/composer_ids_filteris applied.Move the
addto after the successful insert so only persisted composers gate bubble indexing.Proposed fix
for row in composer_rows: composer_id = row["key"].split(":")[1] - indexed_composer_ids.add(composer_id) raw_text = _composer_row_raw_text(row) try: cd = json.loads(raw_text) except (json.JSONDecodeError, TypeError, ValueError): continue if not isinstance(cd, dict): continue title = cd.get("name") or "" if not isinstance(title, str): title = str(title) if title else "" conn.execute( "INSERT OR REPLACE INTO composers" " (composer_id, title, created_ms, updated_ms, raw_json)" " VALUES (?, ?, ?, ?, ?)", ( composer_id, title, to_epoch_ms(cd.get("createdAt")) or 0, _composer_dict_timestamp_ms(cd), raw_text, ), ) composer_count += 1 + indexed_composer_ids.add(composer_id)🤖 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 `@services/search_index.py` around lines 270 - 315, The indexed_composer_ids set is being populated with composer_id immediately after extracting it from the row, before the json.loads validation and isinstance checks that occur at lines 275-279. If these validation checks fail and hit continue statements, the composer is never inserted into the composers table but its id remains in indexed_composer_ids. This allows orphan bubbles to still be indexed later when the filter at lines 306-307 checks if composer_id is in indexed_composer_ids. Move the indexed_composer_ids.add(composer_id) call from its current position to after the successful conn.execute INSERT statement that adds to the composers table, ensuring only composers that are actually persisted have their ids tracked for bubble indexing.
🧹 Nitpick comments (1)
tests/test_workspace_list_count_alignment.py (1)
138-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEquality assertion passes vacuously if both sets are empty.
summary_ids == full_idswould still hold if both paths returned zero tabs (e.g., a regression that drops the seeded composer from both). Add a non-empty guard so the test actually proves parity over real data.💚 Strengthen the assertion
summary_ids = {t["id"] for t in summary["tabs"]} full_ids = {t["id"] for t in full["tabs"]} + assert summary_ids # guard against vacuous parity when both are empty assert summary_ids == full_ids🤖 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 `@tests/test_workspace_list_count_alignment.py` around lines 138 - 140, The equality assertion between summary_ids and full_ids passes vacuously when both sets are empty, which does not actually prove parity. Add an assertion to verify that both summary_ids and full_ids are non-empty (contain at least one tab) before or in conjunction with the equality assertion, ensuring the test validates parity over real data rather than just confirming both endpoints return no tabs.
🤖 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.
Duplicate comments:
In `@services/search_index.py`:
- Around line 270-315: The indexed_composer_ids set is being populated with
composer_id immediately after extracting it from the row, before the json.loads
validation and isinstance checks that occur at lines 275-279. If these
validation checks fail and hit continue statements, the composer is never
inserted into the composers table but its id remains in indexed_composer_ids.
This allows orphan bubbles to still be indexed later when the filter at lines
306-307 checks if composer_id is in indexed_composer_ids. Move the
indexed_composer_ids.add(composer_id) call from its current position to after
the successful conn.execute INSERT statement that adds to the composers table,
ensuring only composers that are actually persisted have their ids tracked for
bubble indexing.
---
Nitpick comments:
In `@tests/test_workspace_list_count_alignment.py`:
- Around line 138-140: The equality assertion between summary_ids and full_ids
passes vacuously when both sets are empty, which does not actually prove parity.
Add an assertion to verify that both summary_ids and full_ids are non-empty
(contain at least one tab) before or in conjunction with the equality assertion,
ensuring the test validates parity over real data rather than just confirming
both endpoints return no tabs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 527e4b70-3695-406f-8620-449b4adfe9bb
📒 Files selected for processing (6)
services/search.pyservices/search_index.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/test_workspace_list_count_alignment.py
🚧 Files skipped from review as they are similar to previous changes (3)
- services/workspace_resolver.py
- services/workspace_listing.py
- services/search.py
Collaborator
Author
|
@CodeRabbit, All @bradjin8 's comments are addressed or answered correctly? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR `#113` review comments by bradjin8 ==="
gh api repos/cppalliance/cppa-cursor-browser/pulls/113/comments \
--paginate \
| jq '[.[] | select(.user.login == "bradjin8") | {id, path, line, body, created_at}]'
echo ""
echo "=== PR `#113` issue-level comments by bradjin8 ==="
gh api repos/cppalliance/cppa-cursor-browser/issues/113/comments \
--paginate \
| jq '[.[] | select(.user.login == "bradjin8") | {id, body, created_at}]'
echo ""
echo "=== PR `#113` reviews by bradjin8 ==="
gh api repos/cppalliance/cppa-cursor-browser/pulls/113/reviews \
--paginate \
| jq '[.[] | select(.user.login == "bradjin8") | {id, state, body, submitted_at}]'Length of output: 7942 🏁 Script executed: #!/bin/bash
echo "=== PR `#113` commits after bradjin8's review (2026-06-23) ==="
gh api repos/cppalliance/cppa-cursor-browser/pulls/113/commits \
--paginate \
| jq '[.[] | {sha: .sha[:8], message: .commit.message | split("\n")[0], date: .commit.committer.date}]'
echo ""
echo "=== All review comments on PR `#113` (including replies to bradjin8) ==="
gh api repos/cppalliance/cppa-cursor-browser/pulls/113/comments \
--paginate \
| jq '[.[] | {id, in_reply_to_id, user: .user.login, path, line, body: .body[:300], created_at}]'Length of output: 21122
✅ Fully Addressed (10/13)
|
| # | Comment | Status |
|---|---|---|
| 1c | Full composer scan for alias inference on every assemble_single_tab call |
Deferred — "too large a change, separate PR" |
| 12 | _INCLUDE_UNKNOWN_TIMESTAMPS_IN_WINDOW behavior should be visible to users (UI tooltip/API docs) |
Partially addressed — documented in code comment; UI tooltip explicitly deferred to a separate PR |
| 13 | api/search.py outer except Exception returns generic 500; should distinguish 400 from 500 |
Deferred — "left as-is, separate PR" |
🔄 Review Status
@bradjin8's formal review is still marked CHANGES_REQUESTED (they have not re-reviewed after the d4d7d761 push). The three deferred items above are the open gaps. The two deferred-to-separate-PR items (1c, 13) are genuine gaps — would it help to open follow-up issues for them so they're tracked and don't get lost?
Collaborator
Author
These will be implemented by follow up PRs by next week |
clean6378-max-it
force-pushed
the
fix/workspace-list-load-and-count
branch
from
ddad5a4 to
0dde9d9
Compare
This was referenced Jun 23, 2026
clean6378-max-it
force-pushed
the
fix/workspace-list-load-and-count
branch
from
0dde9d9 to
9927ba9
Compare
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
2 participants
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.
CLoses #95
Summary
Improves workspace list/navigation performance and makes search fast enough for daily use, while fixing incorrect conversation links from search results.
Workspace listing & navigation (#95)
workspace_composer_scan.pybuild_composer_id_to_workspace_idand bulk-load project layoutsconversationCountwith the same filters used by tab summariescomposerDatarows without spurious warningsget_project_from_file_path(fixesNameErrorthat dropped ~66 conversations)Search performance
all_history=1API flagLIKEscan (fixes scoped-query regression that made 30-day search slower than full history)search_index.sqlitewith FTS5, background rebuild on startup + 60s poll, fingerprint invalidation (same mtime scheme as listing cache)search_index.activepointer (avoidsPermissionErrorwhen replacing an open DB)Search correctness
workspaceIdnow usesassign_composer_workspace(same as sidebar), not roster map alone?tab=deep links; fetches conversation directly instead of opening the first sidebar item; redirects toglobalon 404Test plan
python app.py; confirm log:Search index rebuilt: N composers, M bubbles -> search_index.*.sqlitepytest tests/test_workspace_list_count_alignment.py tests/test_api_search.py tests/test_search_index.py tests/test_search_helpers.py tests/test_project_path_boundary.pyNotes
~/.cache/cursor-chat-browser/CURSOR_CHAT_BROWSER_NOCACHE=1orCURSOR_CHAT_BROWSER_NO_SEARCH_INDEX=1scripts/profile_search*.pySummary by CodeRabbit
Release Notes
New Features
allHistory/searchWindowDaysfor accurate result labeling.Performance/Background
Bug Fixes
Tests