Skip to content

[#24244] Hide terminator scrollbars#17

Open
Danipiza wants to merge 2 commits intomainfrom
fix/textual-scroll
Open

[#24244] Hide terminator scrollbars#17
Danipiza wants to merge 2 commits intomainfrom
fix/textual-scroll

Conversation

@Danipiza
Copy link
Contributor

@Danipiza Danipiza commented Mar 6, 2026

This PR adds Terminator support to TerminalSession so scrollbars are hidden during VulcanAI execution and restored afterward, matching the existing GNOME behavior pattern.

Signed-off-by: danipiza <dpizarrogallego@gmail.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves terminal UX output and expands terminal emulator support by adding a Terminator-specific adapter for hiding scrollbars during a console session.

Changes:

  • Fixes the rendered “Success Criteria” label in plan string formatting output.
  • Refactors GNOME Terminal gsettings helper into GnomeTerminalAdapter and adds a new TerminatorTerminalAdapter that switches profiles via remotinator.
  • Extends TerminalSessionConfig and default adapter list to include Terminator behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/vulcanai/core/plan_types.py Corrects the printed “Success Criteria” label in plan visualization output.
src/vulcanai/console/terminal_session.py Adds Terminator support (profile switching + config manipulation) and updates session defaults/adapters.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+ f"<{color_value}>{node.timeout_ms} ms</{color_value}>"
)
if node.success_criteria:
# Succes Criteria: <node.success_criteria>
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The inline comment still contains a typo: "Succes Criteria" should be "Success Criteria" to match the rendered label and avoid confusing output/log search.

Suggested change
# Succes Criteria: <node.success_criteria>
# Success Criteria: <node.success_criteria>

Copilot uses AI. Check for mistakes.
Comment on lines +332 to +333
return (terminal_uuid, self._config.terminator_profile_base)

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

TerminatorTerminalAdapter.apply() always restores to terminator_profile_base from config, not the profile that was active before switching. This can leave users on the wrong profile after the session. Consider querying the current profile via remotinator (or otherwise capturing the actual active profile) and storing that in the returned state so restore() can reliably revert to the pre-session profile.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@cferreiragonz cferreiragonz Mar 12, 2026

Choose a reason for hiding this comment

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

This is True. We restore to the default profile always. If it does not exist we could fail and right now we do not provide any way of modifying TerminalSessionConfig

Comment on lines +278 to +282
if changed:
try:
config_path.write_text("".join(lines), encoding="utf-8")
except Exception:
return False
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

_ensure_hidden_profile() can permanently modify the user's Terminator config (e.g., creating a new profile or overwriting an existing profile's scrollbar_position) and there is no restoration/rollback in restore(). This has user-facing operational impact. Consider tracking whether the file/profile was changed and reverting it on session end, or only creating/modifying a uniquely-named VulcanAI profile when it doesn't already exist (without overwriting user-defined profiles).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@cferreiragonz cferreiragonz left a comment

Choose a reason for hiding this comment

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

We are permanently modifying the configuration profile of the user by adding a new profile. Maybe we should delete it when VulcanAI is closed?

Comment on lines +219 to +229
profiles_end = len(lines)
for index in range(profiles_start + 1, len(lines)):
if cls._TOP_LEVEL_SECTION_RE.match(lines[index]) and lines[index].strip() != "[profiles]":
profiles_end = index
break

profile_headers: list[tuple[str, int, str]] = []
for index in range(profiles_start + 1, profiles_end):
header_match = cls._PROFILE_HEADER_RE.match(lines[index].rstrip("\r\n"))
if header_match:
profile_headers.append((header_match.group(2).strip(), index, header_match.group(1)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fusion both for loops into one and just check cls._TOP_LEVEL_SECTION_RE.match(lines[index]) and lines[index].strip() != "[profiles]" before breaking

Suggested change
profiles_end = len(lines)
for index in range(profiles_start + 1, len(lines)):
if cls._TOP_LEVEL_SECTION_RE.match(lines[index]) and lines[index].strip() != "[profiles]":
profiles_end = index
break
profile_headers: list[tuple[str, int, str]] = []
for index in range(profiles_start + 1, profiles_end):
header_match = cls._PROFILE_HEADER_RE.match(lines[index].rstrip("\r\n"))
if header_match:
profile_headers.append((header_match.group(2).strip(), index, header_match.group(1)))
profiles_end = len(lines)
profile_headers: list[tuple[str, int, str]] = []
for index in range(profiles_start + 1, len(lines)):
if cls._TOP_LEVEL_SECTION_RE.match(lines[index]) and lines[index].strip() != "[profiles]":
profiles_end = index
break
header_match = cls._PROFILE_HEADER_RE.match(lines[index].rstrip("\r\n"))
if header_match:
profile_headers.append((header_match.group(2).strip(), index, header_match.group(1)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had it separated for a cleaner logic. But this is more efficient.

@Danipiza Danipiza force-pushed the fix/textual-scroll branch from 36d989e to 9504de1 Compare March 12, 2026 15:30
…MINATOR_PROFILE'

Signed-off-by: danipiza <dpizarrogallego@gmail.com>
@Danipiza Danipiza force-pushed the fix/textual-scroll branch from 6d4a7f7 to c97003a Compare March 16, 2026 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants