Skip to content

Add setting for opting in to beta features#11123

Open
ds-boyce wants to merge 5 commits into
mainfrom
d/app-2525-beta-features
Open

Add setting for opting in to beta features#11123
ds-boyce wants to merge 5 commits into
mainfrom
d/app-2525-beta-features

Conversation

@ds-boyce

@ds-boyce ds-boyce commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Introduces a toggle for a new account-level preference for opting in to beta features.

Note

At the moment there are no beta features available.

Example of a beta feature:

beta.mov

With no features available:

none.mov

Feedback is collected via Sentry:

feedback.mov

@linear-code

linear-code Bot commented Jul 9, 2026

Copy link
Copy Markdown

APP-2525

APP-2577

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor
Old size New size Diff
7.75 MB 7.75 MB -90 B (-0.00%)

@ds-boyce ds-boyce marked this pull request as ready for review July 9, 2026 20:18
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @ds-boyce's task in 3m 9s —— View job


Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against origin/main
  • Review new screens/components, state, analytics, storage
  • Post review

Reviewed against the guidance in .github/claude-review-prompt.md. This is a well-constructed PR — the tricky bits (per-account scoping of the cached isBetaUser flag, the useSyncExternalStore-over-useStorage choice in the analytics context, the init promise refactor, and the Sentry-disabled guard in the feedback mutation) are all handled carefully and the reasoning is documented inline. I found one small defect and a couple of non-blocking notes.

Findings

1. Typo in accessibility label — non-blocking  ·  src/screens/Settings/components/BetaFeaturesFeedbackDialog.tsx:111

The other plural branch reads '# cahracters remaining'. Any screen-reader user on the character counter (i.e. remaining !== 1, which is the common case) hears the misspelling. Left an inline suggestion. Fix this →

Notes (not blocking, just flagging)

  • BetaFeaturesSettings.tsx:75-77 — after a successful toggle you account.set([did, 'isBetaUser'], next) directly, and BetaUserStorageSync will also write the same value once invalidateQueries refetches preferences. That's intentional redundancy (its guard skips the second write when the value already matches), so no re-render storm — just noting it's belt-and-suspenders by design.

  • BetaFeaturesFeedbackDialog.tsx:65 — feedback attaches currentAccount?.email to the Sentry report. Assuming that's the intended contact channel for beta feedback; worth a mental confirmation that sending user email to Sentry is acceptable per your privacy posture.

  • Toggle latencyvalue={isBetaUser} is driven by the preferences query, so the switch only visually flips after setIsBetaUser resolves and preferences refetch (the toggle is disabled while isPending). That's correct/non-optimistic behavior; just calling out the brief delay is expected, not a bug.

Things I checked that are fine: all user-facing strings go through Lingui (l/Trans) with sensible context/comment; ALF is used throughout (no legacy StyleSheet/#/view/com); the useSetIsBetaUserMutation invalidates preferencesQueryKey so cache stays consistent; error paths route to toasts and log via logger.error with safeMessage; and the per-account storage scoping avoids the cross-account leak the comments describe. The init refactor (removing the async promise-executor anti-pattern) preserves resolve-on-failure semantics.
· branch d/app-2525-beta-features

Comment thread src/screens/Settings/components/BetaFeaturesFeedbackDialog.tsx Outdated
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Claude finished @ds-boyce's task in 2m 57s —— View job


Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against the PR base
  • Review new screens/components, state, analytics, storage
  • Post review

Re-reviewed after the Feedback commit (beebbaa). Both items from the prior pass are now resolved:

  • Typo fixedBetaFeaturesFeedbackDialog.tsx:108 now reads '# characters remaining' in the other plural branch.
  • Email removed — the Sentry captureFeedback call no longer attaches currentAccount?.email, so the privacy note from last time no longer applies.

I re-checked the tricky parts and they hold up:

  • Per-account isBetaUser scopinguseAccountIsBetaUser (analytics/index.tsx:142) reading via useSyncExternalStore with a did-keyed getSnapshot/subscribe correctly avoids the previous-account leak that plain useStorage would cause on the render-not-remount account switch. getSnapshot returns a primitive (boolean | undefined), so no Object.is tearing/loop.
  • In-session gate refresh — toggling writes account.set([did, 'isBetaUser']), which fires the listener → AnalyticsContext re-renders with new isBetaUserAnalyticsFeaturesContext calls setAttributes with base.isBetaUser. BetaUserStorageSync's equality guard (beta-user-sync.tsx:32) prevents a redundant second write. Consistent.
  • Feedback mutation — the Sentry.getClient()?.getOptions().enabled guard correctly routes the disabled-client no-op to the error toast instead of a false success, and control.close(() => …) is used before the state reset + toast per the dialog-close footgun.
  • Condition tree walkconditionTargetsAttribute recurses through $and/$or/$nor/$not and handles dot-notation, so nested beta-targeting rules are detected.
  • i18n / ALF — all user-facing strings go through l/Trans with context on the platform-specific admonition copy; ALF throughout, no legacy StyleSheet/#/view/com. The AccountSettings.tsx refactor to @lingui/react/macro's l is a clean, behavior-preserving swap.

Findings

No blocking findings.

Non-blocking notes (not new — just flagging for the record):

  • Toggle latencyvalue={isBetaUser} is preferences-driven and the toggle is disabled while isPending, so the switch flips only after the mutation resolves and preferences refetch. Correct, non-optimistic behavior; the brief delay is expected.
  • getFeatureDescription coverage — only Features.AdvancedSearchV2Enable maps to a localized name today, so any other beta-targeted gate would pass hasTargeting but get dropped by the if (featureName) null check in getTargetedFeatures (analytics/features/index.ts:148) and silently not appear in the list. That matches the enum docstring you added ("be sure to add a localized description via getFeatureDescription()"), so it's a documented contributor contract rather than a bug — just noting the failure mode is a silent omission, not a crash.

Note

I couldn't run pnpm typecheck/pnpm test in this environment (dependencies aren't installed on the runner). The @atproto/api bump to 0.20.27 is what backs agent.setIsBetaUser and bskyAppState.isBetaUser; I verified the call sites but not the published types. CI should confirm.

Nice, careful PR — the concurrency/scoping reasoning is well documented inline.
· branch d/app-2525-beta-features

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Claude finished @ds-boyce's task in 14m 40s —— View job


Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against origin/main
  • Review new screens/components, state, analytics, storage
  • Post review

Re-reviewed the full diff on d/app-2525-beta-features. The beta-features implementation itself continues to hold up well — the per-account isBetaUser scoping (useAccountIsBetaUser via useSyncExternalStore), the init promise refactor, the Sentry-disabled guard in the feedback mutation, the condition-tree walk, and the i18n/ALF usage are all solid, and prior findings (the cahracters typo, the email-to-Sentry) are resolved. Two things stand out this pass that weren't flagged before.

Findings

1. eslint-suppressions.json (+2262 lines) looks accidentally committed — non-blocking  ·  eslint-suppressions.json

This file is new on the branch and unrelated to beta features. The repo migrated to Oxlint (fa27d44), and lint is oxlint --quiet src modules driven by oxlint-suppressions.json (which uses typescript/… rule keys). The new file uses the old @typescript-eslint/… ESLint keys and is not referenced by any config, script, .oxlintrc.json, package.json, or CI workflow — the only remaining ESLint setup is bskyembed/eslint.config.mjs, which has its own scope. So this appears to be a stray artifact (likely a pre-migration eslint --suppress dump) that got committed. Recommend removing it to keep the PR's blast radius to the feature. Remove it →

2. getMetadataForLogger return type anyunknown may break pnpm typecheck — verify in CI  ·  src/analytics/metadata.ts:70, consumed at src/analytics/index.tsx:207

The return type changed from Record<string, any> to Record<string, unknown> (to clear the no-explicit-any suppression). That value is passed straight into createLogger(context, metadata), whose second parameter is typed Partial<Metadata> (index.tsx:76). Under strict, Record<string, unknown> is generally not assignable to Partial<Metadata> — the target's base?: BaseMetadata would receive unknown, which isn't assignable to BaseMetadata | undefined. With the old any this always passed; unknown removes that escape hatch.

I couldn't run pnpm typecheck in this environment (dependencies aren't installed and the toolchain is sandboxed), so I can't confirm whether this actually errors — it's possible some inference path saves it. If CI typecheck is green, disregard this. If it's red, the minimal fix is to keep the returned object but annotate the return type as Partial<Metadata> (or a purpose-built subset type) rather than Record<string, unknown>, so the createLogger call stays well-typed. Look at this →

Things I checked that are fine

  • Per-account isBetaUser scopinguseAccountIsBetaUser reads via useSyncExternalStore with a did-keyed getSnapshot/subscribe, correctly avoiding the previous-account leak that plain useStorage would cause on the render-not-remount account switch. getSnapshot returns a primitive so no tearing loop.
  • In-session gate refresh — the toggle writes account.set([did, 'isBetaUser']); BetaUserStorageSync's equality guard (beta-user-sync.tsx:32) prevents a redundant second write when preferences refetch. Consistent.
  • Feedback mutation — the Sentry.getClient()?.getOptions().enabled guard routes the disabled-client no-op to the error toast instead of a false success, and control.close(() => …) is used before the state reset + toast per the dialog-close footgun.
  • i18n / ALF — all user-facing strings go through l/Trans with context on the platform-specific admonition copy; ALF throughout, no legacy StyleSheet/#/view/com. The AccountSettings.tsx swap to @lingui/react/macro's l is clean and behavior-preserving.
  • Mutation cache consistencyuseSetIsBetaUserMutation invalidates preferencesQueryKey; navigation route + types + router path all wired.

Non-blocking note (unchanged from prior passes): getFeatureDescription only maps Features.AdvancedSearchV2Enable today, so any other beta-targeted gate would pass hasTargeting but be dropped by the if (featureName) null check in getTargetedFeatures and silently omitted from the list. That matches the enum docstring's contributor contract, so it's a documented failure mode (silent omission, not a crash), not a bug.
· branch d/app-2525-beta-features

@arcalinea arcalinea temporarily deployed to d/app-2525-beta-features - social-app PR #11123 July 10, 2026 19:51 — with Render Destroyed
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against origin/main
  • Review new/changed files since last pass
  • Post review

View job run

@arcalinea arcalinea temporarily deployed to d/app-2525-beta-features - social-app PR #11123 July 10, 2026 19:53 — with Render Destroyed
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against origin/main
  • Review new/changed files since last pass
  • Post review

View job run

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Claude finished @ds-boyce's task in 5m 15s —— View job


Review: Beta features opt-in setting

  • Read review guidance (.github/claude-review-prompt.md)
  • Analyze the diff against origin/main
  • Review new/changed files since last pass
  • Post review

Re-reviewed the full diff on d/app-2525-beta-features. No blocking findings, and this pass clears the two items that were still open last time:

  • Stray eslint-suppressions.json is gone. The +2262-line artifact flagged in the previous pass is no longer on the branch. oxlint-suppressions.json now only removes the two obsolete entries (src/analytics/features/index.ts, src/analytics/metadata.ts) that this PR's refactors made unnecessary. Clean.
  • getMetadataForLoggerRecord<string, unknown> typechecks. The concern last time was whether the anyunknown return type would still be assignable to createLogger(context, metadata: Partial<Metadata>) at src/analytics/index.tsx:207. CI is green — the PR Tests job (which runs pnpm typecheck) passed, so this is confirmed fine, not just plausibly fine. (Mechanically it holds because createLogger only ever destructures the object into Logger.create; nothing narrows it back to BaseMetadata.)

CI status: Lint, Lockfile, PR Tests, golang, and the aggregate PR check are all ✅.

Things I re-verified and they hold up

  • Per-account isBetaUser scopinguseAccountIsBetaUser (analytics/index.tsx:142) reads via useSyncExternalStore with a did-keyed getSnapshot/subscribe. This correctly avoids the previous-account leak that plain useStorage would cause on the render-not-remount account switch (the AnalyticsContext sits above the <Fragment key={did}> breaker). getSnapshot returns a primitive (boolean | undefined), so no Object.is tearing loop.
  • In-session gate refresh + write dedup — the toggle writes account.set([did, 'isBetaUser'], next) (BetaFeaturesSettings.tsx:76) and BetaUserStorageSync's equality guard (beta-user-sync.tsx:32) skips the redundant second write when preferences refetch. No re-render storm. Belt-and-suspenders by design.
  • Feedback mutation — the Sentry.getClient()?.getOptions().enabled guard (BetaFeaturesFeedbackDialog.tsx:56) routes the disabled-client no-op to the error toast instead of a false success, and control.close(() => …) wraps the post-close state reset + toast per the dialog-close footgun. cahracters typo and the email-to-Sentry privacy note from earlier passes remain resolved.
  • Condition tree walkconditionTargetsAttribute (analytics/features/index.ts:110) recurses through $and/$or/$nor/$not and handles dot-notation, so nested beta-targeting rules are detected rather than missed by a flat top-level scan.
  • Cache consistencyuseSetIsBetaUserMutation invalidates preferencesQueryKey; DEFAULT_LOGGED_OUT_PREFERENCES and the Account storage schema both gained isBetaUser, and the route/types/nav wiring is complete.
  • i18n / ALF — all user-facing strings go through l/Trans, with context: 'web' | 'native' disambiguating the reload-vs-restart admonition copy. ALF throughout, no legacy StyleSheet/#/view/com. The AccountSettings.tsx swap from _(msg`…`) to @lingui/react/macro's l is clean and behavior-preserving, and matches the CLAUDE.md preference.

Non-blocking note (unchanged, documented contract — not a bug): getFeatureDescription (analytics/features/index.ts:78) currently maps nothing (the sole example case is commented out), so any beta-targeted gate passes hasTargeting but is dropped by the if (featureName) null check in getTargetedFeatures and silently omitted from the "Current beta features" list. That matches the Features enum docstring's contributor contract ("be sure to add a localized description via getFeatureDescription()") — the failure mode is a silent omission, not a crash. Worth keeping in mind when the first real beta gate lands.

Nice, careful PR — the concurrency/scoping reasoning is well documented inline, and both prior findings are now cleanly resolved with green CI.
· branch d/app-2525-beta-features

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