Show peer display name on video tile and peer list#690
Show peer display name on video tile and peer list#690iamgabrielsoft wants to merge 41 commits intosecurity-union:mainfrom
Conversation
…into bug-fix-user-rename
…into bug-fix-user-rename
👋 Thanks for your contribution!A maintainer will review your changes shortly. ✅ Automated ChecksCI/CD tests will run automatically. Please ensure all checks pass before requesting review. 🚀 Preview DeploymentsMaintainers can test this PR in a live environment using these commands:
Preview URLs will be posted automatically once deployed. |
| Can you post a before and after screenshot please? |
| /build-and-deploy |
| 🚀 Building images and deploying preview... Started by @testradav Phase 1: Building Docker images (~10-15 min)
Phase 2: Deploying preview (~3-5 min)
⏱️ Total time expected: ~15-20 minutes I'll post updates in this thread as each phase completes. |
| ✅ Images built successfully! 📦 Images pushed to GHCR:
🚀 Starting deployment... |
| ❌ All preview slots are in use (3/3) Cannot deploy preview for PR #690 because all deployment slots are occupied. Active deployments:
Action required: Comment |
| ❌ Deployment failed Check the workflow logs for details. |
1 similar comment
| ❌ Deployment failed Check the workflow logs for details. |
| /deploy |
| 🚀 Deploying PR preview to available slot... Started by @testradav Infrastructure:
⏱️ Deployment typically takes 3-5 minutes, I'll post another comment when it's ready. |
| ❌ Deployment failed Check the workflow logs for details. |
| /build-and-deploy |
| 🚀 Building images and deploying preview... Started by @testradav Phase 1: Building Docker images (~10-15 min)
Phase 2: Deploying preview (~3-5 min)
⏱️ Total time expected: ~15-20 minutes I'll post updates in this thread as each phase completes. |
| ✅ Images built successfully! 📦 Images pushed to GHCR:
🚀 Starting deployment... |
| ❌ Deployment failed Check the workflow logs for details. |
| There's an helm chat issue @testradav |
I see that. Thanks. Was trying to see the feature as I thought we already did display the username and the peer list. That's why providing a screenshot with the PR is something useful thanks |
Never mind. Now I see this is on the tile. |
testradav left a comment
testradav left a comment There was a problem hiding this comment.
PR #690 Review: "Show peer display name on video tile and peer list"
39 files changed, +991/-368 — This PR is doing too much. The stated goal is showing display names instead of session IDs, but it bundles unrelated migrations, models, proto changes, config changes, and formatting tweaks.
Critical / Blocking
-
Protobuf wire-incompatible breaking change (generated code)
The regenerated media_packet.rs has renumbered the MediaType enum — MEDIA_TYPE_UNKNOWN (value 0) was removed and all values shifted down (VIDEO is now 0 instead of 1, AUDIO is now 1 instead of 2, etc.). The VideoMetadata.codec field was also
silently dropped. These changes don't appear in the .proto file diff, meaning the generated Rust code is out of sync with the proto definitions. Any deployed server/client running the old code will misinterpret packets from the new code. This
alone should block the PR. -
Session ID storage key mismatch (bug)
In yew-ui/src/components/attendants.rs:get_or_create_session_id():
// Reads with literal key:
if let Ok(Some(id)) = storage.get_item("session_id") { ... }
// Writes with constant:
let _ = storage.set_item(SESSION_ID_KEY, &new_id); // SESSION_ID_KEY = "vc_session_id"
These are different keys. The session ID will never be found on subsequent loads, generating a new one every time — defeating the purpose of a stable session ID. -
get_peer_email now returns session_id instead of email
video_call_client.rs:559 — The function still called get_peer_email was changed to return peer.session_id instead of peer.email (which was renamed to display_name). This breaks the users_allowed_to_stream() allowlist check in
canvas_generator.rs, which compares against email addresses. -
Host detection broken after name change
canvas_generator.rs now compares host_display_name (set at join time from the email/username) against peer_display_name (mutable, updated via heartbeat). After a user renames themselves, these won't match and the host crown icon disappears.
Should Not Be In This PR
-
Unrelated database migrations — Three new migration files (20251221011540, 20251221011853, 20251221012015) adding meeting_protection_fields, meeting_owners table, and waiting_room_queue table. These are for a completely different feature.
-
meeting_owner.rs model — A full new model with raw SQL queries for meeting ownership, unrelated to display names.
-
DEFAULT_SALT constant — bcrypt salt added to actix-api/src/constants.rs, not used by this PR.
-
Rename-bug-fix.md — Personal brainstorming/design doc committed to repo root. Should not be checked in.
-
webTransportEnabled changed to false in yew-ui/scripts/config.js — Local dev config change that would affect all developers.
-
Migration CASCADE added — 20260203000003 changed DROP CONSTRAINT to use CASCADE, which can silently drop dependent foreign keys and views. This is a separate concern that needs explicit review.
Bugs / Code Quality
-
Dioxus-ui gets no benefit — New fields are set to empty strings with no callback:
session_id: String::new(),
display_name: String::new(),
on_peer_display_name_changed: None, -
Peer.email field left as comment — // pub email: String, should be fully removed, not commented out.
-
Unused code added — PeerStatus::NameChanged variant is never constructed. is_meeting_metadata_packet in ws_chat_session.rs is #[allow(dead_code)]. session_id and display_name fields added to MediaPacket proto (fields 10, 11) but never
populated on the send path. -
Debug-level logging at info level — host.rs has log::info!(">>> SaveChangeName triggered...") style debug breadcrumbs that should be removed or downgraded.
-
Format-only changes scattered throughout — Many files have format!("{}", x) → format!("{x}") changes that inflate the diff and should be a separate PR.
Recommendation
Do not merge as-is. This needs to be split into smaller PRs:
- Proto changes + regeneration (done correctly, without renumbering existing enum values)
- Display name feature (client + yew-ui + dioxus-ui)
- Database migrations for meeting ownership (separate feature)
- Formatting cleanup (separate trivial PR)
The session ID key bug and get_peer_email semantic change need to be fixed before the display name feature can work correctly.
| -- migrate:up | ||
| | ||
| ALTER TABLE meetings | ||
| ADD COLUMN meeting_title VARCHAR(255) NULL, |
There was a problem hiding this comment.
Why do we need DB changes to show peer display name on video tile and peer list @iamgabrielsoft ?
There was a problem hiding this comment.
The PR came from the bug-fix-user-rename branch, that why its seems like I touched it. The pull was from the bug-rename branch. I'll put this PR on draft mode until the bug-rename is merged
3 participants
This PR fixes the display of peer names in the video call interface, showing actual display names instead of raw session IDs.
Changes Made
VideoCallClient Enhancement
get_peer_display_name()method that readspeer.display_namefromPeerDecodeManager(updated via heartbeat packets)Canvas Generator Updates
get_peer_display_name()peer_display_nameprop toPeerTilePropsso Yew re-renders the tile when the name changesPeer List Improvements
peer_display_namesmap toPeerListcomponentAttendants Component Updates
peer_display_nameper-tile inattendants.rsview()functionTesting
Testing Performed