Add Insights (zero-effort issues) #141
Conversation
Signed-off-by: Marek Aufart <[email protected]>
WalkthroughAdds Insights feature surface: routes for Insights list, a new InsightsList page, sidebar navigation to Insights, and an always-visible Insights tab in application details. Extends ReportDto with an optional insights field and updates report queries to translate insights, filtering to zero-effort items when rulesets are present. Minor UI heading added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant InsightsList
participant ContextSelector as SimpleContextSelector
participant ViolationsTable
participant Queries as useAllApplications/useQuery
participant API as Hub API
User->>Router: Navigate to /insights[/applications[/:applicationId]]
Router-->>InsightsList: Render (lazy-loaded)
InsightsList->>ContextSelector: Init with contextKeyFromURL
ContextSelector-->>InsightsList: onChange(context.key)
InsightsList->>Router: navigate(/insights/applications/:key)
InsightsList->>ViolationsTable: Render with { applicationId, insightsMode: true }
ViolationsTable->>Queries: fetch data
alt With rulesets
Queries->>API: fetch rulesets/issues
API-->>Queries: data
Queries-->>ViolationsTable: insights filtered where effort == 0
else Without rulesets
Queries-->>ViolationsTable: use application.insights if present
end
ViolationsTable-->>User: Display Insights rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Code snippet for Insights' incident is not available and that is the same behaviour as current "full" Konveyor UI. Probably might be addressed by UI team. Static report with this PR changes: Full UI: |
|
Putting to draft until deeper check how it works together with static-report generator and kantra&Hub inputs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/pages/application-details/pages/insights/insights.tsx (1)
14-16: Avoid multiple h1s in nested routes; use h2 here for proper heading hierarchy.ApplicationDetails already renders an h1 for the application name (see src/pages/application-details/application-details.tsx, Line 77). Rendering another h1 inside the routed child creates duplicate top-level headings and hurts accessibility. Prefer h2 (or inherit the Tabs region heading).
Apply this diff:
- <Title headingLevel="h1" size="lg" className="pf-v5-u-mb-md"> + <Title headingLevel="h2" size="lg" className="pf-v5-u-mb-md"> Insights (Zero Effort Issues) </Title>src/pages/insights-list/insights-list.tsx (6)
26-31: Double-check UX for the base “/insights” route (applicationId = undefined).With applicationId undefined, ViolationsTable shows an “empty/choose app” state. If that’s intentional, great. If not, consider redirecting “/insights” to “/insights/applications” (all apps) or preselecting the current context to avoid a blank table.
Example (optional) redirect:
-import React from "react"; +import React, { useEffect } from "react"; @@ const applicationId = matchInsightsPage ? undefined : matchAllApplicationsPage ? ALL_APPLICATIONS_ID : matchSingleApplicationPage?.params.applicationId; + useEffect(() => { + if (matchInsightsPage) { + navigate("/insights/applications"); + } + }, [matchInsightsPage, navigate]);
34-36: URL-encode context keys on navigation.If a context key contains characters that are unsafe in URLs, navigation can break. Encode it defensively.
- const onContextChange = (context: Context) => { - navigate("/insights/applications/" + context.key); - }; + const onContextChange = (context: Context) => { + navigate(`/insights/applications/${encodeURIComponent(context.key)}`); + };
45-49: Add an accessible label to the selector.Provide an aria-label so screen readers can announce purpose without relying on surrounding text in the Toolbar.
<SimpleContextSelector contextKeyFromURL={applicationId} - onChange={onContextChange} + onChange={onContextChange} + props={{ "aria-label": "Application selector" }} />
55-60: Prefer paragraph semantics for description; avoid using for body copy.Small text is a styling hint, not ideal for main descriptive content. A paragraph is more semantic and consistent with PF guidance.
- <Text component="small"> - This report provides a concise summary of all insights identified - issues with zero effort. - </Text> + <Text component="p"> + This report summarizes zero‑effort insights across your applications. + </Text>
17-18: Unify ViolationsTable import path across files.Elsewhere (application-details Insights page) ViolationsTable is imported from "@app/shared/components". Consider using the same barrel to keep imports consistent.
-import { ViolationsTable } from "@app/shared/components/violations-table"; +import { ViolationsTable } from "@app/shared/components";
63-63: Use boolean prop shorthand.Cleaner and idiomatic for React boolean props.
- <ViolationsTable applicationId={applicationId} insightsMode={true} /> + <ViolationsTable applicationId={applicationId} insightsMode />src/api/report.ts (1)
1-11: Clarify the semantics of ReportDto.insights.Is this always “zero‑effort only” when provided directly by the hub, or “all insights”? Adding a short doc comment will prevent confusion since the query layer filters to zero‑effort for rulesets, but not for raw insights.
You could annotate the field like:
/** Insights reported by the hub (may be all insights; UI may filter to zero‑effort). */ insights?: IssueDto[];src/queries/report.ts (1)
185-187: Zero‑effort filter applied only for rulesets; confirm intended parity with hub-provided insights.Currently you filter to effort === 0 only when rulesets are present, while hub-provided
a.insightsare passed through unfiltered. If the product expectation is “Insights = zero‑effort everywhere,” consider filtering both branches for consistency. If the hub already guarantees zero‑effort, keep as-is but document it.Option A (filter both branches):
- const insights: IssueProcessed[] = a.rulesets ? - issuesFromRulesetsDto(a.id, a.files, a.rulesets, true).filter(insight => insight.effort === 0) : - (a.insights ? issuesFromIssuesDto(a.id, a.insights) : [] as IssueProcessed[]); + const insights: IssueProcessed[] = a.rulesets + ? issuesFromRulesetsDto(a.id, a.files, a.rulesets, true).filter(i => i.effort === 0) + : (a.insights ? issuesFromIssuesDto(a.id, a.insights).filter(i => i.effort === 0) : [] as IssueProcessed[]);Option B (extract helper for reuse/readability):
// place near other helpers const zeroEffortOnly = <T extends { effort: number }>(items: T[]) => items.filter(i => i.effort === 0);- const insights: IssueProcessed[] = a.rulesets ? - issuesFromRulesetsDto(a.id, a.files, a.rulesets, true).filter(insight => insight.effort === 0) : - (a.insights ? issuesFromIssuesDto(a.id, a.insights) : [] as IssueProcessed[]); + const insights: IssueProcessed[] = a.rulesets + ? zeroEffortOnly(issuesFromRulesetsDto(a.id, a.files, a.rulesets, true)) + : (a.insights ? zeroEffortOnly(issuesFromIssuesDto(a.id, a.insights)) : [] as IssueProcessed[]);src/layout/sidebar.tsx (1)
41-54: Insights nav addition is consistent; minor readability nit on path compositionThe new Insights link mirrors Issues/Dependencies and respects currentContext. For readability, prefer a template literal over string concatenation when appending the key.
Apply this diff:
- : "/insights/applications/" + currentContext.key + : `/insights/applications/${currentContext.key}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/Routes.tsx(2 hunks)src/api/report.ts(1 hunks)src/layout/sidebar.tsx(1 hunks)src/pages/application-details/application-details.tsx(1 hunks)src/pages/application-details/pages/insights/insights.tsx(2 hunks)src/pages/insights-list/index.ts(1 hunks)src/pages/insights-list/insights-list.tsx(1 hunks)src/queries/report.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/queries/report.ts (1)
src/models/api-enriched.tsx (1)
IssueProcessed(17-33)
src/Routes.tsx (2)
src/pages/insights-list/index.ts (1)
InsightsList(1-1)src/pages/insights-list/insights-list.tsx (1)
InsightsList(19-67)
src/pages/insights-list/insights-list.tsx (2)
src/context/simple-context.tsx (2)
Context(17-20)SimpleContextSelector(74-124)src/shared/components/violations-table/violations-table.tsx (1)
ViolationsTable(137-736)
🔇 Additional comments (5)
src/pages/application-details/pages/insights/insights.tsx (1)
4-4: Import looks good.Adding Title from PatternFly is appropriate for introducing a page-level heading.
src/pages/application-details/application-details.tsx (1)
47-50: Always-on Insights tab: good for discoverability; verify empty-state experience.Nice addition. Ensure the routed page provides a helpful empty state when there are no insights for an app. Also, since this layout renders an h1 for the application name (Line 77), the nested Insights page should avoid another h1 (suggested fix provided in insights.tsx).
src/Routes.tsx (2)
9-9: Lazy import for InsightsList aligns with default export wiringImport path and lazy usage match the new index default export; this should code-split cleanly like the other list pages.
55-70: Routes for Insights mirror Issues/Dependencies correctlyThree routes cover root, all applications, and per-application views and are consistent with the sidebar links and the InsightsList useMatch logic. Good placement before Dependencies.
src/pages/insights-list/index.ts (1)
1-1: Default re-export looks goodThis enables
lazy(() => import("./pages/insights-list"))to resolve properly without changing the underlying component’s named export.
|
Test locally with Hub generated data as well as kantra build with this code, both look to work. Ready for review. |
|
I'm not sure about the licence compliance, might need something but probably in a different PR, this didn't change any dependency structure. (fossa page with details is not available for me) |
There was a problem hiding this comment.
Changes all look normal.
My main question is if the effort===0 issues will show up in the issues page and the insights page. The useAllApplications() hook adds a filter for the effort, but the issues calculation doesn't do anything different. It may be ok, but that depends on how issuesFromRulesetsDto works.
3 participants
Fixes: #138
Summary by CodeRabbit