fix(frontend): collapse duplicate URL path slashes before React Router#1596
fix(frontend): collapse duplicate URL path slashes before React Router#1596kaligrafy wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
Add replaceBrowserPathnameIfNeeded on admin and participant entrypoints, with a small pathname helper and Jest coverage. Add a Playwright spec in demo_survey for //home and related cases. closes chairemobilite#2
kaligrafy
force-pushed
the
fix/double-slash-pathname-normalization
branch
from
fee19d6 to
5dff027
Compare
| * @param pathname - `window.location.pathname` (may include duplicate slashes) | ||
| * @returns Canonical pathname starting with `/` when non-empty | ||
| */ | ||
| export const collapseConsecutiveSlashesInPathname = (pathname: string): string => { |
There was a problem hiding this comment.
Cette fonction a-t-elle vraiment besoin d'être exportée? Si elle n'est utilisée que dans ce fichier, éviter de l'exporter, sinon ça veut dire maintenant, ce n,est plus un détail d'implémentation, etc. etc.
There was a problem hiding this comment.
OK, mais alors l'enjeu sera pour les tests:
mocker window.location en jsdom est très fragile et plus complexe que la fonction testée. Ajouter un JSDoc @internal ou un commentaire "Exported solely for unit testing — not part of the public surface" ferai-t-il l'affaire?
There was a problem hiding this comment.
Copilot me dit que c'est possible de faire ça sans mocker window.location, juste en faisant window.history.pushState({}, '', '/my/path?x=1#frag');. Est-ce que ça fonctionnerait?
There was a problem hiding this comment.
3/5 cas passent, 2 échouent. jsdom rejette pushState('//foo') avec SecurityError: pushState() cannot update history to the URL http://interviews/byCode/ABC123 parce que le parser URL interprète //host comme une autorité (host interviews) → origine différente → rejet.
Concrètement :
/foo//bar (slashs au milieu) : ✓ pushState accepte, origine inchangée
//foo, ///foo (préfixe) : ✗ jsdom les voit comme une nav cross-origin
Or le bug en prod est précisément le cas du préfixe (l'utilisateur tape //interviews/... dans la barre d'adresse, le navigateur l'envoie tel quel et React Router ne matche pas). Donc Copilot a raison sur le mécanisme, mais jsdom ne nous laisse pas reproduire le cas qu'on veut tester.
2 participants
Add replaceBrowserPathnameIfNeeded on admin and participant entrypoints, with a small pathname helper and Jest coverage. Add a Playwright spec in demo_survey for //home and related cases.
closes #2