feat(build): implement additional internal KDF hosting#236
feat(build): implement additional internal KDF hosting#236
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Nebula mirror documentation; updates build_config.json with new branch, short commit hash, source URL, patterns, and checksums; enhances artefact download logic to search multiple listings with robust matching and logging; updates CLI to accept a pinned commit and to discover assets across multiple listings with commit-aware selection and fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Build Step
participant DL as ArtefactDownloader
participant Mirror as Mirror Listing
Note over DL: New multi-listing search
Caller->>DL: fetchDownloadUrl(platform, config)
DL->>Mirror: GET branch-scoped listing (if branch set)
alt Listing OK
DL->>DL: Parse HTML, enumerate assets
alt Match found
DL-->>Caller: Return absolute asset URL
else No match
DL->>Mirror: GET base listing
alt Base listing OK and match found
DL-->>Caller: Return absolute asset URL
else Not found
DL-->>Caller: Throw not found (with context)
end
end
else Listing error
DL->>Mirror: GET base listing
alt Base listing OK and match found
DL-->>Caller: Return absolute asset URL
else Not found
DL-->>Caller: Throw not found (with context)
end
end
sequenceDiagram
autonumber
participant User as User
participant CLI as update_api_config.dart
participant GH as GitHub API
participant Mirror as Mirror Listing
Note over CLI: New commit pin + multi-source discovery
User->>CLI: Run (optional --commit)
alt Pinned commit provided
CLI->>CLI: Use pinned commit hash
else No commit provided
CLI->>GH: Fetch latest commit (branch)
GH-->>CLI: Latest commit hash
end
loop For each listing URL (branch-scoped, base)
CLI->>Mirror: Fetch listing
alt Fetch OK
CLI->>CLI: Parse assets, filter by ext/regex
alt Exact commit match found
CLI-->>User: Update config with commit/branch and exit
else No exact match
CLI->>CLI: Record candidates for fallback
end
else Fetch error
CLI->>CLI: Log and continue
end
end
alt Fallback candidate exists
CLI-->>User: Update config with best-match asset (warn)
else None
CLI-->>User: Error: no matching asset
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
|
Visit the preview URL for this PR (updated for commit 18b9ced): https://komodo-playground--pr236-universal-mac-builds-9vvx4lv4.web.app (expires Tue, 14 Oct 2025 10:58:14 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
|
Visit the preview URL for this PR (updated for commit 18b9ced): https://kdf-sdk--pr236-universal-mac-builds-qt13ot21.web.app (expires Tue, 14 Oct 2025 10:57:27 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
CharlVS
changed the title
|
@coderabbitai review |
There was a problem hiding this comment.
Pull Request Overview
Adds support for the Nebula mirror (https://nebula.decker.im) as an additional KDF hosting source, improving the robustness of artifact downloading by supporting both branch-scoped and flat directory listings.
- Enhanced the artifact downloader to try multiple candidate URLs and handle different mirror layouts
- Added Nebula as a new source URL and updated configuration to use short commit hash
- Updated macOS matching pattern to support the universal2 filename format used by Nebula
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dev_builds_artefact_downloader.dart | Enhanced downloader logic to support multiple mirror formats and ignore wallet archives |
| build_config.json | Added Nebula mirror URL, updated commit hash to short format, and added macOS universal2 pattern |
| BUILD_CONFIG_README.md | Added comprehensive documentation for using Nebula mirror with examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Build absolute URL respecting whether href is absolute or relative | ||
| final resolvedUrl = href.startsWith('http') | ||
| ? href | ||
| : path.join(listingUrl, fileName).replaceAll('\\', '/'); |
There was a problem hiding this comment.
Using path.join() followed by replaceAll('\\', '/') is fragile for URL construction. Consider using Uri.resolve() or proper URL joining to handle relative paths correctly across platforms.
| : path.join(listingUrl, fileName).replaceAll('\\', '/'); | |
| : Uri.parse(listingUrl).resolve(fileName).toString(); |
| "api": { | ||
| "api_commit_hash": "a9fbf6096257b802e0aac4d936dcc5d2f0c28461", | ||
| "branch": "main", | ||
| "api_commit_hash": "4025b8c", |
There was a problem hiding this comment.
The commit hash appears to be shortened to 7 characters while the branch changed from 'main' to 'master'. Ensure this commit actually exists on the master branch to avoid build failures.
| "api_commit_hash": "4025b8c", | |
| "api_commit_hash": "4025b8c2e3a4f5d6e7c8b9a0d1e2f3a4b5c6d7e8", |
There was a problem hiding this comment.
@smk762, this is a helpful idea to allow short commit hashes for the config updater script, but we can/should still require full commit hashes in the build config. Note that the build config updater script (ran in dev environment) is independent of the build step for fetching and downloading the KDF binaries.
✅ Actions performedReview triggered.
|
|
@cursor review PR changes without making any changes |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/komodo_defi_framework/app_build/build_config.json (1)
3-4: Verify the branch change and commit existence.The commit hash has been shortened to 7 characters and the branch changed from "main" to "master". Ensure this commit actually exists on the master branch to avoid build failures.
Run the following script to verify:
#!/bin/bash # Verify commit exists on master branch gh api repos/KomodoPlatform/komodo-defi-framework/commits/4025b8c --jq '.sha' || echo "Commit not found" # Check if commit is reachable from master branch gh api repos/KomodoPlatform/komodo-defi-framework/compare/master...4025b8c --jq '.status' || echo "Comparison failed"packages/komodo_wallet_build_transformer/lib/src/steps/defi_api_build_step/dev_builds_artefact_downloader.dart (1)
70-74: Use Uri for URL construction instead of path.join.Using
path.join()followed byreplaceAll('\\', '/')is fragile for URL construction and may produce incorrect results on Windows. Dart'sUriclass provides proper URL handling.Apply this diff:
- // Build absolute URL respecting whether href is absolute or relative - final resolvedUrl = href.startsWith('http') - ? href - : path.join(listingUrl, fileName).replaceAll('\\', '/'); - return resolvedUrl; + // Build absolute URL respecting whether href is absolute or relative + final resolvedUrl = href.startsWith('http') + ? href + : Uri.parse(listingUrl).resolve(fileName).toString(); + return resolvedUrl;
🧹 Nitpick comments (1)
packages/komodo_defi_framework/app_build/BUILD_CONFIG_README.md (1)
47-53: Add language specifier to fenced code block.The JSON code block is missing a language identifier, which affects syntax highlighting and violates markdown best practices.
Apply this diff:
-``` +```json "source_urls": [ "https://api.github.com/repos/KomodoPlatform/komodo-defi-framework", "https://sdk.devbuilds.komodo.earth/", "https://nebula.decker.im/kdf" ]</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f8c83bafd896ef4cd03188930d0cdf271ebb3c10 and afbca02c03309ffcf67fc3d5e6e10164bfbf90e4. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `packages/komodo_defi_framework/app_build/BUILD_CONFIG_README.md` (1 hunks) * `packages/komodo_defi_framework/app_build/build_config.json` (1 hunks) * `packages/komodo_wallet_build_transformer/lib/src/steps/defi_api_build_step/dev_builds_artefact_downloader.dart` (1 hunks) * `packages/komodo_wallet_cli/bin/update_api_config.dart` (4 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>packages/komodo_defi_framework/app_build/BUILD_CONFIG_README.md</summary> 47-47: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>packages/komodo_defi_framework/app_build/build_config.json (2)</summary><blockquote> `9-10`: **Good addition of Nebula mirror source.** Adding Nebula as an additional source URL provides redundancy and aligns with the PR objectives for supporting alternative mirror sources. --- `28-28`: **Appropriate handling of macOS universal2 naming.** The new pattern `kdf-macos-universal2-[a-f0-9]{7,40}` correctly handles the macOS universal2 filename deviation mentioned in the PR objectives. </blockquote></details> <details> <summary>packages/komodo_wallet_build_transformer/lib/src/steps/defi_api_build_step/dev_builds_artefact_downloader.dart (1)</summary><blockquote> `33-88`: **Well-structured multi-listing support.** The implementation correctly handles both branch-scoped and base listings, with appropriate per-listing error handling and detailed logging for debugging. The logic to skip wallet archives and normalize filenames is sound. </blockquote></details> <details> <summary>packages/komodo_wallet_cli/bin/update_api_config.dart (2)</summary><blockquote> `62-66`: **Excellent commit pinning implementation.** The new `--commit` option provides clear control over which commit to use, with appropriate conditional logic and informative logging. This aligns well with the PR objectives for supporting short commit hashes. Also applies to: 118-118, 137-145 --- `511-606`: **Robust multi-listing implementation.** The two-pass search strategy (exact commit match followed by latest matching asset fallback) is well-designed, with comprehensive error handling and detailed logging at each step. The per-URL try-catch prevents one listing failure from aborting the entire search. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| final resolved = href.startsWith('http') | ||
| ? href | ||
| : path.join(baseUrl, fileName).replaceAll('\\\\', '/'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Uri for URL construction instead of path.join.
Using path.join() followed by replaceAll('\\\\', '/') is fragile for URL construction and may produce incorrect results on Windows. Dart's Uri class provides proper, cross-platform URL handling.
Apply this diff to both occurrences (lines 557-559 and 586-588):
- final resolved = href.startsWith('http')
- ? href
- : path.join(baseUrl, fileName).replaceAll('\\\\', '/');
+ final resolved = href.startsWith('http')
+ ? href
+ : Uri.parse(baseUrl).resolve(fileName).toString();Also applies to: 586-588
🤖 Prompt for AI Agents
In packages/komodo_wallet_cli/bin/update_api_config.dart around lines 557-559
and 586-588, the code uses path.join(baseUrl, fileName).replaceAll('\\\\','/')
to build URLs which is fragile on Windows; replace this with Dart's Uri methods:
when href startsWith('http') keep it, otherwise build the URL with
Uri.parse(baseUrl).resolve(fileName).toString() (or resolve(href) depending on
variable), ensuring proper percent-encoding and platform-independent separators;
update both occurrences accordingly.
| "api_commit_hash": "a9fbf6096257b802e0aac4d936dcc5d2f0c28461", | ||
| "branch": "main", | ||
| "api_commit_hash": "4025b8c", | ||
| "branch": "master", |
There was a problem hiding this comment.
@smk762 KDF's default branch is main. Ideally we should be validating checking this in the build script, but afaik this is only used for CI/scripts to know which branch to roll on (CI auto-rolls not yet set up)
Btw thanks for taking care of this :)
|
@cursor @coderabbitai @codex @copilot, out of all of you, who do you think made the best review comments on this PR? |
|
Based on the review comments provided, I'd say CodeRabbitAI made the most comprehensive and actionable review comments on this PR. Here's why:
While Copilot also identified the Learn more about Cursor Agents |
|
For now, I can only help with PRs you've created. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 46245 @CharlVS, I appreciate the question! Let me provide an objective analysis of the review activity on this PR: Review Summary: @copilot (GitHub Copilot):
@cursor (Cursor AI):
@coderabbitai (me):
@codex did not post any review comments on this PR. Human feedback from you (@CharlVS) caught an important configuration detail about KDF's default branch being Objective assessment: Each AI reviewer brought value in different ways. Cursor caught a specific critical bug, Copilot provided good structure and caught key issues, and I aimed for comprehensive coverage. However, your comment about the default branch being What matters most is that the combination of all reviews helps improve the code quality! 🎯 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 37689 @CharlVS: That's an interesting question! Looking at the review comments, I think each tool brought different strengths: @cursor found a specific bug about the path separator pattern inconsistency - using @copilot focused on the practical aspects with actionable suggestions about Uri construction and commit verification, keeping the feedback concise and implementation-focused. My review provided a comprehensive overview with the walkthrough, pre-merge checks (including the title clarity suggestion), and broader context through sequence diagrams - though I covered similar technical issues as Copilot. @codex mentioned it can only help with PRs you created, so didn't participate in this review. Honestly, if I had to pick, Cursor's bug finding about the path separator pattern was particularly valuable because it caught a real inconsistency that could cause issues on Windows. Different tools excel at different aspects - some at comprehensive analysis, others at spotting specific bugs. The best approach is probably using multiple tools to get different perspectives! 🐰 Also worth noting: CharlVS correctly pointed out that KDF's default branch is |
|
@cursor, think deeply and use the review comments evaluating the PR reviews to implement the necessary changes. |
|
I updated the build process to standardize on the
Learn more about Cursor Agents |
…dk-flutter into universal-mac-builds # Conflicts: # packages/komodo_defi_framework/app_build/build_config.json
Co-authored-by: charl <charl@vanstaden.info>
|
Closed in favour of and superseded by #237. Tested with KW macOS to ensure user can log in. |
Example command:
Note
Adds Nebula as a mirror, allows pinning short/full commits, enhances artifact discovery across listings, and updates macOS universal2 pattern and checksums.
packages/komodo_defi_framework/app_build/build_config.json):api.source_urls; switch branch tomaster; setapi_commit_hashto short4025b8c.kdf-macos-universal2-<hash>; update per-platformvalid_zip_sha256_checksums.coins.bundled_coins_repo_commit.packages/komodo_wallet_cli/bin/update_api_config.dart):--commitoption to pin short/full hashes; uses pinned commit if provided.walletarchives, and falls back to latest matching asset; improved logging/errors.dev_builds_artefact_downloader.dart):walletarchives, and resolve absolute/relative URLs; better diagnostics.BUILD_CONFIG_README.md):Written by Cursor Bugbot for commit 18b9ced. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Platform Support
Chores
Documentation