Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe macOS runner now checks whether the app is installed under ChangesmacOS installation flow
Swift compile workflow documentation
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature · Severity of issue fixed: High Sequence Diagram(s)sequenceDiagram
participant main.swift
participant AppInstallationPreflight
participant AppInstallation
participant NSWorkspace
main.swift->>AppInstallationPreflight: run()
AppInstallationPreflight->>AppInstallation: installInBackground()
AppInstallation->>AppInstallation: copy and atomically publish bundle
AppInstallation-->>AppInstallationPreflight: return installation result
AppInstallationPreflight->>NSWorkspace: launch installed app
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The installation preflight, relaunch flow, and localized resources appear mergeable with normal checks. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The PR implements the main requirement in Full details: Out of Scope Changes checkExplanation The installation code, generated strings, build integration, dependency, and tests support Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 3 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Check the installation path before native app startup. Offer background copying and relaunch for eligible apps, with Finder instructions for quarantine, existing installations, and copy failures. Generate native dialog strings from the existing translation catalogs and cover installation safeguards with Swift unit tests. Refs getlantern/engineering#3825.
af2d0dc to
88092d0
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Release-only preflight paths need coverage before approval.
Pull request overview
Adds a macOS installation preflight requiring Lantern to run from /Applications, with background copying, relaunch support, Finder fallback, localization, and tests.
Changes:
- Added installation and preflight UI logic.
- Added localized strings and generation tooling.
- Added native tests and Xcode build integration.
- Updated dependency and CI configuration.
Review note: Release-only preflight branches are not exercised by the current Debug-only test workflow.
File summaries
| File | Description |
|---|---|
scripts/macos/generate_installation_strings.dart |
Generates native localized strings |
pubspec.yaml |
Adds gettext parser dependency |
pubspec.lock |
Locks the dependency |
macos/RunnerTests/AppInstallationTests.swift |
Tests installation behavior |
macos/Runner/main.swift |
Runs the preflight before app startup |
macos/Runner/AppInstallation.swift |
Implements installation logic and preflight UI |
macos/Runner.xcodeproj/project.pbxproj |
Wires sources, tests, localization, and build phases |
assets/locales/en.po |
Adds English installation messages |
.github/workflows/swift-compile-check.yml |
Updates CI workflow documentation |
Review details
Suppressed comments (1)
macos/Runner/AppInstallation.swift:131
- Because
main.swiftexcludes this preflight inDEBUGandmake macos-unit-testsruns only the Debug configuration, CI never exercises these release-only branches (including the Finder/destination routing and Move and Relaunch path). A regression could ship while all of the new filesystem tests pass; add a release-mode launch smoke test or inject the alert/workspace dependencies so these branches can be tested.
guard !installation.isInstalled else { return true }
let app = NSApplication.shared
app.setActivationPolicy(.regular)
app.activate(ignoringOtherApps: true)
- Files reviewed: 8/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Check Lantern’s installation path before Radiance or the system extension starts. If the app is outside
/Applications, prompt the user to install it there before continuing.Resolves https://github.com/getlantern/engineering/issues/3825
Summary by CodeRabbit
New Features
Bug Fixes
Tests