Reviewed and fixed PR setting persistence issue

Summary

Closed, Completed

Understand
Read PR, linked issues, and source files.
Understand
Edit
Fixed missing setter modification for settings persistence.
Edit
Verify
Confirmed fix worked and committed changes.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/972

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Now let me read the linked issue #916 and check the existing source files:

Readeditor.ts

Let me read the rest of the editor.ts file and check for changelog entries:

Readeditor.ts

ReadCHANGELOG.md

Let me check the current state of the relevant files and look at the PR review comment:

Readsettings-manager.ts100 lines

Readsettings.md

Let me check for inline review comments:

Let me check the interactive-mode.ts to understand the editor pattern:

Readinteractive-mode.ts50 lines

I see. The this.editor property is the active editor (which could be either defaultEditor or a custom editor from an extension). The review comment is correct: when updating the autocomplete max visible setting from the settings menu, it should update this.editor (the currently active editor), not just defaultEditor.

However, looking at the editorPaddingX pattern further, I need to verify that pattern:

I see! The existing onEditorPaddingXChange pattern does:

Update this.defaultEditor.setPaddingX(padding)
Also update this.editor.setPaddingX(padding) if it's a different editor

So the review comment is correct - the PR needs to follow the same pattern for autocompleteMaxVisible.

Now let me summarize the PR review:

PR Summary:

Adds a configurable autocompleteMaxVisible setting (range 3-20, default 5) to control dropdown height
Changes in packages/tui (Editor component) and packages/coding-agent (settings, interactive mode, settings selector)
Follows the same pattern as editorPaddingX

Issues Found:

Review comment not addressed: The callback uses this.defaultEditor but should follow the editorPaddingX pattern using this.editor (to also update custom editors)

Changelog Status:

No changelog entries found in either packages/tui/CHANGELOG.md or packages/coding-agent/CHANGELOG.md for this feature

Documentation:

packages/coding-agent/docs/settings.md is updated in the diff (adds the autocompleteMaxVisible row to the table)

Let me verify I've checked everything and provide the review.

Now I have all the information needed to provide a complete review.

PR: https://github.com/badlogic/pi-mono/pull/972

Changelog:

Missing: No changelog entry in packages/tui/CHANGELOG.md or packages/coding-agent/CHANGELOG.md
Required before merge. Will add:
- packages/tui/CHANGELOG.md: ### Added - autocompleteMaxVisible option in EditorOptions with getter/setter methods
- packages/coding-agent/CHANGELOG.md: ### Added - autocompleteMaxVisible setting for configurable autocomplete dropdown height (attribution to @masonc15)

Good:

Follows the established editorPaddingX pattern consistently across both packages
Proper input validation with Math.max(3, Math.min(20, Math.floor(maxVisible))) in both settings-manager and Editor
Uses Number.isFinite() for defensive validation in the Editor component
Documentation is updated in settings.md with the new setting
Clean UI integration with preset values [3, 5, 7, 10, 15, 20] in settings selector

Bad:

Review comment not addressed: In interactive-mode.ts, the onAutocompleteMaxVisibleChange callback uses this.defaultEditor.setAutocompleteMaxVisible(maxVisible) but should follow the editorPaddingX pattern which also updates this.editor when a custom editor is active. The pattern is:

typescript

this.defaultEditor.setAutocompleteMaxVisible(maxVisible);if (this.editor !== this.defaultEditor && this.editor.setAutocompleteMaxVisible !== undefined) {    this.editor.setAutocompleteMaxVisible(maxVisible);}

this.defaultEditor.setAutocompleteMaxVisible(maxVisible);if (this.editor !== this.defaultEditor && this.editor.setAutocompleteMaxVisible !== undefined) {    this.editor.setAutocompleteMaxVisible(maxVisible);}

Missing pkg:tui and pkg:coding-agent labels on the PR

Ugly:

None

Questions or Assumptions:

None. The issue and PR are clear about the intent.

Change summary:

packages/tui/src/components/editor.ts: Adds autocompleteMaxVisible field to EditorOptions and Editor class with getter/setter, uses it in SelectList creation (replaces hardcoded 5)
packages/coding-agent/src/core/settings-manager.ts: Adds autocompleteMaxVisible to Settings interface with getter/setter
packages/coding-agent/src/modes/interactive/components/settings-selector.ts: Adds UI toggle with preset values
packages/coding-agent/src/modes/interactive/interactive-mode.ts: Passes setting to editor, wires up callback
packages/coding-agent/docs/settings.md: Documents the new setting

Tests:

No automated tests added. This is a UI preference feature with straightforward value passing, so manual testing is sufficient.

merge via gh cli, pull from origin, fix the review shit

Readeditor.ts50 lines

Editededitor.ts

Now let me fix the review issue in interactive-mode.ts:

Readinteractive-mode.ts30 lines

Readinteractive-mode.ts15 lines

Editedinteractive-mode.ts

Now add the changelog entries:

ReadCHANGELOG.md10 lines

EditedCHANGELOG.md

ReadCHANGELOG.md20 lines

EditedCHANGELOG.md

Readeditor-component.ts

Editededitor-component.ts

Done. Merged PR #972 to main with fixes:

Fixed review issue: Changed this.defaultEditor.setAutocompleteMaxVisible() to also update this.editor when a custom editor is active (following the editorPaddingX pattern)
Added setAutocompleteMaxVisible to EditorComponent interface for type safety
Added changelog entries to both packages/tui/CHANGELOG.md and packages/coding-agent/CHANGELOG.md with proper attribution to @masonc15
Resolved merge conflicts in editor.ts (adapted to new autocompleteState pattern on main)

when i change the max lines autocomplete shit in /settings, it is applied (e.g. 5 -> 10), but if i then open /settings again it shows 5

The user is saying that when they change the autocompleteMaxVisible setting in /settings, it applies visually, but when they reopen /settings, it shows the old value (5) instead of the new value (10).

This suggests that either:

The setting is not being saved properly to settings.json
The settings selector is not reading the current value correctly when it opens

Let me check the code to understand the flow:

settings-selector.ts - how it gets the current value
settings-manager.ts - the getter/setter
interactive-mode.ts - how it passes the config to the selector

Let me look at these files.

Readsettings-selector.ts30 lines

Readsettings-manager.ts20 lines

Found it. Missing markModified() call:

Editedsettings-manager.ts

Fixed. The setter was missing markModified("autocompleteMaxVisible") which is required for settings to persist after the recent external-edit preservation change.

works, commit and push

just the file(s) you modified, no other file that's changed on disk, another agent is working on that

Already done - committed and pushed in the previous step. The uncommitted changelog changes are from the other agent.