refactor: Break up scorecards.py — two functions over 100 lines each
Extract shared logic into helpers: - _parse_period(): Period parsing with fallback - _format_token_display(): Token formatting with +/- - _format_token_class(): CSS class based on token sign - _build_patterns_html(): Pattern section HTML - _build_narrative_html(): Narrative bullets HTML - _build_metrics_row_html(): Metrics row HTML - _render_scorecard_panel(): Single panel renderer - _render_empty_scorecard(): Empty state renderer - _render_error_scorecard(): Error state renderer - _render_single_panel_wrapper(): Single panel wrapper - _render_all_panels_grid(): Grid layout for all panels Before: - agent_scorecard_panel(): 108 lines - all_scorecard_panels(): 106 lines After: - agent_scorecard_panel(): 26 lines - all_scorecard_panels(): 26 lines All helpers have docstrings. All 49 tests pass. Fixes #1127
This commit is contained in:
@@ -10,6 +10,7 @@ from fastapi.responses import HTMLResponse, JSONResponse
|
||||
|
||||
from dashboard.services.scorecard_service import (
|
||||
PeriodType,
|
||||
ScorecardSummary,
|
||||
generate_all_scorecards,
|
||||
generate_scorecard,
|
||||
get_tracked_agents,
|
||||
@@ -26,6 +27,216 @@ def _format_period_label(period_type: PeriodType) -> str:
|
||||
return "Daily" if period_type == PeriodType.daily else "Weekly"
|
||||
|
||||
|
||||
def _parse_period(period: str) -> PeriodType:
|
||||
"""Parse period string into PeriodType, defaulting to daily on invalid input.
|
||||
|
||||
Args:
|
||||
period: The period string ('daily' or 'weekly')
|
||||
|
||||
Returns:
|
||||
PeriodType.daily or PeriodType.weekly
|
||||
"""
|
||||
try:
|
||||
return PeriodType(period.lower())
|
||||
except ValueError:
|
||||
return PeriodType.daily
|
||||
|
||||
|
||||
def _format_token_display(token_net: int) -> str:
|
||||
"""Format token net value with +/- prefix for display.
|
||||
|
||||
Args:
|
||||
token_net: The net token value
|
||||
|
||||
Returns:
|
||||
Formatted string with + prefix for positive values
|
||||
"""
|
||||
return f"{'+' if token_net > 0 else ''}{token_net}"
|
||||
|
||||
|
||||
def _format_token_class(token_net: int) -> str:
|
||||
"""Get CSS class for token net value based on sign.
|
||||
|
||||
Args:
|
||||
token_net: The net token value
|
||||
|
||||
Returns:
|
||||
'text-success' for positive/zero, 'text-danger' for negative
|
||||
"""
|
||||
return "text-success" if token_net >= 0 else "text-danger"
|
||||
|
||||
|
||||
def _build_patterns_html(patterns: list[str]) -> str:
|
||||
"""Build HTML for patterns section if patterns exist.
|
||||
|
||||
Args:
|
||||
patterns: List of pattern strings
|
||||
|
||||
Returns:
|
||||
HTML string for patterns section or empty string
|
||||
"""
|
||||
if not patterns:
|
||||
return ""
|
||||
|
||||
patterns_list = "".join([f"<li>{p}</li>" for p in patterns])
|
||||
return f"""
|
||||
<div class="mt-3">
|
||||
<h6>Patterns</h6>
|
||||
<ul class="list-unstyled text-info">
|
||||
{patterns_list}
|
||||
</ul>
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
def _build_narrative_html(bullets: list[str]) -> str:
|
||||
"""Build HTML for narrative bullets.
|
||||
|
||||
Args:
|
||||
bullets: List of narrative bullet strings
|
||||
|
||||
Returns:
|
||||
HTML string with list items
|
||||
"""
|
||||
return "".join([f"<li>{b}</li>" for b in bullets])
|
||||
|
||||
|
||||
def _build_metrics_row_html(metrics: dict) -> str:
|
||||
"""Build HTML for the metrics summary row.
|
||||
|
||||
Args:
|
||||
metrics: Dictionary with PRs, issues, tests, and token metrics
|
||||
|
||||
Returns:
|
||||
HTML string for the metrics row
|
||||
"""
|
||||
prs_opened = metrics["prs_opened"]
|
||||
prs_merged = metrics["prs_merged"]
|
||||
pr_merge_rate = int(metrics["pr_merge_rate"] * 100)
|
||||
issues_touched = metrics["issues_touched"]
|
||||
tests_affected = metrics["tests_affected"]
|
||||
token_net = metrics["token_net"]
|
||||
|
||||
token_class = _format_token_class(token_net)
|
||||
token_display = _format_token_display(token_net)
|
||||
|
||||
return f"""
|
||||
<div class="row text-center small">
|
||||
<div class="col">
|
||||
<div class="text-muted">PRs</div>
|
||||
<div class="fw-bold">{prs_opened}/{prs_merged}</div>
|
||||
<div class="text-muted" style="font-size: 0.75rem;">
|
||||
{pr_merge_rate}% merged
|
||||
</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Issues</div>
|
||||
<div class="fw-bold">{issues_touched}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tests</div>
|
||||
<div class="fw-bold">{tests_affected}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tokens</div>
|
||||
<div class="fw-bold {token_class}">{token_display}</div>
|
||||
</div>
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
def _render_scorecard_panel(
|
||||
agent_id: str,
|
||||
period_type: PeriodType,
|
||||
data: dict,
|
||||
) -> str:
|
||||
"""Render HTML for a single scorecard panel.
|
||||
|
||||
Args:
|
||||
agent_id: The agent ID
|
||||
period_type: Daily or weekly period
|
||||
data: Scorecard data dictionary with metrics, patterns, narrative_bullets
|
||||
|
||||
Returns:
|
||||
HTML string for the scorecard panel
|
||||
"""
|
||||
patterns_html = _build_patterns_html(data.get("patterns", []))
|
||||
bullets_html = _build_narrative_html(data.get("narrative_bullets", []))
|
||||
metrics_row = _build_metrics_row_html(data["metrics"])
|
||||
|
||||
return f"""
|
||||
<div class="card mc-panel">
|
||||
<div class="card-header d-flex justify-content-between align-items-center">
|
||||
<h5 class="card-title mb-0">{agent_id.title()}</h5>
|
||||
<span class="badge bg-secondary">{_format_period_label(period_type)}</span>
|
||||
</div>
|
||||
<div class="card-body">
|
||||
<ul class="list-unstyled mb-3">
|
||||
{bullets_html}
|
||||
</ul>
|
||||
{metrics_row}
|
||||
{patterns_html}
|
||||
</div>
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
def _render_empty_scorecard(agent_id: str) -> str:
|
||||
"""Render HTML for an empty scorecard (no activity).
|
||||
|
||||
Args:
|
||||
agent_id: The agent ID
|
||||
|
||||
Returns:
|
||||
HTML string for the empty scorecard panel
|
||||
"""
|
||||
return f"""
|
||||
<div class="card mc-panel">
|
||||
<h5 class="card-title">{agent_id.title()}</h5>
|
||||
<p class="text-muted">No activity recorded for this period.</p>
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
def _render_error_scorecard(agent_id: str, error: str) -> str:
|
||||
"""Render HTML for a scorecard that failed to load.
|
||||
|
||||
Args:
|
||||
agent_id: The agent ID
|
||||
error: Error message string
|
||||
|
||||
Returns:
|
||||
HTML string for the error scorecard panel
|
||||
"""
|
||||
return f"""
|
||||
<div class="card mc-panel border-danger">
|
||||
<h5 class="card-title">{agent_id.title()}</h5>
|
||||
<p class="text-danger">Error loading scorecard: {error}</p>
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
def _render_single_panel_wrapper(
|
||||
agent_id: str,
|
||||
period_type: PeriodType,
|
||||
scorecard: ScorecardSummary | None,
|
||||
) -> str:
|
||||
"""Render a complete scorecard panel with wrapper div for single panel view.
|
||||
|
||||
Args:
|
||||
agent_id: The agent ID
|
||||
period_type: Daily or weekly period
|
||||
scorecard: ScorecardSummary object or None
|
||||
|
||||
Returns:
|
||||
HTML string for the complete panel
|
||||
"""
|
||||
if scorecard is None:
|
||||
return _render_empty_scorecard(agent_id)
|
||||
|
||||
return _render_scorecard_panel(agent_id, period_type, scorecard.to_dict())
|
||||
|
||||
|
||||
@router.get("/api/agents")
|
||||
async def list_tracked_agents() -> dict[str, list[str]]:
|
||||
"""Return the list of tracked agent IDs.
|
||||
@@ -149,99 +360,50 @@ async def agent_scorecard_panel(
|
||||
Returns:
|
||||
HTML panel with scorecard content
|
||||
"""
|
||||
try:
|
||||
period_type = PeriodType(period.lower())
|
||||
except ValueError:
|
||||
period_type = PeriodType.daily
|
||||
period_type = _parse_period(period)
|
||||
|
||||
try:
|
||||
scorecard = generate_scorecard(agent_id, period_type)
|
||||
|
||||
if scorecard is None:
|
||||
return HTMLResponse(
|
||||
content=f"""
|
||||
<div class="card mc-panel">
|
||||
<h5 class="card-title">{agent_id.title()}</h5>
|
||||
<p class="text-muted">No activity recorded for this period.</p>
|
||||
</div>
|
||||
""",
|
||||
status_code=200,
|
||||
)
|
||||
|
||||
data = scorecard.to_dict()
|
||||
|
||||
# Build patterns HTML
|
||||
patterns_html = ""
|
||||
if data["patterns"]:
|
||||
patterns_list = "".join([f"<li>{p}</li>" for p in data["patterns"]])
|
||||
patterns_html = f"""
|
||||
<div class="mt-3">
|
||||
<h6>Patterns</h6>
|
||||
<ul class="list-unstyled text-info">
|
||||
{patterns_list}
|
||||
</ul>
|
||||
</div>
|
||||
"""
|
||||
|
||||
# Build bullets HTML
|
||||
bullets_html = "".join([f"<li>{b}</li>" for b in data["narrative_bullets"]])
|
||||
|
||||
# Build metrics summary
|
||||
metrics = data["metrics"]
|
||||
|
||||
html_content = f"""
|
||||
<div class="card mc-panel">
|
||||
<div class="card-header d-flex justify-content-between align-items-center">
|
||||
<h5 class="card-title mb-0">{agent_id.title()}</h5>
|
||||
<span class="badge bg-secondary">{_format_period_label(period_type)}</span>
|
||||
</div>
|
||||
<div class="card-body">
|
||||
<ul class="list-unstyled mb-3">
|
||||
{bullets_html}
|
||||
</ul>
|
||||
|
||||
<div class="row text-center small">
|
||||
<div class="col">
|
||||
<div class="text-muted">PRs</div>
|
||||
<div class="fw-bold">{metrics["prs_opened"]}/{metrics["prs_merged"]}</div>
|
||||
<div class="text-muted" style="font-size: 0.75rem;">
|
||||
{int(metrics["pr_merge_rate"] * 100)}% merged
|
||||
</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Issues</div>
|
||||
<div class="fw-bold">{metrics["issues_touched"]}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tests</div>
|
||||
<div class="fw-bold">{metrics["tests_affected"]}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tokens</div>
|
||||
<div class="fw-bold {"text-success" if metrics["token_net"] >= 0 else "text-danger"}">
|
||||
{"+" if metrics["token_net"] > 0 else ""}{metrics["token_net"]}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{patterns_html}
|
||||
</div>
|
||||
</div>
|
||||
"""
|
||||
|
||||
html_content = _render_single_panel_wrapper(agent_id, period_type, scorecard)
|
||||
return HTMLResponse(content=html_content)
|
||||
|
||||
except Exception as exc:
|
||||
logger.error("Failed to render scorecard panel for %s: %s", agent_id, exc)
|
||||
return HTMLResponse(
|
||||
content=f"""
|
||||
<div class="card mc-panel border-danger">
|
||||
<h5 class="card-title">{agent_id.title()}</h5>
|
||||
<p class="text-danger">Error loading scorecard: {str(exc)}</p>
|
||||
</div>
|
||||
""",
|
||||
status_code=200,
|
||||
return HTMLResponse(content=_render_error_scorecard(agent_id, str(exc)))
|
||||
|
||||
|
||||
def _render_all_panels_grid(
|
||||
scorecards: list[ScorecardSummary],
|
||||
period_type: PeriodType,
|
||||
) -> str:
|
||||
"""Render all scorecard panels in a grid layout.
|
||||
|
||||
Args:
|
||||
scorecards: List of scorecard summaries
|
||||
period_type: Daily or weekly period
|
||||
|
||||
Returns:
|
||||
HTML string with all panels in a grid
|
||||
"""
|
||||
panels: list[str] = []
|
||||
for scorecard in scorecards:
|
||||
panel_html = _render_scorecard_panel(
|
||||
scorecard.agent_id,
|
||||
period_type,
|
||||
scorecard.to_dict(),
|
||||
)
|
||||
# Wrap each panel in a grid column
|
||||
wrapped = f'<div class="col-md-6 col-lg-4 mb-3">{panel_html}</div>'
|
||||
panels.append(wrapped)
|
||||
|
||||
return f"""
|
||||
<div class="row">
|
||||
{"".join(panels)}
|
||||
</div>
|
||||
<div class="text-muted small mt-2">
|
||||
Generated: {datetime.now().strftime("%Y-%m-%d %H:%M:%S UTC")}
|
||||
</div>
|
||||
"""
|
||||
|
||||
|
||||
@router.get("/all/panels", response_class=HTMLResponse)
|
||||
@@ -258,96 +420,15 @@ async def all_scorecard_panels(
|
||||
Returns:
|
||||
HTML with all scorecard panels
|
||||
"""
|
||||
try:
|
||||
period_type = PeriodType(period.lower())
|
||||
except ValueError:
|
||||
period_type = PeriodType.daily
|
||||
period_type = _parse_period(period)
|
||||
|
||||
try:
|
||||
scorecards = generate_all_scorecards(period_type)
|
||||
|
||||
panels: list[str] = []
|
||||
for scorecard in scorecards:
|
||||
data = scorecard.to_dict()
|
||||
|
||||
# Build patterns HTML
|
||||
patterns_html = ""
|
||||
if data["patterns"]:
|
||||
patterns_list = "".join([f"<li>{p}</li>" for p in data["patterns"]])
|
||||
patterns_html = f"""
|
||||
<div class="mt-3">
|
||||
<h6>Patterns</h6>
|
||||
<ul class="list-unstyled text-info">
|
||||
{patterns_list}
|
||||
</ul>
|
||||
</div>
|
||||
"""
|
||||
|
||||
# Build bullets HTML
|
||||
bullets_html = "".join([f"<li>{b}</li>" for b in data["narrative_bullets"]])
|
||||
metrics = data["metrics"]
|
||||
|
||||
panel_html = f"""
|
||||
<div class="col-md-6 col-lg-4 mb-3">
|
||||
<div class="card mc-panel">
|
||||
<div class="card-header d-flex justify-content-between align-items-center">
|
||||
<h5 class="card-title mb-0">{scorecard.agent_id.title()}</h5>
|
||||
<span class="badge bg-secondary">{_format_period_label(period_type)}</span>
|
||||
</div>
|
||||
<div class="card-body">
|
||||
<ul class="list-unstyled mb-3">
|
||||
{bullets_html}
|
||||
</ul>
|
||||
|
||||
<div class="row text-center small">
|
||||
<div class="col">
|
||||
<div class="text-muted">PRs</div>
|
||||
<div class="fw-bold">{metrics["prs_opened"]}/{metrics["prs_merged"]}</div>
|
||||
<div class="text-muted" style="font-size: 0.75rem;">
|
||||
{int(metrics["pr_merge_rate"] * 100)}% merged
|
||||
</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Issues</div>
|
||||
<div class="fw-bold">{metrics["issues_touched"]}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tests</div>
|
||||
<div class="fw-bold">{metrics["tests_affected"]}</div>
|
||||
</div>
|
||||
<div class="col">
|
||||
<div class="text-muted">Tokens</div>
|
||||
<div class="fw-bold {"text-success" if metrics["token_net"] >= 0 else "text-danger"}">
|
||||
{"+" if metrics["token_net"] > 0 else ""}{metrics["token_net"]}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
{patterns_html}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
"""
|
||||
panels.append(panel_html)
|
||||
|
||||
html_content = f"""
|
||||
<div class="row">
|
||||
{"".join(panels)}
|
||||
</div>
|
||||
<div class="text-muted small mt-2">
|
||||
Generated: {datetime.now().strftime("%Y-%m-%d %H:%M:%S UTC")}
|
||||
</div>
|
||||
"""
|
||||
|
||||
html_content = _render_all_panels_grid(scorecards, period_type)
|
||||
return HTMLResponse(content=html_content)
|
||||
|
||||
except Exception as exc:
|
||||
logger.error("Failed to render all scorecard panels: %s", exc)
|
||||
return HTMLResponse(
|
||||
content=f"""
|
||||
<div class="alert alert-danger">
|
||||
Error loading scorecards: {str(exc)}
|
||||
</div>
|
||||
""",
|
||||
status_code=200,
|
||||
content=f'<div class="alert alert-danger">Error loading scorecards: {exc}</div>'
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user