ref(preprod): Centralize URL building in helper functions #105827
Conversation
github-actions
bot
added
the
Scope: Frontend
- Keep existing getBaseBuildUrl() as foundation - Add getSizeBuildUrl, getInstallBuildUrl, getBuildCompareUrl, getBuildListUrl - Replace all hardcoded /preprod/ URLs with helper function calls - Update 9 component files and 1 test file - No URL pattern changes, purely refactoring for maintainability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Renamed `getBuildCompareUrl` to `getCompareBuildUrl` - Renamed `getBuildListUrl` to `getListBuildUrl` These names follow the pattern of other helper functions like `getSizeBuildUrl` and `getInstallBuildUrl`, making the API more consistent.
runningcode
force-pushed
the
no/refactor-preprod-url-helpers
branch
from
0d6f612 to
fb82bb4
Compare
DRY up the code by reusing getBaseBuildUrl instead of duplicating the URL construction logic.
chromy
left a comment
chromy
left a comment
There was a problem hiding this comment.
thanks lgtm % a few small things.
| return `/organizations/${organizationSlug}/preprod/${projectId}/${baseArtifactId}/`; | ||
| } | ||
|
|
||
| export function getSizeBuildUrl(params: BuildLinkParams): string | null { |
There was a problem hiding this comment.
For this I would normally use undefined rather than null. Works better with optional props on an object e.g. https://www.typescriptlang.org/play/?#code/PTAEEEHcGtIQwE4BNQDsCuAbTAuAUHgGbqoDGALgJYD2qoA5gKbkBi11AFAJQ5paYAfAM7kElVPVABvPKFAJm6BHQBEhdioDceAL4Fx5RgkJxSjUG2rTZoddQD8vEWIna9eUrRG32vS6ABeazk7XiZWdm5Qe3tQEiRGQnFGJAAaXW0CEFAANyEAOkK41ASk1BT8IhIKGjpwgCFEbl54xOSkYVFxSRk5BXIlVQAjRC1dfVRDY1NzRoRg0BGER1BnbrcCT1RvJd45wIXdhmY57nSdTSA
We can't ban null in the frontend unfortunately since the Python API returns it all over the place - but in general if it's possible to use undefined I would do so.
| return `/organizations/${organizationSlug}/preprod/${projectId}/${baseArtifactId}/`; | ||
| } | ||
|
|
||
| export function getSizeBuildUrl(params: BuildLinkParams): string | null { |
There was a problem hiding this comment.
nit: I would name all of these fooPath or fooRoute (see
Technically URLs have the scheme + domain + port which we don't have here:
https://developer.mozilla.org/en-US/docs/Learn_web_development/Howto/Web_mechanics/What_is_a_URL#:~:text=below%20(details%20are%20provided%20in%20the%20following%20sections)%3A
There was a problem hiding this comment.
Ah, I was copying the existing getBaseBuildUrl function but i'll rename that one too.
Do you mean to rename the functions like baseBuildRoute instead of getBaseBuildUrl()?
There was a problem hiding this comment.
baseBuildRoute or getBaseBuildRoute is fine the URL / Route distinction was my main concern
| if (baseArtifactId) { | ||
| path += `/${baseArtifactId}`; | ||
| } | ||
| path += '/'; |
There was a problem hiding this comment.
nit:
let path = `/organizations/${organizationSlug}/preprod/${projectId}/compare/${headArtifactId}/`;
if (baseArtifactId) {
path += `${baseArtifactId}/`;
}
chromy
left a comment
chromy
left a comment
There was a problem hiding this comment.
thanks lgtm % a few small things.
Renamed all `*Url` functions to `*Path` for accuracy since these return URL paths, not full URLs with scheme and domain. Changed `baseArtifactId` parameter from `string | null | undefined` to optional `string?` and return types from `string | null` to `string | undefined`. Using undefined is preferred for optional values in the frontend since it works better with optional object properties. Simplified path construction in `getCompareBuildPath` to build the trailing slash into the base path instead of appending it separately. Refs EME-725
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| organizationSlug, | ||
| projectId: build.project_id.toString(), | ||
| baseArtifactId: build.id, | ||
| }) ?? ''; |
There was a problem hiding this comment.
Trailing slash added to size table build URLs
Low Severity
The URL pattern for size table build links changed from not having a trailing slash to having one. The old code produced /organizations/.../preprod/{projectId}/{buildId} but getSizeBuildPath returns /organizations/.../preprod/{projectId}/{buildId}/. While the PR description states "this doesn't change any behavior", this URL format change could affect routing behavior depending on how the router is configured to handle trailing slashes.
🔬 Verification Test
Why verification test was not possible: This is a string comparison difference that requires the routing infrastructure to determine actual impact. The bug can be verified by comparing the diff directly - the removed line shows no trailing slash while getBaseBuildPath in buildLinkUtils.ts line 14 clearly adds a trailing slash.
Update import from `getBaseBuildUrl` to `getBaseBuildPath` to match the renamed function.
Refactors hardcoded preprod URL strings to use centralized helper functions, establishing a single source of truth for URL patterns to make it easier to change these in the future.
This doesn't change any behavior, just migrates to using functions instead of hard-coded urls.
Related to EME-725