refactor(services): centralize workspace determination in resolve_workspace_context (#91)#96
Open
bradjin8 wants to merge 4 commits into
Open
refactor(services): centralize workspace determination in resolve_workspace_context (#91)#96bradjin8 wants to merge 4 commits into
bradjin8 wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes workspace discovery into a frozen WorkspaceContext with resolve/enrich entrypoints; consumers (scripts/export, api/export_api, services/workspace_listing, services/workspace_tabs) now call the resolvers; tests validate resolution, caching, short-circuiting, and SQLite-based enrichment. ChangesWorkspace context orchestration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant resolve_workspace_context
participant collect_workspace_entries
participant build_composer_id_map_cached
participant build_composer_id_map_uncached
participant build_project_workspace_maps
participant global_db
Caller->>resolve_workspace_context: workspace_path, rules?, workspace_entries?
resolve_workspace_context->>collect_workspace_entries: collect entries (if not provided)
collect_workspace_entries-->>resolve_workspace_context: workspace_entries
resolve_workspace_context->>build_composer_id_map_cached: build cached composer map (if rules)
resolve_workspace_context->>build_composer_id_map_uncached: build uncached composer map (otherwise)
build_composer_id_map_cached-->>resolve_workspace_context: composer_id_to_workspace_id
build_composer_id_map_uncached-->>resolve_workspace_context: composer_id_to_workspace_id
resolve_workspace_context->>build_project_workspace_maps: derive project_name & workspace_path maps
build_project_workspace_maps-->>resolve_workspace_context: project_name_to_workspace_id, workspace_path_to_id
alt populate_project_layouts or populate_bubble_map
resolve_workspace_context->>global_db: load project_layouts_map / bubble_map
global_db-->>resolve_workspace_context: project_layouts_map / bubble_map
end
resolve_workspace_context-->>Caller: WorkspaceContext
Caller->>enrich_workspace_context_from_global_db: ctx, global_db, flags
enrich_workspace_context_from_global_db->>global_db: query cursorDiskKV for requested keys
global_db-->>enrich_workspace_context_from_global_db: serialized KV blobs
enrich_workspace_context_from_global_db-->>Caller: enriched WorkspaceContext
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
services/workspace_context.py (1)
98-104: 💤 Low valueSilent no-op when
populate_*flags are True butglobal_dbis None.If a caller passes
populate_project_layouts=Trueorpopulate_bubble_map=Truewithout providingglobal_db, the maps remain empty with no warning. This could mask misconfiguration where the caller expects data but receives empty maps.Consider either:
- Raising
ValueErrorwhen a populate flag is True butglobal_dbis None, or- Logging a warning to aid debugging.
Option 1: Raise on misconfiguration
project_layouts: dict[str, list] = {} bubble_map: dict[str, dict] = {} + if (populate_project_layouts or populate_bubble_map) and global_db is None: + raise ValueError( + "global_db is required when populate_project_layouts or populate_bubble_map is True" + ) if global_db is not None: if populate_project_layouts: project_layouts = load_project_layouts_map(global_db)🤖 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_context.py` around lines 98 - 104, If populate_project_layouts or populate_bubble_map is True while global_db is None, raise a ValueError (instead of silently returning empty maps) so callers are alerted to the misconfiguration; update the logic around project_layouts, bubble_map, populate_project_layouts, populate_bubble_map and global_db to check for this case and raise a clear error message referencing which flag was set, and keep existing calls to load_project_layouts_map and load_bubble_map unchanged for the normal (global_db is not None) path.
🤖 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/workspace_context.py`:
- Around line 98-104: If populate_project_layouts or populate_bubble_map is True
while global_db is None, raise a ValueError (instead of silently returning empty
maps) so callers are alerted to the misconfiguration; update the logic around
project_layouts, bubble_map, populate_project_layouts, populate_bubble_map and
global_db to check for this case and raise a clear error message referencing
which flag was set, and keep existing calls to load_project_layouts_map and
load_bubble_map unchanged for the normal (global_db is not None) path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 248e1efb-633a-46c3-8df2-e04bd134a69e
📒 Files selected for processing (6)
api/export_api.pyscripts/export.pyservices/workspace_context.pyservices/workspace_listing.pyservices/workspace_tabs.pytests/test_workspace_context.py
timon0305
reviewed
Jun 4, 2026
Collaborator
timon0305
left a comment
timon0305
left a comment
There was a problem hiding this comment.
Nice consolidation — four call sites going through one orchestrator reads cleanly. A few thoughts on the surface area inline.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/workspace_context.py (1)
98-108: 💤 Low valueConsider accepting
workspace_entriesfor consistency.Unlike
resolve_workspace_context()andresolve_workspace_context_cached(), the minimal resolver doesn't accept aworkspace_entriesparameter. If a caller already has collected entries, they cannot reuse them with this function, forcing redundant collection or requiring them to use a different resolver and ignore the extra fields.♻️ Optional refactor for API consistency
-def resolve_workspace_context_minimal(workspace_path: str) -> WorkspaceContext: +def resolve_workspace_context_minimal( + workspace_path: str, + *, + workspace_entries: list[dict] | None = None, +) -> WorkspaceContext: """Entries, project-name, and composer maps only (HTTP export).""" - entries = collect_workspace_entries(workspace_path) + entries = _entries(workspace_path, workspace_entries) return _assemble_context(🤖 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_context.py` around lines 98 - 108, resolve_workspace_context_minimal currently always calls collect_workspace_entries forcing redundant work; change its signature to accept an optional workspace_entries parameter (e.g., workspace_entries: Optional[Iterable[WorkspaceEntry]] = None), update the body to use the provided workspace_entries when non-None and only call collect_workspace_entries(workspace_path) otherwise, and keep the rest of the function calling _assemble_context with the entries; mirror the parameter name/semantics used by resolve_workspace_context and resolve_workspace_context_cached and update the docstring to mention the optional pre-collected entries.tests/test_workspace_context.py (1)
1-171: ⚡ Quick winConsider adding tests for remaining enrichment and caching scenarios.
Current coverage is solid, but a few additional scenarios would strengthen confidence:
nocache=True: Test thatresolve_workspace_context_cached(..., nocache=True)properly bypasses the cache.- Both enrichment flags: Test
enrich_workspace_context_from_global_dbwith bothpopulate_project_layouts=Trueandpopulate_bubble_map=Trueto verify both maps are populated simultaneously.- No-op enrichment: Test
enrich_workspace_context_from_global_dbwith both flagsFalse(or omitted) to verify the original context is returned unchanged (line 124-125 path).🤖 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_context.py` around lines 1 - 171, Add three unit tests: one exercising resolve_workspace_context_cached with nocache=True to assert build_composer_id_to_workspace_id is called (not cached path) and cached builder is not used; one calling enrich_workspace_context_from_global_db with both populate_project_layouts=True and populate_bubble_map=True and asserting both enriched.project_layouts_map and enriched.bubble_map are populated; and one calling enrich_workspace_context_from_global_db with both flags False (or omitted) asserting the returned context equals the original (no changes). Use the existing helpers (_make_workspace_root, _open_global_db) and the functions resolve_workspace_context_cached and enrich_workspace_context_from_global_db from the diff to locate where to add these tests.
🤖 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/workspace_context.py`:
- Around line 98-108: resolve_workspace_context_minimal currently always calls
collect_workspace_entries forcing redundant work; change its signature to accept
an optional workspace_entries parameter (e.g., workspace_entries:
Optional[Iterable[WorkspaceEntry]] = None), update the body to use the provided
workspace_entries when non-None and only call
collect_workspace_entries(workspace_path) otherwise, and keep the rest of the
function calling _assemble_context with the entries; mirror the parameter
name/semantics used by resolve_workspace_context and
resolve_workspace_context_cached and update the docstring to mention the
optional pre-collected entries.
In `@tests/test_workspace_context.py`:
- Around line 1-171: Add three unit tests: one exercising
resolve_workspace_context_cached with nocache=True to assert
build_composer_id_to_workspace_id is called (not cached path) and cached builder
is not used; one calling enrich_workspace_context_from_global_db with both
populate_project_layouts=True and populate_bubble_map=True and asserting both
enriched.project_layouts_map and enriched.bubble_map are populated; and one
calling enrich_workspace_context_from_global_db with both flags False (or
omitted) asserting the returned context equals the original (no changes). Use
the existing helpers (_make_workspace_root, _open_global_db) and the functions
resolve_workspace_context_cached and enrich_workspace_context_from_global_db
from the diff to locate where to add these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93388dd1-75fd-49da-9676-e95f2501622f
📒 Files selected for processing (5)
api/export_api.pyservices/workspace_context.pyservices/workspace_listing.pyservices/workspace_tabs.pytests/test_workspace_context.py
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workspace_listing.py
timon0305
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
services/workspace_context.pywith a typedWorkspaceContextdataclass andresolve_workspace_context()orchestrator that runs the shared workspace-determination ceremony (entries, invalid IDs, project-name/path maps, composer→workspace map).enrich_workspace_context_from_global_db()for optional global KV maps (project_layouts_map,bubble_map) afteropen_global_db.services/workspace_listing.pyservices/workspace_tabs.py(summaries, single-tab, full tabs)scripts/export.pyapi/export_api.py(minimal context: skips invalid/path maps; enriches bubbles only)tests/test_workspace_context.pyfor orchestrator behavior and flag handling.Closes #91.
Test plan
python -m pytest tests/test_workspace_context.py -qpython -m pytest -q(full suite: 393 passed, 4 skipped)GET /api/workspacesandGET /api/workspaces/<id>/tabsunchangedPOST /api/exportandpython scripts/export.pystill assign chats to the expected workspace foldersSummary by CodeRabbit
Refactor
Refactor
Refactor
Tests