PROD-10332 - #5036
Conversation
…ng until an option is toggled - Persist type, membership sub-type, and cleared sub-type changes immediately in AccessControlField by firing onChange with cleared options, instead of only saving when a plan/role toggle changes. Previously a provider or type switch never saved and the prior provider's rule kept enforcing silently. - Rebuild settings app bundle.
…ed panel re-entry - Add a mount-only self-heal in AccessControlField: when the cached panel payload's enrichment type disagrees with the saved value's type (stale cache from out-of-order save responses during rapid selection changes), refetch the options list for the saved selection instead of rendering the wrong type's options. - Rebuild settings app bundle.
…ication - Tag each debounced settings save with a monotonic sequence and only let the newest dispatched save's response apply its echo to the settings state and feature cache. With two saves in flight (rapid consecutive changes), an earlier response arriving last could overwrite the newer state client-side (e.g. a type-only access control payload clobbering the type+provider payload saved after it). Stale responses are skipped; every dispatch already carries the cumulative latest field values. - Rebuild settings app bundle.
…h aborts - Extend the mount self-heal to also compare the enrichment sub-type, so a provider-only switch (same type, different provider) no longer renders the previous provider's options from a stale cached panel payload. - Serialize settings saves into a single-flight channel: while a request is in flight, newer payloads queue and dispatch when it settles, so neither responses nor server-side option writes can apply out of order; the sequence guard remains as defense-in-depth. - Abort any in-flight options fetch in the clear-type, grouped-type and clear-sub-type branches so a late response cannot repaint the previous selection's options. - Align the heal's bb.accessControl.options filter args with the handlers. - Rebuild settings app bundle.
- Move the in-flight flag and pending-payload queue from component-scope refs into the featureId-keyed effect closure. With shared refs, a save still in flight when the admin navigated to another feature caused the new feature's queued payload to be dispatched by the old feature's settle handler under the old feature_id, silently dropping the change. With closure-local channel state, each feature drains its own queue with its own feature_id and a newly-entered feature starts unblocked. - Rebuild settings app bundle.
- Move the "Settings saved." toast after the save-sequence staleness check so a superseded response no longer flashes a redundant toast; the newer save shows its own on arrival. Error toasts stay unconditional since a failed save deserves reporting regardless. - Document that the debounce cleanup guard is intentionally a no-op with the custom debounce (no .cancel), so trailing timers fire post-switch with the correct feature_id — and should be flushed, not cancelled, if a cancellable debounce is ever introduced. - Rebuild settings app bundle.
…ment - Treat a present-but-empty saved value as explicitly cleared in the AccessControlField mount initializers: render the placeholder instead of falling back to the cached panel enrichment's current type and sub-type, which are stale after the clear on SPA re-entry and made a removed rule appear still configured. The enrichment fallback now applies only when no saved value exists at all. - Rebuild settings app bundle.
…ponses - Hoist the feature-cache reconciliation and bbAdminDataUpdates merge above the save-sequence guard. A response superseded by another feature's save was previously discarded entirely, leaving the original feature's cache holding pre-edit values so its next cache-first render showed the edit as lost despite a successful DB save. Same-feature ordering is already guaranteed by the per-feature single-flight channel, so the echo is always the newest for its own feature; the sequence guard now protects only the displayed screen state. - Rebuild settings app bundle.
…lizers - Replace the unreachable null/undefined check in the AccessControlField type and sub-type initializers with an object-shape check: an object value without a type means the rule was explicitly cleared through this UI (the save echo caches the submitted object), while the string default of a never-saved field falls back to the server enrichment, which is computed from the same stored option and always consistent on fresh data. - Rebuild settings app bundle.
…ature - Track the currently displayed feature in a ref and extend the save response guard: apply screen state and toast only when the response is neither superseded nor from a feature the admin has navigated away from. Without a save on the new feature the sequence counter never advances, so the old feature's response previously merged its fields and toast over the newly displayed screen (the reactions path could even replace the new feature's panels). Cache reconciliation and bbAdminDataUpdates still apply unconditionally — they are keyed to the response's own feature. Pre-existing gap; last member of this race class. - Rebuild settings app bundle.
- Move the single-flight channel state from effect-closure locals into a ref map keyed by featureId. Re-entering a feature while its previous save was still in flight created a fresh channel unaware of that request, allowing two concurrent same-feature saves whose responses and DB writes could apply out of order. Reusing the feature's channel queues the next edit behind the in-flight request, keeping same-feature saves strictly serialized across navigation. Channels stay per-feature, so queues are only drained by their own feature's closures and one feature's slow save never blocks another. - Rebuild settings app bundle.
- Move the monotonic save sequence from a global ref onto the per-feature channel object. With per-feature serialization and the displayed-feature guard in place, the global counter's only remaining effect was false positives: another feature dispatching a save while feature A's response was in flight marked A's own correctly-ordered response as superseded, silently skipping its toast and settings echo. Scoped per channel, "superseded" can only mean a newer save on the same feature, making the guard defense-in-depth for the serialization invariant. - Rebuild settings app bundle.
- Split applyReactionPostSave into unconditional cache reconciliation and predicate-gated screen application. Reactions was excluded from the generic pre-guard cache block, so a reactions save resolving after the admin navigated away (or superseded) skipped its cache write entirely, leaving the reactions cache serving pre-edit values on the next cache-first render. The screen predicate is re-evaluated after the reaction_items refetch resolves, and the migration branch now derives its cache write from the cached panels instead of the on-screen feature state, which may belong to another feature by the time it runs. - Rebuild settings app bundle.
…ns, panel and refetch ordering - Clear loading and fetch-error state beside the three abort calls: the aborted fetch's catch early-returns on AbortError, so the spinner stuck forever after switching type mid-fetch. - Resolve the saved sub-type key from the types config before the enrichment key: cached enrichment only carries current_sub_type_key when a sub-type was stored at fetch time, so the first provider save rendered a placeholder on re-entry and the next save erased the stored provider. - Initialize the options lists empty for an explicitly cleared value so the stale enrichment's toggle list cannot render under a placeholder. - Share one grouped-type predicate (items.length > 0) between the mount heal and both change handlers; Pro emits sub_types as an empty array for non-grouped cases, which is truthy in JS. - Hoist refresh_panels cache invalidation above the supersession guard and keep only the refetch event display-gated. - Gate the reactions reaction_items refetch cache write on a latest-save predicate (the refetch runs outside the single-flight channel) and swallow refetch failures; reset changedFields on feature switch. - Rebuild settings app bundle.
…seded - A reaction_items refetch superseded by a newer non-refetching save (e.g. a mode change) was discarded entirely, stranding the temporary react_key_ item IDs in the feature cache until a hard reload; before the supersession gate existed, the same refetch instead clobbered the newer save's cache effects with pre-save server state. A superseded refetch now overlays only settings.reaction_items onto the current cache, and a per-feature sequence ledger orders concurrent refetch writes so an older refetch resolving last cannot regress a newer one. - Reuse isGroupedTypeConfig() for the hasSubTypes render check. - Rebuild settings app bundle.
- Replace the module-level items-refetch sequence ledger with claim state stored on the per-feature channel object, exposed to the reactions post-save helper as an atomic claim callback. The module ledger outlived the channel: a full FeatureSettingsScreen remount reset the sequence counter to 1 while the ledger kept the previous session's high-water mark, so every items-refetch of the new session was silently rejected before any cache or screen write until the counter climbed past the stale value. Channel-scoped claim state shares the counter's exact lifetime, making that precondition unconstructible. - Rebuild settings app bundle.
…fetch races - Move the per-feature single-flight save channels to module scope so they share the lifetime of everything they order — in-flight requests, un-cancellable debounce timers, and the module feature cache. A per-mount ref handed a remounted screen a fresh channel while the old mount's save was still in flight, allowing two concurrent same-feature saves with arbitrary server commit order; channel.seq and the reactions refetch claim now stay monotonic per feature across full remounts. - Order the bb-admin-refetch-feature GET against the save channel: when a save dispatched or is in flight while the GET was in transit, apply the structural panel refresh but preserve current settings instead of full-replacing them with the GET's stale payload, which silently reverted the interleaved save on screen and in the cache. - Clear the toast on feature switch: a navigated-away save's 'Saving changes...' toast never auto-dismisses and has no dismiss control, so it persisted over the next feature once the displayed-feature guard correctly skipped its success toast. - Access control field: clear the option lists when an options fetch starts (type change, provider change, mount heal) so a failed fetch cannot leave the previous type's options rendered and toggleable under the new selection, where a toggle would persist another type's option keys into the rule; abort any in-flight options fetch on unmount. - Reactions items-save: invalidate the feature cache when the post-save refetch fails — that refetch is the items branch's only cache write and the next panel load is cache-first, so pre-save items would otherwise be served indefinitely. - Rebuild settings app bundle.
- Build all three save error paths (server failure, network catch, sync dispatch guard) through a shared helper that, when the failed save's feature is no longer the one on screen, includes that feature's name in the sticky error message. The toast stays deliberately ungated by the supersession/navigation guard — the failed payload's change tracking is already reset on feature switch, so this toast is the only remaining signal the save failed — but naming the source feature stops the error from being misread as belonging to the feature the admin is viewing. - Rebuild settings app bundle.
- Use null-prototype objects for the save-channel map and the feature data cache: the feature id comes from the URL tab param, so an id colliding with an Object.prototype key (e.g. "constructor") would read an inherited value as an existing channel and write channel state onto the global Object constructor. All access is bracket-style, so no consumer changes. - Gate the refresh_panels refetch event on a module-level live-feature slot set and cleared by the save effect, instead of the response closure's displayed-feature ref, whose dead-mount copy freezes at its last value and let a navigated-away feature's response trigger a spurious refetch of whichever feature's screen was currently mounted. - Document the deliberate retention of a dead mount's channel appliers (nulling them on unmount would throw before the unconditional cache reconciliation), replace review-internal jargon in the items-refetch claim docblock with the two concrete failure modes it prevents, and drop an unused catch binding. - Rebuild settings app bundle.
…screen When a reaction_items save was superseded (e.g. by a mode change dispatched before its items-refetch resolved), the refetch patched the feature cache with the real DB IDs but returned without updating the mounted screen, leaving the temporary react_key_ IDs on screen; a later edit/delete of that item then sent a fake ID to the server (duplicate row or silent no-op). shouldApplyScreen() can't gate this — it also requires the save to be the latest, false by definition when superseded — so add a displayed-only gate (isFeatureDisplayed, backed by the module-level live-feature tracker) and apply a narrow reaction_items-only merge to the screen when the feature is still displayed; a newer save still owns the rest of the screen state. Document why the items-refetch failure path invalidates the cache unconditionally (a fail-safe, where the ordering guard would strand temp IDs), and correct the stale refetch-listener comment. Rebuild settings app bundle.
…per-option values - Overlay-merge on the bb-admin-refetch-feature full-replace branch: apply the fresh server settings (so server-derived fields still land) but overlay any edit still pending in the 1s debounce onto the screen, so the refresh no longer momentarily reverts an in-progress edit. The interleave guard keys off channel.seq, which only advances at dispatch, so a still-debouncing edit was invisible to it. Cache and originalSettings stay server-truth (no unsaved draft is cached); the edit's own debounced save reconciles when it fires. Add changedFieldsRef so the feature-scoped listener reads the latest pending edits without a stale closure. - initPerOptionSettings now prefers the saved per-option value over the server enrichment (which travels in the feature cache and can be stale after a rapid change), matching the trust-saved-value rule already used for type/sub-type/options; falls back to the enrichment only when the saved value has no sub-key for the option. - Rebuild settings app bundle.
|
Claude finished @KartikSuthar's task in 4m 14s —— View job Review complete ✅Independent "ultracode" review of PROD-10332 against Todo list
New finding: reactions items-refetch still full-replaces
|
…-replacing settings The async refetch that follows a reaction_items save replaced the whole settings object on its latest-and-displayed branch, reverting any unrelated field the admin edited while the refetch was in flight (the edit sits in the 1s debounce, so channel.seq hasn't advanced and the branch's guards are still true). Merge only reaction_items — the real DB IDs the refetch exists to deliver — onto the live settings and originalSettings, matching the superseded and migration branches which already merge rather than replace. Commit 074cf69 fixed this class for the generic refetch handler but not this reactions path. Rebuild settings app bundle.
|
@claude Please review from scratch ultracode |
|
Claude finished @KartikSuthar's task in 3m 10s —— View job Review complete ✅Independent "ultracode" review of PROD-10332 against Todo list
Prior finding is now fixedThe previous review round flagged that Commit if ( undefined !== freshSettings.reaction_items ) {
context.setSettings((prev) => ({ ...prev, reaction_items: freshSettings.reaction_items }));
context.setOriginalSettings((prev) => ({ ...prev, reaction_items: freshSettings.reaction_items }));
}This is actually tighter than a plain Verified the fix is real, not just source-only:
Re-verified from scratch, confirmed sound
Still-open, non-blocking
Environment noteI did not run |
Jira Issue:
https://buddyboss.atlassian.net/browse/PROD-10332
General Note
Keep all conversations related to this PR in the associated Jira issue(s). Do NOT add comment on this PR or edit this PR’s description.
Notes to Developer
Notes to Reviewer