feat(locations): materializer allowlist picker with drift badges + saved-state banner#1007
feat(locations): materializer allowlist picker with drift badges + saved-state banner#1007caseylocker wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds a Dropbox Materializer allowlist flow with paginated fetching, stale-request protection, Redux state, reconciliation helpers, and a location-page selection panel. Sync-config GET/PUT flows now use guarded dispatch behavior, with expanded reducer, action, and UI tests. ChangesMaterializer Allowlist Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a “materialized media upload types” allowlist picker to the Locations → Synchronization Settings panel, backed by Redux state and a new paged fetch action for MediaUploadTypes, plus UI reconciliation badges and a saved-state banner.
Changes:
- Extends
dropboxSyncStateto storematerialized_media_upload_typesand allowlist option fetch state (options + error). - Implements
getAllMediaUploadTypesForAllowlistto fetch MediaUploadTypes (paged + concurrency-limited), guarded against stale/superseded invocations. - Adds the allowlist picker UI + reconciliation helpers to
location-list-page.js, with extensive unit and component tests plus i18n strings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/reducers/locations/dropbox-sync-reducer.js | Adds allowlist options sub-state and materialized_media_upload_types default to the dropbox sync reducer. |
| src/reducers/tests/dropbox-sync-reducer.test.js | Adds reducer tests covering the new allowlist actions and persisted allowlist field behavior. |
| src/pages/locations/location-list-page.js | Implements the allowlist picker UI, reconciliation logic, and saved-state banner on the Locations page. |
| src/pages/locations/tests/location-list-page-allowlist.test.js | Adds thorough unit/component tests for reconciliation cases, UI behavior, gating, and regression coverage. |
| src/i18n/en.json | Adds UI strings for allowlist section labels, badges, banner text, and error/retry copy. |
| src/actions/dropbox-sync-actions.js | Adds paged allowlist options fetch thunk with stale-invocation guards and global overlay loading. |
| src/actions/tests/dropbox-sync-actions.test.js | Adds action tests for paging, ordering, stale-guard behavior, loading bracketing, and error paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
caseylocker
force-pushed
the
feature/materializer-allowlist-picker
branch
from
f667a66 to
5060ed2
Compare
caseylocker
force-pushed
the
feature/materializer-allowlist-picker
branch
from
5060ed2 to
ba3cd40
Compare
…ved-state banner
Checkbox-group picker on the Locations sync panel selecting which MediaUploadTypes
participate in the Dropbox materializer, saved as {id, name} pairs via updateSyncConfig.
- paginate-to-completeness aggregator with two-tier staleness guard (seq+summit commits,
seq-only loading) and pLimit fan-out
- dropboxSyncState gains allowlistOptions + materialized_media_upload_types default
- reconciliation named exports per the SDS rename-handling decision table (cases 0-8),
drift badges, saved-state banner, error/Retry state, MUI primitives
- stable empty-array fallbacks so out-of-contract sync-config responses cannot crash the page
SDS: ftn-docsnsklz sds/dropbox-materializer-participating-types-allowlist.md (amended 2026-07-06)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
caseylocker
force-pushed
the
feature/materializer-allowlist-picker
branch
from
ba3cd40 to
96a59f6
Compare
| createAction("DUMMY"), | ||
| createAction("DUMMY"), | ||
| endpoint, | ||
| authErrorHandler |
There was a problem hiding this comment.
@caseylocker
This aggregator still passes authErrorHandler as the error handler for both getRequest calls (page 1 here, and the fan-out on line 130). The established pattern for MUI-styled features is snackbarErrorHandler (from base-actions.js) — a drop-in wrapper (authErrorHandler(err, res, setSnackbarMessage)), already used in 20+ action files including recent MUI additions (e.g. media-file-type-actions.js). It renders the same 401/403/404/412/default branches through a non-blocking MUI snackbar instead of a legacy SweetAlert modal — more consistent with this panel's Alert/Chip/FormGroup UI. Suggest swapping both authErrorHandler references (lines 105 and 130) for snackbarErrorHandler.
There was a problem hiding this comment.
Swapped in b69149b. Both aggregator getRequest calls now use snackbarErrorHandler, and the handler identity test pins it. The two tier staleness guard is unchanged since the wrapper routes through the same seq guarded dispatch. Also swept the category: getSyncConfig and updateSyncConfig, which this round rewrites, now use snackbarErrorHandler as well; rebuildSync and resyncRoom are untouched by this PR so they keep authErrorHandler for now.
| createAction("DUMMY"), | ||
| createAction("DUMMY"), | ||
| endpoint, | ||
| authErrorHandler |
There was a problem hiding this comment.
@caseylocker
Same as above — swap this second authErrorHandler (the fan-out page fetch) for snackbarErrorHandler too.
There was a problem hiding this comment.
Swapped in b69149b together with the page 1 call. Details in the thread above.
| props.getLocations(); | ||
| if (window.DROPBOX_MATERIALIZER_API_BASE_URL) { | ||
| props.getSyncConfig(); | ||
| props.getAllMediaUploadTypesForAllowlist(); |
There was a problem hiding this comment.
@caseylocker
Possible race condition: getSyncConfig() has no startLoading() of its own but unconditionally dispatches the global stopLoading() in its .then(), while getAllMediaUploadTypesForAllowlist() brackets the same global overlay with its own startLoading()/stopLoading(). Since baseState.loading is a plain boolean (not ref-counted), whichever of the two resolves first clears the overlay for both — if getSyncConfig wins, storedTypes is populated but options is still [], so every stored row briefly renders as Case 8 ("missing") with Save enabled. Chaining props.getSyncConfig().then(() => props.getAllMediaUploadTypesForAllowlist()) here shrinks that window to a single tick instead of the full multi-page fetch duration, without touching the global loading architecture.
There was a problem hiding this comment.
Fixed in b69149b with the chaining you suggested, plus one hardening a second review pass caught: getLocations shares the same non ref counted overlay and usually finishes while the aggregator is still fetching, so chaining the two sync calls alone still left the window open. The allowlistOptions slice now carries a loaded flag and the panel only renders reconciled rows and enables Save once the options have actually arrived, so the missing badge flash cannot happen regardless of overlay timing. Regression tests cover the chaining order and the not loaded gating.
| const optionsError = allowlistOptions?.error ?? null; | ||
|
|
||
| // Selection model — minimal local state, derived rendering (see SDS § State). | ||
| const [storedRows, setStoredRows] = useState([]); |
There was a problem hiding this comment.
@caseylocker
storedRows is fully derivable from storedTypes + options via reconcileAllowlist (a pure function), but it's stored in useState and synced through a useEffect. Every time storedTypes/options change (mount, post-Save, retry) React commits one render with the stale/previous storedRows before the effect fires and schedules the corrected one — an extra render pass plus a brief flash of stale reconciliation state. summit-admin-popup-dialog-pattern.md's "Derived State, Not Duplicate State" section covers exactly this: compute derived state on render, don't duplicate it in useState.
const storedRows = useMemo(
() => reconcileAllowlist(storedTypes, options).map((row, i) => ({ ...row, uiKey: `stored-${i}` })),
[storedTypes, options]
);Keep uncheckedKeys/removedKeys/pickedIds as useState (genuine local UI state) with their own reset effect on [storedTypes, options].
There was a problem hiding this comment.
Done in b69149b. storedRows is now a useMemo over storedTypes and options, and the three selection Sets stay in useState with their reset effect on the same deps, as you outlined.
|
|
||
| const DROPBOX_STORAGE = "DropBox"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
@caseylocker
This adds ~230 lines of framework-agnostic reconciliation/normalization logic (normalizeName, isValidEntry, RECONCILE_CASE, reconcileValidEntry, reconcileAllowlist) plus a full allowlist-picker state machine (4 useState, ~7 handlers, ~150 lines of JSX) directly inside LocationListPage, which already owns the unrelated locations CRUD table. Two independent extraction opportunities:
reconcileAllowlistis conceptually selector-shaped — a pure(storedTypes, options) => derived rowsfunction, the exact "result function" you'd hand toreselect'screateSelector. This repo has noreselect/selectors layer though (verified: not a dependency, zerocreateSelectorusage, zeroselectors.jsfiles anywhere), so introducing one now would be a new architectural pattern for a single caller.src/models/is where this codebase already puts exactly this shape of pure state-derivation logic without calling it a "selector" — seemodels/lead-report-settings.js'snormalizeLeadReportSettings/denormalizeLeadReportSettings. Suggest movingnormalizeName,isValidEntry,RECONCILE_CASE,reconcileValidEntry,reconcileAllowlistthere; named exports stay the same so the current unit tests import from the new path unchanged.- Panel UI + local state → its own child component (e.g.
AllowlistPanel), rendered fromLocationListPagewithsyncConfig/allowlistOptions/callbacks as props.
Not a correctness blocker — reuse-before-build.md permits inlining single-consumer logic — but the size/self-containment here matches the extraction precedent closely enough to be worth doing before it grows further.
There was a problem hiding this comment.
Done in b69149b. The five helpers moved to src/models/materializer-allowlist.js next to lead-report-settings, same named exports plus DROPBOX_STORAGE, and the picker UI with its selection state is now src/components/locations/allowlist-panel.js, rendered from the page with syncConfig, allowlistOptions, syncLoading and the save and retry callbacks as props. Test ids unchanged, suite green. Recorded as a dated SDS amendment since it supersedes the 2026-07-06 consolidation entry.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/locations/location-list-page.js`:
- Line 357: The Save Allowlist action is missing the shared sync-in-flight
guard, so it can fire overlapping update requests with stale selection data.
Update the save button disable logic in location-list-page’s saveDisabled path
(and any related save action wiring) to also check syncLoading, matching the
Sync toggle and Rebuild button behavior. Keep the change aligned with the
existing dropboxSyncState.loading-based guards so Save is blocked whenever a
sync/config fetch or update is already running.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e701c3d-d22a-4477-a2c8-70b42db90460
📒 Files selected for processing (7)
src/actions/__tests__/dropbox-sync-actions.test.jssrc/actions/dropbox-sync-actions.jssrc/i18n/en.jsonsrc/pages/locations/__tests__/location-list-page-allowlist.test.jssrc/pages/locations/location-list-page.jssrc/reducers/__tests__/dropbox-sync-reducer.test.jssrc/reducers/locations/dropbox-sync-reducer.js
…ing, guard config requests Review round (smarcet + CodeRabbit) on the allowlist picker: - Reconciliation helpers move to src/models/materializer-allowlist.js (exemplar: lead-report-settings) and the picker UI + selection state to src/components/locations/allowlist-panel.js; test ids unchanged. - Aggregator and config thunks use snackbarErrorHandler (MUI snackbar) instead of authErrorHandler. - Mount chains getSyncConfig before the options aggregator, and the panel gates rows/Save on a new allowlistOptions.loaded flag: stored rows are never reconciled (badged "missing") against an options list that merely has not arrived, regardless of global-overlay timing. - storedRows is derived via useMemo, not duplicated in useState. - Save/toggle/Rebuild are mutually exclusive with any in-flight config request: both config thunks dispatch REQUEST_SYNC_CONFIG synchronously before token acquisition, and share the aggregator-style two-tier sequence guard (isNewest/stillCurrent) so a stale response can neither commit cross-summit nor clear the newer request's loading flag; a new SYNC_CONFIG_ERROR clears loading on PUT failure without resetting the stored config; a superseded-but-successful PUT refetches so redux converges on the saved server state. 142 suites / 1255 tests green; eslint clean on changed files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
caseylocker
force-pushed
the
feature/materializer-allowlist-picker
branch
from
7535604 to
b69149b
Compare
ref: https://app.clickup.com/t/86b9y4k8t
What
Adds the materializer allowlist picker to the Locations page Synchronization Settings panel: a checkbox-group selecting which MediaUploadTypes participate in the Dropbox materializer, saved as
{id, name}pairs through the existingupdateSyncConfigPUT. Includes drift-badge reconciliation against the live type list (rename/recreate/storage-change/invalid cases 0–8 per the SDS decision table), a saved-state banner, and an inline error/Retry state.Spec:
ftn-docsnsklz/sds/dropbox-materializer-participating-types-allowlist.md(incl. the 2026-07-06 amendment consolidating plumbing into existing domain files, and the 2026-07-10 amendment recording the review-round changes below).Scope note (9 files, test-dominated, single concern)
Per
process/ai-assisted-development-workflow.md: 9 files changed — aggregator and config thunks indropbox-sync-actions.js, cache in the existingdropboxSyncStateslice, plus two new source files added at review request:src/models/materializer-allowlist.js(reconciliation/normalization helpers, exemplarmodels/lead-report-settings.js) andsrc/components/locations/allowlist-panel.js(the picker UI + selection state, rendered from the Locations page). The large majority of insertions are test code.Review round (second commit, b69149b)
Addresses all five review comments plus the CodeRabbit finding: model + panel extraction;
snackbarErrorHandleron the aggregator and both config thunks; mount fetches chained with the panel gating rows/Save on a newallowlistOptions.loadedflag (stored rows are never reconciled against an options list that has not arrived, regardless of global-overlay timing);storedRowsderived viauseMemo; Save/toggle/Rebuild mutually exclusive with any in-flight config request — both config thunks dispatchREQUEST_SYNC_CONFIGbefore token acquisition and share the aggregator-style sequence + summit guard, so a stale response can neither commit cross-summit nor clear the newer request's loading flag, a failed PUT clears loading via a newSYNC_CONFIG_ERRORwithout resetting the stored config, and a superseded-but-successful PUT refetches so redux converges on the saved server state.Ruling requested: casefold approximation (blocking question for reviewer)
SDS:618 requires the UI's name-normalization key-space to equal the worker's Python
strip().casefold(). JavaScript has no built-in casefold; this PR usestrim().toUpperCase().toLowerCase()— the closest zero-dependency approximation, exact for ASCII and realistic non-ASCII (ß, ligatures). It diverges on the dotless-ı class: JS foldsıtoi, Python keeps them distinct. Consequence in the worst case (a stored name containingıwhose live type now spells iti, same id): the UI classifies it Case 1 (no badge) and the Case-2 save-time rename repair never triggers, so the worker keeps skipping those uploads; worker-side diagnostics MAY surface it but are not guaranteed. The divergence is documented at the normalization site and pinned by a dedicated test.Options:
normalizeName's body (now insrc/models/materializer-allowlist.js) for a vendored Unicode fold table (single-owner design; nothing else changes).UX question: no UI path from one saved type back to zero (non-blocking, product call)
As specced (SDS:686), Save is disabled when the effective selection is empty, so an admin who has saved ≥1 type cannot shrink the allowlist to zero through this UI — the sanctioned off-path is the
dropbox_sync_enabledSynchronization toggle, and the helper text says so. Field-testing feedback: unchecking or removing the last type silently deactivates Save with no adjacent explanation, which reads as a broken button rather than a guarded contract. If this should change (e.g. tooltip on the disabled Save explaining the rule, or allowing an explicit empty save behind a confirm), it needs a product ruling + SDS amendment; the current implementation follows the spec as written. Suggestion: When the effective selection hits zero and there are stored/visible rows (i.e. the user just unchecked or removed the last type — not the pristine never-configured state), render a small Alert severity="info" above the Save button: "At least one type must stay selected. To stop file sync for this summit, turn off Synchronization above." Save stays disabled.Merge gating
Merge only AFTER dropbox-materializer PR #25 deploys — the picker saves a key (
materialized_media_upload_types) that the pre-#25 materializer ignores. Draft until then.Testing
skills/react-frontend.md§ summit-admin Conventions: global-overlay loading (no per-slice loading),Promise.resolve()thunk guards, constants fromutils/constants.js, MUI +sxfor the new section, single-brace i18n, no snapshots.dropbox-materializer/read writeon the show-admin client) untested locally; covered by the staging walk.Summary by CodeRabbit