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
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.
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 👍 / 👎.
…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.
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 👍 / 👎.
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.
This branch has not been deployed
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