Conversation
kyoto
added
the
approved
openshift-ci-robot
added
the
jira/valid-reference
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
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:
📝 WalkthroughWalkthroughReplaced a local Modal implementation with PatternFly Modal primitives across multiple components, removed the custom CloseButton and a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
24-38:⚠️ Potential issue | 🟠 MajorAdd
i18nextas a direct runtime dependency.
src/error.tsimportsTFunctionfromi18nextdirectly, butpackage.jsondoes not declarei18next. Relying on a peer/transitive install is brittle and can break clean installs or stricter package managers. Addi18nextas an explicit dependency at a version compatible withreact-i18next(currently16.5.8).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 24 - 38, package.json is missing i18next even though src/error.ts imports TFunction from i18next; add i18next as a direct runtime dependency in package.json (use a version compatible with react-i18next@16.5.8) so the import in src/error.ts (TFunction) is satisfied and installs reliably rather than relying on a transitive/peer dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/AttachEventsModal.tsx`:
- Around line 180-185: The Cancel button is incorrectly using type="submit"
which can trigger form submission; update the Button component for Cancel (the
one with onClick={onClose} inside ActionGroup in AttachEventsModal) to use
type="button" instead of "submit" so clicking Cancel invokes onClose without
submitting the form; ensure the primary Attach button remains type="submit" and
nothing else changes (references: ActionGroup, Button with onClick={onClose},
onClose, onSubmit, numEvents).
In `@src/components/AttachLogModal.tsx`:
- Around line 487-488: The Cancel button in AttachLogModal is currently rendered
with type="submit" which will trigger form submission; change the Button element
that displays {t('Cancel')} to use type="button" instead so clicking it only
calls onClose and does not submit the form (look for the Button with
onClick={onClose}, {t('Cancel')} and update its type prop).
In `@src/components/AttachmentModal.tsx`:
- Around line 173-190: In AttachmentModal.tsx change the Button elements that
currently have type="submit" but are handled via click callbacks to use
type="button" instead; specifically update the Button rendering the Save action
(onSave) and the Button rendering the Edit action (setEditing) so they use
type="button" to avoid native form submission when the surrounding <Form> has no
submit handler; keep other buttons (e.g., Cancel) as-is.
In `@src/hooks/useLocationContext.ts`:
- Around line 15-19: The parsing useEffect in useLocationContext currently
replaces k8sModels with modelsRef (React.useRef) and thus drops model updates
from the effect dependencies; restore a stable representation of the model set
into the effect deps so re-parse runs when models meaningfully change (e.g.,
derive a stable key from k8sModels such as a sorted concatenation or JSON of
model identifiers/UIDs, or setState a version counter when k8sModels changes)
and include that key/version in the useEffect dependency array alongside
path/search; update references to modelsRef.current inside the effect as needed
so the effect re-runs when the model-set key changes while still avoiding
unstable object identity.
---
Outside diff comments:
In `@package.json`:
- Around line 24-38: package.json is missing i18next even though src/error.ts
imports TFunction from i18next; add i18next as a direct runtime dependency in
package.json (use a version compatible with react-i18next@16.5.8) so the import
in src/error.ts (TFunction) is satisfied and installs reliably rather than
relying on a transitive/peer dependency.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91b5bc71-6e34-45b4-8196-ba4c221c4b28
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
locales/en/plugin__lightspeed-console-plugin.jsonpackage.jsonsrc/components/AttachEventsModal.tsxsrc/components/AttachLogModal.tsxsrc/components/AttachmentModal.tsxsrc/components/CloseButton.tsxsrc/components/GeneralPage.tsxsrc/components/ImportAction.tsxsrc/components/Modal.tsxsrc/components/NewChatModal.tsxsrc/components/Prompt.tsxsrc/components/ResponseToolModal.tsxsrc/error.tssrc/hooks/useFirstTimeUser.tssrc/hooks/useHideLightspeed.tssrc/hooks/useIsDarkTheme.tssrc/hooks/useLocationContext.tssrc/hooks/useToolUIMapping.tstsconfig.json
💤 Files with no reviewable changes (4)
- src/components/Prompt.tsx
- src/components/CloseButton.tsx
- src/components/Modal.tsx
- locales/en/plugin__lightspeed-console-plugin.json
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
openshift-ci
Bot
added
the
needs-rebase
|
/retest |
openshift-ci
Bot
removed
the
needs-rebase
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
32-32: Consider exact-pinningi18nextfor consistency with other core dependencies.
react-i18nextis exact-pinned to16.5.8, whilei18nextuses a caret range. Aligning them would improve consistency across core runtime dependencies.Suggested diff
- "i18next": "^26.0.4", + "i18next": "26.0.4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 32, Change the i18next dependency from a caret range to an exact pin to match how react-i18next is pinned: update the "i18next" entry in package.json from "^26.0.4" to the exact version string "26.0.4" (no leading ^) so core runtime dependencies use consistent exact versions alongside "react-i18next".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/GeneralPage.tsx`:
- Line 505: Update the Prompt prop type to match the new scrollIntoView
signature: change PromptProps' scrollIntoView from () => void to (behavior?:
ScrollBehavior) => void (or React.Dispatch-like signature accepting an optional
ScrollBehavior) so it accepts an optional parameter; update the Prompt
component's prop type declaration in Prompt.tsx (the PromptProps interface/type
around line ~106) and any places that forward or call scrollIntoView to use the
optional parameter signature (calls can still omit the argument).
---
Nitpick comments:
In `@package.json`:
- Line 32: Change the i18next dependency from a caret range to an exact pin to
match how react-i18next is pinned: update the "i18next" entry in package.json
from "^26.0.4" to the exact version string "26.0.4" (no leading ^) so core
runtime dependencies use consistent exact versions alongside "react-i18next".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bdd08335-6bc5-4a9c-afe8-8e51392c9ecc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.tekton/integration-tests/lightspeed-console-pre-commit.yamllocales/en/plugin__lightspeed-console-plugin.jsonpackage.jsonsrc/components/AttachEventsModal.tsxsrc/components/AttachLogModal.tsxsrc/components/AttachmentModal.tsxsrc/components/CloseButton.tsxsrc/components/GeneralPage.tsxsrc/components/ImportAction.tsxsrc/components/Modal.tsxsrc/components/NewChatModal.tsxsrc/components/Prompt.tsxsrc/components/ResponseToolModal.tsxsrc/error.tssrc/hooks/useFirstTimeUser.tssrc/hooks/useHideLightspeed.tssrc/hooks/useIsDarkTheme.tssrc/hooks/useLocationContext.tssrc/hooks/useToolUIMapping.tstsconfig.json
💤 Files with no reviewable changes (4)
- src/components/Prompt.tsx
- locales/en/plugin__lightspeed-console-plugin.json
- src/components/CloseButton.tsx
- src/components/Modal.tsx
✅ Files skipped from review due to trivial changes (7)
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- src/components/ImportAction.tsx
- tsconfig.json
- src/error.ts
- src/components/AttachEventsModal.tsx
- src/components/NewChatModal.tsx
- src/hooks/useToolUIMapping.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/hooks/useHideLightspeed.ts
- src/hooks/useLocationContext.ts
- src/hooks/useIsDarkTheme.ts
- src/hooks/useFirstTimeUser.ts
|
yeah, 4.22 is going to fail for the time being as discussed with @kyoto . Will check for health of 4.22 cluster pools in prow since there's a chance to leverage those. EaaS (process currently used in konflux) only supports to 4.19 or 4.20 (can't remember rn) |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
@kyoto: This pull request references OLS-2770 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/AttachmentModal.tsx`:
- Around line 197-205: The conditional that shows the Revert button uses a falsy
check on attachment?.originalValue so empty-string originals ('') hide the
button; change the predicate in the AttachmentModal render (the block that
references attachment?.originalValue, attachment.originalValue !==
attachment.value, and onRevert) to explicitly test for null/undefined instead of
truthiness (e.g., use attachment?.originalValue != null &&
attachment.originalValue !== attachment.value) so empty strings still allow the
Revert button to render when values differ.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c30c4ac-bfa1-406c-a4f6-25547c2726c8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.tekton/integration-tests/lightspeed-console-pre-commit.yamllocales/en/plugin__lightspeed-console-plugin.jsonpackage.jsonsrc/components/AttachEventsModal.tsxsrc/components/AttachLogModal.tsxsrc/components/AttachmentModal.tsxsrc/components/CloseButton.tsxsrc/components/GeneralPage.tsxsrc/components/ImportAction.tsxsrc/components/Modal.tsxsrc/components/NewChatModal.tsxsrc/components/Prompt.tsxsrc/components/ResponseToolModal.tsxsrc/error.tssrc/hooks/useFirstTimeUser.tssrc/hooks/useHideLightspeed.tssrc/hooks/useIsDarkTheme.tssrc/hooks/useLocationContext.tssrc/hooks/useToolUIMapping.tstsconfig.json
💤 Files with no reviewable changes (3)
- src/components/Prompt.tsx
- src/components/CloseButton.tsx
- src/components/Modal.tsx
✅ Files skipped from review due to trivial changes (4)
- src/error.ts
- tsconfig.json
- src/components/AttachEventsModal.tsx
- src/components/ResponseToolModal.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/hooks/useHideLightspeed.ts
- src/hooks/useToolUIMapping.ts
- src/components/ImportAction.tsx
- src/hooks/useIsDarkTheme.ts
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- src/components/NewChatModal.tsx
- locales/en/plugin__lightspeed-console-plugin.json
- src/components/AttachLogModal.tsx
- src/hooks/useFirstTimeUser.ts
- src/hooks/useLocationContext.ts
| {attachment?.originalValue && attachment.originalValue !== attachment.value && ( | ||
| <SplitItem> | ||
| <ActionGroup> | ||
| <Button icon={<UndoIcon />} isDanger onClick={onRevert} variant="link"> | ||
| {t('Revert to original')} | ||
| </Button> | ||
| </ActionGroup> | ||
| </SplitItem> | ||
| )} |
There was a problem hiding this comment.
Revert action can disappear incorrectly for empty-string originals.
Line 197 uses a truthy check on attachment?.originalValue. If the original value is '', the Revert button is hidden even when current value differs.
🔧 Proposed fix
- {attachment?.originalValue && attachment.originalValue !== attachment.value && (
+ {attachment?.originalValue !== undefined &&
+ attachment.originalValue !== attachment.value && (
<SplitItem>
<ActionGroup>
<Button icon={<UndoIcon />} isDanger onClick={onRevert} variant="link">
{t('Revert to original')}
</Button>
</ActionGroup>
</SplitItem>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {attachment?.originalValue && attachment.originalValue !== attachment.value && ( | |
| <SplitItem> | |
| <ActionGroup> | |
| <Button icon={<UndoIcon />} isDanger onClick={onRevert} variant="link"> | |
| {t('Revert to original')} | |
| </Button> | |
| </ActionGroup> | |
| </SplitItem> | |
| )} | |
| {attachment?.originalValue !== undefined && | |
| attachment.originalValue !== attachment.value && ( | |
| <SplitItem> | |
| <ActionGroup> | |
| <Button icon={<UndoIcon />} isDanger onClick={onRevert} variant="link"> | |
| {t('Revert to original')} | |
| </Button> | |
| </ActionGroup> | |
| </SplitItem> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/AttachmentModal.tsx` around lines 197 - 205, The conditional
that shows the Revert button uses a falsy check on attachment?.originalValue so
empty-string originals ('') hide the button; change the predicate in the
AttachmentModal render (the block that references attachment?.originalValue,
attachment.originalValue !== attachment.value, and onRevert) to explicitly test
for null/undefined instead of truthiness (e.g., use attachment?.originalValue !=
null && attachment.originalValue !== attachment.value) so empty strings still
allow the Revert button to render when values differ.
Also fixes some type errors resulting from the package upgrdes
This has been renamed in the latest version of the plugin SDK
openshift-ci
Bot
removed
the
needs-rebase
4 participants
Summary by CodeRabbit
Refactor
Bug Fixes
Chores