Skip to content
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

🚸(frontend) generate shorter room IDs making URLs easier to share #42

Merged
merged 3 commits into from
Jul 21, 2024

Conversation

lebaudantoine
Copy link
Collaborator

Proposition

Simplify room's name (ID).

Proposal

Please refer to the issue #41 which explains why simplifying room IDs could be beneficial in terms of user experience (UX).
It closes #41.

@lebaudantoine lebaudantoine self-assigned this Jul 17, 2024
@lebaudantoine lebaudantoine marked this pull request as ready for review July 17, 2024 20:02
@lebaudantoine
Copy link
Collaborator Author

lebaudantoine commented Jul 18, 2024

I'm currently debating whether room ID validation should be handled on the frontend or backend side. While my last commit meets our short-term needs, introducing this 'lock' regex might pose issues in the long run.

Addition of 'named' rooms would contradict my previous commit, as we'd only need to validate roomId formats for dynamically generated rooms. Again, we should discuss this further with Samuel.

During my development, I noticed that passing an undefined roomId to the Conference component doesn't trigger an error; instead, the undefined value is converted to the string 'undefined'. Should we improve this part? @manuhabitela

import { slugify } from '@/utils/slugify'

const getRandomChar = () => {
// Google Meet uses only letters in a room identifier
Copy link
Collaborator

@manuhabitela manuhabitela Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we officially are google-meet driven development huh? 😏

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We totally remove this mention if you feel it's weird

UUID-v4 room IDs are long and uninviting. Shorter, custom room IDs
can enhance UX by making URLs easier to share and remember.

While UUID-v4s are typically used in database systems for their low
collision probability, for ephemeral room IDs, the collision risk of e+14
combinations is acceptable.

This aligns room IDs with Google Meet format.

Even if the 'slugify' function is not used anymore, I kept it.
Lmk if you prefer removing it @manuhabitela
@manuhabitela
Copy link
Collaborator

During my development, I noticed that passing an undefined roomId to the Conference component doesn't trigger an error; instead, the undefined value is converted to the string 'undefined'. Should we improve this part? @manuhabitela

Good point, I'll change that

Enhanced security by ensuring users are redirected to a 404 error page
if they
pass an incorrect roomId path, either intentionally or unintentionally.
This is
a critical security mechanism that should be included in our MVP.

Let's discuss extracting hardcoded elements, such as lengths or
the separator, into proper constants to improve code maintainability.
I was concerned that this might make the code harder to read, it could
enhance
clarity and reusability in the long term.

I prefer exposing the roomIdRegex from the same location where we
generate IDs.
However, this increases the responsibility of that file. Lmk if you have
any
suggestion for a better organization.

Additionally, the current 404 error page displays a 'Page not found'
message for
invalid room IDs. Should we update this message to 'Invalid room name'
to
provide more context to the user?
- this feels a bit less boilerplaty to read
- puting the characters whitelist outside the function to prevent
creating the var each time (yes, this of super great importance)
@manuhabitela
Copy link
Collaborator

Thanks! Allowed myself to rewrite a tiny bit the content of the small id generation functions to have a little less boilerplate.

@manuhabitela manuhabitela merged commit 0cf4960 into main Jul 21, 2024
7 of 8 checks passed
@manuhabitela manuhabitela deleted the feature-41 branch July 21, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify room's name
2 participants