OLS-2892: Use separate menu for the Ask/Troubleshooting mode#1837
OLS-2892: Use separate menu for the Ask/Troubleshooting mode#1837openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
kyoto
added
the
approved
openshift-ci-robot
added
the
jira/valid-reference
|
@kyoto: This pull request references OLS-2700 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:
📝 WalkthroughWalkthroughMoves mode selection out of the attach dropdown into a Select in the Prompt component, updates English translation keys (adds two placeholders, removes two chat labels), and adjusts the Cypress test to use and assert the new mode toggle behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Prompt as Prompt UI (Select)
participant Store as Redux Store
participant Test as Cypress Test
User->>Prompt: Click mode toggle / open Select
Prompt->>User: Render options ("Ask","Troubleshooting")
User->>Prompt: Select "Troubleshooting"
Prompt->>Store: dispatch setIsTroubleshooting(true)
Store-->>Prompt: state updated (isTroubleshooting = true)
Prompt-->>User: Update placeholder ("Describe the issue you're troubleshooting...")
Test->>Prompt: Verify toggle text == "Troubleshooting"
Test->>Prompt: Minimize & reopen, verify persisted toggle text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
/cherry-pick release-4.19 |
|
@kyoto: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/lgtm |
kyoto
force-pushed
the
mode-switcher-menu
branch
from
7970eae to
9f4b83f
Compare
|
New changes are detected. LGTM label has been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/Prompt.tsx (1)
757-763: Avoid silently treating unknown menu values as Ask.
value === 'troubleshooting'means any unexpected option value flips the state back to Ask. Since these mode strings are now reused across the selector and request payload, a typed constant/union would make future changes safer.♻️ Suggested direction
+type PromptMode = 'ask' | 'troubleshooting'; +const ASK_MODE: PromptMode = 'ask'; +const TROUBLESHOOTING_MODE: PromptMode = 'troubleshooting'; + const onModeMenuSelect = React.useCallback( (_ev: React.MouseEvent, value: string | number) => { - dispatch(setIsTroubleshooting(value === 'troubleshooting')); + if (value !== ASK_MODE && value !== TROUBLESHOOTING_MODE) { + return; + } + dispatch(setIsTroubleshooting(value === TROUBLESHOOTING_MODE)); setIsModeMenuOpen(false); }, [dispatch], );Also applies to: 774-790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Prompt.tsx` around lines 757 - 763, The handler onModeMenuSelect should not treat any unknown menu value as "Ask"; instead introduce a typed union or enum for mode strings (e.g., Mode = 'ask' | 'troubleshooting') and validate the incoming value against that type, only calling dispatch(setIsTroubleshooting(true/false)) for recognized values and otherwise no-op or log an error; update any other handlers (the related mode-selection block) to use the same typed constant/union so comparisons are explicit and future changes are type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/Prompt.tsx`:
- Around line 757-763: The handler onModeMenuSelect should not treat any unknown
menu value as "Ask"; instead introduce a typed union or enum for mode strings
(e.g., Mode = 'ask' | 'troubleshooting') and validate the incoming value against
that type, only calling dispatch(setIsTroubleshooting(true/false)) for
recognized values and otherwise no-op or log an error; update any other handlers
(the related mode-selection block) to use the same typed constant/union so
comparisons are explicit and future changes are type-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ec6ef4d-e8f1-40ac-8c2a-614eac52be2f
📒 Files selected for processing (3)
locales/en/plugin__lightspeed-console-plugin.jsonsrc/components/Prompt.tsxtests/tests/lightspeed-install.cy.ts
✅ Files skipped from review due to trivial changes (1)
- locales/en/plugin__lightspeed-console-plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests/lightspeed-install.cy.ts
kyoto
force-pushed
the
mode-switcher-menu
branch
from
9f4b83f to
ddea40c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Prompt.tsx (1)
806-811:⚠️ Potential issue | 🟡 MinorRename the plus-button label to match the new menu scope.
Lines 809-810 still say "Message actions", but this button now opens attachment actions only. That copy is misleading and less helpful for screen readers.
✏️ Suggested copy update
buttonProps={{ attach: { icon: <PlusIcon />, - tooltipContent: t('Message actions'), - 'aria-label': t('Message actions'), + tooltipContent: t('Add attachment'), + 'aria-label': t('Add attachment'), }, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Prompt.tsx` around lines 806 - 811, The attach button's label still reads "Message actions" but now opens attachment actions; update the tooltipContent and 'aria-label' passed in buttonProps.attach (where PlusIcon is used) to reflect the new scope (e.g., use t('Attachment actions') or a new i18n key like 'Attach actions' as appropriate) and ensure the corresponding translation entries are added/updated so screen readers and tooltips show the correct copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/Prompt.tsx`:
- Around line 806-811: The attach button's label still reads "Message actions"
but now opens attachment actions; update the tooltipContent and 'aria-label'
passed in buttonProps.attach (where PlusIcon is used) to reflect the new scope
(e.g., use t('Attachment actions') or a new i18n key like 'Attach actions' as
appropriate) and ensure the corresponding translation entries are added/updated
so screen readers and tooltips show the correct copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b6067d9b-21ef-4f51-9c12-01be38e9554f
📒 Files selected for processing (3)
locales/en/plugin__lightspeed-console-plugin.jsonsrc/components/Prompt.tsxtests/tests/lightspeed-install.cy.ts
✅ Files skipped from review due to trivial changes (1)
- locales/en/plugin__lightspeed-console-plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests/lightspeed-install.cy.ts
kyoto
force-pushed
the
mode-switcher-menu
branch
from
ddea40c to
463b549
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/tests/lightspeed-install.cy.ts (1)
372-373: Harden option selection to reduce future flakiness.At Line 372 and Line 382, broad text lookup can accidentally match unintended options if more selects are added. Prefer exact text matching.
♻️ Proposed hardening
- cy.contains('[role="option"]', 'Troubleshooting').click(); + cy.contains('[role="option"]', /^Troubleshooting$/).should('be.visible').click(); - cy.contains('[role="option"]', 'Ask').click(); + cy.contains('[role="option"]', /^Ask$/).should('be.visible').click();Also applies to: 382-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests/lightspeed-install.cy.ts` around lines 372 - 373, The option selection is using broad text matching which can mis-select later; update the cy.contains call for the option (the cy.contains('[role="option"]', 'Troubleshooting') invocation) to match the exact label (e.g., use an exact/anchored string or regex match) and assert the selected state with a strict equality assertion on modeToggle (replace the include.text assertion on modeToggle with a have.text or exact-match check) so the test only passes for the exact "Troubleshooting" option; apply the same change to the other occurrence around lines 382-383.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 372-373: The option selection is using broad text matching which
can mis-select later; update the cy.contains call for the option (the
cy.contains('[role="option"]', 'Troubleshooting') invocation) to match the exact
label (e.g., use an exact/anchored string or regex match) and assert the
selected state with a strict equality assertion on modeToggle (replace the
include.text assertion on modeToggle with a have.text or exact-match check) so
the test only passes for the exact "Troubleshooting" option; apply the same
change to the other occurrence around lines 382-383.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac77df5d-272e-41e9-97eb-f9a950d97418
📒 Files selected for processing (3)
locales/en/plugin__lightspeed-console-plugin.jsonsrc/components/Prompt.tsxtests/tests/lightspeed-install.cy.ts
✅ Files skipped from review due to trivial changes (1)
- locales/en/plugin__lightspeed-console-plugin.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Prompt.tsx
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 `@tests/tests/lightspeed-install.cy.ts`:
- Around line 173-179: The args mapping only handles inline tokens and misses
split tokens like ['--console-image', '<val>']; update the transformation that
builds const args so it handles both formats by iterating with index (or using
map with index) over
csv.spec.install.spec.deployments[0].spec.template.spec.containers[0].args and:
if an element startsWith('--console-image=') replace the RHS with
Cypress.env('CONSOLE_IMAGE'), and if an element === '--console-image' then
replace the following array element with Cypress.env('CONSOLE_IMAGE') (ensure
you adjust/skips the next element to avoid duplicating). Keep the reference
names args and the original args array and preserve other args unchanged.
🪄 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: Enterprise
Run ID: 5f61cb43-a1ee-44b1-a030-e385c9ca59b2
📒 Files selected for processing (1)
tests/tests/lightspeed-install.cy.ts
kyoto
changed the title
|
@kyoto: This pull request references OLS-2892 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. |
Made-with: Cursor
kyoto
force-pushed
the
mode-switcher-menu
branch
from
cbd38fa to
a5a59e7
Compare
|
CI failures now resolved by #1868 |
|
@kyoto: new pull request could not be created: failed to create pull request against openshift/lightspeed-console#release-4.19 from head openshift-cherrypick-robot:cherry-pick-1837-to-release-4.19: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between openshift:release-4.19 and openshift-cherrypick-robot:cherry-pick-1837-to-release-4.19"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"} 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 kubernetes-sigs/prow repository. |
|
release-4.19 back port was done manually: #1867 |
4 participants
Made-with: Cursor
Summary by CodeRabbit