- Notifications
You must be signed in to change notification settings - Fork928
feat: add listening ports protocol selector#12915
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0964e8c toc986370Compare| </Stack> | ||
| {filteredListeningPorts.length === 0 && ( | ||
| <HelpTooltipText css={styles.noPortText}> | ||
| {"No open ports were detected."} |
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.
| {"No open ports were detected."} | |
| Noopenportsweredetected. |
| { | ||
| "The listening ports are exclusively accessible to you. Selecting HTTP/S will change the protocol for all listening ports." | ||
| } |
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 listening ports are exclusively accessible to you. Selecting HTTP/S will change the protocol for all listening ports." | |
| } | |
| Thelisteningportsareexclusivelyaccessibletoyou.Thisoptionwillchangetheprotocolforalllisteningports. |
| const filteredListeningPorts = (listeningPorts ? listeningPorts : []).filter( | ||
| (port) => { | ||
| for (let i = 0; i < filteredSharedPorts.length; i++) { | ||
| if (filteredSharedPorts[i].port === port.port) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
| }); | ||
| return true; | ||
| }, | ||
| ); |
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.
| constfilteredListeningPorts=(listeningPorts ?listeningPorts :[]).filter( | |
| (port)=>{ | |
| for(leti=0;i<filteredSharedPorts.length;i++){ | |
| if(filteredSharedPorts[i].port===port.port){ | |
| returnfalse; | |
| } | |
| } | |
| } | |
| returntrue; | |
| }); | |
| returntrue; | |
| }, | |
| ); | |
| constfilteredListeningPorts=(listeningPorts??[]).filter( | |
| (port)=>filteredSharedPorts.every((sharedPort)=>sharedPort.port!==port.port), | |
| ); |
| export const saveWorkspaceListeningPortsProtocol = ( | ||
| workspaceID: string, | ||
| protocol: WorkspaceAgentPortShareProtocol, | ||
| ) => { | ||
| localStorage.setItem( | ||
| `listening-ports-protocol-workspace-${workspaceID}`, | ||
| protocol, | ||
| ); | ||
| }; | ||
| export const getWorkspaceListeningPortsProtocol = ( | ||
| workspaceID: string, | ||
| ): WorkspaceAgentPortShareProtocol => { | ||
| return (localStorage.getItem( | ||
| `listening-ports-protocol-workspace-${workspaceID}`, | ||
| ) || "http") as WorkspaceAgentPortShareProtocol; | ||
| }; |
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'm always wary of usinglocalStorage for stuff like this. Can you share some of your rationale?
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 am too. We have had many group discussions (dean, jon, kyle, ben) around a few of the PRs coming out of Shared Ports about these protocol selection features. I've suggested keeping this state and other UX state in the DB in the past, but the group thought that putting this button in local storage is better than making a new table to track this stuff. We want an MVP that is feature complete, but can still iterate on later as we get customer feedback, and this was the solution we landed on for now.
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.
Gotcha. If it changes the protocol ofall ports though, is it really "UI state"?
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 promise you I have brought up many different discussions about doing things with this on the backend but the conclusion the group came to was client side storage for now and that we can iterate on the design once we get feedback.
Uh oh!
There was an error while loading.Please reload this page.
Relies on#12908 to be merged first.Closes#12902
Notes: