-
-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: remove leading '/' from path in client-control on first connect #1156
Conversation
WalkthroughThe recent changes enhance path handling across multiple components by standardizing the format of URLs and paths before transmission. Specifically, leading slashes are removed from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Socket
Client->>Socket: Call connectSocket()
Socket->>Client: Get location.pathname
Client->>Client: Remove leading slash (if present)
Client->>Socket: Send cleaned path with query parameters
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/client/src/common/utils/socket.ts (1 hunks)
Additional comments not posted (1)
apps/client/src/common/utils/socket.ts (1)
40-42
: Ensure path normalization is consistent across the application.The change correctly removes the leading slash from the
location.pathname
. Verify that this normalization is consistent with how paths are handled elsewhere in the application to avoid discrepancies.Verification successful
Ensure path normalization is consistent across the application.
The change in
socket.ts
to remove the leading slash fromlocation.pathname
is consistent with the normalization approach inurlPresets.ts
. Other instances oflocation.pathname
usage do not modify the path, which is acceptable for their purposes.
apps/client/src/common/utils/socket.ts
: Removes leading slash.apps/client/src/common/utils/urlPresets.ts
: Removes leading slash usingsubstring(1)
.This confirms that the path normalization is consistent where necessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that path normalization is consistent across the application. # Test: Search for other instances of `location.pathname` usage. Expect: Consistent path normalization. rg --type js --type ts 'location.pathname'Length of output: 1238
Script:
#!/bin/bash # Extract code snippets where `location.pathname` is used to verify path normalization consistency. # Extract from RundownExport.tsx rg --type js --type ts --context 5 'location.pathname' apps/client/src/features/rundown/RundownExport.tsx # Extract from urlPresets.ts rg --type js --type ts --context 5 'location.pathname' apps/client/src/common/utils/urlPresets.ts # Extract from NavigationMenu.tsx rg --type js --type ts --context 5 'location.pathname' apps/client/src/common/components/navigation-menu/NavigationMenu.tsxLength of output: 3182
//remove leading '/' from path | ||
const fullPath = location.pathname.startsWith('/') ? location.pathname.slice(1) : location.pathname; | ||
socketSendJson('set-client-path', fullPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: as far as I understand, the issue is only in the settings popup.
Could we not just remove one of the slashes there? I dont think the fix is to cover up the issue, but to find where it is introduced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The popup gets its data from what is sent in 'set-client-path
We set the client path from 2 place
I changed this one to match the other one
https://github.com/cpvalente/ontime/blob/c7893938eb22a8f5ef73e6820f90247997623b52/apps/client/src/common/hooks/useClientPath.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I still wonder if there is a way to stop the extra / from making it to the path to begin with.
as of now, just looks like we are patching it up in two places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pathname includes a staring slash
but we don't have to remove it the, the redirect works in both cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the issue here is of UX: I still think it is confusing to show the user two slashes.
The easiest way forwards is to remove a slash from the base URL. However, if the user does not add a slash, will things still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/client/src/common/components/client-modal/RedirectClientModal.tsx (1 hunks)
- apps/client/src/common/hooks/useClientPath.ts (1 hunks)
- apps/client/src/common/utils/socket.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/client/src/common/utils/socket.ts
Additional comments not posted (2)
apps/client/src/common/hooks/useClientPath.ts (1)
16-16
: Potential impact on path communication.Concatenating
pathname
andsearch
directly without removing the leading slash frompathname
may affect how paths are communicated via thesocketSendJson
function. Ensure that the server can handle paths starting with a slash.apps/client/src/common/components/client-modal/RedirectClientModal.tsx (1)
41-41
: Potential impact on URL formatting.Assigning
window.location.origin
directly tohost
without adding a trailing slash may affect any subsequent logic that relies on the format of the URL. Ensure that all URL constructions and usages are compatible with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/client/src/common/hooks/useClientPath.ts (1 hunks)
- apps/client/src/common/utils/socket.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- apps/client/src/common/hooks/useClientPath.ts
- apps/client/src/common/utils/socket.ts
No description provided.