fix(server): persist automatic thread settlement - #5402
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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 851d0f2. Configure here.
There was a problem hiding this comment.
Effect service conventions: the new ThreadSettlementReactor service is introduced with a standalone ...Shape interface, split across Services/ and Layers/, and non-canonical make/layer export names. Inline comments describe the expected shape (canonical single module, inline interface, make/layer, Foo["Service"]), which is the pattern used elsewhere in this repo (for example apps/server/src/workspace/WorkspacePaths.ts, apps/server/src/serverSettings.ts). The settlement policy module, its tests, and the mechanical reactor/harness wiring look fine.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Effect service conventions review: the new ThreadSettlementReactor service is introduced in the older split Services/ + Layers/ shape, rather than as one canonical module with an inline service interface and make/layer exports. Two inline comments below; the rest of the diff (pure threadSettlement.ts helpers, contracts/settings changes, client consumers, test/harness Layer.succeed seams) looks consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
|
Effect service conventions — findings (inline comments could not be attached, posting here): 1. The new service is split across a 2. This is the single implementation of the service and the module is named for it, so the canonical export names are plain export const layer = Layer.effect(ThreadSettlementReactor, make);
No other convention issues found: dependencies are acquired via Posted via Macroscope — Effect Service Conventions |
ApprovabilityVerdict: Needs human review This PR introduces a new server-side reactor that actively manages thread settlement, moving logic from client-side computation to server-persisted state. The architectural change introduces new background processing (periodic PR lookups, settlement dispatch) affecting all clients and warrants human review. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
6fb0ffa to
c2d1810
Compare

Thread settlement was partly inferred in each client from local clocks and row-level PR state. That let web and mobile disagree, and pinned threads could appear settled for inactivity or a closed PR.
This moves automatic settlement into a server reactor that persists the existing thread settlement event. Pinned threads now ignore inactivity and closed PRs; a merged PR is their only automatic settlement path. Clients render the projected settled timestamp directly, and the inactivity setting is server-owned.
Tested with 203 focused tests plus targeted lint and typechecks across server, contracts, client runtime, web, mobile, and desktop.
Made with GPT-5.6 Sol in T3 Code via the Codex harness.
Note
High Risk
Changes core thread lifecycle and cross-client consistency; server reactor plus PR provider lookups can mis-settle or delay settlement if reconciliation or caching behaves badly.
Overview
Automatic thread settlement moves to the server so web, mobile, and desktop all read the same
settledAtprojection instead of inferring settled state from local clocks, inactivity rules, and per-row PR subscriptions.A new
ThreadSettlementReactorruns on a timer (and on server settings changes), evaluatesresolveAutomaticSettlementReason, and dispatchesthread.settlefor inactivity or merged PRs. PR lookups are cached and rate-limited; failed lookups do not settle (fail closed). Closed PRs no longer auto-settle; pinned threads skip the inactivity timer and only auto-settle on merge.Clients drop
effectiveSettled,onChangeRequestState, anduseNowMinutefor settlement; list UIs treat a thread as settled whenthread.settledAt !== null. The inactivity threshold moves fromsidebarAutoSettleAfterDays(client) tothreadAutoSettleAfterDays(server settings), edited in beta settings viausePrimarySettings.Reviewed by Cursor Bugbot for commit c2d1810. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Persist automatic thread settlement on the server instead of computing it client-side
ThreadSettlementReactoron the server, which periodically evaluates threads and dispatchesthread.settlecommands based on inactivity or merged PR state, replacing all client-side auto-settlement logic.threadAutoSettleAfterDaysfrom client settings (sidebarAutoSettleAfterDays) to server settings, with a default of 3 and a nullable option to disable; clients no longer compute or store this value.thread.settledAtis non-null (server-projected), removing all client-side PR state tracking and inactivity calculations.effectiveSettled,threadLastActivityAt, andChangeRequestStateLikefrompackages/client-runtime/src/state/threadSettled.ts; any remaining callers will fail to build.Macroscope summarized c2d1810.