feat(Demo): add multi-agent browser demo workspaces - #226
Merged
themassiveone merged 23 commits intoAug 3, 2026
Merged
Conversation
coderabbitai
Bot
added
release-note
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded four Agent-Up demo workspaces, a shared HTTP renderer, direct process launching, workspace display overrides, staged browser clicks, application synchronization, workspace cleanup, and recording workflows. ChangesAgent-Up features and demo workspaces
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant AgentUp
participant DemoProcess
participant Browser
Agent->>AgentUp: Start configured workspace
AgentUp->>DemoProcess: Launch Node application
Agent->>Browser: Inspect local application route
Browser->>DemoProcess: Request HTML or JSON
DemoProcess-->>Browser: Return rendered response
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Demo/agent4/workspace.mjs`:
- Around line 5-6: Assign agent 4 unique default Storefront, AdminPanel,
Fulfillment, and Postgres ports in Demo/agent4/workspace.mjs, then synchronize
those values in Demo/agent4/agent-up.json. Update Demo/agent1/agent-up.json and
Demo/agent3/agent-up.json with matching dedicated port ranges that do not
overlap any workspace. Document the assigned ranges or explicit
environment-variable overrides for concurrent recording in Demo/README.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3377113-0e8e-4a73-b8e3-1406cf67b965
📒 Files selected for processing (30)
Demo/README.mdDemo/agent1/PROMPT.mdDemo/agent1/agent-up.jsonDemo/agent1/backend/server.mjsDemo/agent1/dashboard/server.mjsDemo/agent1/marketing-site/server.mjsDemo/agent1/postgres/server.mjsDemo/agent1/workspace.mjsDemo/agent2/PROMPT.mdDemo/agent2/agent-up.jsonDemo/agent2/dashboard/server.mjsDemo/agent2/marketing-site/server.mjsDemo/agent2/postgres/server.mjsDemo/agent2/worker/server.mjsDemo/agent2/workspace.mjsDemo/agent3/PROMPT.mdDemo/agent3/admin-panel/server.mjsDemo/agent3/agent-up.jsonDemo/agent3/payments/server.mjsDemo/agent3/postgres/server.mjsDemo/agent3/storefront/server.mjsDemo/agent3/workspace.mjsDemo/agent4/PROMPT.mdDemo/agent4/admin-panel/server.mjsDemo/agent4/agent-up.jsonDemo/agent4/fulfillment/server.mjsDemo/agent4/postgres/server.mjsDemo/agent4/storefront/server.mjsDemo/agent4/workspace.mjsDemo/common/demo-server.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AgentUp.Server/Features/Processes/Providers/LocalProcessProvider.cs`:
- Line 97: Harden CreateWorkspaceDirectoryAlias so it fails closed: use a
securely private alias root, revalidate any existing alias target after
IOException races, and never return an unverified aliasPath. If symbolic-link
creation is unavailable on Windows, return workingDirectory or another tested
safe fallback, ensuring the resulting path is verified before use.
In `@docs/user-docs/agent-up-json-reference.md`:
- Line 39: Update the root application-command description in this document to
describe direct executable launching rather than opaque shell commands. State
that the first token must be an allowlisted executable name, and clarify that
custom executable names or paths fail validation at launch time; keep the
wording user-facing and consistent with the `command` field entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bb793f8-bd2f-42c4-9104-d8b481657927
📒 Files selected for processing (3)
AgentUp.Server.Tests/Features/Processes/Provider/WorkspaceProcessManagerTests.csAgentUp.Server/Features/Processes/Providers/LocalProcessProvider.csdocs/user-docs/agent-up-json-reference.md
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.cs (1)
52-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDetect
Commandchanges inUpdateFrom.
UpdateFromassignsCommandunconditionally but does not include it in the returned change flag. If onlyCommandchanges (ports and state stay the same), the method returnsfalse.WorkspaceItemViewModel.UpdateFromuses that return value to decide whether to raiseApplicationsChanged, so a command-only update will not refresh the application panel inMainViewModel. AddCommandto the change detection.🐛 Proposed fix
public bool UpdateFrom(string command, string state, IReadOnlyList<PortMappingDto>? allocatedPorts) { var ports = allocatedPorts ?? []; var portsChanged = !AllocatedPorts.SequenceEqual(ports); + var commandChanged = !string.Equals(Command, command, StringComparison.Ordinal); Command = command; AllocatedPorts = ports; - return portsChanged | UpdateState(state); + return commandChanged | portsChanged | UpdateState(state); }Consider adding a test case in
WorkspaceItemViewModelTests.csthat changes onlyCommandand assertsApplicationsChangedfires once, since the current tests always pair a command change with a port or state change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.cs` around lines 52 - 60, Update WorkspaceApplicationViewModel.UpdateFrom to compare the incoming command with the current Command before assignment and include that result in the returned change flag, preserving the existing port and state detection. Add a WorkspaceItemViewModelTests case covering a command-only change and verify ApplicationsChanged is raised once.
🧹 Nitpick comments (3)
AgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.cs (1)
17-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelay injection is inconsistent; some call sites still use the static
DelayAsync.This change adds
_delayas an injectable delegate for testability. However, lines 58, 74, and 184 still call the staticDelayAsyncmethod directly instead of_delay. This leaves those wait times untestable and, in tests, causes a real ~150 ms sleep in any test path that exercisesReadSettledPageStateAsync(line 184).Route the remaining call sites through
_delayfor consistency with lines 199, 208, and 217.♻️ Proposed fix for consistent delay injection
- await DelayAsync(500, ct); + await _delay(500, ct);- await DelayAsync(1000, ct); + await _delay(1000, ct);- await DelayAsync((int)PageStateSettleInterval.TotalMilliseconds, ct); + await _delay((int)PageStateSettleInterval.TotalMilliseconds, ct);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.cs` around lines 17 - 36, Update the remaining delay calls in BrowserCommandPoller, including the paths around ReadSettledPageStateAsync and the call sites near lines 58 and 74, to invoke the injected _delay delegate instead of the static DelayAsync method. Preserve each existing delay duration and cancellation token argument, matching the already-updated call sites near lines 199, 208, and 217.AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs (1)
198-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale comment on
SwitchApplicationTabForUrl.The comment says the method "Only acts when url targets the currently active workspace."
vm.SelectApplicationForUrlnow switchesSidebar.SelectedWorkspacefor anyworkspaceId, regardless of which workspace is currently active. Update the comment to describe the actual behavior: a background navigation on any workspace can now switch the visible workspace in the desktop UI.Also note:
SelectApplicationForUrlcurrently switches the workspace selection before it confirms a matching application/port exists for the target URL. See the review comment onMainViewModel.SelectApplicationForUrlfor the underlying issue; this call site inherits that risk since it discards the returned boolean.📝 Proposed comment fix
- // Switches the application and sub-tab to match the port in url, so the user can watch - // the agent's navigation. Only acts when url targets the currently active workspace. + // Switches the application and sub-tab to match the port in url, so the user can watch + // the agent's navigation. Switches the visible workspace to match workspaceId even when + // a different workspace is currently active. private void SwitchApplicationTabForUrl(string workspaceId, string url)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs` around lines 198 - 204, Update the comment above SwitchApplicationTabForUrl to state that URL navigation for any workspace may switch the desktop UI’s visible workspace before selecting the matching application/sub-tab; remove the outdated restriction that it only acts on the currently active workspace. Do not change the method behavior here.AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs (1)
252-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the new workspace/application synchronization behavior.
SelectApplicationForUrlintroduces a broader runtime behavior: any background navigation on any workspace can now switch the desktop's active workspace and application selection.docs/developer-guide/mcp.mddocuments the stagedbrowser_clicksequence but not this general synchronization contract.Based on learnings, when behavior, architecture, or runtime contracts change, update the matching
AGENTS.mdor canonical documentation source in the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs` around lines 252 - 280, Document the workspace/application synchronization contract introduced by SelectApplicationForUrl in the canonical developer documentation, updating the staged browser_click guidance in mcp.md or its referenced documentation source. Describe that background navigation can select the matching workspace, application, and port tab, and keep the documentation aligned with the runtime behavior.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs`:
- Around line 8-13: The workspace-start flow must be owned by the Server API
rather than duplicating orchestration in the CLI. In
AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs:8-13, preserve the Dotnet
and Docker inputs in the Server configuration contract; expose a Server HTTP
start-by-worktree endpoint that performs configuration loading, Git
identity/display fallback resolution, registration, and startup. In
AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs:31-58,
replace the local resolution and sequencing with a call to that endpoint. Update
AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs:82-103 to assert the Server
response.
In `@AgentUp.Desktop/Features/Browser/Resources/BrowserScripts.cs`:
- Around line 47-69: Update the delayed click callback in AnimatedClick so
e.click() is wrapped with an explicit catch that resolves the Promise with an
error payload when the click throws. Keep cleanup of the animation element in
finally, but only resolve with {ok:true} after a successful click and prevent
exceptions from producing a false-positive success.
- Around line 47-69: Update AnimatedClick to use an explicit backend-independent
completion signal rather than relying on Promise handling, while retaining the
existing success/error payloads. Track every requestAnimationFrame and timer
handle, and cancel them during completion or timeout so e.click() cannot run
late. Add a host-side timeout in BrowserCommandPoller.EvalCommandAsync that
terminates the pending command and reports timeout failure.
In `@AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs`:
- Around line 252-280: Update SelectApplicationForUrl to find the matching
application through the target workspace’s own Applications collection and
validate the target port before assigning Sidebar.SelectedWorkspace; return
false without visible state changes when no match exists. Preserve the existing
selection and tab behavior for valid matches, and add a MainViewModelTests
regression test asserting a non-matching target-workspace port returns false and
leaves Sidebar.SelectedWorkspace unchanged.
---
Outside diff comments:
In
`@AgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.cs`:
- Around line 52-60: Update WorkspaceApplicationViewModel.UpdateFrom to compare
the incoming command with the current Command before assignment and include that
result in the returned change flag, preserving the existing port and state
detection. Add a WorkspaceItemViewModelTests case covering a command-only change
and verify ApplicationsChanged is raised once.
---
Nitpick comments:
In `@AgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.cs`:
- Around line 17-36: Update the remaining delay calls in BrowserCommandPoller,
including the paths around ReadSettledPageStateAsync and the call sites near
lines 58 and 74, to invoke the injected _delay delegate instead of the static
DelayAsync method. Preserve each existing delay duration and cancellation token
argument, matching the already-updated call sites near lines 199, 208, and 217.
In `@AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs`:
- Around line 252-280: Document the workspace/application synchronization
contract introduced by SelectApplicationForUrl in the canonical developer
documentation, updating the staged browser_click guidance in mcp.md or its
referenced documentation source. Describe that background navigation can select
the matching workspace, application, and port tab, and keep the documentation
aligned with the runtime behavior.
In `@AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs`:
- Around line 198-204: Update the comment above SwitchApplicationTabForUrl to
state that URL navigation for any workspace may switch the desktop UI’s visible
workspace before selecting the matching application/sub-tab; remove the outdated
restriction that it only acts on the currently active workspace. Do not change
the method behavior here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: deb2a50e-7568-4bcf-8f52-29724311d53e
📒 Files selected for processing (24)
AGENTS.mdAgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.csAgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.csAgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.csAgentUp.Desktop.Tests/Features/Browser/Controller/BrowserAutomationControllerTests.csAgentUp.Desktop.Tests/Features/Browser/Unit/BrowserCommandPollerTests.csAgentUp.Desktop.Tests/Features/Workspaces/Unit/MainViewModelTests.csAgentUp.Desktop.Tests/Features/Workspaces/Unit/WorkspaceItemViewModelTests.csAgentUp.Desktop/Features/Browser/Models/ClickTargetDto.csAgentUp.Desktop/Features/Browser/Resources/BrowserScripts.csAgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.csAgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.csAgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.csAgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceItemViewModel.csAgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.csAgentUp.Desktop/Shared/Interfaces/IBrowserWindowHost.csAgentUp.Server.Tests/Features/Orchestration/Controller/OrchestrationMcpResourcesTests.csAgentUp.Server.Tests/Features/Orchestration/Controller/OrchestrationMcpToolsTests.csAgentUp.Server/Features/Orchestration/DTOs/AgentUpConfiguration.csAgentUp.Server/Features/Orchestration/Providers/AgentUpContextProvider.csAgentUp.Server/Features/Orchestration/Services/OrchestrationWorkspaceService.csdocs/developer-guide/mcp.mddocs/user-docs/agent-up-json-reference.mddocs/user-docs/agent-up-json.md
Comment on lines
+8
to
+13
| List<DockerCapabilityDefinition>? Docker = null, | ||
| WorkspaceDisplayOptions? Display = null); | ||
|
|
||
| public sealed record WorkspaceDisplayOptions( | ||
| string? Name = null, | ||
| string? Branch = null); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline AgentUp.Server/Features/Orchestration/Services/OrchestrationWorkspaceService.cs --items all
ast-grep outline AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs --items all
rg -n -C 5 --type cs \
'StartAsync\(string worktreePath|RegisterAsync\(|StartWorkspaceAsync\(|HttpPost|MapPost' \
AgentUp.Server AgentUp.CLIRepository: agent-up-oss/agent-up
Length of output: 23118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server orchestration service ---'
sed -n '1,125p' AgentUp.Server/Features/Orchestration/Services/OrchestrationWorkspaceService.cs
printf '%s\n' '--- server orchestration controller ---'
cat -n AgentUp.Server/Features/Orchestration/Controllers/OrchestrationWorkspaceController.cs
printf '%s\n' '--- server workspace HTTP controller ---'
cat -n AgentUp.Server/Features/Workspaces/Controllers/WorkspacesController.cs
printf '%s\n' '--- CLI command service ---'
cat -n AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs
printf '%s\n' '--- CLI JSON DTO ---'
cat -n AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs
printf '%s\n' '--- CLI API client ---'
cat -n AgentUp.CLI/Features/Workspaces/Providers/WorkspaceApiClient.cs
printf '%s\n' '--- relevant route/interface/config references ---'
rg -n -C 3 --type cs \
'OrchestrationWorkspaceController|StartAsync\(string worktreePath|RegisterWorkspaceRequest|AgentUpConfiguration|WorkspaceCommandService|MapControllers|MapPost' \
AgentUp.Server AgentUp.CLI AgentUp.CLI.TestsRepository: agent-up-oss/agent-up
Length of output: 45240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server configuration and registration DTOs ---'
cat -n AgentUp.Server/Features/Orchestration/DTOs/AgentUpConfiguration.cs
cat -n AgentUp.Server/Features/Workspaces/DTOs/RegisterWorkspaceRequest.cs
cat -n AgentUp.CLI/Features/Workspaces/DTOs/RegisterWorkspaceRequest.cs
printf '%s\n' '--- server start continuation ---'
sed -n '120,240p' AgentUp.Server/Features/Orchestration/Services/OrchestrationWorkspaceService.cs
printf '%s\n' '--- CLI configuration provider and identity provider ---'
cat -n AgentUp.CLI/Features/Workspaces/Providers/WorkspaceConfigurationProvider.cs
cat -n AgentUp.CLI/Features/Workspaces/Providers/WorkspaceIdentityProvider.cs
printf '%s\n' '--- server workspace definitions and lifecycle inputs ---'
rg -n -C 4 --type cs \
'class Workspace|record Workspace|Dotnet|Docker|Applications|Services' \
AgentUp.Server/Features/Workspaces AgentUp.Server/Features/Orchestration
printf '%s\n' '--- start-related tests and CLI composition ---'
rg -n -C 8 --type cs \
'Start_UsesDisplayOverrides|StartAsync\(|/api/workspaces|DisplayName|Dotnet|Docker' \
AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs AgentUp.CLI/Composition/CliRunnerFactory.csRepository: agent-up-oss/agent-up
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
server = Path("AgentUp.Server/Features/Orchestration/Services/OrchestrationWorkspaceService.cs").read_text()
cli = Path("AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs").read_text()
http = Path("AgentUp.Server/Features/Workspaces/Controllers/WorkspacesController.cs").read_text()
config = Path("AgentUp.Server/Features/Orchestration/DTOs/AgentUpConfiguration.cs").read_text()
checks = {
"server_start_loads_config": "_configuration.LoadAsync(worktreePath" in server,
"server_start_reads_identity": "_identity.ReadAsync(worktreePath" in server,
"server_start_resolves_display": "config.Display?.Name" in server and "config.Display?.Branch" in server,
"server_start_registers_and_starts": "_workspaces.RegisterAsync" in server and "StartRegisteredAsync" in server,
"cli_loads_config": "_configuration.LoadAsync(_workingDirectory)" in cli,
"cli_reads_identity": "_identity.ReadAsync(_workingDirectory)" in cli,
"cli_resolves_display": "config.Display?.Name" in cli and "config.Display?.Branch" in cli,
"cli_registers_then_starts": "_client.RegisterAsync" in cli and "_client.StartWorkspaceAsync" in cli,
"rest_has_register_route": '[HttpPost]' in http,
"rest_has_id_start_only": '[HttpPost("{id}/start")]' in http,
"rest_has_no_worktree_start_route": 'worktreePath' not in http,
"server_config_has_no_dotnet_or_docker": "Dotnet" not in config and "Docker" not in config,
}
for name, value in checks.items():
print(f"{name}={value}")
assert all(checks.values())
PYRepository: agent-up-oss/agent-up
Length of output: 628
Move workspace-start orchestration behind a Server API boundary.
WorkspaceCommandService.StartAsync duplicates configuration loading, Git identity lookup, display fallback resolution, registration, and start sequencing from OrchestrationWorkspaceService.StartAsync. Expose the Server orchestration through an HTTP start-by-worktree endpoint, extend Server configuration to preserve the CLI’s Dotnet and Docker inputs, then remove the CLI-side resolution and update the E2E test to assert the Server response.
📍 Affects 3 files
AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs#L8-L13(this comment)AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs#L31-L58AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs#L82-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs` around lines 8 - 13, The
workspace-start flow must be owned by the Server API rather than duplicating
orchestration in the CLI. In
AgentUp.CLI/Features/Workspaces/DTOs/AgentUpJson.cs:8-13, preserve the Dotnet
and Docker inputs in the Server configuration contract; expose a Server HTTP
start-by-worktree endpoint that performs configuration loading, Git
identity/display fallback resolution, registration, and startup. In
AgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.cs:31-58,
replace the local resolution and sequencing with a call to that endpoint. Update
AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs:82-103 to assert the Server
response.
Source: Coding guidelines
Comment on lines
+252
to
+280
| internal bool SelectApplicationForUrl(string workspaceId, string url) | ||
| { | ||
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | ||
| return false; | ||
|
|
||
| var workspace = Sidebar.Workspaces.FirstOrDefault(w => w.Id == workspaceId); | ||
| if (workspace is null) | ||
| return false; | ||
|
|
||
| if (Sidebar.SelectedWorkspace?.Id != workspaceId) | ||
| Sidebar.SelectedWorkspace = workspace; | ||
|
|
||
| var targetPort = uri.Port; | ||
| var matchingApp = Applications.Applications | ||
| .FirstOrDefault(a => a.AllocatedPorts.Any(p => p.AllocatedPort == targetPort)); | ||
| if (matchingApp is null) | ||
| return false; | ||
|
|
||
| PreloadPortUrl(url); | ||
|
|
||
| if (Applications.SelectedApplication != matchingApp) | ||
| Applications.SelectedApplication = matchingApp; | ||
|
|
||
| var targetTab = SubTabs.OfType<PortSubTabViewModel>().FirstOrDefault(t => t.AllocatedPort == targetPort); | ||
| if (targetTab is not null && SelectedSubTab != targetTab) | ||
| SelectedSubTab = targetTab; | ||
|
|
||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the target application before switching Sidebar.SelectedWorkspace.
SelectApplicationForUrl switches Sidebar.SelectedWorkspace to workspace before confirming that any application in that workspace owns targetPort. If no application matches, the method returns false, but the workspace switch already happened. Both callers in MainWindow.axaml.cs discard this return value (SwitchApplicationTabForUrl at lines 202-203) or propagate it through a caller that also discards it (BrowserCommandPoller.ClickAsync). A link with a non-matching port silently leaves the desktop UI on the wrong workspace with no indication of failure.
Search the target workspace's own application list (workspace.Applications, which is independent of the current selection) before switching, so the method makes no visible change on failure.
🐛 Proposed fix
internal bool SelectApplicationForUrl(string workspaceId, string url)
{
if (!Uri.TryCreate(url, UriKind.Absolute, out var uri))
return false;
var workspace = Sidebar.Workspaces.FirstOrDefault(w => w.Id == workspaceId);
if (workspace is null)
return false;
- if (Sidebar.SelectedWorkspace?.Id != workspaceId)
- Sidebar.SelectedWorkspace = workspace;
-
var targetPort = uri.Port;
- var matchingApp = Applications.Applications
+ var targetWorkspaceApp = workspace.Applications
.FirstOrDefault(a => a.AllocatedPorts.Any(p => p.AllocatedPort == targetPort));
- if (matchingApp is null)
+ if (targetWorkspaceApp is null)
return false;
+ if (Sidebar.SelectedWorkspace?.Id != workspaceId)
+ Sidebar.SelectedWorkspace = workspace;
+
PreloadPortUrl(url);
+ var matchingApp = Applications.Applications.FirstOrDefault(a => a.Name == targetWorkspaceApp.Name);
+ if (matchingApp is null)
+ return false;
+
if (Applications.SelectedApplication != matchingApp)
Applications.SelectedApplication = matchingApp;Add a regression test in MainViewModelTests.cs covering a URL whose port does not match any application in the target workspace, asserting SelectApplicationForUrl returns false and Sidebar.SelectedWorkspace is unchanged.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| internal bool SelectApplicationForUrl(string workspaceId, string url) | |
| { | |
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | |
| return false; | |
| var workspace = Sidebar.Workspaces.FirstOrDefault(w => w.Id == workspaceId); | |
| if (workspace is null) | |
| return false; | |
| if (Sidebar.SelectedWorkspace?.Id != workspaceId) | |
| Sidebar.SelectedWorkspace = workspace; | |
| var targetPort = uri.Port; | |
| var matchingApp = Applications.Applications | |
| .FirstOrDefault(a => a.AllocatedPorts.Any(p => p.AllocatedPort == targetPort)); | |
| if (matchingApp is null) | |
| return false; | |
| PreloadPortUrl(url); | |
| if (Applications.SelectedApplication != matchingApp) | |
| Applications.SelectedApplication = matchingApp; | |
| var targetTab = SubTabs.OfType<PortSubTabViewModel>().FirstOrDefault(t => t.AllocatedPort == targetPort); | |
| if (targetTab is not null && SelectedSubTab != targetTab) | |
| SelectedSubTab = targetTab; | |
| return true; | |
| } | |
| internal bool SelectApplicationForUrl(string workspaceId, string url) | |
| { | |
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | |
| return false; | |
| var workspace = Sidebar.Workspaces.FirstOrDefault(w => w.Id == workspaceId); | |
| if (workspace is null) | |
| return false; | |
| var targetPort = uri.Port; | |
| var targetWorkspaceApp = workspace.Applications | |
| .FirstOrDefault(a => a.AllocatedPorts.Any(p => p.AllocatedPort == targetPort)); | |
| if (targetWorkspaceApp is null) | |
| return false; | |
| if (Sidebar.SelectedWorkspace?.Id != workspaceId) | |
| Sidebar.SelectedWorkspace = workspace; | |
| PreloadPortUrl(url); | |
| var matchingApp = Applications.Applications.FirstOrDefault(a => a.Name == targetWorkspaceApp.Name); | |
| if (matchingApp is null) | |
| return false; | |
| if (Applications.SelectedApplication != matchingApp) | |
| Applications.SelectedApplication = matchingApp; | |
| var targetTab = SubTabs.OfType<PortSubTabViewModel>().FirstOrDefault(t => t.AllocatedPort == targetPort); | |
| if (targetTab is not null && SelectedSubTab != targetTab) | |
| SelectedSubTab = targetTab; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs` around lines
252 - 280, Update SelectApplicationForUrl to find the matching application
through the target workspace’s own Applications collection and validate the
target port before assigning Sidebar.SelectedWorkspace; return false without
visible state changes when no match exists. Preserve the existing selection and
tab behavior for valid matches, and add a MainViewModelTests regression test
asserting a non-matching target-workspace port returns false and leaves
Sidebar.SelectedWorkspace unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AgentUp.Desktop/Features/Browser/Resources/BrowserScripts.cs`:
- Around line 77-86: Update CompleteClick to retrieve and remove
__agentUpClickRing, then return an error when the target is missing or
e.matches(':disabled') is true, before setting window.__agentUpMouse. Preserve
successful clicks and return ok:true only when e.click() is actionable, and add
regression tests covering both early-failure paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f461cf32-70fa-4d5c-977a-f3f4d1cebaec
📒 Files selected for processing (7)
AgentUp.Desktop.Tests/Features/Browser/Unit/BrowserCommandPollerTests.csAgentUp.Desktop/Features/Browser/Resources/BrowserScripts.csAgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.csDemo/agent1/PROMPT.mdDemo/agent2/PROMPT.mdDemo/agent3/PROMPT.mdDemo/agent4/PROMPT.md
🚧 Files skipped from review as they are similar to previous changes (6)
- Demo/agent2/PROMPT.md
- Demo/agent3/PROMPT.md
- Demo/agent1/PROMPT.md
- Demo/agent4/PROMPT.md
- AgentUp.Desktop/Features/Browser/Services/BrowserCommandPoller.cs
- AgentUp.Desktop.Tests/Features/Browser/Unit/BrowserCommandPollerTests.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs (1)
82-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a whitespace override fallback test.
StartAsynctreats whitespacedisplay.nameanddisplay.branchvalues as absent. This test only covers non-empty overrides. Add a focused test with whitespace values and verify that the configured name and Git branch remain in the registration payload.As per coding guidelines, “Every validation rule affecting public behavior requires a focused test at the boundary where that behavior is observed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs` around lines 82 - 103, Add a focused test alongside Start_UsesDisplayOverrides_ForDesktopVisuals that writes whitespace-only display.name and display.branch values, runs the start command, and verifies the registered workspace retains the configured project name and Git branch while preserving the existing worktree assertion.Source: Coding guidelines
🧹 Nitpick comments (1)
AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs (1)
304-318: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a partial-removal regression test.
WorkspaceListViewModelcurrently uses onlyAddandRemove; no production path callsMoveorSort, so aMoveguard is not needed. Test removing one workspace from several and assert that the remaining workspace retains its WebView resources and cached state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs` around lines 304 - 318, Add a regression test for partial removal in the workspace collection change handling around OnWorkspaceCollectionChanged: create several workspaces, remove only one, and assert the remaining workspace retains its WebView resources and cached state. Keep the test focused on Remove actions; no Move or Sort handling is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AgentUp.CLI/Features/Workspaces/Providers/WorkspaceApiClient.cs`:
- Around line 54-59: Update DeleteWorkspaceAsync to declare the
HttpResponseMessage returned by _http.DeleteAsync with using, ensuring the
response is disposed after success or failure while preserving the existing
status check and problem-detail exception behavior.
In
`@AgentUp.Server.Tests/Features/Processes/Provider/WorkspaceProcessManagerTests.cs`:
- Around line 229-245: The test
CreateWorkspaceDirectoryAlias_RejectsExistingUnverifiedAlias currently covers
only an existing regular directory; update it to create a second directory, make
the deterministic alias path a symbolic link targeting that directory, and
assert that LocalProcessProvider.CreateWorkspaceDirectoryAlias rejects the
mismatched target with the existing verification failure.
---
Outside diff comments:
In `@AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.cs`:
- Around line 82-103: Add a focused test alongside
Start_UsesDisplayOverrides_ForDesktopVisuals that writes whitespace-only
display.name and display.branch values, runs the start command, and verifies the
registered workspace retains the configured project name and Git branch while
preserving the existing worktree assertion.
---
Nitpick comments:
In `@AgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.cs`:
- Around line 304-318: Add a regression test for partial removal in the
workspace collection change handling around OnWorkspaceCollectionChanged: create
several workspaces, remove only one, and assert the remaining workspace retains
its WebView resources and cached state. Keep the test focused on Remove actions;
no Move or Sort handling is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba6969fb-b353-4c7b-81f0-2812fa461906
📒 Files selected for processing (36)
AgentUp.CLI.Tests/E2E/WorkspaceCommandsTests.csAgentUp.CLI/Composition/CliRunnerFactory.csAgentUp.CLI/Features/Workspaces/Controllers/ClearCommand.csAgentUp.CLI/Features/Workspaces/Controllers/CliRunner.csAgentUp.CLI/Features/Workspaces/Providers/WorkspaceApiClient.csAgentUp.CLI/Features/Workspaces/Services/WorkspaceCommandService.csAgentUp.CLI/Properties/launchSettings.jsonAgentUp.Desktop.Tests/Features/Browser/Unit/BrowserScriptsTests.csAgentUp.Desktop.Tests/Features/Workspaces/Headless/SidebarBehaviorTests.csAgentUp.Desktop.Tests/Features/Workspaces/Provider/WorkspaceEventClientTests.csAgentUp.Desktop.Tests/Features/Workspaces/Unit/MainViewModelTests.csAgentUp.Desktop.Tests/Support/AppDriver.csAgentUp.Desktop.Tests/Support/SidebarDriver.csAgentUp.Desktop/Features/Browser/Resources/BrowserScripts.csAgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.csAgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.csAgentUp.Desktop/Features/Workspaces/Views/MainWindow.axamlAgentUp.Desktop/Features/Workspaces/Views/MainWindow.axaml.csAgentUp.Server.Tests/Fake/ServerTestComposition.csAgentUp.Server.Tests/Features/Processes/Provider/WorkspaceProcessManagerTests.csAgentUp.Server.Tests/Features/Workspaces/Unit/WorkspaceRegistryTests.csAgentUp.Server/Features/Processes/Providers/LocalProcessProvider.csAgentUp.Server/Features/Workspaces/Services/WorkspaceRegistry.csDemo/README.mdDemo/agent1/agent-up.jsonDemo/agent1/workspace.mjsDemo/agent2/agent-up.jsonDemo/agent2/workspace.mjsDemo/agent3/agent-up.jsonDemo/agent3/workspace.mjsDemo/agent4/agent-up.jsonDemo/agent4/workspace.mjsdocs/user-docs/agent-up-json-reference.mddocs/user-docs/agent-up-json.mddocs/user-docs/cli.mddocs/user-docs/index.md
🚧 Files skipped from review as they are similar to previous changes (14)
- Demo/agent1/agent-up.json
- AgentUp.Desktop/Features/Workspaces/ViewModels/WorkspaceApplicationViewModel.cs
- Demo/agent4/agent-up.json
- Demo/agent4/workspace.mjs
- Demo/agent3/agent-up.json
- Demo/agent1/workspace.mjs
- Demo/agent2/workspace.mjs
- Demo/README.md
- docs/user-docs/agent-up-json.md
- docs/user-docs/agent-up-json-reference.md
- Demo/agent3/workspace.mjs
- Demo/agent2/agent-up.json
- AgentUp.Desktop/Features/Browser/Resources/BrowserScripts.cs
- AgentUp.Desktop/Features/Workspaces/ViewModels/MainViewModel.cs
|
🎉 This PR is included in version 3.22.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@coderabbitai
Summary by CodeRabbit
New Features
Bug Fixes
Documentation