starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-20 08:03:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

rm context files

Browse Source
This commit is contained in:
Jinjing
2026-08-01 13:12:14 -07:00
parent 086ae5f0a0
commit fd154bfbf8
2 changed files with 0 additions and 233 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CONTEXT.md
-76
View File
@@ -1,76 +0,0 @@
# Context pack — PR #11950 / Cluster A (React #185 terminal.workbench)
You are in worktree `crash-a-11950-react185`, checked out from **PR #11950** branch `fix/crash-a-active-terminal-repair-loop`.
**Parent triage:** `/Users/jinjingliang/Documents/projects/orca/1.4.163-fix-crashes`
**Local copies:** `triage-context/TRIAGE-REPORT.md`, `AGENT-TASKS.md`, sample reports under `triage-context/reports/`.
## Why this exists
Orca **1.4.163** field crashes (31 stable reports, 2026-08-01 soak):
| Cluster | n | This PR? |
|---|---|---|
| **A terminal.workbench React #185** | **16** | **YES — this PR** |
| D heap OOM / high-heap | ~7 | no (#11954) |
| E Windows GPU | ~4 | no (crash-c4 / #11966 / #11940) |
| B settings #185 | 1 | no (#11962) |
**This is the largest 1.4.163 crash cluster.** Fix is open, CI green, **not merged**, **not in v1.4.163**.
## Field signature (must match)
- Reason: `react-error-boundary`
- Process: `react-render`
- `boundary_id: terminal.workbench`
- Error: `Minified React error #185` (Maximum update depth exceeded)
- Stack (stable):
`getRootForUpdatedFiber ← enqueueConcurrentHookUpdate ← dispatchSetStateInternal ← dispatchSetState ← commitHookEffectListMount ← commitPassiveMountOnFiber`
- Component tops: **TerminalPaneOverlayLayer$1** (~13/16), **SortableTab** (~3/16)
- Platforms: macOS + Windows; heavy repeater **SeungGiJeong ×9**, also LesleyMurfin
Sample report IDs (full text in `triage-context/reports/`):
- `d9434d07` LesleyMurfin Win SortableTab + 70 panes (Slack example)
- `fb339a6b` LesleyMurfin Win Overlay
- `1051d66f`, `1828176e`, `1bdb9c23` SeungGiJeong macOS
- `30eb3a56`, `79ebdb5d`, `bdf0db48` Win Overlay
## Claimed root cause (verify, do not rubber-stamp)
1. Same terminal **tab id** can appear under **two worktree keys** (SSH path rename / repo re-add minting new worktree id while stale key retains tab).
2. `setActiveTab` first-match `Object.entries` ownership: if non-active worktree scanned first → refuses global `activeTabId` write.
3. `Terminal.tsx` repair effect sees “active tab not in active worktree list” forever → loops → #185.
4. Fix: `active-tab-owner-worktree.ts` prefers active worktree for ownership; breadcrumb `terminal_tab_id_owned_by_multiple_worktrees`.
**Honest limits from PR body (still check):**
- Throwing frame is local `useState` in passive effect (victim); repair is driver.
- Overlay measure↔fit (#10026) already shipped — do not re-open without new evidence.
- Competing cold-parking (C5) hypothesis was checked; park churn rare in bundles.
## Your job
1. **Read PR #11950** body + full diff in this worktree.
2. **Read** `triage-context/TRIAGE-REPORT.md` Cluster A section + sample reports.
3. **Review for:**
- Correctness of ownership fix under duplicate tab ids
- Functionality regressions (wrong tab activated, SSH remotes, multi-worktree, sortEpoch side effects)
- Performance regressions (hot path O(worktrees×tabs) on title frames / every render)
- Whether production stacks actually require this fix vs residual Overlay loop
4. **Run** the react185 harness:
```
npx vitest run -c config/vitest.config.ts \
src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx \
src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.react185.test.tsx
```
5. If high-confidence gaps: minimal fix + tests. Else write **REVIEW-11950.md** with ship / ship-with-nits / block.
6. `worker_done` with recommendation and residual risks.
## Do not
- Conflate with Settings #185 (#11962) or GPU (#10624).
- “Fix” by swallowing #185 in the error boundary.
- Expand into heap OOM.
## PR
https://github.com/stablyai/orca/pull/11950
REVIEW-11950.md
-157
View File
@@ -1,157 +0,0 @@
# PR #11950 review — React #185 at `terminal.workbench`
Date: 2026-08-01
Reviewed remote head: `bf184ac400`
Base: `origin/main` at `16c5526dfd`
## Verdict
**SHIP WITH NITS after committing the local amendments in this worktree.**
The core diagnosis is sound: duplicate terminal entity IDs can make `setActiveTab` refuse the
global active-tab repair while reallocating an effect dependency forever. Preferring the active
worktree plus preserving `activeTabIdByWorktree` identity makes the repair converge, without
changing healthy single-owner behavior.
The remote PR head had one medium functionality gap and one low edge-case gap. Both are fixed and
tested locally; no blocking correctness or performance finding remains in the amended worktree.
## Findings fixed during review
### Medium — unified activation could still choose the stale/background duplicate
`setActiveTab` repaired legacy ownership to the active worktree, then independently searched
`Object.values(unifiedTabsByWorktree).flat()` and activated the first unified tab with the same
entity ID. When the stale/background key was inserted first, the React loop stopped but the active
worktree's split group remained on its previous tab.
The new regression test failed on the PR head with:
```text
expected 'u-previous' to be 'u-active'
```
The local amendment scopes unified lookup to the owner selected by
`resolveActiveTabOwnerWorktreeId`, retaining the old global fallback for unified-only terminal
rows. This also reduces the normal activation lookup from all unified worktrees to the owning
worktree.
### Low — null and falsy owner checks disagreed with the resolver contract
The resolver deliberately supports an empty-string worktree ID, but `setActiveTab` used a
truthiness check before updating `activeTabIdByWorktree`. It also treated `null` owner and `null`
active worktree as an ownership match, allowing an ownerless ID to become globally active and to
clear a stale unread signal.
The local amendment uses explicit `!== null` ownership checks. Tests pin the empty-string owner,
ownerless activation, and unread preservation cases.
### Nit — diagnostic comments overstated the breadcrumb cap and were stale
The guard is capped at 256 **verdict keys**, not 256 distinct tab IDs; one ID can consume both a
`true` and `false` key. Comments now describe the actual 128–256-tab coverage and the known
hydration path while preserving the honest fact that existing field reports predate the signal.
## Correctness review
- Duplicate ID, active worktree owns the ID: the resolver returns the active worktree regardless
of insertion order; `activeTabId`, remembered activation, and unified group activation now agree.
- Duplicate ID, active worktree does not own the ID: behavior falls back to the first legacy owner,
preserving background activation and bell semantics.
- Single owner: behavior is equivalent to the prior first-match resolver.
- No owner: global terminal selection and unread state remain unchanged; unified-only activation
still uses the compatibility fallback.
- Redundant activation: `activeTabIdByWorktree` retains identity, removing the repair effect's
self-trigger even if ownership becomes anomalous again.
- SSH, folder workspaces, native worktrees, macOS, Windows, and Linux share this renderer-only
logic. The resolver makes no path, Git, or host assumptions.
The direct-SSH hydration regression test is meaningful: it exercises snapshot application,
path-based remapping, stale-key retention, the real store action, and repair convergence. Its
limits remain correctly documented: catalog ID change and transport are modelled, three IPC
boundaries are stubbed, and `reconnectPersistedTerminals` is load-bearing.
## Functionality review
Normal-state tab selection, background bell retention, and unified-only terminal activation are
preserved. The local unified-owner amendment closes the main wrong-tab risk introduced by applying
the new active-owner preference to only half of the activation model.
Known duplicate-state ambiguity remains outside the crash fix:
- `updateTabTitle` and `clearTabLaunchAgent` still use the last-writer-wins owner cache.
- `setRuntimePaneTitle` cannot distinguish a background pane from an active pane when both share
the same tab ID. The PR chooses to suppress a sort bump if the active worktree owns that ID; a
genuine background copy can therefore miss a re-sort.
- Hydration still retains the stale worktree key. This PR makes the broken state safe; it does not
clean the state.
These do not justify widening the P0 crash patch. They should be resolved together by stale-key
cleanup or by making ambiguous APIs carry worktree identity.
## Performance review
| Path | Added work | Assessment |
|---|---:|---|
| `setActiveTab` | `O(worktrees × terminal tabs)` ownership scan | Discrete activation/IPC path, not render, pointer, title-frame, or PTY-byte hot path |
| Unified activation | Owner-array scan, global fallback only on model mismatch | Net improvement over unconditional `Object.values().flat()` |
| Background classified title change | Up to `O(active-worktree tabs)` membership scan | Technically new work in healthy background cases, but bounded and only on classification changes |
| Duplicate breadcrumb | One full resolver pass plus at most two emissions per tab ID | Renderer set capped at 256 verdict keys; main-process coalescing bounded to two keys |
The PR body's statement that the non-duplicated title path is entirely unchanged is slightly too
strong: a normal background title classification now scans the active worktree's tab list after
the cheap owner inequality. That is not an actionable regression; it avoids the much worse
`O(all worktrees × tabs)` resolver on title frames and is negligible at the field sizes reviewed.
## Does the production stack require this fix?
The field stack is compatible with this driver, but it does **not uniquely require** it. All eight
provided samples match the six-frame passive `useState` dispatch signature, and the checked-in
repair harness proves non-convergence. The PR's production React experiment further demonstrates
that a descendant passenger can throw the exact field stack while the Zustand repair loop is the
driver.
That establishes a credible causal path, not direct attribution of every field report:
- `TerminalPaneOverlayLayer` and `SortableTab` are descendant stack tops, not necessarily drivers.
- The prior overlay geometry loop remains covered and passes; there is no new evidence to reopen
the already-shipped #10026 fix.
- The two later LesleyMurfin samples contain `terminal_park_verdict_churn`, although the churn is
not temporally sufficient to prove a cold-parking loop and one report follows a reload.
- `bdf0db48` also records an earlier #185 at `scrollToEnd`, evidence that this surface can host
more than one update-depth driver.
- Several samples show repeated worktree activation, which supports the repair-path model, while
others do not expose a trigger in their retained activity window.
Therefore the P0 fix should ship, but the cluster should remain monitored. On versions newer than
1.4.163:
1. Check `terminal_tab_id_owned_by_multiple_worktrees` on every continuing
`terminal.workbench` #185.
2. If present, prioritize stale-key cleanup and capture the SSH/repo-remap context.
3. If absent, investigate cold-parking/local passive setters and `scrollToEnd`; do not assume the
overlay geometry loop regressed without new geometry evidence.
## Verification
- Mandatory React #185 command: **2 files, 11 tests passed**.
- Changed-path targeted suite: **6 files, 66 tests passed**.
- Broader renderer store + terminal-pane suite: **364 files, 5,119 tests passed**.
- `oxlint` on all PR files: passed.
- `oxfmt --check` on all PR files: passed after formatting the local amendment.
- Web typecheck: `npx tsc --noEmit -p config/tsconfig.tc.web.json` passed.
- Node typecheck: `npx tsc --noEmit -p config/tsconfig.node.json` passed.
- Remote PR checks were green at the reviewed head.
The repeated npm warnings about `shamefully-hoist` and `minimum-release-age` are unrelated project
configuration warnings; no test or validation command failed because of them.
## Remaining nits
- Update the PR body from “once per tab ID” to “once per tab ID per verdict.”
- Avoid saying the breadcrumb identifies “which duplicate”; its payload intentionally contains no
tab/worktree identity and only proves owner count plus active-owner resolution.
- The production-bundle end-to-end harness is documented rather than committed. The checked-in
convergence and hydration tests are sufficient for this patch, but a future isolated production
React config would make that evidence reproducible without reconstructing the PR experiment.