Skip to content

chore(dmworkbase): remove dead ThreadList component (#282)#478

Draft
lml2468 wants to merge 1 commit into
mainfrom
chore/issue-282-remove-deadcode-threadlist
Draft

chore(dmworkbase): remove dead ThreadList component (#282)#478
lml2468 wants to merge 1 commit into
mainfrom
chore/issue-282-remove-deadcode-threadlist

Conversation

@lml2468

@lml2468 lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

根因 / 实现说明

packages/dmworkbase/src/Components/ThreadList/ is dead code at runtime. Issue #282 was originally filed as a permission bug (over-restrictive delete-button render condition), but a follow-up 5-layer reachability check confirmed the component is unreached by any user path in the current build. This PR deletes the folder per the triage decision (Option A from the issue body).

改动摘要

  • Removed 4 files (~508 lines) under packages/dmworkbase/src/Components/ThreadList/:
    • index.tsx (component)
    • vm.ts (view model)
    • index.css
    • ThreadList.stories.tsx (Storybook placeholder, not the real component)

Dead-code evidence (5 independent layers)

# Check Result
1 Direct import ThreadList outside the component folder 0 occurrences
2 @octo/base re-export (packages/dmworkbase/src/index.tsx) Not exported
3 apps/web/src/** references 0 occurrences
4 apps/extension/src/** references 0 occurrences
5 Storybook entry renders placeholder stub, not the real component

Name collisions verified unrelated to the React component:

  • ThreadListStatus — type alias "active" | "archived" | "all" in Service/Thread.ts
  • onThreadListError — callback option in dmworktodo/utils/buildLinkableChannels.ts
  • isThreadListVisible — local boolean in Pages/Chat/index.tsx (controls ThreadPanel toggle)

The real per-thread UI lives in packages/dmworkbase/src/Components/ThreadPanel/index.tsx (renderListViewrenderThreadItem); deletion happens via a deeper menu (tracked separately in #280, where the gate is over-permissive — opposite direction).

测试

  • 无新增测试。删除的是 0-import 死代码,无运行时调用路径。
  • 依赖 CI typecheck/build 验证 monorepo 其它包不受影响。

验证 (baseline diff)

  • 本地未跑完整 pnpm build(chore tick budget 限制);由 CI 验证 typecheck + build.
  • after − baseline = 0 new failures expected, given the 5-layer reachability proof above.

Fixes #282

`packages/dmworkbase/src/Components/ThreadList/` is unreached at runtime:

- 0 direct imports outside the component folder
- not re-exported from `packages/dmworkbase/src/index.tsx` (so external
  packages cannot resolve `ThreadList` via `@octo/base`)
- 0 references in `apps/web/src/**` or `apps/extension/src/**`
- Storybook entry `ThreadList.stories.tsx` renders a placeholder, not
  the real component
- name collisions in repo (`ThreadListStatus` type, `onThreadListError`
  callback, `isThreadListVisible` local var) are unrelated to the React
  component

The real per-thread UI lives in `Components/ThreadPanel/index.tsx`
(`renderListView` → `renderThreadItem`); deletion happens via a deeper
menu (tracked separately in #280, opposite direction — over-permissive
there).

Removes ~508 lines (index.tsx, vm.ts, index.css, ThreadList.stories.tsx)
with no behavior change.

Fixes #282

@OctoBoooot OctoBoooot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: chore(dmworkbase): remove dead ThreadList component (#282) (#478)

Verdict: Approve

Clean dead-code deletion (−508, 4 files: the Components/ThreadList/ folder — component, vm, css, stories). The PR body's 5-layer reachability proof holds up under independent verification.

Independently verified the zero-importer claim (the load-bearing assertion for any "delete dead code" PR):

  • No file outside Components/ThreadList/ imports the component or references Components/ThreadList.
  • Not re-exported from @octo/base (packages/dmworkbase/src/index.tsx — confirmed no ThreadList export).
  • The repo-wide ThreadList grep returns hits, but I checked each non-folder match and they're all the substring collisions the body correctly enumerated — none is a component import:
    • channelDataSource.threadList(groupNo, ...) — datasource method, unrelated
    • base.threadList.* — i18n keys for ThreadPanel
    • ThreadListStatus — type alias in Service/Thread.ts ("active"|"archived"|"all")
    • isThreadListVisible — local boolean in Pages/Chat/index.tsx (ThreadPanel toggle)
    • onThreadListError / threadList: — callback + property in dmworktodo/buildLinkableChannels.ts
  • The real per-thread UI lives in Components/ThreadPanel/ (untouched), so deleting this folder doesn't remove any reachable feature. CI green.

The triage history is sound: #282 was filed as a delete-button permission bug, but the follow-up reachability check found the whole component unreached, so deleting it (Option A) is the correct resolution rather than fixing a render condition no user can hit.

Praise

  • The 5-layer reachability table (direct import / @octo/base re-export / apps/web / apps/extension / Storybook) plus the explicit name-collision disambiguation (ThreadListStatus, onThreadListError, isThreadListVisible all called out as unrelated) is exactly the evidence a dead-code deletion needs. It turned my verification from "hunt for any importer" into "confirm the enumerated collisions are real" — and every one checked out. This is how to make a deletion reviewable: prove the negative, and pre-empt the false-positive grep hits the reviewer will otherwise chase.
  • Distinguishing this (component fully unreached → delete) from #280 (the real ThreadPanel delete-gate, which is over-permissive — opposite direction) in the body keeps the two threads from being conflated. Precise scoping.

Out of scope (informational)

  • Draft PR; Fixes #282. Pure deletion with no co-located test removed beyond the stories stub — appropriate, since there's nothing left to test. CI typecheck/build is the right safety net here (a stale type reference would fail the build), and it's green.

@Jerry-Xin Jerry-Xin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is in scope for Mininglamp-OSS/octo-web and cleanly removes an unreachable dmworkbase component without leaving component imports, exports, or Storybook references behind.

🔴 Blocking: none.

💬 Non-blocking:

  • 🔵 Suggestion: consider removing now-unused translation keys that were only consumed by the deleted component, such as threadList.createdAt, threadList.createFirst, threadList.deleteConfirm, threadList.join, and threadList.leave in packages/dmworkbase/src/i18n/locales/en-US.json:707 and packages/dmworkbase/src/i18n/locales/zh-CN.json:707. Keep the follow/unfollow keys, because ThreadPanel still uses them at packages/dmworkbase/src/Components/ThreadPanel/index.tsx:1475 and packages/dmworkbase/src/Components/ThreadPanel/index.tsx:1750.

✅ Highlights:

  • Verified no remaining ThreadList, ThreadListVM, Components/ThreadList, Thread/ThreadList, or deleted CSS class references in packages or apps.
  • Confirmed @octo/base does not export the deleted component.
  • The remaining threadList API/type references are unrelated runtime data-source usage, not references to the deleted React component.

Verification caveat: I attempted pnpm --filter @octo/web build, but it could not run because node_modules is missing and vite is not installed in this checkout.

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).


@yujiawei yujiawei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #478 (octo-web)

Scope: delete-only dead-code removal of packages/dmworkbase/src/Components/ThreadList/ (4 files, 508 deletions, 0 additions). Reviewed against head 922e0f0a with merge-base 65eb9375.

1. Spec compliance

The diff does exactly what the PR describes and nothing more:

  • No under-build: the triage decision (Option A) was to delete the unreachable folder; all four files (index.tsx, vm.ts, index.css, ThreadList.stories.tsx) are removed.
  • No over-build: the PR touches only the ThreadList/ folder — no unrelated files, no new flags, no behavioral changes.
  • No deviation: pure deletion, matching the stated approach.

Spec: ✅

2. Code quality / safety

The only real risk in a delete-only PR is a surviving live reference. I independently re-verified the reachability claim rather than trusting the description, across three reference classes:

  1. Static imports — repo-wide search (excluding the deleted folder and node_modules) for the component class ThreadList and ThreadListVM: 0 matches. Sibling components (ThreadCreate, ThreadPanel, ThreadIndicator) carry no import of it.
  2. Dynamic / string refs — no React.lazy, dynamic import(), require(), route/registry maps, Storybook globs, jest/tsconfig/vite path entries referencing the component. A path-literal grep for Components/ThreadList outside the folder returns nothing.
  3. Public API / barrel exportspackages/dmworkbase/src/index.tsx (the @octo/base public surface) uses an explicit named-export list and never exported ThreadList; no export * barrel transitively re-exports it. Downstream consumers of @octo/base import only ThreadListStatus (an unrelated "active" | "archived" | "all" type alias), never the component.

The remaining ThreadList* hits in the tree are confirmed unrelated naming and not affected by this deletion:

  • ThreadListStatus — type alias in Service/Thread.ts:41
  • onThreadListError — callback option in dmworktodo/utils/buildLinkableChannels.ts:42
  • isThreadListVisible — local boolean in Pages/Chat/index.tsx:1175 (controls the ThreadPanel toggle)

The live per-thread UI continues to live in Components/ThreadPanel/, which is untouched. I also checked open sibling PRs; none re-introduces an import of the deleted component.

Coverage note / residual risk: all evidence is static analysis. The only theoretical gap is runtime reflection by the literal string "ThreadList" (e.g. a registry keyed by string) — no such pattern exists anywhere in the repo, so confidence is high. CI typecheck/build is the appropriate final gate and is expected to stay green.

Quality: Approved

3. Overall verdict

APPROVED — Spec ✅ and Quality Approved. Clean, well-evidenced dead-code removal with no live references in any reference class.

4. Suggestions (non-blocking)

  • None required. Optionally, the ThreadList.stories.tsx was a placeholder stub kept only to satisfy a coverage check — its removal here is correct and removes a misleading "component exists" signal in Storybook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL PR size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🧹 [Web] ThreadList component is dead code — originally reported delete-gate bug has no runtime impact

5 participants