test(desktop): restore coverage baseline without lowering thresholds - #1488
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds Vitest coverage for desktop authentication, keyboard shortcuts, search state, file and evidence adapters, and settings synchronization fallbacks. ChangesDesktop test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 (3)
app/desktop/src/utils/keyboardShortcuts.test.ts (1)
15-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject an unexpected modifier.
The test verifies a missing required modifier. It does not verify that
Ctrl+Shift+Kfails for['Ctrl', 'K']. An implementation that accepts extra modifiers would pass this suite.Proposed test
expect(matchesBinding(event({ ctrlKey: true, key: 'k' }), ['Ctrl', 'K'])).toBe(true); + expect(matchesBinding(event({ ctrlKey: true, shiftKey: true, key: 'k' }), ['Ctrl', 'K'])).toBe(false); expect(matchesBinding(event({ ctrlKey: true, key: 'k' }), ['Ctrl', 'Shift', 'K'])).toBe(false);🤖 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 `@app/desktop/src/utils/keyboardShortcuts.test.ts` around lines 15 - 22, Extend the matchesBinding test in the “matches exact modifiers and the cross-platform Ctrl/Command token” case to assert that an event with both Ctrl and Shift does not match ['Ctrl', 'K']. Keep the existing required-modifier, cross-platform token, undefined binding, and key-name assertions unchanged.app/desktop/src/hooks/useGlobalKeyboardShortcuts.test.tsx (1)
11-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover every editable target type.
The hook filters
input,textarea,select, and contenteditable elements. This test only coversinput. Add cases for the other target types so a regression in their filtering cannot pass.🤖 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 `@app/desktop/src/hooks/useGlobalKeyboardShortcuts.test.tsx` around lines 11 - 26, Add coverage to the useGlobalKeyboardShortcuts test for textarea, select, and contenteditable targets, verifying Ctrl+K does not open search when dispatched from each editable element. Preserve the existing input case and outside-editable document case, and clean up every created target after each scenario.app/desktop/src/hooks/useAuth.test.tsx (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the subscription callback before claiming reactive coverage.
The test checks the initial snapshot and confirms that
subscribewas called. It never invokes the listener with a new snapshot. A regression inuseSyncExternalStoreupdate propagation could still pass.Capture the listener, return a real cleanup function, replace
getStatewith a new state object, notify the listener insideact, and assert thatresult.currentupdates.Also applies to: 28-44
🤖 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 `@app/desktop/src/hooks/useAuth.test.tsx` at line 15, Update the subscription test around useAuth to capture the listener passed to subscribe and return a functional cleanup callback. Replace getState with a new state object, invoke the captured listener inside act, and assert that result.current reflects the updated state in addition to the existing initial snapshot and subscription assertions.
🤖 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 `@app/desktop/src/platform/desktopAdapters.test.ts`:
- Line 88: Update the tests around openDesktopEvidencePreview and the
localStorage assertions to verify public adapter behavior instead of
implementation details: remove the hard-coded error-message and storage-key
checks, mock the external remote reads to fail, and confirm readSettings()
returns the cached values after each write.
---
Nitpick comments:
In `@app/desktop/src/hooks/useAuth.test.tsx`:
- Line 15: Update the subscription test around useAuth to capture the listener
passed to subscribe and return a functional cleanup callback. Replace getState
with a new state object, invoke the captured listener inside act, and assert
that result.current reflects the updated state in addition to the existing
initial snapshot and subscription assertions.
In `@app/desktop/src/hooks/useGlobalKeyboardShortcuts.test.tsx`:
- Around line 11-26: Add coverage to the useGlobalKeyboardShortcuts test for
textarea, select, and contenteditable targets, verifying Ctrl+K does not open
search when dispatched from each editable element. Preserve the existing input
case and outside-editable document case, and clean up every created target after
each scenario.
In `@app/desktop/src/utils/keyboardShortcuts.test.ts`:
- Around line 15-22: Extend the matchesBinding test in the “matches exact
modifiers and the cross-platform Ctrl/Command token” case to assert that an
event with both Ctrl and Shift does not match ['Ctrl', 'K']. Keep the existing
required-modifier, cross-platform token, undefined binding, and key-name
assertions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02317de1-749d-4eff-856f-b7f35c7ecba0
📒 Files selected for processing (5)
app/desktop/src/hooks/useAuth.test.tsxapp/desktop/src/hooks/useGlobalKeyboardShortcuts.test.tsxapp/desktop/src/platform/desktopAdapters.test.tsapp/desktop/src/stores/searchStore.test.tsapp/desktop/src/utils/keyboardShortcuts.test.ts
| expect(mockOpenShell).toHaveBeenNthCalledWith(1, 'file:///tmp/readme.md'); | ||
| expect(mockOpenShell).toHaveBeenNthCalledWith(2, './README.md'); | ||
| expect(mockOpenShell).toHaveBeenNthCalledWith(3, 'https://example.test/a'); | ||
| await expect(openDesktopEvidencePreview(invalidEvidence)).rejects.toThrow('No preview target'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove assertions of implementation strings.
Line 88 hard-asserts an error message. Lines 95, 123, and 131 hard-assert localStorage key strings. Assert the public adapter behavior instead. For example, force remote reads to fail and verify that readSettings() returns the cached values after each write.
As per coding guidelines, **/*.{test,spec}.{ts,tsx,js,jsx}: “禁止无保护力测试：不要复制实现 switch、测试常量字符串、硬断错误文案，或 mock 被测函数本身；mock 应模拟外部系统。”
Also applies to: 95-95, 123-123, 131-131
🤖 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 `@app/desktop/src/platform/desktopAdapters.test.ts` at line 88, Update the
tests around openDesktopEvidencePreview and the localStorage assertions to
verify public adapter behavior instead of implementation details: remove the
hard-coded error-message and storage-key checks, mock the external remote reads
to fail, and confirm readSettings() returns the cached values after each write.
Source: Coding guidelines
6309188 to
df17ecd
Compare
…wave) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests.
…wave) (#1492) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests. Co-authored-by: Codex (Delicious233) <codex@vectorcontrol.tech>
…1488) * test(desktop): restore coverage baseline * test(desktop): cover keyboard and search state --------- Co-authored-by: Codex <codex@vectorcontrol.tech>
…wave) (#1492) - Hub integration fixtures: merged in #1489 - Edge tests on Linux: merged in #1491 (incl. ACP request-sequence race fix) - TokenDance ID defaults: merged in #1480 (+ check-secrets.sh *_URL exemption) - Frontend coverage gate: merged #1490 (web 63.14→66.72) / #1488 (desktop) - Edge lint debt: merged in #1491 (102→0, zero exclusions, .gitattributes eol) Remaining: CI policy parser (#1481), Doc SSOT compaction, Desktop typecheck, shared frontend tests. Co-authored-by: Codex <codex@vectorcontrol.tech>
Summary
Evidence
This PR keeps the historical baseline unchanged and restores the desktop floor with behavior tests. Web coverage recovery remains a separate lane.
Summary by CodeRabbit