feat: add ui-review mode with judge panel support#115
Conversation
- Add 7 UI/UX reference docs in automation/ui_review_refs/ - Add ui_review_prompts.py for building review prompts and judge instructions - Add commands/ui_review.py with --file, --description, --pr, --output, --fresh flags - Register cube ui-review command in cli.py - Extend judge_panel.py with ui-review review_type: - Skip worktree prefetch and writer branch checks - Exclude cli-review judges (e.g. CodeRabbit) from ui-review - Add P0/P1/P2 status parsing in _parse_decision_status - Add ui-review header in panel display block Findings from multiple judges are merged by priority and displayed with judge attribution. Supports cross-page consistency review via multiple --file flags. No changes to existing review modes.
|
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:
WalkthroughAdds a UI-review flow: new prompt/judge instruction builders, UI reference docs, a ui-review CLI command and handler, branching in the judge panel to run UI-only judges and emit P0/P1/P2-formatted decisions, conditional worktree handling for UI reviews, and an extended PR fetch signature supporting repo scope and full head SHA. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
🧹 Nitpick comments (2)
python/cube/automation/ui_review_prompts.py (1)
33-82: Avoid the unusedtask_idparameter.
task_idisn’t used (Ruff ARG001). Either remove it or surface it in the prompt for traceability.♻️ Suggested tweak
- return f"""# UI Review - -You are a UI/UX reviewer. Use the knowledge base below to produce a structured, prioritized review report. + return f"""# UI Review + +Task ID: {task_id} + +You are a UI/UX reviewer. Use the knowledge base below to produce a structured, prioritized review report.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/automation/ui_review_prompts.py` around lines 33 - 82, The function build_ui_review_prompt currently has an unused parameter task_id (causing ARG001); either remove task_id from the signature and all callers, or surface it in the generated prompt for traceability by inserting a short "Review ID: {task_id}" (or similar) near the top of the returned prompt string (e.g., directly after "# UI Review") so the parameter is used; update any callers if you remove the parameter.python/cube/commands/ui_review.py (1)
85-134: Filter out CLI-review judges to avoid noisy “missing decision” warnings.UI review launch excludes cli-review judges, but
_collect_findingsstill iterates all configs, so it warns for judges that never ran. Filtering here keeps output tidy.♻️ Suggested tweak
- for jconfig in get_judge_configs(): + for jconfig in (j for j in get_judge_configs() if j.type != "cli-review"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/commands/ui_review.py` around lines 85 - 134, _collect_findings currently iterates all judge configs and prints "No decision file" warnings for cli-review judges that aren't part of UI runs; filter out CLI-review judges early to avoid noisy warnings by skipping any jconfig whose key indicates the CLI reviewer (e.g., if jconfig.key == "cli-review") before computing get_decision_file_path and processing files—add a guard in the loop inside _collect_findings to continue for CLI-review configs so only UI-related judges are checked.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
python/cube/automation/judge_panel.pypython/cube/automation/ui_review_prompts.pypython/cube/automation/ui_review_refs/checklists.mdpython/cube/automation/ui_review_refs/design-psych.mdpython/cube/automation/ui_review_refs/icons.mdpython/cube/automation/ui_review_refs/interaction-psychology.mdpython/cube/automation/ui_review_refs/principles.mdpython/cube/automation/ui_review_refs/review-template.mdpython/cube/automation/ui_review_refs/system-principles.mdpython/cube/cli.pypython/cube/commands/ui_review.py
🧰 Additional context used
🧬 Code graph analysis (2)
python/cube/cli.py (1)
python/cube/commands/ui_review.py (1)
ui_review_command(215-331)
python/cube/commands/ui_review.py (7)
python/cube/github/pulls.py (2)
check_gh_installed(23-29)fetch_pr(32-87)python/cube/cli.py (2)
pr(461-507)clean(573-581)python/cube/core/decision_parser.py (1)
get_decision_file_path(57-76)python/cube/core/user_config.py (1)
get_judge_configs(323-326)python/cube/automation/stream.py (1)
add(19-38)python/cube/automation/judge_panel.py (1)
launch_judge_panel(296-645)python/cube/automation/ui_review_prompts.py (1)
build_ui_review_prompt(33-82)
🪛 LanguageTool
python/cube/automation/ui_review_refs/principles.md
[uncategorized] ~45-~45: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...need a label; icon-only is reserved for universally-known actions. - Show constraints before subm...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
python/cube/automation/ui_review_refs/interaction-psychology.md
[grammar] ~21-~21: It seems that the correct verb form here is “mean”.
Context: ...ith the number of choices. More options means slower decisions and higher abandonment...
(AGREEMENT_SENT_START_2)
[grammar] ~97-~97: Consider using “to” with “prefer”.
Context: ...tive recovery cost. Practical rules: - Prefer inline interactions over modals; prefer modals over page redirects. - I...
(PREFER_OVER_TO)
[grammar] ~97-~97: Consider using “to” with “prefer”.
Context: ...Prefer inline interactions over modals; prefer modals over page redirects. - If a sub-task can be compl...
(PREFER_OVER_TO)
python/cube/automation/ui_review_refs/icons.md
[grammar] ~7-~7: Consider using “to” with “prefer”.
Context: ... mix outlined/filled/3D/emoji styles. - Prefer obvious meanings over clever metaphors. If an icon can be misunderstood, add a...
(PREFER_OVER_TO)
[uncategorized] ~15-~15: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...or text+icon; icon-only is reserved for universally-known actions. - Tooltips: tooltips are suppo...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[grammar] ~18-~18: Consider using “to” with “prefer”.
Context: ...rimary way to understand an action. ## Prefer Text Over Icons When - The action is uncommon in your ...
(PREFER_OVER_TO)
python/cube/automation/ui_review_refs/system-principles.md
[uncategorized] ~12-~12: Possible missing comma found.
Context: ...efinition: Each screen has one dominant objective with the highest visual and interaction...
(AI_HYDRA_LEO_MISSING_COMMA)
python/cube/automation/ui_review_refs/design-psych.md
[misspelling] ~29-~29: This word is normally spelled as one.
Context: ...ng to show what belongs together. - For multi-part objects, align actions with the part th...
(EN_COMPOUNDS_MULTI_PART)
python/cube/automation/ui_review_refs/checklists.md
[uncategorized] ~25-~25: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...OK/Done). - Icon-only is reserved for universally-known actions (search/close/more/settings). -...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[grammar] ~30-~30: The verb form ‘hover’ does not seem to be suitable in this context.
Context: ... - Card/list rows that open should have hover + chevron/affordance cue or a clear "Vi...
(HAVE_VB)
🪛 Ruff (0.15.1)
python/cube/cli.py
[warning] 566-566: Do not catch blind exception: Exception
(BLE001)
python/cube/commands/ui_review.py
[warning] 71-71: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 77-80: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 253-253: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
python/cube/automation/ui_review_prompts.py
[warning] 34-34: Unused function argument: task_id
(ARG001)
🔇 Additional comments (23)
python/cube/automation/ui_review_refs/review-template.md (1)
1-59: Well-structured review template.The template provides clear priority levels (P0/P1/P2) with consistent structure for each finding type. The diagnosis categories align well with the design psychology concepts defined in other reference documents. The verification checklist is comprehensive and actionable.
python/cube/automation/ui_review_refs/icons.md (1)
1-40: Clear and practical icon guidelines.The document provides actionable rules for icon usage with sensible recommendations. The static analysis hints regarding "Prefer X over Y" phrasing and hyphenation are false positives — the current phrasing is idiomatic for guideline documents.
python/cube/automation/ui_review_refs/system-principles.md (1)
1-84: Comprehensive system-level principles.The principles are well-defined with clear review questions. The "UI Copy Source Discipline" section (lines 15-29) is particularly valuable for ensuring AI-generated content doesn't leak implementation details into user-facing copy. The static analysis hint about a missing comma is a false positive.
python/cube/automation/ui_review_refs/interaction-psychology.md (1)
1-145: Solid HCI reference material.The document provides a well-organised compact reference of established HCI principles with practical rules. The specific touch target sizes (44×44 CSS px for web, 48×48 dp for mobile) on line 14 are aligned with platform guidelines.
python/cube/automation/ui_review_refs/principles.md (1)
1-96: Comprehensive UI/UX principles document.The document is well-structured with clear hierarchy. The "Anti-AI Self-Check" section (lines 89-96) is particularly valuable for catching common AI-generated UI issues like decorative gradients and emoji misuse. The concrete spacing values (4px base unit with defined scale) provide actionable guidance.
python/cube/automation/ui_review_refs/checklists.md (1)
1-101: Practical expanded checklists.The document provides detailed, actionable checklists for specific UI patterns and page types. The Universal States section (lines 5-19) ensures comprehensive state coverage, and the Motion Checklist (lines 76-84) includes useful red flags for identifying anti-patterns.
python/cube/automation/ui_review_refs/design-psych.md (1)
1-89: Useful design psychology reference.The document provides clear explanations of core design psychology concepts with practical rules for each. The concepts align with established design literature (Norman's principles). The static analysis hint about "multi-part" spelling is a false positive — hyphenated compound is acceptable here.
python/cube/automation/judge_panel.py (6)
237-241: LGTM — Clean handling of UI-review decision parsing.The early return for
ui-reviewreview type correctly extracts P0/P1/P2 findings and returns a summary string without falling through to other review type logic.
317-320: LGTM — Correctly skips worktree prefetch for UI reviews.UI reviews operate on UI artifacts rather than writer branches, so skipping the worktree prefetch is appropriate.
338-340: LGTM — Appropriate judge filtering for UI reviews.Excluding
cli-reviewtype judges makes sense as they are code-specific tools not suited for UI/UX evaluation.
558-568: LGTM — Consistently skips branch operations for UI reviews.This is consistent with the earlier worktree prefetch skip; UI reviews don't involve writer branches.
571-574: LGTM — Clear UI-review banner and guidance.The banner follows the existing pattern for other review types and provides clear guidance on the expected output format (P0/P1/P2 findings).
366-369: No action needed. The function signature inui_review_prompts.pyisbuild_ui_review_judge_instructions(task_id: str) -> str:, which accepts onlytask_idas required. The import and call at lines 368–369 are correct and follow the lazy-import pattern used for other review types.python/cube/automation/ui_review_prompts.py (3)
7-16: Reference list is tidy and easy to extend.Centralising the filenames keeps the knowledge base manageable.
19-30: Ref loading and knowledge-base assembly look solid.Gracefully skips missing refs and keeps separation clear.
85-144: Judge instruction block is clear and complete.Nice, explicit schema and priority rules.
python/cube/cli.py (1)
22-22: UI review command import is wired correctly.python/cube/commands/ui_review.py (6)
15-21: UI allowlist constants look sensible.
24-37: File concatenation helper is clean.
63-82: PR UI diff fetch flow is clear.
137-139: Markup stripping helper is fine for plain-text output.
142-213: Report rendering is consistent and readable.
215-331: End-to-end UI review orchestration looks coherent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube/cli.py`:
- Around line 526-569: The ui_review() command currently catches all Exceptions
which swallows and reprints typer.Exit raised by ui_review_command; update the
exception handling so typer.Exit is allowed to bubble: add an explicit except
typer.Exit: raise block before the generic except Exception so ui_review_command
can control its own exit behavior, and keep the existing except Exception to
call _print_error(e) and then import sys; sys.exit(1). Reference: function
ui_review, call to ui_review_command, helper _print_error, and typer.Exit.
In `@python/cube/commands/ui_review.py`:
- Around line 40-60: The diff filtering currently uses parts[-1].lstrip("b/")
which strips any leading 'b' or '/' characters and can mangle paths; in
_filter_ui_diff replace that with exact prefix handling (e.g., check if
parts[-1].startswith("b/") and then remove the first two characters) so filepath
becomes the true path before computing suffix and filename for the
UI_FILE_EXTENSIONS and UI_CONFIG_FILENAMES checks; ensure you only remove the
exact "b/" prefix and leave other leading characters intact.
---
Nitpick comments:
In `@python/cube/automation/ui_review_prompts.py`:
- Around line 33-82: The function build_ui_review_prompt currently has an unused
parameter task_id (causing ARG001); either remove task_id from the signature and
all callers, or surface it in the generated prompt for traceability by inserting
a short "Review ID: {task_id}" (or similar) near the top of the returned prompt
string (e.g., directly after "# UI Review") so the parameter is used; update any
callers if you remove the parameter.
In `@python/cube/commands/ui_review.py`:
- Around line 85-134: _collect_findings currently iterates all judge configs and
prints "No decision file" warnings for cli-review judges that aren't part of UI
runs; filter out CLI-review judges early to avoid noisy warnings by skipping any
jconfig whose key indicates the CLI reviewer (e.g., if jconfig.key ==
"cli-review") before computing get_decision_file_path and processing files—add a
guard in the loop inside _collect_findings to continue for CLI-review configs so
only UI-related judges are checked.
| @app.command(name="ui-review") | ||
| def ui_review( | ||
| file: Annotated[ | ||
| Optional[list[str]], typer.Option("--file", "-f", help="UI file(s) to review (repeatable)") | ||
| ] = None, | ||
| description: Annotated[ | ||
| Optional[str], typer.Option("--description", "-d", help="Free-text description of the UI to review") | ||
| ] = None, | ||
| context: Annotated[ | ||
| str, typer.Option("--context", "-c", help="Platform, target user, and primary task (one-liner)") | ||
| ] = "", | ||
| pr: Annotated[ | ||
| Optional[int], typer.Option("--pr", help="GitHub PR number — review UI-related file changes only") | ||
| ] = None, | ||
| output: Annotated[ | ||
| Optional[str], typer.Option("--output", "-o", help="Save report to this file path") | ||
| ] = None, | ||
| fresh: Annotated[ | ||
| bool, typer.Option("--fresh", help="Start fresh judge sessions (ignore cached sessions)") | ||
| ] = False, | ||
| ): | ||
| """Review UI/UX quality using the judge panel. | ||
|
|
||
| Examples: | ||
| cube ui-review --file web-ui/src/pages/Dashboard.tsx | ||
| cube ui-review --file web-ui/src/pages/Dashboard.tsx --file web-ui/src/pages/TaskDetail.tsx | ||
| cube ui-review --pr 123 | ||
| cube ui-review --description "Settings page with 15 ungrouped toggles" | ||
| cube ui-review --file src/App.tsx --context "web dashboard, developer users, monitor agent tasks" | ||
| cube ui-review --file src/App.tsx --output reviews/dashboard.md | ||
| """ | ||
| try: | ||
| ui_review_command( | ||
| files=list(file) if file else [], | ||
| description=description, | ||
| context=context, | ||
| pr=pr, | ||
| output=output, | ||
| fresh=fresh, | ||
| ) | ||
| except Exception as e: | ||
| _print_error(e) | ||
| import sys | ||
| sys.exit(1) |
There was a problem hiding this comment.
Let typer.Exit bubble to avoid double error output.
Line 557-567: ui_review_command already prints validation errors and raises typer.Exit; catching Exception here reprints and normalises the exit. Catch typer.Exit separately.
✅ Safer error handling
try:
ui_review_command(
files=list(file) if file else [],
description=description,
context=context,
pr=pr,
output=output,
fresh=fresh,
)
- except Exception as e:
+ except typer.Exit:
+ raise
+ except Exception as e:
_print_error(e)
import sys
sys.exit(1)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 566-566: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cube/cli.py` around lines 526 - 569, The ui_review() command currently
catches all Exceptions which swallows and reprints typer.Exit raised by
ui_review_command; update the exception handling so typer.Exit is allowed to
bubble: add an explicit except typer.Exit: raise block before the generic except
Exception so ui_review_command can control its own exit behavior, and keep the
existing except Exception to call _print_error(e) and then import sys;
sys.exit(1). Reference: function ui_review, call to ui_review_command, helper
_print_error, and typer.Exit.
There was a problem hiding this comment.
Agreed — will let typer.Exit bubble up directly and only catch unexpected exceptions separately.
There was a problem hiding this comment.
@roy-songzhe-li, sounds good! Just a reminder that the fix should be applied to both ui_review command definitions in cli.py, since the AI summary noted a duplicate registration of @app.command(name="ui-review") was introduced. The corrected pattern for each:
try:
ui_review_command(
files=list(file) if file else [],
description=description,
context=context,
pr=pr,
repo=repo,
output=output,
post=post,
fresh=fresh,
)
except typer.Exit:
raise
except Exception as e:
_print_error(e)
sys.exit(1)Also, the duplicate command definition itself should be resolved — Typer will raise an error at startup if the same command name is registered twice, so one of the two @app.command(name="ui-review") blocks should be removed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
python/cube/cli.py (1)
564-578:⚠️ Potential issue | 🟡 MinorLet
typer.Exitbubble to avoid double error output.Line 575 catches all exceptions and reprints, which also intercepts
typer.Exitraised byui_review_command(it already prints validation errors). Re-raisetyper.Exitfirst.#!/bin/bash # Locate typer.Exit usage to confirm it is raised in command validation paths. rg -n --type=py 'typer\.Exit'✅ Proposed fix
try: ui_review_command( files=list(file) if file else [], description=description, context=context, pr=pr, repo=repo, output=output, post=post, fresh=fresh, ) + except typer.Exit: + raise except Exception as e: _print_error(e) import sys sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/cli.py` around lines 564 - 578, The except block around the ui_review_command call is catching all exceptions including typer.Exit and causing duplicate error output; update the handler so that if the exception is an instance of typer.Exit it is re-raised (allowing typer's own output/exit behavior), otherwise call _print_error(e) and exit as before—refer to ui_review_command and _print_error and check for typer.Exit before handling other exceptions.python/cube/commands/ui_review.py (1)
46-55:⚠️ Potential issue | 🟡 MinorAvoid
lstrip("b/")path mangling.Line 52 uses
lstrip("b/"), which strips any leadingbor/characters (e.g.,b/b.cssbecomes.css), breaking the suffix check. Strip the exact"b/"prefix instead.#!/bin/bash # Demonstrate str.lstrip behaviour with a "b/" prefix. python - <<'PY' print("b/b.css".lstrip("b/")) # Expected output: .css PY🔧 Proposed fix
- filepath = parts[-1].lstrip("b/") + filepath = parts[-1] + if filepath.startswith("b/"): + filepath = filepath[2:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/commands/ui_review.py` around lines 46 - 55, The code in the loop that computes filepath uses filepath = parts[-1].lstrip("b/"), which incorrectly strips any leading 'b' or '/' characters and mangles paths (affecting suffix and filename checks). Change that to remove only the exact "b/" prefix (e.g. use filepath = filepath.removeprefix("b/") or if filepath.startswith("b/"): filepath = filepath[2:]) before computing suffix and filename so UI_FILE_EXTENSIONS and UI_CONFIG_FILENAMES checks work correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cube/commands/ui_review.py`:
- Around line 271-274: The code writes report to output_path without ensuring
its parent directory exists, which fails when the target is in a non-existent
folder; before calling output_path.write_text(clean) in the output_path handling
block, create the parent directory for output_path (use output_path.parent.mkdir
with parents=True and exist_ok=True or equivalent) so the directory is created
if missing, then proceed to write_text and print_success as before (refer to
symbols output_path, write_text, output_path.parent, _strip_markup,
print_success).
- Around line 317-327: The PR task_id currently set as task_id =
f"ui-review-pr-{pr}" can collide across repos; update the task_id generation in
the PR branch (where pr is not None, near _fetch_pr_ui_diff) to include a repo
slug when repo is provided (e.g., use the repo string or repo identifier,
sanitize slashes into dashes) so the id becomes something like
"ui-review-pr-{repo_slug}-{pr}"; ensure the same sanitized slug logic is applied
wherever task_id is later used for prompt/decision file names.
---
Duplicate comments:
In `@python/cube/cli.py`:
- Around line 564-578: The except block around the ui_review_command call is
catching all exceptions including typer.Exit and causing duplicate error output;
update the handler so that if the exception is an instance of typer.Exit it is
re-raised (allowing typer's own output/exit behavior), otherwise call
_print_error(e) and exit as before—refer to ui_review_command and _print_error
and check for typer.Exit before handling other exceptions.
In `@python/cube/commands/ui_review.py`:
- Around line 46-55: The code in the loop that computes filepath uses filepath =
parts[-1].lstrip("b/"), which incorrectly strips any leading 'b' or '/'
characters and mangles paths (affecting suffix and filename checks). Change that
to remove only the exact "b/" prefix (e.g. use filepath =
filepath.removeprefix("b/") or if filepath.startswith("b/"): filepath =
filepath[2:]) before computing suffix and filename so UI_FILE_EXTENSIONS and
UI_CONFIG_FILENAMES checks work correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
python/cube/cli.pypython/cube/commands/ui_review.pypython/cube/github/pulls.pypython/cube/github/reviews.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.1)
python/cube/github/reviews.py
[error] 221-221: subprocess call: check for execution of untrusted input
(S603)
[warning] 223-223: Avoid specifying long messages outside the exception class
(TRY003)
python/cube/cli.py
[warning] 575-575: Do not catch blind exception: Exception
(BLE001)
python/cube/github/pulls.py
[error] 55-55: subprocess call: check for execution of untrusted input
(S603)
[warning] 56-57: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 74-74: Consider ["gh", "pr", "diff", str(pr_number), *repo_flags] instead of concatenation
Replace with ["gh", "pr", "diff", str(pr_number), *repo_flags]
(RUF005)
python/cube/commands/ui_review.py
[warning] 76-76: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 82-85: Avoid specifying long messages outside the exception class
(TRY003)
[error] 198-198: f-string without any placeholders
Remove extraneous f prefix
(F541)
[warning] 323-323: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
python/cube/github/pulls.py (1)
32-75: Repo-scoped PR fetch looks solid.The optional repo handling is threaded consistently without changing default behaviour.
python/cube/github/reviews.py (1)
201-224:post_pr_commentwiring looks good.The helper is straightforward and fits the existing CLI posting pattern.
roy-songzhe-li
force-pushed
the
feature/ui-review
branch
from
57c1e78 to
532e962
Compare
…ad_sha - pulls.py: add repo param to fetch_pr; fix head_sha to use full SHA (was [:7], caused 422 on GitHub review API) - ui_review_prompts.py: add path/line fields to P0/P1 decision format so judges can reference specific diff lines - ui_review.py: add --repo for cross-repo PRs; add --post to publish as GitHub review with inline comments; fallback to plain comment on API error - cli.py: expose --repo, --post flags
There was a problem hiding this comment.
♻️ Duplicate comments (4)
python/cube/commands/ui_review.py (3)
265-335:⚠️ Potential issue | 🟠 MajorCreate the output directory before writing the report.
Line 332–334 will fail if
--outputtargets a non-existent folder (e.g.,reviews/report.md).🛠️ Proposed fix
if output_path: + output_path.parent.mkdir(parents=True, exist_ok=True) clean = "\n".join(_strip_markup(line) for line in lines) output_path.write_text(clean) print_success(f"Report saved to {output_path}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/commands/ui_review.py` around lines 265 - 335, The function _display_findings writes the cleaned report to output_path without ensuring the parent directory exists, which will raise if the target folder is missing; before calling output_path.write_text(clean) in _display_findings, create the directory with output_path.parent.mkdir(parents=True, exist_ok=True) (and only do this when output_path.parent is not empty) so the file can be written safely, then proceed to write and call print_success as before.
338-474:⚠️ Potential issue | 🟠 MajorInclude repo slug in PR task IDs to avoid collisions.
Line 389 uses only the PR number, so prompts/decisions can clash across repos with the same PR ID.
🧭 Suggested fix
- task_id = f"ui-review-pr-{pr}" + repo_slug = (repo or "local").replace("/", "-") + task_id = f"ui-review-pr-{repo_slug}-{pr}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/commands/ui_review.py` around lines 338 - 474, The PR-only branch of ui_review_command builds task_id using only the PR number (task_id = f"ui-review-pr-{pr}"), which can collide across repos; update that branch to include the repo slug (use the provided repo parameter, normalizing "/" to "-" or falling back to a short local tag) when constructing task_id (e.g., incorporate repo into task_id), and ensure any related uses (prompt_path naming, launch_judge_panel call, and _collect_findings/_post_inline_review invocations that use task_id or head_sha) continue to reference the new task_id format; key symbols: ui_review_command, task_id, _fetch_pr_ui_diff, launch_judge_panel, _collect_findings, _post_inline_review.
40-60:⚠️ Potential issue | 🟡 MinorUse exact
b/prefix stripping to avoid path mangling.Line 52 uses
lstrip("b/"), which removes any leadingbor/characters and can corrupt paths likeb/b.css. Strip the exact prefix instead.🛠️ Safer prefix handling
- filepath = parts[-1].lstrip("b/") + filepath = parts[-1] + if filepath.startswith("b/"): + filepath = filepath[2:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/commands/ui_review.py` around lines 40 - 60, The code in _filter_ui_diff uses filepath = parts[-1].lstrip("b/") which can remove any leading 'b' or '/' characters and corrupt paths (e.g., b/b.css); change this to strip the exact "b/" prefix instead by checking if parts[-1] starts with "b/" and then slicing off the first two characters (update the variable filepath accordingly) before computing suffix/name so Path(filepath).suffix and Path(filepath).name work correctly; ensure this exact-prefix logic replaces the lstrip call.python/cube/cli.py (1)
526-577:⚠️ Potential issue | 🟡 MinorLet
typer.Exitbubble to avoid double error output.Line 563–577 catches all
Exception, which interceptstyper.Exitand reprints errors.✅ Safer error handling
try: ui_review_command( files=list(file) if file else [], description=description, context=context, pr=pr, repo=repo, output=output, post=post, fresh=fresh, ) - except Exception as e: + except typer.Exit: + raise + except Exception as e: _print_error(e) import sys sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/cli.py` around lines 526 - 577, The current ui_review wrapper catches all Exception and therefore intercepts typer.Exit, causing duplicate error output; update the exception handling in the ui_review function so typer.Exit is allowed to propagate: either add an explicit except typer.Exit: raise before the generic except, or in the generic except check isinstance(e, typer.Exit) and re-raise; keep the existing _print_error(e) and sys.exit(1) behavior only for non-typer.Exit exceptions. Ensure you reference the ui_review function and its call to ui_review_command and preserve existing cleanup/logging for other exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/cube/cli.py`:
- Around line 526-577: The current ui_review wrapper catches all Exception and
therefore intercepts typer.Exit, causing duplicate error output; update the
exception handling in the ui_review function so typer.Exit is allowed to
propagate: either add an explicit except typer.Exit: raise before the generic
except, or in the generic except check isinstance(e, typer.Exit) and re-raise;
keep the existing _print_error(e) and sys.exit(1) behavior only for
non-typer.Exit exceptions. Ensure you reference the ui_review function and its
call to ui_review_command and preserve existing cleanup/logging for other
exceptions.
In `@python/cube/commands/ui_review.py`:
- Around line 265-335: The function _display_findings writes the cleaned report
to output_path without ensuring the parent directory exists, which will raise if
the target folder is missing; before calling output_path.write_text(clean) in
_display_findings, create the directory with
output_path.parent.mkdir(parents=True, exist_ok=True) (and only do this when
output_path.parent is not empty) so the file can be written safely, then proceed
to write and call print_success as before.
- Around line 338-474: The PR-only branch of ui_review_command builds task_id
using only the PR number (task_id = f"ui-review-pr-{pr}"), which can collide
across repos; update that branch to include the repo slug (use the provided repo
parameter, normalizing "/" to "-" or falling back to a short local tag) when
constructing task_id (e.g., incorporate repo into task_id), and ensure any
related uses (prompt_path naming, launch_judge_panel call, and
_collect_findings/_post_inline_review invocations that use task_id or head_sha)
continue to reference the new task_id format; key symbols: ui_review_command,
task_id, _fetch_pr_ui_diff, launch_judge_panel, _collect_findings,
_post_inline_review.
- Around line 40-60: The code in _filter_ui_diff uses filepath =
parts[-1].lstrip("b/") which can remove any leading 'b' or '/' characters and
corrupt paths (e.g., b/b.css); change this to strip the exact "b/" prefix
instead by checking if parts[-1] starts with "b/" and then slicing off the first
two characters (update the variable filepath accordingly) before computing
suffix/name so Path(filepath).suffix and Path(filepath).name work correctly;
ensure this exact-prefix logic replaces the lstrip call.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
python/cube/automation/ui_review_prompts.pypython/cube/cli.pypython/cube/commands/ui_review.pypython/cube/github/pulls.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
python/cube/cli.py (1)
python/cube/commands/ui_review.py (1)
ui_review_command(338-474)
🪛 Ruff (0.15.2)
python/cube/cli.py
[warning] 574-574: Do not catch blind exception: Exception
(BLE001)
python/cube/commands/ui_review.py
[warning] 71-71: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 77-80: Avoid specifying long messages outside the exception class
(TRY003)
[error] 223-223: subprocess call: check for execution of untrusted input
(S603)
[error] 224-224: Starting a process with a partial executable path
(S607)
[error] 233-233: subprocess call: check for execution of untrusted input
(S603)
[warning] 241-241: Avoid specifying long messages outside the exception class
(TRY003)
[error] 252-252: Starting a process with a partial executable path
(S607)
[warning] 386-386: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
python/cube/automation/ui_review_prompts.py
[warning] 34-34: Unused function argument: task_id
(ARG001)
python/cube/github/pulls.py
[error] 54-54: subprocess call: check for execution of untrusted input
(S603)
[warning] 55-56: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 73-73: Consider ["gh", "pr", "diff", str(pr_number), *repo_flags] instead of concatenation
Replace with ["gh", "pr", "diff", str(pr_number), *repo_flags]
(RUF005)
🔇 Additional comments (10)
python/cube/github/pulls.py (1)
32-90: Solid repo-scoped PR fetch update.Line 32–90 cleanly threads the optional repo into both metadata and diff calls, and keeping the full head SHA improves downstream accuracy.
python/cube/automation/ui_review_prompts.py (3)
7-31: Reference loading looks robust.Skipping missing files and joining non-empty content is a clean, safe approach.
33-82: Prompt composition is clear and well-structured.Context handling and input labelling are straightforward and easy to follow.
85-153: Judge instruction block is crisp and enforceable.The JSON schema and path/line rules are explicit and actionable.
python/cube/commands/ui_review.py (6)
24-37: File ingestion is straightforward and resilient.Missing files and read failures are handled cleanly without aborting the run.
63-82: UI-diff filtering is nicely gated.Clear error messaging when no UI files change makes the UX solid.
85-134: Findings aggregation reads well.The per-judge tagging and quick-win de-duplication are well-implemented.
137-245: Inline review assembly looks correct.The summary + inline split and fallback path are handled cleanly.
247-257: Repo resolution helper is concise.Good to keep the CLI call small and return a safe empty string on failure.
260-262: Markup stripping helper is fine.Simple and effective for Rich tags.
…ud mode cursor-agent without --print tries to connect to local Cursor IDE server on 127.0.0.1:7897 which may not be running. With --print and --trust, cursor-agent connects directly to Cursor's cloud API.
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 `@python/cube/core/adapters/cursor.py`:
- Line 33: The command array assigned to cmd in
python/cube/core/adapters/cursor.py includes an invalid --trust flag which
causes cursor-agent to fail; remove the --trust element from the cmd list (the
array building the invocation of cursor-agent, e.g., the cmd variable created
where the command is constructed) so the resulting invocation matches the
documented pattern (e.g., cursor-agent --print --force --output-format
stream-json --stream-partial-output --model <model> "prompt") and ensure any
tests or callers still pass the prompt argument in the correct position.
jacsamell
left a comment
jacsamell
left a comment
There was a problem hiding this comment.
Review
Good concept and clean structure. A few issues to address:
1. cursor.py — --trust and --print added globally (biggest risk)
python/cube/core/adapters/cursor.py — --trust bypasses permission prompts for all agents (writers, judges, prompters), not just UI review. This was added to make UI review work but affects every flow. Please scope it or confirm these flags are intentionally global.
2. pulls.py — SHA truncation removed, affects all callers
python/cube/github/pulls.py — head_sha now returns the full 40-char SHA instead of a 7-char short SHA. Other code that displays or uses head_sha may not expect this. Either revert here and truncate only in ui_review.py where the full SHA is needed for the GitHub API.
3. ui_review.py — No deduplication of P0/P1/P2 findings across judges
If 3 judges flag the same issue it appears 3 times. quick_wins is deduplicated but findings are not. Group by finding and list contributing judges — same pattern as the pr-review deduper.
4. ui_review.py — Timestamp task ID = never resumable for file/description input
task_id = f"ui-review-{datetime.now().strftime('%Y%m%d-%H%M%S')}" — every run creates new decision files. If a judge is interrupted you start over. For --pr it's stable, for --file/--description it isn't. Consider a hash of the input instead.
5. ui_review.py — --post uses emoji in GitHub comments
The reference docs in this PR explicitly say "No emoji as icons or UI decoration." The --post output uses 🔴 🟡 ⚪. Swap for text: P0 — Blocker, P1 — Important, P2 — Polish.
6. ui_review_prompts.py — Full KB (~600+ lines) embedded in every judge prompt
No caching. Expensive and may push task content out of context for smaller models. Consider passing the KB once or trimming by input type.
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)
python/cube/github/pulls.py (1)
54-77:⚠️ Potential issue | 🟠 MajorAdd timeouts for
ghsubprocess calls infetch_pr.Line 54 and Line 72 call external network-backed commands without
timeout, so the command can hang indefinitely and stall the whole review flow.⏱️ Suggested fix
- result = subprocess.run( - ["gh", "pr", "view", str(pr_number), "--json", "number,title,body,headRefName,baseRefName,headRefOid,reviews"] - + repo_flags, - capture_output=True, - text=True, - cwd=cwd, - ) + try: + result = subprocess.run( + [ + "gh", + "pr", + "view", + str(pr_number), + "--json", + "number,title,body,headRefName,baseRefName,headRefOid,reviews", + *repo_flags, + ], + capture_output=True, + text=True, + cwd=cwd, + timeout=30, + ) + except subprocess.TimeoutExpired as e: + raise RuntimeError(f"Timed out fetching PR #{pr_number} metadata") from e @@ - diff_result = subprocess.run( - ["gh", "pr", "diff", str(pr_number)] + repo_flags, - capture_output=True, - text=True, - cwd=cwd, - ) + try: + diff_result = subprocess.run( + ["gh", "pr", "diff", str(pr_number), *repo_flags], + capture_output=True, + text=True, + cwd=cwd, + timeout=45, + ) + except subprocess.TimeoutExpired as e: + raise RuntimeError(f"Timed out fetching PR #{pr_number} diff") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/github/pulls.py` around lines 54 - 77, The subprocess.run calls in fetch_pr (the calls that set result and diff_result for "gh pr view" and "gh pr diff") lack timeouts and can hang; add a timeout argument (e.g. timeout=30 or a configurable parameter) to both subprocess.run invocations and catch subprocess.TimeoutExpired around those calls to raise a clear RuntimeError (include which PR and that the gh command timed out). Ensure you update both the "gh pr view" call that assigns result and the "gh pr diff" call that assigns diff_result and handle the TimeoutExpired exception to avoid stalling the review flow.
♻️ Duplicate comments (1)
python/cube/cli.py (1)
566-580:⚠️ Potential issue | 🟡 MinorLet
typer.Exitbubble inui_review.Line 577 catches
Exception, which also capturestyper.Exitraised byui_review_commandand can reprint/normalise controlled exits.✅ Suggested fix
try: ui_review_command( files=list(file) if file else [], description=description, context=context, pr=pr, repo=repo, output=output, post=post, fresh=fresh, ) + except typer.Exit: + raise except Exception as e: _print_error(e) import sys sys.exit(1)#!/bin/bash # Verify root cause by checking: # 1) ui_review_command raises typer.Exit # 2) ui_review currently catches broad Exception without a typer.Exit passthrough rg -n -C2 'raise typer\.Exit|def ui_review\(|except typer\.Exit|except Exception as e' \ python/cube/commands/ui_review.py \ python/cube/cli.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cube/cli.py` around lines 566 - 580, The try/except in the CLI is catching typer.Exit raised by ui_review_command; update the exception handling so typer.Exit is allowed to propagate: in the try block around ui_review_command, add an explicit except typer.Exit: raise (or re-raise) before the broad except Exception that calls _print_error and sys.exit(1), ensuring ui_review_command, ui_review_command(...) and _print_error remain the referenced symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cube/github/pulls.py`:
- Around line 54-77: The subprocess.run calls in fetch_pr (the calls that set
result and diff_result for "gh pr view" and "gh pr diff") lack timeouts and can
hang; add a timeout argument (e.g. timeout=30 or a configurable parameter) to
both subprocess.run invocations and catch subprocess.TimeoutExpired around those
calls to raise a clear RuntimeError (include which PR and that the gh command
timed out). Ensure you update both the "gh pr view" call that assigns result and
the "gh pr diff" call that assigns diff_result and handle the TimeoutExpired
exception to avoid stalling the review flow.
---
Duplicate comments:
In `@python/cube/cli.py`:
- Around line 566-580: The try/except in the CLI is catching typer.Exit raised
by ui_review_command; update the exception handling so typer.Exit is allowed to
propagate: in the try block around ui_review_command, add an explicit except
typer.Exit: raise (or re-raise) before the broad except Exception that calls
_print_error and sys.exit(1), ensuring ui_review_command, ui_review_command(...)
and _print_error remain the referenced symbols to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c323679-331c-4a47-92ee-5f95d43e0bdb
📒 Files selected for processing (2)
python/cube/cli.pypython/cube/github/pulls.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (1)
python/cube/cli.py (1)
python/cube/commands/ui_review.py (1)
ui_review_command(338-474)
🪛 Ruff (0.15.4)
python/cube/github/pulls.py
[error] 54-54: subprocess call: check for execution of untrusted input
(S603)
[warning] 55-56: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 73-73: Consider ["gh", "pr", "diff", str(pr_number), *repo_flags] instead of concatenation
Replace with ["gh", "pr", "diff", str(pr_number), *repo_flags]
(RUF005)
python/cube/cli.py
[warning] 577-577: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
python/cube/github/pulls.py (1)
32-39: Repo-scoped PR fetch and full head SHA propagation look good.Line 51, Line 55, and Line 73 consistently apply
--reposcoping, and Line 89 correctly preserves the fullheadRefOidfor downstream commit-accurate operations.Also applies to: 51-57, 73-73, 89-90
python/cube/cli.py (1)
22-23:ui-reviewCLI wiring is clean and matches the command contract.Options are passed through consistently (
--pr,--repo,--post,--output,--fresh), which keeps the CLI surface aligned withui_review_command.Also applies to: 529-575
|
@jacsamell Thanks for the detailed review! All 6 points addressed: 1.
|
2 participants
Summary
Adds a standalone
cube ui-reviewcommand that runs UI/UX reviews through the existing judge panel infrastructure, parallel to the existingpr-reviewandpeer-reviewmodes.What's new
New command:
cube ui-reviewFlags:
--file(repeatable),--description,--pr,--context,--output,--freshHow it works
automation/ui_review_refs/as the knowledge base.prompts/<task-id>.mdlaunch_judge_panel()withreview_type="ui-review"--outputChanges to existing files
cli.pyui-reviewcommand registration (~35 lines)judge_panel.pyui-reviewbranches: skip worktree prefetch, skip writer branch display, exclude cli-review judges, add P0/P1/P2 status parsingNo changes to existing review modes (pr-review, peer-review, panel, writers).
New files
automation/ui_review_refs/— 7 reference docs (principles, system-principles, design-psych, interaction-psychology, checklists, review-template, icons)automation/ui_review_prompts.py— prompt buildercommands/ui_review.py— command logicAdds a new "ui-review" mode and command to run UI/UX reviews using the existing multi-judge judge-panel infrastructure, parallel to pr-review and peer-review.
Key features
Implementation notes
Notes & observations