extract issues summary widget into edition partials, rework chart legend - #1638
extract issues summary widget into edition partials, rework chart legend#1638MattBudz wants to merge 27 commits into
Conversation
Moves the full widget markup into projects/issues/summary/_ce.html.erb and makes _summary.html.erb an edition dispatcher so Pro can override with its own _pro.html.erb without causing merge conflicts on this file.
etdsoft
left a comment
There was a problem hiding this comment.
Automated peer review -- PR #1638
Reviewers: Claude (sub-agent 1), Codex (sub-agent 2)
Verdict: comment
Findings: 0 High, 0 Medium, 2 Low
What I checked
This is an automated peer code review by two independent LLM sub-agents reading the same rubric. Findings flagged by both agents are higher confidence; findings flagged by only one are still worth your attention. The verdict is computed from severity counts -- there is no editorial judgment in the merge step.
Both reviewers agreed
(none)
Claude only
F1 [Low|consistency] app/controllers/projects_controller.rb:43-45 -- redundant .to_json on hash values
What: @chart_data pre-serializes @tag_names and @count_by_tag with .to_json before handing the hash to tag.div data: @chart_data in the view.
Why: Rails' tag.div data: helper already JSON-encodes Hash and Array values when emitting data-* attributes — the wire result is identical either way. Pre-stringifying duplicates work the framework already does and risks subtle drift if to_json representation ever diverges from what Rails emits.
How to fix:
@chart_data = {
dimension: 'tags',
tags: @tag_names,
issues_count: @count_by_tag
}The resulting HTML-escaped data-tags / data-issues-count attributes will be identical, and $chartElement.data('tags') / .data('issues-count') in issues_chart.js will still auto-parse them to JS objects.
F2 [Low|hygiene] PR:- -- thin testing steps for a multi-layer refactor
What: Testing steps cover only "render identically to before" on the project dashboard. The PR touches the controller, the dispatcher partial, two new partials, and rewrites the chart JS.
Why: A single render check won't catch a regression in the empty-state branch (no issues), the unassigned bucket, or the chart's tag/count mapping. For a refactor whose claim is "no behavior change," exercising the branches that could silently change is the point of the testing steps.
How to fix: Add steps covering (a) a project with multiple tagged issues, (b) a project with only unassigned issues, (c) a project with no issues (empty state), and confirm chart bar colors/counts and accordion ordering match the pre-refactor build.
Codex only
(none)
Notes
- Codex ran in a sandboxed environment without GitHub API access; it reviewed the checked-out branch against
origin/developin full but could not perform PR metadata/hygiene checks. Its zero-findings conclusion is based on the local code read. _tags_accordion.html.erbcarries forward two pre-existing patterns from the old_summary.html.erb: inlinestyle="..."for dynamic tag colors (unavoidable — colors are user-configurable) andid="collapse<%= tag.display_name %>"(fragile if a display_name contains spaces,#, or?). Neither introduced by this PR.- The old partial had a typo
collape<...>on both href and id; the new partial spells itcollapse<...>consistently. Incidental cleanup, invisible to users. dimension: 'tags'in@chart_datais unused by the CE chart JS. Reads as deliberate scaffolding for a future Pro partial dispatch key.
Reviewed automatically. Raw outputs in the shared product reviews directory (see TOOLS.md).
| end | ||
| end | ||
|
|
||
| @chart_data = { |
There was a problem hiding this comment.
This breaks parity with pro since it isn't used there in the same controller. As a compromise, I'm fine with moving this definition in the CE view if we add a comment.
There was a problem hiding this comment.
Went a step further than the view-move compromise, extracted the tag-grouping logic (including @chart_data) into a shared IssuesGrouping concern, matching how Pro already needs to call it from more than one controller. ProjectsController#show now just calls build_all_tags_grouping. This should make the sync cleaner since Pro's version of the concern can now land as an addition to this file rather than introducing it from scratch.
for..in leaks the loop variable into the surrounding method scope, which each avoids.
Pro needs this same grouping logic from more than one controller, so it already lives in a concern there. Duplicating it inline here meant ProjectsController#show diverged structurally between the two repos. Extracting it in CE keeps both controllers aligned and lets Pro's version of the concern sync in as an addition rather than a rewrite.
…issues-summary-edition-partial
| @@ -0,0 +1,30 @@ | |||
| module IssuesGrouping | |||
There was a problem hiding this comment.
IssuesGrouping sounds a bit vague. Grouping for what? I believe this concern is building the chart data and the name should reflect that.
There was a problem hiding this comment.
Renamed it to IssuesDimensionGrouping. It's not just building @chart_data, it also builds the accordion groupings (@issues_by_tag in CE, plus @issues_by_list_value in Pro) and, in Pro, enumerates the available list fields for the dimension dropdown. The name attempts to cover all three, not just the chart.
The concern builds chart and accordion data grouped by a selectable dimension (tags today, RTP list fields in Pro), so "grouping" alone undersold what it does.
Foundation for lazy-loading the "Issues so far" dashboard widget, so ProjectsController#show doesn't need to eagerly compute the tag grouping just to embed it synchronously.
Extracts the chart/accordion markup into a shared summary_content partial, rendered as the actual frame body by issues_summary/show. The CE edition partial becomes a shell that fetches this content via turbo_frame_tag's src instead of embedding it, matching the header staying visible even with no issues (previously the whole widget, header included, was replaced by the empty state).
The dashboard widget now lazy-loads its content through Projects::IssuesSummaryController, so computing the same grouping here just to embed it synchronously was wasted, duplicated work.
Blocking rubocop-ci for this branch's changed-file set.
The widget now lazy-loads via a turbo frame, so #issue-chart never exists at turbo:load time. Without this, the chart never rendered.
Request specs alone wouldn't have caught the chart never rendering, since that regression only showed up in the browser (turbo:frame-load wasn't wired up to initialize the chart).
The accordion now renders inside the async-loaded issues-summary frame, so clicking an issue link navigated within that small frame instead of the full page. Added a spec covering the click-through, since the existing chart-rendering spec didn't exercise this path.
Drops the manual { unassigned: 0 } seed hash and the per-tag = 0
pre-seed loop. The chart JS already falls back to 0 for any tag
missing from issues_count, so the pre-seed was never load-bearing.
Builds the fill colors alongside the rest of the chart data in one pass, instead of re-deriving them from tags a second time in the coloring step. No behavior change.
padding to a fixed column breaks as soon as a variable name changes length; plain single-space declarations don't drift
filter() and find() built CSS attribute selectors by string-interpolating option values directly. A value containing a double quote breaks the selector (Sizzle syntax error), leaving the combobox showing a stale label even though the underlying select's value updated correctly. Match by exact attribute value instead, which can't be broken by selector metacharacters.
…-edition-partial' into dashboard/extract-issues-summary-edition-partial
Summary
Moves the full issues summary widget markup out of
_summary.html.erband intoprojects/issues/summary/_ce.html.erb, making_summary.html.erban edition dispatcher:This follows the same pattern used by the navbar (
layouts/hera/navbar/_main_nav.html.erb) so that Pro can provide its own_pro.html.erbwithout_summary.html.erbdiverging between repos and causing merge conflicts on every ce-sync.Building on that, the widget now lazy-loads its content instead of
ProjectsController#showeagerly computing the tag grouping just to embed it synchronously:IssuesGroupingis renamed toIssuesDimensionGrouping, since it builds more than a plain grouping (chart data, accordion data, and the available dimensions).Projects::IssuesSummaryController#show(routeresource :issues_summary) owns the tag-grouping computation.projects/issues/_summary_contentpartial, rendered as the actual frame body byissues_summary/show.html.erb._ce.html.erbbecomes a shell: the header plus aturbo_frame_tagwithsrc:pointing at the new controller, instead of embedding the content.The header now always renders, even with no issues (previously the whole widget, header included, was replaced by the empty state).
Also includes a fix for the shared
ComboBoxmodule (app/assets/javascripts/hera/modules/combobox.js):filter()andfind()built CSS attribute selectors by interpolating option values directly into a template string. A value containing a double quote breaks the selector with a Sizzle syntax error, leaving the combobox showing a stale label even though the underlying select's value updated correctly. Matching is now done by exact attribute value instead of a raw selector, which can't be broken by selector metacharacters.Finally, reworks the "Issues so far" bar chart's legend. Tag names used to render as x-axis tick labels directly under each bar, which overlapped and became unreadable once a name was longer than a few characters. The legend is now a separate list below the chart that wraps horizontally, so each name gets a full row's width instead of a single bar's width, and the chart itself stays full size. The chart container is now selected via
data-behavior="issue-chart"instead of itsid, per the JS element-selection convention.Testing steps
/styles, open the "Multi-select & User-defined Option Example" combobox, type a value containing a double quote (e.g.Weird"Value) and add it as a custom option. Select it and confirm no console error, and the combobox label and selected tag both display correctly.Check List
Added a CHANGELOG entry(internal refactor, no user-facing feature change)