Enable exactOptionalPropertyTypes in shared and webapp - #930
Merged
Conversation
Closes #927 (shared + webapp portion; mobile is deliberately deferred to a follow-up, per the issue). shared/: fixes buildCollaborators (conditional spread instead of assigning possibly-undefined fields) and the codespan case in normalizeInlineTokens (narrow the local `src` before using it, since the object literal's src property can't be assigned an explicitly optional-undefined value under the new flag). webapp/: fixes fall into a few groups: - Presentational components (LetterAvatar, NavigationHeader, Sidebar, MarkdownToolbar, SortableItem, NoteCard/SortableNoteCard/ AnimatedNoteGrid, hook option objects) only ever read their optional props via `?.`/`??`/truthiness, so their prop types are widened to `T | undefined` explicitly. - Construction sites that build a fresh object from possibly-undefined source values (shareAvatars.ts, Toast.tsx) use conditional spread so the key is genuinely absent rather than present-with-undefined. - Merge sites that reset a field via `{...prev, field: undefined}` (useNoteImages retry, Dashboard's SSE image-patch handlers) are rewritten to omit the key instead, which is the actual bug class the flag catches. - CreateNoteRequest/CreateListNoteRequest's `labels` field is widened in shared/types.ts since it's serialized via JSON.stringify before reaching the server, which drops undefined-valued keys anyway. - NoteModal.tsx's hand-rolled TFunction type is replaced with i18next's own TFunction import, fixing 8 errors that were an overload-matching quirk rather than a real optional-property issue. - playwright.config.ts's `workers: undefined` is replaced with a conditional spread. No behavior changes; verified with `task check`.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThe shared and webapp TypeScript configurations enable Possibly related PRs
Poem
Merge Risk: ⚪ Minimal · up to This change tightens optional-property type checking and updates affected shared and webapp code without introducing a known behavior change; the reported checks pass, so no actionable merge-blocking risk remains beyond normal review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
hanzei
marked this pull request as ready for review
August 17, 2026 06:18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
hanzei
added a commit
that referenced
this pull request
Aug 17, 2026
* Enable exactOptionalPropertyTypes in mobile Completes the workspace rollout #927 deliberately deferred to a follow-up: shared and webapp landed in #930, mobile compiles the same shared/src and gets those three fixes for free, then adds its own ~49 sites. Each resolves the same way #930 established — widen a pass-through prop or a JSON request/query field to `T | undefined` (the value never reaches the server either way), or conditionally spread at a construction/merge site where the key's presence is meaningful (the retry-reset image-upload path, the toast queue). Also updates CLAUDE.md: the "nothing anywhere runs exactOptionalPropertyTypes" gap note is now stale, replaced with the widen-vs-conditional-spread convention itself. * Drop exactOptionalPropertyTypes convention writeup from CLAUDE.md --------- Co-authored-by: Claude <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
2 participants
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.
Closes #927.
Turns on
exactOptionalPropertyTypesinshared/tsconfig.jsonandwebapp/tsconfig.json(which also coverstsconfig.node.jsonandtsconfig.e2e.jsonviaextends, plusplaywright.config.ts). Mobile is deliberately left for a follow-up issue, per #927's own suggested shape — it compilesshared/srcdirectly, so it gets the 3sharedfixes for free, but its own ~54 errors are a separate pass once the widen-vs-spread convention is settled here.What changed
shared/(3 errors → 0):collaborators.ts'sbuildCollaborators— was assigning possibly-undefinedvalues (owner?.first_name, etc.) directly ontoCollaborator's optional fields. Switched to conditional spread so the key is genuinely absent rather than present-with-undefined.inlineMarkdown.ts'scodespancase — narrowed the computedsrcinto a local before using it in the object literal, matching the pattern the siblingstrong/em/delcases already used.webapp/(44 errors after shared → 0):TFunctioncluster (8 errors), all inNoteModal.tsx: a hand-rolledtype TFunction = (key: string, opts?: Record<string, unknown>) => stringdidn't structurally match i18next's own (much more complex, overloaded)TFunctiontype once the flag was on — an overload-matching quirk, not a real optional-property bug. Replaced it withimport type { TFunction } from 'i18next'.LetterAvatar,NavigationHeader/ProfileMenu,Sidebar,MarkdownToolbar,SortableItem,NoteCard/SortableNoteCard/AnimatedNoteGrid, and theuseNoteDraft/useNoteImages/useCompletedItems/LabelPickerhook-options objects only ever read their optional fields via?./??/truthiness — nothing spreads them onward. These are widened toT | undefinedexplicitly, which changes no behavior.NoteModal'snoteprop — instead of widening every hook'snoteoption,noteis now defaulted tonullat the top of the component ({ note = null, ... }), so the rest of the component (and the hooks it feeds) keep treating it asNote | nulllike they already did everywhere via?..shareAvatars.ts,Toast.tsx's new-toast object) use conditional spread, same ascollaborators.ts.undefined—useNoteImages's retry handler and Dashboard's SSE image-patch handlers were doing{...prev, field: undefined}, which is exactly the silent-field-wipe bug class the flag exists to catch. Rewritten to omit the key (or return the object unchanged) instead of setting it toundefined.CreateNoteRequest/CreateListNoteRequest.labels(shared/src/types.ts) — widened rather than spread-fixed, since these are POST request bodies serialized withJSON.stringify, which dropsundefined-valued keys before the distinction ever reaches the server.playwright.config.ts'sworkers: process.env.CI ? 4 : undefined→ conditional spread, since Playwright's own config type doesn't accept an explicitundefinedthere.Verification
task checkpasses (lint, all tests, docs/migrations/translations checks).shared,webapp/tsconfig.json,tsconfig.node.json, andtsconfig.e2e.jsonall type-check clean with the flag on.Not in scope
mobile/— left for a follow-up issue, as Turn on exactOptionalPropertyTypes — shared first, then webapp #927 suggests, since its ~54 errors are independent of this PR and the widen-vs-spread convention established here should carry over directly.No screenshots — this is a type-checking change with no UI or behavior difference; the two fixed merge-site bugs (stale error message surviving an image-upload retry, and a possible spurious state update on an SSE image event when there were no prior images) aren't independently visible in a screenshot.
Generated by Claude Code