Skip to content

Conversation

@mdmohsin7
Copy link
Member

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a filter to find only 'completed' conversations when searching for the closest conversation to a given timestamp. While the added filter is correct, the surrounding Firestore query is invalid and will cause a runtime error. The query attempts to use range filters on two different fields and orders by a third field, which violates Firestore's query constraints. I've left a critical comment with details on how to resolve this.

db.collection('users')
.document(uid)
.collection(conversations_collection)
.where(filter=FieldFilter('status', '==', ConversationStatus.completed))
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

While filtering by status is correct, the overall query is invalid and will cause a runtime error due to Firestore limitations. Specifically:

  1. The query uses range filters on two different fields (finished_at on line 962 and started_at on line 963), which is not allowed. A Firestore query can only have range filters on a single field.
  2. The query uses range filters, but the first order_by clause is on a different field (created_at on line 964). If a range filter is present, the first order_by must be on the same field as the range filter.

This query needs to be refactored. A possible solution is to use only one range filter in the query (e.g., on started_at) and then filter the remaining conditions (e.g., finished_at) in memory on the client side.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants