Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 7 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Description check | |
The description explains the bug, the fix, and the verification in detail, but it does not follow the repository template. It has no ELI5 section, no separate "What Changed" or "Why" sections, and no … | Add an ELI5 section in plain language. Split the content into "What Changed" (before and after user experience, plus the mechanism changed) and "Why" (the problem and why this approach was chosen over alternatives). Add a "Linked Issue" sec… |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the main change: keeping unread sleeping workspaces visible.
Full details: Description check
-
Explanation
The description explains the bug, the fix, and the verification in detail, but it does not follow the repository template. It has no ELI5 section, no separate "What Changed" or "Why" sections, and no "Linked Issue" section with a "Fixes #" reference. The Visual Proof section has before and after screenshots, but they are not presented as the required tabbed comparison. The Testing checklist and the AI Disclosure and Agent skill upstream boundary sections are not addressed. The description also has a very long verification log that is hard to scan.
Resolution
Add an ELI5 section in plain language. Split the content into "What Changed" (before and after user experience, plus the mechanism changed) and "Why" (the problem and why this approach was chosen over alternatives). Add a "Linked Issue" section with a real issue reference (for example, "Fixes
#NNNN"). Arrange the BEFORE and AFTER screenshots so they can be switched easily, or use a short video. Mark the Testing checkboxes that apply and list the platforms tested (the description says macOS-only Electron testing was done; state this explicitly). Complete the AI Disclosure section if an AI tool was used, or confirm the author is a StablyAI team member. Complete the Agent skill upstream boundary checkbox. Condense the verification log and state the Notes items (security, cross-platform, SSH, mobile, backwards compatibility, performance) that were checked or not checked. Add the Checklist items that apply.
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/WorktreeJumpPalette.tsx (1)
454-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated hide-sleeping-unless-unread predicate.
This condition mirrors the one just added in
computeVisibleWorktreeIds(src/renderer/src/components/sidebar/visible-worktrees.tsLines 167-180) but is reimplemented inline rather than shared. The two are logically equivalent today, but nothing enforces they stay in sync, and this copy has no direct test coverage (only the sidebar'scomputeVisibleWorktreeIdsgained new tests in this PR).Consider extracting a small shared helper, e.g.
isHiddenBySleepFilter(worktree, opts), used by both call sites, so the behavior has one source of truth and one test target.♻️ Proposed extraction
+// visible-worktrees.ts (or a shared filters module) +export function isHiddenBySleepFilter( + worktree: Pick<Worktree, 'id' | 'isUnread'>, + showSleepingWorkspaces: boolean, + tabsByWorktree: Record<string, Pick<TerminalTab, 'id'>[]> | null, + ptyIdsByTabId: Record<string, string[]> | null, + browserTabsByWorktree?: Record<string, { id: string }[]> | null +): boolean { + if (showSleepingWorkspaces || worktree.isUnread) return false + return isInactiveWorkspace(worktree.id, tabsByWorktree, ptyIdsByTabId, browserTabsByWorktree) +}- if ( - !showSleepingWorkspaces && - !worktree.isUnread && - isInactiveWorkspace(worktree.id, tabsByWorktree, ptyIdsByTabId, browserTabsByWorktree) - ) { + if ( + isHiddenBySleepFilter(worktree, showSleepingWorkspaces, tabsByWorktree, ptyIdsByTabId, browserTabsByWorktree) + ) { return false }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b30e075b-5047-4c0c-a72d-13dacfb87ba1
📒 Files selected for processing (3)
src/renderer/src/components/WorktreeJumpPalette.tsxsrc/renderer/src/components/sidebar/visible-worktrees.test.tssrc/renderer/src/components/sidebar/visible-worktrees.ts
The "Hide sleeping" sidebar filter hid any workspace with no live PTY or browser tab, judging activity purely by live sessions. A slept agent that finished and posted a notification (worktree.isUnread) therefore stayed hidden, so the notification's source was unreachable without toggling the filter off, clicking through, and toggling it back on. Keep an unread workspace visible while sleeping is hidden, in both the sidebar (computeVisibleWorktreeIds) and the worktree jump palette's empty-query list, so an agent completion notification always surfaces the workspace it came from.
Address CodeRabbit review: the hide-sleeping-unless-unread condition was reimplemented inline in both computeVisibleWorktreeIds and the WorktreeJumpPalette empty-query filter, with nothing keeping the two in sync and no direct test coverage of the palette copy. Extract it into one exported helper, isHiddenBySleepFilter, in worktree-activity-state.ts (beside isInactiveWorkspace) and use it from both call sites, so the behavior has a single source of truth and a single test target. Add unit tests covering the full truth table (filter off, unread override, active via PTY, active via browser, inactive+read hidden).
|
Rebased onto current Conflicts resolved in |
2751523 to
be246ff
Compare
# Conflicts: # src/renderer/src/lib/worktree-activity-state.test.ts
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| !isSleepingSweepExemptWorkspace(worktree, alwaysShowDefaultBranchWorkspace) && | ||
| isInactiveWorkspace( | ||
| worktree.id, | ||
| isHiddenBySleepFilter( |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added consumer-level coverage in 4116b90a0a59f767e63144bb86146ef74d59637b through the mounted WorktreeJumpPalette, rather than another predicate-only test. With Hide sleeping enabled, it checks the final local/SSH workspace rows and visit order, keeps archived/detached/CLI-created and current rows excluded, then calls the real clearWorktreeUnread action and verifies the sleeping row disappears while the active workspace stays unchanged.
The regression fails both when the palette drops the unread override and when its visibility memo stops refreshing after the unread state changes. Production behavior is unchanged; the existing DOM harness mocks visual primitives and metadata persistence, not the filtering/order/store transition. Parent reran 89 tests across five files, full pnpm tc, normal hooks and the unchanged original-head quality gate; all pass.
Adapt the original unread visibility fix to the current shared workspace row filter used by desktop, mobile, and the shared sidebar feed. Preserve explicit filters and keep reveal behavior aligned with the sleeping sweep. Co-authored-by: gatsby74 <166927047+gatsby74@users.noreply.github.com> Co-authored-by: mmarabel <166927047+mmarabel@users.noreply.github.com>
nwparker
changed the title
|
Thanks for this, @mmarabel, and sorry it took us so long to reach a decision. We're going to close it. When Hide sleeping is on, users expect sleeping workspaces to be hidden, and keeping unread ones visible would make that filter no longer mean what it says. We'd rather keep the filter predictable. Unread-specific visibility is better explored as its own option (see #24190 for a related "Show unread only" request) than as an exception inside Hide sleeping. We appreciate the careful work and test coverage here. |
With Hide sleeping enabled, an inactive workspace marked unread disappeared from both the sidebar and empty-query Search. This keeps it reachable until it is marked read, using the current shared row filter for desktop, mobile and the shared sidebar feed. Explicit archive, host, project, device and workspace-kind filters still apply.
Reproduced twice on untouched current main using real linked Git worktrees and the actual Mark as unread control. The fixed hidden Electron app retains the unread workspace, hides the read control, removes the target after Mark as read, and restores both when Hide sleeping is disabled. No terminal was opened. This confirms manually marked unread; agent-completion notification delivery was not exercised.
Before, Hide sleeping loses the unread workspace:
After, the unread workspace remains visible:
After, empty-query Search also retains it:
Fresh post-review owner validation at
ef39047e6701e999b1e76be83f801502e6c0e92d(tree74b2fef2741725d0f4a1ae349b6e2285a544402d): 121 desktop/shared tests across 11 suites and 50 mobile tests across two suites pass, including mounted Search, read transitions, same-ID workspaces on separate hosts, filter precedence, and desktop/mobile parity. Full renderer and Node typechecks, focused pure-mobile-filter typecheck, seven changed-code quality gates, scoped anti-slop, formatting and direct build pass. Local full mobile-project typecheck was unavailable without Expo/React Native dependencies; final-head CI Mobile verify subsequently passed installed-dependency import lint, full mobile and test typechecks, tests, lint and formatting. Native mobile, Windows/Linux, live SSH/paired-runtime and browser-server UI were not exercised.Adapted from the original gatsby74/mmarabel change onto current main; original head
4116b90ais an actual parent and contributor history/credit is preserved. No changes to activity classification, notification handling, persistence or wire schemas.Independent behavior review at
c27fd2ff: LOW risk, no blockers, with 75 repository and 50 mobile tests plus 51 adversarial actual-evaluator cases covering unread/read, explicit kind/host/project/archive/device filters, primary preference and folder parity. The finalef39047ecorrection received a separate LOW, no blockers review with 22 feed/shared tests and object/value/branch equivalence confirmed. Screenshots were inspected. After the final review, the owner freshly built and repeated the complete unread/read/filter/Search flow in an isolated hidden app; screenshots were personally inspected, the native window remained hidden/unfocused, and owned apps were stopped.Actual CI merge
886079a2(treeb095a591) has parents mainffd3d335and reviewed headef39047e. None of the 40 paths changed on main since the reproduced base overlap this fix; all 11 reviewed changed file blobs and all 115.githubfiles match the reviewed head and trusted base respectively.The initial CI run found an introduced 301-line module-limit failure. A new commit removes a redundant alias and uses the identical existing empty lineage-map object directly, bringing the module to 300 counted lines. The old SHA was not retried. All 11 changed files pass full-file lint. Final-head CI: all six workflows succeeded. The paginated audit reconciles 41 checks with 41 workflow job IDs: 24 passed, 17 intentional skips, zero pending or failed, all at the published head. This includes both packages, all five Node 24 unit shards, relay integration, static lint/typecheck, installed-dependency mobile verification, mobile web bundle, Headless Node persistence, all three WSL distros, test-selection evidence and the aggregate gate. Five fork runs report API attempt 2 because attempt 1 required approval; there were no final-head execution failures or retries.