Skip to content

chore: remove dead code and sync the specs that described it - #8804

Merged
PolinaGurinovich97 merged 8 commits into
developmentfrom
chore/remove-dead-code
Sep 15, 2026
Merged

PolinaGurinovich97 merged 8 commits into
developmentfrom
chore/remove-dead-code

Conversation

@PolinaGurinovich97

Copy link
Copy Markdown
Collaborator

Description of changes

Knip (production + comprehensive modes) over apps/* and libs/* surfaced code with no callers. This removes what verification confirmed dead and updates the OpenSpec specs that still described it. No behaviour change — nothing removed here had a caller.

Removed from apps/chat

  • FileTypeIcon, collect-stream, server-api/models.ts (and the modelsApi singleton it was the only consumer of), rate.api.ts and its spec
  • OperationLoaderModal and UploadProgressModal with their specs — stale copies of components that now live in libs/chat-shared/src/file-manager. The app copies had already drifted behind (missing mobile:!w-full), so the risk was someone fixing the dead copy. Their tests duplicated the lib's own line for line, so no coverage is lost.

Removed from apps/chat-api

  • list-items-debug, PinConversationDto (absent from the OpenAPI spec — the pin flow goes through conversation-lifecycle.service.ts), and the ManagedConfigProvider placeholder, which threw when resolved and was never registered. The composite provider's comment now reads "(future)" instead of pointing at a stub file that no longer exists.

Kept deliberately

useThemeOptions has zero callers but stays: openspec/changes/add-settings-preferences-tab requires it to survive as the parked theme row's entry point, and docs/architecture.md documents it.

Spec updates in the same change

  • model-listing, model-lookup — dropped the frontend server-api helper requirements. The endpoints themselves stay and remain fully specified; a note in each Purpose records that apps/chat reads model metadata through deploymentsApi instead, so nobody removes a live endpoint as unused or restores the helper as missing.
  • generated-api-client-integration — dropped the models domain requirement and modelsApi from the required api-client.ts exports
  • applications-listing, unified-catalogmodelsApi replaced as the "follow this pattern" example
  • chat-hooks-domain-utilitiescollect-stream.ts dropped from the list of files that must not be modified

Applicable issues

No linked issue — this came out of a dead-code audit. Happy to add one if you'd like it tracked.

UI changes

None.

Verification

  • nx typecheck lint -p chat chat-api — chat green on both; chat-api lint green
  • nx test chat-api — 189 files, 3194 tests, all pass
  • nx test chat — 157/158 files pass; the one failure (SkillEditorPreview "unrecognized text extension") is a pre-existing flake that times out only under the parallel run and passes in isolation, with no import path to anything removed here
  • npm run validate:docs — passes
  • chat-api:typecheck reports 767 errors, identical on a clean origin/development checkout — a pre-existing tsconfig.spec.json / project-reference condition unrelated to this change
  • Knip re-run after the deletions: production-mode dead files 38 → 31, with no new findings introduced by the removals

Checklist

  • I confirm that I don't share any confidential information like API keys or any other secrets and private URLs

🤖 Generated with Claude Code

Knip (production and comprehensive modes) over apps/* and libs/* surfaced
code with no callers. Removed what verification confirmed dead, and updated
the OpenSpec specs that still described it.

Removed from apps/chat:
- FileTypeIcon, collect-stream, server-api/models.ts (and the modelsApi
  singleton it was the only consumer of), rate.api.ts and its spec
- OperationLoaderModal and UploadProgressModal with their specs: stale copies
  of components that now live in libs/chat-shared/src/file-manager. The app
  copies had already drifted behind (missing mobile:!w-full) while their tests
  duplicated the lib's own, so no coverage is lost.

Removed from apps/chat-api:
- list-items-debug, PinConversationDto (absent from the OpenAPI spec), and the
  ManagedConfigProvider placeholder, which threw when resolved and was never
  registered. The composite provider's comment now says "(future)" rather than
  pointing at a stub file.

Specs updated in the same change:
- model-listing, model-lookup: dropped the frontend server-api helper
  requirements. The endpoints stay and remain specified; a note records that
  apps/chat reads model metadata through deploymentsApi instead.
- generated-api-client-integration: dropped the models domain requirement and
  modelsApi from the required api-client exports
- applications-listing, unified-catalog: modelsApi replaced as the example
- chat-hooks-domain-utilities: collect-stream.ts dropped from the list of
  files the change must not modify

useThemeOptions is deliberately kept with no callers:
openspec/changes/add-settings-preferences-tab requires it to survive as the
parked theme row's entry point.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

security-review: Dead-code removal PR. No new attack surface introduced; several security-relevant patterns improved or removed cleanly.

Run details

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown

⚠️ code-review: Strong, well-specified backend change (#8784: bearer-auth chat generation) with thorough adversarial tests, complete OpenSpec artifacts, and clean doc/spec sync. Non-blocking nits: a spec-version lockstep gap in header-token.strategy.ts, and the toolset endpoint URL contract narrowed (sse:// now rejected) as a silent breaking change.

Severity Location Message
warning libs/toolset-editor/src/utils/toolsets.ts Public-contract narrowing shipped without a docs/architecture.md Compatibility note: isValidEndpointUrl (toolset-editor) and ToolsetBodyDto (chat-api) both stop accepting sse:// endpoints, so any toolset previously saved with an sse:// endpoint is now invalid. The change is correct and deliberate — DIAL Core rejects a stored sse scheme with 'invalid URI scheme sse', and the SSE transport is chosen via the transport field over an http(s) endpoint — and the README, .env-adjacent prose, JSDoc defaults, en.json messages, and both specs were updated in the same diff. Only the Compatibility note for the previously documented contract is missing. Non-blocking; a single note in docs/architecture.md and a mention in the PR body would complete it.
nit apps/chat-api/src/auth/strategies/header-token.strategy.ts:120 Same reasoning, two spellings: the new sub rejection and the pre-existing iss rejection both hardcode their own message strings ('Token is missing a "sub" claim' and 'Token is missing an "iss" claim'), while every other rejected-message in this file is already shared through a small set of helper/const patterns. Harmless duplication; extracting a shared missingClaim(claim) helper would keep the invariant 'no message may drift' as a single place. Pure readability nit, not blocking.
fyi apps/chat-api/src/auth/strategies/header-token.strategy.ts:119 Order-of-checks speed-up is real but produces a user-visible error-message change: a token with a missing sub now returns AUTH_HEADER_TOKEN_INVALID with message 'Token is missing a "sub" claim' even when it may be expired or deliberately issued for a refresh flow (which legitimately omits sub per OIDC spec). This is intended and documented ('reject before resolving the bucket, so an unusable token costs no DIAL Core round trip'), and the spec's check-order list was updated to include it. Only note: an OIDC refresh token, which may never carry sub, hitting the BFF would now see a sub-specific message rather than an expiry one — which of the two the caller sees first is a diagnostic nuance, not a bug.

Run details

PolinaGurinovich97 and others added 7 commits September 15, 2026 00:13
Second pass over the same audit. Each removal was verified by hand rather than
taken from the tool: the server-api wrappers each showed exactly two mentions
in the tree — their own declaration and a call to the identically named method
on the generated client, which is a different symbol.

apps/chat/src/server-api:
- del (base.ts), getConversationMetadata and deleteConversations
  (conversations.api.ts), createShareLink (share.api.ts), getUserLimits
  (user-limits.ts), plus the type imports left unused by those removals.
  Sibling wrappers in the same files stay; only the uncalled ones go.

apps/chat/src/i18n/config.ts:
- export default i18n. main.tsx imports the module for its side effect only,
  so the default binding had no reader.

apps/chat-api:
- ConfigValueType, which nothing referenced.

libs/quotations, libs/conversation-input:
- the duplicate `export default memo(X)` on CitationCard, CitationMarker,
  CitationDropdown and ChatSettingsModal, with the now-unused memo import.
  Each barrel re-exports the named binding only, so the default was
  unreachable from the package entry and no consumer can be relying on it.

SaveSuccessMessage is left in place despite having no code reference: it is the
only written record of the QuickApps iframe's SaveSuccess payload contract, and
three JSDoc blocks point at it. Typing the parse site with it would be a better
fix than deleting it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`FeedbackCategory` had no reference anywhere in the tree — only its own
declaration. Callers work with `FEEDBACK_CATEGORIES` directly, which stays.

Found by re-running the dead-code classification so that member-access
occurrences (`someApi.name`) no longer count as references to a same-named
local symbol; the earlier pass read the `FEEDBACK_CATEGORIES` mention on the
declaration line as a use and hid this one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
)

Two drift classes that nothing checked, both of which had already reached
the main line:

- One external declared at several ranges. `@epam/ai-dial-ui-kit` sat at
  `^0.14.0-dev.15`, `-dev.30` and `-dev.37` at once and `react` at
  `^19.0.0`, `^19.2.6` and `^19.2.7` — each lib pinning whatever the
  dependency was when it was scaffolded, not what it needs. A host installs
  one copy regardless, so the spread either agrees by luck or sends the host
  back to a `resolutions` pin.
- A README citing a range its manifest no longer declares. That number is
  what a host copies, and `chat-hooks` advertised
  `@epam/pdf-highlighter-kit ^0.0.18` and `react-file-manager ^0.2.0-dev.10`
  after both manifests had moved on.

Siblings under `libs/` stay exempt from the first check for the same reason
they are exempt from the upper-bound one: their spec is a placeholder
`publish-lib.mjs` rewrites to the release version.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it (Issue #8719)

`settings-panel` and `usage-dashboard` were scaffolded without the
`@nx/dependency-checks` block every other lib carries, so nothing verified
that what they import is what they declare — the mechanism behind the
issue's first acceptance criterion.

Enabling it immediately flagged `settings-panel`'s `@epam/ai-dial-ui-kit`
peer as unused. The peer is real: the panel applies the kit's `dial-h1-text`
/ `dial-small-text` typography classes, which arrive through its stylesheet
rather than an import the rule can see. Recorded that as an ignore with the
reason instead of dropping a peer a host genuinely needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither failure could happen on the Linux CI runners, so both sat unnoticed
while the packaging guards they block are exactly what an embedding host
depends on.

- `assertCoherentTarballs` handed `tar` an absolute path. GNU tar reads the
  drive letter as a remote host ("Cannot connect to C: resolve failed").
  Naming the archive relative to its own directory works on GNU and bsdtar
  alike; `--force-local` would not, macOS having no such flag.
- npm resolves a `file:` dependency spec without percent-decoding it, while
  `pathToFileURL` encodes `~` as `%7E`. On Windows `os.tmpdir()` is an 8.3
  short path (`C:\Users\PALINA~1\...`) once the profile name is long enough,
  so every fixture installed nothing and died with ENOENT on a path
  containing `%7E`. Only `realpathSync.native` expands a short name — the JS
  variant resolves symlinks and junctions but leaves `PALINA~1` alone — so
  the temp root now comes from one `createTmpRoot` helper that all seven
  fixture entry points share.

With both fixed, `publish:lib:coherent-release-test` passes locally: 2/2.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@PolinaGurinovich97
PolinaGurinovich97 merged commit 6fa908d into development Sep 15, 2026
19 of 20 checks passed
@PolinaGurinovich97
PolinaGurinovich97 deleted the chore/remove-dead-code branch September 15, 2026 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant