fix: harden desktop lifecycle boundaries by scotej · Pull Request #208 · scotej/studyvis

scotej · 2026-08-10T04:58:33Z

What changed and why

Hardens the desktop boundary and lifecycle paths identified in review: custom Tauri commands are now explicitly window-scoped; model downloads, sidecars, engine replacement, and friend backups are safer under failure/cancellation; invite and break flows serialize correctly; and Settings/AI-dialog behavior stays visually and thematically consistent. The PR also adds a 1.11.1 changelog section for the release workflow without bumping version files early.

Manual test

npm run tauri dev launched and the changed surface behaves as described — n-a: this Linux container has no desktop host
Checked in both themes and at reduced-motion — n-a: no desktop host

Compatibility surfaces

Peer wire format (trystero payloads, session/pomodoro/AI-alert messages) — no payload schema changed; retry policy now waits for the existing verified recipient acknowledgement.

Gates

npm run build && npm run lint && npm run test — build and lint passed via Bun; the full Vitest worker runner is unavailable in this container.
npm run check-tokens && npm run check-strings && npm run check-contrast
npm run check-migrations && npm run check-stories
npm run build-storybook && npm run check-a11y — unavailable in this container.
cd src-tauri && cargo fmt --check && cargo clippy && cargo test — CI is the first Rust compiler for this box.
cd src-tauri && cargo deny check — CI is the first Rust toolchain for this box.

CI-only gates remain pending.

Merge style

Squash

Summary by CodeRabbit

New Features
- Added automatic theme synchronization with system preferences, including the floating AI dialog.
- Added safer model download, installation, removal, and recovery handling.
Bug Fixes
- Improved floating AI window behavior and prevented stale session actions.
- Fixed invite retry and cancellation behavior.
- Improved break-request handling during rapid session changes.
- Added protection against oversized friends backup files and invalid model downloads.
Security
- Restricted native commands available to the floating AI dialog.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

coderabbitai · 2026-08-10T04:59:03Z

Warning

Review limit reached

@scotej, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 57 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b14221e1-a179-4c84-a71c-238a1c241e32

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef6f55 and 66a1098.

📒 Files selected for processing (1)

src-tauri/src/commands/friends.rs

📝 Walkthrough

Walkthrough

The PR adds window-scoped Tauri command permissions, transactional engine installation, coordinated model and sidecar operations, bounded backup handling, theme boot synchronization, and cancellation-aware invite and break workflows with tests.

Changes

Application safety and lifecycle

Layer / File(s)	Summary
Tauri command ACL generation `.gitignore`, `ARCHITECTURE.md`, `CHANGELOG.md`, `src-tauri/build.rs`, `src-tauri/capabilities/*`, `src-tauri/permissions/window-commands.toml`, `src-tauri/src/commands/applog.rs`	The build generates explicit command permissions. The main window receives the full allowlist. The AI dialog receives only `app_log_append` and `sidecar_status`.
Transactional engine promotion `src-tauri/src/commands/engine.rs`	Engine replacement uses rollback directories and recovers interrupted promotions. Tests cover replacement, rollback, and recovery.
Managed model and sidecar coordination `src-tauri/src/commands/models.rs`, `src-tauri/src/commands/sidecar.rs`, `src-tauri/src/lib.rs`	Model URLs and paths are validated. Per-model gates coordinate downloads, removals, engine access, and sidecar lifecycle operations.
Bounded friends backup handling `src-tauri/src/commands/friends.rs`, `src-tauri/Cargo.toml`	Backup export and import enforce a 16 MiB limit and reject invalid file types and oversized files.
Theme boot synchronization `src/components/ui/dialog.tsx`, `src/design/*`, `src/features/ai/ai-dialog-main.tsx`, `tests/unit/theme.test.ts`	Theme resolution is shared across windows. The AI dialog applies the localStorage boot cache before rendering. Dialog z-index values use `--z-modal`.
Invite delivery acknowledgement and cancellation `src/features/friends/*`, `tests/integration/invite.test.ts`, `tests/unit/inviteRetry.test.ts`	Invite delivery requires verified acknowledgements. Abort signals cancel active sends and retries. Unacknowledged deliveries remain retryable.
Serialized break requests and audit cancellation `src/features/session/SessionView.tsx`, `src/features/session/break.ts`, `tests/unit/break-rules.test.ts`	Break requests use FIFO serialization, abort-aware audit operations, tokenized timers, and session-bound cleanup.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ModelDownload
  participant ModelOperationState
  participant Sidecar
  participant Filesystem
  ModelDownload->>ModelOperationState: acquire model operation gate
  ModelDownload->>Sidecar: stop serving managed model
  ModelDownload->>Filesystem: update model files
  Sidecar->>ModelOperationState: coordinate startup and engine access

sequenceDiagram
  participant SessionView
  participant BreakQueue
  participant AuditPipeline
  participant BreakOrchestrator
  SessionView->>BreakQueue: submit break request with AbortSignal
  BreakQueue->>AuditPipeline: append request audit
  AuditPipeline-->>BreakQueue: complete or abort
  BreakQueue->>BreakOrchestrator: process session state
  BreakOrchestrator-->>SessionView: return verdict or AbortError

Possibly related PRs

scotej/studyvis#45: Both changes harden model-download behavior in src-tauri/src/commands/models.rs.
scotej/studyvis#75: Both changes extend model removal and sidecar coordination.
scotej/studyvis#88: Both changes modify engine installation and sidecar lifecycle code.

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.26% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the PR’s primary focus on hardening desktop lifecycle boundaries.
Description check	✅ Passed	The description covers the required changes, testing, compatibility impact, gates, and merge style.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch agent/address-review-findings

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (4)

tests/integration/invite.test.ts (1)
505-514: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The join assertion depends on the exact microtask count before joinTopic.

A single await Promise.resolve() is enough today, because sendInviteEnvelope queues prev.then(...) before the test yields, and joinTopic runs synchronously inside that callback. If a later change adds one await before joinTopic in sendInviteEnvelopeNow, this assertion reads 0 joins and fails for a reason unrelated to abort behavior.

Consider vi.waitFor so the test waits for the join instead of assuming the hop count.
♻️ Proposed robustness change
-    await Promise.resolve()
     const trystero = (await import('`@/lib/trystero`')) as unknown as {
       __getEvents: () => Array<{ type: 'join' | 'leave'; topic: string }>
     }
-    expect(
-      trystero.__getEvents().filter((e) => e.type === 'join')
-    ).toHaveLength(1)
+    await vi.waitFor(() => {
+      expect(
+        trystero.__getEvents().filter((e) => e.type === 'join')
+      ).toHaveLength(1)
+    })
     controller.abort()
🤖 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 `@tests/integration/invite.test.ts` around lines 505 - 514, Update the join
assertion in the invite cancellation test to use vi.waitFor around the
trystero.__getEvents join-count check, waiting until one join is observed
instead of relying on a single Promise.resolve microtask yield. Preserve the
existing expectation that exactly one join occurs.
src/features/friends/invite.ts (1)
177-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move shared abort helpers to one module.

src/features/friends/invite.ts and src/features/session/break.ts both define createAbortError, throwIfAborted, and withAbort locally, and only invite.ts has waitForAbortableDelay. Centralize these helpers and import them from both files to keep abort semantics consistent.
🤖 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 `@src/features/friends/invite.ts` around lines 177 - 212, Centralize
createAbortError, throwIfAborted, withAbort, and waitForAbortableDelay in a
shared abort-helper module, then remove their local definitions from invite.ts
and the corresponding duplicated helpers from break.ts. Import and reuse the
shared symbols in both feature files, preserving the existing abort behavior and
delay handling.
src-tauri/src/commands/models.rs (1)
1087-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding a short-path case to the rejection table.

The rejection list covers scheme, host, port, path kind, userinfo, and query. It does not cover a URL with fewer than five path segments. That case exercises the segments.len() >= 5 rule, which is the only guard against a resolve URL without a filename.
♻️ Proposed additional cases
             "https://token@huggingface.co/ggml-org/model/resolve/rev/model.gguf",
             "https://huggingface.co/ggml-org/model/resolve/rev/model.gguf?download=true",
+            "https://huggingface.co/ggml-org/model/resolve/rev",
+            "https://huggingface.co/ggml-org/model/resolve/rev/",
         ] {
🤖 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 `@src-tauri/src/commands/models.rs` around lines 1087 - 1099, Extend the
rejection table in
trusted_model_url_rejects_non_hugging_face_or_non_resolve_urls with a valid
HTTPS Hugging Face resolve URL whose path has fewer than five segments, such as
one lacking the filename. Keep the assertion against trusted_hugging_face_url
unchanged so the segments.len() >= 5 guard is explicitly covered.
tests/unit/break-rules.test.ts (1)
473-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the aborted-signal branch inside the timer callback.

This test drives the token mismatch path. The stale callback returns at the token check in src/features/session/break.ts Line 378 and never reaches the deps.signal?.aborted check at Line 380.

That second guard is reachable in production. SessionView aborts the controller in the AI-dialog effect cleanup at Line 1487, while cancelActiveBreakTimer runs only in the separate reset effect cleanup at Line 1094. The two effects have different dependency arrays, so a signal can abort while the timer record is still armed and still owns its token. Without a test, a future change to the guard order would pass silently.
💚 Proposed additional test
+  test('an aborted session timer does not end the break when its record is still armed', async () => {
+    const controller = new AbortController()
+    const input = {
+      requestedDurationSec: 300,
+      aiRecommendation: 'approve' as const,
+      aiReasoning: '',
+      now: 1_700_000_000_000,
+    }
+    const deps = buildDeps(state())
+    await requestBreak(input, { ...deps, signal: controller.signal })
+    const timer = scheduledTimers[0]!
+
+    // Abort only. The timer record keeps its own token, so the callback
+    // reaches the signal guard rather than the token guard.
+    controller.abort()
+    timer.handler()
+
+    expect(endBreakCalls).toHaveLength(0)
+  })
As per path instructions: "Use Vitest for unit and integration tests".
🤖 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 `@tests/unit/break-rules.test.ts` around lines 473 - 499, Add a unit test
covering the timer callback’s aborted-signal guard when the callback still owns
the active timer token: abort the controller without canceling the timer, invoke
its handler, and assert the break does not end while the timer record remains
consistent. Keep the existing stale-token coverage unchanged and use the current
break-rule test helpers and Vitest assertions.
Source: Path instructions

🤖 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 `@src-tauri/src/commands/friends.rs`:
- Around line 109-116: Update read_backup_bounded_with_limit to reject FIFO and
other non-regular paths before any potentially blocking read open. Use a
race-safe Unix-appropriate non-blocking/open-and-validate flow, while preserving
the existing "{} is not a regular file" error and bounded-read behavior for
regular files.

In `@src-tauri/src/commands/sidecar.rs`:
- Around line 153-165: Update kill_blocking’s try_lock spin to use a bounded
deadline, such as an Instant several seconds in the future, and stop retrying
once it expires. Preserve the existing lock acquisition and cleanup behavior
while ensuring the blocking exit handler can return instead of waiting
indefinitely.

In `@src/features/session/break.ts`:
- Around line 337-383: Move the break-end timer setup, including clearing any
existing activeBreakTimer and assigning the new timer token/handle, to
immediately after startApprovedBreak() and before the emitAudit()/withAbort()
broadcast. Preserve the callback’s token, abort, and endBreak checks, while
leaving the broadcast as best-effort and retaining its existing error handling.

In `@tests/integration/invite.test.ts`:
- Around line 150-158: Reset the invite retry manager’s delivered cache during
integration-test teardown, not only its pending entries. Update the cleanup
around inviteRetryManager.cancelAll() to invoke the manager’s
reset/clear-delivery mechanism, ensuring SAMPLE_SESSION recipient registrations
do not persist across tests.

---

Nitpick comments:
In `@src-tauri/src/commands/models.rs`:
- Around line 1087-1099: Extend the rejection table in
trusted_model_url_rejects_non_hugging_face_or_non_resolve_urls with a valid
HTTPS Hugging Face resolve URL whose path has fewer than five segments, such as
one lacking the filename. Keep the assertion against trusted_hugging_face_url
unchanged so the segments.len() >= 5 guard is explicitly covered.

In `@src/features/friends/invite.ts`:
- Around line 177-212: Centralize createAbortError, throwIfAborted, withAbort,
and waitForAbortableDelay in a shared abort-helper module, then remove their
local definitions from invite.ts and the corresponding duplicated helpers from
break.ts. Import and reuse the shared symbols in both feature files, preserving
the existing abort behavior and delay handling.

In `@tests/integration/invite.test.ts`:
- Around line 505-514: Update the join assertion in the invite cancellation test
to use vi.waitFor around the trystero.__getEvents join-count check, waiting
until one join is observed instead of relying on a single Promise.resolve
microtask yield. Preserve the existing expectation that exactly one join occurs.

In `@tests/unit/break-rules.test.ts`:
- Around line 473-499: Add a unit test covering the timer callback’s
aborted-signal guard when the callback still owns the active timer token: abort
the controller without canceling the timer, invoke its handler, and assert the
break does not end while the timer record remains consistent. Keep the existing
stale-token coverage unchanged and use the current break-rule test helpers and
Vitest assertions.

🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e02cde1-751f-4f0c-8c34-342292e5d19b

📥 Commits

Reviewing files that changed from the base of the PR and between caa902d and 4b86858.

📒 Files selected for processing (27)

.gitignore
ARCHITECTURE.md
CHANGELOG.md
src-tauri/build.rs
src-tauri/capabilities/ai-dialog.json
src-tauri/capabilities/default.json
src-tauri/permissions/window-commands.toml
src-tauri/src/commands/applog.rs
src-tauri/src/commands/engine.rs
src-tauri/src/commands/friends.rs
src-tauri/src/commands/models.rs
src-tauri/src/commands/sidecar.rs
src-tauri/src/lib.rs
src/components/ui/dialog.tsx
src/design/theme-resolution.ts
src/design/theme.tsx
src/features/ai/ai-dialog-main.tsx
src/features/friends/invite.ts
src/features/friends/inviteRetry.ts
src/features/friends/pendingInvitesStore.ts
src/features/session/SessionView.tsx
src/features/session/break.ts
tests/integration/invite.test.ts
tests/unit/ai-sidecar.test.ts
tests/unit/break-rules.test.ts
tests/unit/inviteRetry.test.ts
tests/unit/theme.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

GitHub Check: Installer (macOS)
GitHub Check: Installer (Windows)
GitHub Check: Frontend

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{ts,tsx,rs}