Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix thread pagination #10485

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Fix thread pagination #10485

merged 1 commit into from
Feb 25, 2025

Conversation

bosiraphael
Copy link
Contributor

Fixes #10308.

The issue came from the fact that the pagination inside the findAndCount wasn't working properly. The limit was applied on the joint table. Adding a groupBy fixes this, but since it isn't available on the findAndCount I had to modify the query using the query builder.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR fixes thread pagination and duplication issues in the messaging service by replacing the findAndCount method with a more precise query builder approach.

  • Implemented a two-step query process in getAndCountMessageThreads() that first gets thread IDs with proper pagination, then fetches full thread data
  • Added explicit groupBy('messageThread.id') to prevent duplicates in query results
  • Improved ordering with MAX(messages.receivedAt) to ensure consistent thread sorting
  • Changed from using Any to In TypeORM operator for better query performance
  • Fixed the load more functionality by properly applying pagination to the grouped results

Greptile AI: This PR fixes thread pagination and duplication issues in the messaging service by replacing the findAndCount method with a more precise query builder approach.

  • Implemented a two-step query process in getAndCountMessageThreads() that first gets thread IDs with proper pagination, then fetches full thread data
  • Added explicit groupBy('messageThread.id') to prevent duplicates in query results
  • Improved ordering with MAX(messages.receivedAt) to ensure consistent thread sorting
  • Changed from using Any to In TypeORM operator for better query performance
  • Fixed the load more functionality by properly applying pagination to the grouped results

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@Weiko Weiko merged commit 42ea023 into main Feb 25, 2025
35 checks passed
@Weiko Weiko deleted the r--fix-thread-duplication branch February 25, 2025 16:44
@guillim
Copy link
Contributor

guillim commented Mar 3, 2025

Do you think we could add a warning like a linter rule when someone uses findandcount and join ? just thinking KAISEN style :)

@Weiko
Copy link
Member

Weiko commented Mar 3, 2025

Do you think we could add a warning like a linter rule when someone uses findandcount and join ? just thinking KAISEN style :)

Yes!
We can change the code in the workspace repository class that extends typeorm repository since we own that part of the code. A linter rule is fine too if it's too complex 👍 @guillim

@guillim
Copy link
Contributor

guillim commented Mar 3, 2025

True. I understand your suggestion, I will try to work on that on my future "slack friday"

I guess the last thing not 100% clear to me is how to detect accuratly that the where condition is on a joint entity not on the current one. STANDBY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Duplicate threads and load more error in the emails tab
3 participants