Repository navigation
fix(accessibility): read any report link directly: scanner links by their run's build id, Workflow Analyzer links - #527
Conversation
…heir run's build id, Workflow Analyzer links A user usually has only a report's dashboard link, and two kinds of link failed: - A Website Scanner link carries the run's build id (thBuildId), not the scan's ids, so the tool searched the 15 newest pages of the scan list for it. A scan run more than a few hundred scans ago (page 36 of a 3,000-scan account) was "not found", and agents fell back to Ask AI or asked the user for ids the dashboard does not show. The link is now read by its build id through the scanner issues route that takes one (browserstack/accessibility#5689), however old the scan; later pages and a still-generating export are named by the same link. If the API does not know the run by that id (an API without the route answers a 404 page; one that does, a 404), the scan list is searched as before, and the answer says which happened. Comparing runs needs the scan's ids, so compareWith from a link still searches the list. - A Workflow Analyzer link (workflow-analyzer/report?ids=…, or the older reports/report) was not recognised. It is now read by the report id it carries; a link opening several reports asks for one, and an Assisted Test link says this tool does not read those. A build id that neither the Automated Tests nor the App Accessibility API knows is tried as a Website Scanner run's build id before it is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
Conflicts:
- report-fetcher.ts: the scanner issues route by build id goes through
apiBase() (getRegionalHost("accessibilityApi")), and scanRunLink builds its
dashboard link on getRegionalHost("accessibilityDashboard"), as the app
fetcher does; no hardcoded hosts remain.
- accessibility.ts: reportUrl keeps main's trimmed description.
- accessibilityScanOutput.test.ts: both new describe blocks kept.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sarve-shreyas
left a comment
There was a problem hiding this comment.
Reviewed the full diff against main at 6abd4a4. npm run build (lint, prettier, tsc, vitest) passes locally on this head: 139 files, 2,064 tests.
The approach looks right. Reading a scanner link by thBuildId with the scan-list fallback, the routeMissing-vs-JSON-404 split, and the Workflow Analyzer / Assisted Test link handling all read correctly. Comparing from a scanner link still goes through findScanRun, which is the right call.
One blocking item and one nit inline. The blocking item is still present at the top of the stack (#553).
| let reportLink: string | undefined; | ||
| if (source.kind === "scanBuild") { | ||
| try { | ||
| reportLink = await reportFetcher.getReportLink(source, { |
There was a problem hiding this comment.
Blocking: a scanBuild read caches a possibly partial export for the link's whole lifetime.
getReportLink is called here without stillProcessing, because reportStatus returns undefined for scanBuild. So the export goes into reportExports (keyed by JSON.stringify(source)) until the S3 link's Expires minus the margin, which is hours. The first-page note says "a scan that finished in the last few minutes may still be adding issues", but every later call with the same link in this process gets that same snapshot back from the cache, including calls made after the scan has finished. On the hosted server the process lives long, so this is the normal case.
Scenario: user shares a scanner link right after the run ends, the agent reads it (partial), the user asks again 20 minutes later and gets the same partial count with no note that it is stale.
Suggestion: pass stillProcessing: true for scanBuild (neither reuse nor keep the export) until the status can be read by build id. Or give scanBuild entries a short TTL. Once findScanRun has resolved the ids (the fallback path), the normal status check applies, so only the direct route needs this.
| } | ||
| const reportIds = reportPage ? url?.searchParams.get("ids") : null; | ||
| if (reportIds) { | ||
| const ids = reportIds.split(",").map((id) => id.trim()); |
There was a problem hiding this comment.
Non-blocking (nit): empty entries aren't filtered. ?ids=275165, (a trailing comma, easy to get when a link is hand-edited) is reported as "several Workflow Analyzer reports (2)", and ?ids=, reads report "". Suggest .map(trim).filter(Boolean), plus a clear error when nothing is left.
…ip empty WFA ids A run read by its build id has no status check, so its export may be a snapshot of a run still adding issues. Its link is now reused for 5 minutes at most, enough for paging, instead of until the presigned link expires (hours). A Workflow Analyzer link's ids parameter drops empty entries, so a trailing comma no longer reads as two reports, and a link naming none says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sarve-shreyas
left a comment
There was a problem hiding this comment.
Re-reviewed 6abd4a4..18636cd. npm run build passes locally: 139 files, 2,064 tests.
Resolved:
- Export read by build id cached for hours:
scanBuildlinks are now kept at most 5 min (SCAN_BUILD_LINK_TTL_MS). That's enough for paging, and a later read sees the finished run. - Empty Workflow Analyzer ids: filtered out, and a link with none left gets a clear error.
No open findings from me on this PR. A unit test for the 5-minute cap would be nice to have, but it isn't blocking.
sarve-shreyas
left a comment
There was a problem hiding this comment.
Approved at 18636cd. Both items from my review are fixed (5-min cache cap for exports read by build id, empty Workflow Analyzer ids). Build passes locally, 2,064 tests.
Stack: #527 (this, into
main) → #541 → #553. Merge in that order. Updated with currentmain; this PR's new API calls and the scanner run link now use the account's region (#489).Why
A user usually has only a report's dashboard link. Two kinds of link failed:
thBuildId), not the scan's ids. The tool therefore searched the newest 15 pages of the scan list (375 scans) for a scan whose link matched.buildId(rejected), then Ask AI (step limit), then asked the user for ids the dashboard doesn't show./workflow-analyzer/report?ids=…) were not recognised at all.What
Scanner links are read by the run's build id
scanBuild, with no API call.GET /api/website-scanner/v1/scan_runs/issues?thBuildId=…, from browserstack/accessibility#5689). It uses the same CSV export and polling as the other sources, so the link works however old the scan is.reportUrl: "…/site-scanner/projects/<name>/builds?thBuildId=…"). Later pages need no scan ids.compareWithfrom a scanner link still needs the scan's ids, so it still searches the list.Workflow Analyzer links are read by the report id they carry
/workflow-analyzer/report?ids=Nand the older/reports/report?ids=Nwork.workflowReportId. Empty entries (a trailing comma) are ignored, and anidsthat names no report says so.Assisted Test links (
/assisted-test/report?ids=…) now say plainly that this tool doesn't read Assisted Test reports, instead of the generic "this link does not name a report".A
buildIdno build API knows (when the Automated Tests and App Accessibility APIs both answer 404) is tried as a Website Scanner run's build id before it is refused. The refusal then names all three APIs, or, where the API has no route yet, points at the report link.The "where ids come from" line in the tool description is left to #524, which changes it (it drops the Ask AI pointer) and would otherwise conflict.
docs/TOOLS.mdis updated.Verified
npm run build: lint, prettier, 1,616 tests, tsc. New tests inaccessibilityReportLinks.test.ts, plus flow tests inaccessibilityScanOutput.test.tsandaccessibilityFetchPagesAndCompare.test.ts. They cover the route and its URLs, both kinds of 404, the fallback, both Workflow Analyzer link forms, several ids, the Assisted Test message, thebuildIdfallthrough, and comparing from a link.?ids=)🤖 Generated with Claude Code