Skip to content

Conversation: Add column lastUserMessageAt to then sort convo by last used #9788

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

Closed
wants to merge 1 commit into from

Conversation

PopDaph
Copy link
Contributor

@PopDaph PopDaph commented Jan 7, 2025

Description

Context: https://github.com/dust-tt/tasks/issues/1882

This is the first PR to create the new nullable column.
Once deployed I'll add the code to fill this one + use it to sort conversations.

Risk

Column is nullable so should be non-blocking, even it the table is large.

Deploy Plan

Merge
Run migration
Deploy front.

Copy link
Contributor

devloai bot commented Jan 7, 2025

Automatic code review is enabled for this repository. I will perform a review of this PR.

@PopDaph PopDaph requested a review from fontanierh January 7, 2025 12:34
Copy link
Contributor

@devloai devloai bot left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes adding the lastUserMessageAt column. The implementation is solid and takes a safe approach with the nullable column. I've suggested two improvements: adding an index to support efficient sorting and adding documentation for better maintainability. The PR is well-structured and ready to merge after addressing these suggestions.

@@ -0,0 +1,2 @@
-- Migration created on Jan 07, 2025
ALTER TABLE "public"."conversations" ADD COLUMN "lastUserMessageAt" TIMESTAMP WITH TIME ZONE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this column will be used for sorting conversations, it would be good to add an index on it. Consider adding:

Suggested change
ALTER TABLE "public"."conversations" ADD COLUMN "lastUserMessageAt" TIMESTAMP WITH TIME ZONE;
ALTER TABLE "public"."conversations" ADD COLUMN "lastUserMessageAt" TIMESTAMP WITH TIME ZONE;
CREATE INDEX CONCURRENTLY "conversations_last_user_message_at_idx" ON "public"."conversations" ("lastUserMessageAt");

The CONCURRENTLY option ensures the index is built without blocking writes to the table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice

@PopDaph PopDaph added the migration-ack 📁 Label to acknowledge that a migration is required. label Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

Warnings
⚠️

Files in **/lib/models/ have been modified and the PR has the migration-ack label. Don't forget to run the migration from prodbox.

Generated by 🚫 dangerJS against 2e8e90b

@PopDaph
Copy link
Contributor Author

PopDaph commented Jan 7, 2025

Wait actually I wonder if there's not a smarter way to do that using the ConversationParticipants table

@PopDaph PopDaph closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration-ack 📁 Label to acknowledge that a migration is required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant