fix(app): stabilize local-source desktop startup#2155
Conversation
|
Credit balance is too low |
f27b5cb to
c8a227f
Compare
|
Credit balance is too low |
c8a227f to
92475d8
Compare
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Status refresh (2026-05-19): no new source changes pushed here. Normal CI lanes remain green from the latest run; the visible red checks are the same shared Auth P0 / Agent Review |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Credit balance is too low |
|
Validation update for
Known remaining red check: |
|
Final Windows validation update after syncing the nested Eliza source:
Known remaining red status is the existing Agent Review auth gate, not the Windows package/runtime path covered here. |
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on stabilizing the desktop application's startup by resolving module identity conflicts and hardening environment variable handling for Windows. Codacy analysis indicates the changes are 'up to standards' with no new quality issues detected.
Despite the structural improvements, there are concerns regarding the validation of these fixes. Specifically, several critical acceptance criteria—such as the Windows environment variable fallback and Vite alias prioritization—lack corresponding test coverage in the current submission. Additionally, the transition to canned JSON for E2E API testing reduces the suite's ability to detect breaking changes in the actual runtime contract. While the use of globalThis resolves the immediate 'useApp' errors, it should be monitored as a potential source of technical debt.
About this PR
- The E2E test utility 'startLiveApiServer' has been modified to return canned JSON instead of proxying a real runtime. This change prevents the tests from validating actual API contract behavior and may mask regressions in the communication layer.
- Storing the AppContext singleton on 'globalThis' is a 'fail-closed' patch. While it resolves module identity splits in the React tree, it may indicate underlying issues in the build or module resolution strategy that could require long-term maintenance.
Test suggestions
- Verify that Vite aliases correctly prioritize local package source files over built distribution files.
- Verify that the AppContext singleton is correctly stored and retrieved from globalThis in the renderer.
- Verify that the Windows packaged-agent starts successfully when the environment contains 'Path' instead of 'PATH'.
- Verify that @elizaos/plugin-lifeops successfully registers with the narrow automation-node-contributors import.
- Playwright test asserting the visible renderer reaches the app surface without triggering an error boundary.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that Vite aliases correctly prioritize local package source files over built distribution files.
2. Verify that the AppContext singleton is correctly stored and retrieved from globalThis in the renderer.
3. Verify that the Windows packaged-agent starts successfully when the environment contains 'Path' instead of 'PATH'.
4. Verify that @elizaos/plugin-lifeops successfully registers with the narrow automation-node-contributors import.
5. Playwright test asserting the visible renderer reaches the app surface without triggering an error boundary.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Compatibility | 2 medium |
| BestPractice | 1 medium |
| ErrorProne | 4 medium 13 high |
| Security | 22 critical |
| CodeStyle | 41 minor |
| Complexity | 17 medium |
🟢 Metrics 370 complexity · 2 duplication
Metric Results Complexity 370 Duplication 2
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Update after reading the author comments and validation history: this branch had substantial Windows/local-source validation, including focused package-mode tests, fresh hidden Windows package builds, packaged Playwright smoke, artifact inspection, and checks against both latest Eliza source and an older pinned release-contract ref. I should have read that thread before posting this review.
The current-state concern remains, but it should be read as a refresh/split request, not as a dismissal of that validation. Since #2160/#2161 have landed, this branch is stale against current source-install and CI hardening, and a mechanical conflict resolution could replace current workflow/contract behavior in agent-review, CI, release-electrobun, and the bootstrap contract tests.
One blocker still stands and is also called out by Codacy: startLiveApiServer changes from a real app-core-backed API server to canned JSON responses. That may be useful for a renderer-only smoke, but it weakens proof for the runtime contract behind the desktop startup path. Please keep a live-runtime assertion for the startup fix, or split the canned-response helper into a separately scoped renderer smoke.
Keeping changes requested until the branch is refreshed onto current develop while preserving the current guards/contracts and the runtime validation remains representative.
|
Closing as part of the Milady -> elizaOS/eliza PR migration sweep. This PR has requested changes/conflicts and is mostly Milady wrapper/local-source desktop plumbing against paths that do not exist in current upstream. It should not be migrated as-is; any still-needed behavior should be reopened as a fresh elizaOS/eliza PR against the current packages/* layout. |
Summary
Stabilizes local-source/package-mode desktop startup and keeps the Windows packaged desktop path working against current local Eliza source.
eliza/packages/*source wins over stale packagedistfallbacks where Milady intentionally builds from the nested checkout.@elizaos/uibrowser/state imports through canonical local source entries and dedupes@elizaos/uiwith React/app-core to avoid renderer module identity splits.AppContextsingleton onglobalThis, so duplicate renderer module ids cannot produceuseApp must be used within AppProviderat runtime.Pathinstead of uppercasePATHdo not crash before the embedded agent starts.@elizaos/app-core/api/automation-node-contributorsinstead of the broad@elizaos/app-coreroot barrel; this fixes the latest-Eliza@elizaos/plugin-lifeopsstartup import failure (Unexpected as).useAppprovider crash, with uppercasePATHremoved.Current State
63448ea5ac97af5281739bffdb5cb0aa0c44361f77e0dc23308c5fdf13bdf9be23cf71184592935277e0dc23308c5fdf13bdf9be23cf711845929352f7a4e0bc50f966174ea42c41db787359ca6d8e67; the PATH patch matcher was verified against that older source block.@elizaos/core,@elizaos/shared,@elizaos/ui,@elizaos/agent,@elizaos/app-core,@elizaos/cloud-sdk, and local plugin packages.Validation
Validated locally on Windows on 2026-05-19 after fetching remotes and fast-forwarding both Eliza checkouts to latest
develop.Results:
app-core+plugin-lifeopsrebuild passed.Resources/app/eliza-distfor@elizaos/app-core/api/automation-node-contributorsand@elizaos/plugin-lifeops.distand Electrobunbuildoutput first.Path/PATHfallback and the LifeOps narrow app-core import fix.PATH-removed launch environment.http://127.0.0.1:31337/api/statuswith HTTP 200 andstartup-status.jsonreportedstate=running,phase=ready,lastError=null.@elizaos/plugin-lifeopspre-registered and migrated successfully; noUnexpected as,existingPath.split,Agent not ready,Something went wrong, oruseApp must be used within AppProvidermarkers were present.Known warnings during the fresh build are existing upstream/package warnings: Vite/Rolldown dynamic import/chunk warnings, optional package skips, and Electrobun
rcediticon embedding warnings. The build exited 0.CI Status
GitHub validation has started on
63448ea5a; normal CI and release workflow are expected to rerun for the final cleanup head. The previouse464611arun had normal CI, CodeQL, and release workflow green; the remaining visible red Agent Review/Auth P0 gate is the knownpull_request_targetpackage-mode workflow limitation from basedevelop, not this desktop startup change.Previous head
f96687406was green for CI and Validate Electrobun Release Workflow.e464611aadded the LifeOps packaged import fix found during detached manual startup validation against latest Eliza;63448ea5aonly removes the unusedisPillWindowShellimport flagged by code-quality review.Note
Stabilize local-source desktop startup with new build orchestrator and runtime repair pipeline
preflight,stage,package,build, andrunsubcommands that apply patches, repair packaged runtime payloads, publish avatar assets, and validate plugin/vendor package contents.run-eliza-app-core-script.mjsto disable local-source mode whenELIZA_SOURCE=packagesor upstream skips are set, and injectsMILADY_ELIZA_SOURCE=localinto the child env when using a local script without an explicit source env.elizaUiStateSingletonPluginin vite.config.ts to deduplicate theuseApphook module and prevent duplicate React context singletons; also pins React/ReactDOM to concrete resolved paths and adds@elizaos/cloud-sdklocal source aliasing.AppContextglobal singleton, voice onboarding skip button, onboarding avatar component, remote capability endpoint provider, and additional plugin/package hoisting rules.eliza/checkout is absent.startLiveApiServerin E2E tests no longer proxies a real upstream runtime — it now returns canned JSON responses only, which may mask regressions in actual API contract behavior.Macroscope summarized ac9f525.