Srihari Sriraman shared in

Public

Reviewed and fixed PR setting persistence issuePublic

Agent

Model

Claude Opus 4.5 (latest)

Started

Jan 29, 2:13 AM

Messages178

Downloads0

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:

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

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

Let me check for inline review comments:

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

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);}
```
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

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

Now add the changelog entries:

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

Found it. Missing markModified() call:

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.

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

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