[refactor] Rename facilitator-feedback route param: [groupId] -> [meetPersonId] (#2487)#2592
Conversation
…tPersonId] (#2487) Every caller of this route passes a meetPerson id; the `groupId` segment name was misleading. The URL path itself is unchanged. Reads both `meetPersonId` and `groupId` from `router.query` as a defensive fallback for stale clients during the deploy window. Follow-up PR drops the fallback.
Will-Howard
temporarily deployed
to
wh-2487-facilitator-feedback-rename-2026-06 - bluedot-preview PR #2592
Render
Destroyed
|
Warning Review limit reached
More reviews will be available in 31 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
Will-Howard
temporarily deployed
to
wh-2487-facilitator-feedback-rename-2026-06 - bluedot-storybook-preview PR #2592
Render
Destroyed
Greptile SummaryThis PR renames the Next.js dynamic route segment from
Confidence Score: 4/5Safe to merge — this is a pure rename with a well-scoped backward-compat fallback; the URL path is unchanged so no external links or API calls break. The rename itself is clean and the fallback is harmless. The only gap is that the new groupId fallback expression is untested, meaning a regression there would be invisible until the follow-up removal PR. No runtime data or auth paths are affected. The test file would benefit from a second case covering the groupId-only scenario before the follow-up removal PR lands. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant NextRouter
participant FeedbackPage as [meetPersonId].tsx
participant SuccessPage as [meetPersonId]/success.tsx
Browser->>NextRouter: GET /facilitator-feedback/rec123
NextRouter->>FeedbackPage: "router.query.meetPersonId = "rec123""
Note over FeedbackPage: meetPersonId = query.meetPersonId ?? query.groupId
FeedbackPage->>Browser: Render feedback form
Browser->>FeedbackPage: Submit feedback
FeedbackPage->>NextRouter: router.push(/facilitator-feedback/rec123/success)
NextRouter->>SuccessPage: "router.query.meetPersonId = "rec123""
Note over SuccessPage: meetPersonId = query.meetPersonId ?? query.groupId
SuccessPage->>Browser: Render success page
Reviews (1): Last reviewed commit: "[refactor] Rename facilitator-feedback r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const mockRouter = { | ||
| query: { groupId: 'rec-facilitator' }, | ||
| query: { meetPersonId: 'rec-facilitator' }, | ||
| isReady: true, | ||
| asPath: '/facilitator-feedback/rec-facilitator', | ||
| pathname: '/facilitator-feedback/[groupId]', | ||
| pathname: '/facilitator-feedback/[meetPersonId]', | ||
| push: vi.fn(), | ||
| replace: vi.fn(), | ||
| }; |
There was a problem hiding this comment.
groupId fallback path
The new backward-compat expression (router.query.meetPersonId ?? router.query.groupId) is the only functional code change in this PR, but the existing test only exercises meetPersonId. A stale-client scenario where router.query only contains groupId (and meetPersonId is absent) has no coverage, so a regression in the fallback could go undetected before the follow-up removal PR lands.
Rule Used: Consider adding tests for any new functionality in... (source)
Learned From
bluedotimpact/bluedot#956
bluedotimpact/bluedot#969
bluedotimpact/bluedot#958
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Stale tabs from before the rename have rolled over by now; no caller writes the old `groupId` param anymore.
Stale tabs from before the rename have rolled over by now; no caller writes the old `groupId` param anymore.
1 participant
Description
Every caller of
/facilitator-feedback/[groupId]passes a meetPerson id (the value comes frommeetPerson.id, not a group id), so the dynamic-param name is misleading for anyone opening the file. The URL path is unchanged — only the param name (and the page-internalrouter.queryread) move.To keep stale tabs alive during the deploy window, the renamed page reads both names with a fallback:
(router.query.meetPersonId ?? router.query.groupId). A follow-up PR removes the fallback once the deploy stabilises.Issue
Fixes #2487
Developer checklist