[Nightshift] Shared investigation detail flyout - #289402
Conversation
github-actions
Bot
added
the
author:actionable-obs
18bb85c to
cfa5e8d
Compare
jasonrhodes
added
release_note:skip
cfa5e8d to
4e738e5
Compare
| @@ -0,0 +1,58 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can we put it inside a SVG file and use EUI's <Icon> component?
| * they may not yet exist mid-run) to `InvestigationState` (which requires both). | ||
| * Defaults are safe: an empty summary shows nothing; empty hypotheses render nothing. | ||
| */ | ||
| function toInvestigationState(inv: GetInvestigationResponse): InvestigationState { |
There was a problem hiding this comment.
Continuing on this type conversion.
I think ultimately we need to have a single complete type that represents investigation on the client. Having nearly identical GetInvestigationResponse and InvestigationState seems redundant. WDYT?
There was a problem hiding this comment.
Let's leave it as a follow up. There is now a bit of mess in terms of type ownership:
GetInvestigationResponseis innightshift_investigationsInvestigationStateis in@kbn/significant-events-schema<FinalResults>that depends onInvestigationStateis in@kbn/investigation-output
Any type unification would require some awkward boundary crossing. I'd first move everything into nightshift_investigations, then type unification would be more straightforward.
There was a problem hiding this comment.
Yeah this makes sense, a small follow up to just re-organize types would be great, and easy to review.
1b6b98e to
0948be0
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One non-blocking note on missing unit coverage for the new components' formatting/bridging helpers. Implementation itself looks correct — status mapping is exhaustive, the newly-exported HypothesisRow/FinalResults prop shapes match, and the subject fallback chain is runtime-safe.
Generated by Claude Reviewer for #289402 · claude · opus · 183.7 AIC · ⌖ 22.7 AIC · ⊞ 5.5K
| </EuiTitle> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
These new components ship with non-trivial pure logic but no unit tests, while the sibling @kbn/investigation-output package establishes a .test.tsx convention (hypothesis_row, evidence_list, etc.). A few behaviors here can regress silently and are cheap to pin down:
formatDurationboundary handling: sub-minute runs collapse to0 min, the 60-minute rollover to1h, and the{hrs}h {rem}mvs{hrs}hbranch.formatDatefallback to theUnknown timestring on an unparseable ISO value.toInvestigationStatedefaultingsummary/hypotheseswhen absent mid-run, andInvestigationRunStatusBadgemapping eachInvestigationStatus.
Worth a small unit test around the formatting/bridging helpers so future edits (or a new status value) fail loudly rather than rendering a wrong string.
0948be0 to
ac56eaf
Compare
Adds `InvestigationDetailFlyout` and `InvestigationRunStatusBadge` to the `nightshift_investigations` plugin, exported from `public/components/`. Both are stateless — no internal data fetching — so any consumer (Nightshift, observability alerts, etc.) owns the query and passes data as props. Hypotheses, recommendations and blind spots render as real content via `HypothesisRow` and `FinalResults` from `@kbn/investigation-output`, which are newly exported from that package (purely additive). Two escape hatches, matching the existing `flyoutMenuProps` / `onClickCapture` pattern, so a consumer can supply app-specific behaviour without this plugin depending on any app: `getQueryHref` for Discover links on evidence, and the pass-through flyout menu props. `toInvestigationState` bridges `GetInvestigationResponse` (where `summary` and `hypotheses` are optional, since they may not exist mid-run) to `InvestigationState` (which requires both). Defaults are safe: an empty summary renders nothing, empty hypotheses render nothing. Adds `observabilityShared` as a required plugin dep for `NightshiftMarkIcon`. The consumer arrives in the next PR in the stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZHFaTqNZLj89CHFr9YQwc
ac56eaf to
e1e1292
Compare
|
@jasonrhodes, this PR increases one or more page-load bundle sizes by 15% or more:
Large bundle size increases can affect page load performance. Consider whether dependencies can be lazy-loaded or code split to reduce the bundle. See the bundle optimization guide for tips. |
|
Pushed a few changes:
|
There was a problem hiding this comment.
Reviewed the shared flyout components. Types, the GetInvestigationResponse → InvestigationState bridge, and the FinalResults/HypothesisRow prop usage all check out. One non-blocking note left inline about missing unit coverage for the new pure date/duration utils.
Generated by Claude Reviewer for #289402 · claude · opus · 202.7 AIC · ⌖ 25.8 AIC · ⊞ 5.5K
Covers the branches flagged in review: formatDate invalid-input fallback, formatDuration sub-minute clamp, 59-min boundary, whole-hour branch, and hours+minutes branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jasonrhodes
left a comment
jasonrhodes
left a comment
There was a problem hiding this comment.
I added some unit tests for utils functions, all the rest LGTM
💛 Build succeeded, but was flaky
Failed CI Steps
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsshared chunks total size
total optimizer output size
Test Failures
History
|
3 participants
Summary
Adds
InvestigationDetailFlyoutandInvestigationRunStatusBadgeto thenightshift_investigationsplugin, exported frompublic/components/.Important
It's probably not possible to see this code in this review, it's been left as a separated PR to isolate the changes for code review instead. Screenshots are included below, or you can run these commits alongside the next PR in this stack to see it all in action.
Both components are stateless — no internal data fetching — so any consumer (Nightshift, observability alerts, etc.) owns the query and passes data as props.
Hypotheses, recommendations and blind spots render as real content via
HypothesisRowandFinalResultsfrom@kbn/investigation-output, which are newly exported from that package (purely additive — it previously exported onlyInvestigationOutput,useInvestigationState,EvidenceListand the evidence-link helpers).Escape hatches
Two optional props, matching the existing
flyoutMenuProps/onClickCapturepattern, so a consumer can supply app-specific behaviour without this plugin depending on any app:getQueryHref— builds Discover hrefs for evidence links inside hypotheses.HypothesisRowalready types this optional, so degrading to non-linked evidence when absent is free.flyoutMenuProps— pass-through toEuiFlyoutfor share URLs, EBT tracking, etc.The type bridge
toInvestigationStatebridgesGetInvestigationResponse(wheresummaryandhypothesesare optional, since they may not exist mid-run) toInvestigationState(which requires both). Defaults are safe: an empty summary renders nothing, empty hypotheses render nothing.Note
@mykolaharmash raised a good question about this on the previous revision of this PR — whether the transformation belongs at fetch time instead, so the rest of the code doesn't need to think about it. Carried forward here and still open. One consideration: the flyout reads nine fields off
GetInvestigationResponse(status,subject,executed_by,created_at,started_at,completed_at,error,investigation_id,summary) and only five feedInvestigationState, so the component needs the full response for its header regardless — moving the transform would make it an additional derived value rather than a replacement, unless the hook returns both.Icon dependency
NightshiftMarkIconis a local copy (public/components/nightshift_mark_icon.tsx) rather than an import from@kbn/observability-shared-plugin.This plugin is
"group": "platform"andobservability_sharedisobservability/private, so importing it trips@kbn/imports/no_group_crossing_*. The mark is a static SVG with no logic, so duplicating ~45 lines was the cheapest fix. The file carries a comment saying to delete it and go back to the shared import once this plugin moves underx-pack/solutions/observability.Worth noting for whenever that move happens: it can't happen in isolation.
significant_eventsis also"group": "platform"and listsnightshiftInvestigationsinoptionalPlugins, and the manifest rule counts optional dependencies (no_group_crossing_manifests.ts:67) — so moving this plugin alone would just invert the same violation. Both need to move together.Testing
The consumer arrives in PR 3 of the stack. To see it rendered, check out
nightshift/homepage-3-app-and-list.