refactor: atomic sync of logseq-cli and desktop app

[Bad3r]

Summary

• Unify CLI and desktop packages under a single manifest so both derive from the same upstream commit
• Parameterize lib/cli.nix — remove hardcoded rev, version, and hashes; accept them from the manifest via callPackage
• Extend lib/loadManifest.nix with 3 new required keys and a validateHash helper for SRI validation
• Add scripts/update-nightly.sh to compute all CLI hashes via a double-build pattern
• Replace inline manifest rendering in nightly.yml with the new script
• Switch formatter to nixfmt-tree and update CI to use ubuntu-latest

Test plan

• nix flake check passes
• nix build .#logseq and .#logseq-cli succeed
• All pre-commit hooks pass
• All 3 CI workflows pass end-to-end

[Bad3r]

@claude review

[claude]

Claude finished @Bad3r's task in 1m 35s —— View job

---

Code Review

Todo List

• Read repository guidelines
• Review changed files
• Analyze scripts/update-nightly.sh
• Review manifest and library changes
• Check CI workflow updates
• Provide comprehensive feedback

---

Summary

This is a well-structured refactoring that successfully unifies the CLI and desktop builds under a single manifest. The implementation is clean, with good error handling and validation. I found a few minor issues and suggestions for improvement.

Issues Found

1. Error Output Visibility in scripts/update-nightly.sh

Severity: Low
Location: scripts/update-nightly.sh:85

The sed pattern assumes a specific error format, but if the format changes or the regex doesn't match, the error will be silent. The script does check for empty YARN_HASH and prints the build output, but only to stderr.

Suggestion: Consider adding more robust error extraction with multiple fallback patterns, or documenting the expected error format.

2. Missing Error Handling for curl in scripts/update-nightly.sh

Severity: Low
Location: scripts/update-nightly.sh:39-41

The curl command has -f (fail on HTTP errors) but doesn't handle network failures gracefully. If the upstream file is missing or GitHub is down, the error message won't be clear.

Suggestion: Add explicit error handling:

if ! CLI_VERSION=$(curl -fsSL "https://raw.githubusercontent.com/logseq/logseq/${LOGSEQ_REV}/deps/cli/package.json" | jq -r '.version'); then
  echo "ERROR: Failed to fetch CLI version from upstream" >&2
  exit 1
fi

3. Git Diff Check Inconsistency

Severity: Low
Location: .github/workflows/nightly.yml:177

The check changed from git status --short | grep -q to ! git diff --quiet. While the new approach is cleaner, using --quiet without --exit-code might not work as expected in all versions of git. The current form should work, but it's worth noting.

Suggestion: Consider git diff --quiet --exit-code for maximum clarity and portability.

4. Hardcoded Placeholder Hash

Severity: Low
Location: scripts/update-nightly.sh:47

The placeholder hash sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= is hardcoded. While functional, if Nix changes its error output format or validation, this could break.

Suggestion: Document why this specific placeholder is used, or use a constant at the top of the script.

Positive Observations

1. Excellent Error Validation: The validateHash function in lib/loadManifest.nix:18-22 is well-designed, using foldl' for efficiency and providing clear error messages.

2. Double-Build Pattern: The approach to extract yarn deps hash via intentional build failure (scripts/update-nightly.sh:72-94) is clever and well-documented.

3. Atomicity: The refactoring truly achieves atomic sync - both packages now derive from the same logseqRev, which prevents version mismatches.

4. CI Improvements: Adding nix build .#logseq-cli to the validation step (.github/workflows/nightly.yml:189) ensures both packages build successfully.

5. Formatter Migration: Switching to nixfmt-tree is a good choice for RFC 166 compliance.

6. Clear Commit History: The commits are well-structured and tell a clear story of the refactoring progression.

Minor Suggestions

1. scripts/update-nightly.sh: The script uses echo "::group::" syntax which is GitHub Actions-specific. This is fine, but worth noting if the script might be run locally. Consider adding a comment at the top indicating it's designed for GitHub Actions.

2. lib/cli.nix: The comment on line 16 "Manifest-driven parameters (passed from flake.nix)" is helpful. Consider similar comments in other parameterized functions for consistency.

3. Documentation: The README.md updates are thorough. Consider adding a troubleshooting section for what to do if the double-build pattern fails (though this is a very edge case).

Security Considerations

No security issues found. The script properly validates inputs and uses safe shell practices (set -euo pipefail).

Conclusion

This is a high-quality refactoring that achieves its stated goals. The code is well-documented, follows Nix best practices, and includes proper error handling. The few issues identified are minor and don't block the PR. All tests pass and the implementation is production-ready.

Recommendation: Approve with optional minor improvements.

---