feat: SSE console updates from Korrel8r#231
Open
alanconway wants to merge 2 commits intoopenshift:mainfrom
Open
feat: SSE console updates from Korrel8r#231alanconway wants to merge 2 commits intoopenshift:mainfrom
alanconway wants to merge 2 commits intoopenshift:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alanconway The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThis PR introduces AI Agent navigation support for Korrel8r by updating the OpenAPI contract to formalize console control (deprecating the old endpoint spelling, restructuring Console payloads, and adding streaming event support), implementing client-side agent communication with retry logic, wiring agent state through Redux, and adding UI components for toggling the agent while maintaining backward compatibility. ChangesAI Agent Integration & OpenAPI Contract Alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
web/src/components/Korrel8rPanel.tsx (2)
63-63: Type annotation may not reflect actual Redux state.The
agentErrorselector could returnundefinedwhen the state key hasn't been set. Consider updating the type to reflect this:- const agentError: string = useSelector((state: State) => state.plugins?.tp?.get('agentError')); + const agentError: string | undefined = useSelector((state: State) => state.plugins?.tp?.get('agentError'));The current falsy checks (line 184, 190) handle this safely at runtime, so this is just a type accuracy improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Korrel8rPanel.tsx` at line 63, The agentError selector is typed too narrowly as string even though state.plugins?.tp?.get('agentError') can be undefined; update the type on the agentError constant (the useSelector call that reads state.plugins?.tp?.get('agentError')) to reflect string | undefined (or the appropriate union) so the TypeScript type matches runtime behavior and the existing falsy checks around agentError remain valid.
180-196: Good addition of connection status indicator.The status button properly reflects agent connectivity using
agentErrorfrom Redux. The use of PatternFly CSS variables for icon colors is appropriate.Minor i18n concern: String concatenation on line 185 bypasses proper interpolation. Consider using interpolation for better localization support:
- ? t('Correlation service error: ') + agentError + ? t('Correlation service error: {{error}}', { error: agentError })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/Korrel8rPanel.tsx` around lines 180 - 196, Replace the string concatenation when building the Tooltip content with a localized interpolation using the t function so translations can reorder/format the agent error; specifically, update the Tooltip content expression that currently uses agentError and t('Correlation service error: ') to call t with a placeholder (e.g., t('Correlation service error: {{error}}', { error: agentError })) or the project's established interpolation pattern; change only the Tooltip content construction near the agentError usage so Tooltip, agentError, and the t(...) call are used with interpolation instead of simple string concatenation.web/src/hooks/useKorrel8r.ts (1)
53-58: Ref pattern for stable SSE callbacks.Using a ref to hold
navigateToQueryandsearchwithout triggering effect re-runs is a valid pattern. Consider adding a brief comment explaining why the effect at lines 55-58 has no dependency array (intentionally updates ref on every render):// Ref for values used in SSE callback that shouldn't trigger reconnection. const ref = React.useRef({ navigateToQuery, search }); + // Intentionally no deps - update ref on every render to keep callbacks current React.useEffect(() => { ref.current.navigateToQuery = navigateToQuery; ref.current.search = search; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/useKorrel8r.ts` around lines 53 - 58, Add a short inline comment above the ref and the useEffect explaining that ref (created via React.useRef) holds navigateToQuery and search so SSE callbacks remain stable and the empty dependency omission is intentional to update ref on every render; also add an eslint-disable-next-line react-hooks/exhaustive-deps on the effect if the linter complains to make the intent explicit.web/src/redux-actions.ts (1)
48-54: Consider handling missingqueryStrinapiSearch.If
search.queryStris undefined or empty, the function createsqueries: [undefined]orqueries: [''], which may cause issues downstream in the API call. You might want to validate or filter:export const apiSearch = (search: Search): api.Search => { - const start: api.Start = { queries: [search.queryStr] }; + const start: api.Start = { queries: search.queryStr ? [search.queryStr] : [] }; if (search.searchType === SearchType.Goal) { return { goals: { start, goals: [search.goal] } }; } return { neighbors: { start, depth: search.depth } }; };Alternatively, this may already be guarded at call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/redux-actions.ts` around lines 48 - 54, In apiSearch, validate that search.queryStr is a non-empty string before building api.Start; if it is falsy or empty, throw a descriptive error (e.g., "apiSearch: missing queryStr") or otherwise return a safe api.Search variant; update the apiSearch function to check search.queryStr (used when constructing api.Start.queries) and either filter it out / use an empty queries array or throw so downstream API calls never receive [undefined] or [''].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/Korrel8rPanel.tsx`:
- Line 63: The agentError selector is typed too narrowly as string even though
state.plugins?.tp?.get('agentError') can be undefined; update the type on the
agentError constant (the useSelector call that reads
state.plugins?.tp?.get('agentError')) to reflect string | undefined (or the
appropriate union) so the TypeScript type matches runtime behavior and the
existing falsy checks around agentError remain valid.
- Around line 180-196: Replace the string concatenation when building the
Tooltip content with a localized interpolation using the t function so
translations can reorder/format the agent error; specifically, update the
Tooltip content expression that currently uses agentError and t('Correlation
service error: ') to call t with a placeholder (e.g., t('Correlation service
error: {{error}}', { error: agentError })) or the project's established
interpolation pattern; change only the Tooltip content construction near the
agentError usage so Tooltip, agentError, and the t(...) call are used with
interpolation instead of simple string concatenation.
In `@web/src/hooks/useKorrel8r.ts`:
- Around line 53-58: Add a short inline comment above the ref and the useEffect
explaining that ref (created via React.useRef) holds navigateToQuery and search
so SSE callbacks remain stable and the empty dependency omission is intentional
to update ref on every render; also add an eslint-disable-next-line
react-hooks/exhaustive-deps on the effect if the linter complains to make the
intent explicit.
In `@web/src/redux-actions.ts`:
- Around line 48-54: In apiSearch, validate that search.queryStr is a non-empty
string before building api.Start; if it is falsy or empty, throw a descriptive
error (e.g., "apiSearch: missing queryStr") or otherwise return a safe
api.Search variant; update the apiSearch function to check search.queryStr (used
when constructing api.Start.queries) and either filter it out / use an empty
queries array or throw so downstream API calls never receive [undefined] or
[''].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8915985c-7290-4b78-b3a8-d213202392df
⛔ Files ignored due to path filters (2)
web/src/korrel8r/client/sdk.gen.tsis excluded by!**/*.gen.*web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (14)
korrel8r/korrel8r-openapi.yamlweb/locales/en/plugin__troubleshooting-panel-console-plugin.jsonweb/package.jsonweb/src/__tests__/types.spec.tsweb/src/components/Korrel8rPanel.tsxweb/src/hooks/useKorrel8r.tsweb/src/hooks/useNavigateToQuery.tsweb/src/hooks/usePopover.tsxweb/src/hooks/useTroubleshootingPanel.tsxweb/src/korrel8r-client.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.tsweb/src/redux-actions.tsweb/src/redux-reducers.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/hooks/useKorrel8r.ts`:
- Around line 36-40: The current body always includes search: apiSearch(search)
even when the panel is closed or search.queryStr is empty, causing blank/stale
queries to be sent; change the body construction so that search is only added
when isOpen && search?.queryStr is truthy (keep view always), e.g., build body =
{ view, ...(isOpen && search?.queryStr ? { search: apiSearch(search) } : {}) }
before calling retryWithBackoff; reference symbols: agentAllowed, view, isOpen,
search, apiSearch, retryWithBackoff.
- Around line 69-83: The consoleEvents callback currently calls
Query.parse(event.view) and reduxSearch(event.search) directly which can throw
and bubble up to consoleEvents; wrap each external-parse step in a local
try/catch and validate results before applying (e.g., call Query.parse inside
try/catch and only call navigateToQueryRef.current with a valid Query, and call
reduxSearch inside try/catch and only
dispatch(setSearch(...))/dispatch(openTP()) when reduxSearch returns a valid
object), and on parse/validation errors log the malformed payload (using
console.debug or process logger) and return early so the SSE session is not
closed by an exception.
In `@web/src/redux-actions.ts`:
- Around line 60-76: reduxSearch currently lets search.neighbors overwrite
search.goals; instead detect how many top-level variants are set (e.g.,
search.goals and search.neighbors) and only build a Search when exactly one
variant is present. In the reduxSearch function check presence of each variant
(search.goals, search.neighbors), if count !== 1 return undefined; if exactly
one, construct the Search from that branch (use queryStr from
start?.queries?.[0], set searchType and variant-specific fields like goal or
depth) and return it. Ensure the result variable is typed to allow undefined and
remove the last-one-wins logic.
In `@web/src/redux-reducers.ts`:
- Around line 18-23: The initial state currently sets agentAllowed: true in the
reducer's ImmutableMap (the default store returned by the reducer), which
opt-ins users to agent mirroring; change the default to false by updating the
return value that creates the state (the ImmutableMap containing isOpen, search,
agentAllowed, agentConnected) so agentAllowed is initialized to false; also scan
for any tests or initialization logic that rely on the previous default and
update them accordingly (look for uses of agentAllowed in
reducers/selectors/tests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 48bf6374-5fcb-4573-8b02-5c7289ebf447
⛔ Files ignored due to path filters (3)
web/package-lock.jsonis excluded by!**/package-lock.jsonweb/src/korrel8r/client/sdk.gen.tsis excluded by!**/*.gen.*web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (16)
korrel8r/korrel8r-openapi.yamlweb/locales/en/plugin__troubleshooting-panel-console-plugin.jsonweb/package.jsonweb/src/__tests__/types.spec.tsweb/src/components/Korrel8rPanel.tsxweb/src/components/Popover.tsxweb/src/hooks/useKorrel8r.tsweb/src/hooks/useLocationQuery.tsweb/src/hooks/useNavigateToQuery.tsweb/src/hooks/usePopover.tsxweb/src/hooks/useTroubleshootingPanel.tsxweb/src/korrel8r-client.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.tsweb/src/redux-actions.tsweb/src/redux-reducers.ts
✅ Files skipped from review due to trivial changes (6)
- web/package.json
- web/src/hooks/useTroubleshootingPanel.tsx
- web/src/hooks/useNavigateToQuery.ts
- web/src/tests/types.spec.ts
- web/locales/en/plugin__troubleshooting-panel-console-plugin.json
- web/src/korrel8r/client/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/components/Korrel8rPanel.tsx
- web/src/korrel8r-client.ts
- web/src/hooks/usePopover.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
web/src/hooks/useKorrel8r.ts (1)
36-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly send
searchwhen it matches the visible panel state.Line 39 still serializes
searchwheneverviewexists, so a closed panel can resend stale search state and an empty query can still be pushed upstream. This should stay conditional on the panel being open andsearch.queryStrbeing non-empty.Suggested fix
- const body = { view, search: apiSearch(search) }; + const body = { + view, + ...(isOpen && search?.queryStr ? { search: apiSearch(search) } : {}), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/useKorrel8r.ts` around lines 36 - 40, The current logic always includes search in the request body when view exists, allowing a closed panel or empty query to resend stale/empty search state; update the body construction in useKorrel8r so that search is only added when the panel is open (isOpen true) and search?.queryStr is non-empty — e.g., build body = { view, ...(isOpen && search?.queryStr ? { search: apiSearch(search) } : {}) } before calling retryWithBackoff; reference the body variable, apiSearch(search), and the isOpen/search?.queryStr checks to locate where to change the serialization.
🧹 Nitpick comments (1)
web/src/hooks/useTroubleshootingPanel.tsx (1)
39-41: 💤 Low valueRedundant dependency in useEffect.
openis already included ingetActions's dependency array (line 35), so including it again in the useEffect dependency array is unnecessary. The effect will re-run whengetActionschanges, which already happens whenopenchanges.Suggested simplification
useEffect(() => { setActions(getActions()); - }, [open, getActions]); + }, [getActions]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/useTroubleshootingPanel.tsx` around lines 39 - 41, The useEffect that calls setActions(getActions()) has an unnecessary dependency: open is already included in the dependency array of getActions, so remove open from useEffect's dependency list and keep only getActions; i.e., update the effect that references setActions and getActions so it depends solely on getActions to avoid redundant re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/Korrel8rPanel.tsx`:
- Around line 166-170: The Button currently uses the accessibility-only prop
isAriaDisabled={!search?.queryStr} which does not block clicks and allows
onClick={() => refetch()} to fire; update the Button props to use
isDisabled={!search?.queryStr} instead of isAriaDisabled so the control is truly
non-interactive when there is no search query, preventing unintended refetch()
calls (locate the Button component rendering around Korrel8rPanel where onClick
calls refetch).
In `@web/src/korrel8r-client.ts`:
- Around line 178-189: The code currently calls onConnect() before ensuring the
Korrel8r stream is valid, which can report a connection when an HTTP error makes
result.stream unusable; fix by calling sdkConsoleEvents with throwOnError: true
(e.g., await sdkConsoleEvents({ client: korrel8rClient, throwOnError: true }))
so errors are thrown instead of returning an invalid result, and only invoke
onConnect() after verifying result.stream is present/iterable (or after
successfully receiving the first event from result.stream while still respecting
controller.signal) to avoid transient connected-state reporting; update calls
referencing createClient, sdkConsoleEvents, onConnect, result.stream, and
controller.signal accordingly.
In `@web/src/korrel8r/types.ts`:
- Around line 65-70: The mapping in Constraint.static fromAPI currently treats 0
as falsy and converts {limit: 0} to undefined; change the assignment for the
limit field to use the nullish coalescing operator (e.g., limit:
constraint.limit ?? undefined) so that 0 is preserved, and apply the same fix in
the Constraint.prototype.toAPI (or toAPI) mapping to ensure round-tripping
preserves explicit 0 values; update the constructors/users of Constraint if they
rely on the previous falsy behavior.
In `@web/src/redux-actions.ts`:
- Line 61: The variable declaration "let result: Search = undefined;" assigns
undefined to a non-optional Search type; change the type annotation for the
variable named result to allow undefined (e.g., let result: Search | undefined)
or initialize it with a valid Search value so the assignment matches the
declared type; update the declaration of result in redux-actions.ts accordingly.
---
Duplicate comments:
In `@web/src/hooks/useKorrel8r.ts`:
- Around line 36-40: The current logic always includes search in the request
body when view exists, allowing a closed panel or empty query to resend
stale/empty search state; update the body construction in useKorrel8r so that
search is only added when the panel is open (isOpen true) and search?.queryStr
is non-empty — e.g., build body = { view, ...(isOpen && search?.queryStr ? {
search: apiSearch(search) } : {}) } before calling retryWithBackoff; reference
the body variable, apiSearch(search), and the isOpen/search?.queryStr checks to
locate where to change the serialization.
---
Nitpick comments:
In `@web/src/hooks/useTroubleshootingPanel.tsx`:
- Around line 39-41: The useEffect that calls setActions(getActions()) has an
unnecessary dependency: open is already included in the dependency array of
getActions, so remove open from useEffect's dependency list and keep only
getActions; i.e., update the effect that references setActions and getActions so
it depends solely on getActions to avoid redundant re-renders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3a321b8d-7a63-4640-be79-b61186c4e501
⛔ Files ignored due to path filters (3)
web/package-lock.jsonis excluded by!**/package-lock.jsonweb/src/korrel8r/client/sdk.gen.tsis excluded by!**/*.gen.*web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (16)
korrel8r/korrel8r-openapi.yamlweb/locales/en/plugin__troubleshooting-panel-console-plugin.jsonweb/package.jsonweb/src/__tests__/types.spec.tsweb/src/components/Korrel8rPanel.tsxweb/src/components/Popover.tsxweb/src/hooks/useKorrel8r.tsweb/src/hooks/useLocationQuery.tsweb/src/hooks/useNavigateToQuery.tsweb/src/hooks/usePopover.tsxweb/src/hooks/useTroubleshootingPanel.tsxweb/src/korrel8r-client.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.tsweb/src/redux-actions.tsweb/src/redux-reducers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/package.json
- web/src/hooks/useNavigateToQuery.ts
I would go with this icon as per Branding guidelines for all things AI related. I left you some comments in Slack.
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
Author
|
I'm changing the AI stuff again to follow the explicit "ask first" pattern, with an option to "don't ask again". Simplifies the UI since the user is presented with a question automatically the first time we try to interact with the agent. |
Update to latest korrel8r API spec, minor changes to types.
Contributor
PeterYurkovich
left a comment
PeterYurkovich
left a comment
There was a problem hiding this comment.
Apologies for the long time to review
As a more general question I am wondering if we should add some AI enabled flag onto the UIPlugin CR for the troubleshooting panel so a cluster admin can dictate it's usage (this would require changes in obo and adding a feature flag for the troubleshooting panel)
| cancel: () => void; | ||
| } | ||
|
|
||
| export const setConsole = (body: Console): CancellablePromise<void> => { |
Contributor
There was a problem hiding this comment.
The SSE code feels quite divergent from the REST functions. Could we split into a separate file?
Allows console to send state and receive requests to/from an AI agent. Console uses korrel8r REST API, agent uses MCP API. - Menu with AI icon allows enable/disable, shows connection status. - Subscribe to console updates from AI agent via korrel8r. - Always show troubleshooting panel, even if korrel8r is unavailable - it may become available.
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@alanconway: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/src/korrel8r-client.ts (1)
178-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly report “connected” after the stream is actually usable.
This path still omits error-throwing and calls
onConnect()before provingresult.streamexists, so HTTP failures can briefly flip the UI to connected and then immediately error. MoveonConnect()behind a successful stream check and make the SDK call fail hard on non-2xx responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/korrel8r-client.ts` around lines 178 - 186, The code calls onConnect() before ensuring the SDK returned a usable stream and doesn’t surface non-2xx HTTP failures; update the logic around createClient/sdkConsoleEvents so you first call sdkConsoleEvents with the requestWrapper that throws on non-2xx responses (or check the response status) and verify that result.stream is an async iterable before proceeding, only invoking onConnect() after confirming result.stream exists and is usable; adjust requestWrapper or the sdkConsoleEvents call to "fail hard" on non-2xx responses and move the onConnect() call to after the successful stream check (references: createClient, requestWrapper, sdkConsoleEvents, result.stream, onConnect).web/src/korrel8r/types.ts (1)
65-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve an explicit
limit: 0when converting constraints.
|| undefinedstill drops0on both read and write, so{ limit: 0 }does not round-trip. This needs?? undefinedin both places.Suggested fix
static fromAPI(constraint: api.Constraint): Constraint | undefined { if (!constraint) return undefined; return new Constraint({ start: parseDate(constraint.start), end: parseDate(constraint.end), - limit: constraint?.limit || undefined, + limit: constraint?.limit ?? undefined, }); } toAPI(): api.Constraint { return { start: this?.start?.toISOString() || undefined, end: this?.end?.toISOString() || undefined, - limit: this?.limit || undefined, + limit: this?.limit ?? undefined, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/korrel8r/types.ts` around lines 65 - 78, The conversion loses an explicit limit: 0 because the code uses the || operator which treats 0 as falsy; update both Constraint.fromAPI and Constraint.toAPI to use the nullish coalescing operator (?? undefined) when mapping limit so that 0 is preserved (i.e., replace occurrences of constraint?.limit || undefined and this?.limit || undefined with constraint?.limit ?? undefined and this?.limit ?? undefined respectively).
🧹 Nitpick comments (5)
web/src/hooks/useLocationQuery.ts (1)
46-58: ⚡ Quick winMove side effects out of the render phase.
console.warn(line 55) and the relatedsetLastErrorcalls execute during render. In React Strict Mode, render functions are invoked twice, which can cause duplicate warnings despite the deduplication logic. Moving the logging and error-state sync into auseEffectaligns with React's intended data flow and avoids Strict Mode quirks.♻️ Suggested refactor using useEffect
export const useLocationQuery = (): Query | undefined => { const domains = useDomains(); const location = useBrowserLocation(); const [lastError, setLastError] = useState(''); - try { - const query = domains.linkToQuery(new URIRef(location.pathname + location.search)); - if (lastError) setLastError(''); - return query; - } catch (err) { - const errStr = String(err); - if (errStr !== lastError) { - // eslint-disable-next-line no-console - console.warn(`korrel8r useLocationQuery: ${errStr}`); - setLastError(errStr); - } - } + + let query: Query | undefined; + let currentError = ''; + try { + query = domains.linkToQuery(new URIRef(location.pathname + location.search)); + } catch (err) { + currentError = String(err); + } + + useEffect(() => { + if (currentError !== lastError) { + if (currentError) { + // eslint-disable-next-line no-console + console.warn(`korrel8r useLocationQuery: ${currentError}`); + } + setLastError(currentError); + } + }, [currentError, lastError]); + + return query; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/useLocationQuery.ts` around lines 46 - 58, The current try/catch in useLocationQuery runs console.warn and setLastError during render (variables: lastError, setLastError, domains.linkToQuery, URIRef), so move those side effects into a useEffect: have the render phase only compute/return the query (or null/undefined on error) and capture the error value locally, then in a useEffect dependent on that error value update setLastError and call console.warn (or process the error) so logging and state updates run after render and avoid Strict Mode double-invocation issues.web/src/__tests__/types.spec.ts (1)
52-56: ⚡ Quick winAdd a
limit: 0round-trip case here.This matrix now covers
limit, but it still misses the falsy boundary value that currently regresses inConstraint.fromAPI()/toAPI(). Adding{ clientC: { limit: 0 }, typesC: { limit: 0 } }will lock in the fix once the serializer is corrected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/__tests__/types.spec.ts` around lines 52 - 56, Add a test case to the it.each matrix in types.spec.ts to include the falsy boundary value by adding an entry for limit: 0 so the round-trip via Constraint.fromAPI() and toAPI() is exercised; specifically add a case object equivalent to { clientC: { limit: 0 }, typesC: { limit: 0 } } alongside the existing limit cases to ensure the serializer correctly preserves zero.korrel8r/korrel8r-openapi.yaml (2)
339-342: 💤 Low valueConsider specifying empty object schema for 200 response.
The 200 response for
PUT /consolereturnstype: objectwithout properties. If the response is always empty, consider addingadditionalProperties: falseto make the contract explicit. If it may contain data in the future, the current permissive schema is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@korrel8r/korrel8r-openapi.yaml` around lines 339 - 342, The 200 response schema for the PUT /console operation currently uses a permissive "type: object" with no properties; to make an explicit empty response contract, update the response schema for the PUT /console 200 to include "additionalProperties: false" (keeping type: object) so the spec clearly disallows any properties, or alternatively add explicit properties if you expect fields later; locate the 200 response block in the OpenAPI spec where "type: object" is defined and add the additionalProperties: false key there.
688-706: ⚡ Quick winMutual exclusivity of
goalsandneighborsnot enforced in schema.The description states "Set exactly one of 'goals' ... or 'neighbors'" but the schema allows both, neither, or either. Consider using
oneOfto enforce this constraint at the schema level, which would enable client-side validation and clearer generated types.♻️ Suggested schema constraint
Search: description: > Correlation search parameters. Set exactly one of 'goals' (targeted search to specific classes) or 'neighbors' (open-ended exploration to a depth). type: object + oneOf: + - required: [goals] + not: + required: [neighbors] + - required: [neighbors] + not: + required: [goals] properties: goals:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@korrel8r/korrel8r-openapi.yaml` around lines 688 - 706, The Search schema currently allows both or neither of goals and neighbors although its description requires exactly one; update the OpenAPI component for Search to enforce mutual exclusivity by replacing the separate properties block with a oneOf that references the existing Goals and Neighbors schemas (using the same $ref targets), e.g. provide one schema option with goals: { $ref: "#/components/schemas/Goals" } and another with neighbors: { $ref: "#/components/schemas/Neighbors" } so generated clients and validators will require exactly one of goals or neighbors; keep the existing descriptions and x-oapi-codegen-extra-tags on the referenced schemas.web/src/hooks/useKorrel8r.ts (1)
68-78: 💤 Low valueVariable shadowing: inner
searchshadows outer scope.Line 73 declares
const searchwhich shadows thesearchvariable from line 29. While functionally correct here, this can cause confusion during maintenance. Consider renaming tonextSearchor similar.Also, the
asynckeyword on line 68 is unnecessary since the callback doesn't useawait.♻️ Suggested cleanup
return consoleEvents( - async (event) => { + (event) => { if (event.view) { navigateToQueryRef.current(Query.parse(event.view), null); } if (event.search) { - const search = reduxSearch(event.search); - if (search) { - dispatch(setSearch(search)); + const nextSearch = reduxSearch(event.search); + if (nextSearch) { + dispatch(setSearch(nextSearch)); dispatch(openTP()); } } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/useKorrel8r.ts` around lines 68 - 78, Rename the inner const search to avoid shadowing the outer search (e.g., call it nextSearch) and update its usages (reduxSearch -> nextSearch result) in the event callback in useKorrel8r.ts; also remove the unnecessary async keyword from the anonymous event handler since it doesn't use await. Locate the handler that calls navigateToQueryRef.current(Query.parse(event.view)), reduxSearch(event.search), dispatch(setSearch(...)) and dispatch(openTP()) and apply the renaming and async removal there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/korrel8r-client.ts`:
- Around line 193-194: The exponential backoff wait using await new Promise((r)
=> setTimeout(r, delay)) is not abortable and lets cancel() block until the
timeout elapses; change both retry loops to use an abortable delay that races
setTimeout against an AbortSignal (or rejects when the provided signal is
aborted) so the loop can exit immediately on cancellation, update the cancel()
path to call abort() on that signal (or otherwise resolve the delay promise),
and replace the two occurrences that manipulate delay and maxDelay (the backoff
logic around delay/maxDelay) with calls to this abortableDelay helper so
reconnect timers don't fire after unmount/disable.
---
Duplicate comments:
In `@web/src/korrel8r-client.ts`:
- Around line 178-186: The code calls onConnect() before ensuring the SDK
returned a usable stream and doesn’t surface non-2xx HTTP failures; update the
logic around createClient/sdkConsoleEvents so you first call sdkConsoleEvents
with the requestWrapper that throws on non-2xx responses (or check the response
status) and verify that result.stream is an async iterable before proceeding,
only invoking onConnect() after confirming result.stream exists and is usable;
adjust requestWrapper or the sdkConsoleEvents call to "fail hard" on non-2xx
responses and move the onConnect() call to after the successful stream check
(references: createClient, requestWrapper, sdkConsoleEvents, result.stream,
onConnect).
In `@web/src/korrel8r/types.ts`:
- Around line 65-78: The conversion loses an explicit limit: 0 because the code
uses the || operator which treats 0 as falsy; update both Constraint.fromAPI and
Constraint.toAPI to use the nullish coalescing operator (?? undefined) when
mapping limit so that 0 is preserved (i.e., replace occurrences of
constraint?.limit || undefined and this?.limit || undefined with
constraint?.limit ?? undefined and this?.limit ?? undefined respectively).
---
Nitpick comments:
In `@korrel8r/korrel8r-openapi.yaml`:
- Around line 339-342: The 200 response schema for the PUT /console operation
currently uses a permissive "type: object" with no properties; to make an
explicit empty response contract, update the response schema for the PUT
/console 200 to include "additionalProperties: false" (keeping type: object) so
the spec clearly disallows any properties, or alternatively add explicit
properties if you expect fields later; locate the 200 response block in the
OpenAPI spec where "type: object" is defined and add the additionalProperties:
false key there.
- Around line 688-706: The Search schema currently allows both or neither of
goals and neighbors although its description requires exactly one; update the
OpenAPI component for Search to enforce mutual exclusivity by replacing the
separate properties block with a oneOf that references the existing Goals and
Neighbors schemas (using the same $ref targets), e.g. provide one schema option
with goals: { $ref: "#/components/schemas/Goals" } and another with neighbors: {
$ref: "#/components/schemas/Neighbors" } so generated clients and validators
will require exactly one of goals or neighbors; keep the existing descriptions
and x-oapi-codegen-extra-tags on the referenced schemas.
In `@web/src/__tests__/types.spec.ts`:
- Around line 52-56: Add a test case to the it.each matrix in types.spec.ts to
include the falsy boundary value by adding an entry for limit: 0 so the
round-trip via Constraint.fromAPI() and toAPI() is exercised; specifically add a
case object equivalent to { clientC: { limit: 0 }, typesC: { limit: 0 } }
alongside the existing limit cases to ensure the serializer correctly preserves
zero.
In `@web/src/hooks/useKorrel8r.ts`:
- Around line 68-78: Rename the inner const search to avoid shadowing the outer
search (e.g., call it nextSearch) and update its usages (reduxSearch ->
nextSearch result) in the event callback in useKorrel8r.ts; also remove the
unnecessary async keyword from the anonymous event handler since it doesn't use
await. Locate the handler that calls
navigateToQueryRef.current(Query.parse(event.view)), reduxSearch(event.search),
dispatch(setSearch(...)) and dispatch(openTP()) and apply the renaming and async
removal there.
In `@web/src/hooks/useLocationQuery.ts`:
- Around line 46-58: The current try/catch in useLocationQuery runs console.warn
and setLastError during render (variables: lastError, setLastError,
domains.linkToQuery, URIRef), so move those side effects into a useEffect: have
the render phase only compute/return the query (or null/undefined on error) and
capture the error value locally, then in a useEffect dependent on that error
value update setLastError and call console.warn (or process the error) so
logging and state updates run after render and avoid Strict Mode
double-invocation issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 74dc10a4-765d-401c-85f4-985db12d156e
⛔ Files ignored due to path filters (2)
web/src/korrel8r/client/sdk.gen.tsis excluded by!**/*.gen.*web/src/korrel8r/client/types.gen.tsis excluded by!**/*.gen.*
📒 Files selected for processing (15)
korrel8r/korrel8r-openapi.yamlweb/locales/en/plugin__troubleshooting-panel-console-plugin.jsonweb/src/__tests__/types.spec.tsweb/src/components/AIExperienceIcon.tsxweb/src/components/AgentMenu.tsxweb/src/components/Korrel8rPanel.tsxweb/src/hooks/useKorrel8r.tsweb/src/hooks/useLocationQuery.tsweb/src/hooks/usePopover.tsxweb/src/hooks/useTroubleshootingPanel.tsxweb/src/korrel8r-client.tsweb/src/korrel8r/client/index.tsweb/src/korrel8r/types.tsweb/src/redux-actions.tsweb/src/redux-reducers.ts
✅ Files skipped from review due to trivial changes (2)
- web/src/components/AIExperienceIcon.tsx
- web/src/korrel8r/client/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/hooks/usePopover.tsx
- web/src/components/Korrel8rPanel.tsx
Comment on lines
+193
to
+194
| await new Promise((r) => setTimeout(r, delay)); | ||
| delay = Math.min(delay * 2, maxDelay); |
There was a problem hiding this comment.
Make the backoff delay abortable.
Both retry loops wait on a bare setTimeout, so cancel() can stay stuck for up to maxDelay before the loop actually exits. That leaves reconnect timers alive after unmount/disable and can trigger late retries against stale state.
Possible fix
+const sleep = (ms: number, signal: AbortSignal) =>
+ new Promise<void>((resolve, reject) => {
+ const id = setTimeout(resolve, ms);
+ const onAbort = () => {
+ clearTimeout(id);
+ reject(signal.reason ?? new DOMException('Aborted', 'AbortError'));
+ };
+ signal.addEventListener('abort', onAbort, { once: true });
+ });
+
export const consoleEvents = (
onEvent: (event: Console) => void | Promise<void>,
onError: (err: unknown) => void,
onConnect: () => void,
@@
} catch (err) {
if (controller.signal.aborted) break;
onError(err);
- await new Promise((r) => setTimeout(r, delay));
+ await sleep(delay, controller.signal);
delay = Math.min(delay * 2, maxDelay);
}
}
};
@@
} catch (err) {
onError(err); // Set error even if canceled, we are still disconnected
if (controller.signal.aborted) break;
- await new Promise((r) => setTimeout(r, delay));
+ await sleep(delay, controller.signal);
delay = Math.min(delay * 2, maxDelay);
}
}
};Also applies to: 219-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/korrel8r-client.ts` around lines 193 - 194, The exponential backoff
wait using await new Promise((r) => setTimeout(r, delay)) is not abortable and
lets cancel() block until the timeout elapses; change both retry loops to use an
abortable delay that races setTimeout against an AbortSignal (or rejects when
the provided signal is aborted) so the loop can exit immediately on
cancellation, update the cancel() path to call abort() on that signal (or
otherwise resolve the delay promise), and replace the two occurrences that
manipulate delay and maxDelay (the backoff logic around delay/maxDelay) with
calls to this abortableDelay helper so reconnect timers don't fire after
unmount/disable.
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
3 participants
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.
Allows console to send state and receive requests to/from an AI agent.
Console uses korrel8r REST API, agent uses MCP API.
Summary by CodeRabbit
New Features
API Changes
/graphs/neighboursendpoint in favor of/graphs/neighbors.