Skip to content

refactor(ui): harden Lit lifecycle boundaries#102745

Merged
steipete-oai merged 6 commits into
mainfrom
codex/ui-lit-lifecycle-seams
Jul 9, 2026
Merged

refactor(ui): harden Lit lifecycle boundaries#102745
steipete-oai merged 6 commits into
mainfrom
codex/ui-lit-lifecycle-seams

Conversation

@steipete-oai

@steipete-oai steipete-oai commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Closes #102423

What Problem This Solves

Resolves a class of Control UI failures where retained elements could keep listening to a replaced application capability, or publish an obsolete async result after a disconnect, reconnect, provider replacement, route change, or detach. These races could leave stale lists, errors, loading locks, model overrides, task suggestions, token side effects, plugin polling, or navigation state visible in the current UI.

Why This Change Was Made

The Control UI now has shared light-DOM Lit bases for live locale updates and a SubscriptionsController that owns replaceable external sources. Page, capability, router, overlay, terminal, chat, modal, plugin, and session work is scoped to explicit source/connection generations; stale completions become inert and retired resources are released at the owning lifecycle boundary. All production Lit elements—including the newly added Sessions context menu—use the shared base, while upstream Talk, split-view, Codex Sessions, and Settings behavior remains intact.

This is AI-assisted maintainer work. The implementation and every accepted review finding were inspected and corrected before submission.

User Impact

The Control UI remains deterministic across reconnects and in-place runtime replacement: language changes propagate without remounting, stale data and errors cannot overwrite current state, sensitive device-token responses cannot escape a retired request epoch, and chat/plugin/session surfaces cleanly release their old work.

Evidence

  • Exact-head strict UI test typecheck passed: node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.test.ui.json ....
  • Exact-head focused Vitest coverage passed for lifecycle controllers, capability/source replacement, chat task suggestions, device-token rotation/revocation, Sessions context-menu deletion, plugin/Codex bundled views, and reconnect/detach behavior.
  • Changed TypeScript/CSS files pass oxfmt --check; git diff --check passes.
  • Mandatory structured Codex autoreview completed clean after the final conflict integrations.
  • Earlier broad remote baseline passed all 2,382 UI unit tests, all 78 UI browser E2E tests, the production UI build, and check:changed; exact PR-head broad CI is pending because the local Blacksmith Testbox executable was unavailable.

@steipete-oai
steipete-oai requested a review from a team as a code owner July 9, 2026 11:24
@github-actions github-actions Bot added the dependencies-changed PR changes dependency-related files label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Dependency Guard

This PR changes dependency-related files. Maintainers should confirm these changes are intentional.

Changed files:

  • pnpm-lock.yaml
  • ui/package.json

Maintainer follow-up:

  • Review whether the dependency changes are intentional.
  • Inspect resolved package deltas when lockfile, shrinkwrap, or workspace dependency policy changes are present.
  • Treat package-lock.json and npm-shrinkwrap.json diffs as security-review surfaces.
  • Run pnpm deps:changes:report -- --base-ref origin/main --markdown /tmp/dependency-changes.md --json /tmp/dependency-changes.json locally for detailed release-style evidence.

@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui size: XL maintainer Maintainer-authored PR and removed dependencies-changed PR changes dependency-related files labels Jul 9, 2026
@github-actions github-actions Bot added the dependencies-changed PR changes dependency-related files label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Dependency graph change authorized

This PR includes dependency graph changes. A repository admin or member of @openclaw/openclaw-secops authorized this exact head SHA with /allow-dependencies-change.

  • Approved SHA: 7b23de34db10111380d00dffe7ca853f668dc566
  • Approved by: @steipete
  • Reason: Declare the existing production @lit/context import in the Control UI package.

A later push changes the PR head SHA and requires a fresh security approval.

@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 8:13 AM ET / 12:13 UTC.

Summary
The branch refactors Control UI Lit components around shared light-DOM/i18n base elements, a subscription lifecycle controller, route/chat lifecycle ownership, and reconnect-safe page state handling, with broad regression tests and a UI dependency declaration for @lit/context.

Reproducibility: no. high-confidence live browser reproduction was established. Current-main source and the linked issue show the repeated lifecycle/subscription patterns the PR targets, but the observable races remain source-level rather than reproduced in a real UI session here.

Review metrics: 1 noteworthy metric.

  • Dependency graph surfaces: 1 package dependency added, 1 lockfile importer entry added. This is the concrete surface that triggers Dependency Guard and requires security/admin review before merge.

Stored data model
Persistent data-model change detected: serialized state: ui/src/pages/sessions/route.ts, serialized state: ui/src/pages/sessions/sessions-page.test.ts, serialized state: ui/src/pages/sessions/sessions-page.ts, unknown-data-model-change: ui/src/pages/sessions/sessions-page.test.ts, unknown-data-model-change: ui/src/pages/sessions/sessions-page.ts, unknown-truncated-pull-files. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #102423
Summary: The open lifecycle-hardening issue is the canonical tracking item, and this PR is its current candidate implementation path.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Get security/admin approval for the exact-head dependency graph change or remove that delta.
  • [P1] Add redacted real Control UI proof for representative reconnect, route, locale, or chat lifecycle behavior.
  • Rebase/refresh the branch after the base drift before final merge review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and CI provide typecheck/test/build evidence, but no redacted real Control UI browser proof or runtime transcript showing the changed lifecycle behavior after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A supported web UI chat proof would help cover the PR's chat lifecycle and draft/scroll behavior, though it would not replace dependency approval. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis web UI chat proof: verify that chat draft and scroll state remain correct across a route change or reconnect, and capture redacted evidence.

Risk before merge

  • [P1] The current head changes ui/package.json and pnpm-lock.yaml; Dependency Guard blocks that external dependency graph change until security/admin approval or removal.
  • [P1] The PR body lists typecheck, Vitest, and CI evidence, but I did not find real browser/runtime proof showing the reconnect, route, locale, chat, or session lifecycle behavior after the refactor.
  • [P1] The PR is marked behind the base branch, so maintainers should refresh/rebase before treating the exact-head review and CI as final.

Maintainer options:

  1. Approve dependency after security review (recommended)
    Review the @lit/context 1.1.6 package and current lockfile delta, then authorize this exact head if the graph change is intentional.
  2. Remove dependency delta before merge
    If security/admin approval is not intended, revise the branch so ui/package.json and pnpm-lock.yaml no longer change.

Next step before merge

  • [P1] Manual maintainer/security review is needed for the dependency graph approval and protected maintainer-labeled broad UI refactor; there is no narrow safe automated repair until that decision is made.

Maintainer decision needed

  • Question: Should maintainers/security approve the @lit/context Control UI dependency graph change for head 7b23de34db10111380d00dffe7ca853f668dc566, or require a revision that removes the manifest/lockfile change before merge?
  • Rationale: The code path uses @lit/context directly and current main already has the package pinned at the root, but repository policy and the live Dependency Guard require a human security/admin decision for external PR dependency graph changes.
  • Likely owner: steipete — He has the strongest recent Control UI ownership signal and authored the current lifecycle-hardening branch.
  • Options:
    • Approve the exact-head dependency change (recommended): Security/admin reviews the already-pinned @lit/context package and authorizes the current head before the PR is considered landable.
    • Require no dependency graph change: The branch is revised to remove ui/package.json and lockfile dependency changes, accepting that maintainers must verify the UI package still declares its runtime imports correctly.
    • Pause for a smaller owned slice: Maintainers split the lifecycle refactor and dependency decision into smaller PRs if this broad branch is too large to review safely.

Security
Needs attention: The functional UI refactor has no obvious credential or sandbox regression in the sampled paths, but the PR changes the dependency graph and is blocked by the repository supply-chain guard.

Review findings

  • [P1] Resolve the dependency graph block before merge — ui/package.json:13
Review details

Best possible solution:

Keep the lifecycle-controller direction, but land it only after security/admin dependency approval for the exact head, fresh post-rebase CI, and redacted real Control UI proof for representative reconnect/route/chat behavior.

Do we have a high-confidence way to reproduce the issue?

No high-confidence live browser reproduction was established. Current-main source and the linked issue show the repeated lifecycle/subscription patterns the PR targets, but the observable races remain source-level rather than reproduced in a real UI session here.

Is this the best way to solve the issue?

Unclear as merge-ready: the controller/base-element approach matches the affected Lit lifecycle boundary, but the unapproved dependency graph change and missing real behavior proof keep this from being the final best fix yet.

Full review comments:

  • [P1] Resolve the dependency graph block before merge — ui/package.json:13
    This branch adds @lit/context to the Control UI package dependency graph, and the live Dependency Guard says external PR dependency graph changes are blocked until repository admin or security authorizes the exact head. Without that approval or a revision that removes the graph change, merging would bypass the repo's supply-chain gate.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.78

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f786efddcd88.

Label changes

Label changes:

  • add P2: The PR addresses medium-severity intermittent Control UI lifecycle/state drift rather than an urgent setup blocker or core runtime outage.
  • add merge-risk: 🚨 security-boundary: The branch changes the dependency graph in an external PR, and the live guard blocks it pending security/admin authorization.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and CI provide typecheck/test/build evidence, but no redacted real Control UI browser proof or runtime transcript showing the changed lifecycle behavior after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: The PR addresses medium-severity intermittent Control UI lifecycle/state drift rather than an urgent setup blocker or core runtime outage.
  • merge-risk: 🚨 security-boundary: The branch changes the dependency graph in an external PR, and the live guard blocks it pending security/admin authorization.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body and CI provide typecheck/test/build evidence, but no redacted real Control UI browser proof or runtime transcript showing the changed lifecycle behavior after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

Security concerns:

  • [medium] Unapproved UI dependency graph change — ui/package.json:13
    ui/package.json adds @lit/context and pnpm-lock.yaml adds the UI importer entry; Dependency Guard requires security/admin approval for this exact head before merge.
    Confidence: 0.92

What I checked:

  • Repository policy applied: Root AGENTS.md and ui/AGENTS.md were read; the root policy treats lockfile/dependency changes as a security surface and the UI guide applies to generated i18n metadata touched by this PR. (AGENTS.md:13, f786efddcd88)
  • Dependency graph change: The PR adds @lit/context to the Control UI package dependencies, and the lockfile adds the matching UI importer entry. (ui/package.json:13, 7b23de34db10)
  • Dependency Guard block: Live PR timeline shows Dependency Guard explicitly blocking dependency graph changes on head 7b23de34db10111380d00dffe7ca853f668dc566 until repository admin or security authorization. (7b23de34db10)
  • Shared lifecycle controller: The new SubscriptionsController owns late-arriving/replaced sources and disconnect cleanup for reactive hosts. (ui/src/lit/subscriptions-controller.ts:14, 7b23de34db10)
  • Shared Lit base: The new base classes centralize locale invalidation and the app's light-DOM render-root policy. (ui/src/lit/openclaw-element.ts:4, 7b23de34db10)
  • Chat lifecycle refactor: The chat controller now owns render-lifecycle scopes, after-commit cancellation, composer persistence, scroll cancellation, and disconnect teardown. (ui/src/pages/chat/chat-state.ts:1254, 7b23de34db10)

Likely related people:

  • steipete: GitHub history shows recent merged Control UI split-view, sidebar, settings, and chat work across the same files this PR refactors, in addition to authorship of the current branch. (role: recent feature owner and likely follow-up owner; confidence: high; commits: 9269a27f18c3, 5afd4707c77e, aa1929ccb0e3; files: ui/src/app/app-host.ts, ui/src/pages/chat/chat-page.ts, ui/src/pages/chat/chat-pane.ts)
  • shakkernerd: Recent current-main commit history for ui/src/app/app-host.ts includes terminal availability and Control UI shell fixes in the same app-host area. (role: recent adjacent contributor; confidence: medium; commits: 43bcd9ab3eee, dc6550d6e5ef; files: ui/src/app/app-host.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete-oai
steipete-oai force-pushed the codex/ui-lit-lifecycle-seams branch from 80f79c9 to e448b1a Compare July 9, 2026 11:45
@steipete-oai steipete-oai self-assigned this Jul 9, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 9, 2026
@steipete-oai

steipete-oai commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Land-ready code/test proof for 7b23de34db10111380d00dffe7ca853f668dc566 against #102423:

  • Modernized the Control UI around shared Lit light-DOM/locale bases, source-aware subscriptions, renderer-neutral router ownership, and explicit source/client/connection epochs across async pages, capabilities, chat, terminal, overlays, plugins, and mutations.
  • Exact-head hosted CI run 29016571787 plus Dependency Guard rerun 29016569662: all 55 reported checks green, including lint, build, production/test types, every Linux shard, Windows, dependency consistency, UI i18n, and exact-head dependency authorization.
  • Exact non-isolated failure order reproduced and fixed: gateway-source-replacement.test.ts plus sessions-page.test.ts, 23/23 passed.
  • Final focused UI regression command: 7 files, 87/87 passed.
  • node --import tsx scripts/control-ui-i18n.ts check, targeted Oxlint/Oxfmt, and git diff --check: passed.
  • Fresh structured Codex autoreviews: no accepted/actionable findings. Repository-native review artifacts validate as READY FOR /prepare-pr with zero findings and a passing behavioral sweep.
  • Earlier remote broad proof: UI unit run run_481f339b1294 (2,382 passed), UI build run_387e1957817d, UI browser E2E run_61da9d22cf16 (78 passed), and changed gate run_b626fc76eb92 all passed.
  • Dependency report: 0 added, 0 removed, and 0 version-changed resolved packages. @lit/context was already resolved transitively; this PR makes the production UI import a direct package dependency.

Known proof gap: no single live browser scenario reproduces every intermittent lifecycle race; deterministic tests cover each ownership branch. Local Blacksmith Testbox was unavailable, so the exact-head broad proof is the hosted CI run above.

@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

/allow-dependencies-change Declare the existing production @lit/context import in the Control UI package.

@steipete-oai
steipete-oai merged commit deb7faf into main Jul 9, 2026
105 of 111 checks passed
@steipete-oai
steipete-oai deleted the codex/ui-lit-lifecycle-seams branch July 9, 2026 12:28
@steipete-oai

steipete-oai commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

This was referenced Jul 9, 2026
Closed
Closed
Closed
@github-actions github-actions Bot mentioned this pull request Jul 9, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 10, 2026
@steipete-oai @steipete
refactor(ui): harden Lit lifecycle boundaries (openclaw#102745)
f3233a4 
* refactor(ui): harden Lit lifecycle boundaries

* fix(ui): clear ChatPage subscriptions before teardown

* test(ui): satisfy lifecycle lint gates

* chore: defer Lit lifecycle release note

* fix(ui): sync raw-copy baseline

* test(ui): avoid order-sensitive Sessions mock

---------

Co-authored-by: Peter Steinberger <[email protected]>
@clawsweeper clawsweeper Bot mentioned this pull request Jul 12, 2026
Open
This was referenced Jul 21, 2026
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete-oai steipete-oai

Labels

app: web-ui App: web-ui dependencies-changed PR changes dependency-related files maintainer Maintainer-authored PR merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]: Harden Control UI Lit lifecycle boundaries

2 participants

@steipete-oai @steipete