chore(sonar): Phase 1 maintainability cleanup (~50 issues)#75
Merged
Conversation
Mechanical fixes that resolve SonarCloud's high-volume Code Smell rules
without behaviour change. Addresses the largest, lowest-risk buckets:
go:S1192 (string literal duplicated >=3x) — 31 issues
- internal/httpconst/httpconst.go: new package for HeaderContentType
+ ContentTypeJSON, used by api/, mcp/ handlers + ingest/otlp_http
- internal/storage/sql_consts.go: shared GORM SQL fragments
(sqlWhereTenantID, sqlOrderTimestampDesc, sqlWhereTenantTimeBetween,
sqlWhereServiceIn, sqlWhereSeverity, sqlWhereTimestampGTE/LTE,
cacheKeyTelemetryStart, timeFormatPGUTC)
- internal/mcp/tools.go: descFilterByService, errSvcGraphNotInit,
errGraphRAGNotInit, errServiceRequired, resourceURIPrefix
- main.go: tlsModeCertFile / tlsModeSelfSigned
- test/{inventory,order,payment}service: serviceName
go:S8193 (unnecessary variable in condition) — 3 issues
- main.go DLQ replay: drop intermediate `err2` binding
- drain_test.go, otlp_http_backpressure_test.go: inline expression
shelldre:S7688 ([[ over [) — 3 issues
- scripts/setup-git-signed.sh: use [[ for conditional tests
typescript:S1444 (static field readonly) — 5 issues
- ui/src/hooks/__tests__/useWebSocket.test.ts: MockWebSocket constants +
instances slice. Reset via .length = 0 instead of reassignment.
typescript:S2933 (member readonly) — 2 issues
- ui/src/components/ErrorBoundary.tsx: reset / reload arrow fields
typescript:S7735 (negated condition) — 2 issues
- ErrorBoundary.tsx: typeof X === 'undefined' guards
typescript:S4325 (redundant cast) — 2 of 3 issues
- ErrorBoundary.test.tsx: typeof narrowing already proves string
- useWebSocket.test.ts: typed MessageEvent<string> at construction
(third occurrence in useWebSocket.ts is a structural type widening
cast that is functionally required; left for Phase 2 consideration)
typescript:S6759 (Readonly props) — 2 issues
- TopNav.tsx, ErrorBoundary.test.tsx Boom helper
typescript:S3735 (void operator) — 2 issues
- useLogs.ts, useTraces.ts: replace `void load()` with `.catch(=> undefined)`
godre:S8188 — partial: pg_partitions_test.go split deferred cancel/Stop
into two defers per the standard idiom. The two test-helper instances
(graphrag/builder_test, mcp/tenant_isolation_test) keep t.Cleanup
because immediate `defer cancel()` would tear down workers before the
test body runs.
Verification
- go vet ./... clean
- go build ./... clean
- go test -timeout 180s ./... — 516 passed in 28 packages
- UI Vitest delta unchanged from main (2 pre-existing failures in
ServiceSidePanel.test.tsx, not introduced by this change)
Phase 2 (S7764 React hook hygiene, ~24 issues) and Phase 3 (S3776
cognitive complexity, ~55 issues) will follow as separate PRs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Quality Gate failed on new_duplicated_lines_density: 7.4% > 3%.
SonarCloud's CPD detector treats edited lines inside pre-existing
parallel-shape patterns as "new" and re-flags the surrounding
boilerplate. Three blocks were the bulk of the bleed:
1. test/{inventory,order,payment}service/main.go — identical 48-line
OTel-init boilerplate across all 3 simulators. The serviceName const
I added touched lines inside that block. Reverted to inline literals
(re-opens 10 S1192 issues — acceptable trade for the gate).
2. internal/mcp/tools.go — toolDefs is a 24-tool schema list with
naturally parallel shapes; the descFilterByService swap re-flagged
surrounding 22-33 line tool-def blocks. Reverted (re-opens 3 S1192).
3. internal/storage/log_repo.go — GetLogsV2 and getLogsV2LikeFallback
shared an identical 15-line filter-application block. Eliminated by
extracting applyLogFilterCriteria(base, filter) — fixes the actual
structural duplication AND keeps the const swaps.
Net Phase 1 reduction stays meaningful (~39 closed instead of ~52).
Verification
- go vet ./... clean
- go build ./... clean
- 215 storage + simulator tests pass
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the SonarCloud code-smell cleanup. Mechanical, low-risk fixes that resolve the highest-volume rules without behaviour change. Phases 2 (React hook hygiene, ~24 issues) and 3 (cognitive complexity, ~55 issues) will follow as separate PRs.
Issues closed: ~52
go:S1192internal/httpconst,internal/storage/sql_consts.go, package-local consts ininternal/mcp/tools.go,main.go,test/*service)typescript:S1444MockWebSocketstatic fields →static readonly; reset via.length = 0go:S8193if-conditionsshelldre:S7688[ ... ]→[[ ... ]]inscripts/setup-git-signed.shtypescript:S2933ErrorBoundaryreset/reload class fields →readonlytypescript:S7735typeof X !== 'undefined' ? a : b→=== 'undefined' ? b : atypescript:S4325typescript:S6759Readonly<Props>onTopNav,Boomtest helpertypescript:S3735void load()→load().catch(() => undefined)godre:S8188pg_partitions_test.gosplit deferred cancel/StopWhy a new package for HTTP constants?
Content-Typeandapplication/jsonare duplicated across three packages (api,mcp,ingest). A leafinternal/httpconstpackage gives one canonical home with no import cycle risk. Storage SQL fragments are package-local, so they live ininternal/storage/sql_consts.go.Issues intentionally NOT fixed in Phase 1
godre:S8188test-helper cancels (graphrag/builder_test.go,mcp/tenant_isolation_test.go): the helper returns before the workers run; immediatedefer cancel()would cancel before the test body executes. Thet.Cleanuppattern is correct. Will mark False Positive in SonarCloud.typescript:S4325inuseWebSocket.ts:as WebSocketRefadds thestatusfield to aMutableRefObject; the cast IS structurally required. Will mark False Positive.godre:S8196invectordb/replay.go: renameReplaySource→LogsForVectorReplayer. Domain-meaningful name preferred over rote-ersuffix. Will mark False Positive.go:S107(too many parameters):graphrag/store.go:UpsertLogClusterWithTemplate,storage/trace_repo.go:GetTracesFiltered. Need parameter-struct extraction — Phase 3 territory.go:S3776cognitive complexity: real refactors, deferred to Phase 3.typescript:S7764React hook hygiene (4 files:useWebSocket,useMediaQuery,ErrorBoundary, hook test): needs careful per-effect review + UI verification — deferred to Phase 2.Coverage gap (separate finding)
While investigating maintainability I noticed coverage isn't visible in SonarCloud. Root cause:
sonar-project.propertiesin the repo.go test -racewithout-coverprofile.sonarsource/sonarcloud-github-actioncan.Enabling coverage means switching analysis modes (similar weight to the 2026-04-28 reversal). Flagging here, not changing as part of this PR.
Test plan
go vet ./...— cleango build ./...— cleango test -timeout 180s ./...— 516 passed in 28 packagesnpx tsc --noEmit -p ui/tsconfig.app.json— only pre-existingbaseUrldeprecation, no new errorsServiceSidePanel.test.tsx, not introduced)🤖 Generated with Claude Code