feat(agent/agentcontainers): auto detect dev containers #18950
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce automatic discovery of devcontainer projects within agent directories, update the API initialization to support this, and expand manifest information handling. Tests are added for the discovery feature. UI adjustments include new Storybook scenarios, conditional button labeling, fallback display logic, debug logging, and refined control of app section visibility based on devcontainer status. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant ContainerAPI as Container API
participant FS as Filesystem
Agent->>ContainerAPI: Init(WithManifestInfo(..., agentDirectory))
ContainerAPI->>FS: On start, discoverDevcontainerProjects()
FS-->>ContainerAPI: List directories, check for .git and devcontainer configs
ContainerAPI->>ContainerAPI: Update knownDevcontainers map
Estimated code review effort4 (~90 minutes) Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
agent/agentcontainers/api.go (3)
393-398: Consider tracking discovery completion state.The discovery runs asynchronously without tracking completion. This could result in incomplete devcontainer lists being returned before discovery finishes.
Consider adding a discovery completion channel or atomic boolean to track when initial discovery is complete, similar to how
initDoneis used.
403-407: Improve error message clarity.The error message is misleading - it describes what was being checked rather than what failed.
Apply this diff to improve the error message:
- isGitProject, err := afero.DirExists(api.fs, filepath.Join(api.agentDirectory, "/.git")) - if err != nil { - return xerrors.Errorf(".git dir exists: %w", err) - } + isGitProject, err := afero.DirExists(api.fs, filepath.Join(api.agentDirectory, "/.git")) + if err != nil { + return xerrors.Errorf("check .git directory: %w", err) + }
429-432: Improve error message clarity.Same issue with misleading error message.
Apply this diff:
- isGitProject, err = afero.DirExists(api.fs, filepath.Join(api.agentDirectory, entry.Name(), ".git")) - if err != nil { - return xerrors.Errorf(".git dir exists: %w", err) - } + isGitProject, err = afero.DirExists(api.fs, filepath.Join(api.agentDirectory, entry.Name(), ".git")) + if err != nil { + return xerrors.Errorf("check .git directory in %s: %w", entry.Name(), err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
agent/agent.go(1 hunks)agent/agentcontainers/api.go(8 hunks)agent/agentcontainers/api_test.go(4 hunks)site/src/modules/resources/AgentDevcontainerCard.stories.tsx(1 hunks)site/src/modules/resources/AgentDevcontainerCard.tsx(3 hunks)site/src/modules/resources/AgentRow.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
site/src/modules/resources/AgentRow.tsx (1)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use functional updates (setX(prev ⇒ …)) whenever next state depends on previous state.
site/src/modules/resources/AgentDevcontainerCard.tsx (4)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Group related Tailwind classes
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Responsive design - use Tailwind's responsive prefixes (sm:, md:, lg:, xl:)
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use className with clsx for conditional styling
Learnt from: CR
PR: coder/coder#0
File: site/CLAUDE.md:0-0
Timestamp: 2025-07-21T14:33:50.919Z
Learning: Applies to site/src/**/*.tsx : Use Tailwind classes for all new styling
agent/agentcontainers/api.go (4)
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*.go : Coder emphasizes clear error handling, with specific patterns required: Concise error messages that avoid phrases like "failed to"; Wrapping errors with %w to maintain error chains; Using sentinel errors with the "err" prefix (e.g., errNotFound).
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : OAuth2-compliant error responses must use writeOAuth2Error in Go code
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : Follow Uber Go Style Guide
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
agent/agentcontainers/api_test.go (6)
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Never use hardcoded names in concurrent Go tests
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*_test.go : Use unique identifiers in concurrent Go tests to prevent race conditions (e.g., fmt.Sprintf with t.Name() and time.Now().UnixNano())
Learnt from: CR
PR: coder/coder#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:32:56.474Z
Learning: Applies to **/*.go : Follow Uber Go Style Guide
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.034Z
Learning: Applies to coderd/coderd.go : The REST API is defined in coderd/coderd.go and uses Chi for HTTP routing.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests must use t.Parallel() to run concurrently, which improves test suite performance and helps identify race conditions.
Learnt from: CR
PR: coder/coder#0
File: .cursorrules:0-0
Timestamp: 2025-07-21T14:32:43.035Z
Learning: Applies to **/*_test.go : All tests should run in parallel using t.Parallel() to ensure efficient testing and expose potential race conditions.
🪛 GitHub Check: lint
agent/agentcontainers/api.go
[failure] 270-270:
import-shadowing: The name 'fs' shadows an import name (revive)
[failure] 452-452:
unused-parameter: parameter 'info' seems to be unused, consider removing or renaming it as _ (revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test-e2e
- GitHub Check: chromatic
- GitHub Check: test-go-pg (windows-2022)
- GitHub Check: test-go-pg (macos-latest)
- GitHub Check: test-go-race-pg
- GitHub Check: test-go-pg-17
- GitHub Check: test-go-pg (ubuntu-latest)
- GitHub Check: gen
🔇 Additional comments (11)
site/src/modules/resources/AgentDevcontainerCard.stories.tsx (1)
94-103: LGTM! Good addition for edge case testing.The new Storybook story appropriately tests the UI behavior when both container and agent are undefined, which is valuable for comprehensive component testing.
site/src/modules/resources/AgentDevcontainerCard.tsx (2)
223-224: Good fallback logic for devcontainer display name.The updated logic properly falls back to
config_pathwhen bothsubAgent?.nameanddevcontainer.nameare unavailable, providing better UX for auto-detected devcontainers.
259-259: Excellent conditional button labeling.The conditional button text ("Start" vs "Rebuild") based on container presence provides clear user intent and aligns well with the devcontainer lifecycle.
agent/agent.go (1)
1171-1171: Appropriate extension of manifest information.Adding
manifest.Directoryto theWithManifestInfocall properly supports the devcontainer auto-detection feature by providing the necessary filesystem context for discovery.site/src/modules/resources/AgentRow.tsx (1)
140-151: Excellent refinement of app visibility logic.The updated condition properly narrows when to hide parent apps - only when devcontainers are actively "running" or "starting". This provides better UX by showing parent apps when all devcontainers are stopped, while still reducing clutter when devcontainers are active. The inline comments clearly explain the reasoning.
agent/agentcontainers/api_test.go (3)
23-23: LGTM: Appropriate import for filesystem testingThe afero package import is necessary for the new
TestDevcontainerDiscoverytest function which uses in-memory filesystem operations.
1682-1682: LGTM: Consistent updates to WithManifestInfo callsAll three calls to
WithManifestInfohave been correctly updated with the new agent directory parameter ("/parent-agent"), aligning with the updated function signature.Also applies to: 1666-1666, 3367-3367
3194-3423: Well-structured test for devcontainer discovery functionalityThe test comprehensively validates the automatic devcontainer discovery feature with:
- Table-driven test cases covering various directory structures
- Proper use of
t.Parallel()as per codebase conventions- In-memory filesystem for isolated testing
- Deterministic result sorting for reliable assertions
- Appropriate use of
Eventuallypattern for async operationsThe test effectively exercises the new discovery logic that scans agent directories for Git repositories containing devcontainer configurations.
agent/agentcontainers/api.go (3)
346-348: LGTM!Good defensive initialization ensuring the filesystem is always set.
462-478: No issues found with handling incomplete devcontainer entries in the update loop.The
processUpdatedContainersLockedmethod explicitly resets and then populates each devcontainer’sNameandStatus—defaulting to “Stopped” (or “Running” when appropriate) and generating a valid name based on the workspace folder or container friendly name when a container appears. This ensures there’s no race on uninitialized entries.
199-206: AllWithManifestInfoCallers UpdatedAll usages of
WithManifestInfoinagent/agent.goandagent/agentcontainers/api_test.goinclude the newagentDirectoryargument. No further changes are required.
Project discovery doesn't work well with these tests do not set everything up requried for this feature to work.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Tests