fix: strip claude title spinner frames - #2709
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:
📝 WalkthroughWalkthroughClaude detection now recognizes four additional half-circle spinner glyphs. Terminal title stripping also recognizes these glyphs. Configuration versions and tests were updated. ChangesClaude spinner glyph support
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@akbash-bot instead of hard coding claude glyphs, could we use the bundled/downloaded manifest which has regex for those ? in that why strip will be attached to manifests so both can be updated at the same time. wdyt ? |
|
Yes — I think the manifest is the better owner, especially because downloaded manifests can then track agent UI changes without a Herdr release. I would not infer stripping from the existing One important detail: manifest reload/update should recompute derived stripped titles and publish any resulting revision/event change, otherwise an idle pane could expose a new derived value without notifying subscribers. So this is broader than the one-line allowlist fix, but it gives us the right ownership and hot-update behavior. I’m in favor of revising the PR that way. |
|
@akbash-bot agreed then. please do a preflight and feel free to chance pr's direction in a kiss way. |
423ebf1 to
08a75a9
Compare
Greptile SummaryThis PR moves Claude title-spinner handling into the binary-owned terminal-title stripping logic.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported regex-validation issues are invalid at current HEAD because the manifest-owned title regex field and its runtime path were removed.
|
| Filename | Overview |
|---|---|
| src/terminal/title.rs | Adds four Claude spinner glyphs to title normalization and verifies each with focused test coverage. |
Reviews (8): Last reviewed commit: "fix: keep claude detection manifest unch..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09bfb177-fc56-4c84-8347-5dd7613e4e13
📒 Files selected for processing (10)
scripts/agent_detection_manifest_check.pysrc/app/actions.rssrc/app/api.rssrc/app/terminal_titles.rssrc/detect/manifest.rssrc/detect/manifest/tests.rssrc/detect/manifest_update.rssrc/detect/manifests/claude.tomlsrc/terminal/state.rssrc/terminal/title.rs
08a75a9 to
3e30478
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1961c098-4a64-497d-8e6d-ec7ecef36ac8
📒 Files selected for processing (2)
scripts/agent_detection_manifest_check.pyscripts/test_agent_detection_manifest_check.py
3e30478 to
34651dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 736956d5-0686-495e-ac54-647ab3404788
📒 Files selected for processing (12)
scripts/agent_detection_manifest_check.pyscripts/test_agent_detection_manifest_check.pysrc/app/actions.rssrc/app/agent_resume.rssrc/app/api.rssrc/app/mod.rssrc/app/terminal_titles.rssrc/detect/manifest.rssrc/detect/manifest/tests.rssrc/detect/manifests/claude.tomlsrc/terminal/state.rssrc/terminal/title.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/detect/manifest/tests.rs
- src/app/actions.rs
- scripts/agent_detection_manifest_check.py
- src/app/api.rs
- src/detect/manifests/claude.toml
- src/app/terminal_titles.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/terminal/title.rs (1)
1-13: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoute Claude title stripping through the active manifest.
stripped_terminal_titleruns for every terminal. Adding◐–◒toCLAUDE_ACTIVITY_GLYPHSstrips ordinary non-Claude titles such as◐ production. It also bypasses bundled, downloaded, and override manifest activation or reload. Remove Claude-specific glyphs from this global path and use the active manifest’sterminal_title_strip_prefix. Add regression cases for ordinary symbol-prefixed titles and manifest changes.Also applies to: 31-43
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f421f8e-27b8-43d1-bab6-57110467da3f
📒 Files selected for processing (4)
src/detect/manifest/tests.rssrc/detect/manifests/claude.tomlsrc/terminal/title.rswebsite/agent-detection/claude.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/detect/manifest/tests.rs
- src/detect/manifests/claude.toml
|
CodeRabbit’s manifest-routing suggestion is intentionally not applied. This revision removes that architecture: title glyph ownership remains in the existing binary path so detection manifests stay independently hot-updatable for older engine-2 clients. The local allowlist behavior is pre-existing; this fix only adds Claude’s four new U+25D0–U+25D3 activity frames. Greptile and the independent reviewer found no blocking issue. |
Summary
terminal_title_strippedremoves each frameChecks
just checkrefs #2707