Skip to content

refactor(@angular/ssr): combine redirect URL parameter helpers and use DefaultUrlSerializer - #34467

Open
alan-agius4 wants to merge 1 commit into
angular:mainfrom
alan-agius4:refactor/ssr-redirect-params
Open

alan-agius4 wants to merge 1 commit into
angular:mainfrom
alan-agius4:refactor/ssr-redirect-params

Conversation

@alan-agius4

Copy link
Copy Markdown
Collaborator

Follow-up to #34321:

  • Combines buildPathWithParams and buildQueryWithParams into buildUrlWithParams.
  • Reuses DefaultUrlSerializer from @angular/router for query parameter parsing and serialization instead of custom regex/encoding helpers.
  • Adds fast-path checks to avoid regex/splitting and URLSearchParams instantiation when redirect targets do not contain placeholders.
  • Applies joinUrlParts only to the path portion of redirect targets when prepending baseHref or X-Forwarded-Prefix so trailing slashes in query parameters or fragments are preserved.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors URL parameter resolution in Angular SSR by replacing buildPathWithParams and buildQueryWithParams with a unified buildUrlWithParams function that leverages Angular's DefaultUrlSerializer. It also optimizes route parameter replacement and prefix handling. Feedback on the changes points out a potential runtime TypeError when resolving path placeholders if a wildcard or positional placeholder refers to an out-of-bounds index, and suggests adding a fallback to 'undefined'.

Comment thread packages/angular/ssr/src/utils/url.ts Outdated
@alan-agius4

Copy link
Copy Markdown
Collaborator Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors URL and query parameter resolution in the Angular SSR package by consolidating buildPathWithParams and buildQueryWithParams into a single buildUrlWithParams function, and merging replaceRedirectParams into resolveRedirectTo. Feedback on these changes highlights two potential issues: first, appending url.search directly with + for relative redirects could result in malformed URLs with multiple ? characters if the target already contains query parameters; second, resolving path placeholders when fromPath has fewer segments than expected could pass undefined to joinUrlParts, leading to a runtime TypeError.

Comment thread packages/angular/ssr/src/app.ts
Comment thread packages/angular/ssr/src/utils/url.ts Outdated
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 21, 2026
@alan-agius4
alan-agius4 requested a review from dgp1130 October 21, 2026 09:03
…e DefaultUrlSerializer

Combine `buildPathWithParams` and `buildQueryWithParams` into a single `buildUrlWithParams` helper that uses `DefaultUrlSerializer` from `@angular/router` for query parameter parsing and serialization.

Also avoid unnecessary regex/string splitting when redirect targets do not contain parameter placeholders, and only apply `joinUrlParts` to the path portion of redirect URLs so trailing slashes in query parameters or fragments are preserved.
@alan-agius4
alan-agius4 force-pushed the refactor/ssr-redirect-params branch from 1051c1d to c663e81 Compare October 21, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: @angular/ssr target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant