refactor: clear some STRICTNESS_MIGRATION suppressions - #13873
Conversation
### Description
Removes the remaining `@ts-expect-error STRICTNESS_MIGRATION` / `as any`
suppressions left after the first pass (which covered the zero-behavior-change
fixes). This batch fixes real null-safety holes that the suppressions were
hiding, so each one is a small, deliberate behavior change:
- BidButton: renders nothing instead of throwing when `sale` is missing.
- MakeOfferButton / InquiryMakeOfferButton: guard a null mutation payload
instead of throwing inside the Relay callback.
- SendConversationMessage: skip the optimistic update instead of throwing
when the connection/edge isn't found.
- Gemini upload helpers: reject with a real error instead of throwing on a
null asset/credentials payload.
- FeaturedArtists: filter out null artist entries instead of crashing on
`.internalID`; no-op tracking if `tracking` is undefined.
- FullFeaturedArtistList: fix an invalid spacing value (`pb={20}` → `pb="20px"`).
- useScreenDimensions: seed the context with real dimensions instead of `null`.
- DeepZoomTile: silence two known-safe conditional hook calls gated by a
module-level debug constant, surfaced while touching this file.
`yarn tsc` is clean repo-wide. Remaining `STRICTNESS_MIGRATION` markers are
confined to the legacy City Guide / Map scenes, out of scope for this PR.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Restores the STRICTNESS_MIGRATION suppressions in the five spots review flagged, keeping the unrelated cleanups in those files. To be revisited separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| block | ||
| // @ts-expect-error STRICTNESS_MIGRATION --- 🚨 Unsafe legacy code 🚨 Please delete this and fix any type errors if you have time 🙏 | ||
| onPress={() => redirectToBid(incrementCents)} | ||
| onPress={() => incrementCents != null && redirectToBid(incrementCents)} |
There was a problem hiding this comment.
🟡 The != null guard turns the Bid button into a silent no-op when the increment has no cents.
BidIncrementsFormatted.cents is nullable in the schema (data/schema.graphql:7234 — cents: Float), so incrementCents can be null at runtime. Before, redirectToBid(null) at least pushed the user to the bid screen (with a broken ?bid=null); now the primary commerce CTA does nothing at all and gives no feedback.
There's also an inconsistency: when saleArtwork.increments is empty, firstIncrement short-circuits to 0, so incrementCents is 0, which passes != null and navigates with ?bid=0. Two adjacent "no usable increment" cases behave differently.
Either handle the missing value explicitly (navigate without a bid param, or disable the button) or keep the previous navigation and fix the URL — but the current shape makes a real failure invisible.
There was a problem hiding this comment.
this one might be worth paying attention to, would be curious what happens now when incrementCents is null or undefined, if you really can't bid in this case it probably makes sense to disable the button and possibly throw something?
| if (!conversationStore || !newMessageEdge) { | ||
| return | ||
| } | ||
| const connection = ConnectionHandler.getConnection( | ||
| // @ts-expect-error STRICTNESS_MIGRATION --- 🚨 Unsafe legacy code 🚨 Please delete this and fix any type errors if you have time 🙏 | ||
| conversationStore, | ||
| "Messages_messagesConnection" | ||
| ) | ||
| // @ts-expect-error STRICTNESS_MIGRATION --- 🚨 Unsafe legacy code 🚨 Please delete this and fix any type errors if you have time 🙏 | ||
| if (!connection) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 These bare returns swallow a failure the user will notice.
storeUpdater runs as both optimisticUpdater and updater. If the connection or edge isn't found in the updater path, the mutation still succeeded on the server — the message was sent — but it never gets inserted into Messages_messagesConnection, so the thread looks like the message vanished. Before, the throw at least surfaced in logs.
Add observability so this doesn't become a silent class of bug:
if (!conversationStore || !newMessageEdge) {
captureMessage("sendConversationMessage: missing conversation record or message edge")
return
}(same for the !connection case). captureMessage from @sentry/react-native is already used elsewhere in the app, e.g. src/app/Scenes/Artwork/Components/ImageCarousel/ImageCarousel.tsx:195.
| ) | ||
| } | ||
|
|
||
| // eslint-disable-next-line react-hooks/rules-of-hooks -- VISUAL_DEBUG_MODE is a module-level constant, so this condition never changes across renders of a given instance |
There was a problem hiding this comment.
🟢 This suppression is avoidable. useSpringValue doesn't depend on anything inside the VISUAL_DEBUG_MODE block — hoisting it up next to the other hooks (above line 89) makes the call unconditional and removes the need for the disable. The debug branch pays nothing since VISUAL_DEBUG_MODE is false (__deepZoomDebug.ts:1).
The useEffect one at line 91 genuinely needs the escape hatch, so that can stay.
|
|
| jest.fn<typeof relay.commitMutation, Parameters<typeof relay.commitMutation>>(fn as any) | ||
| const commitMutationMock = ( | ||
| fn?: (environment: any, config: MutationConfig<MutationParameters>) => Disposable | null | ||
| ) => jest.fn(fn) |
brainbicycle
left a comment
brainbicycle
left a comment
There was a problem hiding this comment.
looks great, the silent fail on bidIncrements I think worth a second look, if you already looked feel free to disregard
4 participants
This PR resolves []
Description
Removes most of the remaining
@ts-expect-error STRICTNESS_MIGRATION/as anysuppressions. Marker count goes from 143 across 43 files onmainto 63 across 22 files.Most of the diff is type-only (proper param/return annotations,
NonNullable<>instead of asserting through nullable Relay fields) with no runtime change. The exceptions are called out below.Deliberate behavior changes
nullinstead of throwing whensaleis missing (replaces aTODO: Do we need a nil check against +sale+?), and no longer callsredirectToBidwhen the bid increment is unavailable.createGeminiAssetWithS3Credentials/getGeminiCredentialsForEnvironmentreject with a realErroron a null asset/credentials payload instead of throwing on a null property access.trackingprop is undefined, rather than throwing.pb={20}→pb="20px").ScreenDimensionsContextis seeded with real dimensions instead ofnull as any, so consumers rendered outsideProvideScreenDimensionsget usable values instead of crashing.request.statusdirectly rather than off the (untyped) event target._cacheand adds two scopedreact-hooks/rules-of-hooksdisables for hook calls gated by the module-levelVISUAL_DEBUG_MODEconstant — surfaced while touching this file, no runtime change.Dead code removal
AuctionPrice.tsxand its test file (~400 lines). Nothing imports it — it was left behind by an earlier refactor, and it only came up because it carried suppressions.Intentionally left suppressed
Review flagged five spots where clearing the suppression meant picking new null-handling behavior. Rather than make those calls in a cleanup PR, they keep their existing suppressions and are deferred to a follow-up:
MakeOfferButton.tsx/InquiryMakeOfferButton.tsx— a null mutation payload needs a real error path, not a silent return.FeaturedArtists.tsx— the artist counts used for the header and "View all" link need to agree with what actually renders.DeepZoomOverlay.tsx— needs the call-site guard inImageZoomViewtightened before the nesteddeepZoom.imageaccess can be made safe.uploadFileToS3.ts—formDataloop, left as-is.The other remaining markers (55 of 63) are in the legacy City Guide / Map scenes and their helpers, which are out of scope here.
PR Checklist