🐛 Fix custom target order handling #2719
Merged
Conversation
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
sjd78
added
the
cherry-pick/release-0.8
WalkthroughReplace local target ordering with a server-backed ordered list ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as CustomTargetForm
participant UI as MigrationTargets
participant API as Backend
participant Settings as ui.target.order
rect `#E8F8F5`
Note over User,Form: Create target (new flow)
User->>Form: Submit new target
Form->>API: createTargetAsync
API-->>Form: New Target (with id)
Form->>Settings: targetOrderMutateAsync (append new id)
Settings-->>Form: Order updated
Form-->>UI: notify saved (awaited)
UI->>API: useFetchTargets -> returns targets + targetsInOrder
API-->>UI: targetsInOrder
UI->>UI: render using targetsInOrder
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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: 1
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-targets/migration-targets.tsx (1)
218-236: Don't drop all visible targets when the order setting is unavailable.
filteredTargetsInOrderreturns an empty list whenever the setting query is still loading or errors. That leaves the gallery blank (and permanently empty on an error) even thoughfilteredTargetsalready contains the data fromuseFetchTargets, defeating the PR’s goal of always showing new targets.Fall back to the filtered targets when the order setting isn’t ready, and only apply the ordered/append logic once we actually have the setting:
- if (!targetOrderSetting.isSuccess) { - return []; - } + if (!targetOrderSetting.isSuccess || !Array.isArray(targetOrderSetting.data)) { + return filteredTargets; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/src/app/pages/applications/analysis-wizard/set-targets.tsx(2 hunks)client/src/app/pages/migration-targets/components/custom-target-form.tsx(5 hunks)client/src/app/pages/migration-targets/migration-targets.tsx(7 hunks)client/src/app/queries/settings.ts(1 hunks)client/src/app/queries/targets.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-18T23:21:22.818Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2472
File: client/src/app/pages/controls/job-functions/job-functions.tsx:56-58
Timestamp: 2025-07-18T23:21:22.818Z
Learning: In the tackle2-ui codebase, mutation hooks in the queries files already handle cache invalidation by calling queryClient.invalidateQueries() in their onSuccess callbacks. This ensures immediate UI updates after CRUD operations without needing additional manual invalidation in the UI components.
Applied to files:
client/src/app/queries/settings.tsclient/src/app/queries/targets.ts
📚 Learning: 2025-10-03T14:26:45.291Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2631
File: client/src/app/api/rest/applications.ts:14-15
Timestamp: 2025-10-03T14:26:45.291Z
Learning: In the tackle2-ui codebase, delete and update REST functions in files under `client/src/app/api/rest/` return `AxiosPromise<void>` (which resolves to `AxiosResponse<void>`) rather than unwrapped `Promise<void>`. This return type is expected and correctly handled by the React Query mutation hooks in `client/src/app/queries/`, which accept promise-compatible types including AxiosPromise.
Applied to files:
client/src/app/pages/migration-targets/components/custom-target-form.tsxclient/src/app/queries/targets.ts
📚 Learning: 2025-07-21T06:01:43.078Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2467
File: client/src/app/queries/generators.ts:20-20
Timestamp: 2025-07-21T06:01:43.078Z
Learning: The tackle2-ui codebase uses React Query v4 (tanstack/react-query v4.22.0), where the onError callback in query configurations is still valid and supported. The deprecation of onError callbacks only applies to React Query v5, so suggestions about removing them are premature until the codebase is upgraded.
Applied to files:
client/src/app/queries/targets.ts
📚 Learning: 2025-07-29T14:20:50.580Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2468
File: client/src/app/pages/source-platforms/usePlatformProviderList.ts:1-3
Timestamp: 2025-07-29T14:20:50.580Z
Learning: TargetCard component is focused on transformation targets, not source platform providers. It imports Target/TargetLabel models and operates independently from usePlatformProviderList. The codebase maintains clear separation between source platforms (managed by usePlatformProviderList) and transformation targets (managed by TargetCard).
Applied to files:
client/src/app/pages/migration-targets/migration-targets.tsxclient/src/app/pages/applications/analysis-wizard/set-targets.tsx
🧬 Code graph analysis (3)
client/src/app/pages/migration-targets/components/custom-target-form.tsx (5)
client/src/app/api/models.ts (1)
Target(557-567)client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/targets.ts (3)
useCreateFileMutation(138-159)useCreateTargetMutation(115-136)useUpdateTargetMutation(69-89)client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)client/src/app/queries/settings.ts (1)
useSettingMutation(20-32)
client/src/app/queries/targets.ts (4)
client/src/app/Constants.ts (1)
DEFAULT_REFETCH_INTERVAL(16-16)client/src/app/api/models.ts (1)
Target(557-567)client/src/app/api/rest.ts (1)
getTargets(314-315)client/src/app/queries/settings.ts (1)
useSetting(9-19)
client/src/app/pages/migration-targets/migration-targets.tsx (3)
client/src/app/queries/targets.ts (1)
useFetchTargets(19-67)client/src/app/api/models.ts (1)
Target(557-567)client/src/app/hooks/table-controls/useLocalTableControls.ts (1)
useLocalTableControls(12-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
client/src/app/pages/migration-targets/components/custom-target-form.tsx
Show resolved
Hide resolved
ibolton336
approved these changes
Nov 10, 2025
Resolves: https://issues.redhat.com/browse/MTA-6287 Resolves: konveyor#2708 All targets will new be displayed, even if they are not in the target order setting. The target query hook now will return the server ordering of the targets, and ordering by the "ui.target.order" setting. All targets are included in both lists. Any unordered targets is added to the end of the ordered list. By putting this logic in the target query hook, target order can be used the same way everywhere it is needed. On the migration-targets page, the targets are now displayed in the order specified by the "ui.target.order" setting. Any targets not in the setting are added to the end of the list. On the custom-target-form, updated the onSubmit handler to both create a new target and add it to the target order. This is done awaiting async mutations to help ensure the operations finish before the create modal is closed. Signed-off-by: Scott J Dickerson <[email protected]>
Signed-off-by: Scott J Dickerson <[email protected]>
sjd78
force-pushed
the
custom_target_ui.order
branch
from
2e29a90 to
0b79f1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/src/app/pages/migration-targets/components/custom-target-form.tsx (1)
397-412: Silent error handling hides mutation failures.The empty catch blocks on lines 401-403 and 409-411 silently swallow all errors. While the mutation hooks have their own error handling (notifications), the caller has no way to detect if these operations failed, which could be important for debugging or recovery logic.
Note: The race condition where
targetOrdercould be empty if the setting hasn't loaded was already flagged in previous review comments. As discussed, this will be addressed via conditional rendering in a separate issue.Consider at minimum logging errors for observability:
try { const response = await createTargetAsync(payload); newTargetId = response.data.id; } catch { - /* ignore */ + console.error("Failed to create target"); } // Add the new target to the target order and wait for it to complete before continuing. if (newTargetId !== undefined) { try { await targetOrderMutateAsync([...targetOrder, newTargetId]); } catch { - /* ignore */ + console.error("Failed to update target order"); } }
🧹 Nitpick comments (2)
client/src/app/pages/migration-targets/components/custom-target-form.tsx (1)
82-126: Good consolidation of mutation hooks.The
useTargetQueryHookshook centralizes mutation logic that was previously scattered throughout the component, improving maintainability.One observation:
onUpdateTargetFailure(line 110) is a no-op. If error notifications aren't needed for update failures, consider documenting why or removing the empty handler.client/src/app/pages/migration-targets/migration-targets.tsx (1)
97-142: Remove unnecessaryasynckeyword.The function is marked
asyncon line 97 but contains noawaitexpressions. Theasynckeyword is unnecessary and can be removed.Apply this diff:
- const onCustomTargetModalSaved = async (response: AxiosResponse<Target>) => { + const onCustomTargetModalSaved = (response: AxiosResponse<Target>) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/src/app/pages/applications/analysis-wizard/set-targets.tsx(2 hunks)client/src/app/pages/migration-targets/components/custom-target-form.tsx(5 hunks)client/src/app/pages/migration-targets/migration-targets.tsx(7 hunks)client/src/app/queries/settings.ts(1 hunks)client/src/app/queries/targets.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/app/queries/targets.ts
- client/src/app/pages/applications/analysis-wizard/set-targets.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-18T23:21:22.818Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2472
File: client/src/app/pages/controls/job-functions/job-functions.tsx:56-58
Timestamp: 2025-07-18T23:21:22.818Z
Learning: In the tackle2-ui codebase, mutation hooks in the queries files already handle cache invalidation by calling queryClient.invalidateQueries() in their onSuccess callbacks. This ensures immediate UI updates after CRUD operations without needing additional manual invalidation in the UI components.
Applied to files:
client/src/app/queries/settings.ts
📚 Learning: 2025-07-29T14:20:50.580Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2468
File: client/src/app/pages/source-platforms/usePlatformProviderList.ts:1-3
Timestamp: 2025-07-29T14:20:50.580Z
Learning: TargetCard component is focused on transformation targets, not source platform providers. It imports Target/TargetLabel models and operates independently from usePlatformProviderList. The codebase maintains clear separation between source platforms (managed by usePlatformProviderList) and transformation targets (managed by TargetCard).
Applied to files:
client/src/app/pages/migration-targets/components/custom-target-form.tsxclient/src/app/pages/migration-targets/migration-targets.tsx
📚 Learning: 2025-10-03T14:26:45.291Z
Learnt from: sjd78
Repo: konveyor/tackle2-ui PR: 2631
File: client/src/app/api/rest/applications.ts:14-15
Timestamp: 2025-10-03T14:26:45.291Z
Learning: In the tackle2-ui codebase, delete and update REST functions in files under `client/src/app/api/rest/` return `AxiosPromise<void>` (which resolves to `AxiosResponse<void>`) rather than unwrapped `Promise<void>`. This return type is expected and correctly handled by the React Query mutation hooks in `client/src/app/queries/`, which accept promise-compatible types including AxiosPromise.
Applied to files:
client/src/app/pages/migration-targets/components/custom-target-form.tsx
🧬 Code graph analysis (2)
client/src/app/pages/migration-targets/components/custom-target-form.tsx (6)
client/src/app/api/models.ts (1)
Target(557-567)client/src/app/components/NotificationsContext.tsx (1)
NotificationsContext(31-33)client/src/app/queries/targets.ts (3)
useCreateFileMutation(138-159)useCreateTargetMutation(115-136)useUpdateTargetMutation(69-89)client/src/app/utils/utils.ts (1)
getAxiosErrorMessage(10-16)client/src/app/api/rest.ts (1)
updateTarget(304-305)client/src/app/queries/settings.ts (1)
useSettingMutation(20-32)
client/src/app/pages/migration-targets/migration-targets.tsx (2)
client/src/app/queries/targets.ts (1)
useFetchTargets(19-67)client/src/app/hooks/table-controls/useLocalTableControls.ts (1)
useLocalTableControls(12-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: build-and-upload-for-global-ci
🔇 Additional comments (6)
client/src/app/queries/settings.ts (1)
20-31: LGTM! Improved cache invalidation specificity.The change from invalidating all setting queries to invalidating only the specific setting key is a good refinement. This prevents unnecessary refetches of unrelated settings and aligns with React Query best practices.
Based on learnings.
client/src/app/pages/migration-targets/components/custom-target-form.tsx (1)
49-54: LGTM! Props updated to support target ordering.The addition of
targetOrderenables the form to coordinate target ordering after creation, which aligns with the PR's objective to centralize ordering logic.client/src/app/pages/migration-targets/migration-targets.tsx (4)
53-53: LGTM! Using centralized target ordering.The switch to
targetsInOrderaligns with the PR's objective to centralize ordering logic in the target query hook.
169-175: LGTM! Drag handling updated for ordered targets.The active target resolution correctly uses
targetsInOrderto match the rendered gallery items.
218-236: LGTM! Correct implementation of ordered + unordered target display.The logic correctly:
- Orders targets according to
ui.target.ordersetting- Appends targets not in the setting to the end
- Maintains filtering behavior
This directly addresses the PR objective: "All targets will now be displayed even if they are not present in the target order setting."
316-322: LGTM! Target order passed to form for coordinated creation.Passing
targetOrderenables the CustomTargetForm to update the ordering after creating a new target, supporting the PR's goal to centralize ordering logic.
github-actions bot
pushed a commit
that referenced
this pull request
Nov 10, 2025
Resolves: https://issues.redhat.com/browse/MTA-6287 Resolves: #2708 All targets will new be displayed, even if they are not in the target order setting. The target query hook now will return the server ordering of the targets, and ordering by the "ui.target.order" setting. All targets are included in both lists. Any unordered targets is added to the end of the ordered list. By putting this logic in the target query hook, target order can be used the same way everywhere it is needed. On the migration-targets page, the targets are now displayed in the order specified by the "ui.target.order" setting. Any targets not in the setting are added to the end of the list. On the custom-target-form, updated the onSubmit handler to both create a new target and add it to the target order. This is done awaiting async mutations to help ensure the operations finish before the create modal is closed. ## Summary by CodeRabbit * **Refactor** * Unified target ordering so lists, gallery, filters and drag-and-drop use a single ordered source; creation/edit flows now receive and propagate ordering so new items appear in the expected place. * Target fetch API now exposes an ordered list consumers use for rendering and interactions. * **Bug Fixes** * Loading and save flows simplified to reduce flicker and missed updates. * Settings cache invalidation narrowed to specific keys for more reliable refreshes. --------- Signed-off-by: Scott J Dickerson <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
sjd78
pushed a commit
that referenced
this pull request
Nov 10, 2025
Resolves: https://issues.redhat.com/browse/MTA-6287 Resolves: #2708 Backport-Of: #2719 All targets will new be displayed, even if they are not in the target order setting. The target query hook now will return the server ordering of the targets, and ordering by the "ui.target.order" setting. All targets are included in both lists. Any unordered targets is added to the end of the ordered list. By putting this logic in the target query hook, target order can be used the same way everywhere it is needed. On the migration-targets page, the targets are now displayed in the order specified by the "ui.target.order" setting. Any targets not in the setting are added to the end of the list. On the custom-target-form, updated the onSubmit handler to both create a new target and add it to the target order. This is done awaiting async mutations to help ensure the operations finish before the create modal is closed. ## Summary by CodeRabbit * **Refactor** * Unified target ordering so lists, gallery, filters and drag-and-drop use a single ordered source; creation/edit flows now receive and propagate ordering so new items appear in the expected place. * Target fetch API now exposes an ordered list consumers use for rendering and interactions. * **Bug Fixes** * Loading and save flows simplified to reduce flicker and missed updates. * Settings cache invalidation narrowed to specific keys for more reliable refreshes. --------- Signed-off-by: Scott J Dickerson <[email protected]> Signed-off-by: Cherry Picker <[email protected]>
This was referenced Nov 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Resolves: https://issues.redhat.com/browse/MTA-6287
Resolves: #2708
All targets will new be displayed, even if they are not in the target order setting. The target query hook now will return the server ordering of the targets, and ordering by the "ui.target.order" setting. All targets are included in both lists. Any unordered targets is added to the end of the ordered list. By putting this logic in the target query hook, target order can be used the same way everywhere it is needed.
On the migration-targets page, the targets are now displayed in the order specified by the "ui.target.order" setting. Any targets not in the setting are added to the end of the list.
On the custom-target-form, updated the onSubmit handler to both create a new target and add it to the target order. This is done awaiting async mutations to help ensure the operations finish before the create modal is closed.
Summary by CodeRabbit
Refactor
Bug Fixes