Skip to content

fix(dashboard): correct workflow charts and cache overview requests#271

Merged
SantiagoDePolonia merged 2 commits intomainfrom
fix/dashboard-workflow-chart-cache-overview
Apr 25, 2026
Merged

fix(dashboard): correct workflow charts and cache overview requests#271
SantiagoDePolonia merged 2 commits intomainfrom
fix/dashboard-workflow-chart-cache-overview

Conversation

@SantiagoDePolonia
Copy link
Copy Markdown
Contributor

@SantiagoDePolonia SantiagoDePolonia commented Apr 25, 2026

Summary

  • avoid Alpine state shadowing in the shared workflow chart so persisted workflow cards keep Cache and Guardrails nodes
  • centralize cache overview visibility so non-analytics pages do not start unnecessary /cache/overview requests
  • add regression coverage for workflow chart bindings and hidden cache overview requests

Tests

  • node --test internal/admin/dashboard/static/js/modules/*.test.js
  • go test ./internal/admin/dashboard

Summary by CodeRabbit

  • Performance

    • Cache analytics requests are now made only on pages displaying cache analytics cards, reducing unnecessary network calls.
  • Refactor

    • Simplified cache overview loading logic.
    • Updated workflow chart template data structure.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d8fe57ad-8619-4259-967a-fc724b4f7904

📥 Commits

Reviewing files that changed from the base of the PR and between da03630 and 37c7482.

📒 Files selected for processing (6)
  • internal/admin/dashboard/static/js/modules/request-cancellation.test.js
  • internal/admin/dashboard/static/js/modules/usage.js
  • internal/admin/dashboard/static/js/modules/workflows-layout.test.js
  • internal/admin/dashboard/static/js/modules/workflows.js
  • internal/admin/dashboard/static/js/modules/workflows.test.js
  • internal/admin/dashboard/templates/workflow-chart.html

📝 Walkthrough

Walkthrough

The PR adds cache overview visibility gating via a new cacheOverviewVisible() helper method, simplifies workflow runtime config fetch logic, migrates workflow chart template data context from workflow to chart, and introduces tests validating the updated cache fetching and rendering behavior.

Changes

Cohort / File(s) Summary
Cache Overview Visibility Gating
internal/admin/dashboard/static/js/modules/usage.js, internal/admin/dashboard/static/js/modules/workflows.js
Added cacheOverviewVisible() helper to gate cache overview fetching and state resets. Simplified workflows.js to unconditionally invoke fetchCacheOverview() when available, removing prior conditional initialization logic.
Unit Tests
internal/admin/dashboard/static/js/modules/request-cancellation.test.js, internal/admin/dashboard/static/js/modules/workflows.test.js
New tests validating fetchUsage() respects cache analytics card visibility and fetchWorkflowRuntimeConfig() triggers cache overview fetch after successful config load.
Data Context Migration
internal/admin/dashboard/templates/workflow-chart.html, internal/admin/dashboard/static/js/modules/workflows-layout.test.js
Migrated Alpine.js data scope from workflow to chart in template and updated all test assertions (x-data, x-show, :class bindings) to reference new scope.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 The chart scope hops free from workflow's old name,
Cache visibility gates now tame the network flame,
Tests confirm the logic, no cards, no request—
A cleaner dashboard dance puts performance to the test! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and concisely summarizes the main fixes: correcting workflow charts (Alpine state context) and cache overview requests (visibility gating).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dashboard-workflow-chart-cache-overview

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 25, 2026

Greptile Summary

This PR fixes two related bugs: (1) renames the Alpine.js scoped data variable in workflow-chart.html from workflow to chart to prevent state shadowing when persisted workflow cards (with Cache/Guardrails nodes) are embedded inside a parent Alpine component that already owns a workflow variable; (2) centralizes the /cache/overview visibility check inside fetchCacheOverview() via a new cacheOverviewVisible() helper, so pages like workflows and audit-logs never initiate that request. fetchWorkflowRuntimeConfig in workflows.js is simplified to unconditionally delegate to fetchCacheOverview(), which now self-governs via the visibility guard. All three changes are covered by new regression tests.

Confidence Score: 5/5

Safe to merge — all changes are well-scoped bug fixes with targeted regression tests and no P0/P1 findings.

All three fixes are narrow and correct: the template rename is mechanical and fully covered by updated layout tests; the cacheOverviewVisible() guard is correctly applied at the entry point of fetchCacheOverview() so every caller path benefits; the fetchUsagePage() call chain is unaffected because it only executes on the usage page (which is in the visible set). No security concerns, no data loss, no broken contracts.

No files require special attention.

Important Files Changed

Filename Overview
internal/admin/dashboard/templates/workflow-chart.html Renames the Alpine.js local data variable from workflow to chart throughout the template to prevent state shadowing when a parent component exposes a workflow variable.
internal/admin/dashboard/static/js/modules/usage.js Adds cacheOverviewVisible() guard so fetchCacheOverview() and its side-effects (network request, state reset, renderChart) are skipped on pages that don't render cache analytics cards; fetchUsage() updated to match.
internal/admin/dashboard/static/js/modules/workflows.js Simplifies fetchWorkflowRuntimeConfig to always delegate to fetchCacheOverview(), removing the now-redundant inline visibility check and empty-state reset that is handled centrally inside fetchCacheOverview.
internal/admin/dashboard/static/js/modules/workflows.test.js Adds a regression test asserting fetchWorkflowRuntimeConfig always calls fetchCacheOverview once after a successful config load, covering the simplified delegation path.
internal/admin/dashboard/static/js/modules/workflows-layout.test.js Updates all regex assertions to match chart. instead of workflow. after the template rename; adds assert.doesNotMatch for the old x-data="{ workflow:" pattern.
internal/admin/dashboard/static/js/modules/request-cancellation.test.js Adds test verifying that fetchUsage on the workflows page does not issue a third /cache/overview request even when CACHE_ENABLED is on.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[fetchUsage / fetchWorkflowRuntimeConfig] --> B[fetchCacheOverview]
    B --> C{cacheOverviewVisible?\npage === 'overview' OR 'usage'}
    C -- No --> D[return early\nno request, no state reset]
    C -- Yes --> E{cacheAnalyticsEnabled?}
    E -- No --> F[cacheOverview = empty\nrenderChart if overview page]
    E -- Yes --> G[GET /cache/overview\nupdate cacheOverview\nrenderChart if overview page]
Loading

Reviews (1): Last reviewed commit: "fix(dashboard): skip hidden cache overvi..." | Re-trigger Greptile

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@SantiagoDePolonia SantiagoDePolonia merged commit 13a2d07 into main Apr 25, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants