Add compose service selection UI and improve template import flow#18
Conversation
Add support for importing individual Docker Compose services into blueprints and improve generator behavior. Introduces resourceFromComposeService, default service port handling, and unique blueprint name generation; preserves disabled compose services (no Pangolin labels) and retains existing network syntax when injecting the Pangolin network. Update Blueprint and Compose builders to batch-import services/networks/volumes, expose available compose services UI, and fix selection/indexing when adding/removing resources or services. Add tests for template modal and blueprint generator/validator changes, and adjust env output header formatting and template-store state updates after import.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds compose-version sanitization and a validation framework, exports compose→blueprint service conversion helpers with name/port handling, wires sanitization/validation across builders/hooks/components, expands blueprint types/validation, and adds tests plus a Vitest config. ChangesValidation, Compose Sanitization, and Multi-service Builder
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the blueprint and compose builders to support importing all services from a Compose file, regardless of port definitions, and introduces a UI section for adding available services as resources. It includes a localStorage polyfill for tests, new component tests, and refactored resource management logic. Reviewers suggested improving the robustness of slug generation and centralizing name de-duplication by exporting and reusing the uniqueBlueprintName utility function.
| ? (rawService as Record<string, unknown>) | ||
| : {}; | ||
| const port = firstServicePort(svc) ?? DEFAULT_SERVICE_PORT; | ||
| const sluggedKey = slug(serviceKey) || serviceKey.toLowerCase() || "service"; |
There was a problem hiding this comment.
The fallback logic for sluggedKey might still produce invalid blueprint names (e.g., if serviceKey contains only spaces or symbols). Since slug already handles lowercasing and character replacement, falling back directly to a safe default like "service" is more robust.
| const sluggedKey = slug(serviceKey) || serviceKey.toLowerCase() || "service"; | |
| const sluggedKey = slug(serviceKey) || "service"; |
| }; | ||
| } | ||
|
|
||
| function uniqueBlueprintName(base: string, seen: Set<string>): string { |
There was a problem hiding this comment.
Exporting uniqueBlueprintName allows it to be reused in the BlueprintBuilderRoute to handle resource name de-duplication consistently across the application.
| function uniqueBlueprintName(base: string, seen: Set<string>): string { | |
| export function uniqueBlueprintName(base: string, seen: Set<string>): string { |
| defaultResource, | ||
| defaultBlueprint, | ||
| fromCompose, | ||
| resourceFromComposeService, |
There was a problem hiding this comment.
Import uniqueBlueprintName to use it for resource name de-duplication.
| resourceFromComposeService, | |
| resourceFromComposeService, | |
| uniqueBlueprintName, |
| const existingNames = new Set(bp.resources.map((r) => r.blueprintName)); | ||
| const resource = resourceFromComposeService(serviceName, rawService); | ||
| const baseName = resource.blueprintName; | ||
| let nextName = baseName; | ||
| let suffix = 2; | ||
| while (existingNames.has(nextName)) { | ||
| nextName = `${baseName}-${suffix}`; | ||
| suffix += 1; | ||
| } | ||
| resource.blueprintName = nextName; |
There was a problem hiding this comment.
Use the uniqueBlueprintName utility function instead of manually implementing the de-duplication logic. This reduces code duplication and ensures consistent behavior with the fromCompose generator.
| const existingNames = new Set(bp.resources.map((r) => r.blueprintName)); | |
| const resource = resourceFromComposeService(serviceName, rawService); | |
| const baseName = resource.blueprintName; | |
| let nextName = baseName; | |
| let suffix = 2; | |
| while (existingNames.has(nextName)) { | |
| nextName = `${baseName}-${suffix}`; | |
| suffix += 1; | |
| } | |
| resource.blueprintName = nextName; | |
| const existingNames = new Set(bp.resources.map((r) => r.blueprintName)); | |
| const resource = resourceFromComposeService(serviceName, rawService); | |
| resource.blueprintName = uniqueBlueprintName(resource.blueprintName, existingNames); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/routes/docker/compose-builder.tsx (1)
429-465:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImported services can collide with existing service names, producing invalid compose YAML.
importTemplateconcatenatesimportedServicesonto the filteredexistinglist without checking for name collisions. If the imported template defines a service whosenamematches one already inservices(e.g., user already has annginxservice and imports annginxtemplate), the resulting compose document will contain duplicate top-level service keys, which is invalid and will likely overwrite/conflict during YAML serialization.The sibling
blueprint-builder.tsxroute uses auniqueBlueprintNamehelper to deduplicate against existing names — please mirror that behavior here (e.g., append-2,-3, … to colliding names before merging).Also note:
firstImportedIdxis derived from outer-scopeserviceswhilesetServicesuses theprevsnapshot. They will agree in practice today, but ifservicesandprevever diverge (concurrent updates), the post-import selection index will point at the wrong row. Consider computing the index inside the functional updater and applying selection in a follow-up effect, or capturing the resolved value via a ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/docker/compose-builder.tsx` around lines 429 - 465, importTemplate currently merges importedServices onto existing services without deduplicating names and computes firstImportedIdx from outer-scope services which can diverge from the functional updater; update importTemplate to (1) deduplicate imported service names before merging by reusing the same strategy as uniqueBlueprintName (append -2, -3, … until unique against existing names), (2) perform the name-uniquing and the merge inside the setServices functional updater (use the prev snapshot to build existing and produce next), and (3) compute the firstImportedIdx from that same updater result (or store it into a ref) so that selectService is called with the correct index after setServices completes; keep bulkAddNetworks/bulkAddVolumes and templateStore updates as-is.src/utils/blueprint/__tests__/generator.test.ts (1)
12-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name doesn't match its assertion.
Line 12 says "creates one resource per compose service", but line 23 now expects 2 resources. The test correctly validates that both
webanddbservices are converted to resources, but the name is misleading.📝 Suggested fix
- it("creates one resource per compose service", () => { + it("creates one resource per compose service (multiple services)", () => {or more explicitly:
- it("creates one resource per compose service", () => { + it("converts each compose service to a blueprint resource", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/blueprint/__tests__/generator.test.ts` around lines 12 - 33, The test title is misleading: the `it("creates one resource per compose service", ...)` block asserts two resources from `fromCompose(compose, "example.com")` (checking `bp.resources[0]` and `bp.resources[1]`), so update the test name to reflect that it validates conversion of each compose service into a resource (e.g., change to "creates resources for each compose service" or "creates one resource per compose service (each service -> resource)"); keep the assertions using `fromCompose` and the `bp.resources` checks as-is.src/utils/blueprint/generator.ts (1)
289-311:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftServices removed from blueprint resources retain old Pangolin labels.
When a service exists in the stored compose document but is no longer in
bp.resources, the loop at lines 289–311 skips it entirely. This leaves any previously-injected Pangolin labels intact, causing Pangolin to incorrectly continue exposing services marked as disabled.The test at
__tests__/generator.test.ts:90–106doesn't catch this because it only tests fresh compose documents without prior labels—it doesn't verify the round-trip scenario where a service is converted to a resource (adding labels), then removed from resources (which should clean up those labels).To fix, iterate through all services in
baseDoc.servicesand remove Pangolin labels from any service not inbp.resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/blueprint/generator.ts` around lines 289 - 311, The loop over bp.resources currently injects Pangolin labels but never removes them from services that were previously present in baseDoc.services but are no longer in bp.resources; update the generator to iterate all entries in baseDoc.services and for each service name not present in bp.resources remove Pangolin labels by calling removePangolinResourceLabels(labelsToArray(svc.labels), <resourceName>) and write back the cleaned labels (use dedupePreserveOrder as needed) so stale Pangolin labels are cleared; reference the existing variables and helpers used in the diff (services, baseDoc.services, bp.resources, removePangolinResourceLabels, labelsToArray, dedupePreserveOrder) to locate where to add this cleanup after or before the current bp.resources loop.src/routes/blueprint-builder.tsx (1)
134-153: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDestructure
setTemplateDetailOpen,setTemplateStoreOpen,setSelectedTemplate, andsetTemplateDetailTabfromuseTemplateStoreat the component level.The
useTemplateStorehook returns a new object literal on every render without memoization. SinceimportTemplateincludes the entiretemplateStoreobject in its dependency array, this causes the callback to be unnecessarily recreated every render. Destructure the specific methods you use instead and include only those in the dependencies:const { setTemplateDetailOpen, setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab, } = useTemplateStore();Then update
importTemplatedependencies to[blueprint.baseDomain, setTemplateDetailOpen, setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab, toast].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/blueprint-builder.tsx` around lines 134 - 153, The importTemplate callback depends on the entire templateStore object which is recreated each render; destructure the specific setters from useTemplateStore at the component level (setTemplateDetailOpen, setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab) and use those names inside importTemplate instead of templateStore, then update the importTemplate dependency array to [blueprint.baseDomain, setTemplateDetailOpen, setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab, toast] (remove templateStore) so the callback is stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/templates/__tests__/TemplateDetailModal.test.tsx`:
- Around line 35-47: Add a complementary test to TemplateDetailModal.test.tsx
that simulates a failing import by using onImport =
vi.fn().mockRejectedValue(new Error("boom")), renderModal({ onOpenChange,
onImport }), trigger the import via user.click(screen.getByRole("button", {
name: /import template/i })), then assert that onImport was called with
VALID_TEMPLATE, that onOpenChange was NOT called with false, and that an error
toast (or the modal error UI) is shown; reference the existing helpers and
symbols (renderModal, VALID_TEMPLATE, onImport, onOpenChange, userEvent/screen)
when adding the test to ensure the rejected promise path keeps the modal open
and surfaces an error.
- Around line 18-33: The test helper renderModal currently builds defaultProps
(including onOpenChange and onImport) and returns merged, but tests use
outer-scope mocks instead of the helper's returned handlers; update renderModal
(used with TemplateDetailModal) to return the merged props or at least the
handler mocks so callers can assert the actual functions used: after rendering,
return merged (or return { ...merged, onOpenChange: merged.onOpenChange,
onImport: merged.onImport }) so tests can destructure the real
onOpenChange/onImport from the helper and not rely on external variables.
In `@src/utils/blueprint/__tests__/generator.test.ts`:
- Around line 90-106: Add a round-trip test that ensures Pangolin labels are
removed from services disabled after an export/import cycle: use
fromCompose(...) and toComposeYaml(...) to first import the initial compose
(pass a hostname like "example.com" so labels get added), call toComposeYaml(bp)
and assert both services have pangolin.public-resources labels, then re-import
via fromCompose(yaml, "example.com"), remove the "worker" resource from
bp2.resources (filter by serviceContainerName !== "worker"), re-export with
toComposeYaml(bp2) and assert the final YAML still contains
pangolin.public-resources.web but does NOT contain
pangolin.public-resources.worker; place this new test in generator.test.ts to
cover the cleanup path related to the logic in generator.ts that removes old
labels on re-export.
In `@src/utils/blueprint/generator.ts`:
- Around line 300-308: The else branch for svc.networks currently silently
replaces invalid network values with ["pangolin"]; update that branch in the
svc.networks handling block to log a warning (using the available logger or
console.warn) that includes the original svc.networks value and explains it was
replaced with ["pangolin"], or alternatively add a clear comment explaining this
defensive fallback; modify the code within the else branch that sets
svc.networks = ["pangolin"] (referencing svc.networks and "pangolin") to emit
that warning before overwriting.
---
Outside diff comments:
In `@src/routes/blueprint-builder.tsx`:
- Around line 134-153: The importTemplate callback depends on the entire
templateStore object which is recreated each render; destructure the specific
setters from useTemplateStore at the component level (setTemplateDetailOpen,
setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab) and use those
names inside importTemplate instead of templateStore, then update the
importTemplate dependency array to [blueprint.baseDomain, setTemplateDetailOpen,
setTemplateStoreOpen, setSelectedTemplate, setTemplateDetailTab, toast] (remove
templateStore) so the callback is stable.
In `@src/routes/docker/compose-builder.tsx`:
- Around line 429-465: importTemplate currently merges importedServices onto
existing services without deduplicating names and computes firstImportedIdx from
outer-scope services which can diverge from the functional updater; update
importTemplate to (1) deduplicate imported service names before merging by
reusing the same strategy as uniqueBlueprintName (append -2, -3, … until unique
against existing names), (2) perform the name-uniquing and the merge inside the
setServices functional updater (use the prev snapshot to build existing and
produce next), and (3) compute the firstImportedIdx from that same updater
result (or store it into a ref) so that selectService is called with the correct
index after setServices completes; keep bulkAddNetworks/bulkAddVolumes and
templateStore updates as-is.
In `@src/utils/blueprint/__tests__/generator.test.ts`:
- Around line 12-33: The test title is misleading: the `it("creates one resource
per compose service", ...)` block asserts two resources from
`fromCompose(compose, "example.com")` (checking `bp.resources[0]` and
`bp.resources[1]`), so update the test name to reflect that it validates
conversion of each compose service into a resource (e.g., change to "creates
resources for each compose service" or "creates one resource per compose service
(each service -> resource)"); keep the assertions using `fromCompose` and the
`bp.resources` checks as-is.
In `@src/utils/blueprint/generator.ts`:
- Around line 289-311: The loop over bp.resources currently injects Pangolin
labels but never removes them from services that were previously present in
baseDoc.services but are no longer in bp.resources; update the generator to
iterate all entries in baseDoc.services and for each service name not present in
bp.resources remove Pangolin labels by calling
removePangolinResourceLabels(labelsToArray(svc.labels), <resourceName>) and
write back the cleaned labels (use dedupePreserveOrder as needed) so stale
Pangolin labels are cleared; reference the existing variables and helpers used
in the diff (services, baseDoc.services, bp.resources,
removePangolinResourceLabels, labelsToArray, dedupePreserveOrder) to locate
where to add this cleanup after or before the current bp.resources loop.
🪄 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: 2a942a39-926b-4866-b7aa-c9f9c5539c96
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
src/__tests__/setup.tssrc/components/templates/__tests__/TemplateDetailModal.test.tsxsrc/routes/blueprint-builder.tsxsrc/routes/docker/compose-builder.tsxsrc/utils/blueprint/__tests__/generator.test.tssrc/utils/blueprint/__tests__/validator.test.tssrc/utils/blueprint/generator.ts
📜 Review details
⏰ 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: build-and-push
🔇 Additional comments (17)
src/routes/docker/compose-builder.tsx (2)
409-425: LGTM!
469-472: LGTM!Also applies to: 531-531
src/__tests__/setup.ts (1)
3-35: LGTM!src/utils/blueprint/generator.ts (7)
221-229: LGTM!
231-237: LGTM!
132-153: LGTM!
155-165: LGTM!
21-21: LGTM!
186-194: LGTM!
347-347: LGTM!src/utils/blueprint/__tests__/generator.test.ts (1)
244-245: LGTM!src/utils/blueprint/__tests__/validator.test.ts (1)
78-86: LGTM!src/routes/blueprint-builder.tsx (5)
61-75: LGTM!
97-112: LGTM!
114-132: LGTM!
258-286: LGTM!
21-21:resourceFromComposeServiceis correctly exported from the generator module.The export exists at line 132 of
src/utils/blueprint/generator.tsand the import at line 21 is valid.
| function renderModal(props: Partial<Parameters<typeof TemplateDetailModal>[0]> = {}) { | ||
| const defaultProps = { | ||
| open: true, | ||
| onOpenChange: vi.fn(), | ||
| template: VALID_TEMPLATE, | ||
| onImport: vi.fn().mockResolvedValue(undefined), | ||
| }; | ||
|
|
||
| const merged = { ...defaultProps, ...props }; | ||
| render( | ||
| <ToastProvider> | ||
| <TemplateDetailModal {...merged} /> | ||
| </ToastProvider>, | ||
| ); | ||
| return merged; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: renderModal ignores caller-supplied overrides for assertions.
renderModal returns merged, but the test ignores the return value and asserts against the outer-scope onOpenChange/onImport mocks. That works only because the same references are spread into props. If a future test relies on the helper's defaults (e.g., calls renderModal() with no args), it would have no handle on defaultProps.onOpenChange to assert against. Returning the resolved mocks and using them in the test (or destructuring from the return) would make the helper fully self-contained.
♻️ Suggested tweak
- const onOpenChange = vi.fn();
- const onImport = vi.fn().mockResolvedValue(undefined);
- renderModal({ onOpenChange, onImport });
+ const { onOpenChange, onImport } = renderModal();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/templates/__tests__/TemplateDetailModal.test.tsx` around lines
18 - 33, The test helper renderModal currently builds defaultProps (including
onOpenChange and onImport) and returns merged, but tests use outer-scope mocks
instead of the helper's returned handlers; update renderModal (used with
TemplateDetailModal) to return the merged props or at least the handler mocks so
callers can assert the actual functions used: after rendering, return merged (or
return { ...merged, onOpenChange: merged.onOpenChange, onImport: merged.onImport
}) so tests can destructure the real onOpenChange/onImport from the helper and
not rely on external variables.
| describe("TemplateDetailModal", () => { | ||
| it("closes after a successful import", async () => { | ||
| const user = userEvent.setup(); | ||
| const onOpenChange = vi.fn(); | ||
| const onImport = vi.fn().mockResolvedValue(undefined); | ||
| renderModal({ onOpenChange, onImport }); | ||
|
|
||
| await user.click(screen.getByRole("button", { name: /import template/i })); | ||
|
|
||
| expect(onImport).toHaveBeenCalledWith(VALID_TEMPLATE); | ||
| expect(onOpenChange).toHaveBeenCalledWith(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Optional: add a failure-path test.
The current spec only validates the happy path. Consider adding a complementary test where onImport rejects (e.g., vi.fn().mockRejectedValue(new Error("boom"))) and asserts that onOpenChange(false) is not called and that the modal surfaces an error toast. That guards the documented contract that a failed import keeps the modal open — which is the actual regression risk this test is meant to protect against.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/templates/__tests__/TemplateDetailModal.test.tsx` around lines
35 - 47, Add a complementary test to TemplateDetailModal.test.tsx that simulates
a failing import by using onImport = vi.fn().mockRejectedValue(new
Error("boom")), renderModal({ onOpenChange, onImport }), trigger the import via
user.click(screen.getByRole("button", { name: /import template/i })), then
assert that onImport was called with VALID_TEMPLATE, that onOpenChange was NOT
called with false, and that an error toast (or the modal error UI) is shown;
reference the existing helpers and symbols (renderModal, VALID_TEMPLATE,
onImport, onOpenChange, userEvent/screen) when adding the test to ensure the
rejected promise path keeps the modal open and surfaces an error.
| // Ensure pangolin network membership without discarding existing syntax. | ||
| if (Array.isArray(svc.networks)) { | ||
| const nets = svc.networks as unknown[]; | ||
| svc.networks = nets.includes("pangolin") ? nets : [...nets, "pangolin"]; | ||
| } else if (isPlainObject(svc.networks)) { | ||
| svc.networks = { ...svc.networks, pangolin: {} }; | ||
| } else { | ||
| svc.networks = nets; | ||
| svc.networks = ["pangolin"]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Network handling could be more explicit about invalid input.
The else branch at line 307 overwrites svc.networks with ["pangolin"] regardless of its current value. While this defensively handles invalid syntax (e.g., if networks were a string or number), it's implicit. Invalid network configurations would be silently corrected without any indication to the user.
For better observability, consider logging when invalid network syntax is encountered and corrected, or add a comment explaining the defensive fallback behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/blueprint/generator.ts` around lines 300 - 308, The else branch for
svc.networks currently silently replaces invalid network values with
["pangolin"]; update that branch in the svc.networks handling block to log a
warning (using the available logger or console.warn) that includes the original
svc.networks value and explains it was replaced with ["pangolin"], or
alternatively add a clear comment explaining this defensive fallback; modify the
code within the else branch that sets svc.networks = ["pangolin"] (referencing
svc.networks and "pangolin") to emit that warning before overwriting.
hhftechnology
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-builder/CodePanel.tsx`:
- Around line 81-84: The copy actions currently only check validatedOutput.ok,
allowing copies when a validationError is active; update the copy guard to also
check activeValidationError and block copying when it is truthy. Specifically,
in the CodePanel copy handlers where validatedOutput is checked (the branches
that call copyToClipboard and setCopied), change the condition to require
validatedOutput.ok && !activeValidationError so copying and setCopied are
prevented while activeValidationError is set; apply the same change to the other
copy/disable sites in this file that use validatedOutput (the other handlers
referenced near the same blocks and the action at the 143-area) to keep behavior
consistent with the displayed validation state.
In `@src/routes/config-builder.tsx`:
- Around line 98-103: The fieldError function is currently gated on
itemSubmitted which prevents any validation messages from showing because the "+
Add Item" button is disabled for invalid items; update fieldError (and the
similar helper at 276-281) to return validation messages when
currentItemValidation.issues contains an issue for the given path (e.g., check
currentItemValidation.issues.find(...) != null) instead of requiring
itemSubmitted, while keeping the addItem() validation guard unchanged so invalid
inserts remain blocked; ensure you reference and update the fieldError helper
function and its twin helper to use the presence of currentItemValidation.issues
rather than itemSubmitted.
In `@src/utils/blueprint/generator.ts`:
- Around line 232-238: The current removePangolinResourceLabels implementation
only removes labels starting with the exact
`pangolin.public-resources.${blueprintName}.` prefix so stale labels from
previous blueprint names remain; update `removePangolinResourceLabels` to drop
any label that begins with `pangolin.public-resources.` (i.e., ignore the
blueprint segment) so all pangolin public-resource labels are removed regardless
of name, and apply the same change to the other occurrence around the `295-299`
region (the other helper/usage referenced in the diff) so both locations
consistently filter out any label starting with `pangolin.public-resources.`.
In `@src/utils/compose-version.ts`:
- Around line 6-14: The TOP_LEVEL_VERSION_RE regex in compose-version.ts is
complex and lacks documentation; add a concise comment above the
TOP_LEVEL_VERSION_RE declaration explaining what the pattern matches (optional
BOM, top-level non-indented "version:" token with flexible spacing, optional
quoted/unquoted value, optional inline comment, and CRLF/LF or EOF terminator)
so future readers can understand its intent and edge cases used by
sanitizeComposeVersion.
In `@src/utils/validation/scheduler.ts`:
- Around line 164-170: The current check only verifies parsed and 'on' exists
but not that a schedule trigger with cron is present; update the condition
around parsed to require parsed.on.schedule to be an array containing at least
one object with a string cron property and push the same validationError
otherwise. Concretely, replace the loose check using ("on" in parsed) with a
structural check like parsed && typeof parsed === "object" && parsed.on &&
parsed.on.schedule && Array.isArray(parsed.on.schedule) &&
parsed.on.schedule.some(s => s && typeof s === "object" && typeof s.cron ===
"string"), and if that fails call validationError("scheduler.output",
"githubActions", "GitHub Actions output must include an on.schedule trigger").
Ensure you reference the same variables used in the diff (parsed, issues,
validationError) when implementing the change.
In `@vitest.validation.config.ts`:
- Line 10: The vitest.validation.config.ts includes the wrong golden test path
string "src/utils/__tests__/generators.golden.test.ts" so the changed golden
test under src/utils/blueprint/__tests__/golden.test.ts will be skipped; update
the include list in vitest.validation.config.ts to reference the correct test
path (replace the incorrect string with
"src/utils/blueprint/__tests__/golden.test.ts" or add that path to the array of
test files) so the new golden test is picked up by validation.
🪄 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: 7886a947-f3cb-4db9-957a-7d2cad3bc708
📒 Files selected for processing (30)
src/components/ConversionDialog.tsxsrc/components/__tests__/ServicesTab.test.tsxsrc/components/compose-builder/CodePanel.tsxsrc/components/compose-builder/ConfigColumn/ServicesTab.tsxsrc/components/templates/TemplateDetailModal.tsxsrc/hooks/useConversionDialog.tssrc/hooks/useTemplateStore.tssrc/routes/blueprint-builder.tsxsrc/routes/config-builder.tsxsrc/routes/docker/compose-builder.tsxsrc/routes/scheduler-builder.tsxsrc/utils/__tests__/compose-version.test.tssrc/utils/__tests__/template-import.test.tssrc/utils/blueprint/__tests__/golden.test.tssrc/utils/blueprint/generator.tssrc/utils/compose-version.tssrc/utils/template-import.tssrc/utils/validation/__tests__/compose.test.tssrc/utils/validation/__tests__/config.test.tssrc/utils/validation/__tests__/outputs-extra.test.tssrc/utils/validation/__tests__/outputs.test.tssrc/utils/validation/__tests__/scheduler.test.tssrc/utils/validation/__tests__/templates.test.tssrc/utils/validation/compose.tssrc/utils/validation/config.tssrc/utils/validation/issues.tssrc/utils/validation/outputs.tssrc/utils/validation/scheduler.tssrc/utils/validation/templates.tsvitest.validation.config.ts
📜 Review details
⏰ 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: build-and-push
🔇 Additional comments (21)
src/components/compose-builder/ConfigColumn/ServicesTab.tsx (1)
58-60: LGTM!src/utils/validation/__tests__/config.test.ts (1)
1-57: LGTM!src/utils/__tests__/compose-version.test.ts (1)
1-47: LGTM!src/utils/__tests__/template-import.test.ts (1)
6-16: LGTM!src/components/__tests__/ServicesTab.test.tsx (1)
8-23: LGTM!Also applies to: 25-40, 42-60
src/utils/validation/__tests__/templates.test.ts (1)
1-57: LGTM!src/utils/validation/__tests__/compose.test.ts (1)
6-13: LGTM!src/utils/template-import.ts (1)
2-2: LGTM!Also applies to: 379-379
vitest.validation.config.ts (1)
1-9: LGTM!Also applies to: 11-29
src/utils/validation/templates.ts (1)
1-64: LGTM!src/utils/blueprint/__tests__/golden.test.ts (1)
24-24: LGTM!src/utils/validation/__tests__/scheduler.test.ts (1)
1-63: LGTM!src/hooks/useTemplateStore.ts (1)
1-18: LGTM!Also applies to: 23-23, 30-31, 39-40, 56-67, 80-80, 102-115, 155-187
src/utils/validation/config.ts (1)
1-119: LGTM!src/components/ConversionDialog.tsx (1)
11-11: LGTM!Also applies to: 20-20, 55-55, 65-65, 73-73, 83-83, 91-91, 107-112, 134-134, 138-138
src/routes/blueprint-builder.tsx (1)
61-75: LGTM!Also applies to: 99-112, 114-132, 142-146, 259-286, 455-462
src/utils/validation/__tests__/outputs-extra.test.ts (1)
9-60: LGTM!src/utils/validation/issues.ts (1)
3-78: LGTM!src/utils/validation/__tests__/outputs.test.ts (1)
9-55: LGTM!src/utils/validation/compose.ts (1)
35-41: LGTM!Also applies to: 45-45, 69-69
src/hooks/useConversionDialog.ts (1)
2-12: LGTM!Also applies to: 18-20, 27-29, 36-37, 42-56, 67-67
| const TOP_LEVEL_VERSION_RE = /^(?:\uFEFF)?version\s*:\s*(?:"[^"]*"|'[^']*'|[^\r\n#]*)?(?:\s+#.*)?(?:\r?\n|$)/m; | ||
|
|
||
| export function sanitizeComposeVersion(content: string): ComposeVersionSanitization { | ||
| const sanitized = content.replace(TOP_LEVEL_VERSION_RE, ""); | ||
| return { | ||
| content: sanitized, | ||
| removed: sanitized !== content, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding a comment explaining the regex pattern.
The regex TOP_LEVEL_VERSION_RE is complex and matches several edge cases (BOM, various quote styles, inline comments, CRLF/LF). A brief comment documenting what it matches would help maintainability.
Example:
// Matches top-level (non-indented) version declarations:
// - Optional BOM (\uFEFF)
// - "version:" with flexible spacing
// - Optional quoted/unquoted value
// - Optional inline comment (requires space before #)
// - Line terminator or EOF
const TOP_LEVEL_VERSION_RE = /^(?:\uFEFF)?version\s*:\s*(?:"[^"]*"|'[^']*'|[^\r\n#]*)?(?:\s+#.*)?(?:\r?\n|$)/m;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/compose-version.ts` around lines 6 - 14, The TOP_LEVEL_VERSION_RE
regex in compose-version.ts is complex and lacks documentation; add a concise
comment above the TOP_LEVEL_VERSION_RE declaration explaining what the pattern
matches (optional BOM, top-level non-indented "version:" token with flexible
spacing, optional quoted/unquoted value, optional inline comment, and CRLF/LF or
EOF terminator) so future readers can understand its intent and edge cases used
by sanitizeComposeVersion.
| setupFiles: ["./src/__tests__/setup.ts"], | ||
| include: [ | ||
| "src/utils/validation/**/*.{test,spec}.ts", | ||
| "src/utils/__tests__/generators.golden.test.ts", |
There was a problem hiding this comment.
Fix the golden test include path in validation config.
Line 10 references src/utils/__tests__/generators.golden.test.ts, but the changed golden test is src/utils/blueprint/__tests__/golden.test.ts. This config will skip that test.
Suggested diff
include: [
"src/utils/validation/**/*.{test,spec}.ts",
- "src/utils/__tests__/generators.golden.test.ts",
+ "src/utils/blueprint/__tests__/golden.test.ts",
],📝 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.
| "src/utils/__tests__/generators.golden.test.ts", | |
| include: [ | |
| "src/utils/validation/**/*.{test,spec}.ts", | |
| "src/utils/blueprint/__tests__/golden.test.ts", | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vitest.validation.config.ts` at line 10, The vitest.validation.config.ts
includes the wrong golden test path string
"src/utils/__tests__/generators.golden.test.ts" so the changed golden test under
src/utils/blueprint/__tests__/golden.test.ts will be skipped; update the include
list in vitest.validation.config.ts to reference the correct test path (replace
the incorrect string with "src/utils/blueprint/__tests__/golden.test.ts" or add
that path to the array of test files) so the new golden test is picked up by
validation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/validation/templates.ts`:
- Line 49: The composeContent schema currently uses z.string().min(1) which
allows whitespace-only strings; update the schema for composeContent in
templates.ts to reject whitespace-only input by either chaining .trim() before
.min (e.g., z.string().trim().min(1, "Template compose content is required")) or
adding a .refine/transform that checks s.trim().length > 0 and returns the same
error message; modify the composeContent definition accordingly.
- Around line 3-7: The optionalUrl preprocess currently checks value.trim() only
for emptiness but passes the untrimmed string to z.string().url().safeParse,
which rejects valid URLs with surrounding whitespace; update the preprocess to
trim the input before validation and only return undefined when the trimmed
value is empty, i.e., use the trimmed value for the safeParse call and for the
value returned to the schema (keep the surrounding optional() behavior intact),
referencing the optionalUrl constant and its z.preprocess handler.
🪄 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: 43285d66-a218-4a64-97c3-562a1229c2ed
📒 Files selected for processing (3)
src/hooks/__tests__/useTemplateStore.test.tssrc/utils/validation/__tests__/templates.test.tssrc/utils/validation/templates.ts
📜 Review details
⏰ 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: build-and-push
🔇 Additional comments (3)
src/utils/validation/__tests__/templates.test.ts (1)
7-75: LGTM!src/hooks/__tests__/useTemplateStore.test.ts (1)
55-77: LGTM!src/utils/validation/templates.ts (1)
1-2: LGTM!Also applies to: 8-48, 50-75
| const optionalUrl = z.preprocess((value) => { | ||
| if (typeof value !== "string" || value.trim() === "") return undefined; | ||
| const result = z.string().url().safeParse(value); | ||
| return result.success ? result.data : undefined; | ||
| }, z.string().url().optional()); |
There was a problem hiding this comment.
Trim URL input before URL validation to avoid dropping valid links.
optionalUrl checks trim() only for emptiness, then validates the untrimmed string. Inputs like " https://example.com " are treated as invalid and removed.
Proposed fix
+const urlSchema = z.string().url();
+
const optionalUrl = z.preprocess((value) => {
- if (typeof value !== "string" || value.trim() === "") return undefined;
- const result = z.string().url().safeParse(value);
+ if (typeof value !== "string") return undefined;
+ const normalized = value.trim();
+ if (normalized === "") return undefined;
+ const result = urlSchema.safeParse(normalized);
return result.success ? result.data : undefined;
-}, z.string().url().optional());
+}, urlSchema.optional());📝 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.
| const optionalUrl = z.preprocess((value) => { | |
| if (typeof value !== "string" || value.trim() === "") return undefined; | |
| const result = z.string().url().safeParse(value); | |
| return result.success ? result.data : undefined; | |
| }, z.string().url().optional()); | |
| const urlSchema = z.string().url(); | |
| const optionalUrl = z.preprocess((value) => { | |
| if (typeof value !== "string") return undefined; | |
| const normalized = value.trim(); | |
| if (normalized === "") return undefined; | |
| const result = urlSchema.safeParse(normalized); | |
| return result.success ? result.data : undefined; | |
| }, urlSchema.optional()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/validation/templates.ts` around lines 3 - 7, The optionalUrl
preprocess currently checks value.trim() only for emptiness but passes the
untrimmed string to z.string().url().safeParse, which rejects valid URLs with
surrounding whitespace; update the preprocess to trim the input before
validation and only return undefined when the trimmed value is empty, i.e., use
the trimmed value for the safeParse call and for the value returned to the
schema (keep the surrounding optional() behavior intact), referencing the
optionalUrl constant and its z.preprocess handler.
| .max(1000, "Template list is too large"); | ||
|
|
||
| export const templateDetailsSchema = templateMetaSchema.extend({ | ||
| composeContent: z.string().min(1, "Template compose content is required"), |
There was a problem hiding this comment.
Reject whitespace-only compose content.
z.string().min(1) accepts " "; that should be treated as empty compose content.
Proposed fix
- composeContent: z.string().min(1, "Template compose content is required"),
+ composeContent: z.string().trim().min(1, "Template compose content is required"),📝 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.
| composeContent: z.string().min(1, "Template compose content is required"), | |
| composeContent: z.string().trim().min(1, "Template compose content is required"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/validation/templates.ts` at line 49, The composeContent schema
currently uses z.string().min(1) which allows whitespace-only strings; update
the schema for composeContent in templates.ts to reject whitespace-only input by
either chaining .trim() before .min (e.g., z.string().trim().min(1, "Template
compose content is required")) or adding a .refine/transform that checks
s.trim().length > 0 and returns the same error message; modify the
composeContent definition accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/utils/blueprint/generator.ts (1)
503-554:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip stale Pangolin labels from all services before reapplying the current blueprint.
Right now only services touched by the current
resources/privateResources/sitesarrays get cleaned. After a re-export, deleting an item or renaming ablueprintName/keyleaves the oldpangolin.public-resources.*,pangolin.private-resources.*, orpangolin.sites.*labels behind incomposeDocument, so the emitted compose can contain conflicting stale metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/blueprint/generator.ts` around lines 503 - 554, Old Pangolin labels linger on services not touched by current resources; before reapplying labels in toComposeYaml, iterate every service in the cloned compose document and remove any existing Pangolin labels so stale pangolin.public-resources.*, pangolin.private-resources.* and pangolin.sites.* entries are cleared. Use the existing helpers removePangolinLabels and labelsToArray to normalize and strip labels (e.g., call removePangolinLabels(labelsToArray(svc.labels), ['pangolin.public-resources.', 'pangolin.private-resources.', 'pangolin.sites.']) for each service object in the services map), then set svc.labels = dedupePreserveOrder([...cleaned, ...remaining]) or similar before the later applyLabelsToService loops (functions referenced: toComposeYaml, applyLabelsToService, removePangolinLabels, labelsToArray, dedupePreserveOrder).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/blueprint-builder.tsx`:
- Around line 141-154: The addPrivateResource handler currently builds key/alias
using (bp.privateResources?.length ?? 0) + 1 which can collide after deletions;
update addPrivateResource to scan existing bp.privateResources keys (matching
/^private-(\d+)$/), compute the smallest positive integer suffix not already
used (nextUnusedSuffix), and use that suffix when setting key, name, and alias
(instead of length+1). Keep the same usages of setBlueprint and
defaultPrivateResource/defaultLabelService, and apply the identical change to
the corresponding site handler (e.g., addSiteResource) so site-* ids are
generated collision-safely.
- Around line 864-871: When updating resource.protocol in the select onChange
handler, also clear HTTP-only fields so switching to TCP/UDP doesn't leave
invalid state: if the new protocol !== "http", set auth to undefined and
remove/undefine method on each extraTargets entry (e.g. map extraTargets to
strip method), while still setting proxyPort as you already do via onChange;
apply the same change to the other protocol-switch handlers referenced (the
similar blocks around extraTargets and the handler at 895-1046) so
validateBlueprint() no longer rejects hidden HTTP-specific fields.
In `@src/utils/blueprint/generator.ts`:
- Around line 218-239: The subdomain is being set from the raw slug (sluggedKey)
which can collide even when uniqueBlueprintName() produced unique blueprintName;
update resourceFromComposeService to derive subdomain from the same uniquified
name (i.e., call uniqueBlueprintName(sluggedKey) or use the computed
blueprintName) so subdomain and blueprintName are consistent and unique; apply
the same change to the other importer code block referenced (the one around
lines 242-281) so both resource.subdomain and resource.blueprintName are
generated from the same uniqueBlueprintName(...) result.
---
Duplicate comments:
In `@src/utils/blueprint/generator.ts`:
- Around line 503-554: Old Pangolin labels linger on services not touched by
current resources; before reapplying labels in toComposeYaml, iterate every
service in the cloned compose document and remove any existing Pangolin labels
so stale pangolin.public-resources.*, pangolin.private-resources.* and
pangolin.sites.* entries are cleared. Use the existing helpers
removePangolinLabels and labelsToArray to normalize and strip labels (e.g., call
removePangolinLabels(labelsToArray(svc.labels), ['pangolin.public-resources.',
'pangolin.private-resources.', 'pangolin.sites.']) for each service object in
the services map), then set svc.labels = dedupePreserveOrder([...cleaned,
...remaining]) or similar before the later applyLabelsToService loops (functions
referenced: toComposeYaml, applyLabelsToService, removePangolinLabels,
labelsToArray, dedupePreserveOrder).
🪄 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: c5fff83b-02cf-4d2c-8d04-e2cea0afda37
📒 Files selected for processing (7)
src/routes/blueprint-builder.tsxsrc/types/blueprint.tssrc/utils/blueprint/__tests__/generator.test.tssrc/utils/blueprint/__tests__/golden.test.tssrc/utils/blueprint/__tests__/validator.test.tssrc/utils/blueprint/generator.tssrc/utils/blueprint/validator.ts
📜 Review details
⏰ 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: build-and-push
| <select | ||
| value={resource.protocol} | ||
| onChange={(e) => { | ||
| const protocol = e.target.value as BlueprintProtocol; | ||
| onChange({ | ||
| protocol, | ||
| proxyPort: protocol === "http" ? undefined : (resource.proxyPort ?? resource.servicePort), | ||
| }); |
There was a problem hiding this comment.
Clear HTTP-only state when switching to TCP/UDP.
This handler only changes protocol and proxyPort. If the user already filled HTTP-only fields, auth and extraTargets[*].method stay in state, but the TCP/UDP form hides those controls and validateBlueprint() rejects them. That leaves the resource invalid with no visible way to fix it after the protocol switch.
Also applies to: 895-1046
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/blueprint-builder.tsx` around lines 864 - 871, When updating
resource.protocol in the select onChange handler, also clear HTTP-only fields so
switching to TCP/UDP doesn't leave invalid state: if the new protocol !==
"http", set auth to undefined and remove/undefine method on each extraTargets
entry (e.g. map extraTargets to strip method), while still setting proxyPort as
you already do via onChange; apply the same change to the other protocol-switch
handlers referenced (the similar blocks around extraTargets and the handler at
895-1046) so validateBlueprint() no longer rejects hidden HTTP-specific fields.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests