chore: simplify workspace routing #17981
Conversation
| const [visible, setVisible] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| const isHidden = HIDE_DEPLOYMENT_BANNER_PATHS.some((regex) => | ||
| regex.test(location.pathname), | ||
| ); | ||
| setVisible(!isHidden); | ||
| }, [location.pathname]); |
There was a problem hiding this comment.
this doesn't need to be an effect, you're pulling state from a hook. just use a const.
There was a problem hiding this comment.
100%!!! Idk why I did that 🤦
| @@ -10,7 +10,7 @@ import { useAgentLogs } from "./useAgentLogs"; | |||
| * Issue: https://github.com/romgain/jest-websocket-mock/issues/172 | |||
| */ | |||
|
|
|||
| describe("useAgentLogs", () => { | |||
| describe.skip("useAgentLogs", () => { | |||
There was a problem hiding this comment.
Just got the go-ahead to work on fixing this flake today!
| flexDirection: "column", | ||
| }} | ||
| > | ||
| <div className="flex flex-col flex-1"> |
There was a problem hiding this comment.
I'm worried that we missed something in the conversion here:
- The old version didn't treat this div as a flex item itself. It just defined how the item grows in the parent flex container
- We didn't add any padding, when the old version of the element had it
Was that intentional?
There was a problem hiding this comment.
Yeap. I did some storybook reviews and QA around and I didn't find anything broken. Maybe it was a leftover? I would appreciate you helping me with some QA 🙏
| // Workspace page. | ||
| // Hide the banner on workspace page because it already has a lot of information. | ||
| /^\/@[^\/]+\/[^\/]+$/, |
There was a problem hiding this comment.
What types of paths are we trying to catch with this? My gut feeling is that the current regex might be a little too loose in some ways, and a little too strict in others
Like, right now, we're exclusively matching patterns that go like /@something/something-else, but we'll never match anything like /@something/something-else/a-third-segment, because the regex only supports two segments. On the other hand, we're letting the regex accept characters that aren't valid in a URL
There was a problem hiding this comment.
If this wasn't what you wanted, you can let me know what you were going for, and I can try to whip up a regex for it
There was a problem hiding this comment.
Yes, this is exactly what we want. It is to catch the workspace page route, but I'm open in case you have a better regex idea or a way to match the path. Since the route path def is /:username/:workspace it was not working well with the react-router-dom path matching utility.
There was a problem hiding this comment.
In that case, try ^\/@(?<username>[a-zA-Z0-9-]+)\/(?<workspace_name>[a-zA-Z0-9-]+)$
- It adds names to the main groups that we're checking for, so it'll be a little more self-documenting
- It redefines each group to only allow the characters A-Z (lowercase or uppercase), numbers, and hyphens
I just checked what characters we allow for usernames and workspace names, and those seemed to be the only characters we support right now
There was a problem hiding this comment.
This is a good one 👍
No description provided.