refactor(nav): fold Orchestration under Brain + daemon/health cleanup - #5080
Closed
senamakel wants to merge 26 commits into
Closed
refactor(nav): fold Orchestration under Brain + daemon/health cleanup#5080senamakel wants to merge 26 commits into
senamakel wants to merge 26 commits into
Conversation
useDaemonHealth mounts in several places at once (SocketProvider via useDaemonLifecycle; ServiceBlockingGate directly + via useDaemonLifecycle). setupHealthListener awaited the first poll before assigning pollingIntervalId, so all concurrent callers raced past the singleton guard and each spawned their own setInterval — emitting multiple openhuman.health_snapshot RPCs per tick. Assign the interval id synchronously and reference-count consumers: the shared loop starts on the first consumer and is torn down only when the last releases, so one component unmounting can't stop polling for the rest. Adds a regression test covering concurrent setup and partial release.
LocalAIDownloadSnackbar (mounted app-wide) polled inference_status + inference_downloads_progress every 2s forever, even with no download active. Make the poll self-scheduling: 2s while a download is in flight, 15s when idle.
React.StrictMode double-mounts HarnessInitOverlay in dev (effect → cleanup → effect); each setup fires an immediate poll, booting two harness_init_status RPCs at the same instant. Coalesce overlapping status fetches onto one in-flight request (cleared once settled, so ongoing sequential polling is unaffected). Also guards a genuine remount during the boot window.
…valid A bare `OPENHUMAN_SHELL_HIDE_WINDOW=` (empty/whitespace-only, common when a .env or launcher exports the key with no value) fell through to the "unrecognized value ignored" warn arm and logged a warning on every boot. Treat an empty value as absent — silently keep the current setting. Extends the existing env-override test to cover the empty and whitespace-only cases.
…shot log spam policy::initialize() is documented as a once-at-startup call but is wired into build_app_state_snapshot, so it runs on every app_state_snapshot RPC — logging `[keyring_consent] initialize` at INFO each time (several per boot as the frontend polls the snapshot). Hold the write lock across a compare-and-set and return early when the persisted consent is unchanged, so it writes + logs only on a genuine transition. Adds PartialEq/Eq to ConsentPreference and a change-gating regression test.
Points the tauri-cef submodule at fix/cef-helper-verbose-logging, which gates the per-subprocess [cef-helper-*] eprintln! traces behind OPENHUMAN_CEF_HELPER_VERBOSE (silent by default). Note: takes effect in bundled builds only after reinstalling the vendored tauri-cli (cargo install --locked --path app/src-tauri/vendor/tauri-cef/crates/tauri-cli).
CoreStateProvider polled the expensive app_state_snapshot RPC every 2s for the life of the session, only slowing on repeated bootstrap failure. Once booted and authenticated the snapshot changes rarely and mostly via event-driven refreshes, so steady state now polls at STABLE_POLL_MS (5s); bootstrapping/unauthenticated stays at 2s so boot/login transitions still surface promptly.
…lth poll The frontend ran a second dedicated health_snapshot poller (daemonHealthService) alongside the app_state_snapshot poll. Fold the core's health snapshot into app_state_snapshot (new snake_case `health` field) and hydrate the daemon-health store from that one poll instead: - core: AppStateSnapshot carries HealthSnapshot; ComponentHealth/HealthSnapshot gain Deserialize. json_rpc_e2e asserts the folded health shape. - frontend: daemonHealthService drops all polling and becomes an ingestion sink (ingestHealthSnapshot: parse + store update + 30s disconnect watchdog); CoreStateProvider feeds each snapshot's health payload to it; useDaemonHealth no longer starts a poll. Older cores that omit `health` degrade gracefully. Supersedes the earlier health_snapshot poller dedupe (d34101f) — there is now no separate health poll at all. Regression tests rewritten for the sink API.
…e inference poll LocalAIDownloadSnackbar polled inference_status + inference_downloads_progress on its own timer even when idle. inference_status returns the same LocalAiStatus the app_state_snapshot already carries as runtime.localAi, so detect download activity from core state instead: when idle the snackbar issues ZERO inference calls; only once runtime.localAi.state reports loading/downloading/installing does it run the fast 2s poll for granular progress/speed/ETA (downloads-progress detail the 2-5s snapshot cadence can't provide), stopping when the download settles. LocalModelDebugPanel (settings-only, opt-in) keeps its own live poll.
The rule referenced `worktree/*` (singular) but the directory is `worktrees/`, so local `git worktree` checkouts under worktrees/ were never ignored and showed as untracked. Match the real directory name; keep the .gitkeep negation.
Updates .github/tauri-cef-expected-sha to match the tauri-cef submodule bump (11ef51ed), which gates the CEF helper's per-subprocess diagnostic prints behind OPENHUMAN_CEF_HELPER_VERBOSE. 11ef51ed is a direct child of the prior pin 5ec3d883, so the Linux AppImage glibc/NSS library-exclusion fixes the pin guards (tinyhumansai#1996, tinyhumansai#2032, tinyhumansai#2154/tinyhumansai#2088) are preserved. Submodule commit pushed to tinyhumansai/tauri-cef (branch fix/cef-helper-verbose-logging) so CI can resolve the pin.
- LocalAIDownloadSnackbar: gate isDownloading on coreDownloadActive so a download the core no longer reports active can't leave the overlay stuck visible; on a transient poll error keep retrying while core state is active (previously one blip permanently stopped progress polling for the download). - CoreStateProvider: ingest folded health AFTER the snapshot is committed and only when the refresh is still current, so daemon health is written under the freshly-committed identity, not a stale/pre-commit/superseded token. - coreStateApi: type health last_ok/last_error as string | null to match the Rust Option<String> wire contract. - keyring_consent::initialize now returns whether it applied a change, so the change-gate's suppressed write/log is asserted directly (not just unchanged state); config shell-hide-window parsing extracted into a testable classifier that distinguishes Unset (empty → silent) from Unrecognized (warns). - Add trace-level diagnostics to the no-op env/keyring paths and namespaced debug logging to the harness-init coalescing + snackbar poll lifecycles.
…pshots Folding health into app_state_snapshot coupled the 30s disconnect watchdog to an RPC allowed to run up to 90s (first-launch snapshots take 30–40s), so a slow-but- alive core could be marked `disconnected` mid-boot. Widen HEALTH_TIMEOUT_MS to 120s to cover one worst-case slow snapshot plus poll cadence; genuine disconnection (snapshots stop succeeding) is still detected. Addresses Codex review P2.
Per the repo's verbose-diagnostics guideline, log privacy-safe namespaced events for the folded-health ingest branch (request freshness, health presence, component count — never payload/tokens) and the poll-delay selection (delay + reason: bootstrap / authenticated / failure-backoff). Addresses CodeRabbit.
…tructure - Added `OrchestrationRedirect` component to handle legacy `/orchestration` route, redirecting to `/brain?tab=orchestration` with query parameter mapping. - Updated `AppRoutes` to use the new redirect and removed the direct route to `OrchestrationPage`. - Modified `Brain` component to include `orchestration` as a sub-tab, ensuring legacy deep links are correctly routed. - Enhanced documentation for clarity on routing changes and tab structure.
…cution - Improved the pre-push hook to handle abort signals (Ctrl+C/SIGTERM) more effectively, ensuring that the script exits with the correct status code. - Introduced a `run_check` function to encapsulate command execution and error handling, allowing for cleaner and more maintainable code. - Removed the `OrchestrationPage` component and its associated tests, as part of a broader refactor to streamline the application structure.
…gest The refactor armed the disconnect watchdog only inside a successful health parse, so a core whose app_state_snapshots never carry parseable health (repeated timeouts, after the one-shot agent probe already set `running`) would never arm a watchdog and stick at `running`. Arm a baseline watchdog when tracking starts (CoreStateProvider → ensureWatchdogArmed, idempotent) and treat any arriving snapshot as liveness that re-arms it — even a health-less/older-core payload — while only updating the store on a valid parse. Addresses Codex review P2.
Remove the top-level Orchestration sidebar tab and surface it as a Brain sub-tab (/brain?tab=orchestration) via the new OrchestrationView, whose top-level views (Overview/Chat/Agent graph/Tasks/Network) become an in-content chip row so Brain keeps a single sidebar. - navConfig: drop the orchestration NAV_TABS entry (6 tabs now) - settings + legacy /orchestration deep links redirect to the Brain tab, mapping ?tab=/?sub= onto the ?ov=/?sub= scheme - i18n: rename brain.tabs.tinyplaceOrchestration → brain.tabs.orchestration with real translations across all locales - tests: OrchestrationView unit tests, updated navConfig/Brain/nav-rail/e2e
The workflow-orchestration and session-orchestration capability entries still pointed at the retired 'Intelligence > Orchestration' path. Update both to the new home now that Orchestration is a Brain sub-tab.
Contributor
📝 WalkthroughWalkthroughThe pull request folds orchestration into Brain, migrates daemon health into app-state snapshots, changes local-AI and harness polling behavior, updates Rust configuration and consent handling, and adjusts repository tooling and navigation metadata. ChangesBrain orchestration integration
Daemon health snapshot pipeline
Polling reliability
Rust runtime behavior
Repository maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1546c38c4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the inference RPCs. When idle this component issues ZERO inference calls; | ||
| // the snapshot's `runtime.localAi.state` is what flips us into the fast poll. | ||
| const { snapshot: coreSnapshot } = useCoreState(); | ||
| const coreDownloadActive = isInFlightState(coreSnapshot.runtime.localAi?.state ?? undefined); |
There was a problem hiding this comment.
When a download starts just after an app_state_snapshot, runtime.localAi can remain at the cached idle value because build_runtime_snapshot serves cached runtime snapshots for the 10s RUNTIME_SNAPSHOT_TTL. Since this line makes the fast inference poll start only after coreDownloadActive flips, the snackbar won't call inference_status / inference_downloads_progress during that window, and short downloads can finish without ever showing progress; the previous 2s poll caught those transitions. Please trigger a refresh/fast poll from the download-start path or bypass the cached runtime for this predicate.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Deferred (design decision). This gap is inherent to the deliberate perf(local-ai): drive download snackbar off folded core state, no idle inference poll commit on this branch — starting a fast trigger from the download-start path or bypassing the cached runtime snapshot re-introduces the polling that commit intentionally removed. Leaving this thread open for the branch author to decide the tradeoff rather than silently reverting their optimization in this cleanup PR.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
app/src/components/LocalAIDownloadSnackbar.tsx (1)
39-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
currentState/in-flight derivation betweenisDownloadInFlightand the render body.The same "loading/downloading/installing else fall back to status.state ?? downloads.state ?? 'idle'" logic is written twice (lines 43-46 and 132-136), and line 134 re-implements
isInFlightStatevia manual string comparisons instead of reusing it. Consider extracting one sharedderiveCurrentState(status, downloads)helper and reusingisInFlightStatein both places to avoid future divergence between the polling-stop decision and the render-visibility decision.Also applies to: 132-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/LocalAIDownloadSnackbar.tsx` around lines 39 - 51, The current-state derivation is duplicated between isDownloadInFlight and the render body, with the render path manually comparing in-flight states. Extract a shared deriveCurrentState(status, downloads) helper using isInFlightState and the existing fallback order, then reuse it in both locations while preserving the current polling and visibility behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/AppRoutes.tsx`:
- Around line 49-67: Add namespaced, grep-friendly diagnostics to
OrchestrationRedirect covering redirect entry, the selected mapping branch, and
exit. Log only allowlisted branch identifiers and whether a session is present;
never include the session value or unsanitized query parameters.
In `@app/src/components/LocalAIDownloadSnackbar.tsx`:
- Around line 99-108: Update the settlement logic in the polling try block of
LocalAIDownloadSnackbar so settled is evaluated only when at least one of
statusRes.result or downloadsRes.result is present. Preserve the existing state
updates, and keep an empty successful response transient so it does not stop the
fast poll.
In `@app/src/providers/CoreStateProvider.tsx`:
- Around line 354-369: The health ingestion in the current-request branch of
CoreStateProvider must use the freshly resolved destination identity rather than
reading potentially stale React state through
daemonHealthService.ingestHealthSnapshot. Update the ingest path and its service
API to pass the resolved userId explicitly, or synchronize the non-React core
snapshot before ingestion, ensuring identity flips never store health under the
previous or __pending__ user.
In `@app/src/services/daemonHealthService.ts`:
- Around line 41-63: Add namespaced verbose diagnostics to ensureWatchdogArmed
and ingestHealthSnapshot covering initial arm versus existing watchdog,
unconditional rearm, payload presence and parse acceptance/rejection, and
whether the daemon store update is performed or skipped. Include only
privacy-safe structural metadata such as validity and component count; never log
health payload contents, and preserve the existing watchdog and store-update
behavior.
In `@tests/json_rpc_e2e.rs`:
- Around line 6078-6100: Update the health assertions in the JSON-RPC contract
test to require the top-level health.updated_at field and validate that it is a
string, alongside the existing pid, uptime_seconds, and components checks.
---
Nitpick comments:
In `@app/src/components/LocalAIDownloadSnackbar.tsx`:
- Around line 39-51: The current-state derivation is duplicated between
isDownloadInFlight and the render body, with the render path manually comparing
in-flight states. Extract a shared deriveCurrentState(status, downloads) helper
using isInFlightState and the existing fallback order, then reuse it in both
locations while preserving the current polling and visibility behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16b2e649-2521-4fa5-9006-e62fcf038295
📒 Files selected for processing (54)
.github/tauri-cef-expected-sha.gitignore.husky/pre-pushapp/src-tauri/vendor/tauri-cefapp/src/AppRoutes.tsxapp/src/components/InitProgressScreen/HarnessInitOverlay.test.tsxapp/src/components/InitProgressScreen/HarnessInitOverlay.tsxapp/src/components/LocalAIDownloadSnackbar.tsxapp/src/components/__tests__/LocalAIDownloadSnackbar.test.tsxapp/src/components/layout/shell/CollapsedNavRail.test.tsxapp/src/components/orchestration/AgentChatPanel.tsxapp/src/components/orchestration/ConnectionsPanel.tsxapp/src/components/orchestration/OrchestrationView.tsxapp/src/components/orchestration/__tests__/OrchestrationView.test.tsxapp/src/components/settings/settingsRouteElements.tsxapp/src/config/__tests__/navConfig.test.tsapp/src/config/navConfig.tsapp/src/features/conversations/Conversations.tsxapp/src/features/conversations/components/InterruptedAnswer.tsxapp/src/hooks/useDaemonHealth.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/Brain.tsxapp/src/pages/OrchestrationPage.tsxapp/src/pages/__tests__/Brain.test.tsxapp/src/pages/__tests__/OrchestrationPage.test.tsxapp/src/providers/CoreStateProvider.tsxapp/src/services/__tests__/daemonHealthService.test.tsapp/src/services/coreStateApi.tsapp/src/services/daemonHealthService.tsapp/src/store/__tests__/chatRuntimeSlice.thunk.test.tsapp/src/store/chatRuntimeSlice.test.tsapp/test/e2e/specs/navigation.spec.tssrc/openhuman/about_app/catalog_data.rssrc/openhuman/app_state/ops.rssrc/openhuman/config/schema/load/env_overlay.rssrc/openhuman/config/schema/load_tests.rssrc/openhuman/health/core.rssrc/openhuman/keyring_consent/policy.rssrc/openhuman/keyring_consent/types.rstests/json_rpc_e2e.rsworktrees/.gitkeep
💤 Files with no reviewable changes (3)
- app/src/pages/tests/OrchestrationPage.test.tsx
- app/src/pages/OrchestrationPage.tsx
- app/src/components/layout/shell/CollapsedNavRail.test.tsx
- AppRoutes: add privacy-safe entry/exit diagnostics to OrchestrationRedirect (allowlisted branch ids + session-presence only) and cover it with a new OrchestrationRedirect.test.tsx exercising every mapping branch. - LocalAIDownloadSnackbar: treat an empty-but-successful RPC response as transient, not "download complete", so a soft failure no longer freezes the fast poll for the rest of the download. - daemonHealthService: resolve the health-store user from the refresh's own sessionToken passed by CoreStateProvider, not the deferred non-React store (commitState writes it inside a React setState updater) — fixes health being filed under the prior/__pending__ user during an identity flip. Add privacy-safe watchdog/ingest diagnostics. - json_rpc_e2e: assert health.updated_at in the RPC contract test (the frontend rejects the payload without it).
Folding Orchestration under Brain mounted the full Brain component for /brain?tab=orchestration (and the redirected /orchestration), whose effect unconditionally called memoryTreeGraphExport on mount + memory-tree events — an unrelated graph RPC the standalone Orchestration page never issued. Gate the effect off when the orchestration tab is active (re-running when returning to a graph-bearing tab). Addresses Codex review on PR tinyhumansai#5080.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dcd7cc66d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Brain calls useSubconscious() unconditionally, which fires subconsciousStatus / openhumanHeartbeatSettingsGet and installs a 5s poll. Folding Orchestration under Brain meant opening Brain > Orchestration (and the redirected /orchestration) now kept those unrelated heartbeat/subconscious RPCs running — which the standalone OrchestrationPage never did. Add an `enabled` param to useSubconscious (default true, back-compat) that skips the initial fetch + poll when false, and pass `activeTab === 'subconscious'` from Brain. The status is consumed only on that tab. Addresses Codex review on PR tinyhumansai#5080.
coderabbitai
Bot
added
bug
feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/hooks/__tests__/useSubconscious.test.ts (1)
84-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover enabled-state transitions, not only initial disabled mounting.
Add a
rerendertest that starts enabled, verifies polling, switches tofalseand verifies it stops, then switches back totrueand verifies polling resumes. This would exercise the cleanup and dependency behavior introduced by the hook change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/hooks/__tests__/useSubconscious.test.ts` around lines 84 - 98, Add a rerender-based test for useSubconscious that mounts enabled and verifies RPC polling, rerenders with false and confirms polling stops, then rerenders with true and confirms polling resumes. Use the existing subconsciousStatus and openhumanHeartbeatSettingsGet mocks and timer advancement, ensuring the assertions exercise cleanup and dependency behavior across enabled-state transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/hooks/useSubconscious.ts`:
- Around line 160-167: Add namespaced debug diagnostics to the polling effect
around the enabled guard and interval lifecycle: log when polling is skipped
because enabled is false, when the interval is started, and when cleanup tears
it down. Use the repository’s established diagnostics/logging mechanism and
fixed grep-friendly messages only; do not include sensitive data.
- Around line 160-167: Update the refresh lifecycle in useSubconscious so
disabling and re-enabling cannot allow overlapping requests or stale responses
to update state. Keep fetchingRef.current associated with the active refresh
request, or add generation/abort handling so cleanup invalidates the prior
request and its results are ignored; preserve the existing interval behavior for
enabled state.
In `@app/src/services/daemonHealthService.ts`:
- Around line 162-172: Update startHealthTimeout to associate each scheduled
callback with the currently active timeout identity or generation, and have
callbacks return without changing status or clearing healthTimeoutId when they
are stale. Preserve the existing disconnected transition only for the active
watchdog, and add a fake-timer regression test covering re-arming exactly at the
timeout boundary.
---
Nitpick comments:
In `@app/src/hooks/__tests__/useSubconscious.test.ts`:
- Around line 84-98: Add a rerender-based test for useSubconscious that mounts
enabled and verifies RPC polling, rerenders with false and confirms polling
stops, then rerenders with true and confirms polling resumes. Use the existing
subconsciousStatus and openhumanHeartbeatSettingsGet mocks and timer
advancement, ensuring the assertions exercise cleanup and dependency behavior
across enabled-state transitions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79765fd8-6ac8-4e88-b5bf-9b7feff41025
📒 Files selected for processing (10)
app/src/AppRoutes.tsxapp/src/components/LocalAIDownloadSnackbar.tsxapp/src/hooks/__tests__/useSubconscious.test.tsapp/src/hooks/useSubconscious.tsapp/src/pages/Brain.tsxapp/src/pages/__tests__/Brain.test.tsxapp/src/pages/__tests__/OrchestrationRedirect.test.tsxapp/src/providers/CoreStateProvider.tsxapp/src/services/daemonHealthService.tstests/json_rpc_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/json_rpc_e2e.rs
- app/src/pages/tests/Brain.test.tsx
- app/src/pages/Brain.tsx
- app/src/providers/CoreStateProvider.tsx
- app/src/components/LocalAIDownloadSnackbar.tsx
Comment on lines
+160
to
+167
| if (!enabled) return; | ||
| refresh(); | ||
| const interval = setInterval(refresh, 5000); | ||
| return () => { | ||
| clearInterval(interval); | ||
| fetchingRef.current = false; | ||
| }; | ||
| }, [refresh]); | ||
| }, [refresh, enabled]); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add namespaced diagnostics for the polling lifecycle.
The new skip/start/cleanup branches should emit grep-friendly debug logs for disabled polling, interval setup, and teardown without logging sensitive data. This is required for new or changed flows by the repository diagnostics guideline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/hooks/useSubconscious.ts` around lines 160 - 167, Add namespaced
debug diagnostics to the polling effect around the enabled guard and interval
lifecycle: log when polling is skipped because enabled is false, when the
interval is started, and when cleanup tears it down. Use the repository’s
established diagnostics/logging mechanism and fixed grep-friendly messages only;
do not include sensitive data.
Source: Coding guidelines
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent overlapping refreshes across enabled-state transitions.
When enabled changes to false, cleanup resets fetchingRef.current without cancelling or invalidating the existing refresh(). A rapid false → true transition can start a second Promise.all; whichever response finishes last may overwrite status/settings with stale data. Keep the in-flight guard tied to the request, or add request-generation/abort handling that ignores stale results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/hooks/useSubconscious.ts` around lines 160 - 167, Update the refresh
lifecycle in useSubconscious so disabling and re-enabling cannot allow
overlapping requests or stale responses to update state. Keep
fetchingRef.current associated with the active refresh request, or add
generation/abort handling so cleanup invalidates the prior request and its
results are ignored; preserve the existing interval behavior for enabled state.
Comment on lines
+162
to
172
| private startHealthTimeout(sessionToken?: string | null): void { | ||
| if (this.healthTimeoutId) { | ||
| clearTimeout(this.healthTimeoutId); | ||
| } | ||
|
|
||
| const userId = this.getUserId(); | ||
| const userId = this.getUserId(sessionToken); | ||
| this.healthTimeoutId = setTimeout(() => { | ||
| console.warn('[DaemonHealth] Health timeout reached - setting status to disconnected'); | ||
| setDaemonStatus(userId, 'disconnected'); | ||
| this.healthTimeoutId = null; | ||
| }, this.HEALTH_TIMEOUT_MS); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant service and test files.
git ls-files 'app/src/services/daemonHealthService.ts' 'app/src/services/__tests__/daemonHealthService.test.ts'
echo '--- daemonHealthService.ts outline ---'
ast-grep outline app/src/services/daemonHealthService.ts --view expanded || true
echo '--- daemonHealthService.test.ts outline ---'
ast-grep outline app/src/services/__tests__/daemonHealthService.test.ts --view expanded || true
echo '--- relevant snippets ---'
sed -n '130,230p' app/src/services/daemonHealthService.ts
echo '--- tests around timer behavior ---'
sed -n '1,260p' app/src/services/__tests__/daemonHealthService.test.tsRepository: tinyhumansai/openhuman
Length of output: 8538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for timer re-arming, clearTimeout, and status transitions in the service.
rg -n "healthTimeoutId|clearTimeout|setTimeout|disconnected|re-arm|snapshot" app/src/services/daemonHealthService.ts app/src/services/__tests__/daemonHealthService.test.tsRepository: tinyhumansai/openhuman
Length of output: 6707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact service/test snippets and look for existing coverage.
sed -n '1,260p' app/src/services/daemonHealthService.ts
echo '---'
sed -n '1,260p' app/src/services/__tests__/daemonHealthService.test.ts
echo '--- timer usage ---'
rg -n "startHealthTimeout|clearTimeout|setTimeout|healthTimeoutId|disconnected" app/src/services/daemonHealthService.ts app/src/services/__tests__/daemonHealthService.test.ts
# Small Node probe: once a timeout callback is already queued/running, clearTimeout cannot stop it.
node - <<'JS'
const events = [];
let id = setTimeout(() => {
events.push('old-fired');
}, 0);
setTimeout(() => {
clearTimeout(id);
id = setTimeout(() => {
events.push('new-fired');
}, 10);
}, 0);
setTimeout(() => {
console.log(events.join(','));
}, 30);
JSRepository: tinyhumansai/openhuman
Length of output: 15874
Guard against stale watchdog callbacks. A timeout that’s already queued can still run after clearTimeout(), so an old watchdog can flip a healthy daemon to disconnected and clear the replacement timer. Track the active timeout by identity or generation and ignore stale callbacks; add a fake-timer regression test that re-arms at the timeout boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/services/daemonHealthService.ts` around lines 162 - 172, Update
startHealthTimeout to associate each scheduled callback with the currently
active timeout identity or generation, and have callbacks return without
changing status or clearing healthTimeoutId when they are stale. Preserve the
existing disconnected transition only for the active watchdog, and add a
fake-timer regression test covering re-arming exactly at the timeout boundary.
12 tasks
Member
Author
|
Superseded by #5075, which merged the same Orchestration-under-Brain fold + daemon-health/local-AI cleanup into |
6 tasks
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/brain?tab=orchestration) via a newOrchestrationView, whose views (Overview/Chat/Agent graph/Tasks/Network) become an in-content chip row so Brain keeps a single sidebar. Legacy/orchestration+/brain/tinyplace-orchestrationdeep links redirect, mapping?tab=/?sub=onto the new?ov=/?sub=scheme.app_state_snapshot(drop the separate health poll), arm the baseline disconnect watchdog independent of first ingest, widen the watchdog to tolerate slow snapshots, and dedupe concurrent health-snapshot pollers.app_state_snapshotto 5s once stable, drive the local-AI download snackbar off folded core state (no idle inference poll), coalesce duplicate bootharness_init_statuspolls.OPENHUMAN_SHELL_HIDE_WINDOWas unset, change-gate keyring-consent cache init to stop per-snapshot log spam.about_appOrchestrationhow_tostrings at Brain > Orchestration.Problem
app_statesnapshots.Solution
OrchestrationView(chip-nav, query-param driven) embedded inBrain.tsx; dropOrchestrationPageand its top-levelNAV_TABSentry; add redirects inAppRoutes.tsxand update the settings/braindeep links. Rename the stalebrain.tabs.tinyplaceOrchestrationi18n key tobrain.tabs.orchestrationwith real translations across all locales.app_state_snapshotstream and make the disconnect watchdog time-based rather than ingest-gated, with dedup guards.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/ci-lite.yml; focused Vitest suites + Rust checks run locally, full gate enforced by CI.docs/TEST-COVERAGE-MATRIX.mdrows changed.Impact
Brain > Orchestration; old deep links redirect, so bookmarks keep working. No behaviour change to the orchestration surfaces themselves.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
chore/app-cleanup1546c38c4Validation Run
pnpm --filter openhuman-app format:checkpnpm typechecksrc/components/orchestration,src/config,src/pages/__tests__/Brain.test.tsx,src/components/layout/shell/CollapsedNavRail.test.tsx(88 passing);pnpm i18n:check/i18n:english:checkcleancargo fmt --all --check,GGML_NATIVE=OFF cargo check -p openhumancargo check --manifest-path app/src-tauri/Cargo.toml(via pre-push hook)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
OrchestrationRedirectmaps?tab=/?sub=→?ov=/?sub=; Brain legacytinyplace-orchestrationslug bounces to the sub-tab.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Translations