- Notifications
You must be signed in to change notification settings - Fork1.1k
fix(site): add defensive access to entitlement features#21381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
When entitlement features like user_limit might not exist in theresponse, using optional chaining prevents runtime errors.This fixes the License page crash when user_limit feature is notpresent in the entitlements response.Fixes#14784
Verifies that LicensesSettingsPageView renders correctly whenuserLimit feature values are undefined.
Verifies that the page renders correctly when the entitlementsresponse does not include user_limit feature.
60baf2e tod634893Comparedbf5a3e to45a8253CompareDanielleMaywood commentedDec 23, 2025
Is there a reason for using jest over storybook? |
mtojek commentedDec 23, 2025
Yes, there is. The culprit isLicensesSettingsPage.tsx, which is before going into PageView. Storybooks operate on PageView components. |
DanielleMaywood commentedDec 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We have a couple of options then:
|
| userLimitActual={entitlementsQuery.data?.features.user_limit?.actual} | ||
| userLimitLimit={entitlementsQuery.data?.features.user_limit?.limit} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
features: Record<FeatureName, Feature>;, technically any index access should use the?. notation (no guarantees on the existence). There's even atsconfig.json config for this:https://www.typescriptlang.org/tsconfig/#noUncheckedIndexedAccess
This issue is that if this is enabled it might require a humongous amount of changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TIL!
I understand that it is something we definitely don't want to enable without wider refactor 😅 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I do not have a workingsite rn but if you add the option:
"compilerOptions": {"noUncheckedIndexedAccess":true }
How many failures are there when compiling? This is def beyond the scope of this PR though 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I involved Claude & mux in this exercise. Here are the results:
Total: 257 errors, 89 files
| TS2532 | 86 | Object is possibly 'undefined' |
|---|---|---|
| TS2345 | 62 | Argument of type 'X | undefined' is not assignable to... |
| TS2322 | 56 | Type 'X | undefined' is not assignable to type... |
| TS18048 | 46 | 'X' is possibly 'undefined' |
| Others | 7 | (TS2538, TS2339, TS2769, TS2488) |
I asked to create a draft for better picture (clauding now), but we would need more eyes on this anyway 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here is the first draft:#21383
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hahahaha I love how the way it solves this is by asserting (and thus breaking at run time anyway). 145 lines is not bad though, I'm guessing in some cases we can assert and in others we just "handle" the undefined
… user_limitAdded LicensesSettingsPage.stories.tsx with WithoutUserLimitFeature storythat tests the page component renders correctly when entitlements.featuresdoes not include user_limit.
2181a02 to5e0c9f4Compare- LicensesSettingsPage.stories.tsx: WithoutUserLimitFeature story- TemplateInsightsPage.stories.tsx: Added entitlements with user_limit to Loaded story, and WithoutUserLimitFeature story with undefined entitlements
The userLimit prop in ActiveUsersPanel was never used - it was passedthrough but not rendered. This removes:- entitlements query from TemplateInsightsPage- entitlements prop from TemplateInsightsPageView- userLimit prop from ActiveUsersPanel- Associated unused imports
mtojek left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@DanielleMaywood Thanks for challenging. I switched to Storybook and actually discovered unused code in template insights (we don't draw the limit line anymore).
| userLimitActual={entitlementsQuery.data?.features.user_limit?.actual} | ||
| userLimitLimit={entitlementsQuery.data?.features.user_limit?.limit} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TIL!
I understand that it is something we definitely don't want to enable without wider refactor 😅 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, I'm not too familiar with Storybook though 😅
| userLimit={ | ||
| entitlements?.features.user_limit.enabled | ||
| ?entitlements?.features.user_limit.limit | ||
| :undefined | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was this just passed tonothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Passed andunused 👍
| userLimitActual={entitlementsQuery.data?.features.user_limit?.actual} | ||
| userLimitLimit={entitlementsQuery.data?.features.user_limit?.limit} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So I do not have a workingsite rn but if you add the option:
"compilerOptions": {"noUncheckedIndexedAccess":true }
How many failures are there when compiling? This is def beyond the scope of this PR though 😓
923c04e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#14784
Summary
This PR fixes a runtime crash on the License Settings page when the entitlements response doesn't contain the
user_limitfeature, and removes unused code from TemplateInsightsPage.Changes
1. Fix: Defensive access to entitlement features (LicensesSettingsPage.tsx)
When the entitlements response doesn't contain certain feature keys (like
user_limit), the License Settings page crashes with a runtime error because the code assumesfeatures.user_limitalways exists.Solution: Use optional chaining (
?.) to safely access feature properties:The View component already handles
undefinedvalues gracefully through its optional prop types.2. Test: Storybook story for the fix
Added
LicensesSettingsPage.stories.tsxwithWithoutUserLimitFeaturestory that tests the page renders correctly when entitlements don't includeuser_limit.3. Refactor: Remove unused code from TemplateInsightsPage
The
userLimitprop inActiveUsersPanelwas passed through but never actually used in the render. This removes:entitlementsquery fromTemplateInsightsPageentitlementsprop fromTemplateInsightsPageViewuserLimitprop fromActiveUsersPanelentitlements,Entitlements,useEmbeddedMetadata)