refactor: decouple API handlers from _-prefixed service internals (closes #73)#83
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:
📝 WalkthroughWalkthroughRenames underscore-prefixed helpers into public functions and updates all consumers: services, API handlers, export script, utils, and tests; adds docstrings and a documented open_global_db context-manager contract; preserves existing behavior and adjusts export-state error handling. ChangesDe-privatize workspace helpers and update consumers
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/export_api.py`:
- Line 117: The context manager call in open_global_db returns a tuple
(global_db, global_db_path) but global_db_path is never used; update the binding
to ignore that value to satisfy RUF059 by changing the with-statement to capture
only global_db and an unused name (e.g., use "as (global_db, _)" or "as
(global_db, _global_db_path)") so the unused second element is clearly
intentionally ignored while leaving the rest of the code unchanged.
In `@services/cli_tabs.py`:
- Around line 22-23: The docstring currently claims the function returns a
``(jsonify({...}), status)`` tuple but the success path returns a plain Flask
Response; update the docstring in services/cli_tabs.py to state the correct
contract as "Response | tuple[Response, int]" (or "flask.Response |
tuple[flask.Response, int]") so it matches the actual return types, referencing
the success return (a Flask Response) and the error tuple form that uses jsonify
and status codes.
🪄 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: f0912019-02b3-4ccd-a733-e50fadf8bad8
📒 Files selected for processing (7)
api/export_api.pyapi/workspaces.pyservices/cli_tabs.pyservices/workspace_db.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/cli_tabs.py (1)
15-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the backward-compatible private alias.
_get_cli_workspace_tabsappears removed. That can break existing imports and conflicts with the stated compatibility goal for this refactor.Suggested minimal patch
def get_cli_workspace_tabs(workspace_id: str): @@ except Exception as e: _logger.error( "Failed to get CLI workspace tabs for %s: %s (%s)", workspace_id, e, type(e).__name__, exc_info=True, ) return jsonify({"error": "Failed to get CLI workspace tabs"}), 500 + +# Backward-compatible alias for existing callers. +_get_cli_workspace_tabs = get_cli_workspace_tabs🤖 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/cli_tabs.py` around lines 15 - 149, The refactor removed the backward-compatible private alias _get_cli_workspace_tabs which may break callers; restore a module-level alias by assigning _get_cli_workspace_tabs = get_cli_workspace_tabs (or reintroduce a thin wrapper function named _get_cli_workspace_tabs that forwards to get_cli_workspace_tabs) so existing imports continue to work while preserving the new public function get_cli_workspace_tabs and its behavior.
🤖 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 `@tests/test_workspace_db_special_paths.py`:
- Line 66: The test unpacks a second value from the context manager returned by
open_global_db(ws_root) into the variable name path but never uses it, causing a
Ruff RUF059 lint error; update the unpacked name in the with statement (the
tuple unpack from open_global_db in tests/test_workspace_db_special_paths.py) to
a discard name like _path or _ to mark it intentionally unused while leaving
conn unchanged.
---
Outside diff comments:
In `@services/cli_tabs.py`:
- Around line 15-149: The refactor removed the backward-compatible private alias
_get_cli_workspace_tabs which may break callers; restore a module-level alias by
assigning _get_cli_workspace_tabs = get_cli_workspace_tabs (or reintroduce a
thin wrapper function named _get_cli_workspace_tabs that forwards to
get_cli_workspace_tabs) so existing imports continue to work while preserving
the new public function get_cli_workspace_tabs and its 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
Run ID: 6960ee1e-8e18-411d-974c-09e4a11ce50c
📒 Files selected for processing (21)
api/export_api.pyapi/workspaces.pyscripts/export.pyservices/cli_tabs.pyservices/workspace_db.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/test_api_endpoints.pytests/test_cli_chat_reader.pytests/test_cli_tabs.pytests/test_exclusion_rules.pytests/test_invalid_workspace_aliases.pytests/test_models_wired_at_read_sites.pytests/test_project_path_boundary.pytests/test_workspace_assignment_fallback.pytests/test_workspace_db_special_paths.pytests/test_workspace_name_db_errors.pytests/test_workspace_name_inference.pytests/test_workspace_tabs_malformed_nested.pyutils/cli_chat_reader.pyutils/exclusion_rules.py
💤 Files with no reviewable changes (3)
- services/workspace_db.py
- services/workspace_resolver.py
- api/workspaces.py
✅ Files skipped from review due to trivial changes (3)
- tests/test_api_endpoints.py
- tests/test_cli_tabs.py
- utils/cli_chat_reader.py
…ssues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workspace_db_special_paths.py` at line 66, The test unpacks a second value from the context manager returned by open_global_db(ws_root) into the variable name path but never uses it, causing a Ruff RUF059 lint error; update the unpacked name in the with statement (the tuple unpack from open_global_db in tests/test_workspace_db_special_paths.py) to a discard name like _path or _ to mark it intentionally unused while leaving conn unchanged.
|
Actionable comments posted: 0 |
Must fixservices/workspace_tabs.py:362 — replace ctx_window.get('percentageRemainingFloat') or ctx_window.get('percentageRemaining') with an explicit is not None check for each field scripts/export.py:42 — remove the unused from utils.tool_parser import parse_tool_call import Should fixapi/export_api.py:50 — narrow except Exception: pass in _get_export_state to except (json.JSONDecodeError, ValueError, OSError) and add a _logger.warning(...) call api/workspaces.py:146 — wrap the get_cli_workspace_tabs(...) call in a try/except block matching the non-cli branch, returning a structured JSON 500 on failure scripts/export.py:200 — replace bare except Exception: pass on the --since=last timestamp parse with _logger.warning('Could not read last export timestamp; defaulting to full export: %s', e) services/workspace_tabs.py:80 — expand assemble_workspace_tabs docstring to include Args (workspace_id, workspace_path, rules) and Returns (payload field list, meaning of status 404 vs 200) services/workspace_listing.py:41 — expand list_workspace_projects docstring to include Args and a Returns section describing the projects list fields and the warnings structure services/workspace_resolver.py:217 — add Args section to determine_project_for_conversation covering all 8 parameters, particularly composer_id_to_workspace_id=None semantics services/workspace_resolver.py:353 — expand infer_invalid_workspace_aliases docstring to describe the majority-vote algorithm, tie-breaking rule, and when the function returns an empty dict Nice to havetests/test_workspace_tabs_malformed_nested.py:204 — update the comment from "Force _parse_tool_call to return None" to "Force parse_tool_call to return None" api/workspaces.py:51,150 — extract current_app.config.get('EXCLUSION_RULES') or [] into a shared accessor |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/export.py (1)
199-203: 💤 Low valueConsider narrowing the exception clause (optional).
The logging improvement is good. Static analysis flags the broad
except Exception. The expected failure modes here arejson.JSONDecodeError,ValueError(fromfromisoformat/int), andOSError(permission issues). Narrowing would make the catch more intentional, though the current behavior is already correct.♻️ Optional: narrow exception types
- except Exception as e: + except (json.JSONDecodeError, ValueError, OSError) as e: _logger.warning( "Could not read last export timestamp; defaulting to full export: %s", 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 `@scripts/export.py` around lines 199 - 203, Narrow the broad except clause that catches Exception in the block handling the last export timestamp read (the except Exception as e that logs via _logger.warning) to only catch the expected failure types: json.JSONDecodeError, ValueError, and OSError; update the except line to list these specific exceptions and keep the existing logging call unchanged so you still report the error variable e.
🤖 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 `@scripts/export.py`:
- Around line 199-203: Narrow the broad except clause that catches Exception in
the block handling the last export timestamp read (the except Exception as e
that logs via _logger.warning) to only catch the expected failure types:
json.JSONDecodeError, ValueError, and OSError; update the except line to list
these specific exceptions and keep the existing logging call unchanged so you
still report the error variable e.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4413702e-bbc6-4ffa-9777-3b128bc2e123
📒 Files selected for processing (7)
api/export_api.pyapi/workspaces.pyscripts/export.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/test_workspace_tabs_malformed_nested.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_workspace_tabs_malformed_nested.py
- services/workspace_listing.py
- services/workspace_tabs.py
- api/export_api.py
- services/workspace_resolver.py
|
Actionable comments posted: 0 |
|
@CodeRabbit can you review if my previous 11 comments are resolved or not? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== 1. services/workspace_tabs.py around line 362 (is not None check) ==="
grep -n "percentageRemainingFloat\|percentageRemaining\|is not None" services/workspace_tabs.py | head -20
echo ""
echo "=== 2. scripts/export.py around line 42 (unused import parse_tool_call) ==="
grep -n "parse_tool_call\|tool_parser" scripts/export.py | head -10
echo ""
echo "=== 3. api/export_api.py around line 50 (_get_export_state except clause) ==="
sed -n '40,65p' api/export_api.py
echo ""
echo "=== 4. api/workspaces.py around line 146 (get_cli_workspace_tabs try/except) ==="
sed -n '135,165p' api/workspaces.py
echo ""
echo "=== 5. scripts/export.py around line 200 (--since=last bare except) ==="
sed -n '190,215p' scripts/export.py
echo ""
echo "=== 6. services/workspace_tabs.py around line 80 (assemble_workspace_tabs docstring) ==="
sed -n '70,115p' services/workspace_tabs.py
echo ""
echo "=== 7. services/workspace_listing.py around line 41 (list_workspace_projects docstring) ==="
sed -n '30,60p' services/workspace_listing.py
echo ""
echo "=== 8. services/workspace_resolver.py around line 217 (determine_project_for_conversation Args) ==="
sed -n '205,250p' services/workspace_resolver.py
echo ""
echo "=== 9. services/workspace_resolver.py around line 353 (infer_invalid_workspace_aliases docstring) ==="
sed -n '340,390p' services/workspace_resolver.py
echo ""
echo "=== 10. tests/test_workspace_tabs_malformed_nested.py around line 204 (comment update) ==="
sed -n '195,215p' tests/test_workspace_tabs_malformed_nested.py
echo ""
echo "=== 11. api/workspaces.py lines 51, 150 (EXCLUSION_RULES accessor) ==="
grep -n "EXCLUSION_RULES\|_exclusion_rules" api/workspaces.py | head -20Length of output: 15433
All must-fix, should-fix, and nice-to-have items are resolved. The PR looks good to merge from a comment-resolution perspective. |
clean6378-max-it
left a comment
clean6378-max-it
left a comment
There was a problem hiding this comment.
Should fix
scripts/export.py:271 — log at DEBUG when json.loads fails on a composer row before continue
(silent skip on corrupt composerData rows matches the eval’s “silent leak cascade” pattern; IDE export in api/export_api.py already logs equivalent failures at ERROR)
api/export_api.py:118 — use the same _exclusion_rules() helper as api/workspaces.py (or move it to a shared module)
(EXCLUSION_RULES access is duplicated across two API modules; a config key rename would need two edits)
Nice to have
services/cli_tabs.py:10-12 — move utils.* imports above _logger = logging.getLogger(name)
(PEP 8 import order; no runtime impact)
services/workspace_resolver.py:114-125 — add DEBUG logs in the infer_workspace_name_from_context JSON-parse continue paths
(heuristic loops intentionally skip bad rows; logging would help operators without changing behavior)
api/export_api.py:145 — replace to_epoch_ms(...) or to_epoch_ms(...) or 0 with explicit is not None checks
(epoch 0 is falsy and can mis-order timestamps; very unlikely in practice)
services/cli_tabs.py:40 — pass rules as a parameter like assemble_workspace_tabs instead of reading current_app.config inside the service
(keeps the service layer independent of Flask context; tests already use test_request_context)
|
Addressed all items: Should fix
Nice to have
Validation: |
3 participants
Summary
services/workspace_db.py,services/workspace_resolver.py, andservices/cli_tabs.pyto a documented public API (open_global_db,collect_workspace_entries,lookup_workspace_display_name,get_cli_workspace_tabs, etc.).api/export_api.pyandapi/workspaces.pyso they import only public symbols fromservices/— no_-prefixed imports inapi/._…aliases on service modules and thin re-exports onapi/workspaces.pyso existing tests andscripts/export.pycontinue to work without changes.Closes #73.
Eval: addresses Test 20 (Implementation Hiding) — “Invisible Surface” coupling between HTTP handlers and private service internals (eval baseline).
Details
services/workspace_dbcollect_workspace_entries,collect_invalid_workspace_ids,build_composer_id_to_workspace_id,open_global_dbservices/workspace_resolverlookup_workspace_display_name(distinct fromutils.path_helpers.get_workspace_display_name),infer_workspace_name_from_context,determine_project_for_conversation, mapping helpersservices/cli_tabsget_cli_workspace_tabsapi/export_api,api/workspacesservices/workspace_listing,services/workspace_tabsNot in scope:
scripts/export.pystill uses_…aliases (intentional; follow-up to migrate CLI to public names).Test plan
python -m unittest discover tests -q— 307 passed, 4 skippedfrom services… import _inapi/(rg 'from services.*import.*_' api/)GET /api/workspaces,GET /api/workspaces/<id>/tabs(IDE +cli:workspace),POST /api/exportSummary by CodeRabbit