feat/discovery

Comment slightly understates `eligibleKeys` behavior on stale-cache first render

apps/platform/src/components/discovery/FeatureSpotlight.tsx

Comment says 'During the very first load eligibleKeys is empty, so the card waits one tick for the response.' With `keepPreviousData`, the prior query result (a different key) could populate `eligibleKeys` with stale data before the fresh response for the new `requested` key arrives. A key eligible under the stale context could render briefly. Not a regression over the old design (same TanStack Query behavior), but the comment is a slight overstatement. Low impact in practice.

`CoachmarkProps` allows `revisit` and `pulled` simultaneously (type doesn't enforce mutual exclusion)

apps/platform/src/components/discovery/FeatureSpotlight.tsx

No call site passes both today, and the semantics are clearly distinct. Documented as a potential footgun if a future caller passes both: `stripSummonParam` would delete both params, which may or may not be intended. Low risk, no action needed now.

Pull card React `key` doesn't include `orgId` — potential stale instance on org switch

apps/platform/src/components/discovery/FeatureSpotlight.tsx

key={`pull:${page}:${pulledKey}`} — omits `orgId`. If `orgId` changes without a pathname change (same page, org context update), the pull coachmark won't remount. In practice org switches trigger full navigation, so this is a theoretical edge case only.

Double-gate on URL param value is correct and sufficient

apps/platform/src/components/discovery/FeatureSpotlight.tsx

The `requested in SPOTLIGHT_CTA` catalog membership check plus `eligibleKeys.includes(pulledKey)` server validation means the param value never reaches JSX as a raw string and is never used in href/src/dangerouslySetInnerHTML. No XSS or injection vector. `router.replace()` in Next.js App Router cannot navigate to external origins. Clean.

Render-time `setPick` is valid React derived-state pattern

apps/platform/src/components/discovery/FeatureSpotlight.tsx

Calling `setPick` during render (not in useEffect/handler) with a guard (`nextPick.mountId !== pick.mountId || nextPick.key !== pick.key`) is the documented React pattern for synchronizing derived state. React re-renders synchronously before painting, avoiding tearing. The guard prevents infinite loops. Acceptable.

New param-driven pull path has no unit test coverage

apps/platform/src/components/discovery/FeatureSpotlight.tsx

The old pull mechanism had dedicated tests: `shouldConsumePullParam` (4 cases) + origin-latch tests in `nextSpotlightPick`. All are correctly deleted because that code is gone. But the new pull path — 'param present → eligible → render; stripped only on dismiss' — has zero unit coverage. The key invariant (never strip on load, only on dismiss) is unguarded. Because `FeatureSpotlight` is a React component, coverage likely needs RTL render tests. The logic is simple, but the prior blink-bug shows this area bites in subtle ways.

Deleted `shouldConsumePullParam` tests: risk is eliminated by design, not just untested

apps/platform/src/components/discovery/__tests__/spotlight-latch.test.ts

The tests lens flagged these deletions as leaving the blink-bug unguarded. Clarification: the new design makes the blink-bug structurally impossible — the param is NEVER stripped on load (no `useEffect`, no `router.replace` on mount). The strip only happens in `stripSummonParam` on user dismiss. So the risk isn't moved to a guard-free path; it's designed out. Deletions are correct. Flagging for visibility only.

`stripSummonParam` merges two orthogonal summon-param concerns

apps/platform/src/components/discovery/FeatureSpotlight.tsx

The callback handles both `?revisit` and `?descubre` deletion. For two cases this is readable. If a third summon mechanism is added, this will become a multi-branch conditional. Two narrow callbacks (`stripRevisitParam`, `stripPullParam`) would be more self-documenting and independently testable. Not a blocker now.