#30 refactor(app): Refresh the user-interface - #32
Conversation
…e control flow support, including try-except blocks, and introduce new test files.
… add new code examples.
|
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:
📝 WalkthroughWalkthroughMoves flowchart logic into a new src layout (core, resources), replaces removed legacy serpent modules, adds a top-level Streamlit app, updates packaging/CI/pre-commit, adds tests for try/except and break/continue theming, and updates gitignore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as "User\n(Streamlit UI)"
participant App as "app.py\n(Streamlit)"
participant Parser as "AST\n(ast.parse)"
participant Core as "PythonFlowchartGV\n(src/serpent/core.py)"
participant Graphviz as "Graphviz\n(Digraph)"
participant Browser as "Browser\n(Render/Download)"
User->>App: paste/edit Python code + options
App->>Parser: parse & validate AST
Parser->>Core: traverse AST -> create nodes/edges
Core->>Graphviz: build Digraph with styles/labels
Graphviz-->>App: return DOT / renderable image
App->>Browser: render flowchart and present downloads
Browser->>App: request download (PNG or DOT)
App-->>Browser: serve file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app.py`:
- Around line 155-168: The CSS in the stylable_container for
key="output_container" uses an invalid property name "justify_content"; change
it to the correct hyphenated CSS property "justify-content: center;" inside the
css_styles string so the container contents are horizontally centered as
intended (locate the stylable_container block with key "output_container" and
update the css_styles value).
- Around line 148-150: The Generate Flowchart button (generate_btn) is created
but never used, so the flowchart output renders every rerun; either wrap the
output-generation block (the section that renders the flowchart) in an if
generate_btn: guard so the chart only renders when the button is clicked, or
remove the button if you intend automatic rendering—update the code paths that
reference chart_title/flowchart output accordingly to live inside the if
generate_btn: block (or delete the generate_btn declaration and any UI text) to
avoid the decorative button.
In `@src/serpent/resources.py`:
- Around line 9-46: Theme entries "break" and "continue" are never used because
visit_Break and visit_Continue create nodes with shape="box" so new_node
resolves color via self.style_config.get(shape, ...) — either remove those dead
keys or make the renderer use semantic node types: update new_node to accept an
explicit node_type (or semantic_type) parameter and change its fill/color lookup
to use self.style_config.get(node_type, self.style_config.get(shape, "white")),
then update visit_Break and visit_Continue to call new_node(...,
node_type="break") / new_node(..., node_type="continue"); alternatively, if you
prefer deleting config, remove "break" and "continue" from THEMES in
resources.py to avoid misleading entries.
In `@tests/test_try_except.py`:
- Around line 4-5: Fix the docstring in test_try_except_structure: change "Test
that try/except blocks are correctly structure." to "Test that try/except blocks
are correctly structured." so the description is grammatically correct; update
the triple-quoted string in the test_try_except_structure function accordingly.
🧹 Nitpick comments (12)
src/serpent/resources.py (1)
5-6: Prefer built-indictovertyping.Dicton Python 3.10+.The project targets Python
^3.10(perpyproject.toml), soDictfromtypingis unnecessary — built-indictsupports subscript notation natively.Proposed fix
-import textwrap -from typing import Dict +import textwrap -THEMES: Dict[str, Dict[str, str]] = { +THEMES: dict[str, dict[str, str]] = {src/serpent/core.py (4)
20-39:next_edge_labelis never initialized in__init__— initialize it to avoid the fragilegetattrpattern.
new_nodeusesgetattr(self, "next_edge_label", None)because the attribute isn't declared in__init__. Multiple visit methods set it toNone, but it's never set to a truthy value anywhere in this file, making the entire override mechanism (lines 51–54) dead code.At minimum, initialize it in
__init__to make the intent explicit and drop thegetattr:Proposed fix
self.loop_stack: list[dict[str, Any]] = [] + self.next_edge_label: Optional[str] = None # Default colors if not providedThen in
new_node:- override_label = getattr(self, "next_edge_label", None) + override_label = self.next_edge_label
81-87:visit_FunctionDefdoesn't handleAsyncFunctionDef.Python's AST has a separate
ast.AsyncFunctionDefnode forasync deffunctions. Without avisit_AsyncFunctionDef, async functions will fall through togeneric_visitand render as a plain"AsyncFunctionDef"box instead of the nicer"Function: name"oval.Proposed fix — alias the visitor
def visit_FunctionDef(self, node: ast.FunctionDef) -> None: """Handle function definition.""" name = node.name start_node = self.new_node(f"Function: {name}", shape="oval") self.last_nodes = [start_node] for stmt in node.body: self.visit(stmt) + + visit_AsyncFunctionDef = visit_FunctionDef
173-189:breakandcontinuenodes use a generic"box"shape — they won't pick up theme colors from the"break"/"continue"keys.As noted in
resources.py, the THEMES dict defines"break"and"continue"color keys, but these visit methods passshape="box"tonew_node, which looks up colors by Graphviz shape name. These semantic keys are never matched.If you want distinct colors for break/continue, you could introduce a
style_keyparameter tonew_nodethat overrides the shape-based lookup:Sketch — add a `style_key` override to `new_node`
def new_node( self, label: str, shape: str = "box", connect_from: Optional[list[Union[str, tuple[str, Optional[str]]]]] = None, edge_label: str = "", + style_key: Optional[str] = None, ) -> str: ... - fillcolor = self.style_config.get(shape, "white") + fillcolor = self.style_config.get(style_key or shape, "white")Then in the visitors:
- break_node = self.new_node("break", shape="box") + break_node = self.new_node("break", shape="box", style_key="break") ... - cont_node = self.new_node("continue", shape="box") + cont_node = self.new_node("continue", shape="box", style_key="continue")
215-266:visit_Trydoesn't handleast.TryStar(Python 3.11+except*syntax).Since the project targets
^3.10,try...except*blocks on 3.11+ will silently fall through togeneric_visitand render as a bare"TryStar"box. This is a minor gap — worth a note or a stub visitor.app.py (5)
9-11: Unused imports:textwrap,Dict,Any.
textwrap,Dict, andAnyare imported but not referenced anywhere in this file.Proposed fix
import ast import re import shutil import logging -import textwrap from pathlib import Path -from typing import Dict, Any
73-80: Use iterable unpacking instead of list concatenation.Per the Ruff hint (RUF005), prefer spread syntax for clarity:
Proposed fix
- ["(Custom)"] + list(EXAMPLES.keys()), + ["(Custom)", *EXAMPLES.keys()],
140-146: Dead code — thepasson line 146 and the surrounding block do nothing.The comment indicates the intent is to switch the dropdown to
"(Custom)"when the user edits an example, but the implementation is a no-op. Either implement the logic (e.g., by settingst.session_state.example_selector = "(Custom)") or remove the dead branch to avoid confusion.
200-202: Fragile scoping:'valid_graph' in locals()is a code smell.
valid_graphis only assigned inside atry/exceptblock nested within a conditional, so it may not exist in the outer scope. A cleaner approach is to initialize it before the output column:Proposed fix
Initialize before the output column block:
valid_graph = None(e.g., after line 116), then replace the guard:
- if 'valid_graph' in locals() and valid_graph: + if valid_graph:
22-27: FirstImage.opencan raise errors other thanFileNotFoundError.If the file exists but is corrupted or not a valid image,
PIL.Image.openwill raiseUnidentifiedImageError(a subclass ofOSError). Consider catchingOSErrorinstead, orExceptionif you want maximum resilience for the logo loading path.tests/test_app_smoke.py (2)
12-12:import appis unused in the test.The test function uses
AppTest.from_file(...)to load the app, so this module-level import serves no purpose. If the intent is to verify importability, make that an explicit test assertion.
20-26: Smoke test no longer asserts anything meaningful beyond "no crash".The prior assertions on title/caption were removed and replaced with comments and
pass. While a "no exception" check is valuable, consider adding at least one positive assertion (e.g., checking for the presence of specific UI elements) to catch regressions in the app structure.
Co-authored-by: Meeth Amin aminmeeth89@gmail.com
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app.py`:
- Around line 170-199: Initialize the variable valid_graph to None before the
top-level if that checks code.strip() so it always exists for the subsequent
download button logic; keep the existing try/except that assigns valid_graph
when generate_graphviz_flowchart succeeds or sets it to None on error
(SyntaxError and generic Exception) and then simplify any later checks to rely
on the initialized valid_graph instead of using 'valid_graph' in locals().
- Around line 22-27: The Image.open calls for logo and icon can raise
UnidentifiedImageError/OSError, so broaden the except to catch OSError (or
Exception) around the try block that calls Image.open(assets_dir /
"serpent_logo_transparent.png") and the conditional Image.open for
"serpent_logo_compact.png"; on exception set logo = None and icon = None (and
optionally log the error) so any asset-read failure falls back safely instead of
crashing at startup.
- Around line 141-146: The branch detecting edits to st.session_state.code_input
currently does nothing; replace the no-op with logic that sets the example
selector to "(Custom)" and triggers a rerun so the UI reflects the change: when
code != st.session_state.code_input and selected_example != "(Custom)", set the
session state key used for the dropdown (e.g.,
st.session_state.example_selector) to "(Custom)" and call st.rerun() to refresh
the app, or alternatively remove the entire branch if you prefer not to
auto-switch.
- Around line 55-59: The call to st.image(logo, width="stretch") uses the
width="stretch" flag added in Streamlit 1.49.0 but the project requires
>=1.46.1, so replace the unsupported parameter or bump the dependency: either
change the st.image call in the sidebar (the st.sidebar block / st.image usage
where logo is handled) to use an integer width (e.g., a pixel value or None)
instead of width="stretch", or update the project requirement to "streamlit =
^1.49.0" so width="stretch" is valid; update only the st.image invocation or the
dependency constraint accordingly.
🧹 Nitpick comments (4)
tests/test_try_except.py (1)
1-1: Blank first line — no issue, but consider adding the module docstring or removing it.Nit: Line 1 is blank before the import. Typically test files either start with a docstring or directly with imports.
app.py (3)
73-76: Prefer unpacking over concatenation (static analysis hint).Per Ruff RUF005, use iterable unpacking instead of list concatenation for clarity.
Proposed fix
- ["(Custom)"] + list(EXAMPLES.keys()), + ["(Custom)", *EXAMPLES.keys()],
220-221: BareExceptioncatch hides unexpected failures.Ruff BLE001 flags this. Consider catching a narrower exception (e.g.,
graphviz.ExecutableNotFoundorsubprocess.CalledProcessError) so genuinely unexpected errors aren't silently swallowed.
11-11: Unused imports:Dict,Any.Neither
DictnorAnyappear to be used anywhere in this file.
Co-authored by: Iqra Sayeed <iqrsay15@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_break_continue_colors.py`:
- Around line 28-40: Replace the manual print/if/raise pattern in the test that
checks break and continue node colors with pytest-style assertions: for the
break check, assert that either f'fillcolor="{break_color}"' in dot_source or
f'fillcolor={break_color}' in dot_source, providing a clear assertion message
referencing break_color/dot_source; do the same for the continue check using
continue_color/dot_source. Locate the checks around the variables break_color,
continue_color and dot_source in tests/test_break_continue_colors.py (replace
the two if/else blocks and print calls with two assert statements). Ensure the
assertion messages are descriptive so pytest failure output is clear.
- Line 23: Remove the unused local variable box_color (assigned from
theme["box"]) in tests/test_break_continue_colors.py; locate the assignment
"box_color = theme['box']" and delete that line (or replace any intended use
with direct theme["box"] if needed) so there are no unused variable warnings
from Ruff (F841).
🧹 Nitpick comments (2)
src/serpent/core.py (1)
57-59: Remove extra blank lines.There are two consecutive blank lines (58–59) that appear to be leftover from editing.
Proposed fix
fillcolor = self.style_config.get(node_type, self.style_config.get(shape, "white")) - - node_id = f"n{self.counter}"tests/test_break_continue_colors.py (1)
10-10: Moveimport textwrapto the top of the file.Standard library imports belong at module level per PEP 8.
Proposed fix
+import textwrap + from serpent.core import generate_graphviz_flowchart from serpent.resources import THEMES def test_break_continue_colors(): """ Verify that break and continue nodes get their specific colors from the theme configuration. """ - import textwrap code = textwrap.dedent("""
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.pre-commit-config.yaml:
- Around line 15-21: There is a duplicate repo entry for
https://github.com/pre-commit/pre-commit-hooks; remove the second block and
merge its hooks (ids: check-toml, check-json, pretty-format-json with args
[--autofix]) into the existing pre-commit-hooks repo declaration (the block with
rev v6.0.0) so all hooks for that repo live under a single repo entry and avoid
duplicate repo declarations.
- Line 11: The Black hook's pinned revision "rev: 25.1.0" appears to downgrade
from the previous 25.9.0 — either confirm this is intentional or update the
pinned revision to a current stable release (e.g., "rev: 26.1.0"); locate the
Black hook entry in .pre-commit-config.yaml (the line containing "rev: 25.1.0")
and replace it with the chosen version, then run pre-commit autoupdate or
validate the change locally to ensure compatibility.
In `@app.py`:
- Line 11: The import line bringing in typing symbols includes Dict (and Any)
but Dict is unused in app.py; remove Dict from the import (or remove the entire
typing import if neither Any nor Dict is used) by editing the import statement
that currently reads "from typing import Any, Dict" so it only imports what's
used (e.g., "from typing import Any") or remove it entirely if Any is also
unused.
🧹 Nitpick comments (7)
src/serpent/core.py (3)
20-39: Initializenext_edge_labelin__init__.
next_edge_labelis read viagetattr(self, "next_edge_label", None)on line 52 because it isn't set in__init__, yet it's assigned in multiplevisit_*methods. Initializing it in__init__removes the need for defensivegetattrand makes the class's state explicit.Proposed fix
self.style_config = style_config or { "box": "lightyellow", "diamond": "lightblue", "oval": "lightgreen", "circle": "thistle", "parallelogram": "lightcyan", } + self.next_edge_label: Optional[str] = NoneThen simplify line 52:
- override_label = getattr(self, "next_edge_label", None) + override_label = self.next_edge_label
84-90:visit_FunctionDefdoes not handleAsyncFunctionDef.
ast.AsyncFunctionDefis a separate node type and won't be visited byvisit_FunctionDef. If async functions appear in user code, they'll fall through togeneric_visitand render as a plain"AsyncFunctionDef"box instead of the nicer"Function: name"format.Proposed fix
+ visit_AsyncFunctionDef = visit_FunctionDefAdd this line after the
visit_FunctionDefmethod (e.g., after line 90).
281-283:visit_Passsilently swallows the node — consider adding a no-op flowchart node.Currently
passstatements produce no node at all, which means a function body containing onlypasswill have a dangling start node with no successor. This could be confusing in the rendered flowchart. A lightweight"pass"box (like break/continue) would keep the graph connected.tests/test_try_except.py (1)
1-32: Consider adding a test for nested or baretry/except(without else/finally).The current test only covers the full
try/except/else/finallypath. A simplertry/exceptcase would improve coverage of thevisit_Trybranches wherenode.orelseandnode.finalbodyare empty.app.py (3)
176-205: Redundantast.parse— code is parsed twice on every render.Line 181 calls
ast.parse(code)for validation, butgenerate_graphviz_flowchart(line 187) also callsast.parseinternally. Moreover,generate_graphviz_flowchartalready handlesSyntaxErrorby returning an error graph — so the explicit validation here creates a confusing dual error-handling path. Consider removing the redundant parse and relying solely ongenerate_graphviz_flowchart's built-in error handling, or inspecting the returned graph for an error indicator.Proposed fix
if not code.strip(): st.info("Waiting for code input...") else: try: - # Validate - ast.parse(code) - # Generate final_title = chart_title or "Flowchart" selected_theme = THEMES[theme_name] @@ .. st.graphviz_chart(graph, width="stretch") valid_graph = graph - except SyntaxError as e: - st.error(f"Syntax Error: {e}") - valid_graph = None except Exception as e: st.error(f"An error occurred: {e}") logging.exception("Graph generation failed") valid_graph = None
77-79: Nit: prefer unpacking over concatenation per Ruff RUF005.Proposed fix
- ["(Custom)"] + list(EXAMPLES.keys()), + ["(Custom)", *EXAMPLES.keys()],
34-53: Module-level Streamlit calls (set_page_config,st.markdown) run at import time.These calls execute when the module is imported, not when
main()is called. This makes the module untestable in isolation (importing it has side effects) and can conflict if another script imports fromapp.py. Consider moving these intomain().
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/pylint.yml:
- Around line 55-60: The template literal passed into
github.rest.issues.createComment interpolates bodyContent directly (see
bodyContent and github.rest.issues.createComment) which allows backticks or
${...} from pylint to break the literal; fix by either constructing the comment
body with regular string concatenation (e.g., join pieces with '+' so
bodyContent is appended as a normal string) or sanitize bodyContent before
interpolation by escaping backticks and `${` sequences (e.g.,
replace(/`/g,'\\`') and replace(/\$\{/g,'\\${')) and then use the sanitized
variable in the createComment call.
In `@app.py`:
- Around line 175-204: Initialize valid_graph = None before the input/validation
block so it is always defined regardless of exceptions; move a pre-declaration
of valid_graph (e.g., just before the "if not code.strip()" check) and keep the
existing assignments inside the try/except (where generate_graphviz_flowchart,
st.graphviz_chart, and the except branches touch valid_graph) to ensure
valid_graph is defined even if ast.parse or any other call raises an exception.
🧹 Nitpick comments (3)
.github/workflows/pylint.yml (1)
33-68: Workflow posts a new comment on every push — consider updating an existing comment instead.Each push to the PR will trigger a new pylint comment, leading to comment spam. A common pattern is to search for an existing bot comment (by a marker string) and update it, or delete the old one before creating a new one.
🔧 Sketch: find-and-update pattern
+ const marker = '<!-- pylint-report -->'; + const { data: comments } = await github.rest.issues.listComments({ + issue_number: context.issue.number, + owner: context.repo.owner, + repo: context.repo.repo, + }); + const existing = comments.find(c => c.body.includes(marker)); + const body = `${marker}\n### ${icon} ${summary}\n\n<details>\n<summary>View Pylint Report</summary>\n\n\`\`\`\n${bodyContent}\n\`\`\`\n</details>`; + + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { await github.rest.issues.createComment({ issue_number: context.issue.number, owner: context.repo.owner, repo: context.repo.repo, - body: `### ${icon} ${summary}\n\n<details>\n<summary>View Pylint Report</summary>\n\n\`\`\`\n${bodyContent}\n\`\`\`\n</details>` + body, }); + }app.py (2)
76-78: Nit: prefer unpacking over concatenation.Per Ruff RUF005, use spread syntax for cleaner list construction.
Suggested change
- ["(Custom)"] + list(EXAMPLES.keys()), + ["(Custom)", *EXAMPLES.keys()],
216-228: Bareexcept Exceptionon PNG generation is acceptable here, but consider logging.The
except Exceptionat line 227 catches any Graphviz rendering failure and shows a user-friendly warning — that's reasonable. Consider addinglogging.exception(...)(as you do on line 203) so failures are diagnosable in logs.Suggested change
except Exception: + logging.exception("PNG export failed") st.warning("Could not generate PNG (Check Graphviz installation).")
| await github.rest.issues.createComment({ | ||
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| body: `### ${icon} ${summary}\n\n<details>\n<summary>View Pylint Report</summary>\n\n\`\`\`\n${bodyContent}\n\`\`\`\n</details>` | ||
| }); |
There was a problem hiding this comment.
Template-literal injection risk — pylint output may contain backticks or ${.
bodyContent is interpolated directly into a JS template literal on line 59. Pylint output frequently contains code snippets with backticks (`) or expressions like ${...}, which would break the template literal or cause unintended evaluation.
Use string concatenation or escape backticks before interpolation.
🔧 Proposed fix
+ // Escape backticks and dollar-braces so the template literal is safe
+ bodyContent = bodyContent.replace(/`/g, '\\`').replace(/\$/g, '\\$');
+
await github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: `### ${icon} ${summary}\n\n<details>\n<summary>View Pylint Report</summary>\n\n\`\`\`\n${bodyContent}\n\`\`\`\n</details>`
});🤖 Prompt for AI Agents
In @.github/workflows/pylint.yml around lines 55 - 60, The template literal
passed into github.rest.issues.createComment interpolates bodyContent directly
(see bodyContent and github.rest.issues.createComment) which allows backticks or
${...} from pylint to break the literal; fix by either constructing the comment
body with regular string concatenation (e.g., join pieces with '+' so
bodyContent is appended as a normal string) or sanitize bodyContent before
interpolation by escaping backticks and `${` sequences (e.g.,
replace(/`/g,'\\`') and replace(/\$\{/g,'\\${')) and then use the sanitized
variable in the createComment call.
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
|
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
Co-authored-by: Meeth Amin <aminmeeth89@gmail.com>
… and refactor app UI rendering into helper functions.
…ng an `on_change` callback instead of an imperative check.
✅ Pylint passedView Pylint Report |
1 participant
Summary by CodeRabbit