feat(code-search): embed OSC 8 hyperlinks, filter to file matches#426
feat(code-search): embed OSC 8 hyperlinks, filter to file matches#426
Conversation
- TTY output wraps header and line numbers in OSC 8 hyperlinks so URLs stay clickable but are hidden from humans, reducing visual noise. - Non-TTY output prints the file URL once per result instead of per line; AI agents can reconstruct #L<n> anchors themselves, cutting token usage. - Add type:file to queries so repository-name matches no longer get silently filtered out and return "No results found" (e.g. LoadImage). - JSON output gains a file_url field alongside existing per-line URLs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dosubot
bot
added
size:M
|
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:
📝 WalkthroughWalkthroughBuilds queries by injecting Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant QueryBuilder
participant SourcegraphAPI
participant Formatter
participant Terminal
CLI->>QueryBuilder: build(query)
QueryBuilder-->>CLI: query (inject type:file if absent)
CLI->>SourcegraphAPI: fetch_results(query)
SourcegraphAPI-->>CLI: results (files + matches)
CLI->>Formatter: format(results) -> compute file_url, set match urls
Formatter->>Terminal: render(formatted) [checks isatty()]
alt TTY
Terminal-->>Formatter: emit OSC 8 hyperlinks for headers and green linked line numbers
else non-TTY
Terminal-->>Formatter: emit raw repo/path, one file_url per file, plain green line numbers
end
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #426 +/- ##
==========================================
+ Coverage 76.07% 76.30% +0.22%
==========================================
Files 35 35
Lines 4213 4249 +36
==========================================
+ Hits 3205 3242 +37
+ Misses 1008 1007 -1
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves comfy cs output by making terminal output more human-friendly (OSC 8 clickable links without showing raw URLs), reducing URL verbosity for non-TTY/piped output, and ensuring Sourcegraph queries return file matches consistently.
Changes:
- Adds
type:fileto the Sourcegraph query to avoid repo-name matches causing unexpected “No results found”. - Introduces
file_urlat the per-file result level and uses it to build per-line#L<n>URLs. - Updates CLI rendering to use OSC 8 hyperlinks in TTY and to print a single raw file URL per file in non-TTY output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
comfy_cli/command/code_search.py |
Builds type:file queries, constructs file_url, and changes printing logic for TTY vs non-TTY (OSC 8 links / reduced URL noise). |
tests/comfy_cli/command/test_code_search.py |
Updates query-string expectations to include type:file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| encoded_path = quote(file_path, safe="/") | ||
| file_url = f"https://github.com/{clean_name}/blob/{ref}/{encoded_path}" | ||
|
|
||
| line_matches = result.get("lineMatches") or [] | ||
| matches = [] | ||
| if line_matches: | ||
| encoded_path = quote(file_path, safe="/") | ||
| base_url = f"https://github.com/{clean_name}/blob/{ref}/{encoded_path}" | ||
| for m in line_matches: | ||
| line = m.get("lineNumber", 0) + 1 | ||
| preview = m.get("preview", "").rstrip() | ||
| matches.append({"line": line, "preview": preview, "url": f"{base_url}#L{line}"}) | ||
| for m in line_matches: | ||
| line = m.get("lineNumber", 0) + 1 | ||
| preview = m.get("preview", "").rstrip() | ||
| matches.append({"line": line, "preview": preview, "url": f"{file_url}#L{line}"}) | ||
|
|
||
| formatted.append( | ||
| { | ||
| "repository": clean_name, | ||
| "file": file_path, | ||
| "file_url": file_url, | ||
| "branch": branch_name, |
There was a problem hiding this comment.
New behavior in _format_results adds a top-level file_url field per file result, but the tests currently only assert per-line match['url']. Add assertions that file_url is present and correctly formed/encoded (and that match['url'] is based on it) so regressions in URL construction don’t slip through.
There was a problem hiding this comment.
Fixed in 3ebca67 — added explicit file_url assertion and made match['url'] assert it derives from file_url (f"{first['file_url']}#L42").
comfy_cli/command/code_search.py
Outdated
| is_tty = console.is_terminal | ||
|
|
||
| for file_result in results: | ||
| repo = file_result["repository"] | ||
| path = file_result["file"] | ||
| file_url = file_result["file_url"] | ||
|
|
||
| header = Text() | ||
| header.append(file_result["repository"], style="bold cyan") | ||
| header.append(" / ", style="dim") | ||
| header.append(file_result["file"], style="bold") | ||
| if is_tty: | ||
| # Humans: clickable OSC 8 hyperlink, URL hidden from visible output. | ||
| header.append(f"{repo} / {path}", style=f"bold cyan link {file_url}") | ||
| else: | ||
| # Non-TTY (pipes, AI agents): print the raw URL once per file so | ||
| # agents can synthesize #L<line> anchors themselves. | ||
| header.append(f"{repo} / {path}\n") | ||
| header.append(f" {file_url}", style="dim") | ||
| console.print(header) | ||
|
|
||
| for match in file_result["matches"]: | ||
| line_text = Text() | ||
| line_text.append(f" L{match['line']:>5}", style="green") | ||
| line_text = Text(" ") | ||
| line_style = f"green link {match['url']}" if is_tty else "green" | ||
| line_text.append(f"L{match['line']:>5}", style=line_style) | ||
| line_text.append(f" {match['preview']}") | ||
| console.print(line_text) | ||
| console.print(f" [dim]{match['url']}[/dim]") | ||
|
|
There was a problem hiding this comment.
The TTY vs non-TTY formatting change (OSC 8 links in TTY, single raw file_url line in non-TTY, and removal of per-line URL printing) isn’t explicitly covered by tests. Add coverage that exercises both branches (e.g., by monkeypatching comfy_cli.command.code_search.console to Console(force_terminal=True/False, file=io.StringIO())) and asserts that non-TTY output includes exactly one file URL per file and that per-line URLs aren’t printed.
There was a problem hiding this comment.
TTY vs non-TTY coverage already exists in the prior commit (97bc0ed): test_tty_emits_osc8_and_hides_urls uses a Console(force_terminal=True) monkeypatch, and test_non_tty_prints_file_url_once_and_no_per_line_urls plus test_non_tty_ignores_force_color_env cover the non-TTY branch. The assertion quality was improved in 3ebca67.
- TTY detection now uses sys.stdout.isatty() directly. Rich's console.is_terminal treats FORCE_COLOR=1 / TTY_COMPATIBLE=1 as terminal-capable even when stdout is redirected, which would leak OSC 8 escapes into piped output. - type:file is only appended when the user's query doesn't already carry a type: filter, so queries like `type:commit fix bug` still work. - Update CLI help to document that file matches are the default. - Add tests for: user type: filter preservation, non-TTY suppression of OSC 8 under FORCE_COLOR, and OSC 8 emission in forced-TTY mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dosubot
bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 344-346: The assertion using
output.count("https://github.com/Comfy-Org/ComfyUI/blob/abc123def456/nodes.py\n")
>= 0 is a no-op because count() is always non-negative; update the check in
tests/comfy_cli/command/test_code_search.py (the assertions that inspect the
variable output) to assert the string appears at least once (e.g., change >= 0
to >= 1) or assert the exact expected occurrence (e.g., == 1) to actually verify
the URL is printed once per file.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bfd08ad-5769-480b-9f37-2adcf690db69
📒 Files selected for processing (2)
comfy_cli/command/code_search.pytests/comfy_cli/command/test_code_search.py
Generated via Rich export_svg() showing the three output modes: - before: per-line URL spam - after TTY: clickable OSC 8 links, URLs hidden - after pipe: one URL per file, AI-friendly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_screenshots.py`:
- Around line 1-108: Ruff formatter changes are required for this file; run the
formatter and commit the result to satisfy CI. Run `ruff format
scripts/generate_screenshots.py` (or your project’s ruff formatting command),
verify the file-level changes, and commit them so functions like render_before,
render_after_tty, render_after_pipe, make_svg and the script __main__ block are
updated per ruff’s style rules; then push the formatted file to unblock CI.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aa07f8c-6fd5-4916-8cbf-9483260f241c
⛔ Files ignored due to path filters (3)
assets/after_pipe.svgis excluded by!**/*.svgassets/after_tty.svgis excluded by!**/*.svgassets/before.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
scripts/generate_screenshots.py
Amp-Thread-ID: https://ampcode.com/threads/T-019d8137-973a-7192-a1d5-6b0c7df11bce Co-authored-by: Amp <amp@ampcode.com>
- Fix count() >= 0 assertion to == 1 (always true, caught by CodeRabbit) - Assert file_url is present and match URLs derive from it (Copilot review) Amp-Thread-ID: https://ampcode.com/threads/T-019d8137-973a-7192-a1d5-6b0c7df11bce Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_screenshots.py`:
- Around line 100-110: Add a concise docstring to the make_svg function
describing its purpose, parameters, and return value to satisfy pylint C0116:
explain that make_svg(render_fn, title: str) renders content to an in-memory
Console and returns an SVG string, document the render_fn callable parameter
(expects a Console and performs rendering) and the title parameter, and note the
return type is a str containing the exported SVG; place this short docstring
immediately under the def make_svg(...) declaration.
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 355-377: The test test_tty_emits_osc8_and_hides_urls currently
only checks for OSC 8 escapes and an anchor fragment but doesn't confirm raw
URLs are not visible; update the assertions after calling
_print_results(results, stats, json_output=False) to strip ANSI/OSC control
sequences from buf.getvalue() (e.g., remove ESC sequences/OSC 8 payloads) and
then assert the cleaned output contains the visible header/link text you expect
(e.g., "class LoadImage:") and does NOT contain raw "http" or full "https://"
URLs; keep references to _format_results, _get_stats, and _print_results so the
test still uses the same rendering path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b39076c5-2768-4f3b-ba2e-c7f5dab4cf55
📒 Files selected for processing (2)
scripts/generate_screenshots.pytests/comfy_cli/command/test_code_search.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Amp-Thread-ID: https://ampcode.com/threads/T-019d8137-973a-7192-a1d5-6b0c7df11bce Co-authored-by: Amp <amp@ampcode.com>
Strip OSC 8 and SGR escapes, then assert visible text contains headers and previews but no raw https:// URLs. Amp-Thread-ID: https://ampcode.com/threads/T-019d8137-973a-7192-a1d5-6b0c7df11bce Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/generate_screenshots.py`:
- Around line 117-123: The code uses a relative "assets" path and default
encoding when writing SVGs; make it cwd-independent by computing the repository
root from the script location (e.g., repo_root =
pathlib.Path(__file__).resolve().parent.parent) and set out = repo_root /
"assets", ensure directories exist with out.mkdir(parents=True, exist_ok=True),
and write files with an explicit encoding (e.g., use Path.write_text(...,
encoding="utf-8") or open(..., "w", encoding="utf-8")) for the calls that create
before.svg/after_tty.svg/after_pipe.svg (references: out, make_svg,
render_before, render_after_tty, render_after_pipe).
In `@tests/comfy_cli/command/test_code_search.py`:
- Around line 141-149: Add a new unit test named
test_repo_and_user_type_filter_preserved that calls _build_query with a repo
argument and a user-provided type: filter (e.g., _build_query("type:commit fix
bug", "owner/repo", DEFAULT_COUNT)); assert that "type:file" is not injected and
that the returned query string contains the repo prefix and preserves the user's
type: filter in the expected order (e.g., starts with the repo prefix and
includes count and the original "type:commit fix bug"); use the existing
assertion style from test_user_type_filter_preserved and
test_user_type_file_not_duplicated to mirror expectations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4968c164-f898-414d-a26c-1645f845c62f
📒 Files selected for processing (2)
scripts/generate_screenshots.pytests/comfy_cli/command/test_code_search.py
| out = pathlib.Path("assets") | ||
| out.mkdir(exist_ok=True) | ||
|
|
||
| (out / "before.svg").write_text(make_svg(render_before, "comfy cs LoadImage (before)")) | ||
| (out / "after_tty.svg").write_text(make_svg(render_after_tty, "comfy cs LoadImage (after — TTY, links clickable)")) | ||
| (out / "after_pipe.svg").write_text(make_svg(render_after_pipe, "comfy cs LoadImage | cat (after — piped / AI)")) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make asset generation cwd-independent and encoding-explicit.
Line 117 currently uses a relative "assets" path and default encoding. Running this script outside repo root can write files to an unexpected directory, and default encoding can vary by environment.
♻️ Suggested hardening patch
if __name__ == "__main__":
import pathlib
- out = pathlib.Path("assets")
+ repo_root = pathlib.Path(__file__).resolve().parents[1]
+ out = repo_root / "assets"
out.mkdir(exist_ok=True)
- (out / "before.svg").write_text(make_svg(render_before, "comfy cs LoadImage (before)"))
- (out / "after_tty.svg").write_text(make_svg(render_after_tty, "comfy cs LoadImage (after — TTY, links clickable)"))
- (out / "after_pipe.svg").write_text(make_svg(render_after_pipe, "comfy cs LoadImage | cat (after — piped / AI)"))
+ (out / "before.svg").write_text(make_svg(render_before, "comfy cs LoadImage (before)"), encoding="utf-8")
+ (out / "after_tty.svg").write_text(
+ make_svg(render_after_tty, "comfy cs LoadImage (after — TTY, links clickable)"), encoding="utf-8"
+ )
+ (out / "after_pipe.svg").write_text(
+ make_svg(render_after_pipe, "comfy cs LoadImage | cat (after — piped / AI)"), encoding="utf-8"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate_screenshots.py` around lines 117 - 123, The code uses a
relative "assets" path and default encoding when writing SVGs; make it
cwd-independent by computing the repository root from the script location (e.g.,
repo_root = pathlib.Path(__file__).resolve().parent.parent) and set out =
repo_root / "assets", ensure directories exist with out.mkdir(parents=True,
exist_ok=True), and write files with an explicit encoding (e.g., use
Path.write_text(..., encoding="utf-8") or open(..., "w", encoding="utf-8")) for
the calls that create before.svg/after_tty.svg/after_pipe.svg (references: out,
make_svg, render_before, render_after_tty, render_after_pipe).
| def test_user_type_filter_preserved(self): | ||
| """Don't inject type:file when the user already specified a type: filter.""" | ||
| result = _build_query("type:commit fix bug", None, DEFAULT_COUNT) | ||
| assert "type:file" not in result | ||
| assert result == f"count:{DEFAULT_COUNT} type:commit fix bug" | ||
|
|
||
| def test_user_type_file_not_duplicated(self): | ||
| result = _build_query("type:file LoadImage", None, DEFAULT_COUNT) | ||
| assert result.count("type:file") == 1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one combo case: --repo plus explicit user type: filter.
You already verify each behavior independently; adding the combined case would close a regression gap where both constraints interact (repo prefix + preserving user type:). Small test, big net.
🧪 Suggested test addition
class TestBuildQuery:
@@
def test_user_type_file_not_duplicated(self):
result = _build_query("type:file LoadImage", None, DEFAULT_COUNT)
assert result.count("type:file") == 1
+
+ def test_repo_with_user_type_filter_preserved(self):
+ result = _build_query("type:commit fix bug", "ComfyUI", DEFAULT_COUNT)
+ assert "type:file" not in result
+ assert result == f"repo:^Comfy\\-Org/ComfyUI$ count:{DEFAULT_COUNT} type:commit fix bug"🧰 Tools
🪛 Pylint (4.0.5)
[convention] 147-147: Missing function or method docstring
(C0116)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/comfy_cli/command/test_code_search.py` around lines 141 - 149, Add a
new unit test named test_repo_and_user_type_filter_preserved that calls
_build_query with a repo argument and a user-provided type: filter (e.g.,
_build_query("type:commit fix bug", "owner/repo", DEFAULT_COUNT)); assert that
"type:file" is not injected and that the returned query string contains the repo
prefix and preserves the user's type: filter in the expected order (e.g., starts
with the repo prefix and includes count and the original "type:commit fix bug");
use the existing assertion style from test_user_type_filter_preserved and
test_user_type_file_not_duplicated to mirror expectations.
Summary
L<n>are clickable in supported terminals (iTerm2, WezTerm, VS Code, GNOME Terminal), but the URLs themselves no longer clutter the screen for humans.#L<n>anchors from the line numbers, meaningfully reducing token count on large results.type:fileto the Sourcegraph query (when user hasn't specified their owntype:filter) so repository-name matches no longer get silently filtered to "No results found" (e.g.LoadImagewhere a repo name happens to contain the term).file_urlfield alongside existing per-lineurls — no breaking change.sys.stdout.isatty()directly instead of Rich'sconsole.is_terminalto avoidFORCE_COLOR/TTY_COMPATIBLEenv vars leaking OSC 8 escapes into piped output.Before / After
Before — per-line URL spam
After — TTY (human, OSC 8 links clickable, URLs hidden)
After — piped / AI agent (one URL per file)
Test plan
uv run --with pytest pytest tests/comfy_cli/command/test_code_search.py— 40/40 passuv run comfy cs --count 3 LoadImage— now returns FileMatch results (previously "No results found")uv run comfy cs --count 3 CheckpointLoaderSimple | cat— non-TTY shows one URL per fileforce_terminal=TrueFORCE_COLOR=1does not leak OSC 8 into piped outputtype:filters are preserved (not overridden bytype:file)🤖 Generated with Claude Code