[HYPERSHELL-129] feat(web-console): Adding section to gateway UI with instructions for sandbox connecting - #208
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
RunItBack1127
changed the title
195101b to
a540bd8
Compare
Amber reviewStatus: Complete VerdictCOMMENT — This is a clean, well-tested UI addition that adds a "Connect to a sandbox" step to the gateway Connection tab, reusing the existing Hi, Amber here. Two-sentence summary: PR #208 adds a copyable, inline-editable FindingsMinor — Docs link points at a Mintlify preview domain, not the canonical docs host ( Minor — Unrelated comment deletion in Minor — New connect command quotes its positional arg; the sibling create command still does not ( Nit / UX question — shared Cross-PR coordinationI reviewed the other open PRs in Material conflict — PR #210
Decision maintainers need: choose a merge order for #208 and #210, agree on the final Connection-tab step ordering (install → setup → create → connect, or otherwise), and have the second PR rebase to (a) set the Not material: #214 and #209 ("HYPERSHELL-112 Dashboard UI"; #214 is #209 "with adjustments" — those two duplicate each other and should be de-duplicated between their authors) touch Findings Summary (highest severity first)
Convention Checklist
|
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT — This is a clean, well-tested UI addition that adds a "Connect to a sandbox" step to the gateway Connection tab, reusing the existing EditableCommand component and shell-quoting helper correctly, with good unit coverage. No blockers or security issues; the actionable items are a docs-link domain inconsistency, an unrelated comment deletion, and — most importantly — a material overlap with PR #210 that maintainers must sequence.
Hi, Amber here. Two-sentence summary: PR #208 adds a copyable, inline-editable openshell sandbox connect <name> --editor <vscode|cursor> step to the Connection tab, wired through the shared EditableCommand and shellArgument quoting, with matching i18n keys and unit tests. The feature is solid; my only substantive concern is coordination with PR #210, which restructures the very same Connection-tab component and test in an incompatible-if-both-merged way.
Findings
Minor — Docs link points at a Mintlify preview domain, not the canonical docs host (packages/gateway-management-ui/src/gateways/gateway-connections.ts:80-81)
The new sandboxConnectDocsUrl resolves to https://nvidia-openshell.mintlify.app/..., whereas the existing installDocsUrl uses the canonical https://docs.nvidia.com/openshell/.... A Mintlify-hosted app subdomain is typically a preview/staging surface and is more likely to move or break than the published docs host. Recommend pointing to the docs.nvidia.com equivalent for consistency and link durability. Confidence: Medium.
Minor — Unrelated comment deletion in editable-command.tsx (packages/gateway-management-ui/src/gateways/editable-command.tsx:90)
This PR removes the comment explaining why the Enter key is swallowed (it would break both the single-line layout and the shell command). That rationale is genuinely useful to future maintainers, and the deletion is unrelated to the connect-sandbox feature. Suggest restoring it (or dropping the change from this PR to keep the diff focused). Confidence: High.
Minor — New connect command quotes its positional arg; the sibling create command still does not (gateway-connections.ts:122-146)
buildSandboxConnectCommand correctly wraps both name and editor in shellArgument(...), but the pre-existing buildSandboxCreateCommand still emits a raw --name ${name}. Both are fed from the same sanitized editable field, so this is not exploitable today, but the inconsistency is worth a follow-up so the two commands quote identically. Confidence: Medium (pre-existing; not introduced here).
Nit / UX question — shared sandboxName state mirrors across the create and connect steps
Editing the "Existing sandbox name" field in the connect step also rewrites the create command above (asserted by the new "mirrors an edited sandbox name" test). This looks intentional (create then reattach to the same name), but confirm it is the desired UX rather than two independent fields. Confidence: Low.
Cross-PR coordination
I reviewed the other open PRs in openshift-online/hypershell for material (design/plan/interface/ordering) conflicts with #208. Open PRs at review time: #216, #214, #212, #211, #210, #209, #207, #206, #201, #200, #194, #189, #188, #185, #182, #179, #151, #150, #148, #135, #109, #75, #73.
Material conflict — PR #210 [HYPERSHELL-259] feat(web-console): add gateway-matched CLI installation.
#210 and #208 independently restructure the same Connection-tab component and its test, and their changes are not composable as-is:
- Both edit
packages/gateway-management-ui/src/gateways/gateway-connection-steps.tsx, adding a new command block to the same<ol>of steps (#208 appends a "Connect to a sandbox" step; #210 prepends a version-matched "Install the OpenShell CLI" command block). This is a design/ordering decision — the combined step order of the tab is undefined until maintainers pick one. - Both edit
gateway-connection-steps.test.tsxand bump the same shared assertioncontainer.querySelectorAll(".shiki").lengthfrom2— #208 sets it to3, #210 sets it to3. If both merge, the tab will render four highlighted command blocks, so the assertion must become4; a naive merge that keeps either literal will produce a failing/incorrect test. This is a logical conflict, not a textual one. - Both add builders + tests to
gateway-connections.ts/gateway-connections.test.tsand both add keys tomessages.ts/components/web-console/locales/en.json. Additionally, #210 repurposes the sharedconnectionInstallLinkmessage (from "Install the OpenShell CLI" to "View installation documentation") and removesapp.gateway.serviceAccounts.manage; #208 leaves these as-is. Whichever lands second must reconcile against the renamed/removed keys, not just merge new ones. - Both modify
editable-command.tsx(#208 deletes a comment; #210 addseditable-command.test.tsxand changes the component). Competing edits to the same shared component.
Decision maintainers need: choose a merge order for #208 and #210, agree on the final Connection-tab step ordering (install → setup → create → connect, or otherwise), and have the second PR rebase to (a) set the .shiki count to the true combined total (4), (b) reconcile the connectionInstallLink rename / serviceAccounts.manage removal, and (c) unify the editable-command.tsx edits.
Not material: #214 and #209 ("HYPERSHELL-112 Dashboard UI"; #214 is #209 "with adjustments" — those two duplicate each other and should be de-duplicated between their authors) touch components/web-console/locales/en.json and CLAUDE.md, but only in the dashboard area; any overlap with #208 is a routine text merge in en.json, not a design conflict. #216 and #201 are control-plane/deploy changes with no overlap in goals, interfaces, or data model with #208. No other open PR conflicts materially with #208.
Findings Summary (highest severity first)
- [Major] Material cross-PR conflict with #210 over the Connection-tab step layout and the shared
.shikicount assertion (combined total must be 4, both PRs hardcode 3) — Cross-PR coordination - [Minor] Docs link uses a Mintlify preview domain instead of canonical
docs.nvidia.com— Security/Config consistency (gateway-connections.ts:80-81) - [Minor] Unrelated deletion of the Enter-swallow rationale comment — Maintainability (editable-command.tsx:90)
- [Minor] Connect command quotes its arg but sibling create command still does not — Consistency (gateway-connections.ts:122-146)
- [Nit] Shared
sandboxNamestate mirrors across create/connect steps — confirm intended UX
Convention Checklist
| Convention | Result |
|---|---|
| Input validated (shell arg quoting/sanitization) | Pass |
| No secrets in logs or responses | Pass (N/A — no secrets handled) |
| Config separate from code (docs URL as constant) | Pass |
i18n keys mirrored in messages.ts and en.json |
Pass |
| Test additions are additive, not silently rewritten guarantees | Pass |
| PatternFly 6 / reuse shared components | Pass (reuses EditableCommand, PF Alert) |
| Accessible labels + new-tab link semantics | Pass |
| "https://docs.nvidia.com/openshell/about/installation"; | ||
|
|
||
| export const sandboxConnectDocsUrl = | ||
| "https://nvidia-openshell.mintlify.app/reference/cli-sandbox#openshell-sandbox-connect"; |
There was a problem hiding this comment.
[Minor] Docs link uses a preview domain. sandboxConnectDocsUrl points at nvidia-openshell.mintlify.app, a Mintlify-hosted preview surface, while the sibling installDocsUrl (line 77-78) uses the canonical docs.nvidia.com/openshell/.... Preview subdomains are more likely to move/break. Recommend the canonical docs.nvidia.com equivalent for durability and consistency.
| className={[styles.field, colorClassName].filter(Boolean).join(" ")} | ||
| contentEditable="plaintext-only" | ||
| onInput={handleInput} | ||
| onKeyDown={(event) => { |
There was a problem hiding this comment.
[Minor] Unrelated comment removal. This PR drops the comment that explained why Enter is swallowed (a newline would break both the single-line layout and the shell command). That rationale is useful and the deletion is unrelated to the connect-sandbox feature — suggest restoring it or leaving it out of this PR to keep the diff focused.
|
|
||
| await waitFor(() => { | ||
| expect(container.querySelectorAll(".shiki").length).toBe(2); | ||
| expect(container.querySelectorAll(".shiki").length).toBe(3); |
There was a problem hiding this comment.
[Major] Cross-PR conflict with #210. #210 also bumps this shared .shiki count from 2 to 3 (it prepends a version-matched install command block); #208 bumps it to 3 for the new connect step. If both merge, the tab renders four highlighted command blocks, so this assertion must become 4 — a naive merge that keeps 3 will be wrong. Maintainers should pick a merge order and have the second PR set the true combined total and reconcile the Connection-tab step ordering. See the Cross-PR coordination section of the review.
3825c5c to
3e71fe1
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This is a clean, well-tested UI addition, but it currently ships with a failing unit test that turns the web-console CI gates red, and it structurally collides with another in-flight PR that rewrites the same connection-steps component. Fix the assertion and coordinate merge order before this lands.
The feature itself is solid: buildSandboxConnectCommand shell-quotes both the sandbox name and editor value via shellArgument (no copy-paste injection), the i18n strings are mirrored correctly between messages.ts and en.json, the docs link opens in a new tab with rel="noopener noreferrer", and the added tests cover defaults, custom name/editor, and shell-unsafe quoting. My concerns are the broken assertion and cross-PR coordination.
Critical
gateway-connection-steps.test.tsx:73 asserts the wrong Shiki block count — CI is red. The test was bumped to expect 4 highlighted command blocks, but GatewayConnectionSteps renders exactly three EditableCommand instances (one-time setup, create sandbox, connect sandbox), so only three .shiki <pre> elements ever resolve. The "Gateway management UI package quality gates" and "Web console quality gates" jobs both fail with AssertionError: expected 3 to be 4. The sibling assertion at line 95 correctly uses 3. Change line 73 to 3.
Minor
Shared sandbox-name state silently mirrors between the create and connect steps (gateway-connection-steps.tsx:215). Editing "Existing sandbox name" in the connect step writes back into the same sandboxName state used by the create step (and vice-versa). This is the intended "parity" behavior, but because the connect field is labelled Existing sandbox name a user may expect it to be independent of the just-created default. Consider a short note in the step description, or confirm this coupling is desired.
Cross-PR coordination
[HYPERSHELL-259] feat: reconcile gateway version for CLI installation restructures the exact same UI surface this PR extends, and the two designs cannot both land as-is. That PR renames the shared component EditableCommand -> CommandBlock in editable-command.tsx, renames the helper buildSetupScript -> buildOneTimeSetupScript in gateway-connections.ts, and adds an install-command block that changes the Shiki-block count test in gateway-connection-steps.test.tsx (2 -> 3). This PR instead keeps EditableCommand/buildSetupScript, adds a third block, and edits the same count assertion (to 4). These are competing interface/structure changes to the same component, helper module, and test — not a mechanical text merge. Maintainers should pick a merge order and have the second PR rebase onto the renamed component API and reconcile the block-count expectation. Whoever merges second will also need to re-add their step against the other's structure.
Findings Summary (ordered by severity, highest first)
- [Critical]
gateway-connection-steps.test.tsx:73expects 4 Shiki blocks but only 3 render; web-console CI gates fail — Test Correctness (L73) - [Minor] Connect-step "Existing sandbox name" field shares state with the create step, which may surprise users — UX / State Model (L215)
Convention Checklist
| Convention | Result |
|---|---|
| Tests pass / CI green | Fail |
| Input validated (shell-safe quoting of copied command) | Pass |
| No secrets in logs or responses | Pass |
i18n strings mirrored (messages.ts <-> en.json) |
Pass |
External links use rel="noopener noreferrer" + new tab |
Pass |
| Conventional commit messages | Pass |
| PatternFly components reused (Alert, Button, Content) | Pass |
| Modified existing test assertions justified | Pass |
|
|
||
| await waitFor(() => { | ||
| expect(container.querySelectorAll(".shiki").length).toBe(2); | ||
| expect(container.querySelectorAll(".shiki").length).toBe(4); |
There was a problem hiding this comment.
This assertion expects 4 .shiki blocks, but GatewayConnectionSteps renders only three EditableCommand instances (one-time setup, create sandbox, connect sandbox), so exactly three <pre class="shiki"> elements resolve. waitFor never reaches 4, so the callback keeps throwing and the test fails — this is what turns the Gateway management UI package quality gates and Web console quality gates jobs red (AssertionError: expected 3 to be 4). The sibling assertion at line 95 already uses 3.
| expect(container.querySelectorAll(".shiki").length).toBe(4); | |
| expect(container.querySelectorAll(".shiki").length).toBe(3); |
| markers={sandboxConnectMarkers} | ||
| onFieldChange={(marker, value) => { | ||
| if (marker === sandboxMarker) { | ||
| setSandboxName(value); |
There was a problem hiding this comment.
This setSandboxName(value) writes into the same sandboxName state the Create a sandbox step edits, so the two fields mirror each other even though this one is labelled "Existing sandbox name (editable)". If the intent is that a user connecting to a pre-existing sandbox may type a name unrelated to the one they just created, consider separate state; otherwise a one-line note in the step description would set expectations. (Minor — the shared-state behavior is intentional per the PR description.)
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
APPROVE-worthy in isolation, but posted as COMMENT because two other open PRs refactor the exact component and command builders this PR extends, and merge-order needs a maintainer call. The change itself is a clean, additive "Connect to a sandbox" step that reuses the established EditableCommand/marker pattern, ships matching messages.ts/en.json entries, adds solid unit tests, and even hardens the existing create command with shellArgument quoting.
What I checked
- Feature parity & pattern reuse: The new step mirrors the setup/create steps (marker templating,
onFieldChange, resolved copy text). Sharing thesandboxNamestate across the create and connect steps is intentional and covered by the "mirrors an edited sandbox name into both create and connect commands" test. - Security / shell safety:
buildSandboxConnectCommandroutes bothnameandeditorthroughshellArgument, and the PR also fixes the pre-existingbuildSandboxCreateCommandto quote--name. The "quotes shell-unsafe names" test proves injection-unsafe input is single-quoted. No secrets are logged or embedded. - i18n consistency: Every new
messages.tsid has a matchingen.jsonentry (copySandboxConnectCommand,editEditor,editExistingSandboxName,editorOptions*,sandboxConnect.*). The<code>rich-text chunk is wired correctly. - Test Diff Scrutiny: The two modified assertions in
gateway-connection-steps.test.tsx(.toBe(2)->.toBe(3)) and the renamed test titles are legitimate consequences of adding a third command block, not a silently removed guarantee. New behavior is added alongside, not in place of, the old. - Accessibility: The docs link uses
AlertActionLinkwithrel="noopener noreferrer",target="_blank", a "new tab" aria-label, andaria-hiddenon the icon, matching the existing install-link step.
Cross-PR coordination
Two other open pull requests refactor the same connection-steps component and sandbox command builders in ways that materially interact with this PR; a maintainer needs to decide merge order and who reconciles.
- #210 renames the shared
EditableCommandcomponent toCommandBlockandbuildSetupScripttobuildOneTimeSetupScript, and restructures the setup step (moving an install command into the alert). This PR adds a new connection step that imports and usesEditableCommandandbuildSetupScriptunder their current names. These are competing interface changes to the same component and connection-steps structure: whichever merges second must port the new "Connect to a sandbox" step onto the renamed API, and the renaming PR must account for this PR's added usage. Please decide an explicit merge order and owner for the reconciliation. - #245 redefines the output contract of
buildSandboxCreateCommand(prepending aDRIVER_CONFIG='...'variable and a--driver-config-jsonflag) and rewrites the same expected-output assertions this PR touches, while leaving--nameunquoted. This PR instead addsshellArgumentquoting to--name. These are competing changes to one function and its tests; on merge theshellArgumentquoting hardening introduced here must be preserved when combined with #245's restructured command, so the two must be reconciled deliberately rather than by naive conflict resolution.
Findings
Findings Summary (ordered by severity, highest first):
- [Minor]
editorfield accepts any value while the info alert advertises onlyvscode/cursor- UX / Input hint (gateway-connections.ts L138-146)
Convention Checklist (only rows evaluated for this change):
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Input validated / shell-arg quoting | Pass |
| Conventional commit / additive tests | Pass |
i18n keys synchronized (messages.ts <-> en.json) |
Pass |
| Test Diff Scrutiny (no removed guarantees) | Pass |
| Accessible external links | Pass |
|
|
||
| export function buildSandboxConnectCommand( | ||
| name: string = sandboxName, | ||
| editor: string = defaultEditor, |
There was a problem hiding this comment.
[Minor] buildSandboxConnectCommand accepts any editor string, and the inline editable field lets an operator type an arbitrary value, but the adjacent info alert states that only vscode and cursor are supported. shellArgument prevents injection here, so this is a UX nicety rather than a security issue - consider constraining the editor field to the supported set (or noting the value falls through to the CLI's own validation) so a mistyped editor is caught in the console rather than only at openshell runtime. Confidence: Medium.
…onnections, enables the syntax for displaying sandbox connect to the Hypershell UI
…age, shared naming state between the create and connect setup sections for parity
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-tested addition of a "Connect to a sandbox" step to the gateway Connection tab, with proper shell-argument escaping, i18n coverage, an accessible constrained editor selector, and a spec update to match. The only thing that needs maintainer attention is a cross-PR structural conflict with another open UI change; the code itself has just one minor stylistic note.
Nice work on defensive escaping (shellArgument now applied to the create command's --name as well), keeping the create/connect sandbox name in shared state, and adding focused tests for the new command, the dropdown constraint, and the name mirroring.
Findings
Minor - SelectField renders a raw native <select> (editable-command.tsx:108) styled via CSS (gateway-connection-steps.module.css:156). CLAUDE.md's "PatternFly 6 for web UI" convention prefers canonical PatternFly components. A native inline <select> is a defensible pragmatic choice here because the field lives inside a <pre><code> command block where a PatternFly Select/Dropdown popover would not fit visually, and it is consistent with the existing inline contentEditable EditableField. Worth confirming this deviation is intentional. Note the final commit message says "Adding PatternFly dropdown," which does not match the native-select implementation. Confidence: Medium.
Cross-PR coordination
Another open pull request, #210, restructures the same gateway-connection-steps.tsx command-block layout and rewrites the same test invariant. Both this PR and #210 change the .shiki block-count assertion in gateway-connection-steps.test.tsx from 2 to 3, but each adds a different third command block (this PR adds the sandbox-connect block; #210 adds a version-matched OpenShell install block). The correct combined result is four command blocks, so the two changes are mutually incompatible as written. Maintainers should decide a merge order and require whichever lands second to reconcile the block count, the step numbering, and the connection-step tests. This PR also raises the architecture.spec.md "ordered steps" invariant from three to four while #210 adds a step without touching that spec, so the spec's step count must be reconciled as part of the same decision.
Findings Summary (ordered by severity, highest first)
- [Minor] Native
<select>used instead of a PatternFly component; commit message ("PatternFly dropdown") does not match implementation - Convention / UI (editable-command.tsx:108, gateway-connection-steps.module.css:156)
Convention Checklist
| Convention | Result |
|---|---|
Input escaped for shell safety (shellArgument) |
Pass |
| No secrets in logs or responses | Pass |
| i18n messages defined and localized | Pass |
| Accessible names on interactive controls | Pass |
| Test Diff Scrutiny (no removed guarantees) | Pass |
| PatternFly reuse for web UI | Minor deviation |
| Conventional commit messages | Pass |
| ); | ||
| } | ||
|
|
||
| function SelectField({ |
There was a problem hiding this comment.
Minor: SelectField renders a raw native <select>. CLAUDE.md's "PatternFly 6 for web UI" convention prefers canonical PatternFly components. A native inline select is defensible here since the field lives inside a <pre><code> command block where a PatternFly Select/Dropdown popover would not fit, and it mirrors the existing inline EditableField. Please confirm the deviation is intentional. Also, the final commit message says "Adding PatternFly dropdown," which does not match this native-select implementation - consider aligning the message. Confidence: Medium.
…r test expectation
41fd415 to
b2e51fd
Compare
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES. This is a clean, well-tested, i18n-aware UI addition, but the PR ships a spec requirement (a SHALL) that the code does not implement: the updated architecture.spec.md mandates an editor selector and an editor value in the connect command, while the generated command and rendered step contain no editor at all. That contradiction, plus the dead editor scaffolding and the misleading "Editor options" alert, should be reconciled before merge.
Summary
The change adds a fourth "Connect to a sandbox" step to the gateway Connection tab, mirrors the sandbox name with the create step, and shell-quotes untrusted input correctly. The main problem is an internal spec-vs-implementation contradiction around the editor field, which surfaces as dead code and a misleading info alert.
Findings
[Major] Spec requires an editor selector and editor value that the code does not implement - Spec Consistency
specs/web-console/architecture.spec.md:505 (added in this PR) states: the connect command "references the sandbox name from step 3 and an editor value" and "The editor field SHALL be a constrained inline selector defaulting to cursor, offering only the supported values (cursor, vscode)." However:
buildSandboxConnectCommand(packages/gateway-management-ui/src/gateways/gateway-connections.ts:171) emitsopenshell sandbox connect <name>with no--editorflag.- The connect step (
gateway-connection-steps.tsx:199-215) renders only an editable sandbox-name field; there is no editor selector.
The PR description also shows openshell sandbox connect <SANDBOX_NAME> --editor <vscode|cursor>, which does not match what the UI actually produces. A SHALL in the spec that the same PR does not satisfy is a contract defect. Decide one of: (a) implement the constrained editor selector and add --editor to the command (and cover it in tests), or (b) soften the spec/description to describe an editor-less command. Confidence: High.
[Minor] Dead editor scaffolding left behind - Maintainability
validEditors and defaultEditor (gateway-connections.ts:168-169) and the editEditor message (messages.ts:264, plus its entry in components/web-console/locales/en.json) are defined but never referenced anywhere. These are the unused remnants of the editor selector from the finding above. Either wire them up or remove them so the exported surface and localized strings match the shipped UI. Confidence: High.
[Minor] "Editor options" alert advertises a capability the UI does not expose - UX / Content
The info alert (gateway-connection-steps.tsx:173-198) tells users "Supported interactive editors are vscode and cursor", but the copyable command contains no editor flag and there is no selector to choose one. As shipped, the alert points at a capability the step does not provide, which will confuse operators. This resolves once the editor field is either implemented or the messaging is aligned with the editor-less command. Confidence: Medium.
What looks good
buildSandboxConnectCommandreusesshellArgument, and a test asserts shell-unsafe names are quoted - injection is prevented.- Sandbox-name state is shared between the create and connect steps, matching the spec, and a test verifies the mirroring.
- The modified assertions in
gateway-connection-steps.test.tsx(.shikicount2->3, and the renamed "renders ... steps" test) are legitimate consequences of adding a third command block, not removed guarantees.
Cross-PR coordination
Another open pull request refactors the same gateway connection-step files (gateway-connection-steps.tsx, gateway-connections.ts, messages.ts) and renames shared symbols that this PR depends on: it replaces the EditableCommand component with CommandBlock and renames buildSetupScript to buildOneTimeSetupScript, and it also removes the AlertActionLink import that this PR's new connect step relies on. This is a competing interface change, not a plain merge conflict: whichever lands first, the other must be updated to the renamed component/function and reconcile the shared import set, so the maintainers need to pick a merge order and assign the follow-up adaptation. The affected pull request is #210.
Findings Summary (ordered by severity, highest first)
- [Major] Spec
SHALLfor an editor selector/value is unimplemented in the command and UI - Spec Consistency (spec L505;gateway-connections.tsL171;gateway-connection-steps.tsxL199-215) - [Minor] Unused editor scaffolding (
validEditors,defaultEditor,editEditor) - Maintainability (gateway-connections.tsL168-169;messages.tsL264) - [Minor] Info alert advertises editors the step cannot select - UX / Content (
gateway-connection-steps.tsxL173-198)
Convention Checklist
| Convention | Result |
|---|---|
| Input validated / shell-quoted (injection prevented) | Pass |
| No secrets in logs or responses | Pass |
| i18n strings defined and localized | Pass |
| Reuse canonical shared components (no duplicate UI) | Pass |
| Spec matches implementation | Fail |
| No dead/unused exported code | Fail |
| Test diff scrutiny (no silently removed guarantees) | Pass |
| No em dashes | Pass |
|
|
||
| 3. **Create a sandbox.** The step SHALL present an `openshell sandbox create` command that names a sandbox, specifies CPU and memory resource requests and limits via a `--driver-config-json` flag, attaches the provider from step 2, and launches the agent, as a read-only Clipboard Copy value. The default resource values SHALL be CPU requests `100m`, CPU limits `500m`, memory requests `512Mi`, and memory limits `512Mi`. | ||
|
|
||
| 4. **Connect to a sandbox.** The step SHALL present an `openshell sandbox connect` command that references the sandbox name from step 3 and an editor value. The sandbox name field SHALL share state with the create step so both commands stay in sync. The editor field SHALL be a constrained inline selector defaulting to `cursor`, offering only the supported values (`cursor`, `vscode`). An adjacent info alert SHALL document the supported values and link to the sandbox connect documentation on the canonical docs host. |
There was a problem hiding this comment.
This step now mandates (SHALL) a constrained editor selector defaulting to cursor and an editor value in the command, but the implementation renders no editor selector and buildSandboxConnectCommand emits no --editor flag. Either implement the selector + flag (with tests) or soften this requirement so the spec and code agree.
| export const validEditors = ["cursor", "vscode"] as const; | ||
| export const defaultEditor = "cursor"; | ||
|
|
||
| export function buildSandboxConnectCommand(name: string = sandboxName): string { |
There was a problem hiding this comment.
The produced command is openshell sandbox connect <name> with no --editor, which contradicts spec step 4 and the PR description example (--editor <vscode|cursor>). If the editor is intended, add the flag here and thread the selected editor through.
| return `${variable}\n\n${command}`; | ||
| } | ||
|
|
||
| export const validEditors = ["cursor", "vscode"] as const; |
There was a problem hiding this comment.
validEditors and defaultEditor are exported but never referenced anywhere in the diff or codebase. These look like leftovers from an unfinished editor selector - wire them in or remove them.
| defaultMessage: "Details", | ||
| description: "Label for the gateway detail Details tab.", | ||
| }, | ||
| editEditor: { |
There was a problem hiding this comment.
editEditor (and its en.json entry) is defined but never used - there is no editor control in the rendered step. Remove it or add the control it labels.
| title={intl.formatMessage(messages.connectionEditorOptionsTitle)} | ||
| variant="info" | ||
| > | ||
| {intl.formatMessage(messages.connectionEditorOptions, { |
There was a problem hiding this comment.
This alert tells users vscode and cursor are supported editors, but the step exposes no way to choose one and the copyable command has no editor flag. As shipped it advertises a capability the UI does not provide; align the messaging with the actual command or add the selector.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-scoped web-console change that adds a fourth "Connect to a sandbox" step to the gateway Connection tab, with an inline editor selector, mirrored sandbox-name state, shell-safe command building, and matching spec updates. The implementation and tests are coherent and I found no security, error-handling, or convention violations; my notes are minor, but a real cross-PR design decision is needed before merge (see below).
Amber Analysis
The new SelectField in EditableCommand, the buildSandboxConnectCommand builder, and the spec change from three to four ordered steps all line up, and shell-argument encoding is preserved for the sandbox name. The validEditors/defaultEditor values match the spec's constrained selector and the info-alert copy, and the modified test assertions (.shiki count 2 -> 3, renamed step tests) are legitimate additive updates for the new block rather than removed guarantees.
Findings
- [Minor] Missing interaction test for the editor selector (
gateway-connection-steps.test.tsx). The editor variants are only exercised through thebuildSandboxConnectCommandunit test; there is no UI-level test that changing the inline<select>updates the copied command. Adding one would lock in the newselectOptions/SelectFieldwiring. Confidence: High. - [Minor] Removed component contract doc (
editable-command.tsx). The JSDoc that documentedEditableCommand's marker/copyText/mirrored-slot contract was dropped whenSelectFieldwas introduced and not restored; a short doc noting the newselectOptionsbehavior would help future editors. Confidence: Medium.
Cross-PR coordination
Another open pull request (#210) makes competing changes to the same shared connection-steps surface, and a maintainer decision is required before both can land:
- It renames the shared
EditableCommandcomponent toCommandBlockand makes its props optional to support a static-command mode, while this PR extendsEditableCommandin place with a newselectOptionsprop andSelectField. These are competing, incompatible interface changes to the same component; owners must agree on the final component name and a merged prop contract. - It edits the same
specs/web-console/architecture.spec.md"Connection tab" section in a divergent direction: this PR changes it to "four ordered steps" and adds a "Connect to a sandbox" step, while the other rewrites the install/setup step, keeps "three ordered steps", and removes the service-account link and its scenario. The canonical step count, numbering, and enumeration must be reconciled, which also implies a merge order decision so the spec and thegateway-connection-steps.tsxstep list stay consistent.
Findings Summary (ordered by severity, highest first)
- [Minor] No UI-level test that the editor selector updates the copied command - Test Coverage (gateway-connection-steps.test.tsx L141-L181)
- [Minor] Removed
EditableCommandcontract JSDoc not restored - Docs (editable-command.tsx L139)
Convention Checklist
| Convention | Result |
|---|---|
| No secrets in logs or responses | Pass |
| Input validated / shell-arg encoding preserved | Pass |
| Spec updated to match implementation | Pass |
| Test Diff Scrutiny (modified assertions justified) | Pass |
| Conventional commit (PR title) | Pass |
| ); | ||
| }); | ||
|
|
||
| it("mirrors an edited sandbox name into both create and connect commands", async () => { |
There was a problem hiding this comment.
[Minor] Missing interaction test for the editor selector. The new connect-command tests cover the sandbox-name mirroring and the raw copy, and gateway-connections.test.ts covers the --editor vscode variant at the builder level, but nothing exercises the inline <select> in the UI. Consider adding a test that changes the editor selector and asserts the copied command switches to --editor vscode, so the selectOptions/SelectField wiring in EditableCommand is regression-protected. Confidence: High.
| ); | ||
| } | ||
|
|
||
| export function EditableCommand({ |
There was a problem hiding this comment.
[Minor] Restore the component contract doc. The JSDoc that explained the marker/copyText/mirrored-slot behavior of EditableCommand was removed when SelectField was introduced and was not re-added. A short doc block here noting the new optional selectOptions prop (marker -> constrained choices renders a <select> instead of an editable field) would keep the contract discoverable. Confidence: Medium.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a clean, well-tested, UI-only addition of a "Connect to a sandbox" step to the gateway Connection tab, with matching spec, i18n, and unit-test updates. The code itself has no blocking issues; the one item that needs human attention is a design/interface collision with another in-flight PR over the same shared component and helper functions.
What this PR does
Adds a fourth ordered step to the Connection tab that surfaces openshell sandbox connect <name> --editor <editor>. It extends the shared EditableCommand with an optional selectOptions prop rendering a new native <select> (SelectField) for the constrained editor choice, shares the sandbox-name state with the create step, and adds buildSandboxConnectCommand, validEditors, defaultEditor, and sandboxConnectDocsUrl in gateway-connections.ts. Spec, messages.ts, and en.json are updated consistently.
Strengths
- Shell safety is preserved: the connect command routes both the name and editor through
shellArgument, and there is an explicit unit test for a shell-unsafe name ('my $(sandbox)'). - The editor input is a constrained selector over
validEditors, so free-text injection is not possible for that slot; the free-text sandbox name reuses the existingsanitizeFieldValuecharset guard. - Accessibility is handled: the new
<select>carries anaria-label, and the connect-step sandbox field uses a distinct accessible name (Existing sandbox name (editable)) so it does not collide with the create step's field. - Good test coverage: builder unit tests for defaults/custom name/custom editor/quoting, plus component tests for the new step, the copy button, and cross-step name mirroring.
Test Diff Scrutiny
The modified assertions in the pre-existing test files are additive, not removed guarantees. .shiki count 2 -> 3 and the renamed "renders the setup, sandbox create, and sandbox connect steps" test reflect the newly added step and add assertions rather than flipping an existing contract (e.g. accepted -> rejected). No optional-to-required tightening or backfill concern here.
Minor findings
- [Minor] The
editorOptionsinfo-alert text lists the supported editors asvscodethencursor, whilevalidEditorsanddefaultEditorpresent/default tocursorfirst. Cosmetic ordering inconsistency only; consider aligning the message order with the selector order so users see the same order in the note and the dropdown. (messages.ts/en.json)
Cross-PR coordination
A competing, in-flight change (#210) refactors the exact shared building blocks this PR extends: it renames the EditableCommand component to CommandBlock in editable-command.tsx, renames buildSetupScript to buildOneTimeSetupScript, and adds an install-command block into the same GatewayConnectionSteps composition. This PR instead extends EditableCommand in place with a new selectOptions prop and a SelectField, and continues to import buildSetupScript. These are competing interface changes to the same component and helpers, not a plain text merge conflict: whichever merges first, the other must be reworked. Maintainers should decide the merge order and who re-homes this PR's selectOptions/SelectField extension onto the renamed CommandBlock API (and updates the buildSetupScript import), or fold the editor selector into that refactor.
Findings Summary (ordered by severity, highest first)
- [Minor] Editor ordering in the info-alert text differs from the selector/default order - Consistency (messages.ts L88, gateway-connections.ts L168)
Convention Checklist
| Convention | Result |
|---|---|
| Input validated / shell-argument encoding | Pass |
| No secrets in logs or responses | Pass |
| Accessible names on interactive controls | Pass |
| Test diff scrutiny (no silently removed guarantees) | Pass |
| Spec updated to match behavior | Pass |
| i18n messages synced (messages.ts / en.json) | Pass |
| No em dashes | Pass |
| connectionEditorOptions: { | ||
| id: "app.gateway.connection.editorOptions", | ||
| defaultMessage: | ||
| "Supported interactive editors are <code>vscode</code> and <code>cursor</code>.", |
There was a problem hiding this comment.
Minor consistency nit: this note lists the editors as vscode then cursor, but validEditors is ["cursor", "vscode"] and defaultEditor is cursor (gateway-connections.ts L168-169), so the dropdown shows cursor first and pre-selects it. Consider ordering the message as cursor then vscode so the info note matches what the selector shows. Remember to keep en.json in sync.
| ); | ||
| } | ||
|
|
||
| export function EditableCommand({ |
There was a problem hiding this comment.
Heads-up for coordination (no change required in isolation): another in-flight PR renames this exported EditableCommand component to CommandBlock and refactors its call sites, while this PR extends EditableCommand in place with the new selectOptions/SelectField path. Whichever lands first, this extension will need to be re-applied onto the renamed API. See the Cross-PR coordination section in the review summary.
2 participants
Summary
Connectiontab calledConnect to a sandbox, which provides a convenience command for the user to connect to an existing sandbox when they open it in the Gateway UIChanges
gateway-connection-steps.tsx, along with two 2EditableFieldcomponents for the sandbox name and the editor nameConnectiontabTest Plan
make kind-web-console-upto run the local web serveropenshell sandbox connect <SANDBOX_NAME>to connect to the sandboxpnpm --filter @openshift-online/hypershell-gateway-management-ui test:run