- Notifications
You must be signed in to change notification settings - Fork948
feat(agent/agentcontainers): auto detect dev containers#18950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
coderabbitaibot commentedJul 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
initDone
is 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 incoderd/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 uset.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 usingt.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_path
when bothsubAgent?.name
anddevcontainer.name
are 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.Directory
to theWithManifestInfo
call 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
TestDevcontainerDiscovery
test function which uses in-memory filesystem operations.
1682-1682
:LGTM: Consistent updates to WithManifestInfo callsAll three calls to
WithManifestInfo
have 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
Eventually
pattern 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
processUpdatedContainersLocked
method explicitly resets and then populates each devcontainer’sName
andStatus
—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
:AllWithManifestInfo
Callers UpdatedAll usages of
WithManifestInfo
inagent/agent.go
andagent/agentcontainers/api_test.go
include the newagentDirectory
argument. No further changes are required.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements
Tests