✨ Update Migration waves table to use ActionsColumn #2675
Conversation
WalkthroughReplaced per-row Dropdown action menus with an RBAC-gated OverflowMenu containing a Pencil edit button and an ActionsColumn for manage/export/delete actions; removed local isRowDropdownOpen state and ConditionalTooltip usage; added two translation keys for no-applications messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Row as MigrationWavesRow
participant RBAC as RBAC
participant Menu as OverflowMenu + ActionsColumn
participant App as AppLogic
Row->>RBAC: check rbacWriteAccess
alt has write access
Row->>Menu: render (Pencil button + ActionsColumn)
Note right of Menu #E6F4EA: Actions: Edit, Manage, Export, Delete
Menu->>Row: user clicks Edit
Row->>App: open edit modal / navigate to edit
Menu->>Row: user clicks Export/Manage/Delete
Row->>App: perform action (with disabled/tooltip checks)
else no write access
Row-->>Row: no row actions rendered
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/src/app/pages/migration-waves/migration-waves.tsx (1)
473-537: Consider extracting action items configuration for readability.The ActionsColumn configuration (lines 484-535) contains substantial inline logic. For improved maintainability, consider extracting this into a helper function or useMemo hook, especially since the logic includes complex conditional checks for disabled states and tooltips.
Example refactor:
const getRowActions = (migrationWave: WaveWithStatus) => [ { title: t("composed.manage", { what: t("terms.applications").toLowerCase(), }), onClick: () => setWaveToManageModalState(migrationWave), isAriaDisabled: !applications || applications.length === 0, tooltipProps: !applications || applications.length === 0 ? { content: t("message.noApplicationsForAssignment") } : undefined, }, // ... other actions ]; // In render: <ActionsColumn items={getRowActions(migrationWave)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/pages/migration-waves/migration-waves.tsx(3 hunks)
🔇 Additional comments (2)
client/src/app/pages/migration-waves/migration-waves.tsx (2)
17-17: LGTM! New imports align with the refactoring.The added PatternFly imports (OverflowMenu, Tooltip, PencilAltIcon, ActionsColumn) support the new action cell implementation.
Also applies to: 27-27, 31-31, 33-33
495-495: Incorrect review comment: Theapplicationsvariable is always defined.The
useFetchApplications()hook (line 57 ofclient/src/app/queries/applications.ts) explicitly provides a default value:data: data || []. This guarantees thatapplicationsis always an array, never undefined. Accessing.lengthdirectly is safe and the defensive check is unnecessary.Note: The optional chaining used elsewhere in the file (lines 511, 517) applies to
migrationWave.applications(a property of the MigrationWave object), which is a different variable with different nullability semantics.Likely an incorrect or invalid review comment.
Signed-off-by: Maayan Hadasi <[email protected]>
mguetta1
force-pushed
the
migration-waves-table
branch
from
a10a91c to
791d7ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/pages/migration-waves/migration-waves.tsx (1)
672-678: Optional chaining bug can throw at runtime (undefined.includes)
tickets?.map(...)may yield undefined; calling.includeson it throws. Normalize to an array first.Apply:
-const hasExportableApplications = (tickets: Ticket[], applicationRefs: Ref[]) => - applicationRefs.some( - (applicationRef) => - !tickets - ?.map((ticket) => ticket?.application?.id) - .includes(applicationRef.id) - ); +const hasExportableApplications = (tickets: Ticket[] | undefined, applicationRefs: Ref[]) => { + const ticketAppIds = tickets?.map((t) => t?.application?.id) ?? []; + return applicationRefs.some((ref) => !ticketAppIds.includes(ref.id)); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/public/locales/en/translation.json(1 hunks)client/src/app/pages/migration-waves/migration-waves.tsx(3 hunks)
🔇 Additional comments (3)
client/src/app/pages/migration-waves/migration-waves.tsx (2)
17-18: Imports updated to support new action surface — OKAdded OverflowMenu, Tooltip, PencilAltIcon, and ActionsColumn match usage below.
Also applies to: 27-28, 31-34
506-523: Disregard this review comment—the analysis is incorrect.The
fullApplicationsproperty exists and is legitimate: it's defined in theWaveWithStatusinterface asfullApplications: Application[], and is populated viagetApplications(wave.applications)in waves-selector.ts.The code is correct as-is:
migrationWave.applicationsis aRef[], whilefullApplicationsisApplication[]—the expanded application data needed for the export function.- The
hasExportableApplicationsfunction expectsRef[]as its second parameter, so passingmigrationWave?.applicationsis appropriate for the gating logic.The suggested diff would actually break the export functionality by passing reference IDs instead of full application objects.
Likely an incorrect or invalid review comment.
client/public/locales/en/translation.json (1)
292-295: New i18n keys for tooltips — VerifiedKeys align with usages in migration-waves.tsx. Verification confirms both
message.noApplicationsForAssignmentandmessage.noApplicationsForExportare present in the only locale file (en/translation.json) with correct values. No additional locale files exist in the repository.
Signed-off-by: Maayan Hadasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/app/pages/migration-waves/migration-waves.tsx (1)
515-515: Optional: Remove redundant optional chainingLine 515 uses
migrationWave?.applications, butmigrationWaveis guaranteed to be non-null within the.map()iteration. The optional chaining beforeapplicationsis redundant.!hasExportableApplications( tickets, - migrationWave?.applications + migrationWave.applications )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/pages/migration-waves/migration-waves.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-20T07:18:26.598Z
Learnt from: mguetta1
PR: konveyor/tackle2-ui#2675
File: client/src/app/pages/migration-waves/migration-waves.tsx:508-510
Timestamp: 2025-10-20T07:18:26.598Z
Learning: In the tackle2-ui codebase, `WaveWithStatus` is an extended interface that includes `fullApplications: Application[]` property in addition to the base `MigrationWave` properties. The migration waves table component uses `WaveWithStatus` type, so `migrationWave.fullApplications` is a valid property reference.
Applied to files:
client/src/app/pages/migration-waves/migration-waves.tsx
🔇 Additional comments (1)
client/src/app/pages/migration-waves/migration-waves.tsx (1)
17-17: Excellent refactor to ActionsColumn pattern!The migration from per-row Dropdown to OverflowMenu + ActionsColumn is well-executed. Key improvements include:
- Resolved accessibility with
aria-labelon the edit button (line 477)- Proper i18n usage for tooltip messages (lines 500, 525)
- Safe optional chaining for
applications?.length(lines 496-497)- Clean RBAC gating for the entire action surface
Also applies to: 27-27, 31-31, 33-33, 473-539
| setApplicationsToExport( | ||
| migrationWave.fullApplications | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type consistency: fullApplications vs applications
Line 509 passes migrationWave.fullApplications (type Application[]) to setApplicationsToExport, but the state is typed as Ref[] | null (line 101-103). In contrast, the toolbar bulk export (line 345) uses item.applications (type Ref[]). This creates an inconsistency in what type ExportForm receives.
If ExportForm expects Ref[], passing Application[] may cause property access issues. If it expects Application[], the toolbar bulk export may be incorrect.
Run the following script to check the ExportForm component's prop type definitions:
If ExportForm expects Ref[], apply this fix:
onClick: () =>
setApplicationsToExport(
- migrationWave.fullApplications
+ migrationWave.applications
),If ExportForm expects Application[], update the state type and fix the toolbar usage:
- const [applicationsToExport, setApplicationsToExport] = React.useState<
- Ref[] | null
- >(null);
+ const [applicationsToExport, setApplicationsToExport] = React.useState<
+ Application[] | null
+ >(null);And on line 345:
onClick={() =>
setApplicationsToExport(
- selectedItems.flatMap((item) => item.applications)
+ selectedItems.flatMap((item) => item.fullApplications)
)
}🏁 Script executed:
#!/bin/bash
# Check ExportForm's applications prop type
echo "=== ExportForm prop type definition ==="
ast-grep --pattern 'interface $_ {
$$$
applications: $_
$$$
}' | rg -A2 -B2 "ExportForm"
echo -e "\n=== ExportForm component definition ==="
rg -n "ExportForm.*React.FC" -A 20 client/src/app/pages/migration-waves/components/export-form.tsx
echo -e "\n=== Type definitions for Application and Ref ==="
rg -n "^export interface (Application|Ref) " -A 10 client/src/app/api/models.tsLength of output: 1706
The web search returned general information, not the codebase types. Let me search the codebase directly for the type definitions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find WaveWithStatus or MigrationWave type definition
echo "=== Looking for WaveWithStatus or MigrationWave type ==="
rg -n "export (interface|type) (WaveWithStatus|MigrationWave)" -A 15 client/src/app/api/models.ts
echo -e "\n=== Alternative search for Wave types ==="
rg -n "^export (interface|type) .*Wave" -A 10 client/src/app/api/models.ts
echo -e "\n=== Check migration-waves.tsx for type usage ==="
rg -n "migrationWave\.(fullApplications|applications)" -B 2 -A 2 client/src/app/pages/migration-waves/migration-waves.tsx
Length of output: 384
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the correct paths for the files
echo "=== Locating models.ts ==="
fd -type f -name "models.ts"
echo -e "\n=== Locating migration-waves.tsx ==="
fd -type f -name "migration-waves.tsx"
echo -e "\n=== Locating export-form.tsx ==="
fd -type f -name "export-form.tsx"
Length of output: 611
---
🏁 Script executed:
```shell
#!/bin/bash
# Use correct fd syntax
echo "=== Locating models.ts ==="
fd -t f "models\.ts"
echo -e "\n=== Locating migration-waves.tsx ==="
fd -t f "migration-waves\.tsx"
Length of output: 150
---
🏁 Script executed:
```shell
#!/bin/bash
# Find WaveWithStatus or MigrationWave type definition
echo "=== Looking for WaveWithStatus or MigrationWave type ==="
rg -n "export (interface|type) (WaveWithStatus|MigrationWave)" -A 20 client/src/app/api/models.ts
echo -e "\n=== Check migrationWave property access in migration-waves.tsx ==="
rg -n "migrationWave\.(fullApplications|applications)" -B 3 -A 3 client/src/app/pages/migration-waves/migration-waves.tsx
Length of output: 3954
Use applications instead of fullApplications to match ExportForm's expected type
Line 509 passes migrationWave.fullApplications (type Application[]) to setApplicationsToExport, but the component expects Ref[] (see ExportForm's prop type at line 39).
The type hierarchy confirms the mismatch: WaveWithStatus has both applications: Ref[] (inherited from MigrationWave) and fullApplications: Application[]. Since line 345 correctly uses item.applications for the toolbar bulk export, line 509 should do the same:
setApplicationsToExport(
- migrationWave.fullApplications
+ migrationWave.applications
),📝 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.
| setApplicationsToExport( | |
| migrationWave.fullApplications | |
| ), | |
| setApplicationsToExport( | |
| migrationWave.applications | |
| ), |
🤖 Prompt for AI Agents
In client/src/app/pages/migration-waves/migration-waves.tsx around lines 508 to
510, the call to setApplicationsToExport is passing
migrationWave.fullApplications (Application[]), but ExportForm expects Ref[] via
applications; replace migrationWave.fullApplications with
migrationWave.applications to pass the correct type and match the toolbar bulk
export usage elsewhere.
2 participants
Related to #1318
Replacing #2109
Before:
After:
Summary by CodeRabbit