fix: thread list state order after dragable #5141
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
louis-menlo
approved these changes
May 29, 2025
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to b0b8189 in 1 minute and 47 seconds. Click for details.
- Reviewed
109lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ThreadList.tsx:269
- Draft comment:
Avoid mutating the props array using sort(); consider cloning the array (e.g., [...threads].sort(...)) to prevent potential side effects. - Reason this comment was not posted:
Comment was on unchanged code.
2. web-app/src/containers/ThreadList.tsx:328
- Draft comment:
Using index as a key in the list can lead to issues with component identity during drag-and-drop. Use thread.id as a unique key instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_mbsHvghlFPa3g8fy
You can customize 
| @@ -35,7 +35,26 @@ export function DataProvider() { | |||
|
|
|||
| useEffect(() => { | |||
| fetchThreads().then((threads) => { | |||
| setThreads(threads) | |||
| // Sort threads by order if available, otherwise by updated time | |||
| const sortedThreads = threads.sort((a, b) => { | |||
There was a problem hiding this comment.
Sort is performed directly on the fetched threads array. Cloning it before sorting (e.g., [...threads].sort(...)) would avoid unintended mutations.
Suggested change
| const sortedThreads = threads.sort((a, b) => { | |
| const sortedThreads = [...threads].sort((a, b) => { |
| @@ -268,9 +268,14 @@ function ThreadList({ threads }: ThreadListProps) { | |||
|
|
|||
| const sortedThreads = useMemo(() => { | |||
| return threads.sort((a, b) => { | |||
There was a problem hiding this comment.
The sorting comparator logic is duplicated in both ThreadList and DataProvider. Consider extracting it into a shared utility to reduce maintenance overhead.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 820b378 in 1 minute and 48 seconds. Click for details.
- Reviewed
152lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useThreads.ts:38
- Draft comment:
Avoid directly mutating threads (e.g. setting thread.order) in setThreads; consider using an immutable approach (e.g. mapping to new objects) to prevent side‐effects. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/services/threads.ts:58
- Draft comment:
Good update: Propagating the order using 'order: thread.order' (with fallback via e.metadata?.order) ensures proper thread ordering. Verify that thread.order is always defined as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/hooks/useThreads.ts:55
- Draft comment:
Formatting improvements (e.g. consistent trailing commas and removal of unnecessary semicolons) enhance readability; no action needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_RQ0cW8pK7TVaWfgP
You can customize
| const reorderedThreads = [createdThread, ...existingThreads] | ||
|
|
||
| // Use setThreads to handle proper ordering (this will assign order 1, 2, 3...) | ||
| get().setThreads(reorderedThreads) |
There was a problem hiding this comment.
Calling get().setThreads inside a set() callback (in createThread) causes nested state updates. Consider merging updates into a single set call to simplify state management.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 683e9b8 in 1 minute and 17 seconds. Click for details.
- Reviewed
48lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/providers/DataProvider.tsx:1
- Draft comment:
Removed unused useThreads import and setThreads usage. Ensure thread state is now managed elsewhere (e.g., in ThreadList). - Reason this comment was not posted:
Comment looked like it was already resolved.
2. web-app/src/providers/DataProvider.tsx:36
- Draft comment:
Thread sorting logic and default order assignment were removed. Confirm that ordering is now correctly managed (perhaps in ThreadList) to support drag-and-drop. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the ordering is correctly managed after removing the sorting logic. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
Workflow ID: wflow_WqI3WMuH1BqA0eXd
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Describe Your Changes
This pull request focuses on improving thread sorting logic and cleaning up code formatting in the
ThreadListandDataProvidercomponents. The most significant changes include refining the thread sorting mechanism to handle missingordervalues, ensuring consistent drag-and-drop behavior, and removing unnecessary semicolons for cleaner syntax.Thread Sorting Enhancements:
web-app/src/containers/ThreadList.tsx: Updated the sorting logic inThreadListto prioritize threads with anordervalue, followed by updated time for threads without anorder. This ensures a consistent and predictable sorting mechanism.web-app/src/providers/DataProvider.tsx: Enhanced thread initialization by sorting threads and assigning defaultordervalues to those without one, ensuring compatibility with future drag-and-drop operations.Code Formatting Improvements:
web-app/src/containers/ThreadList.tsx: Removed unnecessary semicolons across multiple sections of the code for cleaner syntax. [1] [2] [3]Fixes Issues
Self Checklist
Important
Improves thread sorting logic and code formatting in
ThreadList.tsxandDataProvider.tsx, handling missingordervalues and removing unnecessary semicolons.ThreadList.tsxto prioritize threads withorder, then by updated time iforderis missing.DataProvider.tsx, threads are initialized with defaultordervalues for future drag-and-drop operations.ThreadList.tsxfor cleaner syntax.setThreadsinuseThreads.tsto assignordervalues during thread initialization.createThreadandupdateThreadinthreads.tsto handleordermetadata.This description was created by
for 683e9b8. You can customize this summary. It will automatically update as commits are pushed.