Skip to content

[TTAHUB-5340] AR vs TR graph#3667

Open
AdamAdHocTeam wants to merge 29 commits into
mainfrom
al-ttahub-5340-ar-vs-tr-graph
Open

[TTAHUB-5340] AR vs TR graph#3667
AdamAdHocTeam wants to merge 29 commits into
mainfrom
al-ttahub-5340-ar-vs-tr-graph

Conversation

@AdamAdHocTeam

@AdamAdHocTeam AdamAdHocTeam commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Description of change

This PR add the 'Approved Activity Reports and Training Session Reports by goal category' graph to the TTA History page. It also adds the table view.

How to test

  • Review the code
  • Make sure we match the design on both the graph and table

Issue(s)

Checklists

Every PR

  • Meets issue criteria
  • JIRA ticket status updated
  • Code is meaningfully tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
  • API Documentation updated
  • Boundary diagram updated
  • Logical Data Model updated
  • Architectural Decision Records written for major infrastructure decisions
  • UI review complete
  • QA review complete

Before merge to main

  • OHS demo complete
  • Ready to create production PR

Production Deploy

  • PR created as Draft
  • Staging smoke test completed
  • PR transitioned to Open (this ready_for_review transition triggers the Slack/Jira automation)
  • Reviewer added after the PR is Open (elainaparrish is the authorized approver under normal circumstances)
    • Sequence: Draft PR → Smoke test → Open PR (automation runs) → Add reviewer
    • Confirm that the Slack notification was sent after the PR was opened
    • Confirm that linked Jira ticket(s) transitioned as expected; if not, review the GitHub Actions workflow logs

After merge/deploy

  • Update JIRA ticket status

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

⚠️ Diff size advisory: This PR is 1589 lines (1369+, 220−), exceeding the 500-line guideline. Consider splitting into smaller changes.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

⚠️ Review count advisory: 1 of 2 required human approvals. 1 more needed. Current approvers: thewatermethod.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new “Approved Activity Reports and Training Session Reports by goal category” widget to the Recipient Record → TTA History page, including both a Plotly horizontal bar chart view and a sortable/exportable table view. On the backend, it adjusts the Training Report (TR) portion of the widget query to scope sessions to recipient grants using sessionReports.data.recipients rather than a join table.

Impact assessment

  • Benefits: Medium — delivers a new analytics view (graph + table + export) directly on TTA History and expands TR scoping logic to match current data shape.
  • Risks: Medium — backend query now embeds a dynamically generated grant ID list into raw SQL literals (potential for very large SQL strings and loss of parameterization); also recipientId.ctn behavior for grants is newly introduced without direct test coverage.

Changes:

  • Adds a new frontend widget (graph + table) for approved AR/TR counts by goal category and wires it into the TTA History page.
  • Updates backend TR counting logic to filter sessions by data.recipients with additional date validation guards.
  • Extends grants scope parsing to support recipientId.ctn and updates table widget styling/behavior to support bold totals and border tweaks.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/widgets/approvedARAndTRByGoalCategory.ts Updates TR query to filter by sessionReports.data.recipients and hardens date parsing.
src/widgets/approvedARAndTRByGoalCategory.test.js Adjusts fixtures for new recipient scoping and adds multiple edge-case TR tests.
src/scopes/grants/index.js Adds recipientId.ctn mapping for grant scoping.
frontend/src/widgets/HorizontalTableWidget.scss Restores bold styling for cells with text-bold inside USWDS tables.
frontend/src/widgets/HorizontalTableWidget.js Allows per-cell classNames to be merged with sticky-column classes.
frontend/src/widgets/BarGraph.css Adds a scoped override for first-column border styling in the new table view.
frontend/src/widgets/approvedARAndTRByGoalCategoryHelpers.js New helper module for chart sorting/layout and table shaping.
frontend/src/widgets/ApprovedARAndTRByGoalCategory.js New widget implementation (graph/table toggle, sorting, export, Plotly rendering).
frontend/src/widgets/tests/ApprovedARAndTRByGoalCategory.js New unit tests covering widget rendering and basic interactions.
frontend/src/pages/RecipientRecord/pages/TTAHistory.js Adds the new widget to the TTA History page layout.

Comment thread src/widgets/approvedARAndTRByGoalCategory.ts Outdated
Comment thread src/widgets/approvedARAndTRByGoalCategory.ts
Comment thread src/scopes/grants/index.js
AdamAdHocTeam and others added 2 commits June 3, 2026 15:30
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@thewatermethod

Copy link
Copy Markdown
Collaborator
Screen.Recording.2026-06-04.at.10.51.30.AM.mov

note the weird interaction on the side label and the bar height when I toggle things on and off

@thewatermethod

Copy link
Copy Markdown
Collaborator

Data seems to be missing - I can see that there is a completed session here: /training-report/R01-PD-25-25011/session/294/session-summary with recipient /recipient-tta-records/965/region/1/tta-history but they have nothing on their chart at all

@thewatermethod

thewatermethod commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator
Screenshot 2026-06-04 at 10 54 09 AM Screenshot 2026-06-04 at 10 54 33 AM

Table headings don't match design

@thewatermethod

Copy link
Copy Markdown
Collaborator

Also, capitalization in dropdown doesn't match design

TABLE_HEADINGS,
WIDGET_HEADINGS,
} from './approvedARAndTRByGoalCategoryHelpers';
import './BarGraph.css';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a need to re-import this file. I would, however, separate out the change you made into it's own file and import that instead.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

const layout = buildPlotlyChartLayout(xRangeMax, width, height);
const bottomAxisLayout = buildPlotlyBottomAxisLayout(xRangeMax, width);

import('plotly.js-basic-dist').then((Plotly) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do import we plotly dynamically? Correct me if I'm wrong, but isn't it already in the bundle?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic import is intentional for bundle splitting. plotly.js-basic-dist is large (~1MB+ minified), so
import('plotly.js-basic-dist') tells the bundler (webpack/craco) to split it into a separate chunk that's only downloaded when this
widget actually renders. Without the dynamic import, Plotly would be included in the main bundle and every user would pay that cost on
initial load, even if they never view this widget.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per Claude

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks claude

recipientId: {
in: (query) => withRecipientId(query),
nin: (query) => withoutRecipientId(query),
ctn: (query) => withRecipientId(query),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious as to why we're creating another filter instead of using the existing 'in'?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page was already using the ctn.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, any thought to just fixing it there? Either is fine with me

data: { status: TRAINING_REPORT_STATUSES.COMPLETE },
[Op.and]: sequelize.literal(`TO_DATE("sessionReports"."data"->>'startDate', 'MM/DD/YYYY') >= '${GOAL_CUTOFF_DATE.toISOString().split('T')[0]}'::date`),
[Op.and]: [
sequelize.literal(`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this for session reports, we didn't collect goal templates on sessions before the cutoff date. I'd remove this and just add a comment saying so

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment thread src/widgets/approvedARAndTRByGoalCategory.ts
return items;
}, [showTabularData, capture, exportRows]);

useEffect(() => {

@thewatermethod thewatermethod Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The robot notes this:


Plotly newPlot never purged — memory leak

File: frontend/src/widgets/ApprovedARAndTRByGoalCategory.js (line ~107–126)

Problem: The useEffect that calls Plotly.newPlot has no cleanup function. Each time the effect re-runs (on data change, resize, filter change, checkbox toggle), Plotly creates a new plot on the same DOM node without destroying the previous one. Additionally, when the component unmounts, the Plotly instance is never released.

// Current — no cleanup:
useEffect(() => {
  if (showTabularData || !data || data.length === 0) return;
  // ...
  import('plotly.js-basic-dist').then((Plotly) => {
    if (chartRef.current) {
      Plotly.newPlot(chartRef.current, traces, layout, { displayModeBar: false });
    }
    // ...
  });
}, [data, sortedDataForChart, showAR, showTR, width, showTabularData]);

Impact: Memory accumulates whenever filters change or the component re-renders. On a long-lived TTA History page session with many filter interactions, this will grow noticeably.

Suggested Fix:

useEffect(() => {
  if (showTabularData || !data || data.length === 0) return;

  let cancelled = false;
  const traces = buildPlotlyTraces(sortedDataForChart, showAR, showTR);
  const { height, xRangeMax } = computeChartDimensions(sortedDataForChart, showAR, showTR);
  const layout = buildPlotlyChartLayout(xRangeMax, width, height);
  const bottomAxisLayout = buildPlotlyBottomAxisLayout(xRangeMax, width);

  import('plotly.js-basic-dist').then((Plotly) => {
    if (cancelled) return;
    if (chartRef.current) {
      Plotly.newPlot(chartRef.current, traces, layout, { displayModeBar: false });
    }
    if (bottomAxisRef.current) {
      Plotly.newPlot(bottomAxisRef.current, [{ mode: 'bar' }], bottomAxisLayout, {
        displayModeBar: false,
        responsive: true,
      });
    }
  });

  return () => {
    cancelled = true;
    // Purge on cleanup to release Plotly memory
    import('plotly.js-basic-dist').then((Plotly) => {
      if (chartRef.current) Plotly.purge(chartRef.current);
      if (bottomAxisRef.current) Plotly.purge(bottomAxisRef.current);
    });
  };
}, [data, sortedDataForChart, showAR, showTR, width, showTabularData]);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updaed.

});
}, [data, sortedDataForChart, showAR, showTR, width, showTabularData]);

const subtitle = (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robot says:

subtitle JSX block recreated on every render

File: frontend/src/widgets/ApprovedARAndTRByGoalCategory.js (line ~131–167)

Problem: const subtitle = (...) is a plain JSX expression defined directly in the component body. It is recreated on every render. Since it is passed as a prop to WidgetContainer, it will also cause WidgetContainer to re-render whenever the parent re-renders (even if subtitle's deps haven't changed).

Suggested Fix: Wrap in useMemo:

const subtitle = useMemo(() => (
  <>
    <WidgetContainerSubtitle>
      Data reflects activity starting on 09/01/2025.
    </WidgetContainerSubtitle>
    <DrawerTriggerButton drawerTriggerRef={drawerTriggerRef} customClass="margin-left-1 margin-bottom-2">
      About this data
    </DrawerTriggerButton>
    <div className="display-flex flex-align-center" data-testid="goal-category-sort-container">
      <Label htmlFor="goal-category-sort" className="margin-y-0 margin-right-1 text-no-wrap">
        Sort by
      </Label>
      <Dropdown
        id="goal-category-sort"
        name="goal-category-sort"
        onChange={(e) => setSortOption(e.target.value)}
        value={sortOption}
        className="margin-top-0 width-auto"
      >
        {SORT_OPTIONS.map((opt) => (
          <option key={opt.value} value={opt.value}>{opt.label}</option>
        ))}
      </Dropdown>
    </div>
  </>
), [sortOption, drawerTriggerRef]);

@AdamAdHocTeam

Copy link
Copy Markdown
Collaborator Author

Data seems to be missing - I can see that there is a completed session here: /training-report/R01-PD-25-25011/session/294/session-summary with recipient /recipient-tta-records/965/region/1/tta-history but they have nothing on their chart at all

There is no template linked to this session report. We bucket based on Goal Template standard.

@AdamAdHocTeam

Copy link
Copy Markdown
Collaborator Author

Screenshot 2026-06-04 at 10 54 09 AM Screenshot 2026-06-04 at 10 54 33 AM
Table headings don't match design

updated both columns and action items.

@thewatermethod

Copy link
Copy Markdown
Collaborator
Screenshot 2026-06-05 at 10 51 52 AM

Alignment off on the sort

@AdamAdHocTeam

Copy link
Copy Markdown
Collaborator Author
Screenshot 2026-06-05 at 10 51 52 AM Alignment off on the sort

This should be fixed now.

@kryswisnaskas kryswisnaskas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding three inline review comments from Codex.

@@ -110,18 +118,25 @@ async function getApprovedTRCountsByCategory(
required: true,
where: {
data: { status: TRAINING_REPORT_STATUSES.COMPLETE },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex said: The TR side no longer enforces the 09/01/2025 session cutoff it still claims to represent. The new sessionReports filter only checks status plus recipient membership from data.recipients; the previous startDate >= 2025-09-01 predicate is gone. That means any complete session with a matching recipient and a goal-template junction will now count even if it is pre-cutoff or has an invalid/missing startDate. The revised tests no longer protect that rule either: the “old session” setup deliberately omits a junction row, so the test passes without exercising the regression. Restore a session-level date predicate here, ideally using the same tolerant date parsing approach already used in sessionReports.ts, and add coverage with actual goal-template junctions for pre-cutoff / malformed-date sessions.

Comment thread frontend/src/widgets/ApprovedARAndTRByGoalCategory.js Outdated
<TargetPopulationsTable filters={filtersToApply} />
</Grid>
</Grid>
<ApprovedARAndTRByGoalCategory filters={filtersToApply} />

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex said: The recipient-page integration tests were not updated for the new widget fetch path, so the endpoint name and filter wiring are effectively untested. TTAHistory now mounts ApprovedARAndTRByGoalCategory, but the recipient-page tests still mock/assert the legacy widget requests only. That leaves regressions in /api/widgets/approvedARAndTRByGoalCategory, recipientId.ctn, and combined role/date filters uncaught. Add fetch mocks and at least one assertion that this widget receives the same page filters as the other TTA History widgets.

@thewatermethod thewatermethod left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending the resolution of Krys's comments

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.

5 participants