Merge upstream Komodo v2.1.1 into agentic CLI#8
Merged
Conversation
* fix swarm config content hidden * 2.0.1-dev-1 * bump ts deps * align swarm config links * StackServiceRun command is actually optional * fix swarm not included in read resource toml * deploy 2.0.1-dev-2 * logger support disabling timestamps (eg if they are already provided by docker logs) * full opacity tag color selector * Add UpdateSwarmNode execution and convert several large repetive blocks to macro handling * add node update functionality * deploy 2.0.1-dev-3 * deploy 2.1.0-dev-1 * toml variable export sorted by name * deploy 2.1.0-dev-2 * dont use detach=false because it can hang indefinitely on misconfigured stacks * deploy 2.1.0-dev-3 * swarm stack deploy explicitly '--detach=true' to avoid future changes in behavior * fmt * UI: Stack config: Fix Add Env File button * fix swarm error propogation, add server connection error hover message * deploy 2.1.0-dev-4 * fix service rm (should be stack rm * fix container ports not displaying when server address not available for link * fix build registry custom org configuration * github action: comment out cache step (leads to no space on device) * fix missing nullish check when selecting stack (moghtech#1287) * fix container selector null crash * provider selector custom input label adheres to showLabel * fix: show/hide button non-functional in stack, sync, and build info views (moghtech#1267) Co-authored-by: twalts <t.mwalton@yahoo.com> * swarm stack support env vars through shell source file method * tweaks * bump deps * deploy 2.1.0-dev-5 * auto update should work with swarm * deploy 2.1.0-dev-6 * 2.1.0 --------- Co-authored-by: Shen Li <dustet@gmail.com> Co-authored-by: T <github@mail.taylor.media> Co-authored-by: twalts <t.mwalton@yahoo.com>
* start 2.1.1 to fix swarm auto updates * correctly extract image and digest from swarm stacks / deployments * 2.1.1 fix swarm auto updates not picking up * fmt
Bring the fork forward to Komodo v2.1.1 while preserving the agent-safe CLI behavior and custom ssh/exec workflow.
📝 WalkthroughWalkthroughThis PR implements a new swarm node update feature (UpdateSwarmNode), refactors large match blocks into macros for improved maintainability, introduces structured error types (Serror) throughout error fields, adds logging timestamp configuration via environment variables, enhances environment variable handling for stack deployment, and updates Rust toolchain and dependencies across all components. Changes
🚥 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📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Pull request overview
Merges upstream Komodo changes through v2.1.1 into the agentic CLI fork, updating the Rust workspace (core/periphery/CLI/client) and the UI to match upstream while retaining agentic CLI custom behaviors.
Changes:
- Adds UpdateSwarmNode support end-to-end (API types, core/periphery execution plumbing, UI controls for selecting/updating nodes).
- Switches server/swarm error payloads to a structured error type (message + trace) and introduces reusable UI components for displaying errors/labels.
- Updates logging configuration to include timestamps, refactors core sync/procedure helpers, and adjusts CI workflow triggers/caching.
Reviewed changes
Copilot reviewed 50 out of 86 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/tsconfig.json | Updates TS compiler options / path mapping config. |
| ui/src/ui/section.tsx | Section header layout change to support titleRight rendering. |
| ui/src/ui/labels-group.tsx | New UI component for rendering key/value labels as badges. |
| ui/src/ui/hover-error.tsx | New UI component for structured error + trace hover display. |
| ui/src/resources/sync/info.tsx | Fixes Show/Hide toggle wiring. |
| ui/src/resources/swarm/index.tsx | Refactors swarm error UI to reuse HoverError. |
| ui/src/resources/swarm/docker/nodes/update.tsx | New modal/UI to update swarm node role/availability/labels. |
| ui/src/resources/swarm/docker/nodes/index.tsx | Adds node selection + update action + labels column. |
| ui/src/resources/swarm/config.tsx | Tweaks config UI labeling for links. |
| ui/src/resources/stack/update-available.tsx | Adjusts update-available visibility conditions. |
| ui/src/resources/stack/info.tsx | Fixes Show/Hide toggle wiring. |
| ui/src/resources/stack/config/index.tsx | Updates wording/layout for stack config UI. |
| ui/src/resources/server/index.tsx | Adds structured server error display via HoverError. |
| ui/src/resources/procedure/config/executions.tsx | Adds UpdateSwarmNode execution type + placeholder wording tweak. |
| ui/src/resources/deployment/update-available.tsx | Adjusts update-available visibility conditions. |
| ui/src/resources/build/info.tsx | Fixes Show/Hide toggle wiring. |
| ui/src/resources/build/config.tsx | Registry config list keying change. |
| ui/src/pages/swarm/node/index.tsx | Adds UpdateSwarmNodes action to node subpage. |
| ui/src/pages/settings/tags/color-selector.tsx | Adjusts tag color token usage. |
| ui/src/components/stack-service-selector.tsx | Fixes optional chaining access. |
| ui/src/components/docker/containers-section.tsx | Removes explicit wrap prop usage for ContainerPorts. |
| ui/src/components/docker/container-selector.tsx | Fixes optional chaining access. |
| ui/src/components/docker/container-ports.tsx | Refactors ports rendering + hovercard/link behavior. |
| ui/src/components/config/provider-selector.tsx | Makes “Domain” label conditional on showLabel. |
| ui/src/components/config/organization-selector.tsx | Adds label in custom mode + makes selector searchable. |
| ui/public/client/types.js | Adds UpdateSwarmNode operation to generated client JS. |
| ui/public/client/types.d.ts | Adds UpdateSwarmNode types + structured error type updates. |
| ui/public/client/responses.d.ts | Adds UpdateSwarmNode execute response typing. |
| ui/package.json | Updates UI toolchain/deps and pins packageManager. |
| lib/environment/src/lib.rs | Ensures written env files end with a newline. |
| lib/command/src/lib.rs | Changes default shell selection to explicit absolute paths. |
| example/update_logger/Dockerfile | Bumps Rust image version. |
| example/alerter/Dockerfile | Bumps Rust image version. |
| client/periphery/rs/src/api/swarm.rs | Updates UpdateSwarmNode API shape (label_add as Vec). |
| client/core/ts/yarn.lock | Updates TypeScript version in lockfile. |
| client/core/ts/tsconfig.json | Switches to bundler resolution + sets rootDir. |
| client/core/ts/src/types.ts | Updates core TS types for UpdateSwarmNode + structured errors. |
| client/core/ts/src/responses.ts | Adds UpdateSwarmNode to execute responses. |
| client/core/ts/package.json | Bumps komodo_client TS package version to 2.1.1. |
| client/core/rs/src/entities/swarm.rs | Switches swarm list error field to structured error type. |
| client/core/rs/src/entities/server.rs | Adds structured error field to server list info. |
| client/core/rs/src/entities/mod.rs | Adds UpdateSwarmNode operation enum variant. |
| client/core/rs/src/entities/logger.rs | Adds timestamps to logger config with defaults and trait impl. |
| client/core/rs/src/entities/docker/node.rs | Adds hostname/labels fields + clap ValueEnum derives. |
| client/core/rs/src/entities/config/periphery.rs | Adds env override for periphery logging timestamps. |
| client/core/rs/src/entities/config/core.rs | Adds env override for core logging timestamps + reorders env fields. |
| client/core/rs/src/entities/config/cli/mod.rs | Adds env override for CLI logging timestamps. |
| client/core/rs/src/entities/alert.rs | Switches alert swarm error payload to structured error type. |
| client/core/rs/src/api/execute/swarm.rs | Adds UpdateSwarmNode execute request + OpenAPI metadata. |
| client/core/rs/src/api/execute/openapi.rs | Registers UpdateSwarmNode path in OpenAPI. |
| client/core/rs/src/api/execute/mod.rs | Adds UpdateSwarmNode to Execution enum. |
| Cargo.toml | Bumps workspace/crate dependency versions to v2.1.1 line. |
| Cargo.lock | Lockfile updates for workspace version bumps and deps. |
| bin/periphery/src/docker/node.rs | Includes hostname/labels in node list items. |
| bin/periphery/src/config.rs | Adds timestamps logging override wiring. |
| bin/periphery/src/api/swarm/stack.rs | Adds env-file sourcing for stack config/deploy + adjusts deploy args. |
| bin/periphery/src/api/swarm/mod.rs | Updates label-add argument formatting for node update. |
| bin/periphery/aio.Dockerfile | Bumps Rust image version. |
| bin/core/src/sync/toml.rs | Refactors procedure ID->name replacement helper usage. |
| bin/core/src/sync/resources.rs | Refactors procedure sync logic via helper. |
| bin/core/src/sync/file.rs | Macro refactor for extending resources by tag. |
| bin/core/src/state.rs | Switches swarm cache error type to structured error. |
| bin/core/src/resource/server.rs | Exposes structured server error in list info. |
| bin/core/src/resource/procedure.rs | Refactors execution permission checks via macros + adds UpdateSwarmNode. |
| bin/core/src/monitor/swarm.rs | Switches swarm monitor error storage to structured error type. |
| bin/core/src/monitor/resources.rs | Improves image digest extraction and formatting logic. |
| bin/core/src/helpers/update.rs | Refactors execute-request -> update init via macro + adds UpdateSwarmNode. |
| bin/core/src/helpers/procedure.rs | Large refactor of execution dispatch + adds helper for ID->name replacement. |
| bin/core/src/connection/server.rs | Adjusts onboarding “fix server” logic (address/external_address). |
| bin/core/src/config.rs | Adds timestamps logging override wiring. |
| bin/core/src/api/write/stack.rs | Prevents per-service deploy for swarm stacks. |
| bin/core/src/api/read/toml.rs | Sorts variables by name when exporting to TOML. |
| bin/core/src/api/execute/swarm.rs | Implements UpdateSwarmNode execute handler (core -> periphery). |
| bin/core/src/api/execute/mod.rs | Adds UpdateSwarmNode execute request variant. |
| bin/core/src/api/execute/maintenance.rs | Improves GlobalAutoUpdate checks for server/swarm reachability. |
| bin/core/src/alert/slack.rs | Uses debug formatting for structured error details in alerts. |
| bin/core/src/alert/mod.rs | Updates standard alert content for structured errors + reorder cases. |
| bin/core/src/alert/discord.rs | Updates Discord alerts for structured errors + reorder cases. |
| bin/core/aio.Dockerfile | Bumps Rust image version. |
| bin/cli/src/config.rs | Adds timestamps logging override wiring. |
| bin/cli/src/command/execute.rs | Refactors execution printing/execution via macros; adds UpdateSwarmNode. |
| bin/cli/aio.Dockerfile | Bumps Rust image version. |
| bin/chef.binaries.Dockerfile | Bumps cargo-chef Rust image version. |
| bin/binaries.Dockerfile | Bumps Rust image version. |
| .github/workflows/ci.yml | Runs CI on agentic-cli branch; disables cargo cache to reduce disk usage. |
Comments suppressed due to low confidence (2)
ui/tsconfig.json:28
compilerOptions.pathsis configured, butbaseUrlwas removed. TypeScript requiresbaseUrlto be set when usingpathsmappings; otherwise TS will error and@/...imports won’t resolve in type-checking/IDE. RestorebaseUrl(typically ".") or rework the aliasing approach so TS and Vite stay in sync.
ui/src/resources/swarm/docker/nodes/index.tsx:55selectKeycan produce non-unique IDs (e.g., multiple nodes with missing fields will all become "Unknown", andName/Hostnamearen’t guaranteed unique). BecauseDataTableuses this asgetRowId, collisions will break row selection (selecting one row may select another). Prefer a guaranteed-unique key (ideallynode.ID, with a safe fallback like${Hostname ?? Name ?? "unknown"}-${index}if needed).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+37
| if (Object.keys(portsMap).length > 0) { | ||
| console.log("mapped", portsMap); | ||
| } |
There was a problem hiding this comment.
There are several console.log statements left in this component. This will spam the browser console for any container with ports and can leak potentially sensitive info in shared screenshots/logs. Please remove these debug logs (or gate them behind an explicit dev-only flag).
Comment on lines
62
to
+67
| if (!groupedPorts.length) { | ||
| return null; | ||
| } | ||
|
|
||
| console.log("grouped", groupedPorts); | ||
|
|
There was a problem hiding this comment.
console.log("grouped", groupedPorts); is unconditional, so it will always log whenever this component renders with ports. Please remove this log before merging.
Comment on lines
25
to
31
| export default function ContainerPorts({ | ||
| ports, | ||
| serverId, | ||
| hoverCardProps, | ||
| gap = "xs", | ||
| wrap = "nowrap", | ||
| ...groupProps |
There was a problem hiding this comment.
wrap is destructured from props but not actually respected: DividedChildren is hard-coded to wrap="nowrap". This makes the wrap prop (and any wrap passed via groupProps) ineffective. Use the wrap variable (defaulting to "nowrap") when rendering DividedChildren so callers can override wrapping behavior.
Comment on lines
123
to
+130
| <Group | ||
| renderRoot={(props) => <a target="_blank" href={link} {...props} />} | ||
| renderRoot={(props) => | ||
| link ? ( | ||
| <a target="_blank" href={link} {...props} /> | ||
| ) : ( | ||
| <div {...props} /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Links opened with target="_blank" should include rel="noopener noreferrer" to prevent reverse-tabnabbing. Add the rel attribute on both anchor renderRoots when link is present.
Comment on lines
243
to
246
| {image_registries?.map((registry, index) => ( | ||
| <ImageRegistryConfig | ||
| key={ | ||
| (registry.domain ?? "") + | ||
| (registry.organization ?? "") + | ||
| (registry.account ?? "") + | ||
| index | ||
| } | ||
| key={index} | ||
| registry={registry} |
There was a problem hiding this comment.
Using the array index as a React key for ImageRegistryConfig can cause incorrect UI state when users insert/remove registries (React will reuse component instances for different items). Prefer a stable key derived from registry identity (e.g., an explicit id/uuid, or a concatenation of stable fields if guaranteed unique).
Comment on lines
+29
to
+34
| const [opened, { open, close }] = useDisclosure(); | ||
| const { data: inspect, isPending } = useRead( | ||
| "InspectSwarmNode", | ||
| { swarm, node: nodes[0] }, | ||
| { enabled: opened && nodes.length === 1 }, | ||
| ); |
There was a problem hiding this comment.
useRead("InspectSwarmNode", { swarm, node: nodes[0] }, ...) passes nodes[0] which is string | undefined. Even though the query is disabled unless nodes.length === 1, this still violates the request type and can cause TS errors / confusing cache keys. Consider using node: nodes[0] ?? "" (or another safe placeholder) while keeping the enabled guard.
Comment on lines
+49
to
+57
| const { mutate: update, isPending: updatePending } = useExecute( | ||
| "UpdateSwarmNode", | ||
| { | ||
| onSuccess: () => { | ||
| close(); | ||
| resetForm(); | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
When updating multiple nodes, onSubmit calls mutate in a loop, but the mutation onSuccess closes the modal and resets the form on the first successful update. This can hide failures for later nodes and makes it hard to tell whether all updates completed. Consider aggregating the updates (e.g., track remaining count / use mutateAsync + Promise.allSettled) and only close/reset once all nodes have completed successfully (or present per-node results).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/core/rs/src/entities/alert.rs (1)
92-99:⚠️ Potential issue | 🟠 MajorBackward-compatibility risk for persisted alerts (
errshape change).Line 98 changes
SwarmUnhealthy.errto a structured type. Existing alert documents with legacy stringerrcan fail deserialization unless you add compatibility handling (custom deserializer accepting both string/object) or run a data migration before rollout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/core/rs/src/entities/alert.rs` around lines 92 - 99, SwarmUnhealthy's err field was changed to a structured type (_Serror), which breaks deserialization of persisted alerts that store err as a legacy string; update the deserialization to accept both shapes by adding a compatibility serde handler (e.g., implement Deserialize for _Serror that accepts either a string or an object, or declare an untagged enum wrapper) for the _Serror type used by SwarmUnhealthy.err, or add a migration step to convert legacy string errors into the new structured form before rollout; locate the SwarmUnhealthy definition and the _Serror type in alert.rs and apply the compatibility deserializer or migration accordingly.bin/core/src/connection/server.rs (1)
319-334:⚠️ Potential issue | 🔴 CriticalConstructed
configis not used;external_addresschange is dead code.The
PartialServerConfigbuilt on lines 299–318 (including the newexternal_addresslogic) is only used for theis_none()check on line 319. The actualUpdateServercall on lines 320–333 constructs a new hardcoded config that ignoresexternal_address:UpdateServer { id: server.id, config: PartialServerConfig { enabled: Some(true), address: Some(String::default()), ..Default::default() // <-- external_address is None here }, }The fix should use the constructed
config:if !config.is_none() { UpdateServer { id: server.id, - config: PartialServerConfig { - enabled: Some(true), - address: Some(String::default()), - ..Default::default() - }, + config, } .resolve(&args)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/core/src/connection/server.rs` around lines 319 - 334, The constructed `config` (the PartialServerConfig that may include external_address) is never passed into the UpdateServer call; instead a new hardcoded PartialServerConfig is created. Replace the inline PartialServerConfig in the UpdateServer { id: server.id, config: ... } expression with the previously built `config` value (ensuring it has enabled set to Some(true) and address populated as needed), so UpdateServer uses the constructed config (and keep the .resolve(&args).await.map_err(...) flow unchanged).ui/src/resources/stack/config/index.tsx (1)
531-546:⚠️ Potential issue | 🟡 MinorPoint the compose branch at the compose docs.
Lines 534-536 now describe
docker compose up, but Line 545 still links the non-swarm path todocker/service/create. That sends users to the wrong option set when they follow “See docker docs.”🔧 Suggested change
to={ currSwarmId ? "https://docs.docker.com/reference/cli/docker/stack/deploy/#options" - : "https://docs.docker.com/reference/cli/docker/service/create/#options" + : "https://docs.docker.com/reference/cli/docker/compose/up/#options" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/resources/stack/config/index.tsx` around lines 531 - 546, The non-swarm branch of the Link in the description block still points to the service/create docs; update the ternary that sets Link's to (where currSwarmId is checked) so the false/compose branch points to the Docker Compose "up" docs (e.g. the compose up reference URL) instead of "docker/service/create"; locate the JSX using currSwarmId and the Link component inside the description prop and replace only the non-swarm URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/core/src/alert/mod.rs`:
- Around line 261-264: The code is formatting errors with Debug ("{e:#?}") when
building the alert body; change the mapping closure that produces err (the let
err = err.as_ref().map(|e| format!("\nerror: {e:#?}")).unwrap_or_default();) to
use Display/sanitized text instead (e.g. format!("\nerror: {}", e) or
e.to_string()) so user-facing channels receive clean, non-debug output; apply
the same replacement for the other occurrence that uses "{e:#?}" (the similar
map/format block around lines reported).
In `@bin/core/src/alert/slack.rs`:
- Around line 111-129: The header currently uses the full multiline `text`
(created in the `match` on `alert.level`) which places version details into a
Slack header; change this so `Block::header` receives a short one-line
`header_text` (e.g., "{level} | *{name}*{region}" with the ✅ or ⚠️ marker) and
build a separate `section_text` for the version details (e.g., "Periphery:
{server_version} | Core: {core_version}") that you pass to `Block::section`;
update the `blocks` vector to include `Block::header(header_text)` followed by
the `Block::section(resource_link(...))` and another
`Block::section(section_text)` only for the mismatch branch so Ok still shows a
short header without extra section.
In `@bin/core/src/api/execute/maintenance.rs`:
- Around line 271-299: The reachability check for SwarmOrServer is duplicated;
extract it into a helper function (e.g., is_reachable(swarm_or_server,
server_status_cache, swarm_status_cache) -> bool) that encapsulates the current
logic that checks server_status_cache for ServerState::Ok and swarm_status_cache
for SwarmState::Healthy | SwarmState::Unhealthy, then replace both match blocks
(the one around SwarmOrServer in execute/maintenance.rs and the similar block
used for stacks/deployments) to call that helper and continue when it returns
false; reference SwarmOrServer, server_status_cache, swarm_status_cache,
ServerState::Ok, and SwarmState::Healthy|Unhealthy when implementing the helper.
In `@bin/core/src/api/write/stack.rs`:
- Around line 942-945: The deploy_services condition currently checks
stack.config.swarm_id directly (alongside
stack.config.auto_update_all_services); replace that raw-config swarm detection
with the stack's resolved execution target instead (e.g., use the resolved
execution target accessor on the Stack such as stack.execution_target /
stack.resolved_target or the helper is_swarm() on the resolved target) so the
clause becomes: if stack.config.auto_update_all_services ||
resolved_target.is_swarm() (keeping the existing Swarm comment), ensuring deploy
logic uses the actual resolved target rather than stack.config.swarm_id to avoid
config-drift edge cases.
In `@bin/core/src/connection/server.rs`:
- Around line 310-316: The conditional is inverted: currently it sets
external_address to server.config.address only when
server.config.external_address is non-empty; change the logic so
external_address is set to Some(server.config.address) when
server.config.external_address IS empty (i.e., when no external_address has been
configured) and None otherwise. Locate the construction that assigns
external_address (reference server.config.external_address and the
external_address field in the server struct) and flip the condition to check for
emptiness before moving server.config.address into external_address.
In `@bin/core/src/helpers/procedure.rs`:
- Around line 665-783: The current replace_procedure_stage_ids_with_names
function (and its replace_id_with_name! macro) uses .unwrap_or_default() which
turns cache misses into empty strings; change those fallbacks to preserve the
original identifier by using the existing id/name from the config when a lookup
fails (e.g. assign config.$field = all.$collection.get(&config.$field).map(|r|
r.name.clone()).unwrap_or_else(|| config.$field.clone())); do the analogous
change inside the SendAlert mapping so each alerter maps to a.name.clone() or
falls back to the original alerter id string instead of "" so missing resources
remain unchanged.
In `@bin/core/src/monitor/resources.rs`:
- Around line 80-96: The tag-detection logic around the tuple assignment for
(image, image_digests) is incorrect because image.contains(':') treats registry
host:port as a tag; update the conditional that currently uses
image.contains(':') (in the closure producing the fallback image and
image_digests) to detect a tag only if a colon appears after the last slash
(i.e., a ':' index greater than the last '/' index) so registry ports are
ignored; keep ImageDigest::parse(...) behavior and ensure when no tag is found
you append ":latest" to the image string and set image_digests to None.
In `@bin/core/src/sync/file.rs`:
- Around line 253-266: The manual list passed to the extend_filtered! macro can
drift when ResourcesToml gains new tag-filtered fields; add a regression test
that constructs a ResourcesToml instance with every tag-filtered field populated
(seed each of servers, swarms, stacks, deployments, builds, repos, procedures,
actions, alerters, builders, resource_syncs), runs the same merge/extend logic
(exercise the extend_filtered! path), and asserts the merged result contains
entries for every seeded field to fail if any field is missing; implement this
as a unit test that programmatically enumerates the ResourcesToml fields you
expect and compares them to the items processed by the extend_filtered!
invocation so future additions to ResourcesToml will cause the test to fail
until the macro call list is updated.
In `@bin/periphery/src/api/swarm/mod.rs`:
- Around line 102-104: The loop that builds the shell command concatenates raw
label values from label_add and label_rm into the command string (see label_add
and label_rm usage), which breaks tokenization in run_komodo_standard_command;
fix by shell-escaping or quoting each value before concatenation (e.g., use
shlex::try_quote(value) or wrap the value in quotes and handle errors), so
replace direct concatenation of &key_value with the escaped/quoted variant for
both the label_add and label_rm loops to ensure each label stays a single
argument.
In `@bin/periphery/src/api/swarm/stack.rs`:
- Around line 367-397: The env_file_args function currently injects raw
AdditionalEnvFile.path values directly into shell source commands (write! calls
building res), allowing shell metacharacter injection when the resulting string
is passed to shell -c; fix by ensuring paths are sanitized and safely quoted
before concatenation — either validate/normalize AdditionalEnvFile.path upstream
to reject unsafe characters and enforce absolute/relative path patterns, or
escape single quotes and wrap each path in single quotes when building res (and
apply the same treatment to the env_file_path branch), and add unit tests for
env_file_args to assert that inputs with metacharacters do not produce
executable sequences.
In `@client/core/rs/src/api/execute/swarm.rs`:
- Around line 83-88: The doc comment for the label_rm field is incorrect: it
currently says "Add labels" and shows a `key=value` example; update the doc for
the pub label_rm: Option<Vec<String>> field to clearly state it removes labels
(e.g., "Remove labels from node") and adjust the example to the expected input
shape (likely just `key` or comma-separated keys) so the CLI/OpenAPI help
reflects removal semantics; keep the existing #[arg(long, alias = "lr")]
attributes and alias note intact.
In `@client/core/rs/src/entities/server.rs`:
- Around line 40-42: The doc comment for the field `err: Option<_Serror>`
incorrectly describes it as a message string; update the comment to state that
this field holds an optional structured error object (`_Serror`) representing
details about failures reaching the server (e.g., error code, message,
metadata), rather than a plain message text. Modify the comment above `err` in
server.rs to mention the structured error type and the fact it may be None on
success, and keep the wording concise and factual to avoid implying it's just a
message.
In `@client/core/rs/src/entities/swarm.rs`:
- Around line 29-31: The doc comment for the Swarm struct field `err` is stale —
it describes a message string but the field is now `pub err: Option<_Serror>`;
update the comment to state that `err` contains an optional structured error
payload of type `_Serror` (or similar structured error info) rather than a plain
message, so callers know to deserialize/inspect the `_Serror` fields.
In `@client/core/ts/src/types.ts`:
- Around line 5343-5347: Update the doc comment for the err field to reflect
that it is a structured error object, not a plain message: change the comment on
the err?: _Serror property (and the identical comment near lines referencing the
same type) to describe the shape/purpose of the _Serror payload (e.g., contains
code, message, details fields) and note it may be undefined when there is no
network/server error; also update the upstream source that generates this file
so the generated TS docs match the _Serror structure.
- Around line 10502-10505: The doc comment for the TypeScript property label_rm
is incorrect: it currently states it accepts `key=value` pairs like label_add,
but Docker expects only label keys to remove; update the comment for the
label_rm field (and mention its alias `lr`) to state it accepts label keys
(e.g., `"key1"`, `"key2"`) rather than `key=value` pairs so generated clients
build correct requests.
In `@ui/package.json`:
- Around line 45-55: Add a CI job in ci.yml to validate the UI: configure
setup-node to use Node.js v20.19+ or >=22.12, enable Corepack (pinned yarn), run
yarn install in the ui/ directory, then run tsc --noEmit and vite build there;
reference the UI toolchain changes in package.json (vite, `@vitejs/plugin-react`,
typescript) and ensure the job runs before merge to verify the upgraded tool
versions against the required Node runtime.
In `@ui/public/client/types.d.ts`:
- Line 1697: The public type for the `err` field was changed to `_Serror`, which
is a breaking contract; restore backward compatibility by making the `err`
property accept both shapes (e.g., change `err?: _Serror` to `err?: string |
_Serror`) or provide a backward-compatible union/alias so existing consumers
expecting a string continue to compile; update all occurrences noted (the `err`
property instances including the one currently declared as `err?: _Serror` and
the other places referenced in the review) to use the union type or an alias
type (e.g., `type Err = string | _Serror`) to avoid breaking changes.
- Around line 9921-9924: Documentation for label_rm is incorrect: it currently
says "Add labels" but should indicate removing labels by key. Update the JSDoc
for the label_rm property in types.d.ts (symbol: label_rm) to describe that it
removes labels by key (e.g., "Remove labels from node (key names only)"). Also
correct the alias note if needed (alias: `lr`) and ensure the distinction from
label_add (which accepts `key=value`) is clear.
In `@ui/src/components/config/organization-selector.tsx`:
- Line 28: The TextInput in the OrganizationSelector component always renders a
label in custom mode, ignoring the showLabel prop; update the
OrganizationSelector (or the JSX where TextInput is used) to forward the
showLabel prop (or conditionally render the label) when in custom mode so both
custom and non-custom modes respect showLabel; specifically locate the TextInput
usage in organization-selector.tsx and either pass showLabel={showLabel} into
TextInput or wrap the label rendering with a conditional that checks showLabel.
In `@ui/src/components/docker/container-ports.tsx`:
- Around line 25-31: The component ContainerPorts extracts the caller-provided
prop wrap but then ignores it by passing a hard-coded "nowrap" to
DividedChildren; update the DividedChildren usage(s) in ContainerPorts to pass
the extracted wrap variable instead of the string literal so callers can control
wrapping (check both occurrences around the component render and at the other
noted location). Ensure the prop name wrap is forwarded unchanged to
DividedChildren.
- Around line 35-37: Remove the leftover debug console.log calls in the Docker
container ports component: delete the console.log("mapped", portsMap) inside the
render logic and any other console.log instances noted (around lines handling
ports grouping at the block that uses portsMap and the later mapping/rendering
code), i.e., clean up in the docker/container-ports.tsx component where portsMap
is computed and where port groups are rendered (functions/variables: portsMap,
the mapping/render loop that renders grouped ports), ensuring no
console.debug/console.log calls remain before committing.
In `@ui/src/pages/settings/tags/color-selector.tsx`:
- Line 80: The option swatch uses bg={`Tag${color}.9`} while the selected swatch
still uses bg={`Tag${color}`}, causing inconsistent shades; make both use the
same shared token format (either `Tag${color}.9` or `Tag${color}`) so the
ColorSelector renders consistent colors—update the selected swatch usage (the
Box/render for the active selection) to match the option swatch bg token format
(or vice versa) wherever `Tag${color}`/`Tag${color}.9` appears.
In `@ui/src/resources/build/config.tsx`:
- Line 245: The list rendering uses index-based keys in the image_registries.map
that causes incorrect reconciliation; change the mapping that renders
<ImageRegistryConfig> to use a stable per-item identifier instead of key={index}
— add a unique id (e.g., UUID) to each registry object when created/loaded or
derive a stable composite key from registry properties (like registry.url +
registry.name) and use that value as the key; update any creation code that
builds image_registries to populate the id and ensure ImageRegistryConfig
remains keyed by that id.
In `@ui/src/resources/swarm/docker/nodes/index.tsx`:
- Around line 43-55: The DataTable is using a non-unique selection key
(selectOptions.selectKey) that prefers Name/Hostname and falls back to
"Unknown", causing collisions and mis-selection; change selectOptions.selectKey
to return the node's ID (node.ID) as the primary and only key so selections and
UpdateSwarmNode (prop swarm={id} nodes={...}) map to unique rows; update the
selectKey implementation referenced in DataTable/selectOptions to use node.ID
(and only fallback to a safe unique value if ID is truly missing) so
multi-select and updates target the correct nodes.
In `@ui/src/resources/swarm/docker/nodes/update.tsx`:
- Around line 49-57: The onSuccess handler for the per-node mutate from
useExecute("UpdateSwarmNode") (the mutate alias update()) closes the modal and
calls resetForm() immediately on the first successful request, which hides the
dialog before the whole batch completes; change the logic so that you aggregate
per-node updates (e.g., collect the promises returned by multiple update(...)
calls and await Promise.all or maintain a counter/promise that resolves when
every node's mutate has succeeded), only invoke close() and resetForm() after
the entire batch Promise resolves successfully, and surface any per-node
failures instead of closing; apply the same aggregation fix to the other similar
useExecute block around the 66-79 region.
---
Outside diff comments:
In `@bin/core/src/connection/server.rs`:
- Around line 319-334: The constructed `config` (the PartialServerConfig that
may include external_address) is never passed into the UpdateServer call;
instead a new hardcoded PartialServerConfig is created. Replace the inline
PartialServerConfig in the UpdateServer { id: server.id, config: ... }
expression with the previously built `config` value (ensuring it has enabled set
to Some(true) and address populated as needed), so UpdateServer uses the
constructed config (and keep the .resolve(&args).await.map_err(...) flow
unchanged).
In `@client/core/rs/src/entities/alert.rs`:
- Around line 92-99: SwarmUnhealthy's err field was changed to a structured type
(_Serror), which breaks deserialization of persisted alerts that store err as a
legacy string; update the deserialization to accept both shapes by adding a
compatibility serde handler (e.g., implement Deserialize for _Serror that
accepts either a string or an object, or declare an untagged enum wrapper) for
the _Serror type used by SwarmUnhealthy.err, or add a migration step to convert
legacy string errors into the new structured form before rollout; locate the
SwarmUnhealthy definition and the _Serror type in alert.rs and apply the
compatibility deserializer or migration accordingly.
In `@ui/src/resources/stack/config/index.tsx`:
- Around line 531-546: The non-swarm branch of the Link in the description block
still points to the service/create docs; update the ternary that sets Link's to
(where currSwarmId is checked) so the false/compose branch points to the Docker
Compose "up" docs (e.g. the compose up reference URL) instead of
"docker/service/create"; locate the JSX using currSwarmId and the Link component
inside the description prop and replace only the non-swarm URL.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ad391a6-e043-4058-ad01-b82d251602d9
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockclient/core/ts/yarn.lockis excluded by!**/yarn.lock,!**/*.lockui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (83)
.github/workflows/ci.ymlCargo.tomlbin/binaries.Dockerfilebin/chef.binaries.Dockerfilebin/cli/aio.Dockerfilebin/cli/src/command/execute.rsbin/cli/src/config.rsbin/core/aio.Dockerfilebin/core/src/alert/discord.rsbin/core/src/alert/mod.rsbin/core/src/alert/slack.rsbin/core/src/api/execute/maintenance.rsbin/core/src/api/execute/mod.rsbin/core/src/api/execute/swarm.rsbin/core/src/api/read/toml.rsbin/core/src/api/write/stack.rsbin/core/src/config.rsbin/core/src/connection/server.rsbin/core/src/helpers/procedure.rsbin/core/src/helpers/update.rsbin/core/src/monitor/resources.rsbin/core/src/monitor/swarm.rsbin/core/src/resource/procedure.rsbin/core/src/resource/server.rsbin/core/src/state.rsbin/core/src/sync/file.rsbin/core/src/sync/resources.rsbin/core/src/sync/toml.rsbin/periphery/aio.Dockerfilebin/periphery/src/api/swarm/mod.rsbin/periphery/src/api/swarm/stack.rsbin/periphery/src/config.rsbin/periphery/src/docker/node.rsclient/core/rs/src/api/execute/mod.rsclient/core/rs/src/api/execute/openapi.rsclient/core/rs/src/api/execute/swarm.rsclient/core/rs/src/entities/alert.rsclient/core/rs/src/entities/config/cli/mod.rsclient/core/rs/src/entities/config/core.rsclient/core/rs/src/entities/config/periphery.rsclient/core/rs/src/entities/docker/node.rsclient/core/rs/src/entities/logger.rsclient/core/rs/src/entities/mod.rsclient/core/rs/src/entities/server.rsclient/core/rs/src/entities/swarm.rsclient/core/ts/package.jsonclient/core/ts/src/responses.tsclient/core/ts/src/types.tsclient/core/ts/tsconfig.jsonclient/periphery/rs/src/api/swarm.rsexample/alerter/Dockerfileexample/update_logger/Dockerfilelib/command/src/lib.rslib/environment/src/lib.rsui/package.jsonui/public/client/responses.d.tsui/public/client/types.d.tsui/public/client/types.jsui/src/components/config/organization-selector.tsxui/src/components/config/provider-selector.tsxui/src/components/docker/container-ports.tsxui/src/components/docker/container-selector.tsxui/src/components/docker/containers-section.tsxui/src/components/stack-service-selector.tsxui/src/pages/settings/tags/color-selector.tsxui/src/pages/swarm/node/index.tsxui/src/resources/build/config.tsxui/src/resources/build/info.tsxui/src/resources/deployment/update-available.tsxui/src/resources/procedure/config/executions.tsxui/src/resources/server/index.tsxui/src/resources/stack/config/index.tsxui/src/resources/stack/info.tsxui/src/resources/stack/update-available.tsxui/src/resources/swarm/config.tsxui/src/resources/swarm/docker/nodes/index.tsxui/src/resources/swarm/docker/nodes/update.tsxui/src/resources/swarm/index.tsxui/src/resources/sync/info.tsxui/src/ui/hover-error.tsxui/src/ui/labels-group.tsxui/src/ui/section.tsxui/tsconfig.json
💤 Files with no reviewable changes (4)
- ui/tsconfig.json
- ui/src/resources/deployment/update-available.tsx
- ui/src/resources/stack/update-available.tsx
- ui/src/components/docker/containers-section.tsx
Comment on lines
261
to
264
| let err = err | ||
| .as_ref() | ||
| .map(|e| format!("\nerror: {e}")) | ||
| .map(|e| format!("\nerror: {e:#?}")) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Don't switch alert bodies to Debug formatting.
{e:#?} will send Rust debug output to user-facing channels, which is noisier and can expose internal structure. Keep these messages on Display formatting (or a sanitized string) instead.
Also applies to: 285-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/core/src/alert/mod.rs` around lines 261 - 264, The code is formatting
errors with Debug ("{e:#?}") when building the alert body; change the mapping
closure that produces err (the let err = err.as_ref().map(|e| format!("\nerror:
{e:#?}")).unwrap_or_default();) to use Display/sanitized text instead (e.g.
format!("\nerror: {}", e) or e.to_string()) so user-facing channels receive
clean, non-debug output; apply the same replacement for the other occurrence
that uses "{e:#?}" (the similar map/format block around lines reported).
Comment on lines
+111
to
+129
| let text = match alert.level { | ||
| SeverityLevel::Ok => { | ||
| format!( | ||
| "{level} | *{name}*{region} | Periphery version now matches Core version ✅" | ||
| ) | ||
| } | ||
| _ => { | ||
| format!( | ||
| "{level} | *{name}*{region} | Version mismatch detected ⚠️\nPeriphery: {server_version} | Core: {core_version}" | ||
| ) | ||
| } | ||
| }; | ||
| let blocks = vec![ | ||
| Block::header(text.clone()), | ||
| Block::section(resource_link( | ||
| ResourceTargetVariant::Server, | ||
| id, | ||
| )), | ||
| ]; |
There was a problem hiding this comment.
Keep the mismatch details out of the header block.
Lines 118-120 build a multiline body string, then Line 124 reuses that whole payload as the header. This makes the rich Slack rendering depend on a header-sized block for the version details instead of a normal section, which is much more brittle and harder to scan. Split this into a short header plus a section for the Periphery/Core versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/core/src/alert/slack.rs` around lines 111 - 129, The header currently
uses the full multiline `text` (created in the `match` on `alert.level`) which
places version details into a Slack header; change this so `Block::header`
receives a short one-line `header_text` (e.g., "{level} | *{name}*{region}" with
the ✅ or ⚠️ marker) and build a separate `section_text` for the version details
(e.g., "Periphery: {server_version} | Core: {core_version}") that you pass to
`Block::section`; update the `blocks` vector to include
`Block::header(header_text)` followed by the
`Block::section(resource_link(...))` and another `Block::section(section_text)`
only for the mismatch branch so Ok still shows a short header without extra
section.
Comment on lines
+271
to
299
| // Ensure server / swarm is reachable | ||
| match &swarm_or_server { | ||
| SwarmOrServer::None => continue, | ||
| SwarmOrServer::Server(server) => { | ||
| if !server_status_cache | ||
| .get(&server.id) | ||
| .await | ||
| .map(|s| matches!(s.state, ServerState::Ok)) | ||
| .unwrap_or_default() | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
| SwarmOrServer::Swarm(swarm) => { | ||
| if !swarm_status_cache | ||
| .get(&swarm.id) | ||
| .await | ||
| .map(|s| { | ||
| matches!( | ||
| s.state, | ||
| SwarmState::Healthy | SwarmState::Unhealthy | ||
| ) | ||
| }) | ||
| .unwrap_or_default() | ||
| { | ||
| continue; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the reachability gate into one helper.
These two SwarmOrServer matches are now identical. Keeping the server/swarm “reachable” predicate duplicated here will drift the next time the status rules change. Please centralize it behind a small helper and reuse it for both stacks and deployments.
Also applies to: 351-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/core/src/api/execute/maintenance.rs` around lines 271 - 299, The
reachability check for SwarmOrServer is duplicated; extract it into a helper
function (e.g., is_reachable(swarm_or_server, server_status_cache,
swarm_status_cache) -> bool) that encapsulates the current logic that checks
server_status_cache for ServerState::Ok and swarm_status_cache for
SwarmState::Healthy | SwarmState::Unhealthy, then replace both match blocks (the
one around SwarmOrServer in execute/maintenance.rs and the similar block used
for stacks/deployments) to call that helper and continue when it returns false;
reference SwarmOrServer, server_status_cache, swarm_status_cache,
ServerState::Ok, and SwarmState::Healthy|Unhealthy when implementing the helper.
Comment on lines
+942
to
+945
| let deploy_services = if stack.config.auto_update_all_services | ||
| // Swarm stacks don't support individual service deploy | ||
| || !stack.config.swarm_id.is_empty() | ||
| { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer using resolved execution target over raw config for swarm detection.
This avoids config-drift edge cases and keeps deploy logic coupled to the actual resolved target.
♻️ Proposed refactor
- let deploy_services = if stack.config.auto_update_all_services
- // Swarm stacks don't support individual service deploy
- || !stack.config.swarm_id.is_empty()
+ let deploy_services = if stack.config.auto_update_all_services
+ // Swarm stacks don't support individual service deploy
+ || swarm_or_server.swarm_id().is_some()
{
Vec::new()
} else {📝 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
| let deploy_services = if stack.config.auto_update_all_services | |
| // Swarm stacks don't support individual service deploy | |
| || !stack.config.swarm_id.is_empty() | |
| { | |
| let deploy_services = if stack.config.auto_update_all_services | |
| // Swarm stacks don't support individual service deploy | |
| || swarm_or_server.swarm_id().is_some() | |
| { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/core/src/api/write/stack.rs` around lines 942 - 945, The deploy_services
condition currently checks stack.config.swarm_id directly (alongside
stack.config.auto_update_all_services); replace that raw-config swarm detection
with the stack's resolved execution target instead (e.g., use the resolved
execution target accessor on the Stack such as stack.execution_target /
stack.resolved_target or the helper is_swarm() on the resolved target) so the
clause becomes: if stack.config.auto_update_all_services ||
resolved_target.is_swarm() (keeping the existing Swarm comment), ensuring deploy
logic uses the actual resolved target rather than stack.config.swarm_id to avoid
config-drift edge cases.
Comment on lines
+310
to
+316
| // Move address to external_address if not set. | ||
| // This helps preserve container port link behavior. | ||
| external_address: if !server.config.external_address.is_empty() { | ||
| Some(server.config.address) | ||
| } else { | ||
| None | ||
| }, |
There was a problem hiding this comment.
Logic appears inverted relative to the comment.
The comment states "Move address to external_address if not set," but the condition !server.config.external_address.is_empty() triggers when external_address is already set (non-empty). This seems backwards—if the intent is to preserve the container port link address by moving address to external_address only when external_address hasn't been configured yet, the condition should be:
- external_address: if !server.config.external_address.is_empty() {
+ external_address: if server.config.external_address.is_empty() && !server.config.address.is_empty() {
Some(server.config.address)Please verify the intended behavior.
📝 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
| // Move address to external_address if not set. | |
| // This helps preserve container port link behavior. | |
| external_address: if !server.config.external_address.is_empty() { | |
| Some(server.config.address) | |
| } else { | |
| None | |
| }, | |
| // Move address to external_address if not set. | |
| // This helps preserve container port link behavior. | |
| external_address: if server.config.external_address.is_empty() && !server.config.address.is_empty() { | |
| Some(server.config.address) | |
| } else { | |
| None | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/core/src/connection/server.rs` around lines 310 - 316, The conditional is
inverted: currently it sets external_address to server.config.address only when
server.config.external_address is non-empty; change the logic so
external_address is set to Some(server.config.address) when
server.config.external_address IS empty (i.e., when no external_address has been
configured) and None otherwise. Locate the construction that assigns
external_address (reference server.config.external_address and the
external_address field in the server struct) and flip the condition to check for
emptiness before moving server.config.address into external_address.
Comment on lines
+35
to
+37
| if (Object.keys(portsMap).length > 0) { | ||
| console.log("mapped", portsMap); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the debug console.logs before merging.
These logs run on normal renders and dump port mapping/grouping details into the browser console. They look like leftover debugging noise rather than intentional client-side observability.
Also applies to: 44-46, 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/docker/container-ports.tsx` around lines 35 - 37, Remove
the leftover debug console.log calls in the Docker container ports component:
delete the console.log("mapped", portsMap) inside the render logic and any other
console.log instances noted (around lines handling ports grouping at the block
that uses portsMap and the later mapping/rendering code), i.e., clean up in the
docker/container-ports.tsx component where portsMap is computed and where port
groups are rendered (functions/variables: portsMap, the mapping/render loop that
renders grouped ports), ensuring no console.debug/console.log calls remain
before committing.
| <Group justify="space-between"> | ||
| <Text>{fmtUpperCamelcase(color)}</Text> | ||
| <Box w={25} h={25} bg={"Tag" + color} bdrs="md" /> | ||
| <Box w={25} h={25} bg={`Tag${color}.9`} bdrs="md" /> |
There was a problem hiding this comment.
Use one shared swatch token format for selected + options.
Line 80 uses Tag${color}.9, but the selected swatch (Line 55) still uses Tag${color}. This can show different shades for the same color depending on location.
🎯 Proposed fix
export default function TagColorSelector({
tagId,
color,
disabled,
...comboboxProps
}: TagColorSelectorProps) {
+ const getTagSwatchBg = (c: Types.TagColor) => `Tag${c}.9`;
const { search, setSearch, combobox } = useSearchCombobox();
@@
<Button
onClick={() => combobox.toggleDropdown()}
- rightSection={<Box w={25} h={25} bg={"Tag" + color} bdrs="md" />}
+ rightSection={<Box w={25} h={25} bg={getTagSwatchBg(color)} bdrs="md" />}
loading={isPending}
w={200}
justify="space-between"
>
@@
<Group justify="space-between">
<Text>{fmtUpperCamelcase(color)}</Text>
- <Box w={25} h={25} bg={`Tag${color}.9`} bdrs="md" />
+ <Box w={25} h={25} bg={getTagSwatchBg(color)} bdrs="md" />
</Group>
</Combobox.Option>
))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/pages/settings/tags/color-selector.tsx` at line 80, The option swatch
uses bg={`Tag${color}.9`} while the selected swatch still uses
bg={`Tag${color}`}, causing inconsistent shades; make both use the same shared
token format (either `Tag${color}.9` or `Tag${color}`) so the ColorSelector
renders consistent colors—update the selected swatch usage (the Box/render for
the active selection) to match the option swatch bg token format (or vice versa)
wherever `Tag${color}`/`Tag${color}.9` appears.
| (registry.account ?? "") + | ||
| index | ||
| } | ||
| key={index} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether ImageRegistryConfig keeps local/uncontrolled state
fd -i 'image-registry-config' --extension ts --extension tsx --exec sh -c '
echo "===== {} ====="
sed -n "1,260p" "{}"
'
# Inspect callsites using index keys in registry list rendering
rg -n -C3 --type=tsx 'image_registries\?\.map|key=\{index\}|onRemove'Repository: FarisZR/komodo-agentic-cli
Length of output: 3109
🏁 Script executed:
# Read the config.tsx file around line 245 to see the context
head -n 260 ./ui/src/resources/build/config.tsx | tail -n 40Repository: FarisZR/komodo-agentic-cli
Length of output: 1371
🏁 Script executed:
# Search for image_registries rendering in the file to see the full context
rg -n 'image_registries' ./ui/src/resources/build/config.tsx -B 5 -A 5Repository: FarisZR/komodo-agentic-cli
Length of output: 1881
🏁 Script executed:
# Look at the broader structure to understand the registry data
rg -n 'key=' ./ui/src/resources/build/config.tsx -B 2 -A 2Repository: FarisZR/komodo-agentic-cli
Length of output: 284
🏁 Script executed:
# Check where registry items are created/initialized
rg -n 'image_registry' ./ui/src/resources/build/config.tsx | head -20Repository: FarisZR/komodo-agentic-cli
Length of output: 248
🏁 Script executed:
# Verify ImageRegistryConfig component doesn't have useState or useRef
rg -n 'useState|useRef|useReducer' ./ui/src/components/config/image-registry-config.tsxRepository: FarisZR/komodo-agentic-cli
Length of output: 52
🏁 Script executed:
# Check the full context of how image_registry is defined in the config
rg -n 'image_registry:' ./ui/src/resources/build/config.tsx -A 2 -B 2Repository: FarisZR/komodo-agentic-cli
Length of output: 921
Replace index-based key with a stable per-item identifier.
At line 245, key={index} causes React reconciliation to break during add/remove operations. When a registry item is deleted, all subsequent indices shift, causing React to incorrectly reuse component instances. Since registry objects currently lack a unique ID field, consider adding one (e.g., UUID) or using a stable composite key based on content.
{image_registries?.map((registry, index) => (
<ImageRegistryConfig
key={index} // ← Replace with stable ID
registry={registry}
...
/>
))}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/resources/build/config.tsx` at line 245, The list rendering uses
index-based keys in the image_registries.map that causes incorrect
reconciliation; change the mapping that renders <ImageRegistryConfig> to use a
stable per-item identifier instead of key={index} — add a unique id (e.g., UUID)
to each registry object when created/loaded or derive a stable composite key
from registry properties (like registry.url + registry.name) and use that value
as the key; update any creation code that builds image_registries to populate
the id and ensure ImageRegistryConfig remains keyed by that id.
Comment on lines
+43
to
+55
| titleRight={ | ||
| <UpdateSwarmNode swarm={id} nodes={Object.keys(selectState[0])} /> | ||
| } | ||
| actions={<SearchInput value={search} onSearch={setSearch} />} | ||
| > | ||
| <DataTable | ||
| tableKey="swarm-nodes" | ||
| data={filtered} | ||
| selectOptions={{ | ||
| selectKey: (node) => | ||
| node.Name ?? node.Hostname ?? node.ID ?? "Unknown", | ||
| state: selectState, | ||
| }} |
There was a problem hiding this comment.
Use the node ID as the selection key.
Line 44 forwards the selection keys to UpdateSwarmNode, but Lines 52-53 derive those keys from Name/Hostname before ID. Those fields are display-oriented and can collide, and the "Unknown" fallback guarantees collisions when metadata is missing. Key the rows by ID so multi-select cannot collapse rows or update the wrong node.
🔧 Suggested change
<DataTable
tableKey="swarm-nodes"
data={filtered}
selectOptions={{
- selectKey: (node) =>
- node.Name ?? node.Hostname ?? node.ID ?? "Unknown",
+ selectKey: (node) =>
+ node.ID ?? node.Name ?? node.Hostname ?? "Unknown",
state: selectState,
}}📝 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
| titleRight={ | |
| <UpdateSwarmNode swarm={id} nodes={Object.keys(selectState[0])} /> | |
| } | |
| actions={<SearchInput value={search} onSearch={setSearch} />} | |
| > | |
| <DataTable | |
| tableKey="swarm-nodes" | |
| data={filtered} | |
| selectOptions={{ | |
| selectKey: (node) => | |
| node.Name ?? node.Hostname ?? node.ID ?? "Unknown", | |
| state: selectState, | |
| }} | |
| <DataTable | |
| tableKey="swarm-nodes" | |
| data={filtered} | |
| selectOptions={{ | |
| selectKey: (node) => | |
| node.ID ?? node.Name ?? node.Hostname ?? "Unknown", | |
| state: selectState, | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/resources/swarm/docker/nodes/index.tsx` around lines 43 - 55, The
DataTable is using a non-unique selection key (selectOptions.selectKey) that
prefers Name/Hostname and falls back to "Unknown", causing collisions and
mis-selection; change selectOptions.selectKey to return the node's ID (node.ID)
as the primary and only key so selections and UpdateSwarmNode (prop swarm={id}
nodes={...}) map to unique rows; update the selectKey implementation referenced
in DataTable/selectOptions to use node.ID (and only fallback to a safe unique
value if ID is truly missing) so multi-select and updates target the correct
nodes.
Comment on lines
+49
to
+57
| const { mutate: update, isPending: updatePending } = useExecute( | ||
| "UpdateSwarmNode", | ||
| { | ||
| onSuccess: () => { | ||
| close(); | ||
| resetForm(); | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Wait for the whole batch before closing the modal.
update() is fired once per node, but the shared onSuccess closes and resets the form after the first success. If later node updates fail, the dialog is already gone and the user gets a false “all done” signal. Please aggregate the per-node requests and only close/reset after the batch has fully succeeded.
Also applies to: 66-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/resources/swarm/docker/nodes/update.tsx` around lines 49 - 57, The
onSuccess handler for the per-node mutate from useExecute("UpdateSwarmNode")
(the mutate alias update()) closes the modal and calls resetForm() immediately
on the first successful request, which hides the dialog before the whole batch
completes; change the logic so that you aggregate per-node updates (e.g.,
collect the promises returned by multiple update(...) calls and await
Promise.all or maintain a counter/promise that resolves when every node's mutate
has succeeded), only invoke close() and resetForm() after the entire batch
Promise resolves successfully, and surface any per-node failures instead of
closing; apply the same aggregation fix to the other similar useExecute block
around the 66-79 region.
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
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
v2.1.1, including the required workspace, client, core, periphery, and CLI updatesssh/execcommand behavior with remote exit-code passthroughagentic-cliPRs while keeping cargo cache disabled to avoid disk pressure during larger workspace buildsValidation
cargo fmt --allcargo build --workspace --all-targetscargo test --workspacecargo run -p komodo_cli -- --helpcargo run -p komodo_cli -- exec --helpcargo run -p komodo_cli -- ssh --helpcargo run -p komodo_cli -- stack --helpcargo run -p komodo_cli -- sync --helpcargo run -p komodo_cli -- procedure --helpcargo run -p komodo_cli -- variable --helpcargo run -p komodo_cli -- configSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements