Skip to content

chore: codex/ts strict phase3#5493

Open
rebelchris wants to merge 3 commits intomainfrom
codex/ts-strict-phase3
Open

chore: codex/ts strict phase3#5493
rebelchris wants to merge 3 commits intomainfrom
codex/ts-strict-phase3

Conversation

@rebelchris
Copy link
Contributor

@rebelchris rebelchris commented Feb 16, 2026

Changes

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Preview domain

https://codex-ts-strict-phase3.preview.app.daily.dev

@rebelchris rebelchris requested a review from a team as a code owner February 16, 2026 08:23
@vercel
Copy link

vercel bot commented Feb 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
daily-webapp Ready Ready Preview Feb 17, 2026 11:22am
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
storybook Ignored Ignored Feb 17, 2026 11:22am

Request Review

const [flow, setFlow] = useState('');
const [activeDisplay, setActiveDisplay] = useState(() =>
storage.getItem(SIGNIN_METHOD_KEY) && !forceDefaultDisplay
storage.getItem?.(SIGNIN_METHOD_KEY) && !forceDefaultDisplay
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is getItem undefined?

throw new Error('Missing refetchBoot in AuthOptionsInner');
}

return refetchBoot();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refetchBoot should be required no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's fine to throw no?

Comment on lines +387 to +391
if (!onSocialRegistration) {
throw new Error(
'Missing social registration handler in AuthOptionsInner',
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be optional in some cases, so it should not throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this one should be able to be optional

Comment on lines +491 to +495
if (!provider) {
throw new Error(
'Missing social provider result in AuthOptionsInner',
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it just throws when anything is missing, its not always a good thing to do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you prefer in these cases?
Otherwise user silently gets error no?

@rebelchris
Copy link
Contributor Author

@claude see feedback

@claude
Copy link
Contributor

claude bot commented Feb 16, 2026

Claude finished @rebelchris's task in 1m 24s —— View job


Reviewing PR Feedback

I've analyzed the TypeScript strict mode changes and the reviewer's feedback. Here's my assessment of each concern:

1. Line 144: "when is getItem undefined?" ✅ Valid concern

storage.getItem?.(SIGNIN_METHOD_KEY) && !forceDefaultDisplay

Context: storage is storageWrapper which provides a safe wrapper around localStorage/sessionStorage. The optional chaining (?.) is used here because:

  • In server-side rendering (SSR) contexts, storage methods might not be available
  • The storage wrapper can return undefined methods in non-browser environments
  • This is a defensive pattern used throughout the codebase

Verdict: This change is correct and necessary for strict mode. The optional chaining prevents runtime errors in SSR contexts.


2. Line 173: "refetchBoot should be required no?" ⚠️ Architecture question

const refetchBootOrThrow = async () => {
  if (!refetchBoot) {
    throw new Error('Missing refetchBoot in AuthOptionsInner');
  }
  return refetchBoot();
};

Context: refetchBoot comes from useAuthContext() and is typed as optional. The PR introduces a helper that throws when it's missing.

Analysis:

  • Current state: refetchBoot is optional in AuthContext type definition
  • This PR's approach: Fails fast with clear error when an internal invariant is violated (aligns with CLAUDE.md guidance)
  • Alternative: Make refetchBoot required in the type definition

Recommendation: This follows the project's "fail fast" principle from CLAUDE.md. However, if refetchBoot is always expected to exist in AuthOptionsInner, consider making it required at the context level rather than throwing at runtime. This would catch issues at compile time instead.


3. Line 391: "this could be optional in some cases, so it should not throw?" ❌ Disagrees with strict approach

if (!onSocialRegistration) {
  throw new Error('Missing social registration handler in AuthOptionsInner');
}
await onSocialRegistration(provider);

Context: This is inside onProviderClick when a user clicks a social auth provider. The callback is from useRegistration hook.

Analysis:

  • If onSocialRegistration is missing here, the auth flow is fundamentally broken
  • Silently failing would leave users in a broken state
  • This is an internal invariant - if this fails, it's a programming error, not a user error

Verdict: The throw is correct here per CLAUDE.md's guidance: "Do not silently ignore impossible states" and "Fail fast with a clear thrown error message when an internal invariant is violated." If the social registration handler is missing, the feature cannot work.


4. Line 495: "I see it just throws when anything is missing, its not always a good thing to do" ⚠️ Valid broader concern

const provider = result[0];
if (!provider) {
  throw new Error('Missing social provider result in AuthOptionsInner');
}

Context: This is after calling getKratosProviders() in the error handling flow for social auth.

Analysis: The PR adds 4 similar throw statements:

  1. refetchBoot missing (line 170)
  2. Social auth popup missing (line 227)
  3. onSocialRegistration missing (line 388)
  4. Social provider result missing (line 492)

Philosophical consideration:

  • Pros: Clear error messages, fail fast, no silent failures, easier debugging
  • ⚠️ Cons: Less graceful degradation, could crash the auth flow for edge cases

Recommendation: This approach aligns with the codebase's philosophy (from CLAUDE.md: "Fail fast with a clear thrown error message when an internal invariant is violated"). However, consider:

  • Are these truly impossible states (programming errors) or could they happen in production due to race conditions, network issues, or edge cases?
  • If they're programming errors → throwing is correct
  • If they're possible runtime errors → should handle gracefully with user-facing error messages

Summary

The TypeScript strict mode changes are generally sound and follow the project's "fail fast" principle. However, there's a valid architectural question: Are we treating runtime errors as invariant violations?

Suggested approach:

  1. ✅ Keep the throws for truly impossible states (programming errors)
  2. ⚠️ Consider if any of these could be legitimate runtime errors that need graceful handling
  3. 💡 Consider making refetchBoot required at the type level if it's always needed

The changes make the code more type-safe and fail-fast, which is good for catching bugs early. The trade-off is less graceful degradation in unexpected scenarios.


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.

2 participants