Skip to content

add SSH environments from the composer - #274

Merged
damienrj merged 7 commits into
mainfrom
codex/add-ssh-environment-flow
Sep 3, 2026
Merged

add SSH environments from the composer#274
damienrj merged 7 commits into
mainfrom
codex/add-ssh-environment-flow

Conversation

@damienrj

@damienrj damienrj commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Category: improvement
User Impact: Users can add and select a new SSH environment directly from the chat composer.

Problem: The environment picker only lists SSH hosts Berd already knows about, so adding a new host requires leaving the chat flow and finding the remote-host settings. Solution: Add a separated “Add SSH environment” action that opens an accessible connection dialog, accepts an SSH config alias or user@host, selects the environment after a successful connection, and keeps loading and error feedback in context. This PR is stacked on #269 and should merge after it.

File changes

src/features/chat/ui/AddRemoteHostDialog.tsx
Adds the focused connection dialog, including validation, pending-state protection, backend error feedback, and selection after a successful connection.

src/features/chat/ui/RemoteHostSelector.tsx
Adds the separated “Add SSH environment” menu action and preserves focus while transitioning from the environment menu into the dialog.

src/features/chat/ui/tests/RemoteHostSelector.test.tsx
Covers the empty-host menu, successful add-and-select flow, and retained dialog feedback when connection fails.

src/shared/i18n/locales/en/chat.json
Adds localized copy and accessible labels for the new environment action and dialog.

@damienrj
damienrj changed the base branch from codex/fix-remote-daemon-recovery to main September 2, 2026 16:19
@damienrj
damienrj requested a review from a team September 2, 2026 16:19

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

REQUEST_CHANGES. The remote-host lifecycle can be overwritten by stale connection completions and status events, the legacy lock fallback can reclaim a live successor lock, and reserved selector values collide with valid SSH aliases. The changed tests do not honestly exercise one pending-connect race. This is a meaningful UI change, but the supplied PR evidence contains no screenshots or screen recording; please add screenshots or a short screen recording. GitHub check evidence is present and currently pending, so required checks still govern merge readiness.

Deterministic publication result: 3 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.

🤖 Blocking · Screenshots needed

This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.

Comment thread src/features/remoteHosts/stores/remoteHostStore.ts Outdated
Comment thread src-tauri/src/services/remote_backend/remote_daemon.sh Outdated
Comment thread src/features/chat/ui/AddRemoteHostDialog.tsx
Comment thread src-tauri/src/services/remote_backend/mod.rs
@damienrj
damienrj force-pushed the codex/add-ssh-environment-flow branch from 0139c25 to 93a98e0 Compare September 2, 2026 18:30
@damienrj
damienrj changed the base branch from main to codex/fix-remote-daemon-recovery September 2, 2026 18:30
@damienrj

damienrj commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Automated code review

REQUEST_CHANGES. The remote-host lifecycle can be overwritten by stale connection completions and status events, the legacy lock fallback can reclaim a live successor lock, and reserved selector values collide with valid SSH aliases. The changed tests do not honestly exercise one pending-connect race. This is a meaningful UI change, but the supplied PR evidence contains no screenshots or screen recording; please add screenshots or a short screen recording. GitHub check evidence is present and currently pending, so required checks still govern merge readiness.

Deterministic publication result: 3 blocking and 1 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 1 blocking screenshot-evidence requirement(s) in this review body.

🤖 Blocking · Screenshots needed

This PR changes Berd’s graphical interface. Please add screenshots or a short screen recording so the visual result can be reviewed. Screenshots are review evidence; they do not replace accessibility, responsive, theme, localization, or behavior validation.

image

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

REQUEST_CHANGES. While an SSH connection is pending, the new modal disables or suppresses every dismissal route, so a connection attempt that never completes can trap the user and block the chat workflow indefinitely. The supplied screenshot evidence covers this meaningful graphical UI change. The four prior review issues are fixed in the current comparison and their resolved threads contain substantive human-account replies. GitHub check evidence is structurally valid and currently pending; required checks still govern merge readiness.

Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src/features/chat/ui/AddRemoteHostDialog.tsx Outdated
@damienrj
damienrj force-pushed the codex/add-ssh-environment-flow branch from e30bfb6 to e6869b2 Compare September 2, 2026 22:39

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

REQUEST_CHANGES. A connection that is forgotten or superseded while the add dialog waits can still be selected in the composer when its promise resolves, and the already-ready persistence path can lose concurrent manual-host additions by writing from a stale snapshot. The supplied evidence includes a screenshot for this meaningful graphical UI change. All five prior automated findings are resolved, fixed in the current comparison, and have substantive human-account replies. GitHub check evidence is structurally valid; required checks still govern merge readiness.

Deterministic publication result: 2 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src/features/chat/ui/AddRemoteHostDialog.tsx Outdated
Comment thread src/features/remoteHosts/stores/remoteHostStore.ts Outdated
@damienrj
damienrj force-pushed the codex/add-ssh-environment-flow branch from e6869b2 to 103d1c2 Compare September 2, 2026 23:20

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

COMMENT. The implementation is clear across product flow, graphical UX, architecture, and engineering, and the seven prior automated findings are fixed with substantive human-account replies in their resolved threads. One product vocabulary decision remains: the new flow calls the existing SSH host concept an SSH environment while adjacent surfaces continue to call it a host. The supplied evidence includes a screenshot for this meaningful graphical UI change. GitHub check evidence is structurally valid; required checks still govern merge readiness.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Base automatically changed from codex/fix-remote-daemon-recovery to main September 2, 2026 23:56
@damienrj

damienrj commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Addressed in 67e7ccf. The user-facing action, dialog title, and accessibility label now say “Add SSH host,” matching the existing “SSH hosts” terminology. “Environment” remains the broader concept of where a chat runs. The selector coverage was updated, and the focused tests plus the full frontend gate pass.

@damienrj
damienrj force-pushed the codex/add-ssh-environment-flow branch from 67e7ccf to a5aba65 Compare September 3, 2026 00:15

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

REQUEST_CHANGES. A superseded or forgotten connection returns false to the add-host dialog, which silently stops loading and leaves the unchanged dialog open without explaining what happened or how to recover. The supplied evidence includes screenshots for this meaningful graphical UI change. The seven prior automated findings are resolved, fixed in the current comparison, and have substantive human-account replies. GitHub check evidence is structurally valid; required checks still govern merge readiness.

Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Comment thread src/features/chat/ui/AddRemoteHostDialog.tsx Outdated

@morgmart morgmart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Automated code review

APPROVE. No new publishable findings remain after the full static review. The shared connection outcome can still be silently ignored by the existing Settings caller, but that is the same underlying superseded-connection feedback issue as the protected resolved thread and is therefore suppressed. The eight prior automated threads are resolved with substantive human-account replies. Supplied evidence includes screenshots for this meaningful graphical UI change. GitHub check evidence is structurally valid; required checks still govern merge readiness.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

Pending checks: 3 check(s) are not complete.

This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.

@damienrj
damienrj merged commit 5c31189 into main Sep 3, 2026
10 checks passed
@damienrj
damienrj deleted the codex/add-ssh-environment-flow branch September 3, 2026 03:40
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.

2 participants