Conversation
📝 WalkthroughWalkthroughThis pull request implements optional per-session ChangesTop P Nucleus Sampling Feature
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
Actionable comments posted: 18
🧹 Nitpick comments (2)
test/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts (1)
133-135: ⚡ Quick winRemove duplicate assertion.
Lines 111-120 already verify the complete v23 migration SQL with exact string equality, which confirms the
top_p REALcolumn is included. ThetoContaincheck here is redundant.♻️ Proposed fix
expect(table.getMigrationSQL(29)).toBe('ALTER TABLE deepchat_sessions ADD COLUMN top_p REAL;') - - expect(table.getMigrationSQL(23)).toContain( - 'ALTER TABLE deepchat_sessions ADD COLUMN top_p REAL;' - ) })🤖 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 `@test/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts` around lines 133 - 135, Remove the redundant assertion that checks table.getMigrationSQL(23) contains 'ALTER TABLE deepchat_sessions ADD COLUMN top_p REAL;' — since the v23 migration SQL is already validated with an exact equality assertion earlier (lines 111–120), delete the expect(...toContain(...)) line referencing table.getMigrationSQL(23) to avoid duplication; keep the comprehensive equality check and any surrounding setup/asserts (i.e., the table.getMigrationSQL(23) call used in the earlier exact-match assertion).test/renderer/stores/draft.test.ts (1)
6-9: ⚡ Quick winRemove unnecessary mock that delegates to actual implementation.
The mock delegates directly to the actual Pinia module without modification, making it redundant. Remove the
vi.doMockcall entirely.♻️ Proposed fix
beforeEach(() => { vi.resetModules() - vi.doMock('pinia', async () => { - const actual = await vi.importActual<typeof import('pinia')>('pinia') - return actual - }) })🤖 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 `@test/renderer/stores/draft.test.ts` around lines 6 - 9, The test contains a redundant mock: remove the vi.doMock call that simply re-exports the actual Pinia module. Delete the vi.doMock('pinia', ...) block in test/renderer/stores/draft.test.ts so the real Pinia import is used directly (no replacement of import behavior in the test), ensuring any setup/uses of Pinia in the test continue to import the real module.
🤖 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/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 3628-3635: The current update path deletes next.topP when
normalizeTopP(patch.topP) returns undefined, which silently resets a valid
session value for inputs like 2 or 'abc'; change the logic in the block handling
patch.topP (where normalizeTopP is called) so that if normalizeTopP returns a
finite value you assign next.topP = normalizedTopP, but if it returns undefined
you do nothing (leave next.topP unchanged) instead of deleting it; reference the
normalizeTopP function, the patch object and the next.topP assignment to locate
and update the code.
In `@src/main/presenter/llmProviderPresenter/aiSdk/runtime.ts`:
- Around line 1182-1183: The spread currently emits normalizedModelConfig.topP
whenever it's not undefined, which allows invalid values (0, NaN, >1) to reach
the AI SDK; update the emission to only include topP when it's a finite number
inside the valid probability range (e.g.,
Number.isFinite(normalizedModelConfig.topP) && normalizedModelConfig.topP > 0 &&
normalizedModelConfig.topP <= 1) so the object spread uses a guarded expression
like ...(validTopP ? { topP: normalizedModelConfig.topP } : {}); apply the same
guarded check for all occurrences that reference normalizedModelConfig.topP in
this file (the spreads around the current diff and the other spots you noted).
In `@src/renderer/src/components/chat/ChatStatusBar.vue`:
- Around line 1089-1090: The UI clamps topP using TOP_P_MIN = 0.01 which is
stricter than the validation range (0,1], causing decrement/button logic to jump
small valid values (e.g., 0.005) up to 0.01; change the clamp minimum to 0 (or
to a value matching validation) and keep the button step at 0.01 if desired so
manual inputs <0.01 remain allowed — update all occurrences (TOP_P_MIN/TOP_P_MAX
and any clamp calls referencing them, plus related UI step props) and ensure
topP is still validated as >0 and <=1 in the input/submit validation paths
(e.g., where topP is used/validated and where the decrement/increment handlers
reference TOP_P_MIN).
In `@src/renderer/src/i18n/fa-IR/chat.json`:
- Line 170: Replace the English strings in the Persian locale JSON for the keys
"topPDescription" and the key at line 188 with Persian translations so the fa-IR
file is fully localized; open src/renderer/src/i18n/fa-IR/chat.json, locate the
"topPDescription" entry and the other untranslated entry at line 188, and update
their values to appropriate Persian text while preserving the JSON key names and
punctuation.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 627-630: The fa-IR locale file has English text for the recently
added keys; update the fa-IR entries for "topP.description" and
"validation.topPRange" to Persian translations (and also fix the duplicate at
the other occurrence around the 655 area) so the UI is fully localized; locate
the "topP" object and the "validation" object in
src/renderer/src/i18n/fa-IR/settings.json and replace the English strings with
proper Persian strings for the keys topP.description and validation.topPRange.
In `@src/renderer/src/i18n/he-IL/chat.json`:
- Around line 169-170: Translate the English strings for the topP UI into Hebrew
by replacing the current values for "topPDescription" and "validation.topPRange"
in the he-IL locale file so the advanced settings panel is fully localized;
specifically update the "topPDescription" value (currently "Nucleus sampling;
leave blank to use provider default") and the "validation.topPRange" value to
their appropriate Hebrew translations while keeping the keys "topP",
"topPDescription", and "validation.topPRange" unchanged.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 627-630: Translate the English strings for the newly added keys
into Hebrew: replace topP.description ("Controls nucleus sampling. Leave blank
to use the model or provider default.") with an appropriate Hebrew translation,
and update validation.topPRange (the validation message around allowed Top P
values) to Hebrew as well; there are two occurrences (the topP block and the
validation.topPRange entries referenced around the later lines), so update both
topP.description and validation.topPRange to their Hebrew equivalents while
preserving the keys and punctuation.
In `@src/renderer/src/i18n/id-ID/chat.json`:
- Around line 180-181: The messages for the keys topP, topPDescription, and
validation.topPRange are still in English; update their values to Indonesian so
the advanced settings UI is fully localized: translate the string for "topP"
(label), "topPDescription" (description: "Nucleus sampling; leave blank to use
provider default") and the validation message under validation.topPRange into
natural Indonesian, preserving the existing key names (topP, topPDescription,
validation.topPRange) and quotation/JSON formatting.
In `@src/renderer/src/i18n/id-ID/settings.json`:
- Around line 626-629: The new "topP" entries in
src/renderer/src/i18n/id-ID/settings.json still have English text; update the
"topP.description" value to an Indonesian translation of "Controls nucleus
sampling. Leave blank to use the model or provider default." and also translate
the "topPRange" validation message (the second occurrence around lines 730-731)
into Indonesian so both the label/description and the validation string are
localized consistently; locate the "topP" and "topPRange" keys in the file and
replace their English strings with appropriate Indonesian equivalents.
In `@src/renderer/src/i18n/ms-MY/chat.json`:
- Around line 180-181: The ms-MY locale file still contains English for the
"topP" and "topPDescription" keys (and the other instance around line 199);
replace those English values with proper Malay translations so the UI is fully
localized—update the "topP" value to a short Malay label (e.g., "Top P" -> "Top
P" or a Malay equivalent) and change "topPDescription" to a Malay sentence
conveying "Nucleus sampling; leave blank to use provider default" (e.g.,
"Pensampelan teras; biarkan kosong untuk menggunakan nilai lalai penyedia"), and
do the same for the duplicate key instance.
In `@src/renderer/src/i18n/ms-MY/settings.json`:
- Around line 626-629: The "topP" entry in the ms-MY settings JSON has English
text; replace its label/description with Malay and ensure the duplicate "topP"
occurrence later uses the same translation — specifically update the "topP"
key's "label" to "Top P" (retain the technical name) and the "description" to a
Malay string such as "Mengawal pensampelan teras (nucleus). Biarkan kosong untuk
menggunakan nilai lalai model atau penyedia." and apply the identical changes to
the other "topP" instance so both entries are consistent.
In `@src/renderer/src/i18n/pl-PL/chat.json`:
- Around line 180-181: The Polish locale has untranslated strings for the
advanced-settings keys "topP" and "topPDescription" (and the related validation
message referenced at the second occurrence around "topP" key); update the
values for those JSON keys in pl-PL chat.json to their proper Polish
translations while keeping the keys unchanged and preserving punctuation and
capitalization so the UI and validation messages display fully in Polish.
In `@src/renderer/src/i18n/pl-PL/settings.json`:
- Around line 690-693: The Polish locale currently contains English text for the
new Top P UI strings: translate the entries under the "topP" key (label and
description) into Polish and update any adjacent/new keys introduced near "topP"
(the other strings added around the same area) so they are fully localized;
locate the "topP" object in src/renderer/src/i18n/pl-PL/settings.json and
replace the English values with their Polish equivalents, making sure the keys
stay unchanged.
In `@src/renderer/src/i18n/ru-RU/chat.json`:
- Around line 169-170: The value for the i18n key "topPDescription" currently
mixes English and Russian; replace the mixed-language string with a fully
Russian translation for consistency (update the "topPDescription" entry to a
complete Russian sentence describing nucleus sampling and the default provider
behavior, keeping the "topP" key unchanged).
In `@src/renderer/src/i18n/vi-VN/chat.json`:
- Line 181: The vi-VN locale JSON still contains English help strings for keys
like "topPDescription" (and the other untranslated key around line 199); replace
those English values with proper Vietnamese translations so the UI is fully
localized—locate the keys "topPDescription" and the untranslated key near line
199 in src/renderer/src/i18n/vi-VN/chat.json and update their string values to
Vietnamese equivalents that convey the same meaning as the originals.
In `@src/renderer/src/i18n/vi-VN/settings.json`:
- Around line 690-693: The vi-VN locale has English text for topP.description
and validation.topPRange; locate the "topP" entry (key "topP" with
"label"/"description") and the "validation.topPRange" string in the vi-VN
settings.json and replace the English phrases with Vietnamese equivalents (e.g.,
description -> "Điều khiển nucleus sampling. Để trống để sử dụng mặc định của mô
hình hoặc nhà cung cấp." and validation.topPRange -> a Vietnamese range
validation message like "Vui lòng nhập một giá trị giữa 0 và 1."), and ensure
the same change is applied to the other occurrence mentioned
(validation.topPRange elsewhere in the file).
In `@src/renderer/src/i18n/zh-HK/chat.json`:
- Around line 177-178: The zh-HK locale entries for the sampling parameter
strings are still in English; update the "topP", "topPDescription", and
"topPRange" keys in chat.json to proper Cantonese (zh-HK) localized copy
(replace "Top P", "Nucleus sampling; leave blank to use provider default", and
the English range text with their zh-HK equivalents), ensuring the translated
strings are used for both occurrences mentioned (including the second occurrence
referenced at lines 196); keep the key names unchanged so existing code (e.g.,
lookups for "topP", "topPDescription", "topPRange") continues to work.
In `@src/renderer/src/i18n/zh-HK/settings.json`:
- Around line 627-630: The zh-HK locale still has the new topP strings in
English; update the "topP" entry's "label" and "description" (and the related
range/validation message for the same topP key elsewhere) to Traditional
Chinese, replacing "Top P", "Controls nucleus sampling. Leave blank to use the
model or provider default." and any English range/validation text with
appropriate zh-HK translations so both occurrences of the topP localization are
fully translated.
---
Nitpick comments:
In `@test/main/presenter/sqlitePresenter/deepchatSessionsTable.test.ts`:
- Around line 133-135: Remove the redundant assertion that checks
table.getMigrationSQL(23) contains 'ALTER TABLE deepchat_sessions ADD COLUMN
top_p REAL;' — since the v23 migration SQL is already validated with an exact
equality assertion earlier (lines 111–120), delete the expect(...toContain(...))
line referencing table.getMigrationSQL(23) to avoid duplication; keep the
comprehensive equality check and any surrounding setup/asserts (i.e., the
table.getMigrationSQL(23) call used in the earlier exact-match assertion).
In `@test/renderer/stores/draft.test.ts`:
- Around line 6-9: The test contains a redundant mock: remove the vi.doMock call
that simply re-exports the actual Pinia module. Delete the vi.doMock('pinia',
...) block in test/renderer/stores/draft.test.ts so the real Pinia import is
used directly (no replacement of import behavior in the test), ensuring any
setup/uses of Pinia in the test continue to import the real module.
🪄 Autofix (Beta)
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
Run ID: ac84c48c-bf2d-47ff-9201-4218d37f652e
📒 Files selected for processing (61)
docs/features/model-top-p-settings/plan.mddocs/features/model-top-p-settings/spec.mddocs/features/model-top-p-settings/tasks.mdsrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/configPresenter/modelConfig.tssrc/main/presenter/llmProviderPresenter/aiSdk/runtime.tssrc/main/presenter/sqlitePresenter/schemaCatalog.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/chat.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/chat.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/chat.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/chat.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/chat.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/chat.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/chat.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/chat.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/draft.tssrc/shared/contracts/common.tssrc/shared/contracts/domainSchemas.tssrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/utils/generationSettingsValidation.tssrc/types/i18n.d.tstest/main/presenter/sqlitePresenter/deepchatSessionsTable.test.tstest/main/shared/generationSettingsValidation.test.tstest/renderer/stores/draft.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/renderer/src/components/chat/ChatStatusBar.vue (1)
1115-1117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTop P floor at
0.1now silently coerces/drops lower values.Lines 1116, 2881, and 2915-2919 enforce
topP >= 0.1and auto-clamp on commit. This can unexpectedly rewrite existing or manually entered low values instead of surfacing a validation error, and it narrows controllability for models that support finer/low nucleus sampling.Suggested adjustment
-const TOP_P_STEP = 0.1 -const TOP_P_MIN = 0.1 +const TOP_P_STEP = 0.01 +const TOP_P_MIN = Number.EPSILON @@ function normalizeTopP(value: unknown): number | undefined { const numeric = parseFiniteNumericValue(value) - return numeric !== undefined && numeric >= 0.1 && numeric <= 1 ? numeric : undefined + return numeric !== undefined && numeric > 0 && numeric <= 1 ? numeric : undefined } @@ - const draftNum = parseFiniteNumericValue(numericInputDrafts.value.topP) - if (draftNum !== undefined) { - if (draftNum < TOP_P_MIN) { - numericInputDrafts.value.topP = String(TOP_P_MIN) - } else if (draftNum > TOP_P_MAX) { - numericInputDrafts.value.topP = String(TOP_P_MAX) - } - } - const next = commitNumericField('topP', numericInputDrafts.value.topP)Also applies to: 2879-2882, 2913-2920
🤖 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/renderer/src/components/chat/ChatStatusBar.vue` around lines 1115 - 1117, The code is silently coercing low nucleus-sampling values because TOP_P_MIN is set to 0.1 and clamp logic auto-writes on commit; change TOP_P_MIN from 0.1 to 0 (or remove the hard floor) and stop auto-clamping in the commit/submit handler (the method that uses TOP_P_MIN/TOP_P_MAX and performs the clamp) — instead add explicit validation that rejects values below model-supported min and surfaces a UI validation error/message so manual low values are not overwritten; keep TOP_P_STEP as-is unless a finer step is required and ensure any clamp calls reference the new TOP_P_MIN or are only applied as an upper bound, not silently on commit.
🤖 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 `@docs/issues/chat-top-p-tooltip/plan.md`:
- Line 17: Update the plan text to reflect that Top P clamp behavior was changed
in ChatStatusBar.vue: state that the minimum Top P is now 0.1 (previously
lower), that commit-time clamping is applied (values are clamped when
committing/ saving), and any persistence/tests should expect the tightened
min/clamp behavior rather than "unchanged"; reference ChatStatusBar.vue and the
Top P min/clamp change so reviewers/testers use the correct assumptions.
---
Duplicate comments:
In `@src/renderer/src/components/chat/ChatStatusBar.vue`:
- Around line 1115-1117: The code is silently coercing low nucleus-sampling
values because TOP_P_MIN is set to 0.1 and clamp logic auto-writes on commit;
change TOP_P_MIN from 0.1 to 0 (or remove the hard floor) and stop auto-clamping
in the commit/submit handler (the method that uses TOP_P_MIN/TOP_P_MAX and
performs the clamp) — instead add explicit validation that rejects values below
model-supported min and surfaces a UI validation error/message so manual low
values are not overwritten; keep TOP_P_STEP as-is unless a finer step is
required and ensure any clamp calls reference the new TOP_P_MIN or are only
applied as an upper bound, not silently on commit.
🪄 Autofix (Beta)
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
Run ID: f8bb7365-ea03-4f44-ad4d-2d070d686f6f
📒 Files selected for processing (13)
docs/features/model-top-p-settings/plan.mddocs/features/model-top-p-settings/spec.mddocs/features/model-top-p-settings/tasks.mddocs/issues/chat-top-p-tooltip/plan.mddocs/issues/chat-top-p-tooltip/spec.mddocs/issues/chat-top-p-tooltip/tasks.mdsrc/main/presenter/agentRuntimePresenter/index.tssrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/settings/ModelConfigDialog.vuesrc/shared/contracts/common.tssrc/shared/contracts/domainSchemas.tssrc/shared/utils/generationSettingsValidation.tstest/main/shared/generationSettingsValidation.test.ts
✅ Files skipped from review due to trivial changes (4)
- docs/issues/chat-top-p-tooltip/tasks.md
- docs/issues/chat-top-p-tooltip/spec.md
- docs/features/model-top-p-settings/tasks.md
- docs/features/model-top-p-settings/spec.md
🚧 Files skipped from review as they are similar to previous changes (7)
- src/shared/contracts/common.ts
- docs/features/model-top-p-settings/plan.md
- test/main/shared/generationSettingsValidation.test.ts
- src/shared/contracts/domainSchemas.ts
- src/shared/utils/generationSettingsValidation.ts
- src/main/presenter/agentRuntimePresenter/index.ts
- src/renderer/src/components/settings/ModelConfigDialog.vue
|
|
||
| ## Compatibility | ||
|
|
||
| No persisted configuration changes. The chat settings Top P clamp behavior remains unchanged. |
There was a problem hiding this comment.
Plan text is stale relative to this PR’s Top P behavior.
Line 17 says clamp behavior is unchanged, but this PR also tightens Top P min/clamp behavior in ChatStatusBar.vue (e.g., min set to 0.1 and commit-time clamping). Please update the plan so reviewers/testers aren’t working from incorrect assumptions.
🤖 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 `@docs/issues/chat-top-p-tooltip/plan.md` at line 17, Update the plan text to
reflect that Top P clamp behavior was changed in ChatStatusBar.vue: state that
the minimum Top P is now 0.1 (previously lower), that commit-time clamping is
applied (values are clamped when committing/ saving), and any persistence/tests
should expect the tightened min/clamp behavior rather than "unchanged";
reference ChatStatusBar.vue and the Top P min/clamp change so reviewers/testers
use the correct assumptions.
close #1165
Summary by CodeRabbit