Normalize backend protocol for web vitals reporting#144
Conversation
WalkthroughA Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| function normalizeProtocol(protocol: string | undefined): string | undefined { | ||
| if (protocol === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const trimmedProtocol = protocol.trim(); | ||
|
|
||
| if (!trimmedProtocol) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return trimmedProtocol.endsWith(':') ? trimmedProtocol : `${trimmedProtocol}:`; |
There was a problem hiding this comment.
The new normalizeProtocol helper appends a colon without first removing any trailing slashes. When NEXT_PUBLIC_BACKEND_PROTOCOL is set to the common value 'https://' (or 'http://'), the function now returns 'https://:', so buildEndpoint emits URLs like https://://host/.... fetch rejects these with TypeError: Failed to parse URL, which prevents web‑vitals from being reported. Before this change the same configuration produced 'https:////host', which browsers normalize to a valid https://host/... request, so this is a regression. Consider trimming any trailing slashes before ensuring the colon.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/reportWebVitals.ts(1 hunks)
🔇 Additional comments (1)
frontend/src/app/reportWebVitals.ts (1)
25-26: LGTM! Protocol fallback chain is sound.The normalization is correctly applied to the environment variable, and the fallback to
window.location.protocolor'http:'handles cases where the normalized value isundefined.
| function normalizeProtocol(protocol: string | undefined): string | undefined { | ||
| if (protocol === undefined) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const trimmedProtocol = protocol.trim(); | ||
|
|
||
| if (!trimmedProtocol) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return trimmedProtocol.endsWith(':') ? trimmedProtocol : `${trimmedProtocol}:`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add test coverage for the normalization function.
The PR notes indicate testing was not run. Given that normalizeProtocol handles various edge cases (undefined, empty strings, whitespace, trailing colons, etc.), unit tests would help prevent regressions and document expected behavior.
Do you want me to generate test cases for normalizeProtocol covering scenarios like:
undefined→undefined""→undefined" "→undefined"http"→"http:""https:"→"https:""http://"→"http:"" https "→"https:"
🧹 Nitpick | 🔵 Trivial
Consider validating the protocol value.
While the normalization handles basic formatting, it doesn't validate that the protocol is one of the expected values (http or https). An invalid protocol value could lead to failed requests or unexpected behavior.
Consider adding validation:
function normalizeProtocol(protocol: string | undefined): string | undefined {
if (protocol === undefined) {
return undefined;
}
const trimmedProtocol = protocol.trim();
if (!trimmedProtocol) {
return undefined;
}
const withoutSlashes = trimmedProtocol.replace(/\/+$/, '');
- return withoutSlashes.endsWith(':') ? withoutSlashes : `${withoutSlashes}:`;
+ const normalized = withoutSlashes.endsWith(':') ? withoutSlashes : `${withoutSlashes}:`;
+
+ // Validate against expected protocols
+ const baseProtocol = normalized.replace(/:$/, '').toLowerCase();
+ if (!['http', 'https', 'ws', 'wss'].includes(baseProtocol)) {
+ console.warn(`Invalid protocol "${protocol}", expected http/https/ws/wss`);
+ return undefined;
+ }
+
+ return normalized;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/app/reportWebVitals.ts around lines 6 to 18, the
normalizeProtocol function trims and appends a colon but does not validate that
the protocol is an allowed value; add validation after trimming to allow only
"http" or "https" (case-insensitive) and return undefined for anything else.
Implement by normalizing case (e.g., toLowerCase()), check against a small
whitelist ["http","https"], and return the trimmed protocol with a trailing
colon only when it matches; otherwise return undefined.
Handle protocol strings containing slashes.
The normalization doesn't account for common misconfigurations like "http://" or "https://". If NEXT_PUBLIC_BACKEND_PROTOCOL is set to "http://", the function returns "http://:", which produces an invalid URL at line 32: "http://:hostname...".
Apply this diff to strip trailing slashes before adding the colon:
function normalizeProtocol(protocol: string | undefined): string | undefined {
if (protocol === undefined) {
return undefined;
}
const trimmedProtocol = protocol.trim();
if (!trimmedProtocol) {
return undefined;
}
- return trimmedProtocol.endsWith(':') ? trimmedProtocol : `${trimmedProtocol}:`;
+ const withoutSlashes = trimmedProtocol.replace(/\/+$/, '');
+ return withoutSlashes.endsWith(':') ? withoutSlashes : `${withoutSlashes}:`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function normalizeProtocol(protocol: string | undefined): string | undefined { | |
| if (protocol === undefined) { | |
| return undefined; | |
| } | |
| const trimmedProtocol = protocol.trim(); | |
| if (!trimmedProtocol) { | |
| return undefined; | |
| } | |
| return trimmedProtocol.endsWith(':') ? trimmedProtocol : `${trimmedProtocol}:`; | |
| } | |
| function normalizeProtocol(protocol: string | undefined): string | undefined { | |
| if (protocol === undefined) { | |
| return undefined; | |
| } | |
| const trimmedProtocol = protocol.trim(); | |
| if (!trimmedProtocol) { | |
| return undefined; | |
| } | |
| const withoutSlashes = trimmedProtocol.replace(/\/+$/, ''); | |
| return withoutSlashes.endsWith(':') ? withoutSlashes : `${withoutSlashes}:`; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/app/reportWebVitals.ts around lines 6 to 18, normalizeProtocol
currently trims the input but does not remove trailing slashes, so values like
"http://" become "http://:" and produce invalid URLs; update the function to
remove any trailing slashes after trimming (e.g. strip all trailing '/'
characters), then treat an empty result as undefined and finally ensure it ends
with a single ':' (add one if missing) before returning.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #144 (comment) The following files were modified: * `frontend/src/app/reportWebVitals.ts`
Docstrings generation was requested by @shayancoin. * #144 (comment) The following files were modified: * `frontend/src/app/reportWebVitals.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f1571576ac8330b06da1c887db0274
Summary by CodeRabbit