🗂️ feat: Chat List Properties Menu and Project-Scoped Chats - #16246
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: 71f2c0e8ad
ℹ️ 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".
| updatedAfter, | ||
| createdAfter, | ||
| endpoints, | ||
| hasFiles, | ||
| sharedOnly, |
There was a problem hiding this comment.
Adding these facets to allConversations query variants also makes every existing cache writer facet-sensitive, but conversationInsertVerdict and conversationBelongsToListQuery in client/src/utils/convos.ts still consider only project, archive, tags, search, and sort. Consequently, while an endpoint/files/shared/date filter is active, upsertConvoInAllQueries, duplication/fork mutations, or SSE updates can insert or retain a conversation that the server excluded—for example, an unshared conversation appears in a sharedOnly list after an SSE update. Treat these facets as server-reconciled predicates or teach the writers how to evaluate them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| const updatedAfter = rangeCutoff(get(updatedRangeAtom)); | ||
| const createdAfter = rangeCutoff(get(createdRangeAtom)); |
There was a problem hiding this comment.
When either date range is selected, this derived atom computes the cutoff only when one of its source atoms changes; Jotai caches the derived value rather than reevaluating it on every render. If the app remains open across local midnight, “Today” continues using yesterday's midnight and the other windows likewise grow by a day, even when React Query refetches on focus, until the user changes or resets the facet. Schedule an update at the next local midnight or derive the cutoff from a day-valued atom that advances then.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb5842a: cutoffs derive from a day anchor that useFreshLocalDay advances at local midnight while the list is mounted; covered by facets.spec.
| }; | ||
| }); | ||
|
|
||
| export const resetFacetsAtom = atom(null, (_get, set) => { |
There was a problem hiding this comment.
These new atoms are global session state, but endSessionClientState in client/src/hooks/AuthContext.tsx resets only resetChatFilterSessionAtom; resetFacetsAtom is currently invoked only from filter UI actions. After logout or an in-app account switch, the next user therefore inherits the prior user's date, endpoint, attachment, and sharing filters and can land on an unexpectedly empty chat list. Wire this reset into the existing authentication-boundary cleanup.
Useful? React with 👍 / 👎.
| /** Which dead end this is decides both the line and the glyph above it: a search | ||
| * that found nothing, a filter that matched nothing, an empty archive, and an | ||
| * account with no chats yet are four different situations wearing one sentence. */ | ||
| let emptyLabel: TranslationKeys = 'com_ui_no_chats'; |
There was a problem hiding this comment.
Once the default request is scoped to projectId: 'unassigned', an empty response no longer proves that the account has no chats: it also occurs whenever every chat belongs to a project. In that common case the project rows are rendered above this state while the Chats section still displays com_ui_no_chats, contradicting the branch's stated “account is empty” meaning. Suppress this account-level state for the scoped list or use copy that explicitly describes the absence of unassigned chats.
Useful? React with 👍 / 👎.
|
|
||
| /** Searching reaches into the categories, so both lists have to be here rather than | ||
| * inside the submenu that normally owns them. Both are cached queries. */ | ||
| const { data: bookmarkData } = useGetConversationTags(); |
There was a problem hiding this comment.
For users whose role lacks BOOKMARKS:USE, showBookmarks is false and all bookmark UI is hidden, but opening the Filter submenu still runs this query unconditionally. The /api/tags router applies checkBookmarkAccess to every request, so these users receive a 403 (with React Query's default retries) merely for opening an otherwise permitted filter menu. Pass an enabled: showBookmarks option or mount the query only in the authorized branch.
Useful? React with 👍 / 👎.
danny-avila
added
the
🗺️ Chat UI Shell
71f2c0e to
fff951c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff951c2f1
ℹ️ 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 (typeof conversation.endpoint === 'string') { | ||
| if (!endpoints.includes(conversation.endpoint)) { | ||
| return 'skip'; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
When an already-cached conversation changes provider—for example, after the endpoint selector updates conversation.endpoint and an SSE sidebar update calls upsertConvoInAllQueries—this new skip result governs only insertion when the row is absent. The existing-row paths still check conversationBelongsToListQuery, which considers only project and archive state, while queryNeedsServerReconciliation also excludes endpoint filters; consequently, the conversation is rewritten but remains visible in its old endpoint-only list until a later refetch. This re-raises the earlier cache-writer concern with fresh evidence: the current patch added the endpoint verdict without applying it to existing rows. Evict an existing row on skip, or invalidate endpoint-filtered variants after these writes. CLAUDE.mdL338-L342
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b8e576a: list membership for existing rows now includes the endpoint, so upsert and in-place updates evict a row whose known endpoint left the facet (an endpoint-less row stays). convos.spec cases fail without it.
| * Search and the archived view stay whole: both are places the user goes to | ||
| * find something, and a project chat that appears in neither list nor result | ||
| * would have no way back. */ | ||
| projectId: isArchivedView || search.debouncedQuery ? undefined : 'unassigned', |
There was a problem hiding this comment.
When the initial projects request fails while the conversations request succeeds, this unconditional unassigned scope removes every project-owned active chat from Chats. ProjectsSection currently consumes only data and isLoading, so an error without cached data is rendered as an empty project list with a “New project” action rather than an error or retry; there is then no project row or fallback chat row through which the affected conversations can be opened. Gate this scope on usable project data, or expose the project failure and recovery path before hiding the fallback rows.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f142342799
ℹ️ 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".
| ? localize('com_ui_none') | ||
| : localize('com_ui_active_count', { count: narrowingCount }); | ||
|
|
||
| const totalCount = activeCount + facetCount; |
There was a problem hiding this comment.
When two or more bookmarks are selected, chatFilterCountAtom contributes every tag to activeCount, while the Filter row deliberately counts the entire bookmark group as one facet and multi-selected endpoints also count once. The trigger therefore announces values such as “Filters active: 3” while its only active Filter row says “1 active,” giving screen-reader users a contradictory filter count. Derive the trigger total from the same grouped bookmark/facet count used by filterValue, plus the view and sort deviations.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a94b76e1e9
ℹ️ 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".
a94b76e to
2aa610c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aa610c100
ℹ️ 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".
| maxEndpointFilters: resolveConversationListLimits(appConfig?.conversationList) | ||
| .maxEndpointFilters, |
There was a problem hiding this comment.
When a principal-scoped config override changes conversationList.maxEndpointFilters, this payload publishes the merged per-user value because appConfig comes from getAppConfigOptionsFromUser, while resolveConversationListFilters explicitly reads the deployment's baseOnly config. If an override raises the limit, the UI permits a selection that the list route rejects with 400; if it lowers the limit, valid selections are unnecessarily disabled. Resolve this startup field from the same base configuration used by the request parser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 34fc4e2: both list-filter limits now come from loadConversationListLimits in packages/api, which reads the base config the same way the list route does (the route calls it too), and the startup config publishes them as conversationListLimits. config.spec covers the configured, default and principal-override cases. endpoint-filter-stops-at-limit passes on c421468.
| const served = Object.keys(endpointsConfig ?? {}).filter( | ||
| (endpoint) => endpointsConfig?.[endpoint] != null, | ||
| ); |
There was a problem hiding this comment.
When a served custom endpoint's name exceeds conversationList.maxEndpointNameLength—which can also be configured below the 128-character default—this includes it as a selectable facet even though parseEndpoints rejects that same value. Selecting an otherwise usable endpoint therefore turns the Chats request into a 400 and displays the list error state. Publish and enforce the name limit in this control, or ensure configured endpoint names are exempt from the request-input limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
|
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: c4214685da
ℹ️ 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 scopeToUnassigned = | ||
| !isArchivedView && !search.debouncedQuery && !(projectsFailed && projectsData == null); |
There was a problem hiding this comment.
When the top-level projects request succeeds but ProjectChatsInline fails to load an expanded project's conversations, this condition still scopes the main list to unassigned chats. The child query ignores isError and treats missing data as com_ui_no_project_chats, so the project's unpinned chats disappear from the sidebar behind a false empty state with no retry. Keep fallback chats visible or add an error/retry path to each project list before relying on this exclusive scope.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61a0a73 (ProjectsSection.tsx): a project whose own chat request failed with no rows now shows an alert with Retry instead of the empty state, so its chats stay reachable while Chats is scoped to unassigned. Verified by project-chats-failure-offers-retry (fails that project's /api/convos request, asserts the alert, retries, sees the chat) on desktop light, dark and mobile.
The old menu listed every option at once. It now opens on three rows that state where the list stands, Show, Sort and Filter, each with its current value and a submenu holding the choices. Reading the list's state no longer means reading the whole menu. Filter holds the five facets and a search field that looks inside them, so typing openai finds the endpoint rather than the category that contains it, and typing a category name offers everything under it. Reset sits in the header next to the title, and the active facet count rides on the trigger. The facets are one derived atom, so the query key and the request are built from the same description of the filter. Date cutoffs snap to local midnight: taken from the current instant they would differ on every render and refetch the list each time. The panels also fade their content at the foot when a list runs past the fold, and the chat list's loading label shimmers the way every other in-flight label in the app does.
A chat assigned to a project was listed twice in one sidebar: once under the project and once in Chats. The chats list now asks for the chats that belong to no project, which the list query and its cache helpers already knew how to answer. Search and the archived view keep asking for everything. Projects is not rendered while a search is on, and an archived chat has no project row to appear under, so excluding them from either would leave a chat with no way back to it.
…-scoped Cache writers judged list membership only by project, archive, tags and sort, so an SSE update or a fork could insert a chat the server's filter had excluded. The endpoint and date facets are decided from the row itself, and the attachment and sharing facets mark their variants for a refetch, since only the server can place a row in them. The date cutoffs now derive from a day anchor a mounted consumer advances at local midnight, so a tab left open stops serving yesterday's today. Facets reset at the authentication boundary with the rest of the session state, the bookmark query stops firing for roles without BOOKMARKS:USE, an empty Chats section under existing projects says so instead of claiming the account is empty, and the design-rule violations this branch had suppressed in Conversations and ChatFilterMenu are fixed and their entries pruned.
An empty Chats list read as "No chats yet" whenever the projects query had not succeeded, so a slow or failed projects request claimed an account was empty when its chats may all live under projects. The account-level wording now needs a loaded, empty project list; otherwise the section says only that nothing sits outside a project.
…matches The endpoint facet only guarded insertion, so a chat already in an endpoint-filtered list stayed there after switching provider until the next refetch. List membership now includes the endpoint for existing rows as well; a row with no endpoint stays until the server decides.
Scoping Chats to unassigned chats hid every project chat behind the Projects section, which shows nothing when its request fails. A failed project list without cached data now leaves Chats unscoped, so those chats stay reachable, and an empty unscoped list reads as an empty account.
The trigger added one per selected bookmark while the Filter row counts the whole bookmark group as one, so a screen reader heard two different totals for the same state. Bookmarks now count once, like the endpoint facet.
The Endpoint facet let a user pick more endpoints than conversationList.maxEndpointFilters allows, and the list request past it answered 400, leaving Chats in its error state. The post-login startup config now publishes the limit, resolved by the same helper the list parser uses, and the facet disables further endpoints once the selection reaches it, with a note saying why.
The sidebar renders before the post-login startup config resolves, and until then the Endpoint facet had no limit to enforce, so a selection made in that window could pass conversationList.maxEndpointFilters. Endpoints are now offered disabled until that config has loaded.
…ocked A selected bookmark whose last chat dropped it fell out of both bookmark lists, so only Reset could clear it; a selected tag now stays listed. The Endpoint facet waits only while the startup config is loading, so a config that failed no longer locks every endpoint without saying why.
…ps listing it An endpoint that left the endpoints config, or a config that failed to load, dropped out of both endpoint lists while it stayed selected, so it kept narrowing the chats and could not be turned off. Both lists now come from one helper that keeps the selection listed.
The startup config published maxEndpointFilters from the caller's merged config while the list route reads the deployment's base config, so a principal override could let the sidebar allow a selection the route refused, or disable one it accepted; and the endpoint name limit was not published at all, so a long custom endpoint name could be selected and turn the list into a 400. Both limits now come from one loader the list route also uses and are published together as conversationListLimits; the Endpoint facet leaves out names past the length limit.
A selected bookmark that was deleted no longer came back from the tags query, so neither bookmark list could show it and only Reset cleared it. Both lists now come from one helper that keeps every selected bookmark.
Chats lists only chats outside any project, so a project whose own chat request failed hid its chats behind the empty state. The project now says its chats could not load and offers a retry.
c421468 to
61a0a73
Compare
|
@codex review Please review the current PR head 61a0a73. State the exact reviewed commit and ignore findings that apply only to earlier heads. Purpose supplied by the requester: 1 fix since reviewed head c421468: project chat list shows an error with retry when its own request fails (finding 4115607380); head also rebased onto canary |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ 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". |
2 participants
Summary
Part 4 of 6 of the quieter-layout stack. The parts below it (#16243, #16244, #16245) have merged, so this now sits directly on
canary.The chat list filter listed every option at once. It now opens on three rows (Show, Sort, Filter), each with its current value and a submenu holding the choices. Filter holds the five facets from the previous PR plus a search field that looks inside them. Reset sits beside the title, and the active facet count rides on the trigger. The facets are one derived atom, so the query key and the request come from the same description, and date cutoffs snap to local midnight so the list does not refetch on every render.
A chat assigned to a project was listed twice in the sidebar: under the project and again in Chats. Chats now asks only for chats that belong to no project. Search and the archived view still ask for everything, and so does a sidebar whose project list failed to load, so no chat is left with no way back to it. Cache writers only place a row in a filtered list when the row provably matches it; the attachment and sharing filters are refetched from the server instead.
Type of change
Testing
Tested environments/configuration:
canaryAutomated tests:
npx tsc --noEmitinclientandpackages/clientnpx eslinton every changed file, including theshadcn/*design rulesnpx jest --findRelatedTestsover the changed client files, passinge2e/specs/mock/scenarios/chat-filter-menu.spec.ts(8 tests: keyboard filtering, reset, date window, project scoping, a live reply under a server-only filter, reset on sign-out, a role without bookmarks, a failed project list), passing on desktop light, desktop dark and mobileScreenshots / recordings
Before is
canary; after is the top of this stack, so an image can also show changes from later PRs in the chain. Chromium, 1440x900 desktop and 390x844 mobile.Risk / compatibility
Uses the list filters from #16245, which is on
canary.Checklist