Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
βΉοΈ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with π while any review is running, comments if it has suggestions, and reacts with π once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f3749ae70
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* `files` is absent on most rows and `[]` on rows that lost their last attachment, | ||
| so both have to read as "no attachments". */ | ||
| if (hasFiles === true) { | ||
| filters.push({ files: { $exists: true, $not: { $size: 0 } } } as FilterQuery<IConversation>); |
There was a problem hiding this comment.
Filtering only on Conversation.files excludes conversations whose attachments are stored on messages. In particular, the Assistants flow writes uploads to userMessage.files and attempts to write convo.file_ids (api/server/services/Threads/manage.js:94-108), while the conversation schema defines only files (packages/data-schemas/src/schema/convo.ts:368-370); therefore Assistants conversations containing uploaded files never satisfy this predicate. The facet needs to query or maintain a field derived from all persisted message attachments rather than relying solely on this conversation field.
Useful? React with πΒ / π.
| const date = new Date(raw); | ||
| if (Number.isNaN(date.getTime())) { | ||
| return { invalid: true }; | ||
| } | ||
| return { date }; |
There was a problem hiding this comment.
new Date(raw) is substantially more permissive than the documented ISO 8601 contract and normalizes invalid calendar dates. For example, updatedAfter=2026-02-30 is accepted as March 2, while updatedAfter=1 becomes January 1, 2001, so malformed requests silently apply an unintended cutoff instead of returning the promised 400 response. Validate the syntax and calendar value before accepting the parsed date.
Useful? React with πΒ / π.
| if (firstValue(query.hasFiles) === 'true') { | ||
| filters.hasFiles = true; | ||
| } | ||
| if (firstValue(query.sharedOnly) === 'true') { | ||
| filters.sharedOnly = true; |
There was a problem hiding this comment.
Only the exact string "true" is recognized, while every other supplied valueβincluding typos such as hasFiles=tru, objects, or unsupported casingβis silently treated as an absent filter. In those cases the endpoint returns a broader conversation list even though the parser's contract says malformed facets fail the request. Explicitly accept true and false, and return 400 for any other supplied value for both flags.
Useful? React with πΒ / π.
| const MAX_ENDPOINT_FILTERS = 50; | ||
|
|
||
| /** One endpoint name. Long enough for a custom endpoint, short enough to bound the query. */ | ||
| const MAX_ENDPOINT_LENGTH = 128; |
There was a problem hiding this comment.
The new count and name-length limits are fixed at 50 and 128, so deployments exposing more endpoints or longer custom endpoint identifiers reject otherwise legitimate filter requests with no librechat.yaml setting to adjust the policy. Add corresponding configSchema fields with these values as defaults and inject the resolved limits into the parser.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with πΒ / π.
| if (sharedOnly === true) { | ||
| const sharedIds = await getSharedConversationIds(user); |
There was a problem hiding this comment.
With sharedOnly=true, getSharedConversationIds has already established that every conversation admitted by this filter has an active share, but the function later calls attachSharedFlags unconditionally at packages/data-schemas/src/methods/conversation.ts:3306, issuing another SharedLink query after the conversation query. This adds a redundant serial database round trip to every filtered page and can even leave a qualifying row without isShared if that second best-effort lookup fails. Retain the ID set and mark the returned rows directly, or skip the second lookup for this path.
Useful? React with πΒ / π.
4f3749a to
05569a9
Compare
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05569a917a
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !projectId || projectId === 'unassigned' || /^[a-f\d]{24}$/i.test(projectId); | ||
|
|
||
| router.get('/', async (req, res) => { | ||
| router.get('/', configMiddleware, async (req, res) => { |
There was a problem hiding this comment.
On a cache-cold sidebar request, adding configMiddleware makes the list wait for effective-config resolution before starting getConvosByCursor; getAppConfig first resolves user principals and then applicable config overrides, which can add two serial database reads to the visible conversation's load even when the request uses no new facets. Resolve these limits without the per-user config middleware, or reuse configuration/principals already loaded for the request so ordinary list requests retain their previous latency.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in a01e7e9: the list route no longer runs configMiddleware; the limits are deployment-level and come from the in-memory base config (getAppConfig({ baseOnly: true })). It also exposed that conversationList never reached AppConfig at all, so AppService now carries it (service.spec covers both).
| const ISO_DATE = | ||
| /^(\d{4})-(\d{2})-(\d{2})(?:[T ](\d{2}):(\d{2})(?::(\d{2})(?:\.(\d{1,3}))?)?(Z|[+-](?:0\d|1[0-4])(?::?[0-5]\d)?)?)?$/; |
There was a problem hiding this comment.
When a caller supplies an accepted timestamp without an offset, such as updatedAfter=2026-09-01T12:00:00, new Date() interprets it in the server process's local timezone even though these parameters are documented as absolute cutoffs. The identical request therefore becomes 16:00Z on a New York host but 03:00Z on a Tokyo host and can return different conversations after a deployment or timezone change; require Z or an explicit offset whenever a time component is present, or deliberately interpret zone-less timestamps as UTC.
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in 272ce3c: a timestamp without Z or an offset is read as UTC, the same as a date-only cutoff, so the host zone no longer changes the result. Covered in filters.spec.
05569a9 to
9f15215
Compare
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f1521533c
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tags = Array.isArray(req.query.tags) ? req.query.tags : [req.query.tags]; | ||
| } | ||
|
|
||
| const { conversationList } = await getAppConfig({ baseOnly: true }); |
There was a problem hiding this comment.
This adds a service call and configuration behavior directly to /api, although the repository boundary requires CJS routes to retain only wiring and delegate behavior to packages/api. Keeping getAppConfig here also couples the route to the application singleton and makes the filter path harder to test or reuse; move the list handler/config resolution into the TypeScript module and inject the config reader and database method from this route.
AGENTS.md reference: AGENTS.md:L53-L57
Useful? React with πΒ / π.
There was a problem hiding this comment.
| maxEndpointFilters: z.number().int().min(1).max(1_000).optional(), | ||
| /** One endpoint name. Long enough for a custom endpoint, short enough to | ||
| * bound the query. Omission keeps 128. */ | ||
| maxEndpointNameLength: z.number().int().min(1).max(1_024).optional(), |
There was a problem hiding this comment.
When either setting is omitted, the validated config leaves it undefined and packages/api/src/conversations/filters.ts supplies a separate hard-coded default, so the schema is not the required source of truth and future changes can make validation, documentation, and runtime behavior drift. The fresh evidence in this head is that both final schema fields remain .optional() while 50 and 128 are still defined separately in DEFAULT_CONVERSATION_LIST_LIMITS; apply those defaults in configSchema and consume the resolved values instead.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed on 6302c93: 50 and 128 are now field defaults on conversationListConfigSchema, AppService resolves them through loadConversationListConfig (an invalid block keeps the defaults), and the parser takes the resolved values with no constants of its own. service.spec covers unset, partial and invalid blocks.
| if (sharedOnly === true) { | ||
| const activeShares = await getSharedConversationIds(user); |
There was a problem hiding this comment.
When hasFiles=true and sharedOnly=true, the full message-attachment lookup is awaited before this independent shared-link lookup begins. This adds the two database latencies on every combined-filter page, and when the user has no active shares it scans and materializes the attachment history only to discard it at the early return below. Start the user-scoped lookups together, or resolve the cheaper short-circuit before performing the attachment query, rather than introducing serial reads on the conversation-list path.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed on 6302c93: the attachment and share lookups start together with Promise.all, so a combined page pays one round trip of latency instead of two.
9f15215 to
6302c93
Compare
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ae89b064
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ISO_DATE = | ||
| /^(\d{4})-(\d{2})-(\d{2})(?:[T ](\d{2}):(\d{2})(?::(\d{2})(?:\.(\d{1,3}))?)?(Z|[+-](?:0\d|1[0-4])(?::?[0-5]\d)?)?)?$/; |
There was a problem hiding this comment.
The public parameter is documented as ISO 8601, but this expression accepts only one to three fractional-second digits. Common serializers emit six or nine digitsβfor example, Python can send 2026-09-01T00:00:00.123456Zβso otherwise valid client requests receive a 400 response. Accept an arbitrary nonempty fractional part and truncate it to the millisecond precision supported by Date rather than rejecting the timestamp.
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in bd77594 (commit 'Accept list cutoffs with microsecond or finer fractions'): any fractional part is accepted and Date keeps the milliseconds; filters.spec covers 6 and 9 digits, with and without Z.
a5ae89b to
bd77594
Compare
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd77594597
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd77594597
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!Message) { | ||
| return null; | ||
| } | ||
| return Message.find({ user, 'files.0': { $exists: true } }).distinct('conversationId'); |
There was a problem hiding this comment.
When a user attaches a file through a mid-run steer, applySteerPart persists the reference under message.content[].files (api/server/controllers/agents/client.js:814-827) rather than top-level message.files; a conversation with only such an attachment therefore never enters this ID set and is omitted by hasFiles=true. Fresh evidence beyond the earlier top-level-message fix is that the replay path explicitly reads these durable nested references at api/app/clients/BaseClient.js:1957-1969; include content-part file references in the attachment predicate as well.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in 05eac27. The facet now matches files on message content parts (content.files.0) as well as message.files. Covered by a steer-part case in conversation.spec and by the list-files-facet-reads-message-attachments scenario, which seeds a chat whose only file rides on a steer; it passes on 05eac27 in desktop light, desktop dark and mobile.
|
@codex review |
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05eac27f6f
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return Message.find({ | ||
| user, | ||
| $or: [{ 'files.0': { $exists: true } }, { 'content.files.0': { $exists: true } }], |
There was a problem hiding this comment.
When a stored message's only attachment uses a provider-native content[].file, content[].image_file, or content[].file_id reference, this predicate omits its conversation because it checks only content.files.0. Fresh evidence beyond the earlier content[].files thread is that StoredMessagePartInput supports the singular file and image_file shapes, while modelBoundContent.ts explicitly discovers and replays all three forms; consequently hasFiles=true returns an incomplete list for these persisted messages. Extend the predicate to every supported content-part file-reference shape.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in 218d269. The facet now matches every content-part shape replay reads (modelBoundContent.ts:390-401): files[], file.file_id, image_file.file_id and a bare file_id. Three conversation.spec cases cover the singular shapes and fail without the change; the list-files-facet-reads-message-attachments scenario seeds an image_file-only chat and passes on 218d269 in desktop light, desktop dark and mobile.
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 218d269aee
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $or: [ | ||
| { 'files.0': { $exists: true } }, | ||
| { 'content.files.0': { $exists: true } }, |
There was a problem hiding this comment.
When a conversation's only file is an assistant or tool output, it is persisted under message.attachments rather than any of the paths in this predicate: BaseClient.js assigns completed artifactPromises to responseMessage.attachments, and the client renders those entries through ContentRender. With no user upload, Conversation.files also remains unset, so hasFiles=true omits conversations that visibly contain generated files; include attachment entries carrying a file_id in this query.
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in 6f6d2d6. The facet now reads attachments.file_id, so a chat whose only file is tool or assistant output qualifies; an attachment without a file_id (web search) does not. With message.files, attachments and every content-part shape, the predicate covers each Message field that holds a file. Covered by a conversation.spec case and by the list-files-facet-reads-message-attachments scenario, which seeds a tool-output-only chat and passes on 6f6d2d6.
218d269 to
6f6d2d6
Compare
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f6d2d6d41
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| user, | ||
| $or: [ | ||
| { 'files.0': { $exists: true } }, | ||
| { 'attachments.file_id': { $type: 'string' } }, |
There was a problem hiding this comment.
When a persisted tool/code result uses the supported download-only attachment shape (filename, filepath, and expiresAt, but no file_id), this condition is false, so a conversation whose only file has that shape is omitted by hasFiles=true. Fresh evidence beyond the prior attachments.file_id thread is the preserved historical shape in api/app/clients/specs/BaseClient.test.js:3299-3331 and the client rendering any attachment with a filepath in client/src/components/Chat/Messages/Content/Parts/Attachment.tsx:182-184; match valid filepath-only attachment records as well.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with πΒ / π.
There was a problem hiding this comment.
Fixed in 1313b8e. An attachment now qualifies on a file_id or a non-empty filepath, the same rule Attachment.tsx:182-184 uses to render one as a file, so a download-only code output counts and a web search result does not. Covered by the conversation.spec attachments case and by the list-files-facet-reads-message-attachments scenario, which seeds a download-only chat and passes on 1313b8e.
β¦sharing The list query takes five new facets. Dates and endpoints narrow it directly; hasFiles matches conversations that carry at least one file; sharedOnly resolves the user's live share links and matches their conversation ids, since a share expires and a denormalized flag on the conversation would outlive it. parseConversationListFilters owns what a valid facet is, so the route keeps the call and nothing else. A malformed date fails the request rather than being dropped: a filter that is quietly ignored answers with the conversations the user asked not to see. The endpoint list is bounded at 50 names of 128 characters to keep an unbounded $in out of the query. Two indexes back the new paths: user, archived, endpoint, updatedAt on conversations, and user, conversationId on shares. Cursor pagination is unchanged, so a filtered list pages the same way an unfiltered one does.
β¦parsing The hasFiles facet matched only the conversation's own files array, which the standard send flow never writes: uploads ride on user messages, so an ordinary chat with an attachment was invisible to the filter. The facet now OR-matches message-derived conversation IDs alongside the conversation-level array. Dates are validated as ISO 8601 before construction, so 2026-02-30 and a bare year no longer roll over into an unintended cutoff, and a mistyped hasFiles or sharedOnly value fails with 400 instead of reading as absent. The endpoint count and name-length limits moved to conversationList in configSchema and are injected into the parser, so a deployment serving more endpoints can raise them. The shared filter resolves its IDs with distinct and reuses the set to mark isShared, dropping the second SharedLink query on every filtered page.
The route mock re-implemented the parser loosely: it ignored the configured endpoint limits and dropped a mistyped flag, so a route that stopped forwarding conversationList or stopped answering 400 still passed. The mock now wraps the real parser, and the facet tests cover a mistyped flag and an endpoint list over the configured limit.
Each facet is exercised against the running API with its own endpoint name, so the rows a test seeds are the only rows the filter can return: date cutoffs on updated and created time, an OR-matched endpoint list, an attachment that only exists on a message, shares that are live, expiring or lapsed, malformed facets answering 400, and a list without facets returning what it did before.
conversationList never reached the resolved app config: AppService builds it from an explicit field list and the new knob was not on it, so a configured limit was silently ignored and the defaults always applied. It is now carried onto AppConfig. The list route also stops resolving the caller's merged config for it. The limits are deployment-level, and configMiddleware put principal and override reads in front of every cache-cold sidebar request, including ones with no facets; the base config is in memory.
A timestamp without Z or an offset was handed to new Date(), which reads it in the server's local zone, so the same request selected different conversations on hosts in different zones. It is now UTC, matching how a date-only cutoff is read.
With hasFiles and sharedOnly both on, the message-attachment lookup finished before the share lookup began, adding the two latencies on every page. Both are independent user-scoped reads, so they now run in parallel.
The 50 and 128 defaults lived in both the parser and the schema's comments. They are now field defaults on conversationListConfigSchema, AppService resolves them (an invalid block keeps the defaults), and the parser takes the resolved values. The base-config read moves out of the CJS route into resolveConversationListFilters, which receives getAppConfig from the route.
The base config sits behind the config cache, which can be Redis, so every sidebar request paid that round trip and could fail on it even though the limits bound only the endpoint facet. A request without endpoints now reads no config.
The cutoff pattern allowed at most three fractional digits, so a valid ISO 8601 timestamp from a serializer that emits microseconds (Python, most databases) answered 400. Any fraction is accepted now and Date keeps the milliseconds.
β¦he file facet A file attached through a mid-run steer is stored on the steer content part rather than on the message, so hasFiles=true left that conversation out.
Replay reads a file on a content part as files[], file.file_id, image_file.file_id or a bare file_id; the facet matched only files[].
β¦acet A file a tool produces is persisted on message.attachments, the one place on a message the facet did not read, so a chat whose only file was generated output was left out of hasFiles=true.
A code or tool result can persist an attachment with a filepath and no file_id; the client renders any attachment with a filepath as a file, so the facet now uses the same rule.
1313b8e to
c98f65d
Compare
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting β@codex reviewβ. βΉοΈ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with π. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch has not been deployed
2 participants
Summary
Part 3 of 6 of the quieter-layout stack. #16244 has merged, so this is based on
canary.The conversation list query takes five new facets: a date range, endpoints,
hasFilesandsharedOnly. Dates and endpoints narrow the query directly,hasFilesmatches conversations with a file on the conversation or on any of its messages, andsharedOnlyresolves the user's live share links, because a share expires and a denormalized flag on the conversation would outlive it.parseConversationListFiltersinpackages/apiowns what a valid facet is, so the route only calls it and returns its 400. A malformed date or a mistyped flag fails the request rather than being dropped, and the endpoint list is capped (50 names of 128 characters by default,conversationListinlibrechat.yaml) to keep an unbounded$inout of the query. Two indexes back the new paths, and cursor pagination is unchanged.Type of change
Testing
Tested environments/configuration:
mongodb-memory-serverAutomated tests:
packages/api:npx jest src/conversations/filters(passing)packages/data-schemas:npx jest src/methods/conversation.spec(passing)api:npx jest server/routes/__tests__/convos.spec(103 passing, through the real parser)@scenariotests ine2e/specs/mock/scenarios/conversation-list-filters.spec.ts(date cutoffs, endpoint list, message attachments, live and expired shares, malformed facets, unfiltered list), 18/18 passing across desktop light, desktop dark and mobilenpx tsc --noEmitinpackages/apiandpackages/data-schemasScreenshots / recordings
No user-facing change. The UI that uses these filters is the next PR in the stack.
Risk / compatibility
Adds two indexes:
{ user, isArchived, endpoint, updatedAt, _id }on conversations and{ user, conversationId }on shares. Requests without the new query parameters behave exactly as before.Checklist