feat(web): add project filter and flat sidebar grouping - #4334
danieliser wants to merge 30 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds persisted sidebar filters, archived-thread support, explicit unread tracking, first-seen completion detection, flat thread-list rendering, and active-thread visit tracking. ChangesSidebar thread settings and snapshots
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CompletedThreadHook
participant UiStateStore
participant Sidebar
participant ChatView
CompletedThreadHook->>UiStateStore: mark newly completed thread unread
UiStateStore->>Sidebar: expose explicit unread marker
Sidebar->>Sidebar: apply status and sidebar filters
ChatView->>UiStateStore: mark active thread visited
UiStateStore->>Sidebar: clear explicit unread marker
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:XXL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
@codex review |
|
@claude review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts (1)
25-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the environment reconnect scenario.
Consider adding a case where
environmentSnapshotIdsomits an environment on one call (simulating a disconnect) and then includes it again withpreviouslySeenThreadKeysByEnvironmentcarried from the first call — asserting that a thread completed during the gap is still reported innewlyUnreadThreads. This directly covers the gap flagged inuseMarkFirstSeenCompletedThreadsUnread.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts` around lines 25 - 72, Add a regression test in the resolveFirstSeenCompletedThreads suite that performs two calls: first seed and retain an environment’s seen-thread state, then call again after that environment was omitted from environmentSnapshotIds and reintroduced with a newly completed thread. Carry forward nextSeenThreadKeysByEnvironment between calls and assert the completed thread appears in newlyUnreadThreads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts`:
- Around line 32-51: Update resolveFirstSeenCompletedThreads so
nextSeenThreadKeysByEnvironment preserves entries and seen-thread-key sets from
previouslySeenThreadKeysByEnvironment, including environments absent from the
current environmentSnapshotIds. Merge or clone existing sets for reconnecting
environments, while initializing genuinely new environments empty, so a
transient snapshot gap does not reseed threads as unseen.
---
Nitpick comments:
In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts`:
- Around line 25-72: Add a regression test in the
resolveFirstSeenCompletedThreads suite that performs two calls: first seed and
retain an environment’s seen-thread state, then call again after that
environment was omitted from environmentSnapshotIds and reintroduced with a
newly completed thread. Carry forward nextSeenThreadKeysByEnvironment between
calls and assert the completed thread appears in newlyUnreadThreads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bd0c054-3401-47cb-a1b2-6eb96fa4bf4e
📥 Commits
Reviewing files that changed from the base of the PR and between b44ed83 and 963ce6fe3187464508c190392fba2c14d064b7fd.
📒 Files selected for processing (16)
apps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/SidebarV2.tsxapps/web/src/components/ThreadStatusIndicators.tsxapps/web/src/components/ui/menu.tsxapps/web/src/environmentGrouping.test.tsapps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.tsapps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.tsapps/web/src/routes/__root.tsxapps/web/src/sidebarProjectGrouping.tsapps/web/src/uiStateStore.test.tsapps/web/src/uiStateStore.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
| export function resolveFirstSeenCompletedThreads(input: { | ||
| readonly threads: ReadonlyArray<FirstSeenThreadInput>; | ||
| readonly environmentSnapshotIds: ReadonlyArray<EnvironmentId>; | ||
| readonly previouslySeenThreadKeysByEnvironment: ReadonlyMap<EnvironmentId, ReadonlySet<string>>; | ||
| }): { | ||
| readonly nextSeenThreadKeysByEnvironment: Map<EnvironmentId, Set<string>>; | ||
| readonly newlyUnreadThreads: ReadonlyArray<{ | ||
| readonly threadKey: string; | ||
| readonly completedAt: string | null; | ||
| }>; | ||
| } { | ||
| const snapshotEnvironmentIds = new Set(input.environmentSnapshotIds); | ||
| const nextSeenThreadKeysByEnvironment = new Map<EnvironmentId, Set<string>>(); | ||
| const newlyUnreadThreads: Array<{ | ||
| readonly threadKey: string; | ||
| readonly completedAt: string | null; | ||
| }> = []; | ||
| for (const environmentId of snapshotEnvironmentIds) { | ||
| nextSeenThreadKeysByEnvironment.set(environmentId, new Set()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Environment disconnect/reconnect silently wipes "first-seen" tracking, causing missed unread flags.
nextSeenThreadKeysByEnvironment is rebuilt only from the current snapshotEnvironmentIds, and the full ref is overwritten each run (line 95). If an environment's snapshot momentarily goes to None (a transient disconnect) and later comes back, its previously-tracked seen-thread-keys are dropped. On reconnect, previouslySeenThreadKeysByEnvironment.get(environmentId) is undefined again, so the "seed without marking unread" branch fires for every thread in that environment — including brand-new threads that completed entirely while disconnected. This defeats the hook's stated purpose (surfacing first-seen completed threads) for the exact scenario it's meant to catch, and it's currently untested.
🐛 Proposed fix: preserve seen-thread-keys across a temporary snapshot gap
const snapshotEnvironmentIds = new Set(input.environmentSnapshotIds);
- const nextSeenThreadKeysByEnvironment = new Map<EnvironmentId, Set<string>>();
+ const nextSeenThreadKeysByEnvironment = new Map<EnvironmentId, Set<string>>(
+ Array.from(input.previouslySeenThreadKeysByEnvironment, ([environmentId, threadKeys]) => [
+ environmentId,
+ new Set(threadKeys),
+ ]),
+ );
const newlyUnreadThreads: Array<{
readonly threadKey: string;
readonly completedAt: string | null;
}> = [];
for (const environmentId of snapshotEnvironmentIds) {
- nextSeenThreadKeysByEnvironment.set(environmentId, new Set());
+ if (!nextSeenThreadKeysByEnvironment.has(environmentId)) {
+ nextSeenThreadKeysByEnvironment.set(environmentId, new Set());
+ }
}This keeps the existing tests passing (traced each case) while ensuring a reconnecting environment is compared against its real prior "seen" baseline instead of being treated as brand new.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function resolveFirstSeenCompletedThreads(input: { | |
| readonly threads: ReadonlyArray<FirstSeenThreadInput>; | |
| readonly environmentSnapshotIds: ReadonlyArray<EnvironmentId>; | |
| readonly previouslySeenThreadKeysByEnvironment: ReadonlyMap<EnvironmentId, ReadonlySet<string>>; | |
| }): { | |
| readonly nextSeenThreadKeysByEnvironment: Map<EnvironmentId, Set<string>>; | |
| readonly newlyUnreadThreads: ReadonlyArray<{ | |
| readonly threadKey: string; | |
| readonly completedAt: string | null; | |
| }>; | |
| } { | |
| const snapshotEnvironmentIds = new Set(input.environmentSnapshotIds); | |
| const nextSeenThreadKeysByEnvironment = new Map<EnvironmentId, Set<string>>(); | |
| const newlyUnreadThreads: Array<{ | |
| readonly threadKey: string; | |
| readonly completedAt: string | null; | |
| }> = []; | |
| for (const environmentId of snapshotEnvironmentIds) { | |
| nextSeenThreadKeysByEnvironment.set(environmentId, new Set()); | |
| } | |
| export function resolveFirstSeenCompletedThreads(input: { | |
| readonly threads: ReadonlyArray<FirstSeenThreadInput>; | |
| readonly environmentSnapshotIds: ReadonlyArray<EnvironmentId>; | |
| readonly previouslySeenThreadKeysByEnvironment: ReadonlyMap<EnvironmentId, ReadonlySet<string>>; | |
| }): { | |
| readonly nextSeenThreadKeysByEnvironment: Map<EnvironmentId, Set<string>>; | |
| readonly newlyUnreadThreads: ReadonlyArray<{ | |
| readonly threadKey: string; | |
| readonly completedAt: string | null; | |
| }>; | |
| } { | |
| const snapshotEnvironmentIds = new Set(input.environmentSnapshotIds); | |
| const nextSeenThreadKeysByEnvironment = new Map<EnvironmentId, Set<string>>( | |
| Array.from(input.previouslySeenThreadKeysByEnvironment, ([environmentId, threadKeys]) => [ | |
| environmentId, | |
| new Set(threadKeys), | |
| ]), | |
| ); | |
| const newlyUnreadThreads: Array<{ | |
| readonly threadKey: string; | |
| readonly completedAt: string | null; | |
| }> = []; | |
| for (const environmentId of snapshotEnvironmentIds) { | |
| if (!nextSeenThreadKeysByEnvironment.has(environmentId)) { | |
| nextSeenThreadKeysByEnvironment.set(environmentId, new Set()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts` around lines 32
- 51, Update resolveFirstSeenCompletedThreads so nextSeenThreadKeysByEnvironment
preserves entries and seen-thread-key sets from
previouslySeenThreadKeysByEnvironment, including environments absent from the
current environmentSnapshotIds. Merge or clone existing sets for reconnecting
environments, while initializing genuinely new environments empty, so a
transient snapshot gap does not reseed threads as unseen.
![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: 963ce6fe31
ℹ️ 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".
| [archivedSnapshots], | ||
| ); | ||
| const providerEntries = useMemo( | ||
| () => deriveProviderInstanceEntries(serverProviders), |
There was a problem hiding this comment.
When a Source filter is selected in a multi-environment sidebar, this map only contains provider instance IDs from the primary server. Threads from secondary environments can use custom provider instance IDs that exist only in that environment's config; those lookups return null, and matchesSidebarThreadFilters rejects them whenever filters.sources is non-empty even if the driver kind matches. Build the instance-to-driver lookup from each thread's environment (or from all environment configs) so source filtering does not hide valid secondary-environment threads.
Useful? React with 👍 / 👎.
| if (thread.isExplicitlyUnread) { | ||
| return "unread"; | ||
| } |
There was a problem hiding this comment.
If a user marks a running or approval/input-blocked thread unread, this early return classifies it as unread before checking needs_attention or working. That makes the thread disappear from the Working/Needs attention status filters even though it is still active or actionable; treat explicit unread as a fallback/secondary state after the live blockers are classified.
Useful? React with 👍 / 👎.
| const threadKey = scopedThreadKey(scopeThreadRef(thread.environmentId, thread.id)); | ||
| const providerInstanceId = | ||
| thread.session?.providerInstanceId ?? thread.modelSelection.instanceId; | ||
| return matchesSidebarThreadFilters({ |
There was a problem hiding this comment.
When Include archived is enabled, this filter allows archived snapshot rows into visibleProjectThreads, and that same array builds orderedProjectThreadKeys for shift-click range selection. Shift-selecting across an archived row can therefore mark it selected and include it in the bulk-action count, but the bulk handlers later resolve only live readThreadShell() entries and silently skip the archived thread; keep archived rows out of the range-selection order while still rendering them.
Useful? React with 👍 / 👎.
| previousThreadKeys !== undefined && | ||
| !previousThreadKeys.has(threadKey) && | ||
| thread.latestTurn?.state === "completed" |
There was a problem hiding this comment.
When a completed thread is archived, the next seen snapshot no longer contains its key; if the user later unarchives that same old thread, this condition treats it as newly seen and marks it unread. Unarchiving is a user-initiated restore rather than a new completion, so retain previously seen keys (or seed archived keys) to avoid old archived threads reappearing as unread.
Useful? React with 👍 / 👎.
| previousThreadKeys !== undefined && | ||
| !previousThreadKeys.has(threadKey) && | ||
| thread.latestTurn?.state === "completed" |
There was a problem hiding this comment.
If a background thread first appears while still running, this condition records its key in the seen set but does not mark it unread; when that same thread later transitions to completed, previousThreadKeys.has(threadKey) is already true, so it never becomes explicitly unread. Because never-visited threads also fail hasUnseenCompletion without a lastVisitedAt, these completed background jobs look read immediately; track the last seen turn/completion state instead of only first presence.
Useful? React with 👍 / 👎.
963ce6f to
187f0d3
Compare
1dfffa0 to
6ae6cb6
Compare
94de4b2 to
7aaf828
Compare
|
@coderabbitai review |
|
@codex review |
|
@claude review once |
✅ Action performedReview finished.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts (1)
60-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStale environment entries in the observation ref are never pruned.
nextObservedThreadsByEnvironmentclones every environment ever seen frompreviouslyObservedThreadsByEnvironment, including environments that have been permanently removed fromenvironmentCatalog(not merely disconnected). Since this ref lives for the lifetime of the mounted hook (effectively the whole session), long sessions with many ephemeral/short-lived environments will accumulate unbounded map entries that are never reclaimed.Consider dropping entries whose
environmentIdis no longer present inenvironmentCatalogat all (as opposed to just missing a snapshot), while still preserving entries for environments that are merely snapshot-less (disconnected).♻️ Sketch of a fix
export function resolveFirstSeenCompletedThreads(input: { readonly threads: ReadonlyArray<FirstSeenThreadInput>; readonly environmentSnapshotIds: ReadonlyArray<EnvironmentId>; readonly previouslyObservedThreadsByEnvironment: ReadonlyMap< EnvironmentId, ReadonlyMap<string, ObservedThreadTurn> >; readonly activeThreadKey?: string | null; + readonly knownEnvironmentIds?: ReadonlyArray<EnvironmentId>; }): { ... const nextObservedThreadsByEnvironment = new Map( - [...input.previouslyObservedThreadsByEnvironment].map(([environmentId, threads]) => [ - environmentId, - new Map(threads), - ]), + [...input.previouslyObservedThreadsByEnvironment] + .filter( + ([environmentId]) => + input.knownEnvironmentIds === undefined || + input.knownEnvironmentIds.includes(environmentId), + ) + .map(([environmentId, threads]) => [environmentId, new Map(threads)]), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts` around lines 60 - 66, Filter the entries copied into nextObservedThreadsByEnvironment using the current environmentCatalog, removing only environment IDs no longer present there. Preserve entries for catalog environments that are absent from snapshotEnvironmentIds, since those represent disconnected or snapshot-less environments.apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts (1)
27-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the environment disconnect/reconnect scenario.
The suite covers archiving (threads temporarily absent) but not an environment that temporarily drops out of
environmentSnapshotIds(e.g. transient disconnect) and later reappears — the exact scenario a prior review flagged as a critical bug. Since the fix relies on cloningpreviouslyObservedThreadsByEnvironmentregardless of currentenvironmentSnapshotIds, a dedicated test would guard against a regression of that specific fix.it("preserves observed threads while an environment loses its snapshot and does not re-flag them on reconnect", () => { const threadKey = scopedThreadKey(scopeThreadRef(localEnvironmentId, ThreadId.make("stable"))); const previous = new Map([ [localEnvironmentId, new Map([[threadKey, { turnId: "turn-stable", state: "completed" }]])], ]); const whileDisconnected = resolveFirstSeenCompletedThreads({ threads: [], environmentSnapshotIds: [], previouslyObservedThreadsByEnvironment: previous, }); const afterReconnect = resolveFirstSeenCompletedThreads({ threads: [thread("stable")], environmentSnapshotIds: [localEnvironmentId], previouslyObservedThreadsByEnvironment: whileDisconnected.nextObservedThreadsByEnvironment, }); expect(afterReconnect.newlyUnreadThreads).toEqual([]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts` around lines 27 - 177, Add a regression test in the resolveFirstSeenCompletedThreads suite covering an environment omitted from environmentSnapshotIds and later reconnected. Start with a previously observed completed thread, process a disconnected snapshot with no environments, then process the thread after reconnection and assert newlyUnreadThreads remains empty, confirming observations are preserved across disconnects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.ts`:
- Around line 27-177: Add a regression test in the
resolveFirstSeenCompletedThreads suite covering an environment omitted from
environmentSnapshotIds and later reconnected. Start with a previously observed
completed thread, process a disconnected snapshot with no environments, then
process the thread after reconnection and assert newlyUnreadThreads remains
empty, confirming observations are preserved across disconnects.
In `@apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts`:
- Around line 60-66: Filter the entries copied into
nextObservedThreadsByEnvironment using the current environmentCatalog, removing
only environment IDs no longer present there. Preserve entries for catalog
environments that are absent from snapshotEnvironmentIds, since those represent
disconnected or snapshot-less environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b009e8ee-4ea9-408c-852a-bd57eb88db9d
📥 Commits
Reviewing files that changed from the base of the PR and between 963ce6fe3187464508c190392fba2c14d064b7fd and 7aaf828ec7776b7259ea1db180dc44a1de282b5c.
📒 Files selected for processing (19)
apps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/SidebarV2.tsxapps/web/src/components/ThreadStatusIndicators.tsxapps/web/src/components/sidebar/SidebarFilterMenu.tsxapps/web/src/components/sidebar/sidebarProviderFilters.test.tsapps/web/src/components/sidebar/sidebarProviderFilters.tsapps/web/src/components/ui/menu.tsxapps/web/src/environmentGrouping.test.tsapps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.test.tsapps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.tsapps/web/src/routes/__root.tsxapps/web/src/sidebarProjectGrouping.tsapps/web/src/uiStateStore.test.tsapps/web/src/uiStateStore.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/web/src/routes/__root.tsx
- packages/contracts/src/settings.test.ts
- apps/web/src/components/ThreadStatusIndicators.tsx
- apps/web/src/components/ChatView.tsx
- apps/web/src/sidebarProjectGrouping.ts
- apps/web/src/uiStateStore.test.ts
- apps/web/src/components/ui/menu.tsx
- apps/web/src/components/Sidebar.logic.test.ts
- apps/web/src/uiStateStore.ts
- apps/web/src/components/Sidebar.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aaf828ec7
ℹ️ 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".
| ...project, | ||
| id: project.projectKey, | ||
| })); | ||
| const sortableThreads = filteredSidebarThreads.map((thread) => { |
There was a problem hiding this comment.
When includeArchived is enabled, filteredSidebarThreads contains archived snapshots, and this line feeds them into sortProjectsForSidebar, which does not exclude archivedAt rows. As a result, a project with only an old archived thread can be sorted ahead of an active project by updated_at/created_at, making archived history change the main project order whenever users turn on the Archived filter; use only non-archived threads for the project activity sort while still rendering archived rows inside the project.
Useful? React with 👍 / 👎.
| const removedProjectKeys = new Set( | ||
| members.map((member) => scopedProjectKey(scopeProjectRef(member.environmentId, member.id))), | ||
| ); |
There was a problem hiding this comment.
When a grouped project has duplicate/stale physical records, the filter menu selects every projectGroup.memberProjectRef, but this cleanup derives removedProjectKeys only from the de-duplicated visible members. In that case removing the selected group can leave hidden project keys in sidebarThreadFilters.projectKeys, so the sidebar remains scoped to a project the user just removed until they manually reset filters; clear the corresponding memberProjectRefs as well when removing a whole group (and any refs represented by the removed member).
Useful? React with 👍 / 👎.
| const previewThreads = | ||
| isThreadListExpanded || !hasOverflowingThreads | ||
| showAllThreads || !hasOverflowingThreads | ||
| ? visibleProjectThreads | ||
| : visibleProjectThreads.slice(0, sidebarThreadPreviewCount); |
There was a problem hiding this comment.
With includeArchived enabled in grouped mode, visibleProjectThreads now contains archived rows as well as active rows, so this preview slice counts archived rows against sidebarThreadPreviewCount. If several recently archived rows sort to the top of a project, active unarchived threads can be pushed behind “Show more” even though the archived rows are muted and non-navigable; keep archived rows out of the preview budget or append them after choosing the active preview rows.
Useful? React with 👍 / 👎.
7aaf828 to
72b4ad3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72b4ad320d
ℹ️ 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".
| onContextMenu={handleProjectButtonContextMenu} | ||
| > | ||
| {!projectExpanded && projectStatus ? ( | ||
| {!flatMode ? ( |
There was a problem hiding this comment.
When users turn off Group by project, the sidebar renders the single project row with flatMode, and this guard skips the entire project header that contains the SquarePen new-thread button. The flat sidebar header only has search/filter/sort/add-project controls, so especially with an empty thread list there is no direct sidebar affordance to start a thread; render a top-level new-thread control or keep the header action available in flat mode.
Useful? React with 👍 / 👎.
| onCheckedChange={(nextChecked) => { | ||
| onFiltersChange({ | ||
| ...filters, | ||
| projectKeys: toggleSidebarFilterValues( |
There was a problem hiding this comment.
When a project is selected through this new classic-sidebar filter, removing that project through the project context menu leaves its persisted environmentId:projectId in sidebarThreadFilters.projectKeys; after the delete succeeds, filteredSidebarThreads rejects every remaining row until the user manually resets filters. Clear matching projectKeys in the remove-project success path so deleting the currently filtered project does not leave the sidebar stuck on an impossible scope.
Useful? React with 👍 / 👎.
| ); | ||
| for (const thread of archivedThreads) { |
There was a problem hiding this comment.
Once archived snapshots are merged into the V2 visible list, users can enable Archived, see archived rows under a project, and then remove that project from the project settings dialog. That removal path still builds its count and force decision from threads only, so an archived-only project is confirmed as empty and sends project.delete without force; the server still rejects because archived threads are not deleted. Use the archived-inclusive thread set for the removal count/force decision as well.
Useful? React with 👍 / 👎.
5167fe7 to
1e98d34
Compare
| const previousEnvironmentThreads = input.previouslyObservedThreadsByEnvironment.get( | ||
| thread.environmentId, | ||
| ); | ||
| const previousThread = | ||
| previousEnvironmentThreads?.get(threadKey) ?? | ||
| input.seededObservedThreadsByEnvironment?.get(thread.environmentId)?.get(threadKey); |
There was a problem hiding this comment.
🟡 Medium hooks/useMarkFirstSeenCompletedThreadsUnread.ts:98
resolveFirstSeenCompletedThreads lets the stale in-memory observation win over the archived snapshot seed when determining previousThread. When a thread was last seen as running, then archived and completed while archived, and the archived snapshot seed is loaded before unarchive, previousThread still resolves to the stale running entry from previouslyObservedThreadsByEnvironment instead of the completed seed. On re-entry the completion transition check passes and the archived history is incorrectly marked unread. The archived seed should take precedence over stale retained observations, so swap the fallback order so the seed is checked first.
| const previousEnvironmentThreads = input.previouslyObservedThreadsByEnvironment.get( | |
| thread.environmentId, | |
| ); | |
| const previousThread = | |
| previousEnvironmentThreads?.get(threadKey) ?? | |
| input.seededObservedThreadsByEnvironment?.get(thread.environmentId)?.get(threadKey); | |
| const previousThread = | |
| input.seededObservedThreadsByEnvironment?.get(thread.environmentId)?.get(threadKey) ?? | |
| previousEnvironmentThreads?.get(threadKey); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/hooks/useMarkFirstSeenCompletedThreadsUnread.ts around lines 98-103:
`resolveFirstSeenCompletedThreads` lets the stale in-memory observation win over the archived snapshot seed when determining `previousThread`. When a thread was last seen as `running`, then archived and completed while archived, and the archived snapshot seed is loaded before unarchive, `previousThread` still resolves to the stale `running` entry from `previouslyObservedThreadsByEnvironment` instead of the completed seed. On re-entry the completion transition check passes and the archived history is incorrectly marked unread. The archived seed should take precedence over stale retained observations, so swap the fallback order so the seed is checked first.
| ), | ||
| ); | ||
| return sortThreads( | ||
| input.threads.filter( |
There was a problem hiding this comment.
🟡 Medium components/Sidebar.logic.ts:499
getFlatSidebarRenderedThreads filters out every archived thread with thread.archivedAt === null, so when includeArchived is enabled in flat mode the archived threads that matchesSidebarThreadFilters lets through are silently dropped and never render — they cannot be viewed or unarchived from the flat sidebar. Consider removing the thread.archivedAt === null filter so archived threads are shown when the caller has already opted into them.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/Sidebar.logic.ts around line 499:
`getFlatSidebarRenderedThreads` filters out every archived thread with `thread.archivedAt === null`, so when `includeArchived` is enabled in flat mode the archived threads that `matchesSidebarThreadFilters` lets through are silently dropped and never render — they cannot be viewed or unarchived from the flat sidebar. Consider removing the `thread.archivedAt === null` filter so archived threads are shown when the caller has already opted into them.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1e98d34. Configure here.
| selectedEnvironmentIds: sidebarThreadFilters.environmentIds, | ||
| includeArchived: sidebarThreadFilters.includeArchived, | ||
| }), | ||
| [environments, sidebarThreadFilters.environmentIds, sidebarThreadFilters.includeArchived], |
There was a problem hiding this comment.
Classic removal skips archived threads
Medium Severity
The classic sidebar's archive loading doesn't fetch archived threads unless the includeArchived filter is active. This undercounts threads for project removal, making projects with only archived threads appear empty and bypass non-empty warnings.
Reviewed by Cursor Bugbot for commit 1e98d34. Configure here.
|
Note 🤖 GPT-5.6 Sol responding on behalf of Theo Closing this PR after an automated pass over open pull requests. Abandoned sidebar rewrite proposes filtering and flat grouping that already exist. |
3 participants
What Changed
Why
Sidebar V2 introduced useful project filtering and a flat thread list, but those controls should live in the shared filter model so either sidebar can use the same persisted behavior.
Stack
This is intentionally stacked on #4330 and has been restacked onto its exact current head. After the unread and filter PRs land, GitHub reduces this PR to the six grouping-only commits.
UI Changes
Users can switch between project groups and one flat chat list without changing sidebar implementations. Project filters remain available in either mode, and flat mode retains an obvious New thread action.
Verification
git diff --checkpassed.Checklist
Note
Add project filter and flat thread grouping to the sidebar
SidebarFilterMenucomponent that lets users filter sidebar threads by status, project, environment, provider source, and archived state, with multi-select checkboxes and a reset action.markThreadUnreadnow sets a flag inthreadExplicitlyUnreadByIdinstead of backdating last-visited timestamps; the sidebar and status pill render an 'Unread' pill and badge for flagged threads.markActiveThreadVisitedto the UI state store and mountsActiveThreadRouteTracker/CompletedThreadUnreadTrackerat the root to clear explicit unread flags when a thread is revisited and to mark first-seen completed threads unread.sidebarThreadFiltersinClientSettingswith a validated schema defaulting to all statuses, grouped-by-project, and no archive inclusion.markThreadUnreadbehavior changes — it no longer adjuststhreadLastVisitedAt, so any code relying on the old timestamp backdating to detect unread state will no longer work.Macroscope summarized 1e98d34.