Conversation
Contributor
Reviewer's GuideRefactors GPT integration into reusable LLM classes and updates callers/tests, and adds ANSI-to-HTML conversion so server responses can return styled HTML when requested, along with a minor dependency and version bump. Sequence diagram for handling surf report HTTP request with HTML/ANSI conversionsequenceDiagram
actor User
participant Browser
participant FastAPIApp
participant default_route
participant SurfCli
participant ansi_to_html
User->>Browser: Request surf report
Browser->>FastAPIApp: GET / (Accept: text/html or text/plain)
FastAPIApp->>default_route: Call with Request
default_route->>SurfCli: run(args)
SurfCli-->>default_route: ANSI colored text output
default_route->>default_route: Read Accept header
alt Accept includes text/html
default_route->>ansi_to_html: ansi_to_html(output)
ansi_to_html-->>default_route: HTML string
default_route-->>Browser: HTMLResponse
else Accept does not include text/html
default_route-->>Browser: PlainTextResponse(output)
end
Browser-->>User: Rendered surf report
Updated class diagram for LLM abstraction and helper usageclassDiagram
class Llm {
- Any client
- str model
+ Llm(model)
+ call_llm(surf_summary, gpt_prompt) str
}
class FreeGpt {
+ FreeGpt(model)
}
class OpenAILlm {
+ OpenAILlm(api_key, model)
}
class Helper {
+ print_gpt(surf_data, gpt_prompt, gpt_info) str
}
Llm <|-- FreeGpt
Llm <|-- OpenAILlm
Helper ..> FreeGpt : uses
Helper ..> OpenAILlm : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ansi_to_html, the naivestr.replaceover the entire string can lead to incorrect or unbalanced<span>tags when multiple ANSI codes occur or when a reset is missing; consider parsing the string sequentially (e.g., with regex and a small state machine) to build properly nested HTML. - The
Llm.call_llmmethod always returns a string (either the model content or the fallback message), so the return type hintstr | Noneis misleading; tightening this tostrwill make downstream usage clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ansi_to_html`, the naive `str.replace` over the entire string can lead to incorrect or unbalanced `<span>` tags when multiple ANSI codes occur or when a reset is missing; consider parsing the string sequentially (e.g., with regex and a small state machine) to build properly nested HTML.
- The `Llm.call_llm` method always returns a string (either the model content or the fallback message), so the return type hint `str | None` is misleading; tightening this to `str` will make downstream usage clearer.
## Individual Comments
### Comment 1
<location path="src/gpt.py" line_range="15-20" />
<code_context>
- except Exception as e:
- logger.error("OpenAI request failed: %s", e)
- return "Unable to generate GPT response."
+class Llm(ABC):
+ def __init__(self, model):
+ self.model = model
+ self.client: Any = None
+
+ def call_llm(self, surf_summary, gpt_prompt) -> str | None:
+ try:
+ response = self.client.chat.completions.create(
</code_context>
<issue_to_address>
**issue:** The return type annotation for `call_llm` doesn’t match the actual behavior.
Since this function always returns a string (either model content or the fallback message) and never `None`, the annotation should be tightened to `str`. If you intend to allow `None`, please add explicit `None` returns in the appropriate paths so the hint matches actual behavior.
</issue_to_address>
### Comment 2
<location path="src/art.py" line_range="51" />
<code_context>
+}
+
+
+def ansi_to_html(text: str) -> str:
+ html = text.replace("&", "&").replace("<", "<").replace(">", ">")
+ for code, css in ANSI_TO_CSS.items():
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ANSI spans are never explicitly closed if the text ends without a reset code.
Because `</span>` is only emitted on `�[0m`, any final color/bold sequence without a reset leaves the last `<span>` open, producing invalid HTML that can affect subsequent styling. Please track when a span is opened and emit a closing `</span>` at the end if one is still active.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+20
| class Llm(ABC): | ||
| def __init__(self, model): | ||
| self.model = model | ||
| self.client: Any = None | ||
|
|
||
| def call_llm(self, surf_summary, gpt_prompt) -> str | None: |
Contributor
There was a problem hiding this comment.
issue: The return type annotation for call_llm doesn’t match the actual behavior.
Since this function always returns a string (either model content or the fallback message) and never None, the annotation should be tightened to str. If you intend to allow None, please add explicit None returns in the appropriate paths so the hint matches actual behavior.
| } | ||
|
|
||
|
|
||
| def ansi_to_html(text: str) -> str: |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): ANSI spans are never explicitly closed if the text ends without a reset code.
Because </span> is only emitted on �[0m, any final color/bold sequence without a reset leaves the last <span> open, producing invalid HTML that can affect subsequent styling. Please track when a span is opened and emit a closing </span> at the end if one is still active.
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
1 participant
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.
General:
Code:
Summary by Sourcery
Introduce a shared LLM abstraction, improve GPT integration usage, and add HTML rendering for colored CLI output in the server response.
New Features:
Enhancements:
Build:
Tests: