fix(locations): send venue/room ids when resyncing a room to the materializer - #1026
Conversation
📝 WalkthroughWalkthroughThe room resync flow now uses integer venue and room IDs throughout ChangesSync action updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LocationForm
participant EditLocationPage
participant resyncRoom
participant MaterializerEndpoint
User->>LocationForm: Click room resync action
LocationForm->>EditLocationPage: venueId, roomId
EditLocationPage->>resyncRoom: venueId, roomId
resyncRoom->>MaterializerEndpoint: POST materializer URL
MaterializerEndpoint-->>resyncRoom: success or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Fixes the “Resync Room” action on the Locations page by aligning summit-admin’s request shape with the materializer’s updated route contract (integer venue/room IDs instead of names), preventing production 404s after the materializer’s destination-path restructure.
Changes:
- Updated the resync call chain (LocationForm → EditLocationPage → Redux action) to pass
venueId/roomIdinstead ofvenueName/roomName. - Updated the materializer URL construction to use integer IDs (and removed
encodeURIComponentname interpolation). - Added tests to pin the exact resync URL and verify the UI callback receives the correct IDs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/locations/edit-location-page.js | Updates handleRoomResync to forward venue/room IDs to the thunk. |
| src/components/forms/location-form.js | Sends persisted venue ID + room ID when invoking onRoomResync (no name lookup). |
| src/components/forms/tests/location-form.test.js | Adds UI test asserting the resync action calls onRoomResync(venueId, roomId). |
| src/actions/dropbox-sync-actions.js | Changes resyncRoom thunk signature and URL to /materialize/{summitId}/{venueId}/{roomId}/. |
| src/actions/tests/dropbox-sync-actions.test.js | Adds assertion pinning the constructed resync URL to the integer-ID route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rializer The Resync Room button sent venue and room NAMES. The materializer's per-room route stopped accepting names in dropbox-materializer PR #24 (merged 2026-06-15), which replaced sync/materialize/<int:summit_id>/<str:venue>/<str:room>/ with sync/materialize/<int:summit_id>/<int:venue_id>/<int:room_id>/ and shipped no name-based fallback. That PR's own commit message called for a coordinated change here; it was never made. Since the materializer deployed, every click has 404'd and shown a "Not Found" dialog instead of resyncing. LocationForm already receives the room id and was discarding it to look the room's name back up, so no new data is needed. It now passes this.props.entity.id (the persisted venue, so an unsaved rename in the form cannot become the target) and the room id straight through, guarding against a venue that has never been saved (id 0). The existing action tests only asserted that postRequest was called, never the URL, which is why they stayed green through the regression. Added a test pinning the built URL, and a LocationForm test that opens the Rooms panel, clicks the real resync action and asserts the ids reach onRoomResync. Both fail if the old behavior is restored. Full suite: 154 suites, 1410 tests, green. Co-Authored-By: Claude <noreply@anthropic.com>
77a7a88 to
2d6b74e
Compare
| @@ -442,7 +458,7 @@ describe("dropbox sync actions", () => { | |||
| test("resyncRoom dispatches STOP_LOADING on failure", async () => { | |||
There was a problem hiding this comment.
please review all tests @caseylocker , tests like this one make no sense
There was a problem hiding this comment.
@santipalenque, fair. I went through all 35 in the file, not just this one. This test and its rebuildSync twin were the worst. They asserted an action sequence production never actually emits, guarding an overlay that uicore's error handler already clears on its own. I rewrote them down to the one thing they really protect (the promise settles after a failed POST, since nothing upstream catches it) and made them prove the POST actually fired.
While I was in there I deleted three more that were duplicates, or literally could not execute their own assertions, folded three near-overlaps into siblings, and replaced the media-upload disjointness test. Its mock threw away the action creators and so could never catch the regression it was named for. That one is mutation-verified now. The rest I kept on purpose: the sync-config failure tests are the only thing clearing the loading flag that disables Save and Rebuild, and each guard test covers a distinct branch. 35 down to 29, all green, in 4edbdc2.
Full pass over the file in response to review. Deleted three tests (a duplicate single-page summit-switch test, a supersession test strictly subsumed by the A-B-A test's nothing-was-dispatched assertion, and one whose distinctive assertions iterated an array just asserted empty). Folded three near-overlaps into siblings without losing an assertion (loading bracketing, the snackbarErrorHandler identity pin - now in the mid-fan-out test so both getRequest call sites are covered, and the resyncRoom action sequence into the URL pin). Rewrote the rebuildSync/ resyncRoom failure tests to assert the real contract - the dispatch promise settles and the POST fired - instead of a two-action sequence production never emits (authErrorHandler stops loading before uicore rejects). Replaced the media-upload disjointness test with a creator-dispatching mock across both getRequest sites; the old mock discarded the creators, so it could not detect the regression it named (verified by mutation: wiring a media-upload creator in now fails it). Filtered the inert DUMMY placeholder out of the two exact-sequence assertions. 35 tests -> 29, every survivor with a nameable regression. Co-Authored-By: Claude <noreply@anthropic.com>
![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/actions/__tests__/dropbox-sync-actions.test.js`:
- Line 446: Update the rejected request mock setup using mockRequestImplReject
so it matches postRequest’s curried invocation: the mock must return a
params-level function that returns a dispatch-level function, which then returns
the rejected Promise. Apply this correction to both affected
postRequest.mockImplementation occurrences so the thunk’s catch handler receives
the rejection instead of a TypeError.
🪄 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 Plus
Run ID: ab79f9ad-d2fe-44e8-b601-9110b996ca1a
📒 Files selected for processing (1)
src/actions/__tests__/dropbox-sync-actions.test.js
ref: https://app.clickup.com/t/86bb4r5df
What's broken
The Resync Room action on the Locations page has been 404ing in production since the materializer's destination-path restructure deployed.
We send the venue and room by name:
The materializer only accepts integers:
Django's
intconverter refuses a non-numeric segment, so the URL does not resolve. There is no name-based fallback. uicore'sauthErrorHandlerrenders a "Not Found" dialog on the 404, so the user sees an error and the resync never runs.How it happened
dropbox-materializer PR #24 (
5bb2d5e, merged 2026-06-15) replaced<str:venue>/<str:room>with<int:venue_id>/<int:room_id>. Its own commit message asked for this change:It was never made. This repo last touched that URL in PR #833, months before the contract changed. It stayed invisible because production ran pre-#24 materializer code until recently; the deployed container now has the ID-based build, so the mismatch is live.
The change
LocationFormwas already handed the room id and threw it away to look the room's name back up, so no new data is needed.location-form.js::handleRoomResyncnow passesthis.props.entity.idandroomId. Reading the venue id offprops(notstate) keeps an unsaved rename in the form from becoming the target, matching what the old code did withpersistedVenueName. Guarded so a never-saved venue (id: 0) does not fire.edit-location-page.js::handleRoomResyncanddropbox-sync-actions.js::resyncRoomrenamed tovenueId/roomId; the URL drops theencodeURIComponentname interpolation.Prettier reflowed the body of
resyncRoombecause the shortened signature now fits on one line. That islint-staged's own output, not a hand edit.No materializer change is needed. It has accepted and tested integer ids since #24:
MaterializeRoomViewbuildsroom_filter = "{venue_id}/{room_id}", which matchesbuild_schedule's keys.Tests
The existing action tests only asserted that
postRequestwas called, never the URL — which is exactly why they stayed green through this regression. Added:.../sync/materialize/1/12/345/LocationFormtest that opens the Rooms panel, clicks the real resync action, and assertsonRoomResyncreceives the two idsBoth were mutation-tested: restoring the old name-based behavior fails them (
Expected: 781, 5002 / Received: "International Barcelona Convention Center", "Room A").The existing inline props in
location-form.test.jswere lifted into abuildProps()factory so both describes share them; the pre-existing test is otherwise untouched.Full suite: 154 suites, 1410 tests, green.
Worth a manual check
Nothing automated crosses the two repos, so a click-through on staging after merge would be worth it.
Summary by CodeRabbit
Convention pass
Run against
skills/react-frontend.md§§1–7 andpatterns/show-admin/summit-admin-testing-patterns.md, by grep rather than eyeball.Clean on §1 (no URL/query built in a component — this change moves toward it, deleting the name lookup from the form), §2 (no new
utils/files), §3/§4 (no reimplemented formatters, no hand-rolled date/money, no new strings), §5 magic numbers (the only literals areVENUE_ID/ROOM_IDnamed constants in a test), §6 (no.css), §7 (full suite, not just the focused files).Two notes for the reviewer, both deliberate:
The bare
returnguard inresyncRoomis not a §5 violation. §5 asks thunks to return a thenable on every branch, and the diff showsif (!baseUrl || !summitId) return;as an added line — but only because prettier reindented the body when the shortened signature collapsed onto one line. Every thunk in this file isasync, so a barereturnalready yields a resolved promise; the rule's concern (undefined returned synchronously while the happy path returns a Promise) does not arise. I confirmed this by mutation: changing it toPromise.resolve()and back leaves behavior and tests identical. Left as-is rather than adding a no-op.The new component test queries by
data-tip, not role/name. The testing playbook prefersgetByRole("button", { name }). uicore'sTablerenders custom row actions as<a href="" data-tip={tooltip}><i className="fa fa-refresh" /></a>— icon-only, no accessible name — so role/name is not reachable without adding anaria-labelto production markup purely for the test, which the same playbook rules out. The selector is commented in place.