chore(sonar): honest analysis surface — mobile screens/components/hooks + re-justified exclusions (#243 Phase 1) - #446
Conversation
…/hooks, re-justify exclusions (#243 Phase 1) Phase 1 (config only, no app code) of the #243 pre-launch gate: make SonarCloud analyze the right surface so the metrics mean something, before any burn-down. sonar.sources: add apps/mobile/{app,components,hooks} for web parity — mobile screens, components, and hooks are now analyzed on both platforms (previously only stores/lib/widget were, while web analyzed everything). sonar.coverage.exclusions — tightened to genuinely-untestable code only, every entry justified inline: - DROP apps/web/components/** — heavily behavior-tested (~153 component tests); a blanket coverage exclusion was the "broom" the campaign forbids. Decided the components parity question by TIGHTENING both (no mobile components exclusion), so the eventual "100% on analyzed surface" stays meaningful. - DROP apps/web/app/**/page.tsx — route entries hold real tested app logic (e.g. TodayPage: 573 lines, selection-cascade/bulk-op branching + a page test). - DROP apps/mobile/lib/google-auth.ts — its lcov is actually measured by vitest; excluding it discarded real coverage and hid its uncovered native lines. - ADD apps/mobile/app/**/_layout.tsx — Expo Router nav/bootstrap, parity mirror of the kept web app/**/layout.tsx. - KEEP (re-justified): web layout.tsx, web app/api/** (BFF platform glue), mobile bootstrap/native-seam libs (providers, theme-provider, use-app-theme, supabase, offline-queue, orbit-widget, auth-store), modules/**, shared theme/button.ts. sonar.cpd.exclusions — all 7 re-justified inline (thin server-action shells, re-export barrels, deliberate web/mobile mirrors, pure token/scheme data); none stale or over-broad, kept as-is. The lcov merge (web+mobile+shared prefixes) and the push:main trigger already feed overall/main analysis correctly — confirmed, unchanged. #243
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #446
Scope: chore(sonar): honest analysis surface — mobile screens/components/hooks + re-justified exclusions (#243 Phase 1)
Recommendation: APPROVE
Summary
Single-file, config-only diff (sonar-project.properties, +32/-2). It widens sonar.sources so mobile app/, components/, hooks/ are analyzed like web already is, tightens sonar.coverage.exclusions to genuinely-untestable code with an inline one-line justification per entry, and re-justifies (unchanged) sonar.cpd.exclusions. No apps/web/**, apps/mobile/**, orbit-api/**, or packages/shared/src/types/** source files changed, so every gated dimension/subagent below is N/A by design — this is exactly the "config only, no app code" scope the PR body claims.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
None (per the rubric's signal gate, Low/Info nits are not posted even if noticed).
Verification performed
sonar.sourcesdiff adds exactlyapps/mobile/app/,apps/mobile/components/,apps/mobile/hooks/— all three directories exist and are non-empty.sonar.coverage.exclusionsdiff: droppedapps/web/app/**/page.tsx,apps/web/components/**,apps/mobile/lib/google-auth.ts; addedapps/mobile/app/**/_layout.tsx. All 12 remaining entries exist on disk, and the new inline comment block lists exactly those 12 paths in the same order — no drift between comment and directive.google-auth.tsdrop claim (its lcov is actually measured by vitest, not vitest-excluded, has a test) verified againstapps/mobile/vitest.config.ts:coverage.includeis['lib/**/*.{ts,tsx}', 'stores/**/*.ts']andcoverage.excludedoes not listgoogle-auth.ts(it does list the other mobile lib exclusions that correctly remain in the sonar list). A test file exists atapps/mobile/__tests__/lib/google-auth.test.ts. Claim confirmed true.sonar.cpd.exclusionsline is unchanged in the diff (pure context, no+/-) — matches the PR body's "all 7 re-justified inline, kept" claim.- No inaccuracy, drift, or unjustified exclusion found.
Rubric dimensions
Correctness: PASS (diff matches PR body, verified against live repo state). All other dimensions (dead code, SOLID, comments, workarounds, type safety, console.log, DESIGN.md/AI-slop, parity, i18n, contract drift/backward-compat, security, backend rules) are N/A — the only changed file is root sonar-project.properties, which doesn't touch any app/API/shared source.
Subagents
None fire — diff doesn't touch apps/web/**, apps/mobile/**, orbit-landing-page/src/**, orbit-api, or packages/shared/src/types/*. (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer: all N/A)
Validation
Skipped per CI-wrapper instructions — this PR runs Build / Unit Tests / SonarCloud as separate required checks (SonarCloud Quality Gate already passed per the bot comment on this PR).
What's good
- Every exclusion carries a one-line, checkable WHY justification inline in the properties file itself.
- The PR body states the expected coverage regression up front (0% until Phase 2) rather than hiding it; every quantitative claim spot-checked held up against the actual repo state.
- Correctly resolves the pre-existing web/mobile asymmetry (
apps/web/components/**was excluded, mobile had no equivalent) by tightening rather than loosening. - Clean scope discipline: explicitly defers the vitest
coverage.includewidening and new native-hook exclusions to a named Phase 2, rather than scope-creeping this "make the surface honest" PR into a metric burn-down.
Recommendation
APPROVE. No Critical/High findings, no drift between the PR's narrative and the actual repo state, scope correctly held to "analysis surface only" per the PR title and #243 Phase 1 framing.
…es (orbit-ui-mobile#243 Phase 1) (#316) Phase 1 (config only, no app code) of the pre-launch SonarCloud gate — the consumer side is thomasluizon/orbit-ui-mobile#243. Make the coverage surface honest before any burn-down. - Widen /Extensions/ServiceCollectionExtensions.cs -> ServiceCollectionExtensions*.cs: the exact-name glob matched only 1 of 4 DI partials; all 4 are DI registration, so exclude them consistently. - DROP Services/AppConfigService.cs (pure ConvertValue<T> + injectable cache/repo) and Services/UserDateService.cs (user-timezone "today" + week-start, injectable deps) from coverage.exclusions — both are unit-testable and live in Orbit.Infrastructure, which is already in the coverage run. - Every remaining exclusion re-justified inline (bootstrap/DI, EF/generated, options POCOs, external-SDK glue, hosted BackgroundServices), with Phase-2 extract targets flagged: MinimumVersionMiddleware's semver comparator, GeoLocation's IsPrivateIp classifier, GoogleToken's outcome classification. opencover paths (**/coverage.opencover.xml) span all three coverage-producing test projects (Domain/Application/Infrastructure.Tests); Analyzers.Tests runs outside the runtime coverage set by design, and there is no Api test project yet. Confirmed, unchanged. Paired PR: thomasluizon/orbit-ui-mobile#446. thomasluizon/orbit-ui-mobile#243
|
There was a problem hiding this comment.
Code Review: PR #446
Scope: PR #446, single file changed: sonar-project.properties (+32/-2), no app code.
Recommendation: APPROVE
Summary
Config-only change widening sonar.sources to analyze apps/mobile/app/, apps/mobile/components/, apps/mobile/hooks/ (parity with web), tightening sonar.coverage.exclusions to genuinely-untestable files with an inline justification per entry, and re-justifying sonar.cpd.exclusions. Every factual claim in the PR body was independently verified against the live repo (not just trusted from the description) and everything checked out.
Findings
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Critical / High / Medium: None.
Info: Coverage % on both projects will drop and duplication will spike as an explicit, intended, and already-documented consequence — mobile apps/mobile/vitest.config.ts coverage.include is confirmed still lib/** + stores/** only, so newly-analyzed app/components/hooks emit no lcov yet. PR body names this and defers the fix to a stated Phase 2. No action needed.
Verification performed (facts checked, not just read)
apps/web/components/**: 235 files vs. 145 matching__tests__(PR claimed "~235 / ~153" — close enough, confirms "heavily tested").apps/web/app/**/page.tsx(TodayPage): test exists atapps/web/__tests__/app/today-page.test.tsx.apps/mobile/lib/google-auth.ts: present, inside vitestcoverage.include: lib/**, not in vitest'scoverage.exclude, hasapps/mobile/__tests__/lib/google-auth.test.ts— confirms it was wrongly coverage-excluded before and is correctly dropped now.apps/mobile/app/**/_layout.tsxglob: matches the three real files (app/_layout.tsx,app/(onboarding)/_layout.tsx,app/(tabs)/_layout.tsx).packages/shared/src/theme/button.ts: confirmed a branchless static token table..github/workflows/sonarcloud.yml: confirmed lcov merge prefixesapps/web/apps/mobile/packages/sharedcorrectly andpush: [main]keeps overall/main analysis running — matches the PR body's "Confirmations" section.- No stray non-
.ts/.tsxfiles inside the newly-addedapps/mobile/app,components,hookssource globs that would need a hard exclusion. - Every coverage-exclusion and cpd-exclusion entry in the properties file has a matching one-line inline comment; counts match (12 coverage-exclusion entries / 12 comment lines, 7 cpd-exclusion entries / 7 comment lines).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — gate not met (no apps/web/**/apps/mobile/** source file changed, only root sonar-project.properties) |
| i18n-syncer | N/A |
| contract-aligner | N/A |
| security-reviewer | N/A |
| design-reviewer | N/A |
Validation
Phase 6 (/validate) skipped per CI workflow instructions — this PR runs Build / Unit Tests / SonarCloud as separate required checks. No TS/JS/C# files are in this diff regardless.
Deferred
- Dimensions #2–#7, #10–#14: N/A, no application code, no i18n/contract/security/backend/feature surface touched.
- #8 DESIGN.md, #9 Parity: N/A by gate — note the PR's substance is a parity fix (mobile sources now mirror web); the remaining gap (mobile vitest
coverage.includenot yet widened) is explicitly named and deferred to the PR's own stated Phase 2, not silently dropped. - Paired
orbit-apicross-repo dimension (contract-aligner, backward-compat guard): not verifiable in this CI job — sibling repo not checked out here. - Second commit on the branch since the prior
claudereview is a merge-from-mainonly, no new substantive diff to re-review.
What's good
Every claim in the PR description survived independent verification against the actual tree, config, and CI workflow — file counts, test existence, vitest include/exclude lists, glob matches, and the lcov-merge step all matched exactly as described. Each exclusion carries its own inline justification in the file, not just in the PR prose, so future drift is self-documenting. Scope discipline is strong: no threshold changes, no gate-required flip, no vitest widening snuck in — all correctly named as Phase 2 work.
Recommendation
Merge as-is. No blocking issues found; this diff earns a plain approval.
…flake web suite (#508) Root cause (#446): sonar.sources analyzed all of apps/mobile app/components/hooks/stores, but apps/mobile/vitest.config.ts only instrumented lib/**, stores/**, and one hook — so every tested mobile screen/component/hook was absent from lcov and scored 0% by SonarCloud, dragging combined coverage to ~57%. - Broaden mobile vitest coverage.include to the full app/components/hooks/stores/lib/widget surface (mirrors sonar.sources) so the existing suite's real coverage lands in lcov. - Add surgical sonar.coverage.exclusions + matching vitest exclude for genuinely-presentational RN code (JSX screens/sections, style tables, Expo Router shells, native bridges, bootstrap glue) — enumerated, never a blanket apps/mobile/** wildcard, so the ~178 render-tested components keep counting. Removed offline-queue.ts + auth-store.ts from the exclusions: now instrumented and well-covered. - Remove the invalid `all` coverage option (not a Vitest 4 type; no runtime effect). - 38 new/deepened mobile test files (behavior + edge + failure) across data hooks, 0%-logic hooks, app-level logic, request/response builders, error mapping, and pure helpers. Mobile kept-set coverage: ~53% raw -> 78.9% lines / 77.6% statements over 10,102 instrumented lines. - De-flake the web suite under parallel/coverage load: server-fetch.test.ts (per-test module reset + vi.stubEnv restore, fixing an APP_VERSION "undefined"-string env leak), push-prompt (reset leaked globalThis.PushManager), tour-store (reset persistent hiddenSections). Green in default and shuffle order. Refs #243 (coverage burn-down) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ss web+mobile+shared (#243) (#509) Second coverage burn-down toward the #243 SonarCloud-to-zero launch gate. Adds ~500 intelligent Vitest tests across the three workspaces and mirrors #508's mobile instrument-gap fix onto web. - apps/web: add coverage.include mirroring sonar.sources so files no test imports are counted (closes the same #446/#508 gap on web, which was mobile-only); new hook/component/page tests. 74% -> 84% lines locally. - apps/mobile: hook/store/lib + component/screen tests; unblock keyboard-aware-scroll-view by adding an emit-capable Keyboard + findNodeHandle to the RN test mock. 79% -> 85%. - packages/shared: store/util/validation/query-key branch tests. 94% -> 99% lines. - Extend sonar.coverage.exclusions with genuinely-presentational web files only (route/error/not-found shells, static legal pages, motion/nav glue, the desktop astra-copilot rail chrome, style primitives) — enumerated with rationale, mirroring #508's mobile set. The 12 borderline web files with real logic are tested, not excluded. - Raise the Vitest coverage thresholds in all three configs to ratchet the gains. Estimated combined SonarCloud line coverage ~87% (up from ~79%). All suites green (shared/web/mobile); web verified under --sequence.shuffle; lint + type-check clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Phase 1 (step a) of #243 — SonarCloud analysis surface, orbit-ui-mobile
Config only, no app code. This is the single slice-shaped phase of the #243 campaign: get the analysis surface honest before any metric burn-down, so "0 smells / 100% coverage" will mean something later. Does not begin Phase 2 (the
/prod-readinessloop or the metric burn-down), and does not flip the Sonar check to required (that's step d).What changed (
sonar-project.properties)sonar.sources— mobile now mirrors web. Addedapps/mobile/app/,apps/mobile/components/,apps/mobile/hooks/. Previously mobile analyzed onlystores/,lib/, and the widget module, while web analyzed everything — so mobile screens/components/hooks weren't analyzed at all. They are all.ts/.tsxwith no co-located tests or generated files, so the whole three dirs go in; nothing deliberately left out of sources.sonar.coverage.exclusions— tightened to genuinely-untestable code, each entry justified inline. Applied one litmus: an exclusion survives only if the file is bootstrap/DI, generated, pure config/tokens, or platform glue.apps/web/components/**apps/web/app/**/page.tsxTodayPageis 573 lines of selection-cascade / bulk-op branching with a page test. Not "genuinely untestable." Dropping it also gives clean parity with mobile route screens (also covered).apps/mobile/lib/google-auth.tslib/**include, not vitest-excluded, has a test). Excluding it discarded real coverage and hid its uncovered native lines.apps/mobile/app/**/_layout.tsxapp/**/layout.tsx.layout.tsx, webapp/api/**, mobileproviders/theme-provider/use-app-theme/supabase/offline-queue/orbit-widget,stores/auth-store,modules/**,shared theme/button.tssonar.cpd.exclusions— all 7 re-justified inline, kept. Thin server-action shells, re-export barrels, deliberate web/mobile mirrors, and pure token/scheme data. None stale, none over-broad.Confirmations (task c)
apps/web/apps/mobile/packages/sharedcorrectly; whatever mobile coverage exists maps toapps/mobile/*. Mechanically correct, unchanged.push: [main], so main is analyzed and overall metrics are tracked, not just PR deltas. Unchanged. (The SonarCloud new-code definition is a dashboard setting outside these files — verify it in the project UI when flipping the gate in step d.)Honest Phase-1 state (expected, intended)
Coverage % will drop on both projects — that's the point. The newly-added mobile
app/components/hookscurrently emit no lcov (mobilevitest.config.tscoverage.includeislib/**+stores/**), so they read as 0% covered until Phase 2. Duplication will spike too, because web↔mobile parity mirrors are now analyzed in the same project. Both reflect reality; the gate is non-required, so red here is fine.Handoff to Phase 2 (metric burn-down — do NOT do here)
The inventory surfaced these; the burn-down session should:
vitest.config.tscoverage.includetoapp/**+hooks/**(rebaseline thresholds), then write tests.use-ad-mob,use-play-billing,use-speech-to-text,use-push-notifications,use-review-reminder,use-share-card, andapp/(tabs)/profile/_components/use-data-export.ts(native SDK at module scope).offline-queue,orbit-widget,auth-store) at both layers, or keep them excluded with the current justification.packages/sharedwhere possible; justifiedcpd.exclusionsfor genuine unavoidable cross-platform mirrors).Paired orbit-api PR: thomasluizon/orbit-api#316. Part of #243 Phase 1.
#243