Skip to content

refactor(ui): reuse shared requirement records - #140407

Merged
steipete merged 1 commit into
mainfrom
steipete/deletion-ui-requirement-types
Sep 6, 2026
Merged

refactor(ui): reuse shared requirement records#140407
steipete merged 1 commit into
mainfrom
steipete/deletion-ui-requirement-types

Conversation

@steipete

@steipete steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Related: #135868

What Problem This Solves

The Skills UI repeats requirement and configuration-check records already owned by the shared requirements module. Those copies create avoidable maintenance debt in the maintainer-directed deletion campaign.

Why This Change Was Made

Use the existing Requirements and RequirementConfigCheck types for the UI's required, missing, and configuration-check fields. Keep the broader UI status model because its field requirements intentionally differ from the server model.

User Impact

No behavior or appearance change. Production delta is −16 (+4/−20), with zero test/tooling/docs change. Every existing UI status field retains its type and optionality.

Evidence

  • Compiler comparison of the complete SkillStatusEntry and SkillStatusReport shapes against real shared types: identical, including nested records and arrays.
  • Full-file emitted JavaScript is byte-identical; the new import is type-only.
  • node scripts/run-vitest.mjs --config ui/vitest.config.ts ui/src/lib/skills ui/src/pages/skills: 3 files, 69 tests passed.
  • Full node scripts/check-changed.mjs: passed, including core-test/UI types, i18n, dead exports, and lint.
  • Independent Codex P0–P2 autoreview: scoped-clean, no accepted/actionable findings.
  • No test removal, new dependency, schema/configuration change, runtime import, lazy boundary, or published API change. No build or visual capture is needed for erased type-only code.

@clawsweeper

clawsweeper Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete force-pushed the steipete/deletion-ui-requirement-types branch from 3ca598f to acfe2ab Compare September 6, 2026 20:03
@steipete
steipete marked this pull request as ready for review September 6, 2026 20:03
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui size: XS maintainer Maintainer-authored PR labels Sep 6, 2026
@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 4:09 PM ET / 20:09 UTC.

ClawSweeper review

What this changes

The Skills UI reuses shared requirement and configuration-check types while preserving its existing status fields and optionality.

Merge readiness

✅ Ready for maintainer review

Useful, narrowly scoped cleanup that remains absent from the inspected main revision. No actionable correctness findings; maintainer-authored, protected work should remain open.

Priority: P3
Reviewed head: acfe2ab31c0f66c21989ee72a76e234b00c25288

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Focused, structurally equivalent type reuse with appropriate reported validation and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: This maintainer-authored PR changes only UI type declarations; the shared shapes match, and no runtime or visible behavior requires demonstration.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This maintainer-authored PR changes only UI type declarations; the shared shapes match, and no runtime or visible behavior requires demonstration.
Evidence reviewed 6 items Introduced change preserves record shapes: The pinned introduction replaces only duplicate type declarations with a type-only import. Requirements retains five required string-array fields; RequirementConfigCheck retains path:string and satisfied:boolean.
Canonical requirement records: The shared definitions exactly match the removed UI records. The server status model already uses these types, while its broader required fields differ from the UI model, supporting the PR's limited reuse.
Consumer boundary: loadSkillStatusReport requests skills.status with SkillStatusReport; the Skills page reads missing.bins and missing.anyBins. The patch changes neither the request nor rendering behavior.
Findings None None.
Security None None.

How this fits together

The Gateway evaluates skill prerequisites and sends status records to the Control UI. The UI uses these records to display missing requirements and available installation actions.

flowchart LR
  A[Skill metadata] --> B[Requirement evaluation]
  B --> C[Gateway skill status]
  C --> D[UI status records]
  E[Shared requirement types] -.-> B
  E -.-> D
  D --> F[Skills page]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source -16. Total -16 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 4 20 -16
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 4 20 -16

Review metrics

None.

Technical review

Best possible solution:

Keep requirement records canonical in the shared module while retaining the UI-specific status model.

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

Not applicable: this is type-only cleanup, and direct source comparison confirms the existing duplicate records.

Is this the best way to solve the issue?

Yes: reusing the exact shared records removes duplication without importing runtime code or imposing the server's broader status requirements on the UI.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 9474ff67da1f.

Labels

Label changes:

  • add P3: This removes duplicate type declarations without changing runtime behavior or user-facing contracts.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This maintainer-authored PR changes only UI type declarations; the shared shapes match, and no runtime or visible behavior requires demonstration.

Label justifications:

  • P3: This removes duplicate type declarations without changing runtime behavior or user-facing contracts.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This maintainer-authored PR changes only UI type declarations; the shared shapes match, and no runtime or visible behavior requires demonstration.

Evidence

What I checked:

  • Introduced change preserves record shapes: The pinned introduction replaces only duplicate type declarations with a type-only import. Requirements retains five required string-array fields; RequirementConfigCheck retains path:string and satisfied:boolean. (ui/src/api/types.ts:512, acfe2ab31c0f)
  • Canonical requirement records: The shared definitions exactly match the removed UI records. The server status model already uses these types, while its broader required fields differ from the UI model, supporting the PR's limited reuse. (src/shared/requirements.ts:2, acfe2ab31c0f)
  • Consumer boundary: loadSkillStatusReport requests skills.status with SkillStatusReport; the Skills page reads missing.bins and missing.anyBins. The patch changes neither the request nor rendering behavior. (ui/src/lib/skills/index.ts:193, acfe2ab31c0f)
  • Still useful on inspected main: Fetched main retains SkillsStatusConfigCheck and both inline requirement records. Raw test-merge parents match pinned main followed by PR head; its comparison changes only ui/src/api/types.ts, preserving unrelated base work. Release-file inspection could not complete because the required object was unavailable and network resolution failed; no shipped-status claim is made. (ui/src/api/types.ts:454, 9474ff67da1f)
  • Related work and supplied validation: The captured PR body describes maintainer-directed cleanup associated with feat(triage): start installation-owned recovery after update and startup failures #135868, whose recovery implementation is distinct. It reports equivalent compiler shapes, identical emitted JavaScript, 69 passing focused tests, and passing changed-file checks. These are contributor-reported results, not reviewer execution. (acfe2ab31c0f)
  • Area history and inspection limits: Main history includes Peter Steinberger's adjacent UI type-sharing commit and earlier shared-requirement refactoring. Deeper blame and string-history inspection failed on unavailable objects, so no introducing-author claim is made. Live GitHub refresh also failed; the supplied captured context supports this review. (ui/src/api/types.ts, 7115aca8246f)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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
steipete merged commit 4401701 into main Sep 6, 2026
178 checks passed
@steipete
steipete deleted the steipete/deletion-ui-requirement-types branch September 6, 2026 20:14
@steipete

steipete commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

vincentkoc added a commit that referenced this pull request Sep 6, 2026
@vincentkoc
Merge remote-tracking branch 'origin/main' into docs-audit/split-cont…
9fd61f8 
…rol-ui

* origin/main:
  docs: remove the empty release-sections nav group (#140427)
  fix(tooling): omit Codex machine squash credit (#140420)
  chore(ui): refresh control ui locales (#140384)
  refactor(plugin-sdk): reuse native approval input contracts (#140415)
  fix: silent model WebSocket streams hang past the idle timeout (#140335)
  perf(ai): avoid unused text extraction for image results (#140409)
  fix(ui): distinguish failed GitHub status checks from disconnected accounts (#140333)
  refactor(cron): share runtime feedback schema fields (#140408)
  fix: avoid duplicate cloud worker teardown during recovery (#140377)
  fix(cli): show executable approval scope (#138386)
  refactor(ui): reuse shared requirement records (#140407)
  test(gateway): table-drive device token lifecycle checks (#140403)
  fix(chat): fence delayed history at terminal retirement (#140379)
  test(skills): compare large upload bytes natively (#140402)
  refactor(delivery): reuse the canonical rendered batch plan type (#140386)
  docs(start): repair the first-run path from landing page to first channel (#140390)

# Conflicts:
#	docs/.i18n/glossary.zh-CN.json
@github-actions github-actions Bot mentioned this pull request Sep 6, 2026
Open
@steipete steipete mentioned this pull request Sep 7, 2026
Merged
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 7, 2026
@steipete
refactor(ui): reuse shared requirement records (openclaw#140407)
b00aa3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui maintainer Maintainer-authored PR P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete