Skip to content

Return a stable no-data strain target - #2015

Merged
Asherlc merged 3 commits into
mainfrom
codex/issue-1979
Jul 26, 2026
Merged

Asherlc merged 3 commits into
mainfrom
codex/issue-1979

Conversation

@Asherlc

@Asherlc Asherlc commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • return explicit null when no strain target can be computed
  • allow the dashboard’s strain target contract to represent that no-data state
  • prevent tRPC from serializing undefined as a result with no data field, which TanStack Query rejects

Closes #1979

Validation

  • regression test failed first with expected undefined to be null
  • pnpm exec vitest run packages/server/src/routers/recovery.test.ts --project unit (131 passed)
  • focused server/dashboard tests (183 passed)
  • pnpm exec tsc --noEmit
  • pnpm --filter dofek-server exec tsc --noEmit
  • pnpm --filter dofek-web exec tsc --noEmit
  • pnpm --filter dofek-web build
  • pnpm lint
  • pnpm test (836 files, 13,719 tests passed; 21 skipped)

Summary by cubic

Return null for missing strain targets and validate the response shape in tRPC, ensuring a stable no‑data state and avoiding omitted data fields (Linear #1979).

  • Bug Fixes

    • Server: recoveryRouter.strainTarget returns null when no recovery summary exists and validates responses with .output(strainTargetResultSchema.nullable()).
    • Web/Tests: DailyOverview accepts StrainTargetResult | null; unit test updated to expect null.
  • Refactors

    • Server: derive StrainTargetResult from strainTargetResultSchema (z.infer) to keep types and runtime validation in sync.

Written for commit 745a223. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling when recovery data is unavailable by consistently returning and supporting a null result.
    • Daily overview displays remain unchanged when no strain target data exists.
  • Tests

    • Updated recovery endpoint coverage to verify the expected null response when no summary is available.

Preserve the explicit no-data state through tRPC so TanStack Query does not reject an omitted data field.
Copilot AI review requested due to automatic review settings July 26, 2026 05:57
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sourcery-ai sourcery-ai Bot 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.

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Return null for missing strain target to preserve no-data state

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Return explicit null when no strain target can be computed.
• Align the dashboard strain-target contract to represent the no-data state.
• Avoid tRPC/React Query issues caused by undefined being serialized as omitted data.
Diagram

sequenceDiagram
  participant Web as "Dashboard UI"
  participant TSQ as "TanStack Query"
  participant TRPCc as "tRPC Client"
  participant TRPCs as "tRPC Server"
  participant Router as "recoveryRouter"
  participant DB as "Sensor Store/DB"

  Web->>TSQ: "useQuery(strainTarget)"
  TSQ->>TRPCc: "fetch"
  TRPCc->>TRPCs: "RPC strainTarget"
  TRPCs->>Router: "strainTarget query"
  Router->>DB: "load readiness summary"
  DB-->>Router: "no rows"
  Router-->>TRPCs: "null (no-data)"
  TRPCs-->>TRPCc: "data: null"
  TRPCc-->>TSQ: "resolved null"
  TSQ-->>Web: "render no-data state"
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Discriminated union result (e.g., {status:'no_data'} | {status:'ok', data: ...})
  • ➕ More explicit than null; easier to extend with reasons/metadata
  • ➕ Avoids accidental null handling bugs via exhaustive checks
  • ➖ More boilerplate across server/client types and UI branching
  • ➖ Bigger API surface change than needed for this bug fix
2. Always return an object wrapper (e.g., {target: StrainTargetResult | null})
  • ➕ Stable response shape; avoids 'missing data field' issues entirely
  • ➕ Easier to add additional fields later without changing top-level type
  • ➖ Requires more code churn and migrations across callers
  • ➖ Less ergonomic than returning the target directly for the common case

Recommendation: The current approach (returning null instead of undefined) is the best minimal fix: it preserves an explicit no-data state through tRPC serialization and matches TanStack Query expectations, while keeping the API surface small. Consider a discriminated union only if you expect multiple distinct no-data/error states that the UI must differentiate.

Files changed (3) +6 / -6

Bug fix (2) +4 / -4
recovery.tsReturn null when readiness metrics are missing in strainTarget query +2/-2

Return null when readiness metrics are missing in strainTarget query

• Changes the strainTarget query return type from 'StrainTargetResult | undefined' to 'StrainTargetResult | null' and returns 'null' when no readiness metrics are available.

packages/server/src/routers/recovery.ts

DailyOverview.tsxAllow null strainTarget in DailyOverview component props +2/-2

Allow null strainTarget in DailyOverview component props

• Updates component prop types so strain target can be explicitly 'null', matching the server contract and enabling a stable no-data state in the UI layer.

packages/web/src/components/DailyOverview.tsx

Tests (1) +2 / -2
recovery.test.tsUpdate strainTarget no-data expectation to null +2/-2

Update strainTarget no-data expectation to null

• Renames the test case to reflect the no-data contract and asserts 'null' is returned when no recovery summary exists.

packages/server/src/routers/recovery.test.ts

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The strain target endpoint now returns null when no readiness metrics exist. Server tests and web component prop types are updated to reflect the normalized no-data value.

Changes

Strain target no-data handling

Layer / File(s) Summary
Server strain target contract
packages/server/src/routers/recovery.ts, packages/server/src/routers/recovery.test.ts
The endpoint returns null when readiness metrics are missing, and the corresponding test expects null.
Web strain target types
packages/web/src/components/DailyOverview.tsx
DailyOverview and StrainBreakdown accept `StrainTargetResult

Estimated code review effort: 2 (Simple) | ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Empty dashboards display a normal Strain no-data state instead of an internal query-key error [#1979]

Possibly related PRs

  • Asherlc/dofek#1141 — Updates the same recoveryRouter.strainTarget response behavior and client handling.

Suggested labels: area/server, area/web, type/bug

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title matches the change, but it omits the required area prefix for a cross-package update. Prefix it with the relevant area, such as "[server] Return a stable no-data strain target", and keep it imperative with no trailing punctuation.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Storybook previews for 266a7934 are ready:

This comment updates automatically on each PR push.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/server/src/routers/recovery.ts`:
- Around line 606-608: Add a Zod schema named strainTargetResultSchema matching
StrainTargetResult near the existing schemas, then attach it to the strainTarget
cachedProtectedQuery with .output(strainTargetResultSchema.nullable()). Preserve
the nullable runtime contract so valid results or null are accepted and
malformed or undefined values are rejected.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: be47c6ac-177d-407b-849f-5362abf57358

📥 Commits

Reviewing files that changed from the base of the PR and between 1329954 and 5e3929d.

📒 Files selected for processing (3)
  • packages/server/src/routers/recovery.test.ts
  • packages/server/src/routers/recovery.ts
  • packages/web/src/components/DailyOverview.tsx

Comment thread packages/server/src/routers/recovery.ts
@qodo-code-review

qodo-code-review Bot commented Jul 26, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 171 rules

Grey Divider


Remediation recommended

1. strainTarget missing .output() schema ✓ Resolved 📘 Rule violation ≡ Correctness
Description
The modified recoveryRouter.strainTarget tRPC procedure defines an input schema but still has no
.output(...) Zod schema, relying only on a TypeScript return type (`Promise<StrainTargetResult |
null>`). This violates the requirement to validate procedure outputs (including nullable outputs)
with Zod at runtime.
Code

packages/server/src/routers/recovery.ts[608]

+    .query(async ({ ctx, input }): Promise<StrainTargetResult | null> => {
Relevance

⭐⭐⭐ High

Team has accepted adding missing tRPC .output(...) schemas to enforce runtime response validation.

PR-#1123

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 722090 requires every modified tRPC procedure to define both .input(...) and
.output(...) Zod schemas; the modified strainTarget procedure shows .input(...) followed
directly by .query(...) with no .output(...), while also changing the return type to include
null.

Rule 722090: Define Zod schemas for all tRPC procedure inputs and outputs
packages/server/src/routers/recovery.ts[606-609]
packages/server/src/routers/recovery.ts[680-681]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`recoveryRouter.strainTarget` was modified to return `StrainTargetResult | null`, but the procedure still omits an `.output(...)` Zod schema. Compliance requires all tRPC procedures to declare both input and output schemas, including explicit `nullable()` when `null` is a valid result.

## Issue Context
This procedure currently uses only a TypeScript return type annotation (`Promise<StrainTargetResult | null>`) and returns `null` when no readiness metrics exist; without `.output(...)`, the runtime contract is not validated.

## Fix Focus Areas
- packages/server/src/routers/recovery.ts[606-609]
- packages/server/src/routers/recovery.ts[680-681]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Null result gets cached 🐞 Bug ☼ Reliability
Description
recoveryRouter.strainTarget now returns null for the no-readiness case, and because the cached
query middleware treats only undefined as a cache miss, that null will be cached and reused
until the cache TTL expires. This can keep the dashboard in a stale “no data” state for up to 10
minutes even after readiness data becomes available, unless the cache key is explicitly invalidated.
Code

packages/server/src/routers/recovery.ts[681]

+      if (!readinessMetrics) return null;
Relevance

⭐ Low

Similar cache-staleness concern (cached response becomes outdated after upstream data changes) was
previously rejected.

PR-#1961

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The procedure is cached with a 10-minute TTL and now returns null when readiness is missing. The
caching middleware considers any cached value other than undefined a hit and writes result.data
to the cache on success; the cache layer explicitly supports storing null, so a null response
becomes sticky until TTL expiry.

packages/server/src/routers/recovery.ts[602-683]
packages/server/src/trpc.ts[205-209]
packages/server/src/trpc.ts[270-317]
packages/server/src/lib/cache-extended.test.ts[106-111]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`cachedProtectedQuery` caches successful results even when `result.data` is `null`. After this PR, `recovery.strainTarget` returns `null` when no readiness row exists, so the first "no data" response will be cached and repeatedly served until TTL expiry.

### Issue Context
- `CacheTTL.MEDIUM` is 10 minutes.
- The caching middleware uses `hit !== undefined` to determine cache hits, so cached `null` is treated as a valid hit.

### Fix Focus Areas
- packages/server/src/routers/recovery.ts[606-683]
- packages/server/src/trpc.ts[205-209]
- packages/server/src/trpc.ts[270-317]

### Suggested fix
Implement a per-procedure cache policy option (e.g., `cacheNull?: boolean`, defaulting to `true`) in `packages/server/src/trpc.ts`:
- On lookup: treat `hit === null` as a miss when `cacheNull` is `false`.
- On store: do not `queryCache.set(...)` when `result.data === null` and `cacheNull` is `false`.

Then set `cacheNull: false` for `recovery.strainTarget`, so it still returns `null` to satisfy the tRPC/TanStack contract but does not pin the empty state in cache.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread packages/server/src/routers/recovery.ts
@Asherlc

Asherlc commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Review follow-up: I verified the optional null-cache note against the cache invalidation paths. null is an intentional successful no-data response and follows the same TTL policy as other valid cached results; successful provider/HealthKit sync and ingestion invalidate the user cache. Adding a new per-procedure cache bypass would broaden this focused serialization fix and change established cache semantics, so I am not adding it here.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

2 similar comments
@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

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.

Empty account dashboard renders an internal query key instead of a Strain empty state

2 participants