- Notifications
You must be signed in to change notification settings - Fork907
chore: simplify workspace routing#17981
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
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this doesn't need to be an effect, you're pulling state from a hook. just use aconst.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just got the go-ahead to work on fixing this flake today!
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.
Changes mostly look fine to me. The main thing I'm worried about is the regex, but if you think it's fine, I can go ahead and approve
| flexDirection: "column", | ||
| }} | ||
| > | ||
| <div className="flex flex-col flex-1"> |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
ParkreinerMay 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
ParkreinerMay 23, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a good one 👍
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.
Should be good to go 👌
5b90c69 intomainUh oh!
There was an error while loading.Please reload this page.
No description provided.