LWLP-752: add separate route for report - #1216
Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Pages/Lightwell/Coverage/hooks/useCoverageReport.ts" line_range="15" />
<code_context>
+ const location = useLocation();
+ const filename = (location.state as { filename?: string } | null)?.filename;
+
+ const { data, isLoading } = useCoverageReportQuery(reportUUID ?? '', false);
+
+ const report: CompletedCoverageReport | undefined =
</code_context>
<issue_to_address>
**issue (bug_risk):** Coverage report request failures are converted into the same `!report` state as a missing or non-completed report, so the page renders `LightwellNotFound` without an error state, retry action, or clear indication that the request failed.
**Triggers:** When loading the report endpoint returns a network error or server error.
**Suggested fix:** Read `isError` from `useCoverageReportQuery` and render an appropriate error state separately from the not-found case.
</issue_to_address>
### Comment 2
<location path="src/Pages/Lightwell/Coverage/hooks/useCoverageReport.ts" line_range="13" />
<code_context>
+
+ const { navigateTo } = useLightwellNavigateTo();
+ const location = useLocation();
+ const filename = (location.state as { filename?: string } | null)?.filename;
+
+ const { data, isLoading } = useCoverageReportQuery(reportUUID ?? '', false);
</code_context>
<issue_to_address>
**issue (broader_impact):** The report filename is stored only in React Router navigation state, so refreshing or directly opening `/lens/:reportUUID` sets `filename` to undefined and changes the heading from the manifest-specific title to the generic `Match analysis` title.
**Triggers:** When the user refreshes the completed report page or opens its URL directly.
**Suggested fix:** Persist the filename with the completed report data or another durable store, or include it in a URL/query parameter if the API cannot provide it.
</issue_to_address>
### Comment 3
<location path="src/Hooks/Lightwell/navigation/lightwellNavigationPaths.ts" line_range="32" />
<code_context>
export const lightwellNavigationPaths: Record<LightwellDestinationKey, BuildLightwellPath> = {
repositories: ({ rootPath }) => rootPath,
lens: ({ rootPath }) => `${rootPath}/lens`,
+ lensReport: ({ rootPath, reportUUID }) => `${rootPath}/lens/${encodeURIComponent(reportUUID!)}`,
repositoryPackages: ({ rootPath, repoSlug, packagesParams }) =>
appendSearchParams(`${rootPath}/${repoSlug}`, packagesParams ?? { search: '', page: 1 }),
</code_context>
<issue_to_address>
**nitpick (bug_risk):** `lensReport` accepts the same `LightwellNavigationParams` shape as destinations whose parameters are optional, so `navigateTo('lensReport')` compiles and produces a `/lens/undefined` URL because the path builder relies on `reportUUID!` at runtime.
**Triggers:** When a caller invokes the new destination without supplying `reportUUID`.
**Suggested fix:** Make `reportUUID` required for the `lensReport` destination, or validate it before constructing the path.
```suggestion
lensReport: ({ rootPath, reportUUID }) => {
if (!reportUUID) {
throw new Error('Lightwell lens report navigation requires a reportUUID');
}
return `${rootPath}/lens/${encodeURIComponent(reportUUID)}`;
},
```
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: src/Pages/Lightwell/Coverage/hooks/useCoverageReport.ts:15, src/Pages/Lightwell/Coverage/hooks/useCoverageReport.ts:13
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1216 +/- ##
==========================================
- Coverage 63.74% 63.70% -0.04%
==========================================
Files 149 149
Lines 5511 5511
Branches 1077 1077
==========================================
- Hits 3513 3511 -2
Misses 1697 1697
- Partials 301 303 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Quick, non-blocking UX thought on breadcrumbs: Lightwell > Lightwell Lens takes you to the upload form when clicking Lightwell, but I'd naturally expect it to lead to the Repositories page. What do you think about using Lightwell > Lightwell Lens > Scan Result for the breadcrumbs? This aligns with our repo breadcrumbs, keeps Lightwell Lens pointing to the upload form, and Scan Result serves as a clean label so we can avoid messy UUIDs or file names. What do you think? |
katarinazaprazna
left a comment
There was a problem hiding this comment.
Awesome work! ❤️ I left one small UX suggestion (non-blocking), and a note about filename persistence. Let me know what you think about the filename issue. I'm not entirely sure how often users would actually share report results in practice
i've removed the breadcrumbs i added initially since it seems like those will be handled globally (from this PR). i like the approach you've proposed here though! i can either add a temporary breadcrumb for Lightwell > Lightwell Lens > Scan Result or open a separate PR to add a global breadcrumb for this once that other PR is merged? |
|
I see! @xbhouse, I think we can skip adding a temporary breadcrumb here and let it be added once the PR is merged |
Summary
/lens), report page loads completed results (/lens/:reportUUID)Testing steps
/lens/:reportUUID