Repository navigation
v2.6.2 — sync recovery UX, callout fix, protocol versioning - #113
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 49 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 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request hardens sync protocol compatibility and restore handling, enables and stabilizes editor callout widgets, centralizes agent approval scope and wording, and adds configurable model output limits. It updates desktop and mobile surfaces, chat providers, tests, caches, and release documentation. ChangesSync lifecycle hardening
Editor callout widgets
Agent approval scope
Configurable model output limits
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 5
🤖 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 `@changelogs/v2.6.2.md`:
- Line 4: The release notes overstate detection of outdated peers relative to
the protocol-version contract. Update the entries at changelogs/v2.6.2.md:4 and
mobile/changelogs/v0.1.6.md:3 to state that the “device needs updating” hint
applies to peers with a lower protocol version, without changing the protocol
version itself.
In `@mobile/app/sync.tsx`:
- Line 145: Update the stale-peer warning row in the relevant sync rendering
component to use styles.conflictRowActive instead of styles.restoreRowActive,
preserving styles.conflictRow and the existing warning behavior.
- Around line 47-52: Update the manual sync flow around requestSync("manual") to
refresh stalePeers after the request settles, reusing the existing
stalePeerCount refresh and error-handling behavior. Ensure the refresh runs for
both successful and failed sync completion without relying on the screen focus
refresh in refresh.
In `@src/components/settings/SyncActivityPanel.tsx`:
- Around line 289-295: Update the empty-state logic in SyncActivityPanel by
calculating behindPeers before the loaded-state check, then use
behindPeers.length instead of stalePeers.length when deciding whether to render
the empty state. Preserve non-stale protocol differences without showing an
empty container, while retaining existing behavior for actual stale peers.
In `@src/lib/livePreview.component.test.tsx`:
- Around line 112-126: Extend the test case around mount and the callout
regression to exercise physical coordinate mapping with browser-capable view
coordinates: target the rendered “target line after callout” below the
.cm-lp-callout using posAtCoords or a mouse click, then assert the resolved or
selected document position matches that line’s position. Keep the existing
lineBlockAt assertion, but ensure the new check validates rendered widget height
rather than only document-position mapping.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3298084-b33c-47a4-a78c-3bccab104411
📒 Files selected for processing (23)
changelogs/v2.6.2.mddocs/plans/sync-lifecycle-hardening.mdelectron/preload.tselectron/sync/desktop-sync.tselectron/sync/sync-handlers.tsmobile/app/restore.tsxmobile/app/sync.tsxmobile/changelogs/v0.1.6.mdmobile/src/db/queries.tsshared/sync/engine.test.tsshared/sync/engine.tsshared/sync/oplog-guard.tsshared/sync/transport.test.tssrc/components/agent/AgentMessageBubble.tsxsrc/components/agent/PlanApprovalCard.component.test.tsxsrc/components/settings/SyncActivityPanel.tsxsrc/components/shared/ConnectorToolCard.component.test.tsxsrc/lib/callout-widget.tsxsrc/lib/livePreview.component.test.tsxsrc/lib/livePreview.tssrc/lib/sync-client.tssrc/lib/tool-risk.test.tssrc/lib/tool-risk.ts
The chat, coding-agent, and subagent loops each hardcoded a max_tokens cap (4096/8192). A reasoning model counts its chain-of-thought against that budget, so on a hard prompt it could spend the whole cap thinking and stop with finish_reason:"length" and empty content. That empty assistant turn, replayed on the next message, made the provider reject the request: "content or tool_calls must be set" (400). Verified against the live opencode.ai/zen endpoint: with no cap the model finishes naturally (finish_reason:"stop", full reasoning + answer) using far fewer tokens than any cap — a cap only ever truncates the tail case. - resolveMaxOutputTokens(userOverride?) -> number | undefined: Auto (default) omits max_tokens entirely; a positive manual value is sent. - Thread maxTokens through chat, pi-agent, and subagent request configs; omit the field when unset. - New 'Max output tokens' control (Auto = no limit + manual override) in AI and Agent settings; StepperSettingsRow gains autoSuppressesValue so a previously-typed number clears when Auto is re-selected. - Parse limit.output into ModelInfo.maxOutput for settings guidance; bump the models.dev cache key (v2) so existing caches refetch immediately. - Belt-and-braces: isSendableMessage / assistantTurnIsSendable drop any assistant turn with neither content nor tool_calls (desktop chat + dispatch history, mobile mapMessage) so a stalled empty turn can never poison the next request. Streaming branch surfaces captured reasoning if a reply is ever cut off.
…state - changelogs: describe the 'device needs updating' hint as firing on peers reporting a LOWER protocol version (matching listPeerProtocols' behind flag), not the local device stamping a newer version. - mobile sync: stale-peer row uses conflictRowActive (warning) instead of restoreRowActive (danger), matching its warning icon/intent; refresh peer + restorable counts after a manual sync settles (success or failure) rather than relying only on the focus refresh. - SyncActivityPanel: compute behindPeers before the empty-state check and gate the empty state on behindPeers.length, so a non-stale protocol difference no longer suppresses the empty state while leaving an empty shell.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@electron/ipc/pi-agent.ts`:
- Line 219: Persist the selected maxTokens value in both agent cache calls:
update electron/ipc/pi-agent.ts lines 200-207 and 294-301 to pass
req.config?.maxTokens to cacheLlmConnection("agent", ...), covering the fallback
reads at lines 219 and 313.
In `@shared/models/model-catalog.ts`:
- Line 280: Update maxOutput handling in the model-catalog mapping and the
limit.output path around the referenced validation logic to accept only positive
numbers, converting zero, negative, and invalid values to null. Reuse a shared
positive-number validator in both paths, and add tests covering zero and
negative inputs.
- Around line 455-457: Update resolveMaxOutputTokens to require userOverride >=
1 before flooring, ensuring fractional values such as 0.1 and 0.999 return
undefined rather than zero; add tests covering both cases while preserving
existing handling of valid positive caps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ed930b71-819c-408a-a8de-b19010a48bbc
📒 Files selected for processing (28)
changelogs/v2.6.2.mdelectron/ipc/chat.tselectron/ipc/pi-agent.tselectron/lib/chat-loop.tselectron/lib/chat-subagent-loop.tselectron/lib/config-cache.tselectron/lib/llm-sendable.test.tselectron/lib/llm.tselectron/lib/pi-agent-loop.tselectron/lib/tools.tsmobile/app/sync.tsxmobile/changelogs/v0.1.6.mdmobile/src/chat/models-dev.tsmobile/src/chat/providers/openai.tsmobile/src/chat/providers/types.test.tsmobile/src/chat/providers/types.tsshared/models/model-catalog.test.tsshared/models/model-catalog.tssrc/components/agent/AgentChatPane.tsxsrc/components/chat/chat-panel/index.tsxsrc/components/settings/AISettings.tsxsrc/components/settings/AgentSettings.tsxsrc/components/settings/SyncActivityPanel.tsxsrc/components/settings/shared.tsxsrc/hooks/useChatStream.tssrc/lib/constants.tssrc/lib/models-dev.tssrc/store/slices/ui.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mobile/app/sync.tsx
- src/components/settings/SyncActivityPanel.tsx
…rsing
- pi-agent: pass req.config.maxTokens into both cacheLlmConnection('agent') calls
so the fallback reads at the cached-config merge actually recover a user's cap
on a later turn that omits it (previously always undefined).
- model-catalog: add a shared positiveTokenLimit() validator; use it for
maxOutput in both the parse and normalize paths so limit.output of 0 (which
~180 real models.dev entries have), negative, or non-finite becomes null
instead of a broken 0 ceiling on the settings stepper.
- resolveMaxOutputTokens: require userOverride >= 1 before flooring, so a
fractional cap in (0,1) returns undefined instead of Math.floor'ing to 0 and
sending max_tokens:0.
- tests for zero/negative limit.output, positiveTokenLimit, and fractional
overrides.
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
The v2.6.2 cycle — a consolidation release that hardens what 2.6.1 shipped and fixes issues found by on-device testing. Five commits, each independently reviewable; two are user-facing fixes surfaced by testing the sync work on real devices, one re-enables a shelved editor feature, one is defensive sync infrastructure, and one is a test-only verification pass.
1. Restore / conflict-copy overlap (
14ff77c)Found on-device. Delete a note on device A, edit it on B before B sees the delete → the delete-won path keeps B's edit as a conflict copy and tombstones the original with its body intact. Both hold the same content, so B was offered a Restore button next to the conflict — and restoring produced two identical notes.
Fix: a new engine predicate
hasLivePreservingCopysuppresses the restore offer when a live conflict copy already preserves that content, with a typedpreserved-as-copyreason pointing the user to conflict resolution instead. Wired into the singlerestorability()check so the list and the action can't disagree. A plain peer delete with no competing edit still offers Restore.2. Inline callout widget re-enabled (
07aec36)Callouts (
> [!note]) render inline in the editor's live preview again. They were switched off (CALLOUTS_ENABLED = false) because clicking near or below a callout dropped the cursor several lines off — text after a callout was unselectable. Root cause:createRoot().render()paints async, so CodeMirror measured the widget at ~0px and mis-mapped every line below. The prior fix chased it withrequestMeasureafter paint, which never fully recovered. This mounts React synchronously withflushSyncintoDOM(), so the widget has real height the first time CM measures — correct by construction, not by timing.3. Sync protocol versioning + stale-peer detection (
faed1c5)The oplog wire format had no version field, so a device inferred a peer's capability from which optional fields happened to be present. Adds
OplogEntry.v(stamped at the export boundary; missing = pre-2.6.2 = protocol 1), records the highest version seen per peer insync_state, and surfaces a "device needs updating" hint (desktop Settings → Sync + mobile Sync screen) when a peer is behind — because a behind peer may not honour deletes. Skew contract documented indocs/plans/sync-lifecycle-hardening.md§6.4. Agent approval / plan / connector test suite (
2eeacee)The verification pass the roadmap flagged as missing for the v2.5.22 approval UX. 16 tests across
PlanApprovalCard,ConnectorToolCard, andtool-risk. To test the approval card's grant-scoping without faking Electron, its inline decision logic was extracted into pureapprovalGrantScope()/approvalScopeLabel()helpers and the card rewired to use them — so the tested logic is the shipped logic.5. Platform-neutral approval copy (
128b9d6)The extracted
approvalScopeLabeldescribed where a tool reaches as "this Mac" — but Cairn ships on Windows and Linux. Changed to "this device" (the codebase's existing neutral term), with a guard test asserting no scope label matches\bMac\b.Type of change
Screenshots / recording
Checklist
npm run type-check:allpasses — plusnpm --prefix mobile run type-checknpm run lintpasses (mobile lint: 2 pre-existing warnings intoken-breakdown.test.ts, untouched)npm testpasses — 1,815 passed / 12 pre-existing skipsnpm run test:e2epasses — 17/17text-[Npx]pixel font classeshandle()and returnIpcResult<T>—sync:peerProtocolsOplogEntry.vis a wire-format field stamped at export, not a stored column; peer versions live in the existingsync_statetableDatabaseconstruction; sync SQL stays inside the shared engine--externalflagsNotes for reviewer
Focus:
shared/sync/engine.ts. Two additions —hasLivePreservingCopy(restore suppression) and the protocol-version tracking. Both are additive to reconciliation, not changes to the delete-wins rules that 2.6.1 established.Two things are unit-green but not fully provable in CI, called out honestly on their cards:
Also in this branch: created the two missing Obsidian-import roadmap cards (re-import conflict handling
EM6_XaQAlTrH, rollback/undogADsLmLlctVP) — both were 🔴 Missing in roadmap §4 with no board representation. Card-creation only; not implemented here.Both platforms:
changelogs/v2.6.2.mdandmobile/changelogs/v0.1.6.mdupdated; both sit one patch ahead of their versions (2.6.1 / 0.1.5) so both release gates pass.Summary by CodeRabbit