cleanup workspace_tabs and cursor_md_exporter hygiene debt#138
Conversation
📝 WalkthroughWalkthroughWorkspace tab assembly now uses shared context, metadata, and composer-iteration helpers. Cursor CLI and IDE Markdown exports are split into reusable rendering phases, and bubble metadata is more precisely typed. ChangesWorkspace tabs and export refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/workspace_tabs.py (1)
245-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
has_anyomitsctx_token_limit, unlikemax_ctx_tokens.Line 293-298 folds
max_ctx_tokensintohas_anybut notctx_token_limit. If a bubble carriescontextTokenLimitwithout a positivecontextTokensUsed(and no other usage signal is present),tab_metais set toNoneat Line 300 even thoughtab_meta_raw["contextTokenLimit"](Line 316) would otherwise be non-empty — the tab metadata is silently dropped.🐛 Proposed fix
has_any = any([ total_input, total_output, total_cached, total_response_ms, total_cost, models_set, total_tool_calls, total_thinking_ms, lines_added, lines_removed, files_added, files_removed, - max_ctx_tokens, + max_ctx_tokens, ctx_token_limit, ])🤖 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_tabs.py` around lines 245 - 330, Update the has_any condition in _aggregate_tab_metadata to include ctx_token_limit alongside max_ctx_tokens, ensuring metadata containing only a positive contextTokenLimit still produces tab_meta and preserves the existing tab_meta_raw handling.
🧹 Nitpick comments (1)
services/workspace_tabs.py (1)
405-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
composer_model_namesinstead of re-deriving early model names inline.Lines 441-442 reimplement the same
model_name_from_config() != "default"logic thatcomposer_model_names(fromservices/workspace_composer_scan.py, already imported and used at Line 581 of this file) provides. Consolidating avoids two implementations of the same rule drifting apart.♻️ Proposed refactor
- _early_model_name = composer.model_name_from_config() - _early_model_names = [_early_model_name] if _early_model_name and _early_model_name != "default" else None + _early_model_names = composer_model_names(composer)🤖 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_tabs.py` around lines 405 - 457, Update _assemble_tab_from_composer_data to use the existing composer_model_names helper for the rule-matching model names instead of calling composer.model_name_from_config() and rebuilding the default filtering inline. Preserve the current behavior by passing the helper’s result to build_searchable_text.
🤖 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 `@utils/cursor_md_exporter.py`:
- Around line 140-144: Validate that the decoded value assigned to session_meta
is a mapping before calling .get() in the metadata loading flow. Update the
logic around _CLI_META_READ_ERRORS and session_meta so valid JSON lists or
scalars fall back to an empty metadata mapping, preserving the existing default
agentId behavior.
- Around line 442-443: Update the early-return condition in the
summary-generation logic to also consider tool_result_stats, so non-empty Tool
Results summaries are preserved for search/web-only sessions. Return an empty
string only when files_read_list, files_written_list, commands_run_list, and
tool_result_stats are all empty or absent.
---
Outside diff comments:
In `@services/workspace_tabs.py`:
- Around line 245-330: Update the has_any condition in _aggregate_tab_metadata
to include ctx_token_limit alongside max_ctx_tokens, ensuring metadata
containing only a positive contextTokenLimit still produces tab_meta and
preserves the existing tab_meta_raw handling.
---
Nitpick comments:
In `@services/workspace_tabs.py`:
- Around line 405-457: Update _assemble_tab_from_composer_data to use the
existing composer_model_names helper for the rule-matching model names instead
of calling composer.model_name_from_config() and rebuilding the default
filtering inline. Preserve the current behavior by passing the helper’s result
to build_searchable_text.
🪄 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: 0ad477bb-44e1-4ed3-9c1b-1636b4c5302e
📒 Files selected for processing (3)
services/workspace_tabs.pyutils/cursor_md_exporter.pyutils/display_bubble.py
bradjin8
left a comment
bradjin8
left a comment
There was a problem hiding this comment.
No dedicated regression tests for the three 5197228 edge cases (contextTokenLimit-only metadata, non-dict CLI meta JSON, web-only tool_result_stats summary). Existing test_workspace_tabs_* / test_cursor_md_exporter.py pass but don’t pin these paths.
yep, added three: test_workspace_tabs_aggregate_metadata.py for limit-only tab metadata, plus two in test_cursor_md_exporter.py (non-dict cli meta, web-only tool stats). |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/workspace_tabs.py (1)
468-494: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestrict the assembly failure handler to expected drift errors.
Line 490 catches every application bug as a recoverable malformed-composer case, silently returning a partial
200response. Catch the bounded storage/schema error set (for example_COMPOSER_ROW_ERRORS) and let unexpected defects surface.🤖 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_tabs.py` around lines 468 - 494, Update _safe_assemble_tab_from_composer_data to catch only the bounded expected composer storage/schema errors represented by _COMPOSER_ROW_ERRORS, while preserving failure recording and the None return for those errors. Let all unexpected exceptions propagate instead of treating them as recoverable drift.Source: Linters/SAST tools
🤖 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_tabs.py`:
- Around line 468-494: Update _safe_assemble_tab_from_composer_data to catch
only the bounded expected composer storage/schema errors represented by
_COMPOSER_ROW_ERRORS, while preserving failure recording and the None return for
those errors. Let all unexpected exceptions propagate instead of treating them
as recoverable drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6731fc35-6505-4460-bec0-fe4d0ef47477
📒 Files selected for processing (2)
services/workspace_tabs.pytests/test_workspace_tabs_malformed_nested.py
3 participants
Closes #136
follow-up from the trim-model-exports review. broke up the big tab assembly and markdown export paths into smaller helpers, pulled the duplicate composer loop and tool-call rendering into shared functions, and replaced a few broad except Exception blocks with narrower ones. display_bubble.py now returns BubbleMetadata | None from build_storage_bubble_metadata so the casts can go.
no intentional user-visible behavior change. mypy --strict on the three files and full pytest -q (569 passed).
Summary by CodeRabbit