Fix redirect params placement for hash-based route URLs#13
Open
carl-wells-crowdhandler wants to merge 3 commits intomainfrom
Open
Fix redirect params placement for hash-based route URLs#13carl-wells-crowdhandler wants to merge 3 commits intomainfrom
carl-wells-crowdhandler wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes redirectIfPromoted() URL rewriting so ch-* parameters are appended to the real query string (before any # fragment), ensuring they are visible via window.location.search on hash-based SPA routes.
Changes:
- Update redirect URL construction to split out the hash fragment and append
ch-*params ahead of it. - Regenerate/build distributed JS artifacts reflecting the new redirect behavior.
- Update
package-lock.jsonproject metadata (version/license).
Reviewed changes
Copilot reviewed 1 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/gatekeeper/gatekeeper.ts | Adjusts promoted-user redirect URL building to preserve #... while placing ch-* params in the host query string. |
| dist/gatekeeper/gatekeeper.js | Compiled output reflecting the updated redirect logic. |
| dist/crowdhandler.cjs.js | Bundled CJS build updated with new redirect logic (and header metadata change). |
| dist/crowdhandler.esm.js | Bundled ESM build updated with new redirect logic (and header metadata change). |
| dist/crowdhandler.umd.js | Bundled UMD build updated with new redirect logic (and header metadata change). |
| dist/crowdhandler.umd.min.js | Minified UMD build updated with new redirect logic (and header metadata change). |
| package-lock.json | Updates lockfile project metadata (version/license). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
711
to
715
| for (const param of params) { | ||
| const [key] = param.split('='); | ||
| // Skip CrowdHandler parameters | ||
| if (!['ch-id', 'ch-id-signature', 'ch-requested', 'ch-code', 'ch-fresh'].includes(key)) { | ||
| existingParams.push(param); | ||
| } |
There was a problem hiding this comment.
The CrowdHandler query param key list is duplicated here and elsewhere (e.g. src/common/processURL.ts). Consider centralizing the set of ch-* keys (or a small helper) to avoid drift if new parameters are added/renamed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
redirectIfPromoted() was splitting the destination URL on '?' to append ch-* query parameters. For hash-based routes (e.g. example.com/#/path), this placed the parameters inside the hash fragment where they are invisible to window.location.search on the host domain.
The urlRedirect destination is now preserved intact while ch-* parameters are correctly appended to the query string, ensuring they are visible to the host-domain.