Skip to content

EDM-4063: Add docker:// protocol if link does not contain any#675

Merged
rawagner merged 2 commits into
flightctl:mainfrom
rawagner:explicit_protocol
Jun 3, 2026
Merged

EDM-4063: Add docker:// protocol if link does not contain any#675
rawagner merged 2 commits into
flightctl:mainfrom
rawagner:explicit_protocol

Conversation

@rawagner

@rawagner rawagner commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 096c97ec-3a56-4bd1-90a2-6f5c58ed1671

📥 Commits

Reviewing files that changed from the base of the PR and between 97f0434 and 0acdacd.

📒 Files selected for processing (1)
  • libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx

Summary by CodeRabbit

  • Bug Fixes
    • Installation wizard download button now detects existing URL protocols and correctly prefixes artifact links (e.g., with docker://) when needed, ensuring download links open correctly.

Walkthrough

SelectTargetStep.tsx adds protocol detection to artifact URLs. A PROTOCOL_REGEX constant identifies URLs with explicit protocols, and the download button's href conditionally prefixes docker:// only when no protocol is detected.

Changes

Protocol-aware download button

Layer / File(s) Summary
Protocol detection and conditional URL prefix
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
PROTOCOL_REGEX detects whether artifactUrl starts with a protocol. A computed hasProtocol boolean is used to conditionally prefix the button href with docker:// only when the URL lacks an explicit protocol.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided by the author, making it impossible to evaluate relevance to the changeset. Add a pull request description explaining the purpose and context of this change for better clarity.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding docker:// protocol detection and prefixing when links lack an explicit protocol.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx (1)

209-281: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Protocol regex misclassifies host:port image refs as “already has protocol.”

/^[a-zA-Z][a-zA-Z0-9+.-]*:/ matches localhost: in values like localhost:5000/my-image, so hasProtocol becomes true and Line 281 skips the docker:// prefix. That breaks the new behavior for common registry URLs without scheme.

Use stricter detection (e.g., require :// or a small allowlist of expected schemes) so host:port is treated as no-protocol.

🤖 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
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx`
around lines 209 - 281, The PROTOCOL_REGEX used by NewDeviceTarget
(PROTOCOL_REGEX and hasProtocol) incorrectly treats host:port (e.g.,
localhost:5000/...) as having a scheme; change the detection to require an
explicit scheme separator (e.g., "://") or match a small allowlist of known
schemes so that values like "localhost:5000/..." are considered no-protocol and
get the docker:// prefix; update PROTOCOL_REGEX and any uses of hasProtocol
(where artifactUrl is checked before building href) accordingly so only true
URIs (like "http://", "https://", "docker://", "oci://", etc.) bypass the
docker:// prepend.
🤖 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.

Outside diff comments:
In
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx`:
- Around line 209-281: The PROTOCOL_REGEX used by NewDeviceTarget
(PROTOCOL_REGEX and hasProtocol) incorrectly treats host:port (e.g.,
localhost:5000/...) as having a scheme; change the detection to require an
explicit scheme separator (e.g., "://") or match a small allowlist of known
schemes so that values like "localhost:5000/..." are considered no-protocol and
get the docker:// prefix; update PROTOCOL_REGEX and any uses of hasProtocol
(where artifactUrl is checked before building href) accordingly so only true
URIs (like "http://", "https://", "docker://", "oci://", etc.) bypass the
docker:// prepend.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: b2d736dc-9312-43b7-a773-18caa7cda6eb

📥 Commits

Reviewing files that changed from the base of the PR and between afa4951 and 97f0434.

📒 Files selected for processing (1)
  • libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx

@rawagner rawagner force-pushed the explicit_protocol branch from 97f0434 to 0acdacd Compare June 3, 2026 08:22
@rawagner rawagner merged commit 24c25bf into flightctl:main Jun 3, 2026
10 checks passed
celdrake pushed a commit to celdrake/flightctl-ui that referenced this pull request Jun 3, 2026
celdrake added a commit that referenced this pull request Jun 3, 2026
* EDM-4058: Correctly identify RBAC exclusions (#671)

Made-with: Cursor
(cherry picked from commit afa4951)

* EDM-3976 vulnerabilities empty state (#651)

* No need to return placeholder when no configs exist

Made-with: Cursor
(cherry picked from commit 15c4ddc)

* EDM-4008: Enforce only valid date range selection (#660)

Made-with: Cursor
(cherry picked from commit 5ee7434)

* EDM-4020: Refresh vulnerability summary in Overview (#657)

Made-with: Cursor
(cherry picked from commit 4204596)

* EDM-4063: Add docker:// protocol if link does not contain any (#675)

(cherry picked from commit 24c25bf)
Made-with: Cursor

* EDM-4014 device logs cancel (#659)

* EDM-4014: Add cancel button and improve overall UX
* EDM-4012: Reset filters when logType changes
* Fix disconnect banner for dark theme with suggested token

Made-with: Cursor
(cherry picked from commit 8c875e0)

* EDM-4018: Allow search for vulnerabilities be case-insensitive (#656)

* EDM-4018: Allow search for vulnerabilities be case-insensitive
* Fix space not being an allowed character

Made-with: Cursor
(cherry picked from commit 623d65a)

* EDM-4011: Fix parser missing footer depending on formatting (#663)

Made-with: Cursor
(cherry picked from commit 0ce4600)

* EDM-4013 device logs search validations (#662)

* EDM-4013: Block file paths with dots for moving away of /var/log
* EDM-4013: Strengthen validations for systemd unit names

Made-with: Cursor
(cherry picked from commit ac370df)

---------

Co-authored-by: Rastislav Wagner <rawagner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants