Skip to content

Conversation

elianiva
Copy link

@elianiva elianiva commented Sep 6, 2025

Related GitHub Issue

Closes: #7496

Roo Code Task Context (Optional)

Description

This PR introduces a UI for custom reordering of the API profiles in addition to the already existing alphabetical ordering.

Test Procedure

Test the feature as shown in the video below.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Screen.Recording.2025-09-06.at.10.36.39.mov

Documentation Updates

There should be a section showing off this reordering feature.

Additional Notes

Get in Touch

@elianiva


Important

Adds custom reordering of API profiles in ApiConfigSelector with drag-and-drop support and updates i18n files for localization.

  • Behavior:
    • Adds custom reordering of API profiles in ApiConfigSelector.tsx, supporting both alphabetical and custom order.
    • Implements drag-and-drop functionality for reordering when in custom mode.
  • UI Components:
    • Updates ApiConfigSelector to include drag-and-drop handlers and reorder mode controls.
    • Adds buttons for switching between alphabetical and custom sort modes.
  • Testing:
    • Adds test in ApiConfigSelector.spec.tsx for drag-and-drop reordering functionality.
  • Localization:
    • Updates i18n files for multiple languages to include new strings related to reordering functionality.

This description was created by Ellipsis for 5ce0f51. You can customize this summary. It will automatically update as commits are pushed.

@elianiva elianiva requested review from mrubens, cte and jr as code owners September 6, 2025 04:26
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. UI/UX UI/UX related or focused labels Sep 6, 2025
Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! I've reviewed the API profile reordering feature and found several issues that need attention before merging.

Critical Issues (Must Fix):

  1. Missing persistence of custom order - The component doesn't persist the custom order to storage. When users reload VSCode, their custom ordering will be lost. The component should communicate with the extension backend to save/load the custom order preference.

  2. Missing parent callback handlers - The component accepts onSortModeChange and onCustomOrderChange props but never calls them. These callbacks should be invoked when the sort mode or custom order changes to notify the parent component (lines 133-140 and 168-180 in ApiConfigSelector.tsx).

Important Suggestions (Should Consider):

  1. Incomplete implementation - The PR only addresses API profiles reordering but issue #7496 also requests mode selector reordering. Should this be addressed in a separate PR or included here?

  2. Accessibility concerns - The drag-and-drop functionality lacks keyboard navigation support. Users who rely on keyboard navigation cannot reorder items. Consider adding keyboard shortcuts (e.g., Ctrl+Up/Down) for accessibility.

  3. UX improvement needed - When switching from custom to alphabetical sort, the custom order is lost without warning. Consider showing a confirmation dialog or preserving the custom order for when users switch back.

Minor Improvements (Nice to Have):

  1. Visual feedback enhancement - The drag-over indicator (border-top) could be improved with a more visible drop zone indicator or preview of where the item will be placed.

  2. Test coverage - While there's a test for drag-and-drop, it could be expanded to cover edge cases like dragging pinned items, canceling reorder mode, and switching between sort modes.

Overall, the implementation is well-structured and the UI looks good. The main concerns are around persistence and parent component integration which are critical for the feature to work properly across sessions.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 6, 2025
@elianiva
Copy link
Author

elianiva commented Sep 6, 2025

ah right forgot about the persistence, brb

@elianiva
Copy link
Author

elianiva commented Sep 6, 2025

I've addressed the critical issues, lmk if we want keyboard navigation.
I tried adding keyboard navigation previously but found several issues with it so I didn't include it in this PR. I'm not sure how important is that :/

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 6, 2025
@elianiva
Copy link
Author

elianiva commented Sep 7, 2025

Screen.Recording.2025-09-07.at.09.12.45.mov

managed to make keyboard navigation works (ignore the pink outline, it's using system's colour, it should be default to blue for most people)

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Sep 8, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 8, 2025
@daniel-lxs
Copy link
Collaborator

daniel-lxs commented Sep 8, 2025

Hey @elianiva thank you for your contribution!

One observation about the state management: The dual state approach with persistedCustomOrder and internalCustomOrder (lines 47, 62, 65-66 in ApiConfigSelector.tsx) adds some complexity. Have you considered simplifying this to a single source of truth?

The current pattern could potentially lead to sync issues between the two states. Perhaps you could rely solely on the persisted state from the extension context and update it directly via vscode.postMessage, letting the context update flow back through props. This would eliminate the need for the internal fallback state and make the data flow more predictable.

Just a thought to consider for reducing complexity and potential edge cases! The implementation is solid otherwise.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap Sep 8, 2025
@elianiva
Copy link
Author

I've updated so that it uses a single source of truth, also fixed a reordering issue due to unnecessary complexity when using keyboard navigation.

@hannesrudolph
Copy link
Collaborator

@brunobergher before I get the merge conflicts on this fixed, thoughts?

@brunobergher
Copy link
Collaborator

Cool! I didn't realize this need but it makes sense.
I'd just leave the ChatArea dropdowns simple and focused just on use, and move this functionality to the dropdown under settings, which is where I tihnk it makes sense to deeply customize things. I'm even open to making that an already expanded list (say 8-10 items talls) instead of a dropdown in that context.

That way we avoid the always prominent mention of "drag to reorder" for people who just want to switch providers or modes.

Is that fair?

@elianiva
Copy link
Author

Cool! I didn't realize this need but it makes sense. I'd just leave the ChatArea dropdowns simple and focused just on use, and move this functionality to the dropdown under settings, which is where I tihnk it makes sense to deeply customize things. I'm even open to making that an already expanded list (say 8-10 items talls) instead of a dropdown in that context.

That way we avoid the always prominent mention of "drag to reorder" for people who just want to switch providers or modes.

Is that fair?

Ah yes, agreed, if we move it inside the settings then we can directly show the items instead of making it as a dropdown toggle.

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Sep 29, 2025
@elianiva
Copy link
Author

Screen.Recording.2025-09-29.at.15.02.43.mov

Here's what I came up with based on previous feedback, would love to have some feedback on this.

The delay comes from the fact that we're relying on the server state instead of optimistic UI update. I'm not sure whether if it's worth it to update the UI optimistically and revert on failed call. I chose the simplest implementation first, I can always add it later if it's needed.

@brunobergher
Copy link
Collaborator

I haven't tested it deeply, but overall it's looking good!

One question: do you think the "enter reorder mode" toggle is needed? I'm wondering if we couldn't already land users that, and instead of having a mode switcher, that button could be a "Reset to Alphabetical" or something. Fewer steps.

Not a blocker though, this is good UI work.

@elianiva
Copy link
Author

One question: do you think the "enter reorder mode" toggle is needed? I'm wondering if we couldn't already land users that, and instead of having a mode switcher, that button could be a "Reset to Alphabetical" or something. Fewer steps.

I tried doing that at first, but it felt a bit awkward to me. If the user wants to edit the config they would need to click on the item they want to edit, but at the same time it's also used to drag / reorder. Not sure if it's just a me thing.

Another solution is making it clickable, and only draggable if you hold it on the handle, but my thought was I'm afraid that it'd be too small for users to click, I think it's nicer if the user can just drag the entire thing freely.

As for the alphabetical order, I kept it on the popup selector so people can switch between them easily. We have 'A-Z' and 'Custom' button below the selection list in the popup.

To be honest, I have no idea which one fits best because I haven't use it long enough to form a solid opinion.
I'm fine with either options, so feel free to tell me which one you prefer and I can update the PR accordingly :)

@brunobergher
Copy link
Collaborator

If you tried it out and settled here, sounds good. This is not too hard to change later, the core mechanics are legit.

@elianiva
Copy link
Author

If you tried it out and settled here, sounds good. This is not too hard to change later, the core mechanics are legit.

Great! I agree we can always change it later, it's a relatively simple change.
I'll clean up the diff and double check everything to see if I missed anything tomorrow morning, seems like there's some translation leftovers and unrelated changes which isn't good.

@elianiva elianiva force-pushed the feat/reorder-profile branch from 54212aa to fe9dfa2 Compare September 30, 2025 13:35
@elianiva elianiva force-pushed the feat/reorder-profile branch from fe9dfa2 to b1e93ed Compare September 30, 2025 13:39
Copy link
Author

@elianiva elianiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is pretty much ready to review.

While I was cleaning up this PR, I made a script to check redundant translation keys and found that we have quite a bit of leftovers.

Checking components directory (locale: en)
Found 1324 translation keys
Found 1036 used keys in source code

Checking src directory (locale: en)
Found 286 translation keys
Found 356 used keys in source code

@cte Do you think it's a good idea if I make a separate PR for adding the script and clean up the unused translations?

z.object({
id: z.string(),
index: z.number(),
pinned: z.boolean(),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the pinned field here because I'm not sure what to do with the pinned configs for now. I tried doing separate reordering but that feels a bit overkill, so I reverted it and thought of leaving this here just in case we might want to do something with it in the future.

; LINQ expressions
(query_expression) @definition.linq_expression
`

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea why this even got in, probably due to formatting? I'm not sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR - Changes Requested size:XXL This PR changes 1000+ lines, ignoring generated files. UI/UX UI/UX related or focused
Projects
Status: PR [Changes Requested]
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Allow sorting and pinning for Modes and API Profiles under main input
4 participants