Skip to content

refactor: centralize GitHub API logic and add check-version command#2987

Closed
ProdByBuddha wants to merge 3 commits intofreenet:mainfrom
ProdByBuddha:refactor/github-api-utils
Closed

refactor: centralize GitHub API logic and add check-version command#2987
ProdByBuddha wants to merge 3 commits intofreenet:mainfrom
ProdByBuddha:refactor/github-api-utils

Conversation

@ProdByBuddha
Copy link
Copy Markdown

Problem

GitHub API fetching logic was duplicated across update.rs and auto_update.rs. Additionally, a bug in a previous refactoring attempt (due to shell interpolation) corrupted the $? variable in the macOS service wrapper update detection, making it always appear as \0. Finally, there was no way for users to manually check for updates without starting the update process.

Solution

  • Extracted shared GitHub API logic into a new commands::utils module.
  • Centralized the Release and Asset structs and added a 10s timeout to reqwest calls for better resilience.
  • Implemented a dedicated check-version command.
  • Corrected the shell escaping for $? in the macOS service wrapper update check.

Testing

  • Unit tests added to check_version.rs to verify semver comparison logic.
  • Verified cargo clippy and cargo fmt pass in Codespace.
  • Manual verification of the correctly escaped $? in the generated update.rs.

Fixes #2962

@ProdByBuddha ProdByBuddha force-pushed the refactor/github-api-utils branch from cbae14d to 584780f Compare February 12, 2026 14:05
This refactor extracts the GitHub release fetching logic into a shared `commands::utils` module, reducing duplication across `update`, `auto-update`, and the new `check-version` commands.

Key changes:
- Centralized `get_latest_release` in `utils.rs` with a 10s timeout.
- Added `freenet check-version` command for manual update checks.
- Fixed a bug in `update.rs` where `$?` was incorrectly escaped in the service wrapper update check.
- Added unit tests for version comparison.
@ProdByBuddha ProdByBuddha force-pushed the refactor/github-api-utils branch from 584780f to 971d392 Compare February 13, 2026 04:54
@sanity sanity requested a review from Copilot February 18, 2026 00:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 9, 2026

🔍 Rule Review: centralize GitHub API logic + check-version command

Rules checked: git-workflow.md, code-style.md, testing.md
Files reviewed: 6 (auto_update.rs, check_version.rs, mod.rs, update.rs, utils.rs, freenet.rs)

Critical

None.

Warnings

None.

Info

  • crates/core/src/bin/commands/utils.rs:1-2 — Import ordering violation: use anyhow::{Context, Result} (external crate) appears before use std::time::Duration (std). Rule requires std:: → external crates → crate::. (rule: code-style.md)

  • crates/core/src/bin/commands/utils.rs:7-44 — New public API items (Release, Asset, GITHUB_API_URL, get_latest_release) have no /// doc comments. Rule requires a one-line summary at minimum for public APIs; get_latest_release should also document # Errors. (rule: code-style.md)

  • crates/core/src/bin/commands/check_version.rs:8-38CheckVersionCommand and its run method are public with no doc comments. (rule: code-style.md)

To acknowledge findings: Check each box above once addressed (or a tracking issue opened),
or post /ack to dismiss all findings for this revision.
Merge is not blocked (Critical and Warning counts are both zero); these are style-level suggestions only.


Automated review against .claude/rules/. Critical and Warning findings block merge — check boxes or post /ack to acknowledge.

@sanity
Copy link
Copy Markdown
Collaborator

sanity commented Mar 14, 2026

Thanks for the contribution! The deduplication of GitHub API logic between update.rs and auto_update.rs is a valid cleanup, and the check-version command is a nice idea.

However, both files have been heavily modified since this PR was opened (auto-update restart loop fixes, dirty build detection, range-based version compatibility, etc.), so this PR now has significant merge conflicts and would need to be essentially rewritten to rebase on current main.

Closing for now — if we want to revisit the deduplication it'll be easier to do fresh from the current codebase.

[AI-assisted - Claude]

@sanity sanity closed this Mar 14, 2026
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.

4 participants