Repository navigation
Implement managed emulator functionality and enhance profile management - #5
Conversation
- Added support for managed Docker emulator in the application, including a new EmulatorManager for handling emulator lifecycle. - Updated the App struct to include an emulator manager and methods for starting, stopping, and checking the status of the emulator. - Enhanced ConnectionProfile to support emulator modes (off, external, managed) and added validation for managed emulator settings. - Improved ProfileDialog to allow configuration of emulator settings, including port, auto-start, and auto-stop options. - Refactored connection methods to integrate emulator management, ensuring proper handling during connection and disconnection processes. - Added tests for emulator manager functionality and connection profile validation.
📝 WalkthroughWalkthroughAdds a Docker-backed managed Pub/Sub emulator subsystem, tri-state emulator config (off/external/managed) across backend and frontend, lifecycle APIs to start/stop/check emulators, per-profile emulator status tracking, and integrates emulator readiness into connection and resource initialization flows. Changes
Sequence DiagramsequenceDiagram
participant User as Frontend User
participant Frontend as Frontend
participant App as App (Backend)
participant Manager as Emulator Manager
participant Docker as Docker Daemon
participant Emulator as Pub/Sub Emulator
participant Handler as Connection Handler
User->>Frontend: Connect using profile (mode=managed)
Frontend->>App: connectWithProfile(profileID)
App->>Manager: Start(profileID, managedConfig)
Manager->>Docker: Check Docker availability
Docker-->>Manager: Docker ready
Manager->>Docker: Run container (image, port, bind)
Docker->>Emulator: Launch emulator
Manager->>Manager: waitForEmulator (TCP probe)
Emulator-->>Manager: Port reachable (ready)
Manager-->>App: Start returns (running)
App->>Handler: Establish Pub/Sub connection using effective host
Handler->>Emulator: Open connection to host:port
Emulator-->>Handler: Connection established
User->>Frontend: Disconnect
Frontend->>App: disconnect
App->>Manager: Stop(profileID) (if autoStop)
Manager->>Docker: Stop & remove container
Docker->>Emulator: Shutdown
Manager-->>App: Stop returns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR implements managed Docker emulator functionality, allowing users to automatically start/stop Pub/Sub emulators per connection profile. The implementation follows the existing emulator guide patterns and extends the connection profile model with three emulator modes: off, external, and managed. Key Changes
TestingComprehensive test coverage includes:
Architecture AlignmentThe implementation adheres to the emulator guide (
No breaking changes for existing users - backward compatibility maintained through migration logic. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ProfileDialog
participant App
participant EmulatorManager
participant Docker
participant ConnectionHandler
participant GCPClient
User->>ProfileDialog: Configure managed emulator mode
ProfileDialog->>ProfileDialog: Set port, autoStart, autoStop
User->>ProfileDialog: Save profile
ProfileDialog->>App: SaveProfile(profile)
User->>App: SwitchProfile(profileID)
App->>App: connectWithProfile(profile)
alt Managed Emulator Mode
App->>EmulatorManager: CheckDocker()
EmulatorManager->>Docker: docker info
Docker-->>EmulatorManager: OK
alt AutoStart enabled
App->>EmulatorManager: Start(profileID, config)
EmulatorManager->>EmulatorManager: Check existing container
alt Container exists and running
EmulatorManager-->>App: Reuse existing
else New container needed
EmulatorManager->>EmulatorManager: Check port available
EmulatorManager->>Docker: docker run pubsub-gui-emulator-{profileID}
Docker-->>EmulatorManager: Container started
EmulatorManager->>EmulatorManager: Wait for emulator ready (TCP check)
EmulatorManager-->>App: Emulator running
end
App->>App: Wait for emulator ready
end
App->>App: Get effective emulator host
App->>ConnectionHandler: ConnectWithADC(projectID, emulatorHost)
else External Emulator Mode
App->>App: Get emulator host from profile
App->>ConnectionHandler: ConnectWithADC(projectID, emulatorHost)
else Emulator Off
App->>ConnectionHandler: ConnectWithADC(projectID, "")
end
ConnectionHandler->>GCPClient: NewClient(ctx, projectID, endpoint)
GCPClient-->>ConnectionHandler: Connected
ConnectionHandler-->>App: Connection established
App-->>User: Connected
User->>App: Disconnect()
App->>App: stopManagedEmulatorIfNeeded()
alt Managed mode with AutoStop
App->>EmulatorManager: Stop(profileID)
EmulatorManager->>EmulatorManager: Cancel context
EmulatorManager->>Docker: docker stop pubsub-gui-emulator-{profileID}
Docker-->>EmulatorManager: Stopped
EmulatorManager-->>App: Emulator stopped
end
App->>ConnectionHandler: ClearEmulatorHost()
App->>GCPClient: Close()
App-->>User: Disconnected
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Settings/ProfileDialog.tsx (1)
48-77: Reset the advanced toggle when loading a new profile.
showAdvancedpersists across profile changes, so a newly opened dialog can unexpectedly show advanced fields.🐛 Suggested fix
useEffect(() => { if (profile) { setFormData({ name: profile.name, projectId: profile.projectId, authMethod: profile.authMethod, serviceAccountPath: profile.serviceAccountPath || '', oauthClientPath: profile.oauthClientPath || '', emulatorHost: profile.emulatorHost || 'localhost:8085', isDefault: profile.isDefault, }); setEmulatorMode(getEffectiveEmulatorMode(profile)); setManagedConfig(profile.managedEmulator || { ...defaultManagedConfig }); } else { setFormData({ name: '', projectId: '', authMethod: 'ADC', serviceAccountPath: '', oauthClientPath: '', emulatorHost: 'localhost:8085', isDefault: false, }); setEmulatorMode('off'); setManagedConfig({ ...defaultManagedConfig }); } + setShowAdvanced(false); setError(''); }, [profile]);
🤖 Fix all issues with AI agents
In `@internal/emulator/manager.go`:
- Around line 85-121: The Start method currently dereferences config without
checking for nil, which can panic; in Manager.Start add an initial nil check for
the config parameter (before reading config.Port, config.Image,
config.BindAddress) and return a clear error (or default to a safe config) if
config is nil; update any uses of info (EmulatorInfo) and
containerName(profileID) initialization to occur after the nil check so you
never dereference config when it is nil.
- Around line 125-138: The code currently reuses any running container in
Start() after isContainerRunning(info.ContainerName) returns true without
validating the container's current configuration; change Start() to inspect the
running container's actual configuration (image, exposed/host ports, bind
addresses) for info.ContainerName (via the Docker client or existing inspect
helper), compare those values to the requested values stored in
m.emulators[profileID] (e.g., info.Image, info.Port, info.BindAddr), and if any
mismatch is found stop/remove and recreate the container with the new config (or
restart with corrected settings) instead of immediately marking info.Status =
StatusRunning; ensure error handling updates logs and m.emulators[profileID]
status consistently.
- Around line 412-424: The isContainerRunning function currently swallows all
errors from exec.CommandContext and returns (false, nil); instead, capture the
error from cmd.Output(), and if it's an *exec.ExitError inspect its Stderr for
the "No such" / "No such object" text (the expected "container not found" case)
and return (false, nil) only then; for any other error (non-ExitError, or
ExitError whose stderr does not indicate missing container, or context deadline
errors) return (false, err) so callers can handle real Docker/permission/daemon
failures. Reference symbols: isContainerRunning, exec.CommandContext,
cmd.Output, *exec.ExitError, Stderr.
In `@internal/models/connection_test.go`:
- Around line 1-6: The file has formatting issues; run the Go formatter and
commit the changes: execute `go fmt ./...` (or `gofmt -w
internal/models/connection_test.go`) to reformat the package models and update
connection_test.go so it matches gofmt style rules; then stage and push the
formatted file.
In `@internal/models/connection.go`:
- Around line 116-120: The current validation in the managed emulator block
falsely rejects port 0 even though GetEffectiveEmulatorHost treats 0 as the
default (8085); update the check in the validation for cp.ManagedEmulator.Port
inside the EmulatorModeManaged branch to accept 0 or any value between 1 and
65535 (i.e., allow port == 0 || (port >= 1 && port <= 65535)), keeping the same
error message for other out-of-range values; locate the validation near the
EmulatorModeManaged / ManagedEmulator checks in connection.go and adjust the
conditional accordingly.
- Around line 133-140: GetEffectiveEmulatorMode currently treats any non-empty
EmulatorHost (including whitespace-only) as external; update the method on
ConnectionProfile to trim whitespace from cp.EmulatorHost (use
strings.TrimSpace) before testing it and only return EmulatorModeExternal when
the trimmed host is non-empty, keeping the existing cp.EmulatorMode check
intact; reference the GetEffectiveEmulatorMode method, the EmulatorHost field,
and the EmulatorModeExternal constant when making the change.
🧹 Nitpick comments (2)
app.go (1)
1096-1107: Surface errors fromGetEmulatorStatus.
Returning(EmulatorStatus, error)lets callers handle empty/invalid profile IDs and keeps the App API consistent.♻️ Suggested adjustment
-func (a *App) GetEmulatorStatus(profileID string) EmulatorStatus { - info := a.emulatorManager.GetStatus(profileID) - return EmulatorStatus{ - ProfileID: info.ProfileID, - ContainerName: info.ContainerName, - Host: info.Host, - Port: info.Port, - Status: string(info.Status), - Error: info.Error, - } -} +func (a *App) GetEmulatorStatus(profileID string) (EmulatorStatus, error) { + if profileID == "" { + return EmulatorStatus{}, fmt.Errorf("profile ID cannot be empty") + } + info := a.emulatorManager.GetStatus(profileID) + return EmulatorStatus{ + ProfileID: info.ProfileID, + ContainerName: info.ContainerName, + Host: info.Host, + Port: info.Port, + Status: string(info.Status), + Error: info.Error, + }, nil +}As per coding guidelines, App methods exposed to the frontend should return errors.
internal/models/connection.go (1)
144-187: Preferstrconv.Itoaover a customitoa.
The helper is non-idiomatic and adds extra maintenance without benefit.♻️ Proposed refactor
import ( "errors" "strings" "time" + "strconv" ) @@ - return bindAddr + ":" + itoa(port) + return bindAddr + ":" + strconv.Itoa(port) @@ -// itoa converts int to string (simple helper to avoid importing strconv) -func itoa(i int) string { - if i == 0 { - return "0" - } - if i < 0 { - return "-" + itoa(-i) - } - var digits []byte - for i > 0 { - digits = append([]byte{byte('0' + i%10)}, digits...) - i /= 10 - } - return string(digits) -}
- Extract helper functions to reduce Start method complexity (95 -> 50 lines) - resolveConfig: applies defaults to emulator configuration - buildDockerArgs: builds docker run command arguments - tryReuseContainer: checks if existing container can be reused - parsePortMapping: parses Docker port mapping output - normalizeBindAddr: normalizes addresses with defaults - Add container configuration validation before reusing existing containers - Validates image, port, and bind address match requested config - Stops and recreates container if configuration mismatches - Improve error handling in isContainerRunning - Distinguish between "container not found" and actual errors - Properly handle context deadline exceeded errors - Fix port validation to allow port 0 (defaults to 8085) - Fix whitespace handling in emulator host migration logic - Add comprehensive tests for all new helper functions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/emulator/manager.go`:
- Around line 336-374: The Stop method currently ignores errors from
isContainerRunning which can hide Docker/unreachable errors and incorrectly mark
an emulator stopped; update Stop to check the error returned by
isContainerRunning(containerName) after the sleep and if err != nil log the
error (using logger.Error) and return the error instead of proceeding to set
StatusStopped; only set info.Status = StatusStopped and delete
m.cancels[profileID] after confirming isContainerRunning returned no error and
the container is not running (or after a successful force stop via
m.stopContainer), keeping references to the Stop function, isContainerRunning,
containerName, m.stopContainer, and logger to locate the change.
…Dialog to use nullish coalescing operator (`??`) for default values. This change improves handling of falsy values and ensures safer defaults across the component's state management.
- Added error handling when checking if the container is running, logging the error if the check fails. - Enhanced comments for clarity on the container stopping process.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Settings/ProfileDialog.tsx (1)
48-78: Reset “Show advanced” when switching profiles.
showAdvancedpersists across profile changes, so advanced settings can remain open when opening a different profile.🩹 Proposed fix
useEffect(() => { if (profile) { setFormData({ name: profile.name, projectId: profile.projectId, authMethod: profile.authMethod, serviceAccountPath: profile.serviceAccountPath ?? '', oauthClientPath: profile.oauthClientPath ?? '', emulatorHost: profile.emulatorHost ?? 'localhost:8085', isDefault: profile.isDefault, }); setEmulatorMode(getEffectiveEmulatorMode(profile)); setManagedConfig(profile.managedEmulator ?? { ...defaultManagedConfig }); } else { setFormData({ name: '', projectId: '', authMethod: 'ADC', serviceAccountPath: '', oauthClientPath: '', emulatorHost: 'localhost:8085', isDefault: false, }); setEmulatorMode('off'); setManagedConfig({ ...defaultManagedConfig }); } + setShowAdvanced(false); setError(''); }, [profile]);
🤖 Fix all issues with AI agents
In @.cursor/rules/react-tailwind.mdc:
- Around line 113-182: Remove the duplicate "TypeScript Best Practices" section
titled "Nullish Coalescing (`??`) vs Logical OR (`||`)" so only one copy
remains; locate the repeated header and its entire explanatory block (the
examples and anti-patterns under that heading) and delete the redundant
occurrence, leaving the first (or most complete) instance intact, then scan the
rest of the document to verify no other identical section remains.
🧹 Nitpick comments (2)
frontend/src/components/Settings/ProfileDialog.tsx (2)
45-47: Merge managed config with defaults to avoid partial data.If a stored profile has a partial
managedEmulator, missing fields propagate into the payload. Consider merging with defaults on load.♻️ Proposed fix
- const [managedConfig, setManagedConfig] = useState<ManagedEmulatorConfig>( - profile?.managedEmulator ?? { ...defaultManagedConfig } - ); + const [managedConfig, setManagedConfig] = useState<ManagedEmulatorConfig>( + { ...defaultManagedConfig, ...(profile?.managedEmulator ?? {}) } + );- setManagedConfig(profile.managedEmulator ?? { ...defaultManagedConfig }); + setManagedConfig({ ...defaultManagedConfig, ...(profile.managedEmulator ?? {}) });Also applies to: 64-65
462-470: Optionally disable Save when managed port is invalid.Keeps the button state aligned with validation, reducing round-trip errors.
♻️ Proposed tweak
disabled={ saving || !formData.name.trim() || !formData.projectId.trim() || (formData.authMethod === 'ServiceAccount' && !formData.serviceAccountPath.trim()) || (formData.authMethod === 'OAuth' && !formData.oauthClientPath.trim()) || - (emulatorMode === 'external' && !formData.emulatorHost.trim()) + (emulatorMode === 'external' && !formData.emulatorHost.trim()) || + (emulatorMode === 'managed' && (managedConfig.port < 1 || managedConfig.port > 65535)) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/emulator/manager.go`:
- Around line 369-378: The code unconditionally sets info.Status = StatusStopped
after calling m.stopContainer(containerName) which swallows errors; change
m.stopContainer to return an error (or capture its error) and, after calling it,
verify the container is stopped (e.g., via an inspect/IsRunning helper) or at
minimum check the returned error; only set info.Status = StatusStopped and
delete m.cancels[profileID] when the stop/verification succeeds, otherwise log a
warning/error with containerName and keep the status unchanged (ensure you
acquire m.mu around status changes and cancels updates); reference
m.stopContainer, info.Status, StatusStopped, m.cancels, containerName, and
logger in the fix.
🧹 Nitpick comments (4)
internal/emulator/manager.go (4)
219-221: Consider moving the sleep into thewaitForEmulatorgoroutine.The 500ms sleep at line 220 blocks the caller. If
Startis invoked from the UI/main thread, this introduces noticeable latency. Moving the initial delay intowaitForEmulatorwould makeStartreturn immediately while the emulator continues starting in the background.♻️ Suggested change
go m.runContainer(ctx, profileID, args) - time.Sleep(500 * time.Millisecond) - go m.waitForEmulator(ctx, profileID, fmt.Sprintf("127.0.0.1:%d", cfg.Port)) + go func() { + time.Sleep(500 * time.Millisecond) + m.waitForEmulator(ctx, profileID, fmt.Sprintf("127.0.0.1:%d", cfg.Port)) + }()
249-273: Consider checkingscanner.Err()after the loop.If the emulator produces an unusually long log line (>64KB default buffer),
scanner.Scan()returnsfalsewith an error. Currently this is silently ignored. While unlikely for emulator logs, checking the error improves robustness.♻️ Suggested change (stdout example)
go func() { scanner := bufio.NewScanner(stdout) for scanner.Scan() { line := scanner.Text() logger.Info(line, "source", "emulator", "profileId", profileID, "stream", "stdout", ) } + if err := scanner.Err(); err != nil { + logger.Warn("Error reading emulator stdout", "profileId", profileID, "error", err) + } }()
523-523: Misleading use ofnormalizeBindAddrfor image normalization.
normalizeBindAddris semantically named for bind addresses but is used here for image name defaulting. Consider renaming to a genericdefaultIfEmptyor creating a separate helper for clarity.♻️ Suggested refactor
+// defaultIfEmpty returns defaultVal if val is empty, otherwise returns val. +func defaultIfEmpty(val, defaultVal string) string { + if val == "" { + return defaultVal + } + return val +} // In validateContainerConfig: - normalizedExpectedImage := normalizeBindAddr(expectedImage, "google/cloud-sdk:emulators") + normalizedExpectedImage := defaultIfEmpty(expectedImage, "google/cloud-sdk:emulators")
556-567: Consider documenting whycontext.Background()is used instead ofm.ctx.Unlike other methods that use
m.ctx,stopContainerusescontext.Background(). This is likely intentional (to ensure cleanup during app shutdown whenm.ctxis cancelled), but a brief comment would clarify this design decision.📝 Suggested documentation
// stopContainer stops a container func (m *Manager) stopContainer(name string) { + // Use context.Background() instead of m.ctx to ensure cleanup + // completes even during app shutdown when m.ctx may be cancelled ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
| // Force stop if still running | ||
| if running { | ||
| logger.Info("Force stopping container", "container", containerName) | ||
| m.stopContainer(containerName) | ||
| } | ||
|
|
||
| m.mu.Lock() | ||
| info.Status = StatusStopped | ||
| delete(m.cancels, profileID) | ||
| m.mu.Unlock() |
There was a problem hiding this comment.
Container may remain running if stopContainer fails silently.
stopContainer (line 372) ignores all errors, but line 376 unconditionally sets status to StatusStopped. If Docker fails to stop the container, the UI will report it as stopped while it continues running. Consider verifying the container stopped or at least logging warnings.
🛠️ Suggested improvement
// Force stop if still running
if running {
logger.Info("Force stopping container", "container", containerName)
m.stopContainer(containerName)
+ // Verify container actually stopped
+ stillRunning, _ := m.isContainerRunning(containerName)
+ if stillRunning {
+ logger.Warn("Container may still be running after stop attempt", "container", containerName)
+ }
}📝 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.
| // Force stop if still running | |
| if running { | |
| logger.Info("Force stopping container", "container", containerName) | |
| m.stopContainer(containerName) | |
| } | |
| m.mu.Lock() | |
| info.Status = StatusStopped | |
| delete(m.cancels, profileID) | |
| m.mu.Unlock() | |
| // Force stop if still running | |
| if running { | |
| logger.Info("Force stopping container", "container", containerName) | |
| m.stopContainer(containerName) | |
| // Verify container actually stopped | |
| stillRunning, _ := m.isContainerRunning(containerName) | |
| if stillRunning { | |
| logger.Warn("Container may still be running after stop attempt", "container", containerName) | |
| } | |
| } | |
| m.mu.Lock() | |
| info.Status = StatusStopped | |
| delete(m.cancels, profileID) | |
| m.mu.Unlock() |
🤖 Prompt for AI Agents
In `@internal/emulator/manager.go` around lines 369 - 378, The code
unconditionally sets info.Status = StatusStopped after calling
m.stopContainer(containerName) which swallows errors; change m.stopContainer to
return an error (or capture its error) and, after calling it, verify the
container is stopped (e.g., via an inspect/IsRunning helper) or at minimum check
the returned error; only set info.Status = StatusStopped and delete
m.cancels[profileID] when the stop/verification succeeds, otherwise log a
warning/error with containerName and keep the status unchanged (ensure you
acquire m.mu around status changes and cancels updates); reference
m.stopContainer, info.Status, StatusStopped, m.cancels, containerName, and
logger in the fix.
1 participant
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.