Remove dead code identified by reachability analysis by dpage · Pull Request #395 · pgEdge/ai-dba-workbench

dpage · 2026-08-07T13:57:19Z

What this is

Following on from the Codacy review, I looked at what the tree contains
that nothing calls. Running deadcode from each service entry point
found 94 functions unreachable from the collector, server and
alerter binaries.

Cross-checking against the v1.0.0 tag settled whether any of it was
work-in-progress: every one of the 94 was already unreachable at that
release two months ago, so none is recent code awaiting a caller. Worth
noting the converse, though: four functions that were dead at v1.0.0
have since been adopted (EstimateTokens, EstimateTotalTokens,
GetBriefDescription, HasToolResults), which is a decent argument for
removing on evidence rather than case-by-case optimism.

This PR takes the mechanically safe subset. Two follow-ups are noted at
the bottom.

Scope

	Lines
Production Go removed	698 (47 functions)
Test Go removed	2,110
Client	117
Total	2,925 deletions, 22 insertions (the insertions are the changelog)

Two removals went beyond single functions:

The alerter's secretManager had no callers, and with it gone the
SecretManager interface in interfaces.go had no implementation and
no users either, so both are deleted.
The resource Registry type, its constructor and its methods were
used only by their own tests. Resource and Handler, declared in
the same file, stay: the MCP resource definitions in
pg_system_info.go use them.

On the client, the unused HeaderStatusIndicator component goes, along
with @dnd-kit/sortable and @dnd-kit/utilities. Only @dnd-kit/core
is imported and it remains.

How the deletions were validated

Each function was deleted and verified individually, and kept only
where the resulting breakage was confined to test functions whose own
names target the deleted symbol. Anything else was reverted
automatically.

That guard is the most important thing in this PR. A first pass without
it deleted 262 test functions and 11,382 lines, including entire
integration suites such as the auth handler login, rate-limiting and
secure-cookie tests, and the alert Unacknowledge tests. Those cover
live code and broke only because they call a dead method during cleanup
or use a private setter as an injection seam. I had allowed a test to be
removed when its name mentioned the receiver type, which is far too
loose; the rule now requires the function name itself.

Coverage

Essentially unmoved, because the deleted code and its deleted tests
cancel out:

Module	`main`	This branch
collector	85.5%	85.5%
server	55.5%	55.3%
alerter	65.9%	65.6%

No new code is added, so there is nothing new to cover and the 90%
floor for new and modified code does not bite. The server sitting below
the 60% repository goal is pre-existing rather than introduced here,
and closing that 4.7-point gap is a substantial separate piece of work
rather than something to bolt onto a deletion.

Verification

gofmt, golangci-lint and go vet clean on all three modules.
golangci-lint caught one genuine follow-on, an orphaned reWord
regexp left behind by deleting CountWords, now removed.
Full suites pass on collector and alerter. The server's only failures
are the two pre-existing vector(3) fixture failures from Gemini provider: (1) knowledge base search silently falls back to OpenAI due to missing gemini_embedding column in search_knowledgebase.go; (2) session startup fails with 400 "empty Part" error #337,
which reproduce identically on unmodified main.
Client: lint clean, production build succeeds, 3,488 tests across 171
files pass.

One note for anyone re-running this locally: the suites must be run with
-p=1, as the sub-project Makefiles do. I first ran them without it and
got a spray of deadlock detected and relation already exists
failures, because the integration tests all build the same schema in one
shared database. Incidentally collector/Makefile does not set
-p=1 where server/Makefile does, which looks like an oversight worth
a separate look.

Deliberately left for follow-up

42 further dead functions whose tests need editing rather than
deleting. These cluster usefully: the ten notification-channel CRUD
methods in the alerter all share one
TestNotificationQueriesReturnErrorOnClosedPool, the internal/logging
façade shares TestLogLevelFiltering, and several are test seams like
AuthHandler.Close and AlertHandler.setAlertResolver.
The two superseded handler constructors, NewConnectionHandler
and NewNotificationChannelHandler. cmd/mcp-server calls the
...WithSecurity variants; the test suites still use the old ones, so
32 tests need repointing at the live constructors first.
Three test helpers in non-test files (NewTestClient,
NewTestDatastoreWithSecret, NewTestDatastore). They compile into
the shipped binaries but are used across package boundaries by tests,
so they cannot simply become _test.go files; the right fix is
extracting them into a test-only package, which is import churn rather
than deletion.

Summary by CodeRabbit

Removed
- Removed numerous unused internal APIs and their dedicated tests across alerting, collection, server, search, tracing, and resource management.
- Removed unused notification secret encryption utilities and configuration-saving helpers.
- Removed obsolete status-panel UI and drag-and-drop runtime dependencies.
- Removed legacy access, logging, validation, caching, analytics, and formatting helpers.
Documentation
- Updated the unreleased changelog to summarize removed functionality.

Codacy's report prompted a look at what the tree contains that nothing calls. Running `deadcode` from each service entry point found 94 functions unreachable from the collector, server and alerter binaries. Comparing that set against the v1.0.0 tag showed every one of them was already unreachable at that release two months ago, so none is recent work waiting for a caller. Four functions that were dead at v1.0.0 have since been adopted, which is a fair argument for removing on evidence rather than case-by-case optimism. This change takes the safe subset of that set: 47 functions, 698 lines of production code, and the 2,110 lines of tests that existed only to exercise them. Each deletion was applied and verified individually, and kept only where the resulting breakage was confined to tests whose own names target the deleted function; anything else was reverted automatically and is left for follow-up. That guard matters, because a first pass without it removed entire integration suites, such as the auth handler login and rate-limiting tests, which cover live code and merely call a dead method during cleanup. Two removals went further than single functions. The alerter's `secretManager` had no callers and no remaining implementation of the `SecretManager` interface, so both go. The resource `Registry` type, its constructor and its methods were used only by their own tests; the `Resource` and `Handler` types declared in the same file stay, because the MCP resource definitions use them. On the client, the unused `HeaderStatusIndicator` component goes along with the `@dnd-kit/sortable` and `@dnd-kit/utilities` packages. Only `@dnd-kit/core` is imported, and it remains. Coverage is essentially unmoved, because the deleted code and the deleted tests cancel out: collector holds at 85.5%, server goes from 55.5% to 55.3%, and alerter from 65.9% to 65.6%. No new code is added, so there is nothing new to cover. Left for follow-up, deliberately: 42 further dead functions whose tests need editing rather than deleting, the two superseded handler constructors whose test suites must be repointed at the `...WithSecurity` variants, and three test helpers in non-test files that ship in the binaries and want extracting into a test-only package.

coderabbitai · 2026-08-07T13:57:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a7580bbc-a987-4132-bd8a-6e3671f30182

📥 Commits

Reviewing files that changed from the base of the PR and between 19c645d and d06aaf0.

⛔ Files ignored due to path filters (1)

client/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (51)

alerter/src/internal/cron/cron.go
alerter/src/internal/cron/cron_test.go
alerter/src/internal/database/metric_queries.go
alerter/src/internal/notifications/interfaces.go
alerter/src/internal/notifications/secrets.go
alerter/src/internal/notifications/secrets_test.go
client/package.json
client/src/components/StatusPanel/HeaderStatusIndicator.tsx
collector/src/database/monitored_pool.go
collector/src/database/monitored_pool_test.go
collector/src/database/schema.go
collector/src/database/schema_test.go
collector/src/probes/base.go
collector/src/probes/base_test.go
docs/changelog.md
server/src/internal/api/auth_handlers.go
server/src/internal/api/request_helpers.go
server/src/internal/api/request_helpers_test.go
server/src/internal/auth/access.go
server/src/internal/auth/access_test.go
server/src/internal/auth/middleware.go
server/src/internal/auth/middleware_test.go
server/src/internal/compactor/analytics.go
server/src/internal/compactor/analytics_test.go
server/src/internal/compactor/cache.go
server/src/internal/compactor/cache_test.go
server/src/internal/compactor/token_estimator.go
server/src/internal/compactor/token_estimator_test.go
server/src/internal/config/config.go
server/src/internal/config/config_test.go
server/src/internal/config/reload.go
server/src/internal/database/logger.go
server/src/internal/database/logger_test.go
server/src/internal/database/parser.go
server/src/internal/database/parser_test.go
server/src/internal/database/queries_test.go
server/src/internal/database/timeline_queries.go
server/src/internal/metrics/query.go
server/src/internal/metrics/query_test.go
server/src/internal/resources/registry.go
server/src/internal/resources/registry_test.go
server/src/internal/search/chunking.go
server/src/internal/search/chunking_test.go
server/src/internal/search/column_detection.go
server/src/internal/search/column_detection_test.go
server/src/internal/tools/describe_probe.go
server/src/internal/tools/describe_probe_test.go
server/src/internal/tools/validation.go
server/src/internal/tools/validation_test.go
server/src/internal/tracing/tracer.go
server/src/internal/tracing/tracer_test.go

💤 Files with no reviewable changes (50)

server/src/internal/tracing/tracer.go
collector/src/probes/base.go
server/src/internal/tools/describe_probe.go
server/src/internal/search/column_detection.go
server/src/internal/compactor/analytics.go
server/src/internal/compactor/analytics_test.go
server/src/internal/search/chunking.go
server/src/internal/search/column_detection_test.go
server/src/internal/tools/validation.go
server/src/internal/tracing/tracer_test.go
server/src/internal/database/parser.go
server/src/internal/auth/access.go
server/src/internal/tools/describe_probe_test.go
server/src/internal/compactor/token_estimator.go
server/src/internal/auth/middleware.go
server/src/internal/compactor/cache_test.go
collector/src/database/monitored_pool_test.go
collector/src/database/schema_test.go
alerter/src/internal/cron/cron_test.go
client/package.json
server/src/internal/api/request_helpers.go
server/src/internal/config/config.go
alerter/src/internal/database/metric_queries.go
collector/src/probes/base_test.go
server/src/internal/database/timeline_queries.go
server/src/internal/compactor/token_estimator_test.go
server/src/internal/resources/registry_test.go
server/src/internal/metrics/query.go
server/src/internal/auth/middleware_test.go
server/src/internal/search/chunking_test.go
server/src/internal/config/config_test.go
server/src/internal/api/auth_handlers.go
client/src/components/StatusPanel/HeaderStatusIndicator.tsx
server/src/internal/config/reload.go
server/src/internal/database/logger_test.go
alerter/src/internal/notifications/interfaces.go
alerter/src/internal/cron/cron.go
server/src/internal/auth/access_test.go
server/src/internal/database/parser_test.go
server/src/internal/tools/validation_test.go
server/src/internal/database/logger.go
server/src/internal/resources/registry.go
alerter/src/internal/notifications/secrets_test.go
alerter/src/internal/notifications/secrets.go
server/src/internal/api/request_helpers_test.go
server/src/internal/metrics/query_test.go
collector/src/database/monitored_pool.go
server/src/internal/compactor/cache.go
collector/src/database/schema.go
server/src/internal/database/queries_test.go

Walkthrough

This change removes unused APIs, implementations, tests, client dependencies, and the web status indicator across the alerter, collector, server, and client. The changelog records the removals. Existing retained functionality remains unchanged.

Changes

Alerter and client cleanup

Layer / File(s)	Summary
Alerter API and secret cleanup `alerter/src/internal/cron/`, `alerter/src/internal/database/metric_queries.go`, `alerter/src/internal/notifications/`	Removed unused cron and metric wrappers, the `SecretManager` interface, the secret implementation, and related tests.
Client dependency and component cleanup `client/package.json`, `client/src/components/StatusPanel/HeaderStatusIndicator.tsx`	Removed two drag-and-drop dependencies and the unused `HeaderStatusIndicator` component.

Collector cleanup

Layer / File(s)	Summary
Collector database API cleanup `collector/src/database/monitored_pool.go`, `collector/src/database/schema.go`, `collector/src/database/*_test.go`	Removed pool version and connection-limit methods, migration-status retrieval, and their dedicated tests.
Collector probe cleanup `collector/src/probes/base.go`, `collector/src/probes/base_test.go`	Removed feature-cache invalidation and its tests.

Server cleanup

Layer / File(s)	Summary
Authentication and request helper cleanup `server/src/internal/api/`, `server/src/internal/auth/`	Removed proxy-header configuration, query-time and allowed-value helpers, database access checking, IP-context lookup, and dedicated tests.
Compactor and configuration cleanup `server/src/internal/compactor/`, `server/src/internal/config/`	Removed analytics summaries and reset behavior, cache statistics methods, text helpers, configuration saving, and related tests.
Database, search, tools, and tracing cleanup `server/src/internal/database/`, `server/src/internal/search/`, `server/src/internal/tools/`, `server/src/internal/tracing/`	Removed unused logging, parser, duration-formatting, metric-query, search-formatting, validation, identifier-wrapper, session-ID, and related test code.

Documentation

Layer / File(s)	Summary
Unreleased removal record `docs/changelog.md`	Documented the removed unreachable APIs, secret manager, resource registry, status indicator, and client dependencies.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

pgEdge/ai-dba-workbench#267: Modified the LoadSecretKey API that this change removes.
pgEdge/ai-dba-workbench#297: Also removes legacy unused interfaces and implementations.
pgEdge/ai-dba-workbench#390: Modified the GetLatestMetricValue API that this change removes.

Suggested reviewers: susan-pgedge

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: removing dead code identified through reachability analysis.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch remove-dead-code

_{Comment @coderabbitai help to get the list of available commands.}

codacy-production · 2026-08-07T13:59:05Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -171 complexity · -4 duplication

Metric Results

Complexity -171

Duplication -4

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

This was referenced Aug 7, 2026

Repoint handler tests at the WithSecurity constructors #396

Open

Remove dead code that needed test edits rather than deletions #397

Open

Stop the AuthHandler login rate limiter on shutdown #398

Open

Navigation Menu

Uh oh!

Remove dead code identified by reachability analysis - #395

Remove dead code identified by reachability analysis#395
dpage wants to merge 1 commit into
mainfrom
remove-dead-code

dpage commented Aug 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading

Uh oh!

codacy-production Bot commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

dpage commented Aug 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this is

Scope

How the deletions were validated

Coverage

Verification

Deliberately left for follow-up

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

codacy-production Bot commented Aug 7, 2026

Up to standards ✅

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dpage commented Aug 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 7, 2026 •

edited

Loading