Skip to content

Join room#27

Draft
maayamohan wants to merge 38 commits into
mainfrom
join-room
Draft

Join room#27
maayamohan wants to merge 38 commits into
mainfrom
join-room

Conversation

@maayamohan

Copy link
Copy Markdown
Collaborator

Room Logic

Which issue(s) does this PR address?

Implementation of room logc

Why do we need this PR?

We need rooms

What logical changes are present in this PR?

Separating client-side code into host and join relevant files.

How did you test the changes in this PR?

I didnt

Are there any breaking changes in this PR?

Maybe

Is there some additional work to be done later that is NOT covered in this PR?

server.js needs some changes to accomodate rooms

@maayamohan

Copy link
Copy Markdown
Collaborator Author

Room Logic

Which issue(s) does this PR address?

Implementation of room logc

Why do we need this PR?

We need rooms

What logical changes are present in this PR?

Separating client-side code into host and join relevant files.

How did you test the changes in this PR?

I didnt

Are there any breaking changes in this PR?

Maybe

Is there some additional work to be done later that is NOT covered in this PR?

server.js needs some changes to accomodate rooms

@mebinthattil @Andy34G7 @Delta18-Git please check this whenever you can

@mebinthattil mebinthattil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have shared my feedback after a quick scan and tests. Please resolve those issues.
Excited to see where y'all take it from here!

Requesting review from @Delta18-Git & @Andy34G7 as well.

Comment thread public/js/navigation.js Outdated
Comment thread public/js/navigation.js Outdated
Comment thread public/page2.html Outdated
Comment thread public/page2.html Outdated
@Delta18-Git

Delta18-Git commented Aug 19, 2025

Copy link
Copy Markdown
Member

Will be reviewing in 30 minutes, thanks for the ping.

EDIT: 4 hrs later the review is complete, my bad 😞

@Delta18-Git Delta18-Git left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Feel like its a good start, but since the server code is not present no real way of tracing the logic completely yet. Also quite confusing that Maaya has been the only one working on this.

Comment thread public/js/navigation.js Outdated
Comment thread public/js/navigation.js Outdated
Comment thread public/js/PeerConnection.js
Comment thread public/host.html
Comment on lines +7 to +58
<style>
body {
background-color: #152238;
color: white;
font-family: 'Fjalla One', sans-serif;
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
height: 100vh;
}

h1 {
font-size: 60px;
margin-bottom: 40px;
}

button {
padding: 14px 28px;
font-size: 22px;
border: none;
border-radius: 10px;
background-color: #0F52BA;
color: white;
cursor: pointer;
margin-top: 20px;
transition: background-color 0.3s;
}

button:hover {
background-color: #0c47a1;
}

#jamCodeDisplay {
font-size: 28px;
margin-top: 20px;
color: #00ffff;
}

#enterRoomHostBtn {
display: none;
}

a {
position: absolute;
top: 20px;
left: 20px;
color: #00ffff;
text-decoration: none;
font-size: 18px;
}
</style>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes sense to move the CSS to a separate file which is imported as there is a lot of shared loc in these webpages.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment applies for all other pages.

@mebinthattil mebinthattil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@SolarPhoenix13
I see that you have created two commits both named update index.html, you could have squashed both those commits into a single commit named 'updating index.html to solve excessive glow animation on JamSesh text.'

Just a suggestion for your future commits.

@Delta18-Git Delta18-Git left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are some minor changes that I've found. Good work, perhaps look into how you can reduce the latency as timesync seems to not have helped much.

Comment thread public/index.html
@@ -1,92 +1,82 @@
<!DOCTYPE html>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using LocalStorage/Cookie instead of query parameters makes more sense to me here.

Comment thread public/jamming.html
@@ -0,0 +1,189 @@
<!DOCTYPE html>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This JS <script> would be better off imported, it gets a bit too complicated here.

Comment thread public/js/host.js

const init = () => {
ws = new WebSocket("ws://localhost:8080");
ws = new WebSocket("wss://jamsesh-8wui.onrender.com");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should not be hardcoded ideally

Comment thread public/js/host.js
window.currentClientId = clientId;
}
// The roomCode is searched on load
const urlParams = new URLSearchParams(window.location.search);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be trivial to move to LocalStorage or a cookie.

Comment thread public/js/host.js
const leftClientId = leavingParticipant.id;
console.log(`Client ${leavingParticipant.username} (${leftClientId}) left.`);
allParticipants = allParticipants.filter(p => p.id !== leftClientId);
if (typeof window.updateParticipantList === 'function') {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why would it be possible to overwrite this to not be a function?

Comment thread public/js/join.js
startBtn.disabled = true;

const init = () => {
ws = new WebSocket("wss://jamsesh-8wui.onrender.com");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is again hardcoded, when it shouldn't be.

Comment thread public/js/join.js
@@ -0,0 +1,378 @@
function loadScript(src) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Check if TURN servers are avoided when they are not needed, or if they are used even without a NAT.

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.

6 participants