fix(settings): respect completed pull request auto-settlement preference - #2
Merged
Conversation
Reviewer's GuideAdds a user-controllable “auto-settle completed pull requests” preference across shared contracts, web/desktop settings, and mobile settings, wires it into the thread settlement logic, and updates mobile/web UI, persistence, search, restore, and docs so completed-PR settlement respects this toggle while maintaining existing inactivity and manual settlement semantics. Sequence diagram for thread settlement with completed-PR auto-settle togglesequenceDiagram
participant Sidebar
participant ClientSettings
participant ThreadShell
participant effectiveSettled
Sidebar->>ClientSettings: get sidebarAutoSettleAfterDays
Sidebar->>ClientSettings: get sidebarAutoSettleCompletedChangeRequests
Sidebar->>ThreadShell: read changeRequestState
Sidebar->>effectiveSettled: effectiveSettled(shell,{now,autoSettleAfterDays,autoSettleCompletedChangeRequests,changeRequestState})
alt autoSettleCompletedChangeRequests && changeRequestState in [merged, closed]
effectiveSettled-->>Sidebar: true (auto-settle by completed PR)
else changeRequestState == open
effectiveSettled-->>Sidebar: false (open PR blocks inactivity settle)
else settledOverride == "settled"
effectiveSettled-->>Sidebar: true (explicit settle wins)
else settledOverride == "active"
effectiveSettled-->>Sidebar: false (explicit keep-active wins)
else inactivity window passed
effectiveSettled-->>Sidebar: true (auto-settle by inactivity)
else
effectiveSettled-->>Sidebar: false (keep active)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Defmon3
marked this pull request as ready for review
August 13, 2026 17:58
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The web helper
shouldShowCompletedPullRequestAutoSettleSettingand the mobile helpershouldShowAutoSettleCompletedChangeRequestsSettingimplement similar gating logic but with slightly different criteria and naming; consider standardizing the naming and semantics (or documenting the differences) to make it clearer why the behaviors diverge. - On mobile, both
SettingsRouteScreen.GeneralSettingsSectionanduseAutoSettleCompletedChangeRequestsderivepreferencesLoadedindependently frommobilePreferencesAtom; you could centralize this into the hook (returning both value and loaded state) to avoid duplicated logic and keep the hydration behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- The web helper `shouldShowCompletedPullRequestAutoSettleSetting` and the mobile helper `shouldShowAutoSettleCompletedChangeRequestsSetting` implement similar gating logic but with slightly different criteria and naming; consider standardizing the naming and semantics (or documenting the differences) to make it clearer why the behaviors diverge.
- On mobile, both `SettingsRouteScreen.GeneralSettingsSection` and `useAutoSettleCompletedChangeRequests` derive `preferencesLoaded` independently from `mobilePreferencesAtom`; you could centralize this into the hook (returning both value and loaded state) to avoid duplicated logic and keep the hydration behavior consistent.
## Individual Comments### Comment 1
<locationpath="apps/web/src/components/settings/SettingsPanels.tsx"line_range="1867-1872" />
<code_context>
<SettingsRow {...searchableSetting("auto-settle-inactive-threads")}- description="Sidebar threads with no activity for this long settle automatically. Threads on merged or closed PRs always settle."
</code_context>
<issue_to_address>
**suggestion:** The auto-settle descriptions for inactivity and completed pull requests may conflict or confuse users.
The new wording for `auto-settle-inactive-threads` (“Independent of merged or closed pull requests.”) seems inconsistent with the completed-PR toggle, which explicitly settles threads on merge/close. This could suggest that inactivity never interacts with PR state, even though open PRs still block inactivity settlement and closed/merged PRs can now settle via the new toggle.
Consider rephrasing both descriptions to clarify that:
- inactivity-based auto-settle depends on the time window and is blocked while the PR is open; and
- completed-PR auto-settle is a separate control that can settle threads immediately on merge/close, or otherwise leave them to follow the normal inactivity rules.
This should make the interaction between the two settings clearer to users.
Suggested implementation:
```typescript<SettingsRow
{...searchableSetting("auto-settle-inactive-threads")}
description="Sidebar threads with no activity for this long settle automatically, as long as their pull request is not open or settled immediately by the completed pull request setting."resetAction={
settings.sidebarAutoSettleAfterDays !==
DEFAULT_UNIFIED_SETTINGS.sidebarAutoSettleAfterDays ? (
```You’ll also want to update the description text for the completed pull request auto-settle setting (likely another `SettingsRow` using something like `searchableSetting("auto-settle-completed-pull-requests")` or similar) to explain its relationship with inactivity-based settling. For example:- Old (hypothetical) description:`description="Sidebar threads settle automatically when their pull request is merged or closed."`- New description:`description="Sidebar threads on completed pull requests settle immediately when the pull request is merged or closed. When this is off, threads on completed pull requests follow the normal inactivity-based auto-settle rules."`Please adjust the exact `SettingsRow` and `searchableSetting` key names to match the actual code where the completed-PR toggle is defined.</issue_to_address>### Comment 2<location path="apps/web/src/components/settings/settingsSearch.ts" line_range="211" /><code_context> export type SettingsSearchItemId = (typeof SETTINGS_SEARCH_ITEMS)[number]["id"];+export function shouldShowCompletedPullRequestAutoSettleSetting(input: {+ readonly legacySidebarEnabled: boolean;+}): boolean {</code_context><issue_to_address>**issue (complexity):** Consider simplifying the new settings helpers by inlining the legacy sidebar check and/or encoding visibility directly in SETTINGS_SEARCH_ITEMS instead of using a separate single-purpose wrapper function.You can simplify the new helpers while keeping the new behavior (the extra setting + legacy sidebar hiding) and avoiding extra indirection.### 1. Inline the trivial boolean and remove `shouldShowCompletedPullRequestAutoSettleSetting`The helper is just a negation; it doesn’t buy much. You can make `getSettingsSearchItems` the single place that encodes the condition:```ts
export function getSettingsSearchItems(input: {
readonly legacySidebarEnabled: boolean;
}): ReadonlyArray<SettingsSearchItem> {
if (input.legacySidebarEnabled) {
returnSETTINGS_SEARCH_ITEMS.filter(
(item) =>item.id!=="auto-settle-completed-pull-requests",
);
}
return SETTINGS_SEARCH_ITEMS;
}
```Then at call sites that need to know visibility of the specific setting, use the inline boolean:```tsconstshowCompletedPullRequestAutoSettleSetting=!legacySidebarEnabled;
```This keeps behavior but removes a single-use wrapper function.### 2. Keep `SETTINGS_SEARCH_ITEMS` as the single source of truthRight now `getSettingsSearchItems` is effectively a second “list” definition via filtering logic. If you prefer a more data-driven approach, you can encode the visibility on the item itself and make `getSettingsSearchItems` generic:```tsconstSETTINGS_SEARCH_ITEMS= [
{
id: "auto-settle-completed-pull-requests",
title: "Auto-settle completed pull requests",
to: "/settings/general",
hiddenWhenLegacySidebar: true,
},
// …
] asconstsatisfiesReadonlyArray<SettingsSearchItem& {
hiddenWhenLegacySidebar?:boolean;
}>;
exportfunction getSettingsSearchItems(input: {
readonly legacySidebarEnabled:boolean;
}):ReadonlyArray<SettingsSearchItem> {
returnSETTINGS_SEARCH_ITEMS.filter(
(item) =>!input.legacySidebarEnabled||!item.hiddenWhenLegacySidebar,
);
}
```This way:- `SETTINGS_SEARCH_ITEMS` remains the canonical list.- The legacy-specific visibility is encoded in the data, not scattered in helper-specific conditions.- The helper scales if you later add more legacy-dependent items.</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
1867
to
1872
| <SettingsRow | ||
| {...searchableSetting("auto-settle-inactive-threads")} | ||
| description="Sidebar threads with no activity for this long settle automatically. Threads on merged or closed PRs always settle." | ||
| description="Sidebar threads with no activity for this long settle automatically. Independent of merged or closed pull requests." | ||
| resetAction={ | ||
| settings.sidebarAutoSettleAfterDays !== | ||
| DEFAULT_UNIFIED_SETTINGS.sidebarAutoSettleAfterDays ? ( |
There was a problem hiding this comment.
suggestion: The auto-settle descriptions for inactivity and completed pull requests may conflict or confuse users.
The new wording for auto-settle-inactive-threads (“Independent of merged or closed pull requests.”) seems inconsistent with the completed-PR toggle, which explicitly settles threads on merge/close. This could suggest that inactivity never interacts with PR state, even though open PRs still block inactivity settlement and closed/merged PRs can now settle via the new toggle.
Consider rephrasing both descriptions to clarify that:
- inactivity-based auto-settle depends on the time window and is blocked while the PR is open; and
- completed-PR auto-settle is a separate control that can settle threads immediately on merge/close, or otherwise leave them to follow the normal inactivity rules.
This should make the interaction between the two settings clearer to users.
Suggested implementation:
<SettingsRow{...searchableSetting("auto-settle-inactive-threads")}description="Sidebar threads with no activity for this long settle automatically, as long as their pull request is not open or settled immediately by the completed pull request setting."resetAction={
settings.sidebarAutoSettleAfterDays!==DEFAULT_UNIFIED_SETTINGS.sidebarAutoSettleAfterDays ? (You’ll also want to update the description text for the completed pull request auto-settle setting (likely another SettingsRow using something like searchableSetting("auto-settle-completed-pull-requests") or similar) to explain its relationship with inactivity-based settling. For example:
Old (hypothetical) description:
description="Sidebar threads settle automatically when their pull request is merged or closed."New description:
description="Sidebar threads on completed pull requests settle immediately when the pull request is merged or closed. When this is off, threads on completed pull requests follow the normal inactivity-based auto-settle rules."
Please adjust the exact SettingsRow and searchableSetting key names to match the actual code where the completed-PR toggle is defined.
| export type SettingsSearchItemId = (typeof SETTINGS_SEARCH_ITEMS)[number]["id"]; | ||
| export function shouldShowCompletedPullRequestAutoSettleSetting(input: { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new settings helpers by inlining the legacy sidebar check and/or encoding visibility directly in SETTINGS_SEARCH_ITEMS instead of using a separate single-purpose wrapper function.
You can simplify the new helpers while keeping the new behavior (the extra setting + legacy sidebar hiding) and avoiding extra indirection.
1. Inline the trivial boolean and remove shouldShowCompletedPullRequestAutoSettleSetting
The helper is just a negation; it doesn’t buy much. You can make getSettingsSearchItems the single place that encodes the condition:
exportfunctiongetSettingsSearchItems(input: {readonlylegacySidebarEnabled: boolean;}): ReadonlyArray<SettingsSearchItem>{if(input.legacySidebarEnabled){returnSETTINGS_SEARCH_ITEMS.filter((item)=>item.id!=="auto-settle-completed-pull-requests",);}returnSETTINGS_SEARCH_ITEMS;}Then at call sites that need to know visibility of the specific setting, use the inline boolean:
constshowCompletedPullRequestAutoSettleSetting=!legacySidebarEnabled;This keeps behavior but removes a single-use wrapper function.
2. Keep SETTINGS_SEARCH_ITEMS as the single source of truth
Right now getSettingsSearchItems is effectively a second “list” definition via filtering logic. If you prefer a more data-driven approach, you can encode the visibility on the item itself and make getSettingsSearchItems generic:
constSETTINGS_SEARCH_ITEMS=[{id: "auto-settle-completed-pull-requests",title: "Auto-settle completed pull requests",to: "/settings/general",hiddenWhenLegacySidebar: true,},// …]asconstsatisfiesReadonlyArray<SettingsSearchItem&{hiddenWhenLegacySidebar?: boolean;}>;exportfunctiongetSettingsSearchItems(input: {readonlylegacySidebarEnabled: boolean;}): ReadonlyArray<SettingsSearchItem>{returnSETTINGS_SEARCH_ITEMS.filter((item)=>!input.legacySidebarEnabled||!item.hiddenWhenLegacySidebar,);}This way:
SETTINGS_SEARCH_ITEMSremains the canonical list.- The legacy-specific visibility is encoded in the data, not scattered in helper-specific conditions.
- The helper scales if you later add more legacy-dependent items.
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 freeto 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.
Completed pull requests currently settle their associated threads regardless of the user's inactivity preference. This makes the existing auto-settle controls incomplete: users cannot keep completed-PR threads active without manually restoring them.
This change adds a dedicated Auto-settle completed pull requests preference across the shared settings contract, web/desktop settings, and mobile settings. The completed-PR settlement path now respects that preference while explicit settlement and inactivity-based settlement retain their existing behavior. Existing installations decode the new preference as enabled, preserving current behavior until a user turns it off.
The mobile and web consumers share the same settlement decision semantics, preference loading avoids a transient incorrect state, settings reset/search behavior is covered, and the user documentation explains how the two automatic settlement controls interact.
Validation:
@t3tools/contracts,@t3tools/client-runtime,@t3tools/web,@t3tools/desktop, and@t3tools/mobilegit diff --check origin/custom...HEADpassedImplemented and verified with GPT-5.6 Terra coding agents and GPT-5.6 Sol reviewers through the Codex harness in T3 Code.
Summary by Sourcery
Add a dedicated preference for auto-settling threads on completed pull requests and wire it through contracts, clients, and settlement logic so completed-PR auto-settlement can be independently controlled from inactivity-based auto-settlement.
New Features:
Enhancements:
Documentation:
Tests: