fix: stop the compliance panel crashing on a clean period, and leaking a blob per download (#1138) - #1143
Merged
Aditya8369 merged 1 commit intoSep 1, 2026
Conversation
…g a blob per download (Aditya8369#1138) Four defects in the generate-then-download path. 1. `exceedances` is read unguarded by both the table and exportToCSV, but it is optional over the wire -- a compliant period is reasonably returned as `{ totalExceedances: 0 }` with no list at all. The component even has a "No exceedances found in this period. Good job!" row for that case, and reaching it threw `TypeError: Cannot read properties of undefined (reading 'length')`. The one outcome the feature exists to report was the one that broke it. normaliseReport() now guarantees the array at the service boundary. 2. Neither download branch revoked its object URL, so every export pinned its blob for the life of the document, and the anchor was never attached before the click -- which some browsers ignore outright. chartExport.js already had this right in a private helper; it moves to src/utils/downloadFile.js and chartExport now imports it rather than the code being copied a third time. downloadComplianceReport, imported by the component and never called, uses it too. 3. The error path called `await response.json()` unguarded, which throws on a 502 from a proxy, an HTML error page or an empty body. The `|| 'Failed to generate report'` fallback on the next line could never run, because the line before it was what threw, and the user was shown `SyntaxError: Unexpected token '<'`. 4. The form only checked that both dates were present, so an end date before the start was submitted happily, and the filename carried only the start date -- two reports for different periods sharing a start date overwrote each other on disk. Also here, all in files this change already touches: the auth header is omitted rather than sent as "Bearer null", the token read is guarded so blocked site data cannot throw a SecurityError out of a service call, the request is aborted on unmount, the three form labels are tied to their inputs, validation messages land in a role="alert", and the stat grid declares four columns and now has four tiles in it instead of two. Tests: 11 for downloadFile, 19 for complianceEngine, 18 for the panel. Against the code before this change, 16 of them fail.
|
@MOHITKOURAV01 is attempting to deploy a commit to the Aditya Mahajan's projects Team on Vercel. A member of the Team first needs to authorize it. |
Thank You for Your Contribution! 🎉Hi @MOHITKOURAV01, Thank you for opening this Pull Request and contributing to our project. We truly appreciate your efforts.
The maintainer @Aditya8369 will review your PR shortly! Happy Contributing! 🚀 |
|
🎉 Your PR just got merged, @MOHITKOURAV01 — thank you for contributing to Pollution Control Hub! Your work is now part of the project. Here's what to do next:
We really appreciate you taking the time. See you in the next PR! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Four defects in the Regulatory Compliance Reporting generate-then-download path. Together
they mean a clean compliance period crashes the page and every download leaks memory.
1. A report with no exceedances crashes the panel.
exceedancesis read unguarded byboth the table (
report.exceedances.length) andexportToCSV(report.exceedances.map),but it is optional over the wire — a compliant period is reasonably returned as
{ id, standard, totalExceedances: 0 }with no list at all. The component even has a"No exceedances found in this period. Good job!" empty row for exactly that case, and
reaching it gave
TypeError: Cannot read properties of undefined (reading 'length'). Theone outcome this feature exists to report was the one that broke it.
2. Every download leaked an object URL. Neither branch of
handleDownloadcalledrevokeObjectURL, so each export pinned its blob in memory until the tab closed. Theanchor was also never attached to the document before the click, which some browsers ignore
outright.
3. The error path replaced the server's message with a parser error.
A 502 from a proxy, an HTML error page or an empty 500 body all reject with
SyntaxError: Unexpected token '<', "<html>"...— and that is the string the user saw.4. No date-range validation, and the filename carried only the start date, so two
reports for different periods sharing a start date overwrote each other in the downloads
folder.
Related Issue
Closes #1138
Type of Change
Changes Made
src/utils/downloadFile.js(new)downloadFile(content, mimeType, filename)— attaches the anchor before clicking it,removes it in a
finally, and revokes the object URL on the next tick (revokingsynchronously cancels the download in browsers that read the blob after the click returns;
never revoking leaks).
This is not new code so much as relocated code:
chartExport.jsalready had it right andkept it private.
chartExport.jsnow imports it, so the correct version is in one placeinstead of being copied a fourth time.
safeFilenamePartsanitises the parts of a filename that come from form input.src/services/complianceEngine.jsnormaliseReport()guaranteesexceedancesis an array and derivestotalExceedancesfrom the list only when the server didn't send a count (the list can be a truncated page
of a larger total, so the server's number wins). Exported, because the component needs
the same guarantee for a report it already holds.
describeFailure()reads the error body in atry, so a non-JSON body falls back toFailed to generate report (HTTP 502)instead of aSyntaxError.Bearer null, and the token read isguarded — blocked site data makes the
localStorageaccess itself throw, and a 401 is amuch better failure than a
SecurityErrorout of a service call.downloadComplianceReport— imported by the component and never called — goes throughthe shared helper, and encodes the report id and format into its URL.
AbortSignal.src/components/ComplianceReportGenerator.jsxnormaliseReport(report), so the missing-exceedancesshape reachesneither the table nor the exporter.
describeRangeProblem()andreportFilename()— exported and pure, so the rules aretestable without rendering. The filename now carries both ends of the period.
role="alert", and the form isnoValidate: the nativebubble is not reliably announced and vanishes on the next interaction, so one announced
message for every failure is better here. The
requiredattributes stay for thearia-requiredthey imply.htmlFor/id. That also clears thethree
jsx-a11y/label-has-associated-controlwarnings on this file.md:grid-cols-4and only ever had two tiles in it. The period andthe generation time were already on the report and nowhere on screen — they fill it.
Tests (new) —
downloadFile.test.js(11),complianceEngine.test.js(19),ComplianceReportGenerator.test.jsx(18).Testing
The 8 existing
chartExporttests still pass against the extracted helper — that's thecheck that the move didn't change its behaviour. Reverting the two source files and
re-running the new suites fails 16 tests, including the two that reproduce the
TypeErrorand theSyntaxError.Deliberately not touched
src/utils/reportExporter.js— the CSV escaping there is #1052, and #1057 is already openagainst that file. Keeping off it means these two can merge in either order.
Note on CI
Lint, Build and Playwright are red on
mainand on every open PR —npm run buildfailson the 3 parse errors of #1129 (
App.jsx,Leaderboard.jsx,NoisePollutionTracker.jsx).Nothing here touches those files.