Skip to content

Update channels#205

Merged
Jordonbc merged 23 commits intoDevfrom
Update-Channels
Mar 25, 2026
Merged

Update channels#205
Jordonbc merged 23 commits intoDevfrom
Update-Channels

Conversation

@Jordonbc
Copy link
Copy Markdown
Collaborator

No description provided.

@github-actions
Copy link
Copy Markdown

Code Review Summary

Quality Issues

  1. Temp directory cleanup in tauri-build.js:115-116

    fs.rmSync(path.dirname(channelConfigPath), { recursive: true, force: true });

    This removes the parent directory but not the actual temp directory itself. The temp dir is created at os.tmpdir()/openvcs-tauri-config-XXXXXX/, so path.dirname(channelConfigPath) is correct, but the pattern could be clearer.

  2. Duplicate channel definitions - build.rs and write-tauri-channel-config.js both define channel metadata independently. If one is updated without the other, they will diverge.

  3. persistence_name() in app_identity.rs:43-46 - The self parameter is explicitly ignored (let _ = self) but kept in the signature. This is intentional per the docs but could be confusing. Consider either removing the parameter or documenting why it exists.

Potential Bugs

  1. Nightly workflow removes Flatpak support - The changes in nightly.yml completely remove the Flatpak build steps. The packaging/flatpak/README.md notes "Flatpak packaging remains stable-only" but this should be verified that it's intentional for nightly.

  2. Unused FROM variable - In Backend/build.rs, the ChannelConfig struct doesn't use the product_name field that has a hyphen in the Rust string but not in JS version ("OpenVCS-Beta" vs "OpenVCS Beta"). This inconsistency might cause issues.

Improvements

  1. Workflow args format - The new args format format('{0} --config {1}/tauri.channel.conf.json', matrix.args, runner.temp) assumes matrix.args exists. Should verify this is always populated in all workflow triggers.

  2. Install script variant detection - The detect_release_variant function in install.sh:124-140 checks for *openvcs-v* to identify stable, but this might miss future version formats.

  3. Missing test for invalid channel - app_identity.rs has tests for empty/unknown and known channels, but no test for whitespace-padded inputs like " beta ".

Minor Issues

  1. Inconsistent hyphenation: product_name in Rust is "OpenVCS-Beta" (hyphen) but in JS the display name is "OpenVCS Beta" (space). This shows in the window title and may cause user confusion.

  2. No validation - If OPENVCS_UPDATE_CHANNEL is set to an unexpected value, both build.rs and write-tauri-channel-config.js silently default to "stable" without logging.

New%20session%20-%202026-03-24T18%3A18%3A19.727Z
opencode session  |  github run

@github-actions
Copy link
Copy Markdown

Based on my review of the PR changes, here is my analysis:


Code Review: Update Channels PR

Quality Issues

  1. Memory leak in build.rs:64-73 - The ChannelConfig::from_entry uses Box::leak() to create static strings. While intentional for build-time config, this permanently leaks memory. Consider using Cow<'static, str> or lazy_static instead.

  2. Temp cleanup in tauri-build.js:112-114 - The cleanup comment is misleading:

    // channelConfigPath is a file inside the temp dir, so dirname removes the temp dir itself

    Actually, path.dirname(channelConfigPath) returns the temp directory itself (not parent), which is correct. But the comment is confusing.

  3. Duplicate channel parsing - Both build.rs and write-tauri-channel-config.js independently parse channel-metadata.json. If the JSON schema changes, these could diverge. Consider extracting shared logic.

  4. Install script regex in install.sh:131 - The pattern *stable*|*openvcs-v* would also match "unstable" since it contains "stable". Should use word boundaries or more specific patterns.

Potential Bugs

  1. Nightly workflow removes Flatpak - nightly.yml removes all Flatpak build steps (lines 187-232 of original). This should be verified as intentional since packaging/flatpak/README.md says "Flatpak packaging remains stable-only".

  2. Hyphen vs space inconsistency - channel-metadata.json has "productName": "OpenVCS-Beta" (hyphen) but in tauri-build.js:77 it's "OpenVCS Beta" (space). This causes different branding between CLI output and actual app.

  3. Uninitialized tmpDir on error in tauri-build.js:83 - If writeChannelConfig() throws, the tmpDir variable is created but never cleaned up since error handling happens in .catch() at line 120-122, not in the spawn callback.

  4. Missing validation for workflow matrix.args - The new format format('{0} --config ...', matrix.args, runner.temp) assumes matrix.args is always populated. Should add a default if undefined.

Improvements

  1. Silent fallback to stable - Both build.rs:62-64 and write-tauri-channel-config.js:17-20 default to "stable" silently without logging which channel was selected. Consider adding explicit logging.

  2. Missing whitespace test - app_identity.rs tests persistence_name() but doesn't test channel parsing with whitespace-padded inputs like " beta ".

  3. Unused variable in tauri-build.js:80 - channelProductName is computed from channelNames but only used in console output. The actual product name comes from the config file, making this potentially misleading.


Summary

The PR introduces a solid channel architecture, but has some inconsistencies between the JS and JSON definitions (hyphen vs space in product names), removes Flatpak from nightly (should verify intent), and has a confusing temp directory cleanup comment. The memory leak from Box::leak should be addressed if this code runs frequently.

New%20session%20-%202026-03-24T23%3A56%3A15.400Z
opencode session  |  github run

@github-actions
Copy link
Copy Markdown

Code Review: Update Channels PR

Summary

This PR introduces a channel-based build system with centralized metadata (channel-metadata.json) for stable, beta, and nightly builds.


Quality Issues

1. Inconsistent product naming (hyphen vs space)

  • channel-metadata.json:15 has "productName": "OpenVCS-Beta" (hyphen)
  • scripts/tauri-build.js:77 uses "OpenVCS Beta" (space)

This causes branding inconsistency between CLI output and the actual app window title.

2. Unused variable in tauri-build.js:80

const channelProductName = channelNames[channelSlug] || 'OpenVCS';

This is computed but never used - the actual product name comes from writeChannelConfig(). Either use it or remove it.

3. Misleading comment in build.rs:87-88

// Box::leak is acceptable here - this is build-time config that lives
// only for the duration of the build process, after which memory is reclaimed.

This is technically incorrect. Box::leak creates 'static lifetimes - the memory is leaked for the entire program execution, not reclaimed. Since this is a build script that runs during compilation, it's acceptable, but the comment should say "build script memory that persists through the build" rather than implying reclamation.


Potential Bugs

4. Nightly workflow removes Flatpak support
The changes in nightly.yml (compared to the original) completely remove Flatpak build steps. The packaging/flatpak/README.md says "Flatpak packaging remains stable-only" but verify this is intentional for nightly.

5. Temp directory cleanup logic in tauri-build.js:119

fs.rmSync(path.dirname(channelConfigPath), { recursive: true, force: true });

The comment at line 117-118 is confusing. Since channelConfigPath is already a file path, path.dirname() correctly returns the temp directory. The code is correct, but the comment should be clearer:

// channelConfigPath is /tmp/openvcs-tauri-config-XXXXXX/tauri.channel.conf.json
// dirname gives us /tmp/openvcs-tauri-config-XXXXXX which we then remove

Improvements

6. Silent fallback to stable
Both build.rs:70-74 and write-tauri-channel-config.js:17-24 default to "stable" without logging which channel was selected when the env var is missing. Consider adding debug logging:

  • build.rs already has eprintln! for unknown channels - extend this to log when defaulting

7. Install script variant detection in install.sh:147-148

*nightly*) printf 'nightly' ;;
*beta*) printf 'beta' ;;

These patterns could match unintended strings. More precise matching:

*openvcs-nightly*|*nightly*) printf 'nightly' ;;
*openvcs-beta*|*beta*) printf 'beta' ;;

8. Missing whitespace test in app_identity.rs
Tests don't cover whitespace-padded inputs like " beta ". Add:

#[test]
fn handles_whitespace_padded_channel() {
    // Channel detection with whitespace should still work
}

Minor Issues

9. Unused self parameter in app_identity.rs:15

pub fn persistence_name() -> &'static str {

Takes &self but ignores it. Either document why or consider if this was intended for a trait implementation.

10. Repo URL hardcoded in channel-metadata.json:2

"repo": "https://github.com/Jordonbc/OpenVCS"

Should this be a variable or is the repo permanently Jordonbc's?


Positive Aspects

  • Centralized channel metadata is well-designed
  • Both Rust and JS implementations are consistent
  • Good error handling with warnings for unknown channels
  • The build script correctly propagates channel info via cargo env vars
  • Workflow files properly use the new channel config system

New%20session%20-%202026-03-25T00%3A00%3A12.216Z
opencode session  |  github run

@github-actions
Copy link
Copy Markdown

Based on my analysis of the PR changes, here's my independent code review:

Code Review: Update Channels PR

Quality Issues

1. Hyphen vs space inconsistency

  • channel-metadata.json:15 has "productName": "OpenVCS-Beta" (hyphen)
  • scripts/tauri-build.js:77 uses "OpenVCS Beta" (space)

This causes branding mismatch between CLI output and window title.

2. Misleading comment in build.rs:87-88

// Box::leak is acceptable here - this is a build script that runs once during
// compilation. The leaked memory persists for the build duration only.

Box::leak creates 'static lifetime - memory is not reclaimed. The comment should be corrected.

3. Duplicate channel logic - Both build.rs and write-tauri-channel-config.js independently parse channel-metadata.json. Consider extracting to a shared module.

4. Unused variable channelProductName in tauri-build.js:80

const channelProductName = channelNames[channelSlug] || 'OpenVCS';

Used only in console output (line 108), not in actual config. Potentially misleading.

Potential Bugs

5. Nightly workflow removes Flatpak - The changes in nightly.yml remove all Flatpak build steps. Per packaging/flatpak/README.md "Flatpak packaging remains stable-only" - verify this is intentional.

6. Install script variant detection in install.sh:146-148

*nightly*) printf 'nightly' ;;
*beta*) printf 'beta' ;;

The pattern *beta* will also match *nightly* (contains "beta"). More precise patterns needed:

*openvcs-nightly*|*nightly*) printf 'nightly' ;;
*openvcs-beta*|*beta*) printf 'beta' ;;

7. Silent fallback without logging - Both build.rs:70-74 and write-tauri-channel-config.js:17-24 default to "stable" when channel is unknown, but only build.rs logs a warning. The JS version logs only when explicitly set to unknown value.

Minor Issues

8. Missing test for whitespace input - app_identity.rs tests don't cover whitespace-padded channel inputs like " beta ".

9. self parameter unused in persistence_name() - Takes &self but ignores it (let _ = self). Consider removing or documenting.

Positive Aspects

  • Centralized channel-metadata.json is well-designed
  • Consistent JSON-RPC/endpoint fallback chain in updater config
  • Good error handling with warnings for unknown channels
  • Build script correctly propagates channel via cargo env vars

New%20session%20-%202026-03-25T00%3A05%3A08.705Z
opencode session  |  github run

@Jordonbc Jordonbc merged commit 7d75141 into Dev Mar 25, 2026
5 of 6 checks passed
@Jordonbc Jordonbc deleted the Update-Channels branch March 25, 2026 00:10
@github-actions
Copy link
Copy Markdown

fatal: couldn't find remote ref Update-Channels

opencode session  |  github run

@github-actions
Copy link
Copy Markdown

fatal: couldn't find remote ref Update-Channels

opencode session  |  github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant