release: v0.1.0-alpha.28 #397
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds community docs, centralizes Caddy client (GetCaddyClient), injects correlation IDs into deployment task payloads, adds server-side GitHub repository pagination and repository-by-ID fetch, and introduces an XTerm-based Container Terminal UI and tab plus minor UI fixes and metadata timestamp update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant API as Deploy Tasks
participant GetClient as GetCaddyClient()
participant AppCfg as AppConfig
participant Caddy as Caddy API
participant Queue as Task Queue
User->>API: trigger create/update/redeploy/restart/rollback
API->>GetClient: GetCaddyClient()
GetClient->>AppCfg: read Proxy.CaddyEndpoint
AppCfg-->>GetClient: endpoint
GetClient-->>API: caddygo.Client (singleton)
API->>Queue: prepare TaskPayload (includes CorrelationID)
Queue-->>API: enqueue ack
API->>Caddy: configure/reload/delete domain (via client)
Caddy-->>API: response
API-->>User: result
sequenceDiagram
autonumber
participant Browser
participant TerminalComp as Terminal (XTerm)
participant WS as Backend WS
participant Container
Browser->>TerminalComp: mount(containerId)
TerminalComp->>WS: open session (sessionId)
WS-->>TerminalComp: ready
TerminalComp->>WS: send init attach command (docker exec -it ...)
WS->>Container: exec & attach
Container-->>WS: IO stream
WS-->>TerminalComp: forward IO
TerminalComp-->>Browser: render IO in XTerm
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
CONTRIBUTING.md (1)
8-10: Fix MD019: remove extra space in H2.There's a double space after the hashes.
Apply this patch:
-## Contribution Guide +## Contribution GuideCODE_OF_CONDUCT.md (1)
12-12: Add a concrete reporting channel.Consider adding a specific email (e.g., security@/conduct@) or a link to a reporting form to make reporting actionable.
I can draft the line once you confirm the preferred contact.
api/internal/features/deploy/tasks/init.go (2)
31-41: Use constants for queue/task names.These identifiers are immutable; make them
constto prevent accidental mutation and allow compile-time use.-var ( - TASK_CREATE_DEPLOYMENT = "task_create_deployment" - QUEUE_CREATE_DEPLOYMENT = "create-deployment" - QUEUE_UPDATE_DEPLOYMENT = "update-deployment" - TASK_UPDATE_DEPLOYMENT = "task_update_deployment" - QUEUE_REDEPLOYMENT = "redeploy-deployment" - TASK_REDEPLOYMENT = "task_redeploy_deployment" - QUEUE_ROLLBACK = "rollback-deployment" - TASK_ROLLBACK = "task_rollback_deployment" - QUEUE_RESTART = "restart-deployment" - TASK_RESTART = "task_restart_deployment" -) +const ( + TASK_CREATE_DEPLOYMENT = "task_create_deployment" + QUEUE_CREATE_DEPLOYMENT = "create-deployment" + QUEUE_UPDATE_DEPLOYMENT = "update-deployment" + TASK_UPDATE_DEPLOYMENT = "task_update_deployment" + QUEUE_REDEPLOYMENT = "redeploy-deployment" + TASK_REDEPLOYMENT = "task_redeploy_deployment" + QUEUE_ROLLBACK = "rollback-deployment" + TASK_ROLLBACK = "task_rollback_deployment" + QUEUE_RESTART = "restart-deployment" + TASK_RESTART = "task_restart_deployment" +)
88-99: Prefer structured logging over fmt.Println in task handlers.Use the existing logger for consistency and observability.
Example change per handler:
- fmt.Println("Updating deployment") + // import "github.com/raghavyuva/nixopus-api/internal/features/logger" + t.Logger.Log(logger.Info, "Updating deployment")Repeat for “Redeploying application”, “Rolling back deployment”, and “Restarting deployment”.
Also applies to: 112-122, 136-146, 160-170
api/internal/features/deploy/tasks/delete.go (1)
24-59: ValidateMOUNT_PATHbefore use
api/internal/features/deploy/tasks/delete.go:52 usesos.Getenv("MOUNT_PATH")directly—if unset,repoPathresolves to a relative path. Add validation or a fallback (e.g. uses.Config.Deployment.MountPathwith a default) to prevent unexpected directory paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(1 hunks)api/api/versions.json(1 hunks)api/internal/features/deploy/tasks/create.go(3 hunks)api/internal/features/deploy/tasks/delete.go(1 hunks)api/internal/features/deploy/tasks/init.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
api/internal/features/deploy/tasks/create.go (4)
api/internal/features/deploy/tasks/init.go (3)
GetCaddyClient(46-51)ReDeployQueue(23-23)TaskReDeploy(24-24)api/internal/features/deploy/tasks/context.go (2)
ContextTask(12-18)ContextConfig(25-28)api/internal/features/deploy/tasks/types.go (1)
TaskService(11-17)api/internal/types/task.go (1)
TaskPayload(3-8)
api/internal/features/deploy/tasks/delete.go (1)
api/internal/features/deploy/tasks/init.go (1)
GetCaddyClient(46-51)
api/internal/features/deploy/tasks/init.go (2)
api/internal/types/task.go (1)
TaskPayload(3-8)api/internal/queue/init.go (1)
RegisterQueue(24-29)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
8-8: Multiple spaces after hash on atx style heading
(MD019, no-multiple-space-atx)
🔇 Additional comments (7)
api/api/versions.json (1)
6-6: LGTM on metadata bump.RFC3339 format looks correct; no further action from my side.
CONTRIBUTING.md (1)
1-12: Helpful addition.Clear, concise, and links out to the full guide. Good to ship.
CODE_OF_CONDUCT.md (1)
1-14: Good starter CoC.Reads well and links to the full policy.
api/internal/features/deploy/tasks/create.go (1)
76-89: Skip Caddy setup when domain is empty and handle Reload errors
- Return early if
TaskPayload.Application.Domainis empty to skip reverse-proxy configuration- Check
client.Reload()’s return value and update status/return on errorPlease verify the signature of
(*caddygo.Client).Reload()and adjust error handling accordingly.api/internal/features/deploy/tasks/delete.go (3)
60-66: Centralized Caddy client: good change.Removes the localhost coupling and uses configured endpoint. 👍
60-66: Guard empty domain and surface reload errors (verify Reload signature)client := GetCaddyClient() -if domain == "" { - // no-op -} else { - if err := client.DeleteDomain(domain); err != nil { - s.Logger.Log(logger.Error, "Failed to remove domain", err.Error()) - } - client.Reload() -} +if domain == "" { + s.Logger.Log(logger.Info, "No domain configured; skipping domain removal") +} else { + if err := client.DeleteDomain(domain); err != nil { + s.Logger.Log(logger.Error, "Failed to remove domain", err.Error()) + } + // If Reload() returns an error, surface/log it. + if err := client.Reload(); err != nil { + s.Logger.Log(logger.Error, "Failed to reload Caddy", err.Error()) + } +}Confirm whether
client.Reload()in thecaddygo.ClientAPI returns an error; if it does not, remove theif err :=wrapper aroundReload().
1-68: No remaining direct caddygo.NewClient calls
Verified via search: the only call tocaddygo.NewClientis withinGetCaddyClientatapi/internal/features/deploy/tasks/init.go:48. All other code usesGetCaddyClient.
| application, err := t.Storage.GetApplicationById(request.ID.String(), organizationID) | ||
| if err != nil { | ||
| return shared_types.Application{}, err | ||
| } | ||
|
|
||
| contextTask := ContextTask{ | ||
| TaskService: t, | ||
| ContextConfig: request, | ||
| UserId: userID, | ||
| OrganizationId: organizationID, | ||
| Application: &application, | ||
| } | ||
|
|
||
| TaskPayload, err := contextTask.PrepareReDeploymentContext() | ||
| if err != nil { | ||
| return shared_types.Application{}, err | ||
| } | ||
|
|
||
| err = ReDeployQueue.Add(TaskReDeploy.WithArgs(context.Background(), TaskPayload)) | ||
| if err != nil { | ||
| fmt.Printf("error enqueuing redeploy: %v\n", err) | ||
| } | ||
|
|
||
| return application, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bubble up enqueue failures for redeploy.
Currently logs/prints but returns success, which can mislead callers when the job isn’t queued.
-err = ReDeployQueue.Add(TaskReDeploy.WithArgs(context.Background(), TaskPayload))
-if err != nil {
- fmt.Printf("error enqueuing redeploy: %v\n", err)
-}
-
-return application, nil
+if err := ReDeployQueue.Add(TaskReDeploy.WithArgs(context.Background(), TaskPayload)); err != nil {
+ return application, fmt.Errorf("enqueue redeploy: %w", err)
+}
+return application, nil📝 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.
| application, err := t.Storage.GetApplicationById(request.ID.String(), organizationID) | |
| if err != nil { | |
| return shared_types.Application{}, err | |
| } | |
| contextTask := ContextTask{ | |
| TaskService: t, | |
| ContextConfig: request, | |
| UserId: userID, | |
| OrganizationId: organizationID, | |
| Application: &application, | |
| } | |
| TaskPayload, err := contextTask.PrepareReDeploymentContext() | |
| if err != nil { | |
| return shared_types.Application{}, err | |
| } | |
| err = ReDeployQueue.Add(TaskReDeploy.WithArgs(context.Background(), TaskPayload)) | |
| if err != nil { | |
| fmt.Printf("error enqueuing redeploy: %v\n", err) | |
| } | |
| return application, nil | |
| application, err := t.Storage.GetApplicationById(request.ID.String(), organizationID) | |
| if err != nil { | |
| return shared_types.Application{}, err | |
| } | |
| contextTask := ContextTask{ | |
| TaskService: t, | |
| ContextConfig: request, | |
| UserId: userID, | |
| OrganizationId: organizationID, | |
| Application: &application, | |
| } | |
| TaskPayload, err := contextTask.PrepareReDeploymentContext() | |
| if err != nil { | |
| return shared_types.Application{}, err | |
| } | |
| if err := ReDeployQueue.Add(TaskReDeploy.WithArgs(context.Background(), TaskPayload)); err != nil { | |
| return application, fmt.Errorf("enqueue redeploy: %w", err) | |
| } | |
| return application, nil |
🤖 Prompt for AI Agents
In api/internal/features/deploy/tasks/create.go around lines 104 to 127, the
code currently prints enqueue errors for ReDeployQueue.Add but still returns
success; change this so enqueue failures are propagated to the caller.
Specifically, after calling ReDeployQueue.Add(...), if err != nil then wrap or
annotate the error with context (e.g., fmt.Errorf("failed to enqueue redeploy:
%w", err)), optionally log it, and return shared_types.Application{} and the
wrapped error instead of continuing to return the application and nil.
| var caddyClient *caddygo.Client | ||
|
|
||
| func GetCaddyClient() *caddygo.Client { | ||
| if caddyClient == nil { | ||
| caddyClient = caddygo.NewClient(config.AppConfig.Proxy.CaddyEndpoint) | ||
| } | ||
| return caddyClient | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Caddy client init concurrency-safe (and deterministic).
GetCaddyClient has a racy check-then-set; multiple goroutines can initialize it. Use sync.Once.
Apply:
-var caddyClient *caddygo.Client
+var (
+ caddyClient *caddygo.Client
+ caddyInitOnce sync.Once
+)
-func GetCaddyClient() *caddygo.Client {
- if caddyClient == nil {
- caddyClient = caddygo.NewClient(config.AppConfig.Proxy.CaddyEndpoint)
- }
- return caddyClient
-}
+func GetCaddyClient() *caddygo.Client {
+ caddyInitOnce.Do(func() {
+ endpoint := config.AppConfig.Proxy.CaddyEndpoint
+ // Optional: fallback if unset; remove if you prefer hard-fail.
+ if endpoint == "" {
+ endpoint = "http://localhost:2019"
+ }
+ caddyClient = caddygo.NewClient(endpoint)
+ })
+ return caddyClient
+}📝 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.
| var caddyClient *caddygo.Client | |
| func GetCaddyClient() *caddygo.Client { | |
| if caddyClient == nil { | |
| caddyClient = caddygo.NewClient(config.AppConfig.Proxy.CaddyEndpoint) | |
| } | |
| return caddyClient | |
| } | |
| var ( | |
| caddyClient *caddygo.Client | |
| caddyInitOnce sync.Once | |
| ) | |
| func GetCaddyClient() *caddygo.Client { | |
| caddyInitOnce.Do(func() { | |
| endpoint := config.AppConfig.Proxy.CaddyEndpoint | |
| // Optional: fallback if unset; remove if you prefer hard-fail. | |
| if endpoint == "" { | |
| endpoint = "http://localhost:2019" | |
| } | |
| caddyClient = caddygo.NewClient(endpoint) | |
| }) | |
| return caddyClient | |
| } |
🤖 Prompt for AI Agents
In api/internal/features/deploy/tasks/init.go around lines 44 to 51, the
GetCaddyClient function performs a racy check-then-set on the package-level
caddyClient; make initialization concurrency-safe and deterministic by adding a
package-level sync.Once (e.g., caddyOnce) and use caddyOnce.Do(func() {
caddyClient = caddygo.NewClient(config.AppConfig.Proxy.CaddyEndpoint) }) inside
GetCaddyClient, then return caddyClient so only one goroutine initializes the
client.
raghavyuva
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
view/app/containers/[id]/components/OverviewTab.tsx (1)
78-81: Avoid index in React keys; prefer a stable composite keyUsing the array index can cause unnecessary remounts on reordering. If possible, build a stable key from the data itself (include protocol/type to reduce collisions).
Apply:
- {container?.ports?.map((port, index) => ( - <Badge key={`${port.private_port}-${port.public_port}-${index}`} variant="outline"> + {container?.ports?.map((port) => ( + <Badge key={`${port.type}-${port.public_port}-${port.private_port}`} variant="outline">view/app/containers/[id]/components/Terminal.tsx (3)
18-25: Dispose terminal on unmount to prevent leaksThe hook exposes destroyTerminal but it’s never called; unmounting the tab/page could leak resources.
Apply:
- const { terminalRef: termRef, initializeTerminal, terminalInstance } = useTerminal( + const { terminalRef: termRef, initializeTerminal, destroyTerminal, terminalInstance } = useTerminal( true, 0, 0, true, sessionId ); @@ useEffect(() => { if (isMounted) { initializeTerminal(); } - }, [isMounted, initializeTerminal]); + return () => { + destroyTerminal(); + }; + }, [isMounted, initializeTerminal, destroyTerminal]);Also applies to: 28-33
14-15: Remove redundant ref and ts-ignore; use the hook’s ref directlyThe local terminalRef is unused beyond forwarding and requires a ts-ignore. Use termRef directly.
Apply:
- const terminalRef = useRef<HTMLDivElement | null>(null); @@ - <div - ref={(el) => { - terminalRef.current = el; - // @ts-ignore - if (termRef) termRef.current = el; - }} + <div + ref={termRef as React.RefObject<HTMLDivElement>}Also applies to: 46-55
40-43: Avoid arbitrary 150ms delay before sending initTie the init send to readiness only; the timeout can be flaky.
Apply:
- setTimeout(() => { - sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } }); - }, 150); + sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } });view/app/containers/[id]/page.tsx (3)
29-29: Code-split the heavy terminal to keep the default bundle leanXTerm + CSS are sizable; load the Terminal tab on demand.
Apply:
+import dynamic from 'next/dynamic'; -import { Terminal as TerminalComponent } from './components/Terminal'; +const TerminalComponent = dynamic(() => import('./components/Terminal'), { + ssr: false, + loading: () => <div className="h-48" /> +});
208-210: Localize the fallback messageConsistent i18n with the rest of the page.
Apply:
- Start the container to use the terminal + {t('terminal.start_to_use')}If the key doesn’t exist yet, add it to your translations.
182-185: Disabled tab + hidden content is redundant; pick one UXIf the tab is disabled, users can’t access the message. Either:
- Keep the tab enabled and show the message when not running, or
- Keep it disabled and drop the message block.
I’d enable the tab so users discover the Terminal and see why it’s unavailable.
Also applies to: 204-212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/api/versions.json(1 hunks)view/app/containers/[id]/components/OverviewTab.tsx(1 hunks)view/app/containers/[id]/components/Terminal.tsx(1 hunks)view/app/containers/[id]/page.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/api/versions.json
🧰 Additional context used
🧬 Code graph analysis (2)
view/app/containers/[id]/page.tsx (1)
view/app/containers/[id]/components/Terminal.tsx (1)
Terminal(13-57)
view/app/containers/[id]/components/Terminal.tsx (3)
view/hooks/socket-provider.tsx (1)
useWebSocket(189-197)view/app/terminal/utils/useTerminal.ts (1)
useTerminal(21-192)view/app/terminal/utils/isContainerReady.ts (1)
useContainerReady(3-44)
⏰ 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). (1)
- GitHub Check: test
| useEffect(() => { | ||
| if (!hasSentInitRef.current && terminalInstance && isReady) { | ||
| hasSentInitRef.current = true; | ||
| // TODO: optimize this such that backend handles this instead of client. | ||
| const cmd = `if docker ps >/dev/null 2>&1; then D=docker; else D='sudo -n docker'; fi; $D exec -it ${containerId} /bin/bash || $D exec -it ${containerId} /bin/sh\r`; | ||
| setTimeout(() => { | ||
| sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } }); | ||
| }, 150); | ||
| } | ||
| }, [terminalInstance, isReady, sendJsonMessage, containerId, sessionId]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Client-side shell command construction with interpolated containerId → injection risk
Interpolating containerId into a shell command is dangerous if the value is ever non-hex or user-controlled. Also, sudo fallbacks are a server concern. Move attach logic server-side (structured message) or strictly validate and quote.
Minimal mitigation in-place:
- useEffect(() => {
- if (!hasSentInitRef.current && terminalInstance && isReady) {
- hasSentInitRef.current = true;
- // TODO: optimize this such that backend handles this instead of client.
- const cmd = `if docker ps >/dev/null 2>&1; then D=docker; else D='sudo -n docker'; fi; $D exec -it ${containerId} /bin/bash || $D exec -it ${containerId} /bin/sh\r`;
- setTimeout(() => {
- sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } });
- }, 150);
- }
- }, [terminalInstance, isReady, sendJsonMessage, containerId, sessionId]);
+ useEffect(() => {
+ if (!hasSentInitRef.current && terminalInstance && isReady) {
+ hasSentInitRef.current = true;
+ // Prefer backend-side attach (send containerId, let server exec safely).
+ const safeId = /^[a-f0-9]{12,64}$/i.test(containerId) ? containerId : null;
+ if (!safeId) {
+ console.error('Invalid containerId; refusing to initialize terminal.');
+ return;
+ }
+ const cmd = `if docker ps >/dev/null 2>&1; then D=docker; else D='sudo -n docker'; fi; ` +
+ `$D exec -it -- "${safeId}" /bin/bash || $D exec -it -- "${safeId}" /bin/sh\r`;
+ setTimeout(() => {
+ sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } });
+ }, 150);
+ }
+ }, [terminalInstance, isReady, sendJsonMessage, containerId, sessionId]);If you’re open to it, I can draft the server-side “attach” message flow next.
📝 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.
| useEffect(() => { | |
| if (!hasSentInitRef.current && terminalInstance && isReady) { | |
| hasSentInitRef.current = true; | |
| // TODO: optimize this such that backend handles this instead of client. | |
| const cmd = `if docker ps >/dev/null 2>&1; then D=docker; else D='sudo -n docker'; fi; $D exec -it ${containerId} /bin/bash || $D exec -it ${containerId} /bin/sh\r`; | |
| setTimeout(() => { | |
| sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } }); | |
| }, 150); | |
| } | |
| }, [terminalInstance, isReady, sendJsonMessage, containerId, sessionId]); | |
| useEffect(() => { | |
| if (!hasSentInitRef.current && terminalInstance && isReady) { | |
| hasSentInitRef.current = true; | |
| // Prefer backend-side attach (send containerId, let server exec safely). | |
| const safeId = /^[a-f0-9]{12,64}$/i.test(containerId) ? containerId : null; | |
| if (!safeId) { | |
| console.error('Invalid containerId; refusing to initialize terminal.'); | |
| return; | |
| } | |
| const cmd = `if docker ps >/dev/null 2>&1; then D=docker; else D='sudo -n docker'; fi; ` + | |
| ` $D exec -it -- "${safeId}" /bin/bash || $D exec -it -- "${safeId}" /bin/sh\r`; | |
| setTimeout(() => { | |
| sendJsonMessage({ action: 'terminal', data: { value: cmd, terminalId: sessionId } }); | |
| }, 150); | |
| } | |
| }, [terminalInstance, isReady, sendJsonMessage, containerId, sessionId]); |
🤖 Prompt for AI Agents
In view/app/containers/[id]/components/Terminal.tsx around lines 35 to 44, the
code builds a raw shell command with an interpolated containerId and sends it to
the backend, which risks shell injection and mixes client/server
responsibilities; change the client to send a structured attach request instead
of a raw command (e.g. sendJsonMessage({ action: 'attach', data: { containerId:
sessionId, shell: ['/bin/bash','/bin/sh'] } })), remove the sudo/docker
detection and raw cmd construction from the client, and let the server validate
and safely construct/execute the docker exec command (server must strictly
validate containerId against a safe pattern like /^[a-fA-F0-9]+$/ and avoid
invoking a shell with unescaped strings); if you cannot change server now, as a
minimal mitigation validate containerId on the client to a strict allowed
pattern before sending and avoid interpolating it into any shell string.
3 participants
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Improvements