Skip to content

fix: Address todo list advanced settings feedback #7809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: Address todo list advanced settings feedback #7809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions src/core/config/ProviderSettingsManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export const providerProfilesSchema = z.object({
diffSettingsMigrated: z.boolean().optional(),
openAiHeadersMigrated: z.boolean().optional(),
consecutiveMistakeLimitMigrated: z.boolean().optional(),
todoListEnabledMigrated: z.boolean().optional(),
})
.optional(),
})
Expand All @@ -56,7 +55,6 @@ export class ProviderSettingsManager {
diffSettingsMigrated: true, // Mark as migrated on fresh installs
openAiHeadersMigrated: true, // Mark as migrated on fresh installs
consecutiveMistakeLimitMigrated: true, // Mark as migrated on fresh installs
todoListEnabledMigrated: true, // Mark as migrated on fresh installs
},
}

Expand Down Expand Up @@ -123,7 +121,6 @@ export class ProviderSettingsManager {
diffSettingsMigrated: false,
openAiHeadersMigrated: false,
consecutiveMistakeLimitMigrated: false,
todoListEnabledMigrated: false,
} // Initialize with default values
isDirty = true
}
Expand Down Expand Up @@ -152,12 +149,6 @@ export class ProviderSettingsManager {
isDirty = true
}

if (!providerProfiles.migrations.todoListEnabledMigrated) {
await this.migrateTodoListEnabled(providerProfiles)
providerProfiles.migrations.todoListEnabledMigrated = true
isDirty = true
}

if (isDirty) {
await this.store(providerProfiles)
}
Expand Down Expand Up @@ -263,18 +254,6 @@ export class ProviderSettingsManager {
}
}

private async migrateTodoListEnabled(providerProfiles: ProviderProfiles) {
try {
for (const [_name, apiConfig] of Object.entries(providerProfiles.apiConfigs)) {
if (apiConfig.todoListEnabled === undefined) {
apiConfig.todoListEnabled = true
}
}
} catch (error) {
console.error(`[MigrateTodoListEnabled] Failed to migrate todo list enabled setting:`, error)
}
}

/**
* Clean model ID by removing prefix before "/"
*/
Expand Down
43 changes: 0 additions & 43 deletions src/core/config/__tests__/ProviderSettingsManager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ describe("ProviderSettingsManager", () => {
diffSettingsMigrated: true,
openAiHeadersMigrated: true,
consecutiveMistakeLimitMigrated: true,
todoListEnabledMigrated: true,
},
}),
)
Expand Down Expand Up @@ -187,48 +186,6 @@ describe("ProviderSettingsManager", () => {
expect(storedConfig.migrations.consecutiveMistakeLimitMigrated).toEqual(true)
})

it("should call migrateTodoListEnabled if it has not done so already", async () => {
mockSecrets.get.mockResolvedValue(
JSON.stringify({
currentApiConfigName: "default",
apiConfigs: {
default: {
config: {},
id: "default",
todoListEnabled: undefined,
},
test: {
apiProvider: "anthropic",
todoListEnabled: undefined,
},
existing: {
apiProvider: "anthropic",
// this should not really be possible, unless someone has loaded a hand edited config,
// but we don't overwrite so we'll check that
todoListEnabled: false,
},
},
migrations: {
rateLimitSecondsMigrated: true,
diffSettingsMigrated: true,
openAiHeadersMigrated: true,
consecutiveMistakeLimitMigrated: true,
todoListEnabledMigrated: false,
},
}),
)

await providerSettingsManager.initialize()

// Get the last call to store, which should contain the migrated config
const calls = mockSecrets.store.mock.calls
const storedConfig = JSON.parse(calls[calls.length - 1][1])
expect(storedConfig.apiConfigs.default.todoListEnabled).toEqual(true)
expect(storedConfig.apiConfigs.test.todoListEnabled).toEqual(true)
expect(storedConfig.apiConfigs.existing.todoListEnabled).toEqual(false)
expect(storedConfig.migrations.todoListEnabledMigrated).toEqual(true)
})

it("should throw error if secrets storage fails", async () => {
mockSecrets.get.mockRejectedValue(new Error("Storage failed"))

Expand Down
7 changes: 4 additions & 3 deletions webview-ui/src/components/settings/ApiOptions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,9 +751,10 @@ const ApiOptions = ({

{!fromWelcomeView && (
<Collapsible open={isAdvancedSettingsOpen} onOpenChange={setIsAdvancedSettingsOpen}>
<CollapsibleTrigger className="flex items-center gap-1 w-full cursor-pointer hover:opacity-80 mb-2">
<span className={`codicon codicon-chevron-${isAdvancedSettingsOpen ? "down" : "right"}`}></span>
<span className="font-medium">{t("settings:advancedSettings.title")}</span>
<CollapsibleTrigger className="flex items-center gap-2 w-full cursor-pointer hover:opacity-80 mb-2">
<span
className={`codicon codicon-chevron-${isAdvancedSettingsOpen ? "down" : "right"} flex-shrink-0`}></span>
Copy link
Contributor

@roomote roomote bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix for the text wrapping issue! The combination of flex-shrink-0 on the chevron and whitespace-nowrap on the text should prevent the Advanced Settings label from breaking onto multiple lines. The gap-2 also provides better visual spacing than gap-1.

<span className="font-medium whitespace-nowrap">{t("settings:advancedSettings.title")}</span>
</CollapsibleTrigger>
<CollapsibleContent className="space-y-3">
<TodoListSettingsControl
Expand Down
Loading