fix(deployment): coerce the provider's null lease-status arrays at the query boundary - #3616
Conversation
…e query boundary The redesigned Details tab blanked with "Cannot read properties of null (reading 'map')" for any deployment whose services expose no URIs, such as one declaring only RANDOM_PORT endpoints. Providers marshal Go nil slices and maps as JSON null, so such a service arrives as uris: null. The endpoint converters guarded with default parameters, which only substitute for undefined, and LeaseStatusDto claimed uris was always string[] (and typed ips as any), so nothing flagged the unsafe map. Normalize the nulls in the lease-status queryFn instead of widening the type through every call site. LeaseStatusDto's arrays are now true rather than merely guarded, existing consumers keep their guards and compile untouched, and the unguarded Object.keys(leaseStatus.services) calls stop being a latent crash too.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe lease query now normalizes nullable provider endpoint collections into typed empty records and arrays. Endpoint converters and placement rows accept nullable values. Tests cover normalization, prototype-safe service keys, link conversion, and empty-state rendering. ChangesNullable lease endpoint handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change normalizes nullable deployment status arrays at the query boundary and adds focused regression coverage; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentPlacements/PlacementServiceRow.spec.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentPlacements/PlacementServiceRow.tsxESLint skipped: the matched ESLint configuration already failed (missing-dependency). apps/deploy-web/src/components/deployments/DeploymentDetail/DeploymentPlacements/ServiceEndpoints.spec.tsxESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3616 +/- ##
==========================================
- Coverage 77.84% 77.12% -0.73%
==========================================
Files 1171 1078 -93
Lines 30586 28154 -2432
Branches 7645 7174 -471
==========================================
- Hits 23811 21714 -2097
+ Misses 5967 5662 -305
+ Partials 808 778 -30
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. Straightforward, well-tested null-safety fix that normalizes the provider's nullable lease-status collections at the query boundary.
Extended reasoning...
Overview
The PR fixes a crash (TypeError: Cannot read properties of null (reading 'map')) that occurs when a provider reports a service with no URIs, since Go marshals nil slices/maps as JSON null. The fix normalizes forwarded_ports, ips, and each service's uris to empty collections once in useLeaseStatus's queryFn (via a new normalizeLeaseStatus function), replacing the previous ad-hoc default-parameter guards (which only cover undefined, not null) in ServiceEndpoints.tsx. It also deduplicates the ForwardedPort/ServiceIp type declarations into useLeaseQuery.ts and tightens ips from any to a real type. The three converter functions and PlacementServiceRowProps additionally accept | null as defense in depth.
Security risks
None. This is purely defensive null-handling on read-only display data (lease status endpoints/ports/IPs shown in the UI). No auth, injection, or data-exposure surface is touched. I verified the __proto__-keyed-service test case: lodash/mapValues iterates via Object.keys/baseAssignValue, which safely treats __proto__ as an own property rather than reassigning the prototype, consistent with the added test.
Level of scrutiny
Moderate-low. The change touches a shared query hook (useLeaseStatus) consumed by several components (LeaseRow, PlacementCard, DeploymentLogs, DeploymentLeaseShell), but the change is additive/normalizing rather than behavior-changing for well-formed data — arrays that were already non-null pass through mapValues unchanged. I confirmed the description's claim that Object.keys(leaseStatus.services) is used unguarded in multiple call sites, all of which now benefit from the fix without being touched directly. omitAttestationSidecar, which composes with the normalized result in select, is generic over Record<string, V> | null | undefined and unaffected by the type tightening.
Other factors
Test coverage is thorough and targeted: ServiceEndpoints.spec.tsx covers each converter with null/undefined, useLeaseQuery.spec.tsx covers normalizeLeaseStatus directly (including nil-map and nil-slice cases and the __proto__ edge case), and PlacementServiceRow.spec.tsx reproduces the original crash and confirms the fix end-to-end. The bug-hunting system found no issues, and the one candidate raised (a null individual service entry crashing normalizeLeaseStatus) was examined and refuted. The diff is self-contained to the query/component pair described and doesn't touch CODEOWNER-sensitive or security-critical code.
1 participant
Why
The redesigned Details tab blanked out with
TypeError: Cannot read properties of null (reading 'map')for any deployment whose services expose no URIs. A deployment declaring onlyRANDOM_PORTendpoints hits it every time.Providers marshal Go nil slices as JSON
null, so such a service arrives asuris: null. The endpoint converters guarded with default parameters, which only substitute forundefined:Nothing flagged the unsafe
mapbecauseLeaseStatusDtoclaimeduriswas alwaysstring[]and typedipsasany. The rest of the codebase already knew better:DeploymentDetailHeaderwritesservice.uris ?? [],DeploymentNamewrites(service.uris || []).map(...), andDeploymentName.spec.tsxhas had auris: nullcase for a while.Ref CON-822
What
Rather than widen
uris,forwarded_ports, andipsto| nulland add a guard at every call site, this normalizes the nulls once in the lease-statusqueryFn.LeaseStatusDtois now a type that is actually true instead of one that needs guarding, so no future caller can re-arm the trap.normalizeLeaseStatuscoerces a nil map to{}and a nil slice to[]for all three collections.LeaseStatusResponsedescribes the wire shape (nullable) andLeaseStatusDtodescribes what consumers get (never null).ForwardedPortandServiceIpare now declared once inuseLeaseQuery.ts.forwarded_portspreviously carried an inline duplicate of the former andipswasany.PlacementServiceRowPropsaccept| nullas defense in depth, so an un-normalized caller degrades to an empty list instead of throwing.queryFnrather than inselectkeeps the coerced value cached per fetch, so React Query's structural sharing holds the reference stable across the 30s status refetch. That avoids the hand-rolled referential-stability danceomitAttestationSidecarneeds, and the caller-suppliedselectcontract is unchanged.Forwarded ports and IPs are covered on the same path as URIs, at both the map level and the per-service level. Two smaller things fall out of this:
Object.keys(leaseStatus.services)inPlacementCardandLeaseRowwas unguarded and would have thrown the same way on a nil services map. It can't now.LeaseRow.tsx,DeploymentName.tsx, andDeploymentDetailHeader.tsxare untouched and still compile. Their existing guards are redundant after this, but harmless, and leaving them alone keeps this diff off a component with no test coverage.I swept the rest of the redesigned detail page for the same class of bug and found nothing else. The other provider-proxy consumers on the page (attestation evidence, TEE carve-outs) read chain data or already-guarded arrays. Chain REST goes through grpc-gateway, which emits
[]for an empty repeated field rather thannull, which is why this only ever bit the provider path.Tests
PlacementServiceRow.spec.tsxrenders withuris,forwardedPorts, andipsall null and expects theNoneplaceholder. This one reproduced the reported stack trace before the fix.useLeaseQuery.spec.tsxcovers the query boundary end to end plusnormalizeLeaseStatusdirectly, including a nil services map and a service named__proto__.ServiceEndpoints.spec.tsxcovers each converter withnullandundefined.Verification
tsc --noEmitreports the same 88 pre-existing errors before and after, with zero delta and none in the touched files.eslint --quietis clean./deployments/[dseq]/previewcompiles under Turbopack, confirming the new type-only import across the query and component boundary is erased as expected.Summary by CodeRabbit
Bug Fixes
Tests