feat(sponsor-reports): port Sponsor Reports FE into Summit Admin (Sponsors → Reports)#997
feat(sponsor-reports): port Sponsor Reports FE into Summit Admin (Sponsors → Reports)#997caseylocker wants to merge 83 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds sponsor reports configuration, Redux wiring, shared report UI, routes, and two sponsor report pages with supporting utilities and tests. ChangesSponsor reports feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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 (3)
src/pages/sponsors/sponsor-reports/__tests__/routing.test.js (1)
33-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest replicates route ordering instead of asserting on the real layout.
Because
renderSwitchhand-copies the<Switch>table rather than mountingsponsor-reports-layout.js, it validates React Router's behavior, not the production ordering. If the real layout's route order drifts, this test won't catch it. The drilldown-vs-landing ordering is already covered against the real component insponsor-reports-layout.test.js, so this can either be removed or driven through the actual layout to guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-reports/__tests__/routing.test.js` around lines 33 - 65, This test is duplicating the route table instead of exercising the real sponsor-reports layout, so it won’t catch regressions in the production ordering. Update the routing coverage to mount sponsor-reports-layout.js directly (using the existing SponsorReportsLayout or equivalent wrapper) instead of the local renderSwitch Switch copy, or remove this redundant spec if sponsor-reports-layout.test.js already covers the drilldown-vs-list ordering.src/components/sponsors/reports/FilterBar.js (1)
28-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
draftwon't re-sync if thevalueprop changes after mount.
useState(value)only seeds the initial state; later changes to thevalueprop (e.g. an external filter reset) won't be reflected indraft. Current flows keep parent/child in sync manually (Apply commits the draft, Clear resets both to{}), so there's no active bug, but this coupling is fragile ifvalueever becomes externally driven. Consider syncing via an effect keyed onvalueif that scenario is anticipated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/sponsors/reports/FilterBar.js` around lines 28 - 29, The local draft state in FilterBar is only initialized from the value prop and will not update if that prop changes later. If external changes to value are expected, add synchronization in FilterBar using an effect keyed on value so setDraft stays aligned; keep the update helper unchanged and make sure the draft state refreshes whenever the incoming value object changes.src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js (1)
110-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
PURCHASE_DETAILS_VALIDATION_CLEAR.This file validates the set path for
validationError, but not the clear action branch; add one test to prevent regressions in inline/toast reset flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js` around lines 110 - 124, Add a test for the PURCHASE_DETAILS_VALIDATION_CLEAR branch in purchaseDetailsReducer to cover the reset path for validationError and loading state. Use the existing PURCHASE_DETAILS_VALIDATION_ERROR test in sponsor-reports-reducers.test.js as a guide, but assert that dispatching PURCHASE_DETAILS_VALIDATION_CLEAR clears validationError without replacing the current data/body so the inline/toast reset flow stays protected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/actions/sponsor-reports-actions.js`:
- Around line 34-54: Add an early guard in getPurchaseDetailsReport and
getPurchaseDetailsFilters to check that currentSummit and currentSummit.id exist
before calling dispatch(startLoading()) or base(currentSummit.id). If the summit
is missing, exit the thunk immediately (optionally dispatch a validation/error
action) so the synchronous throw never happens and stopLoading() is not skipped.
Keep the fix localized in sponsor-reports-actions.js and use the existing thunk
names to update both purchase-details actions consistently.
In `@src/utils/reports-money.js`:
- Around line 10-13: Update formatUsd in reports-money.js so blank or
whitespace-only strings are treated as missing values instead of being converted
by Number(). Add an explicit check before parsing string input (for example in
the formatUsd path) to return "—" when the value is empty after trimming, while
keeping the existing USD.format behavior for valid numeric values.
---
Nitpick comments:
In `@src/components/sponsors/reports/FilterBar.js`:
- Around line 28-29: The local draft state in FilterBar is only initialized from
the value prop and will not update if that prop changes later. If external
changes to value are expected, add synchronization in FilterBar using an effect
keyed on value so setDraft stays aligned; keep the update helper unchanged and
make sure the draft state refreshes whenever the incoming value object changes.
In `@src/pages/sponsors/sponsor-reports/__tests__/routing.test.js`:
- Around line 33-65: This test is duplicating the route table instead of
exercising the real sponsor-reports layout, so it won’t catch regressions in the
production ordering. Update the routing coverage to mount
sponsor-reports-layout.js directly (using the existing SponsorReportsLayout or
equivalent wrapper) instead of the local renderSwitch Switch copy, or remove
this redundant spec if sponsor-reports-layout.test.js already covers the
drilldown-vs-list ordering.
In `@src/reducers/sponsors/__tests__/sponsor-reports-reducers.test.js`:
- Around line 110-124: Add a test for the PURCHASE_DETAILS_VALIDATION_CLEAR
branch in purchaseDetailsReducer to cover the reset path for validationError and
loading state. Use the existing PURCHASE_DETAILS_VALIDATION_ERROR test in
sponsor-reports-reducers.test.js as a guide, but assert that dispatching
PURCHASE_DETAILS_VALIDATION_CLEAR clears validationError without replacing the
current data/body so the inline/toast reset flow stays protected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 878ce767-9e7a-43b8-97cf-abd302c54c9b
📒 Files selected for processing (57)
.env.examplesrc/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/app.jssrc/components/menu/menu-definition.jssrc/components/sponsors/reports/ExportCsvButton.jssrc/components/sponsors/reports/FilterBar.jssrc/components/sponsors/reports/GroupByComponentView.jssrc/components/sponsors/reports/GroupBySponsorView.jssrc/components/sponsors/reports/GroupByToggle.jssrc/components/sponsors/reports/OrdersTable.jssrc/components/sponsors/reports/ReportShell.jssrc/components/sponsors/reports/StatusPill.jssrc/components/sponsors/reports/StatusRollupChips.jssrc/components/sponsors/reports/SummaryPanel.jssrc/components/sponsors/reports/TierBadge.jssrc/components/sponsors/reports/__tests__/ExportCsvButton.test.jssrc/components/sponsors/reports/__tests__/FilterBar.test.jssrc/components/sponsors/reports/__tests__/GroupByComponentView.test.jssrc/components/sponsors/reports/__tests__/GroupBySponsorView.test.jssrc/components/sponsors/reports/__tests__/GroupByToggle.test.jssrc/components/sponsors/reports/__tests__/OrdersTable.test.jssrc/components/sponsors/reports/__tests__/ReportShell.test.jssrc/components/sponsors/reports/__tests__/StatusPill.test.jssrc/components/sponsors/reports/__tests__/StatusRollupChips.test.jssrc/components/sponsors/reports/__tests__/SummaryPanel.test.jssrc/components/sponsors/reports/__tests__/TierBadge.test.jssrc/components/sponsors/reports/__tests__/statusTone.test.jssrc/components/sponsors/reports/__tests__/usePrint.test.jssrc/components/sponsors/reports/report-print.csssrc/components/sponsors/reports/statusTone.jssrc/components/sponsors/reports/usePrint.jssrc/i18n/en.jsonsrc/layouts/__tests__/sponsor-reports-layout.test.jssrc/layouts/sponsor-layout.jssrc/layouts/sponsor-reports-layout.jssrc/pages/sponsors/sponsor-reports/__tests__/routing.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.jssrc/pages/sponsors/sponsor-reports/reports-landing-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/reports-landing-page/index.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/index.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/sponsor-asset-report-page/index.jssrc/reducers/sponsors/__tests__/sponsor-reports-reducers.test.jssrc/reducers/sponsors/sponsor-reports-drilldown-reducer.jssrc/reducers/sponsors/sponsor-reports-purchase-details-reducer.jssrc/reducers/sponsors/sponsor-reports-sponsor-asset-reducer.jssrc/store.jssrc/utils/constants.jssrc/utils/report-errors.jssrc/utils/report-query.jssrc/utils/reports-api.jssrc/utils/reports-money.jssrc/utils/reports-text.jssrc/utils/section-csv-query.js
- guard all report thunks against a missing currentSummit (prevents a stuck spinner from base(currentSummit.id) throwing after startLoading) - formatUsd: treat blank/whitespace strings + non-finite numbers as missing (em dash) instead of $0.00; add reports-money tests - FilterBar: re-sync draft when the committed value prop changes externally - drop redundant routing.test.js (real-layout integration test in sponsor-reports-layout.test.js already covers route ordering) - add PURCHASE_DETAILS_VALIDATION_CLEAR reducer test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Ports the Sponsor Reports frontend (Purchase Details + Sponsor Assets, including per-sponsor drill-down) into summit-admin under Sponsors → Reports, using the existing admin-sponsors access gate and the in-context summit selection.
Changes:
- Adds a new Sponsor Reports layout + routes (landing, purchase details, sponsor assets, drill-down) and wires it into the Sponsors menu.
- Implements report UI building blocks (report shell, filters, grouping views, CSV export, print) plus supporting query/text/money utilities.
- Introduces Redux actions/reducers for sponsor reports and adds extensive unit/integration test coverage + i18n strings.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/section-csv-query.js | Builds section-scoped CSV export query while preserving unrelated query params. |
| src/utils/reports-text.js | HTML-to-plain-text helper for report text fields. |
| src/utils/reports-money.js | USD formatting helper for report summary/rows. |
| src/utils/reports-api.js | Reports API base URL helper + positive-int route ID validator. |
| src/utils/report-query.js | Translates report filter UI state into base-api-utils query params. |
| src/utils/report-errors.js | Normalizes/classifies report API errors and provides a getRequest-shaped handler factory. |
| src/utils/constants.js | Adds HTTP 503 constant used by report error classification. |
| src/utils/tests/reports-money.test.js | Unit tests for USD formatting helper. |
| src/store.js | Registers sponsor reports reducers and excludes their state from persistence. |
| src/reducers/sponsors/sponsor-reports-sponsor-asset-reducer.js | Sponsor Assets grouped report reducer. |
| src/reducers/sponsors/sponsor-reports-purchase-details-reducer.js | Purchase Details report reducer (incl. validation vs read-error split). |
| src/reducers/sponsors/sponsor-reports-drilldown-reducer.js | Sponsor Assets drill-down reducer. |
| src/reducers/sponsors/tests/sponsor-reports-reducers.test.js | Reducer unit tests covering all sponsor reports reducers. |
| src/pages/sponsors/sponsor-reports/sponsor-asset-report-page/index.js | Sponsor Assets grouped report page (group-by, filters, pagination, CSV/print). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-report-page/tests/index.test.js | Sponsor Assets report page tests (dispatching, toggles, states, CSV props). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/index.js | Per-sponsor drill-down page (section cards, content rendering, per-section CSV). |
| src/pages/sponsors/sponsor-reports/sponsor-asset-drilldown-page/tests/index.test.js | Drill-down page tests including ContentCell behaviors. |
| src/pages/sponsors/sponsor-reports/reports-landing-page/index.js | Reports landing page with two navigation cards. |
| src/pages/sponsors/sponsor-reports/reports-landing-page/tests/index.test.js | Landing page tests for cards and links. |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js | Purchase Details report page (filters, sorting, pagination, CSV/print, tiles). |
| src/pages/sponsors/sponsor-reports/purchase-details-report-page/tests/index.test.js | Purchase Details page tests including filter->query behavior and CSV dispatch. |
| src/layouts/sponsor-reports-layout.js | Sponsor Reports sub-router + breadcrumb composition + admin gate. |
| src/layouts/sponsor-layout.js | Adds /reports route under Sponsors layout. |
| src/layouts/tests/sponsor-reports-layout.test.js | Layout routing + Restrict gate integration tests (landing, unauthorized, drilldown, crumbs). |
| src/i18n/en.json | Adds menu label + Sponsor Reports page strings. |
| src/components/sponsors/reports/usePrint.js | Hook wrapper around window.print() for reports. |
| src/components/sponsors/reports/TierBadge.js | Tier badge chip with explicit tier color mapping. |
| src/components/sponsors/reports/SummaryPanel.js | Shared summary tiles panel component. |
| src/components/sponsors/reports/statusTone.js | Central status->chip-tone mapping helper. |
| src/components/sponsors/reports/StatusRollupChips.js | Renders stable-order rollup chips for grouped Sponsor Assets cards. |
| src/components/sponsors/reports/StatusPill.js | Single status chip component with shared tone mapping. |
| src/components/sponsors/reports/ReportShell.js | Shared report page shell (header, action slot, filter slot, print CSS hook). |
| src/components/sponsors/reports/report-print.css | Print-only CSS to print report body. |
| src/components/sponsors/reports/OrdersTable.js | Purchase Details table built on uicore MuiTable + date/time formatting helpers. |
| src/components/sponsors/reports/GroupByToggle.js | Toggle for sponsor/component grouping mode. |
| src/components/sponsors/reports/GroupBySponsorView.js | Sponsor-grouped card list with drill-down links. |
| src/components/sponsors/reports/GroupByComponentView.js | Component-grouped card list with per-sponsor links and content previews. |
| src/components/sponsors/reports/FilterBar.js | Shared report filter bar (sponsor multiselect + optional search + extra controls). |
| src/components/sponsors/reports/ExportCsvButton.js | CSV export button integrating uicore getCSV and access token retrieval. |
| src/components/sponsors/reports/tests/usePrint.test.js | Unit test for print hook. |
| src/components/sponsors/reports/tests/TierBadge.test.js | Unit tests for TierBadge rendering behavior. |
| src/components/sponsors/reports/tests/SummaryPanel.test.js | Unit tests for SummaryPanel rendering and empty behavior. |
| src/components/sponsors/reports/tests/statusTone.test.js | Unit tests for status tone mapping helper. |
| src/components/sponsors/reports/tests/StatusRollupChips.test.js | Unit tests for rollup chips ordering and null rollup behavior. |
| src/components/sponsors/reports/tests/StatusPill.test.js | Unit tests for StatusPill label behavior and tone mapping re-export. |
| src/components/sponsors/reports/tests/ReportShell.test.js | Unit tests for ReportShell slots and icon rendering. |
| src/components/sponsors/reports/tests/OrdersTable.test.js | Unit tests for table helpers and basic rendering/sort interactions. |
| src/components/sponsors/reports/tests/GroupByToggle.test.js | Unit tests for group toggle behavior (ignore null toggle). |
| src/components/sponsors/reports/tests/GroupBySponsorView.test.js | Unit tests for sponsor-grouped card rendering and link targets. |
| src/components/sponsors/reports/tests/GroupByComponentView.test.js | Unit tests for component-grouped rendering, content hints, and links. |
| src/components/sponsors/reports/tests/FilterBar.test.js | Unit tests for FilterBar sponsorIds emission and optional search box. |
| src/components/sponsors/reports/tests/ExportCsvButton.test.js | Unit tests for CSV dispatch args, disabling, and in-flight click guard. |
| src/components/menu/menu-definition.js | Adds Sponsors → Reports menu entry guarded by admin-sponsors. |
| src/app.js | Injects SPONSOR_REPORTS_API_URL into window config. |
| src/actions/sponsor-reports-actions.js | Thunks for reports endpoints (purchase details, sponsor assets, filters, drilldown). |
| src/actions/tests/sponsor-reports-actions.test.js | Unit tests for sponsor reports thunks and read-error handler behavior. |
| .env.example | Adds SPONSOR_REPORTS_API_URL example env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- reports-text: decode uppercase/mixed-case HTML entities (the case-insensitive
match was missing the lowercase ENTITIES map); add toPlainText tests
- report-errors: export-disabled now dispatches { kind, status, message } to
match the other error branches
- report-query: coerce sponsorIds to positive integers and drop the rest so the
filter never emits sponsor_id==NaN/==0; add buildReportQuery tests
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 26: The SCOPES aggregate still includes reports/all, which is broader
than needed and conflicts with the dedicated sponsor report scopes. Update the
SCOPES value in .env.example to remove reports/all while keeping the existing
scoped report permissions such as SPONSOR_REPORTS_SCOPES and the other named
scope variables intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05a2e27c-7d13-497e-bd72-9787506508fa
📒 Files selected for processing (12)
.env.examplesrc/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/FilterBar.jssrc/reducers/sponsors/__tests__/sponsor-reports-reducers.test.jssrc/utils/__tests__/report-query.test.jssrc/utils/__tests__/reports-money.test.jssrc/utils/__tests__/reports-text.test.jssrc/utils/report-errors.jssrc/utils/report-query.jssrc/utils/reports-money.jssrc/utils/reports-text.js
✅ Files skipped from review due to trivial changes (2)
- src/utils/tests/reports-text.test.js
- src/utils/tests/reports-money.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/utils/reports-money.js
- src/utils/report-query.js
- src/utils/report-errors.js
- src/reducers/sponsors/tests/sponsor-reports-reducers.test.js
- src/actions/tests/sponsor-reports-actions.test.js
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Two low-severity findings from the deep review — both are code-quality nits with no functional impact on the feature. @caseylocker tagging you for awareness.
| // Defense-in-depth: callers pass route/backend integer ids (the drill-down page | ||
| // validates :sponsorId/:summitId before rendering). Never interpolate a | ||
| // non-integer value into a filter clause sent to the CSV endpoint. | ||
| if (Number.isInteger(sid)) kept.push(`sponsor_id==${sid}`); |
There was a problem hiding this comment.
[LOW] Defense-in-depth guard does not match its comment.
Number.isInteger(0) is true, so a sponsorId of "0" would emit sponsor_id==0 into the filter clause. The drilldown page validates upstream with isPositiveIntId, so there is no live attack path — but the guard here does not live up to the comment above it ("Never interpolate a non-integer value").
Suggestion: change to sid > 0 (also handles NaN since NaN > 0 is false), and same for pid on the next line.
There was a problem hiding this comment.
Done in e06b1e9e. Reworded the comment to match the guard — it now describes the sponsor_id/page_id integer check and drops the stale :summitId reference (this function doesn't touch summit).
| // MuiTable keys rows on row.id; the API exposes purchase_id, not id. | ||
| // The page must call rows.map(r => ({ ...r, id: r.purchase_id })) before | ||
| // passing data, or use this helper for explicit mapping. | ||
| export const getOrderRowId = (row) => row.purchase_id; |
There was a problem hiding this comment.
[LOW] Dead export with a misleading comment.
The comment on lines 43–45 says "The page must call rows.map(r => ({ ...r, id: r.purchase_id })) before passing data, or use this helper" — but OrdersTable already does that remapping internally in the data prop passed to MuiTable. getOrderRowId is never imported anywhere in the codebase.
A future developer reading this comment might add a redundant .map() in the page component, producing double-mapped rows.
Suggestion: either remove the export entirely, or update the comment to clarify the remapping is already done inside OrdersTable.
There was a problem hiding this comment.
Done in e06b1e9e. Confirmed unused — the table maps purchase_id → id inline at the data= prop; only its own test referenced it. Removed the export, comment, and test.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Convention audit against the established codebase patterns. 4 inline findings below — none are blockers, but two are 1-line fixes. @caseylocker
| PURCHASE_DETAILS_VALIDATION_CLEAR | ||
| } from "../../../../actions/sponsor-reports-actions"; | ||
|
|
||
| const DEFAULT_PAGE_SIZE = 10; |
There was a problem hiding this comment.
[Convention] DEFAULT_PAGE_SIZE should be imported from constants.js, not redefined locally.
src/utils/constants.js already exports DEFAULT_PER_PAGE = 10. Defining the same value under a different name here is a 1-line fix:
import { DEFAULT_PER_PAGE } from '../../../../utils/constants';
// then replace DEFAULT_PAGE_SIZE → DEFAULT_PER_PAGE belowThere was a problem hiding this comment.
Done in e06b1e9e — now imports DEFAULT_PER_PAGE from constants.js. (The Line Items view keeps a local LINES_DEFAULT_PAGE_SIZE = 50, since there's no matching constant for 50.)
|
|
||
| // Local pagination/sort state. MuiTable dir = 1 (asc) | -1 (desc). | ||
| const [filters, setFilters] = useState({}); | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
[Convention] Pagination/sort state is in local useState instead of Redux — deviation from the list-page pattern.
The established pattern (summit-admin-list-page-pattern.md) stores currentPage, perPage, order, orderDir in the Redux reducer via the 5th argument of getRequest, so the REQUEST_* case can sync them:
// Standard pattern
case REQUEST_ITEMS: {
const { currentPage, perPage, order, orderDir, term } = payload;
return { ...state, currentPage, perPage, order, orderDir, term };
}Here those four values live in useState and REQUEST_PURCHASE_DETAILS only sets loading: true. Same deviation exists in sponsor-asset-report-page.
Practical difference: a page refresh or summit switch always resets pagination to page 1 (because local state is lost on unmount). That may be intentional for a report view — if so, worth a brief comment saying so to signal it was a deliberate choice rather than an oversight.
There was a problem hiding this comment.
Left as local useState for now. These are read-only report views with ephemeral, page-local pagination/sort that nothing else in the app reads — a Redux slice would add actions/reducer for state that never leaves the component. The Line Items view stacked on this PR mirrors the same local pattern, so moving it would be a broader change across both grids. I'd lean toward keeping it local and revisiting if another view ever needs to read this state — happy to match the list-page pattern if you feel strongly.
| // Function form of mapDispatchToProps: injects raw dispatch (needed for the | ||
| // PURCHASE_DETAILS_VALIDATION_CLEAR action in the Snackbar handler) alongside | ||
| // the bound action creators. | ||
| const mapDispatchToProps = (dispatch) => ({ |
There was a problem hiding this comment.
[Convention] mapDispatchToProps in function form — the rest of the codebase uses the object shorthand.
The function form is used here to get raw dispatch for the Snackbar's onClose handler (dispatch({ type: PURCHASE_DETAILS_VALIDATION_CLEAR })). The conventional fix is to export a named action creator and map it normally:
// In sponsor-reports-actions.js
export const clearPurchaseDetailsValidation = () => ({
type: PURCHASE_DETAILS_VALIDATION_CLEAR
});
// In the page
export default connect(mapStateToProps, {
getPurchaseDetailsReport,
getPurchaseDetailsFilters,
clearPurchaseDetailsValidation
})(PurchaseDetailsReportPage);Not a blocker, but keeps the pattern consistent across all pages.
There was a problem hiding this comment.
Done in e06b1e9e. Converted to object shorthand. The function form only needed raw dispatch to clear the validation toast, so I added a clearPurchaseDetailsValidation() action creator and bound it via the object form — the raw dispatch prop is gone.
| // Prints the currently loaded report body only (server-paginated page). | ||
| // v1 caveat: captures only the currently loaded page, not the full filtered view. | ||
| // A true full-report print would need a print-mode fetch of all pages — out of scope. | ||
| const usePrint = () => useCallback(() => window.print(), []); |
There was a problem hiding this comment.
[Convention] Custom hook should live in src/hooks/, not co-located with components.
The project's canonical location for custom hooks is src/hooks/ (see src/hooks/useScrollToError.js). Moving this file there is a 1-line import path change in the two pages that consume it.
There was a problem hiding this comment.
Done in e06b1e9e. Moved to src/hooks/usePrint.js (next to useScrollToError) and updated the three reports pages' imports.
|
|
||
| const query = {}; | ||
| if (filter.length > 0) query["filter[]"] = filter; | ||
| if (search) query.search = search; |
There was a problem hiding this comment.
[Convention] Free-text search param is not passed through escapeFilterValue.
The project rule (summit-admin-redux-actions.md) says: "Always use escapeFilterValue(term) before inserting user input into filters." Here search goes directly as a query param rather than into a filter[] clause, so the immediate injection risk is lower — but the guidance covers user-supplied strings broadly.
Worth confirming with the backend team whether the search param on the reports API needs escaping on the client side, or whether the server handles it. If not needed, a short comment here would clarify why the escape was intentionally omitted.
There was a problem hiding this comment.
The search value goes through as a top-level query param, not into a filter[] clause, so escapeFilterValue (which escapes filter operators) doesn't directly apply.
|
Addressing the two remaining false positives from CodeRabbit's nitpick summary (review #4567585391):
The useEffect(() => {
setDraft(value);
}, [value]);The comment above it even explains the intent: "Re-sync the draft when the committed
The test is present, starting at line 127: describe("PURCHASE_DETAILS_VALIDATION_CLEAR", () => {
it("clears validationError without replacing the body", () => {
...
expect(result.validationError).toBeNull();
});
});CodeRabbit scanned lines 110–124 (the set path) and didn't look at the lines immediately following. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js`:
- Around line 126-141: The three effects in
purchase-details-report-page/index.js are missing the in-context summit as a
dependency, so they won’t rerun when the summit changes after mount. Update the
useEffect hooks that call fetchFilters, fetchReport, and fetchLinesReport to
depend on currentSummit?.id (or the equivalent summit identifier from the
component scope) along with the existing inputs so the page refetches filters
and reports when the active summit is hydrated or switched. Keep the mount-only
behavior only if it still includes summit changes as a trigger for the
data-loading effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aceb4674-64b2-4907-afd9-8c0ad10a05a7
📒 Files selected for processing (14)
src/actions/__tests__/sponsor-reports-actions.test.jssrc/actions/sponsor-reports-actions.jssrc/components/sponsors/reports/LinesManifestView.jssrc/components/sponsors/reports/ReportViewToggle.jssrc/components/sponsors/reports/__tests__/LinesManifestView.test.jssrc/components/sponsors/reports/__tests__/ReportViewToggle.test.jssrc/i18n/en.jsonsrc/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.jssrc/reducers/sponsors/__tests__/sponsor-reports-purchase-details-lines-reducer.test.jssrc/reducers/sponsors/sponsor-reports-purchase-details-lines-reducer.jssrc/store.jssrc/utils/__tests__/manifest-grouping.test.jssrc/utils/manifest-grouping.js
✅ Files skipped from review due to trivial changes (1)
- src/utils/manifest-grouping.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/store.js
- src/i18n/en.json
- src/actions/tests/sponsor-reports-actions.test.js
- index.js: import DEFAULT_PER_PAGE from constants (drop local DEFAULT_PAGE_SIZE dup) - index.js: mapDispatchToProps to object shorthand via new clearPurchaseDetailsValidation action creator (removes the raw dispatch prop) - OrdersTable.js: remove dead getOrderRowId export + its test (page maps id inline) - usePrint: move to src/hooks/ per the codebase custom-hook convention - section-csv-query.js: align defense-in-depth comment with the sponsor_id/page_id guard Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js (1)
196-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the duplicated CSV url/query derivation into a small helper.
linesCsvUrl/linesCsvQueryrepeat the exact base-URL construction andpage/per_pagestripping already done for the orders export just above. A tiny helper (e.g.buildCsv(path, srcQuery)returning{ url, query }) would remove the copy and keep both exports in sync if the strip rules change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js` around lines 196 - 208, The CSV export setup for purchase details duplicates the same URL construction and pagination-stripping logic already used for the orders export. Extract that repeated logic into a small shared helper in this page (for example, a function that builds the CSV url and query from a path and source query), then use it for both exports so `linesCsvUrl` and `linesCsvQuery` stay aligned with the existing order export behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js`:
- Around line 196-208: The CSV export setup for purchase details duplicates the
same URL construction and pagination-stripping logic already used for the orders
export. Extract that repeated logic into a small shared helper in this page (for
example, a function that builds the CSV url and query from a path and source
query), then use it for both exports so `linesCsvUrl` and `linesCsvQuery` stay
aligned with the existing order export behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a24726cb-2af3-4f33-a505-8cb87decc70c
📒 Files selected for processing (2)
src/pages/sponsors/sponsor-reports/purchase-details-report-page/__tests__/index.test.jssrc/pages/sponsors/sponsor-reports/purchase-details-report-page/index.js
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Hey @caseylocker, this PR doesn't follow our codebase's coding patterns. For example, you added several files to the utils dir that are specific to these reports. No other page or section of the admin does this — if it did, utils would end up with dozens of one-off files, defeating the purpose of having a generic utils dir.
I left a few examples inline. Could you go through the PR and align the solution with summit-admin's coding patterns?
| @@ -0,0 +1,7 @@ | |||
| export const getReportsApiBaseUrl = () => window.SPONSOR_REPORTS_API_URL; | |||
There was a problem hiding this comment.
having one utils file per page is not scalable, please remove this unnecessary helper method and move isPositiveIntId to utils/method.js file since it is a generic helper not tied to reports api
There was a problem hiding this comment.
Done — moved isPositiveIntId to utils/methods.js (fd1296c0) and deleted reports-api.js, repointing the import (6757a71d). No per-page utils file remains.
|
|
||
| // Formats a DOLLAR amount (number or numeric string) as "$1,234.56". | ||
| // Non-numeric / null → em dash. | ||
| export const formatUsd = (value) => { |
There was a problem hiding this comment.
we already have a currency parser in uicore, please use that and remove this file
There was a problem hiding this comment.
Done — switched to uicore currencyAmountFromCents and deleted reports-money.js (a18a6d5c). Since the uicore parser is cents-based, I also moved the reports JSON API to a cents contract (it was returning dollar strings): fntechgit/sponsor-reports-api#29. The CSV export stays in dollars for readability.
Two notes: that backend PR must deploy before/with this one (the FE now feeds these values straight to currencyAmountFromCents), and money renders without a thousands separator ($1536.00) to match the rest of summit-admin.
| " ": " " | ||
| }; | ||
|
|
||
| export const toPlainText = (html) => { |
There was a problem hiding this comment.
we already have a method that does this in uicore, please use that and remove this file
There was a problem hiding this comment.
Done — deleted reports-text.js (500cab97). I checked uicore first: neither utils/methods nor anywhere else in the package exports an html→text helper, and summit-admin's existing htmlToString uses documentElement.textContent, which fuses adjacent-tag text (<p>a</p><b>b</b> → ab). So I added a generic htmlToPlainText to methods.js (fd1296c0) — tags→space + DOMParser entity decode — and pointed both consumers at it. If you did have a specific uicore helper in mind, point me at it and I'll switch.
| // stripping the sponsor clause and re-emitting `status==Paid` as its own bracket turns | ||
| // OR into AND. The v1 query builder never emits mixed brackets (sponsor is always its | ||
| // own bracket), so this is a defensive edge, not a live path. | ||
| export const buildSectionCsvQuery = ( |
There was a problem hiding this comment.
this is only used in reports action file, lets move it there and remove this file
There was a problem hiding this comment.
Done — folded into the exportSponsorAssetSectionCsv action thunk and deleted the file (306ce261).
| setBusy(true); | ||
| try { | ||
| const token = await getAccessTokenSafely(); | ||
| dispatch(getCSV(url, { ...query, access_token: token }, filename)); |
There was a problem hiding this comment.
create a method in actions file the builds the query based on page, id , filters props like every other export method in summit_admin. We need to keep a pattern in our code ir order to make it easier to understand it
There was a problem hiding this comment.
Done — all four CSV exports are now action thunks that own url/params/filename like exportEventRsvpsCSV: exportPurchaseDetailsCsv/exportPurchaseDetailsLinesCsv (a0310a18) and exportSponsorAssetCsv/exportSponsorAssetSectionCsv (306ce261); the pages dispatch them from a plain MUI button and ExportCsvButton.js is gone.
Two intentional consequences worth flagging: (1) the plain buttons drop the old in-flight double-click guard — that matches every other export in summit-admin (none debounce), so I left it off rather than re-add a bespoke wrapper; (2) the thunks put access_token first ({ access_token, ...query }) to match the sibling fetch thunks in the same file, so the serialized query-string order differs from the old button — param values are identical and the backend is order-independent.
Backend now emits money fields as integer cents. Drop the bespoke reports-money.js formatter (per santi PR #997 review) and use the platform-wide currencyAmountFromCents from openstack-uicore-foundation. Null guard via inline/local const rather than a new shared helper file. Test fixtures updated to cents input; assertions to no-comma "$X.XX" output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@santipalenque Thank you very much for the excellent review. These are great callouts and will help me to align closer with code patterns in the project going forward. I believe that I've addressed all of your concerns with the latest push. Let me know if I missed something, especially with this one #997 (comment). |
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Hey @caseylocker, thanks for the fixes. It looks like only the inline-commented spots were addressed — could you go through the rest of the PR and check for similar coding pattern issues elsewhere, not just the ones I flagged?
| const { currentSummit } = currentSummitState; | ||
| // No summit in context → skip. Otherwise base(currentSummit.id) throws | ||
| // synchronously after startLoading() and the spinner is never cleared. | ||
| if (!currentSummit?.id) return undefined; |
There was a problem hiding this comment.
this function can return either undefined synchronously or a Promise depending on the branch, which is inconsistent for callers using .then()/await.
There was a problem hiding this comment.
Good catch — normalized every thunk's guard branch to return Promise.resolve() so callers always get a thenable (all 10 in this file), 35974ee.
| @@ -0,0 +1,16 @@ | |||
| /* src/components/sponsors/reports/report-print.css */ | |||
There was a problem hiding this comment.
lets use inline styles following MUI standards:
`<Box
sx={{
"@media print": {
display: "none"
}
}}
`
There was a problem hiding this comment.
I looked at this one and I don't think an element-scoped sx can replace it here. This stylesheet does whole-page print isolation — body * { visibility: hidden } then .report-body, .report-body * { visibility: visible } — so on print everything except the report body is hidden. An sx={{ "@media print": { display: "none" } }} block only targets its own element + descendants, so it can't hide the rest of the page chrome the way the body * rule does. The MUI-idiomatic equivalent would be with the same @media print rules, but that's a lateral move (and there are standalone CSS/Less files elsewhere in summit-admin), so I left the stylesheet as-is. Happy to switch it to if you'd prefer that over a .css file — your call.
There was a problem hiding this comment.
standalone css files is legacy code that we are trying to move away from. if there is a way of avoiding it without it being too messy please do.
There was a problem hiding this comment.
Good call — switched it to MUI <GlobalStyles> and deleted report-print.css. The print behavior is unchanged (same whole-page isolation: hide everything, reveal only .report-body), it's just sourced from the component now instead of a standalone stylesheet. That was the last .css file in the reports feature. fea1f2d1 I'm also making note of the fact that we're moving away from standalone and isolated css so I'll know to avoid it in the future. Thanks for that.
| // eslint-disable-next-line no-magic-numbers | ||
| const PER_PAGE_OPTIONS = [10, 25, 50, 100]; | ||
| // eslint-disable-next-line no-magic-numbers | ||
| const DEFAULT_PER_PAGE = 50; |
There was a problem hiding this comment.
this default already exists in constants.js . Also you can build the options like this:
const BASE_PER_PAGE_OPTIONS = [DEFAULT_PER_PAGE, TWENTY_PER_PAGE, FIFTY_PER_PAGE];
There was a problem hiding this comment.
Done — now imports the named per-page constants from src/utils/constants.js (DEFAULT_PER_PAGE/TWENTY_PER_PAGE/FIFTY_PER_PAGE/MAX_PER_PAGE), dropped the magic numbers and the eslint-disable, 473c625.
| import StatusPill from "./StatusPill"; | ||
|
|
||
| const ISO_DATE_LENGTH = 10; // "YYYY-MM-DD" | ||
| const MS_PER_SECOND = 1000; |
There was a problem hiding this comment.
Done — replaced with MILLISECONDS_IN_SECOND from constants, 473c625.
| // (pending ClickUp 86bagnfmn). Parses date/time directly off the ISO string | ||
| // parts so the displayed time always matches the stored UTC value and tests | ||
| // stay timezone-stable regardless of the machine's local TZ offset. | ||
| export const formatCheckoutTime = (value) => { |
There was a problem hiding this comment.
can't we use moment js to parse the date ?
There was a problem hiding this comment.
Done — formatCheckoutTime now uses moment.utc(...) instead of the hand-rolled regex, kept UTC so output is unchanged, 78bc8c6.
| ]; | ||
|
|
||
| // eslint-disable-next-line no-magic-numbers | ||
| const DEFAULT_PER_PAGE = 10; |
There was a problem hiding this comment.
Done — removed the local DEFAULT_PER_PAGE, imports it from constants (the page already did), 473c625.
| // A status token rendered as a colored, filled chip. `label` overrides the | ||
| // displayed text (e.g. a T.translate'd label); the color always derives from | ||
| // the raw `status` token via the shared tone map. | ||
| const StatusPill = ({ status, label, size = "small" }) => ( |
There was a problem hiding this comment.
make this configurable so you can use it with StatusRollupChips so SatusTone can be part of this component and doesn't need to be its own file.
There was a problem hiding this comment.
Done — folded statusTone (map + lookup) into StatusPill as a named export and deleted the standalone file; StatusRollupChips now imports statusTone from StatusPill, 6dcd257.
| // src/components/sponsors/reports/__tests__/statusTone.test.js | ||
| import { statusTone } from "../statusTone"; | ||
|
|
||
| describe("statusTone", () => { |
There was a problem hiding this comment.
this looks like a silly test, this file is just an array map
There was a problem hiding this comment.
Removed with the file in 6dcd257; kept the non-trivial branch coverage (case-insensitivity, unknown→default, null-safety) in StatusPill.test.js.
| > | ||
| <SummaryPanel tiles={tiles} /> | ||
| {/* 412 → inline toast; body preserved (rows stay visible) */} | ||
| <Snackbar |
There was a problem hiding this comment.
we already have a snackbar hook to show success or error messages
There was a problem hiding this comment.
Done — replaced the inline Snackbar/Alert with the existing useSnackbarMessage hook (global provider already mounted), preserving the redux validation→clear lifecycle, 7632cbe.
…nsors Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tCSV) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… refund tile) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gial SORT_FIELD_MAP Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…utes) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace placeholder with two MUI card links (Purchase Details, Sponsor Assets); add breadcrumb; add landing_title i18n key; update layout test for card assertions; 6 new tests, 912 total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
toOrderParam moved to the query thunk in an earlier commit; the column comment no longer describes a page-handler conversion.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1oFJHczgo13Z6MnUUHjwV
Wire ContentTypeToggle (default: collected) into SponsorAssetReportPage — injects moduleType:"Media" on fetch + CSV export when collected, omits it for all; resets page on toggle. Tests: 12/12 (+3 new), full suite 995/995. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1oFJHczgo13Z6MnUUHjwV
…r in drilldown Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1oFJHczgo13Z6MnUUHjwV
Drop-empty-section now applies only in Collected mode; All renders every section as-is, including a page the sponsor never submitted to.
…ia); remove toggle Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1oFJHczgo13Z6MnUUHjwV
… HTML-flatten test - exportSponsorAssetSectionCsv now filters module_type==Media so the per-page Download CSV matches the collected-only view (was an escape path for Document/Info rows). - Restore ContentCell HTML-flatten coverage as a Media text/input row (the prior test used an Info row removed with the toggle teardown).
…lToPlainText Use an input with entities + adjacent tags (<p>Booth A</p><p>B & C</p> → "Booth A B & C") so the test exercises entity-decode + whitespace-collapse + tag-to-space — the behavior that distinguishes htmlToPlainText from stripTags. A regression swapping the helper would now fail.
…rop port-framing comments Convention-sweep fixes (Codex full-branch pass): - query + section-CSV thunks reuse isPositiveIntId instead of hand-rolled Number.isInteger ID checks; section CSV bails on an invalid id rather than emitting a broadened (whole-sponsor) export - replace local GROUP_PER_PAGE=25 (an arbitrary literal carried from the sponsor-services source, not spec'd) with the existing TWENTY_PER_PAGE - rewrite OrdersGrid/sponsor-services port-framing comments to describe summit-admin behavior only Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S1oFJHczgo13Z6MnUUHjwV
…yles Removes the last standalone stylesheet in the reports feature (legacy in summit-admin per santipalenque). Whole-page print isolation is preserved verbatim via an inline <GlobalStyles> block in ReportShell; the .report-body className anchor is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FbjUV2hmmvm8BWErNvJfdF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…memo Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_page paging Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rouping Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…exports; fix empty-state flash Retire getSponsorAssetReport (no callers after the pivot rewire), its dead reducer fields/case and the group_by query branch, drop unused axisKeyOf/axisLabelOf, and default the reducer loading flag true to avoid a no-results flash before first fetch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uest token) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FbjUV2hmmvm8BWErNvJfdF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FbjUV2hmmvm8BWErNvJfdF
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FbjUV2hmmvm8BWErNvJfdF
…setRows via guardedDispatch Introduces a single `guardedDispatch` closure (checked against `mySeq` at every call site) to replace the two existing ad-hoc terminal guards. Hole 1 — stuck loading: REQUEST_SPONSOR_ASSET was dispatched after `await getAccessTokenSafely()` without a seq-guard. A stale call that resumed after a newer call had already committed RECEIVE_SPONSOR_ASSET_ROWS would re-flip loading:true with no terminal to clear it. guardedDispatch suppresses the REQUEST dispatch for any superseded invocation. Hole 2 — persistent stale error: fetchPage passed raw dispatch to getRequest, so a stale request's HTTP error handler fired SPONSOR_ASSET_READ_ERROR unguarded — clobbering fresh success state with readError. Changing the innermost `)(dispatch)` to `)(guardedDispatch)` threads the guard through getRequest's error-handler invocation. The RECEIVE and catch-block SPONSOR_ASSET_READ_ERROR terminals now also use guardedDispatch, consolidating all four dispatch sites under one mechanism. Tests added (getSponsorAssetRows): • suppresses REQUEST_SPONSOR_ASSET from a stale call (prevents stuck-loading) • suppresses SPONSOR_ASSET_READ_ERROR from a stale call's error handler Full suite: 1089/1089 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
caseylocker
force-pushed
the
feature/sponsor-reports-port
branch
from
fea1f2d to
ac0a1f7
Compare
…onsor Assets UI The report is scoped to Media assets (server applies module_type==Media), whose completion strategies only ever emit pending/in_progress/completed — never not_applicable. The N/A summary tile and per-group rollup chip were therefore always zero. Remove not_applicable from the tile keys, rollup chips, pivot-tree accumulator, and the now-dead i18n key. Backend contract is untouched (it may still send the key; the FE simply ignores it). StatusPill's generic not_applicable->default tone mapping is left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… request) Single-select Status filter (Any / Completed / In Progress / Pending) in the Sponsor Assets report's FilterBar extraControls slot. Pure FE: the backend SponsorAssetFilter already allows status==, buildReportQuery already emits it, and every row carries status — only the UI control was missing. Reuses STATUS_TILE_KEYS so the options stay in lockstep with the summary tiles. Applies server-side (refetch on Apply), so the summary tiles recompute on the filtered set and CSV export inherits the filter. No backend change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… Purchase Details orders table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FbjUV2hmmvm8BWErNvJfdF
…report Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ortable, exact em-dash count)
ref: https://app.clickup.com/t/86bak754x
Ports the two sponsor reports (Purchase Details + Sponsor Assets, incl. the per-sponsor asset drill-down) out of
sponsor-servicesand into summit-admin as an admin-only "Reports" sub-item under the Sponsors menu, reading the in-context summit. Backend (sponsor-reports-api) is unchanged. This is Phase 1 — parity port; the JP-pivot logistics/finance features are Phase 2 follow-ons.What's included
Restrict(withRouter(SponsorReportsLayout), "admin-sponsors")— an explicit admin-only gate that reuses the existingadmin-sponsorskey and closes the direct-URL gap for the reports area. Summit is in-context (summit picker +/reports/summitsdropped).connect()(no hooks), react-router 4withRouter(no v6 hooks), i18n-reactT.translate(no react-i18next), uicoreMuiTable(no@mui/x-data-grid), CSV via uicoregetCSV.OrdersTablerebuilt onMuiTable;formatCheckoutTimehandles both ISO and epoch; Total Refunded tile renders only whensummary.total_refunded != null(auto-appears once that field ships — no FE change needed)./app/summits/:summitId/sponsors/reports/sponsor-assets/sponsors/:sponsorId.SPONSOR_REPORTS_API_URLinjected inapp.js+.env/.env.example.Tests / quality
yarn jest: 915 passing, 0 regressions vs master.yarn lint: 0 new errors (only the repo's pre-existing prop-types warnings).Deploy note (not blocking this PR)
Backend auth wiring (D5) is done on dev — repointed the deployed
sponsor-reports-apiscope env toreports/all+ added the admin groups; verified both reports load real data on dev. Staging/prod need the same env at launch (tracked in the ClickUp ticket). No IDP change was needed (summit-admin already carriesreports/all).🤖 Generated with Claude Code
Summary by CodeRabbit