feat: add openchoreo assistant perch#511
Merged
Merged
Conversation
yashodgayashan
requested review from
kaviththiranga,
sameerajayasoma and
stefinie123
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces Perch, an opt-in AI assistant: configuration keys and a feature flag, a backend forwarder plugin and streaming router, a frontend plugin with API client, drawer/context, and contextual launchers, app-level wiring, CI hook for failed-run detection, tests, and documentation. ChangesPerch Assistant Feature Introduction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
plugins/openchoreo-perch/package.json (1)
36-38: ⚡ Quick winReact-markdown and remark-gfm are ESM-only packages — plan test setup accordingly if adding tests.
react-markdownv9 andremark-gfmv4 are ESM-only modules. However, the siblingopenchoreo-observabilityplugin uses the same packages with a comprehensive test suite, using@backstage/cliwhich abstracts Jest configuration. No customjest.config.jsortransformIgnorePatternsare visible in either plugin.Since
openchoreo-perchcurrently has no test files, there is no immediate concern. If tests are added in the future, follow the same pattern asopenchoreo-observability(use@backstage/cli package test,renderInTestAppfor Backstage components, and the utilities documented inTESTING_GUIDE.md).🤖 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 `@plugins/openchoreo-perch/package.json` around lines 36 - 38, The package.json added ESM-only dependencies react-markdown and remark-gfm which will break Jest if tests are added; update the test plan to mirror openchoreo-observability by using `@backstage/cli` test runner (run via `@backstage/cli package test`), rely on the Backstage testing utilities (renderInTestApp) and the TESTING_GUIDE.md patterns, and avoid custom jest.config.js that mishandles ESM modules—if you must add custom jest settings, configure transforms/transformIgnorePatterns to handle ESM or use the `@backstage/cli` abstraction so react-markdown and remark-gfm are resolved correctly when tests are added.app-config.production.yaml (1)
167-172: ⚡ Quick winMismatched
assistantfeature flag and agent URL can silently degrade Perch.If
OPENCHOREO_FEATURES_ASSISTANT_ENABLED=truebutOPENCHOREO_ASSISTANT_AGENT_URLis unset, the frontend renders Perch UI components while the backend proxy self-disables — every assistant API call returns 404 with no obvious error in the config.Backstage's config substitution evaluates a missing env var to
undefined(not an error), but also supports a${MY_VAR:-default}fallback syntax. Consider documenting this dependency in a comment, and optionally using the fallback syntax to make the misconfiguration more visible:- assistantAgentUrl: ${OPENCHOREO_ASSISTANT_AGENT_URL} + # Must be set when openchoreo.features.assistant.enabled is true. + # If omitted, the perch-backend plugin self-disables and all Perch API + # calls will 404 even though the UI feature flag is on. + assistantAgentUrl: ${OPENCHOREO_ASSISTANT_AGENT_URL:-}Using
:-(empty-string default) ensures the key is always present in config, which may help debuggability — an emptyassistantAgentUrlis more obvious in a config dump than a completely absent key.Also applies to: 201-205
🤖 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 `@app-config.production.yaml` around lines 167 - 172, The assistantAgentUrl key can be unset when OPENCHOREO_FEATURES_ASSISTANT_ENABLED=true which causes silent degradation; update the app-config entries that reference OPENCHOREO_ASSISTANT_AGENT_URL (including assistantAgentUrl and the other similar entries around lines 201-205) to use Backstage's fallback substitution (e.g. the ${VAR:-} form) so the config always contains the key (an empty string makes misconfiguration obvious), and add a short comment referencing the dependency on OPENCHOREO_FEATURES_ASSISTANT_ENABLED and advising operators to set OPENCHOREO_ASSISTANT_AGENT_URL when enabling the assistant.packages/app/src/components/catalog/EntityPage.tsx (1)
261-264: ⚡ Quick winPerch components call hooks before checking the assistant feature gate—wrap with
FeatureGateto prevent unnecessary mounting.
FailedBuildSnackbar,BuildPagePromptLauncher, andLogsPageDebugPromptall calluseAssistantEnabled()internally and returnnullwhen disabled, which is correct. However, they call other hooks (useLatestFailedRun(),useAssistantDrawer()) before the feature check. In particular,useLatestFailedRun()triggers polling of build data even when the assistant feature is disabled.While the components don't render when
assistantisfalse, they still mount, execute hooks, and potentially trigger API calls. Wrapping them withFeatureGate feature="assistant"at the composition level inEntityPage.tsxprevents this wasted work entirely:♻️ Proposed pattern (applies to
genericComponentEntityPageandsystemPageas well)- <FailedBuildSnackbar /> + <FeatureGate feature="assistant"> + <FailedBuildSnackbar /> + </FeatureGate><FeatureGatedContent feature="workflows"> + <FeatureGate feature="assistant"> <BuildPagePromptLauncher /> + </FeatureGate> <Workflows /> </FeatureGatedContent><FeatureGatedContent feature="observability"> <ObservabilityRuntimeLogs /> + <FeatureGate feature="assistant"> <LogsPageDebugPrompt /> + </FeatureGate> </FeatureGatedContent>🤖 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 `@packages/app/src/components/catalog/EntityPage.tsx` around lines 261 - 264, The three components FailedBuildSnackbar, BuildPagePromptLauncher, and LogsPageDebugPrompt currently mount and run internal hooks (like useLatestFailedRun and useAssistantDrawer) even when the assistant feature is disabled; wrap each usage in EntityPage.tsx with <FeatureGate feature="assistant">...</FeatureGate> so they never mount unless the assistant gate is enabled—apply the same pattern to instances in genericComponentEntityPage and systemPage; locate the component usages (FailedBuildSnackbar, BuildPagePromptLauncher, LogsPageDebugPrompt) and replace/encapsulate them with FeatureGate to prevent unnecessary hook execution and polling.plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts (1)
22-24: ⚡ Quick winUpdate the defaulting doc to match actual behavior.
Line 23 says all features default to enabled, but
ciliumandassistantintentionally default tofalse. Please align the comment with runtime behavior to avoid config confusion.🤖 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 `@plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts` around lines 22 - 24, Update the top-of-file doc comment in useOpenChoreoFeatures.ts to reflect runtime defaults: instead of saying "All features default to enabled", explicitly state that most features default to enabled but the "cilium" and "assistant" features intentionally default to false; mention that defaults come from openchoreo.features.* in app-config.yaml and that the hook useOpenChoreoFeatures enforces these exceptions so readers understand the actual behavior.plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx (1)
348-387: 💤 Low valueRead
timelinevia functional setState to drop it from deps.
handleApprovereadstimelinedirectly via.find(), sotimelinemust be in theuseCallbackdeps. As a result the callback is recreated on every streaming chunk, message append, status update, etc., which churns the JSX-boundonClickhandlers on all proposal cards. The same pattern applies tosendMessage(line 500: depends onmessageHistorywhich is derived fromtimeline).Reading the proposal via
setTimeline(prev => …)lets you keep the deps minimal and always operate on the latest state without recreating the callback on unrelated timeline mutations.Additionally,
api.executeAction(actionId)is awaited without anAbortSignal, so closing the drawer mid-execute leaves the request in flight and may then update state on an unmounted/cleared timeline.♻️ Sketch of the proposed pattern
const handleApprove = useCallback( async (actionId: string) => { updateProposalStatus(actionId, { kind: 'running' }); - const proposalItem = timeline.find( - (it): it is Extract<ChatItem, { kind: 'proposal' }> => - it.kind === 'proposal' && it.action.action_id === actionId, - ); + let proposalItem: Extract<ChatItem, { kind: 'proposal' }> | undefined; + setTimeline(prev => { + proposalItem = prev.find( + (it): it is Extract<ChatItem, { kind: 'proposal' }> => + it.kind === 'proposal' && it.action.action_id === actionId, + ); + return prev; + }); try { const result = await api.executeAction(actionId); ... } }, - [api, ackForAction, timeline, updateProposalStatus], + [api, ackForAction, updateProposalStatus], );🤖 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 `@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx` around lines 348 - 387, handleApprove currently reads timeline directly and so must be included in the callback deps, causing frequent recreation; change it to read the proposal via functional state (use setTimeline(prev => { const proposalItem = prev.find(...); ... return prev; })) so you no longer reference timeline in the hook deps (remove timeline from the dependency array) and keep updateProposalStatus, api, and ackForAction as the only deps. Also make api.executeAction accept/receive an AbortSignal (or wrap the fetch with AbortController) and pass a signal that you cancel when the drawer unmounts/ closes to avoid updates after unmount; handleAbort by treating it as an error path and calling updateProposalStatus/ackForAction similarly. Ensure references to handleApprove, setTimeline, api.executeAction, ackForAction, and updateProposalStatus are updated accordingly.
🤖 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 `@app-config.yaml`:
- Around line 168-172: The app-config.yaml currently references
assistantAgentUrl with ${OPENCHOREO_ASSISTANT_AGENT_URL}, which causes startup
failures when the env var is unset; comment out the assistantAgentUrl line
(following the pattern used for optional keys like thunder.token) so the setting
is inactive by default and only enabled when explicitly configured, ensuring
deployments that leave openchoreo.features.assistant.enabled false don’t require
the env var.
In `@plugins/openchoreo-ci/src/hooks/useLatestFailedRun.ts`:
- Around line 63-65: The polling using setInterval in useLatestFailedRun can
start overlapping fetchBuilds() calls; change the logic to a self-scheduling
loop (use setTimeout or an async loop) instead of setInterval and ensure each
iteration awaits fetchBuilds() before scheduling the next run to prevent
concurrency; also propagate or log errors from fetchBuilds() instead of
swallowing them (replace .catch(() => {}) with proper error handling), and keep
the timeout id cleanup that currently uses id so clearTimeout/clearInterval in
the hook teardown still cancels pending schedules.
In `@plugins/openchoreo-perch-backend/src/router.ts`:
- Around line 217-256: forwardJson currently calls fetch without a timeout,
causing hung upstream requests; add an AbortSignal.timeout-based signal to the
fetch options (e.g., signal:
AbortSignal.timeout(routerOptions?.upstreamTimeoutMs ?? 30000)) so the request
is aborted after a bounded period; locate forwardJson and modify the fetch
invocation to include that signal and surface the timeout via RouterOptions
(e.g., upstreamTimeoutMs) so it’s configurable. Also update forwardStream to
combine its existing client-disconnect AbortController with an upstream deadline
using AbortSignal.any([clientAbort.signal,
AbortSignal.timeout(routerOptions?.streamTimeoutMs ?? <longer-ms>)]) to ensure
streaming handlers also time out if the backend stalls.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 36-40: The module-level sentinel constant NO_KEY_YET is declared
between import statements, which breaks import ordering and triggers the
import/first lint rule; move the const NO_KEY_YET = Symbol('no-key-yet') so that
all import lines (including import type { PinnedContext } from
'../AssistantContext/AssistantDrawerContext') appear first, followed by the
NO_KEY_YET declaration and then the rest of the module code to restore idiomatic
import ordering and satisfy ESLint.
In
`@plugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsx`:
- Around line 139-146: The close IconButton currently uses a hardcoded
aria-label ("Dismiss Perch prompt") which won't match a customized title; update
the IconButton in AssistantPromptLauncher so its aria-label is derived from the
component's title prop (falling back to a sensible default like "Perch prompt")
and keep the existing onClick={setOpen(false)} behavior and classes.closeBtn;
locate the IconButton render and replace the static aria-label with a computed
string that includes the title prop (e.g., `Dismiss ${title || 'Perch prompt'}`)
to ensure assistive tech announces the correct panel name.
---
Nitpick comments:
In `@app-config.production.yaml`:
- Around line 167-172: The assistantAgentUrl key can be unset when
OPENCHOREO_FEATURES_ASSISTANT_ENABLED=true which causes silent degradation;
update the app-config entries that reference OPENCHOREO_ASSISTANT_AGENT_URL
(including assistantAgentUrl and the other similar entries around lines 201-205)
to use Backstage's fallback substitution (e.g. the ${VAR:-} form) so the config
always contains the key (an empty string makes misconfiguration obvious), and
add a short comment referencing the dependency on
OPENCHOREO_FEATURES_ASSISTANT_ENABLED and advising operators to set
OPENCHOREO_ASSISTANT_AGENT_URL when enabling the assistant.
In `@packages/app/src/components/catalog/EntityPage.tsx`:
- Around line 261-264: The three components FailedBuildSnackbar,
BuildPagePromptLauncher, and LogsPageDebugPrompt currently mount and run
internal hooks (like useLatestFailedRun and useAssistantDrawer) even when the
assistant feature is disabled; wrap each usage in EntityPage.tsx with
<FeatureGate feature="assistant">...</FeatureGate> so they never mount unless
the assistant gate is enabled—apply the same pattern to instances in
genericComponentEntityPage and systemPage; locate the component usages
(FailedBuildSnackbar, BuildPagePromptLauncher, LogsPageDebugPrompt) and
replace/encapsulate them with FeatureGate to prevent unnecessary hook execution
and polling.
In `@plugins/openchoreo-perch/package.json`:
- Around line 36-38: The package.json added ESM-only dependencies react-markdown
and remark-gfm which will break Jest if tests are added; update the test plan to
mirror openchoreo-observability by using `@backstage/cli` test runner (run via
`@backstage/cli package test`), rely on the Backstage testing utilities
(renderInTestApp) and the TESTING_GUIDE.md patterns, and avoid custom
jest.config.js that mishandles ESM modules—if you must add custom jest settings,
configure transforms/transformIgnorePatterns to handle ESM or use the
`@backstage/cli` abstraction so react-markdown and remark-gfm are resolved
correctly when tests are added.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 348-387: handleApprove currently reads timeline directly and so
must be included in the callback deps, causing frequent recreation; change it to
read the proposal via functional state (use setTimeline(prev => { const
proposalItem = prev.find(...); ... return prev; })) so you no longer reference
timeline in the hook deps (remove timeline from the dependency array) and keep
updateProposalStatus, api, and ackForAction as the only deps. Also make
api.executeAction accept/receive an AbortSignal (or wrap the fetch with
AbortController) and pass a signal that you cancel when the drawer unmounts/
closes to avoid updates after unmount; handleAbort by treating it as an error
path and calling updateProposalStatus/ackForAction similarly. Ensure references
to handleApprove, setTimeline, api.executeAction, ackForAction, and
updateProposalStatus are updated accordingly.
In `@plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts`:
- Around line 22-24: Update the top-of-file doc comment in
useOpenChoreoFeatures.ts to reflect runtime defaults: instead of saying "All
features default to enabled", explicitly state that most features default to
enabled but the "cilium" and "assistant" features intentionally default to
false; mention that defaults come from openchoreo.features.* in app-config.yaml
and that the hook useOpenChoreoFeatures enforces these exceptions so readers
understand the actual behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b155aa8b-bf6e-4843-8602-3180f99aa3e9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
app-config.local.yaml.exampleapp-config.production.yamlapp-config.yamlpackages/app/package.jsonpackages/app/src/apis.tspackages/app/src/components/Root/Root.tsxpackages/app/src/components/catalog/EntityPage.tsxpackages/app/src/components/catalog/FeatureGatedContent.tsxpackages/backend/package.jsonpackages/backend/src/index.tsplugins/openchoreo-ci/src/hooks/index.tsplugins/openchoreo-ci/src/hooks/useLatestFailedRun.tsplugins/openchoreo-ci/src/index.tsplugins/openchoreo-common/src/types/features.tsplugins/openchoreo-perch-backend/README.mdplugins/openchoreo-perch-backend/package.jsonplugins/openchoreo-perch-backend/src/index.tsplugins/openchoreo-perch-backend/src/plugin.test.tsplugins/openchoreo-perch-backend/src/plugin.tsplugins/openchoreo-perch-backend/src/router.test.tsplugins/openchoreo-perch-backend/src/router.tsplugins/openchoreo-perch/README.mdplugins/openchoreo-perch/package.jsonplugins/openchoreo-perch/src/api/AssistantAgentApi.tsplugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsxplugins/openchoreo-perch/src/components/AssistantChatDrawer/index.tsplugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.tsplugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsxplugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsxplugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsxplugins/openchoreo-perch/src/index.tsplugins/openchoreo-perch/src/plugin.tsplugins/openchoreo-perch/tsconfig.jsonplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.tsplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.tsplugins/openchoreo-react/src/index.ts
yashodgayashan
force-pushed
the
perch
branch
2 times, most recently
from
84deb1e to
c68a725
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
plugins/openchoreo-perch-backend/src/router.test.ts (1)
291-312: ⚡ Quick win
forwardStreamtimeout test is missing several assertions present in theforwardJsontimeout test.The warmup-timeout test (lines 258-289) validates the full 504 payload shape (
error,message,request_id), elapsed bound, andlogger.error. The streaming-timeout test only checksres.body.errorandelapsed, leaving the response body structure and server-side logging unverified for the streaming path.♻️ Proposed additions to bring parity with the warmup-timeout test
expect(res.status).toBe(504); expect(res.body.error).toBe('GATEWAY_TIMEOUT'); + expect(res.body).toMatchObject({ + error: 'GATEWAY_TIMEOUT', + message: expect.stringMatching(/\d+ ms/), + request_id: expect.any(String), + }); expect(elapsed).toBeLessThan(2_000); + expect(logger.error).toHaveBeenCalledWith( + expect.stringMatching(/upstream.*timed out/i), + );🤖 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 `@plugins/openchoreo-perch-backend/src/router.test.ts` around lines 291 - 312, The streaming timeout test for forwardStream is missing assertions present in the forwardJson/warmup-timeout test: extend the 'forwardStream returns 504 when the upstream does not send response headers within streamTimeoutMs' test (which sets up upstream, tightRouter via createRouter, tightApp and streamTimeoutMs) to also assert the full 504 payload shape (res.body.error, res.body.message, res.body.request_id) and verify the server logged the timeout via logger.error (the same pattern used in the warmup-timeout test for forwardJson); ensure the elapsed bound check remains and that the logger assertion looks for the forward-stream timeout message emitted by forwardStream.plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx (1)
24-99: ⚡ Quick winReuse
isFailedStatusfromuseLatestFailedRuninstead of duplicating it.The same predicate (
includes('fail') || includes('error')on a lower-cased status, with a "kept in sync with BuildStatusChip" intent) lives inplugins/openchoreo-ci/src/hooks/useLatestFailedRun.ts. Two copies will drift the moment one side updates the heuristic. Export it from the CI plugin and import it here.♻️ Proposed change (in `useLatestFailedRun.ts`)
-/** Failure heuristic — kept in sync with BuildStatusChip. */ -const isFailedStatus = (status?: string) => { +/** Failure heuristic — kept in sync with BuildStatusChip. */ +export const isFailedStatus = (status?: string) => { const lowered = (status ?? '').toLowerCase(); return lowered.includes('fail') || lowered.includes('error'); };And in this file:
-import { useLatestFailedRun } from '@openchoreo/backstage-plugin-openchoreo-ci'; +import { + isFailedStatus, + useLatestFailedRun, +} from '@openchoreo/backstage-plugin-openchoreo-ci'; @@ -const isFailedStatus = (status?: string) => { - const lowered = (status ?? '').toLowerCase(); - return lowered.includes('fail') || lowered.includes('error'); -};🤖 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 `@plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx` around lines 24 - 99, The component duplicates the status predicate; remove the local isFailedStatus in BuildPagePromptLauncher and instead import the shared predicate exported from useLatestFailedRun (the existing hook that contains the canonical isFailedStatus), update the component's imports to reference that exported isFailedStatus, and ensure BuildPagePromptLauncher (and any callers using targetIsFailed) uses the imported isFailedStatus consistently so the heuristic stays in one place.
🤖 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 `@plugins/openchoreo-perch/package.json`:
- Around line 5-12: The package.json has contradictory settings: "private": true
prevents publishing but publishConfig.access is set to "public"; either remove
"private": true if you intend to publish, or remove only the
publishConfig.access field if this is a permanently private package; keep
publishConfig.main and publishConfig.types if you still want the
prepack/postpack path-swapping behavior, but drop publishConfig.access when
choosing the private option (edit the "private" and the publishConfig.access
keys accordingly).
- Line 41: Update the peer dependency range for "react" in package.json to only
allow React 18 (change the "react" peerDependency from "^16.13.1 || ^17.0.0 ||
^18.0.0" to "^18.0.0") because react-markdown@^9.0.1 requires React ≥18; ensure
the "react" key in package.json's peerDependencies reflects this narrower
constraint so consumers using React 16/17 won't be allowed.
- Line 42: The package.json currently pins the react-router-dom peer dependency
to an exact version "6.30.1", causing peer resolution conflicts; update the
dependency entry for react-router-dom in plugins/openchoreo-perch's package.json
from the exact version to a caret range (e.g., "^6.30.1" or a repo-consistent
range like "^6.0.0") so it matches the repository pattern and allows compatible
minor/patch versions; locate the react-router-dom entry in package.json to make
this change and run a quick install/test to ensure no new peer warnings.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 596-602: The disabled predicate on the Clear IconButton is
inverted/unreachable because sendMessage appends to timeline before setting
isSending, so change the condition in the IconButton (inside
AssistantChatDrawer.tsx) from disabled={isSending && timeline.length === 0} to
disabled={isSending || timeline.length === 0} so the Clear button is disabled
when sending or when the timeline is empty; update the IconButton/Tooltip block
that wraps handleClear accordingly (references: handleClear, isSending,
timeline, sendMessage).
---
Nitpick comments:
In `@plugins/openchoreo-perch-backend/src/router.test.ts`:
- Around line 291-312: The streaming timeout test for forwardStream is missing
assertions present in the forwardJson/warmup-timeout test: extend the
'forwardStream returns 504 when the upstream does not send response headers
within streamTimeoutMs' test (which sets up upstream, tightRouter via
createRouter, tightApp and streamTimeoutMs) to also assert the full 504 payload
shape (res.body.error, res.body.message, res.body.request_id) and verify the
server logged the timeout via logger.error (the same pattern used in the
warmup-timeout test for forwardJson); ensure the elapsed bound check remains and
that the logger assertion looks for the forward-stream timeout message emitted
by forwardStream.
In
`@plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx`:
- Around line 24-99: The component duplicates the status predicate; remove the
local isFailedStatus in BuildPagePromptLauncher and instead import the shared
predicate exported from useLatestFailedRun (the existing hook that contains the
canonical isFailedStatus), update the component's imports to reference that
exported isFailedStatus, and ensure BuildPagePromptLauncher (and any callers
using targetIsFailed) uses the imported isFailedStatus consistently so the
heuristic stays in one place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cae4df87-ad66-452f-8e0a-9534c43b6298
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
app-config.local.yaml.exampleapp-config.production.yamlapp-config.yamlpackages/app/package.jsonpackages/app/src/apis.tspackages/app/src/components/Root/Root.tsxpackages/app/src/components/catalog/EntityPage.tsxpackages/app/src/components/catalog/FeatureGatedContent.tsxpackages/backend/package.jsonpackages/backend/src/index.tsplugins/openchoreo-ci/src/hooks/index.tsplugins/openchoreo-ci/src/hooks/useLatestFailedRun.tsplugins/openchoreo-ci/src/index.tsplugins/openchoreo-common/src/types/features.tsplugins/openchoreo-perch-backend/.eslintrc.jsplugins/openchoreo-perch-backend/README.mdplugins/openchoreo-perch-backend/package.jsonplugins/openchoreo-perch-backend/src/index.tsplugins/openchoreo-perch-backend/src/plugin.test.tsplugins/openchoreo-perch-backend/src/plugin.tsplugins/openchoreo-perch-backend/src/router.test.tsplugins/openchoreo-perch-backend/src/router.tsplugins/openchoreo-perch/.eslintrc.jsplugins/openchoreo-perch/README.mdplugins/openchoreo-perch/package.jsonplugins/openchoreo-perch/src/api/AssistantAgentApi.tsplugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsxplugins/openchoreo-perch/src/components/AssistantChatDrawer/index.tsplugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.tsplugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsxplugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsxplugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsxplugins/openchoreo-perch/src/index.tsplugins/openchoreo-perch/src/plugin.tsplugins/openchoreo-perch/tsconfig.jsonplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.tsplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.tsplugins/openchoreo-react/src/index.ts
✅ Files skipped from review due to trivial changes (15)
- plugins/openchoreo-perch/.eslintrc.js
- plugins/openchoreo-perch/src/components/AssistantChatDrawer/index.ts
- plugins/openchoreo-perch-backend/src/index.ts
- plugins/openchoreo-perch/tsconfig.json
- packages/app/package.json
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.ts
- packages/backend/package.json
- plugins/openchoreo-ci/src/hooks/index.ts
- plugins/openchoreo-perch-backend/.eslintrc.js
- plugins/openchoreo-perch-backend/package.json
- plugins/openchoreo-common/src/types/features.ts
- plugins/openchoreo-perch-backend/src/plugin.ts
- packages/backend/src/index.ts
- packages/app/src/components/catalog/FeatureGatedContent.tsx
- plugins/openchoreo-perch/README.md
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/openchoreo-react/src/index.ts
- packages/app/src/components/Root/Root.tsx
- packages/app/src/apis.ts
- plugins/openchoreo-perch/src/plugin.ts
- plugins/openchoreo-ci/src/index.ts
- plugins/openchoreo-perch-backend/src/plugin.test.ts
- app-config.local.yaml.example
- plugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsx
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts
- packages/app/src/components/catalog/EntityPage.tsx
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsx
- app-config.production.yaml
- plugins/openchoreo-perch-backend/README.md
- plugins/openchoreo-perch/src/index.ts
- plugins/openchoreo-perch/src/api/AssistantAgentApi.ts
- plugins/openchoreo-perch-backend/src/router.ts
yashodgayashan
force-pushed
the
perch
branch
2 times, most recently
from
78bef2f to
42d23ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/openchoreo-perch/src/api/AssistantAgentApi.ts (1)
167-175: ⚡ Quick winTighten the error swallow to JSON parsing only.
The current
try/catchwraps bothJSON.parse(trimmed)and the consumer'sonEvent(...)invocation, so any exception thrown from anonEventhandler (e.g. a bug in the drawer's reducer) is silently dropped under the comment "Skip malformed JSON". Parse and dispatch separately so handler errors actually surface — they should at minimum reach the outercatchinstreamChat's caller, otherwise the stream looks like it's silently working while the UI never updates.♻️ Proposed fix
for (const line of lines) { const trimmed = line.trim(); if (!trimmed) continue; + let event: StreamEvent; try { - onEvent(JSON.parse(trimmed) as StreamEvent); + event = JSON.parse(trimmed) as StreamEvent; } catch { // Skip malformed JSON + continue; } + onEvent(event); }The same pattern should be applied to the tail-buffer block at lines 177-184.
🤖 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 `@plugins/openchoreo-perch/src/api/AssistantAgentApi.ts` around lines 167 - 175, The JSON parsing and event dispatch are currently wrapped together so handler errors are swallowed; update the loop in AssistantAgentApi.ts to first attempt JSON.parse(trimmed) inside a try/catch that only handles and skips malformed JSON (so parse errors are ignored), then call onEvent(parsed as StreamEvent) outside that parse-catch so any exceptions thrown by the consumer handler propagate; apply the same change to the tail-buffer block that mirrors this parsing+dispatch logic so handler exceptions are not silently swallowed there either.plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts (1)
113-121: 💤 Low valueUse
/**JSDoc to match the other helper-hook docstrings.The new hook's leading block comment opens with
/*(line 114) instead of/**like every other helper hook in this file (e.g.useWorkflowsEnabledat line 74,useObservabilityEnabledat line 82, etc.). As-is, IDEs and TypeDoc will not pick this up as JSDoc foruseAssistantEnabled.♻️ Proposed fix
-/* +/** * Helper hook to check if the OpenChoreo Assistant is enabled. * Defaults to false (opt-in). */ export function useAssistantEnabled(): boolean {🤖 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 `@plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts` around lines 113 - 121, The leading block comment for the helper hook useAssistantEnabled uses /* instead of JSDoc /**, so IDEs/TypeDoc won't pick up its docstring; update the comment immediately above export function useAssistantEnabled() to start with /** (matching other helpers like useWorkflowsEnabled and useObservabilityEnabled) and keep the same description text and formatting so it becomes a proper JSDoc comment.plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx (1)
442-464: 💤 Low valueDefensive fallback can attach a stale prior reply.
If
event.messageis ever empty/falsy ondone, the fallback walksprevfor the last assistant message and re-appends its content as a new turn — duplicating the previous reply. TheStreamEventcontract typesdone.messageas requiredstring(AssistantAgentApi.tsline 71-72), so this only triggers if the agent violates the contract, but the chosen fallback is still incorrect: the in-flight turn lives in thestreamingstate, not inprev. Either drop the fallback (let the empty turn render) or capture the streaming buffer via a ref and use it here.🤖 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 `@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx` around lines 442 - 464, The current 'done' case in the setTimeline block uses a fallback that re-reads the last assistant message from prev and can duplicate stale replies; instead either remove that fallback or use the in-flight streaming buffer: create a ref (e.g., streamingBufferRef) that you update wherever you append streaming chunks to the timeline/streaming state, then in the 'done' handler use event.message || streamingBufferRef.current as the content for the new assistant message (or simply use event.message and drop the fallback), and finally clear setStreaming('')/setToolStatus(null) as before; update references to streaming and streaming handlers to maintain the ref so the final append uses the actual in-flight buffer rather than prev.
🤖 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 `@plugins/openchoreo-perch-backend/src/router.ts`:
- Around line 137-155: buildForwardHeaders is currently copying most incoming
headers (leaking sensitive headers like cookie) and generates an x-request-id
that other code paths still read from req.headers; change buildForwardHeaders to
forward only a tight whitelist of safe headers (e.g., accept, accept-language,
content-type, user-agent, authorization if needed) and explicitly exclude
cookies and other sensitive headers, and return the effective request ID (the
generated or forwarded x-request-id) so callers can use that single source of
truth; update call sites that currently read req.header('x-request-id') (the
handlers that log/respond at the locations corresponding to lines ~185/186,
~223/224, ~325/326, ~331/333) to use the returned x-request-id from
buildForwardHeaders instead of reading the original request header.
---
Nitpick comments:
In `@plugins/openchoreo-perch/src/api/AssistantAgentApi.ts`:
- Around line 167-175: The JSON parsing and event dispatch are currently wrapped
together so handler errors are swallowed; update the loop in
AssistantAgentApi.ts to first attempt JSON.parse(trimmed) inside a try/catch
that only handles and skips malformed JSON (so parse errors are ignored), then
call onEvent(parsed as StreamEvent) outside that parse-catch so any exceptions
thrown by the consumer handler propagate; apply the same change to the
tail-buffer block that mirrors this parsing+dispatch logic so handler exceptions
are not silently swallowed there either.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 442-464: The current 'done' case in the setTimeline block uses a
fallback that re-reads the last assistant message from prev and can duplicate
stale replies; instead either remove that fallback or use the in-flight
streaming buffer: create a ref (e.g., streamingBufferRef) that you update
wherever you append streaming chunks to the timeline/streaming state, then in
the 'done' handler use event.message || streamingBufferRef.current as the
content for the new assistant message (or simply use event.message and drop the
fallback), and finally clear setStreaming('')/setToolStatus(null) as before;
update references to streaming and streaming handlers to maintain the ref so the
final append uses the actual in-flight buffer rather than prev.
In `@plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts`:
- Around line 113-121: The leading block comment for the helper hook
useAssistantEnabled uses /* instead of JSDoc /**, so IDEs/TypeDoc won't pick up
its docstring; update the comment immediately above export function
useAssistantEnabled() to start with /** (matching other helpers like
useWorkflowsEnabled and useObservabilityEnabled) and keep the same description
text and formatting so it becomes a proper JSDoc comment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35dd5b5d-706d-4740-a6aa-1267b9e0ef80
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
app-config.local.yaml.exampleapp-config.production.yamlapp-config.yamlpackages/app/package.jsonpackages/app/src/apis.tspackages/app/src/components/Root/Root.tsxpackages/app/src/components/catalog/EntityPage.tsxpackages/app/src/components/catalog/FeatureGatedContent.tsxpackages/backend/package.jsonpackages/backend/src/index.tsplugins/openchoreo-ci/src/hooks/index.tsplugins/openchoreo-ci/src/hooks/useLatestFailedRun.tsplugins/openchoreo-ci/src/index.tsplugins/openchoreo-common/src/types/features.tsplugins/openchoreo-perch-backend/.eslintrc.jsplugins/openchoreo-perch-backend/README.mdplugins/openchoreo-perch-backend/package.jsonplugins/openchoreo-perch-backend/src/index.tsplugins/openchoreo-perch-backend/src/plugin.test.tsplugins/openchoreo-perch-backend/src/plugin.tsplugins/openchoreo-perch-backend/src/router.test.tsplugins/openchoreo-perch-backend/src/router.tsplugins/openchoreo-perch/.eslintrc.jsplugins/openchoreo-perch/README.mdplugins/openchoreo-perch/package.jsonplugins/openchoreo-perch/src/api/AssistantAgentApi.tsplugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsxplugins/openchoreo-perch/src/components/AssistantChatDrawer/index.tsplugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.tsplugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsxplugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsxplugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsxplugins/openchoreo-perch/src/index.tsplugins/openchoreo-perch/src/plugin.tsplugins/openchoreo-perch/tsconfig.jsonplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.tsplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.tsplugins/openchoreo-react/src/index.ts
✅ Files skipped from review due to trivial changes (11)
- packages/app/package.json
- plugins/openchoreo-perch/.eslintrc.js
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.ts
- plugins/openchoreo-perch-backend/src/index.ts
- plugins/openchoreo-perch-backend/.eslintrc.js
- plugins/openchoreo-perch/tsconfig.json
- plugins/openchoreo-perch/src/components/AssistantChatDrawer/index.ts
- plugins/openchoreo-perch-backend/package.json
- plugins/openchoreo-perch/README.md
- plugins/openchoreo-perch/src/index.ts
- plugins/openchoreo-perch/package.json
🚧 Files skipped from review as they are similar to previous changes (18)
- plugins/openchoreo-react/src/index.ts
- plugins/openchoreo-ci/src/hooks/index.ts
- packages/app/src/components/Root/Root.tsx
- plugins/openchoreo-common/src/types/features.ts
- plugins/openchoreo-perch-backend/src/plugin.test.ts
- app-config.production.yaml
- plugins/openchoreo-ci/src/index.ts
- app-config.yaml
- app-config.local.yaml.example
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.ts
- plugins/openchoreo-perch-backend/src/plugin.ts
- plugins/openchoreo-ci/src/hooks/useLatestFailedRun.ts
- plugins/openchoreo-perch-backend/README.md
- packages/app/src/apis.ts
- plugins/openchoreo-perch/src/plugin.ts
- plugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsx
- plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx
- plugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsx
yashodgayashan
force-pushed
the
perch
branch
3 times, most recently
from
0c8da59 to
684e771
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@packages/app/src/apis.ts`:
- Around line 43-46: Remove the redundant app-level assistantAgentApiRef factory
and its AssistantAgentClient import: the plugin already registers
assistantAgentApiRef in createPlugin, so delete the assistantAgentApiRef factory
registration and the AssistantAgentClient import from the app-level APIs to
avoid shadowing the plugin implementation; keep any other app-level API
registrations intact and ensure no other code references the removed factory
before committing.
In `@plugins/openchoreo-perch-backend/src/router.ts`:
- Around line 395-402: The response body read (await upstream.text()) must be
performed and error-handled before writing headers/status to the client to avoid
mid-stream failures turning into generic 500s; move the upstream.text() call
into a try-catch, read the body and on success then call res.status(...) and
applyResponseHeaders(upstream.headers, res) and send the body (res.send or
res.end), and on error catch the failure and return the plugin BAD_GATEWAY (502)
envelope with the request_id (matching the pattern used around the upstream
fetch handling earlier) so upstream connection resets are converted to proper
502 responses.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 301-304: The useEffect that auto-scrolls the drawer (useEffect in
AssistantChatDrawer.tsx using bodyRef.current and setting el.scrollTop =
el.scrollHeight) is missing `error` in its dependency array, so add `error` to
the dependencies alongside `timeline`, `streaming`, and `toolStatus` to ensure
the drawer scrolls when an error appears; update the dependency list referenced
in that useEffect accordingly.
In
`@plugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsx`:
- Around line 16-21: The launcher uses the same stacking layer as real snackbars
which can cause overlap; update the styles in useStyles (root) to lower its
z-index below snackbars by subtracting one or using a dedicated value (e.g.,
theme.zIndex.snackbar - 1 or a new theme value) so AssistantPromptLauncher
renders beneath FailedBuildSnackbar and other MUI Snackbars; change the zIndex
assignment in the root style to reference the adjusted value.
- Around line 165-176: The FAB (the <Fab> in AssistantPromptLauncher) needs an
aria-controls that points to the panel's id and focus management so assistive
tech and keyboard users can navigate: add a stable id for the panel (ensure the
panel element rendered by this component has that id), add
aria-controls={panelId} to the <Fab> (alongside existing aria-expanded), and use
refs (e.g., fabRef for the Fab and panelRef for the panel) to move focus into
the panel when setOpen(true) runs and return focus to fabRef when setOpen(false)
runs; update the onClick handler or use an effect watching open to call
panelRef.current.focus() on open and fabRef.current.focus() on close.
In `@plugins/openchoreo-perch/src/plugin.ts`:
- Around line 31-41: The plugin-level createApiFactory for assistantAgentApiRef
is duplicated by the app-level registration and is therefore shadowed; remove
the duplicate and keep a single canonical registration. Either delete the
createApiFactory entry from the openchoreoPerchPlugin apis array (the
createPlugin call that constructs AssistantAgentClient) or remove the app-level
createApiFactory for assistantAgentApiRef so only the plugin registers it;
whichever you keep, ensure the remaining factory uses AssistantAgentClient with
deps { discoveryApiRef, fetchApiRef } and update or remove the JSDoc in
openchoreoPerchPlugin/assistantAgentApiRef to reflect the chosen canonical
location.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 573d7b80-80f6-43e5-aad7-fc553e00316a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
app-config.local.yaml.exampleapp-config.production.yamlapp-config.yamlpackages/app/package.jsonpackages/app/src/apis.test.tspackages/app/src/apis.tspackages/app/src/components/Root/Root.tsxpackages/app/src/components/catalog/EntityPage.tsxpackages/app/src/components/catalog/FeatureGatedContent.tsxpackages/backend/package.jsonpackages/backend/src/index.tsplugins/openchoreo-ci/src/hooks/index.tsplugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsxplugins/openchoreo-ci/src/hooks/useLatestFailedRun.tsplugins/openchoreo-ci/src/index.tsplugins/openchoreo-common/src/types/features.tsplugins/openchoreo-perch-backend/.eslintrc.jsplugins/openchoreo-perch-backend/README.mdplugins/openchoreo-perch-backend/package.jsonplugins/openchoreo-perch-backend/src/index.tsplugins/openchoreo-perch-backend/src/plugin.test.tsplugins/openchoreo-perch-backend/src/plugin.tsplugins/openchoreo-perch-backend/src/router.test.tsplugins/openchoreo-perch-backend/src/router.tsplugins/openchoreo-perch/.eslintrc.jsplugins/openchoreo-perch/README.mdplugins/openchoreo-perch/package.jsonplugins/openchoreo-perch/src/api/AssistantAgentApi.tsplugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsxplugins/openchoreo-perch/src/components/AssistantChatDrawer/index.tsplugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.tsplugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsxplugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsxplugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsxplugins/openchoreo-perch/src/index.tsplugins/openchoreo-perch/src/plugin.tsplugins/openchoreo-perch/tsconfig.jsonplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.tsplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.tsplugins/openchoreo-react/src/index.ts
✅ Files skipped from review due to trivial changes (13)
- plugins/openchoreo-perch/src/components/AssistantChatDrawer/index.ts
- plugins/openchoreo-react/src/index.ts
- plugins/openchoreo-perch-backend/src/index.ts
- plugins/openchoreo-perch/.eslintrc.js
- plugins/openchoreo-perch/tsconfig.json
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.ts
- plugins/openchoreo-ci/src/hooks/index.ts
- packages/backend/package.json
- packages/app/src/apis.test.ts
- plugins/openchoreo-perch/package.json
- plugins/openchoreo-perch/README.md
- plugins/openchoreo-perch-backend/package.json
- plugins/openchoreo-perch-backend/src/plugin.test.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/app/src/components/catalog/FeatureGatedContent.tsx
- plugins/openchoreo-perch-backend/.eslintrc.js
- packages/app/src/components/Root/Root.tsx
- plugins/openchoreo-ci/src/index.ts
- app-config.local.yaml.example
- app-config.yaml
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts
- app-config.production.yaml
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.ts
- packages/app/src/components/catalog/EntityPage.tsx
- plugins/openchoreo-common/src/types/features.ts
- plugins/openchoreo-perch-backend/README.md
- plugins/openchoreo-perch-backend/src/plugin.ts
- packages/backend/src/index.ts
- plugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsx
- plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx
- plugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsx
- plugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsx
- plugins/openchoreo-perch/src/api/AssistantAgentApi.ts
- plugins/openchoreo-perch/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app-config.yaml (1)
185-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the feature-env documentation list in sync with actual keys.
Line 186’s env-var summary is now stale after adding
assistant.enabled; please includeOPENCHOREO_FEATURES_ASSISTANT_ENABLED(andOPENCHOREO_FEATURES_AUTHZ_ENABLED, which is also used below) to avoid operator misconfiguration.Suggested update
- # Environment variables: OPENCHOREO_FEATURES_WORKFLOWS_ENABLED, OPENCHOREO_FEATURES_OBSERVABILITY_ENABLED, OPENCHOREO_FEATURES_AUTH_ENABLED, OPENCHOREO_FEATURES_CILIUM_ENABLED + # Environment variables: OPENCHOREO_FEATURES_WORKFLOWS_ENABLED, OPENCHOREO_FEATURES_OBSERVABILITY_ENABLED, OPENCHOREO_FEATURES_AUTH_ENABLED, OPENCHOREO_FEATURES_AUTHZ_ENABLED, OPENCHOREO_FEATURES_CILIUM_ENABLED, OPENCHOREO_FEATURES_ASSISTANT_ENABLED🤖 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 `@app-config.yaml` around lines 185 - 213, The environment-variable summary comment is out of date and missing the new assistant and authz feature keys; update the top comment that lists environment variables so it includes OPENCHOREO_FEATURES_ASSISTANT_ENABLED and OPENCHOREO_FEATURES_AUTHZ_ENABLED, and verify the features block keys (workflows.enabled, observability.enabled, auth.enabled, authz.enabled, cilium.enabled, assistant.enabled) match the documented env var names so operators can configure via those variables.
🧹 Nitpick comments (1)
plugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsx (1)
244-258: ⚡ Quick winDouble
Promise.resolve()flush is fragile.The two chained
await Promise.resolve()calls at lines 248–251 assume the rejection flows through exactly two microtask hops in the hook's catch block. If the implementation adds an extraawait, this flush will be insufficient andfetchBuildsmay not yet be rescheduled when the 30 s timer is advanced, causing a flaky test.A more robust pattern is to flush all pending microtasks explicitly:
♻️ Proposed fix
- // Allow the rejected promise to settle through the catch. - await act(async () => { - await Promise.resolve(); - await Promise.resolve(); - }); + // Flush all pending microtasks so the catch block fully settles. + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 0)); + });🤖 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 `@plugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsx` around lines 244 - 258, The double await Promise.resolve() is fragile; replace it with a robust microtask flush (e.g., await act(async () => await new Promise(r => setImmediate(r)))) so the rejected promise in the hook's catch always settles before advancing timers; update the test in useLatestFailedRun.test.tsx around the fetchBuilds assertions to call this microtask flush (referencing the existing act(...) / jest.advanceTimersByTime(...) sequence and the fetchBuilds mock) instead of the two chained Promise.resolve() calls.
🤖 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 `@packages/app/src/apis.test.ts`:
- Around line 119-123: Replace the fragile apis.find(... )! lookup and the
unused catalogApi argument with the provided helper: use findFactory(apis,
catalogGraphApiRef) to locate the factory and then invoke that factory; remove
the hardcoded 'plugin.catalog-graph.service' string and drop the unnecessary
catalogApi: stubCatalog parameter from the invoke call so the test uses the
official catalogGraphApiRef factory lookup and no longer relies on a
non-existent dep.
In `@plugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsx`:
- Around line 40-42: The afterEach hook currently calls jest.useRealTimers() but
does not clear pending fake timers, which can leak timeouts into later tests;
update the afterEach block (the afterEach function that calls
jest.useRealTimers) to first call jest.clearAllTimers() (and optionally
jest.resetAllMocks() if needed) before calling jest.useRealTimers(), ensuring
any pending fake timers created in tests are cleared prior to restoring real
timers.
- Around line 88-100: The test mounts useLatestFailedRun repeatedly in a loop
without unmounting prior instances, leaving their effects/timers active; update
the loop so each renderHook call captures and calls its unmount (e.g., const {
result, unmount } = renderHook(() => useLatestFailedRun()) and call unmount() at
the end of each iteration) or use the testing-library cleanup between
iterations; adjust the test that iterates over statuses (using
mockUseWorkflowData and makeBuild) to unmount after each expectation to ensure
timers/effects from previous hook mounts are disposed.
In
`@plugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsx`:
- Around line 442-463: The current done-event handler in AssistantChatDrawer.tsx
appends an assistant message using event.message || fallback which can duplicate
the last assistant message when event.message is falsy; change the logic in the
done case inside setTimeline/setStreaming handling so you only append a new
timeline entry when event.message is a non-empty string (i.e., guard before
calling setTimeline), and if event.message is empty just call setStreaming('')
and setToolStatus(null) (or log a warning) instead of using the last timeline
entry as a fallback; update references to setTimeline, setStreaming,
setToolStatus, event.message, and the fallback lookup so duplication is avoided.
- Line 754: Replace the deprecated TextField prop rowsMax with the newer maxRows
inside the AssistantChatDrawer component; locate the TextField usage in
AssistantChatDrawer (look for rowsMax={4}) and change that prop name to
maxRows={4} so Material-UI no longer emits the deprecation warning.
---
Outside diff comments:
In `@app-config.yaml`:
- Around line 185-213: The environment-variable summary comment is out of date
and missing the new assistant and authz feature keys; update the top comment
that lists environment variables so it includes
OPENCHOREO_FEATURES_ASSISTANT_ENABLED and OPENCHOREO_FEATURES_AUTHZ_ENABLED, and
verify the features block keys (workflows.enabled, observability.enabled,
auth.enabled, authz.enabled, cilium.enabled, assistant.enabled) match the
documented env var names so operators can configure via those variables.
---
Nitpick comments:
In `@plugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsx`:
- Around line 244-258: The double await Promise.resolve() is fragile; replace it
with a robust microtask flush (e.g., await act(async () => await new Promise(r
=> setImmediate(r)))) so the rejected promise in the hook's catch always settles
before advancing timers; update the test in useLatestFailedRun.test.tsx around
the fetchBuilds assertions to call this microtask flush (referencing the
existing act(...) / jest.advanceTimersByTime(...) sequence and the fetchBuilds
mock) instead of the two chained Promise.resolve() calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f250f4e-94e8-4602-91ed-ceba06ac9707
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
app-config.local.yaml.exampleapp-config.production.yamlapp-config.yamlpackages/app/package.jsonpackages/app/src/apis.test.tspackages/app/src/apis.tspackages/app/src/components/Root/Root.tsxpackages/app/src/components/catalog/EntityPage.tsxpackages/app/src/components/catalog/FeatureGatedContent.tsxpackages/backend/package.jsonpackages/backend/src/index.tsplugins/openchoreo-ci/src/hooks/index.tsplugins/openchoreo-ci/src/hooks/useLatestFailedRun.test.tsxplugins/openchoreo-ci/src/hooks/useLatestFailedRun.tsplugins/openchoreo-ci/src/index.tsplugins/openchoreo-common/src/types/features.tsplugins/openchoreo-perch-backend/.eslintrc.jsplugins/openchoreo-perch-backend/README.mdplugins/openchoreo-perch-backend/package.jsonplugins/openchoreo-perch-backend/src/index.tsplugins/openchoreo-perch-backend/src/plugin.test.tsplugins/openchoreo-perch-backend/src/plugin.tsplugins/openchoreo-perch-backend/src/router.test.tsplugins/openchoreo-perch-backend/src/router.tsplugins/openchoreo-perch/.eslintrc.jsplugins/openchoreo-perch/README.mdplugins/openchoreo-perch/package.jsonplugins/openchoreo-perch/src/api/AssistantAgentApi.tsplugins/openchoreo-perch/src/components/AssistantChatDrawer/AssistantChatDrawer.tsxplugins/openchoreo-perch/src/components/AssistantChatDrawer/index.tsplugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsxplugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.tsplugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsxplugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsxplugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsxplugins/openchoreo-perch/src/index.tsplugins/openchoreo-perch/src/plugin.tsplugins/openchoreo-perch/tsconfig.jsonplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.tsplugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.tsplugins/openchoreo-react/src/index.ts
✅ Files skipped from review due to trivial changes (20)
- plugins/openchoreo-perch-backend/.eslintrc.js
- plugins/openchoreo-perch/.eslintrc.js
- plugins/openchoreo-perch-backend/src/index.ts
- packages/app/package.json
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/index.ts
- plugins/openchoreo-perch/tsconfig.json
- plugins/openchoreo-ci/src/hooks/index.ts
- plugins/openchoreo-react/src/index.ts
- plugins/openchoreo-perch/src/plugin.ts
- app-config.local.yaml.example
- plugins/openchoreo-perch-backend/src/plugin.test.ts
- packages/backend/package.json
- plugins/openchoreo-common/src/types/features.ts
- plugins/openchoreo-perch/src/components/AssistantChatDrawer/index.ts
- plugins/openchoreo-perch-backend/package.json
- plugins/openchoreo-perch-backend/README.md
- packages/backend/src/index.ts
- plugins/openchoreo-perch/src/index.ts
- plugins/openchoreo-perch/package.json
- plugins/openchoreo-perch/README.md
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/openchoreo-ci/src/index.ts
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.ts
- app-config.production.yaml
- packages/app/src/components/catalog/FeatureGatedContent.tsx
- plugins/openchoreo-react/src/hooks/useOpenChoreoFeatures.test.ts
- plugins/openchoreo-perch-backend/src/plugin.ts
- plugins/openchoreo-perch/src/components/LogsPageDebugPrompt/LogsPageDebugPrompt.tsx
- plugins/openchoreo-ci/src/hooks/useLatestFailedRun.ts
- plugins/openchoreo-perch/src/components/BuildPagePromptLauncher/BuildPagePromptLauncher.tsx
- plugins/openchoreo-perch/src/components/FailedBuildSnackbar/FailedBuildSnackbar.tsx
- plugins/openchoreo-perch-backend/src/router.ts
- plugins/openchoreo-perch/src/components/AssistantContext/AssistantDrawerContext.tsx
- plugins/openchoreo-perch/src/api/AssistantAgentApi.ts
- packages/app/src/components/catalog/EntityPage.tsx
- plugins/openchoreo-perch/src/components/AssistantPromptLauncher/AssistantPromptLauncher.tsx
- plugins/openchoreo-perch-backend/src/router.test.ts
yashodgayashan
force-pushed
the
perch
branch
5 times, most recently
from
ee3d974 to
d976faf
Compare
stefinie123
reviewed
May 14, 2026
|
|
||
| const DRAWER_WIDTH = 440; | ||
|
|
||
| const useStyles = makeStyles(theme => ({ |
Contributor
There was a problem hiding this comment.
Shall we move the styles to a separate file since this file is bit long
| "react-router-dom": "^6.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@backstage/cli": "0.34.1", |
Contributor
There was a problem hiding this comment.
Suggested change
| "@backstage/cli": "0.34.1", | |
| "@backstage/cli": "0.34.3", |
Mismatching backstage cli version. We have used 0.34.3 in all other files of this repo.
| "@testing-library/jest-dom": "6.0.0", | ||
| "@testing-library/react": "15.0.0", | ||
| "@types/node": "20.19.13", | ||
| "@types/react": "18.0.0", |
Contributor
There was a problem hiding this comment.
Suggested change
| "@types/react": "18.0.0", | |
| "@types/react": "^18.0.0", |
| "@openchoreo/backstage-plugin-common": "workspace:^", | ||
| "@openchoreo/backstage-plugin-openchoreo-ci": "workspace:^", | ||
| "@openchoreo/backstage-plugin-openchoreo-observability": "workspace:^", | ||
| "@openchoreo/backstage-plugin-openchoreo-perch": "workspace:^", |
Contributor
There was a problem hiding this comment.
This results in sibling package coupling, which is discouraged in the backstage ecosystem.
In the current implementation observerbility plugin directly depends on the perch plugin, and this dependency is only through InvestigateLogButton button.
Shall we pass this button as a prop to LogEntry so that we can pass this through the RuntimeLogs page via the shell app
yashodgayashan
force-pushed
the
perch
branch
2 times, most recently
from
c1c5cf3 to
8324f7b
Compare
Signed-off-by: yashodgayashan <yashodgayashan@gmail.com>
stefinie123
approved these changes
May 14, 2026
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.
Purpose
Add Perch the OpenChoreo assistant
Goals
Approach
Screen.Recording.2026-05-07.at.16.02.33.mov
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Chores / Configuration
Documentation
Tests