Improved Streamer Mode #2792
Improved Streamer Mode #2792
Conversation
Updated the description for Streamer Mode to be more concise.
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pull request enhances the private lobby creation experience by adding lobby ID visibility controls, clipboard copy functionality, and integrating user preferences. The modal now displays copy feedback and allows users to toggle ID visibility based on saved settings. Changes
Sequence DiagramsequenceDiagram
participant User
participant Modal as JoinPrivateLobbyModal
participant Settings as UserSettings
participant Clipboard
User->>Modal: Open Modal
activate Modal
Modal->>Settings: Fetch lobbyIdVisibility
Settings-->>Modal: Return setting value
Modal->>Modal: Initialize UI state<br/>(lobbyIdVisible, copySuccess)
deactivate Modal
rect rgb(220, 240, 255)
Note over User,Modal: Lobby ID Display Phase
alt User not yet joined
Modal-->>User: Show paste input
else User joined
Modal-->>User: Show ID with<br/>visibility toggle & copy
end
end
User->>Modal: Toggle Visibility
activate Modal
Modal->>Modal: Update lobbyIdVisible state
Modal-->>User: Re-render UI
deactivate Modal
User->>Modal: Click Copy
activate Modal
Modal->>Clipboard: Copy lobby ID to clipboard
Clipboard-->>Modal: Success
Modal->>Modal: Set copySuccess flag<br/>Show feedback
Modal-->>User: Display confirmation
deactivate Modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/JoinPrivateLobbyModal.ts (3)
83-86: Remove redundantrequestUpdate()calls.Properties decorated with
@state()automatically trigger re-renders when their values change. The explicitrequestUpdate()call is not needed here.🔎 Proposed fix
@click=${() => { this.lobbyIdVisible = !this.lobbyIdVisible; - this.requestUpdate(); }}Same applies to line 104.
281-290: Consider adding user feedback on copy failure.The copy operation silently fails if clipboard access is denied. Users might not know why the copy didn't work. A brief error message could improve UX, though this is a nice-to-have.
🔎 Proposed enhancement
private async copyLobbyId() { if (!this.lobbyIdValue) return; try { await navigator.clipboard.writeText(this.lobbyIdValue); this.copySuccess = true; setTimeout(() => (this.copySuccess = false), 2000); } catch (err) { console.error("Failed to copy lobby id: ", err); + // Optionally show a brief error state to the user } }
201-206: Use theUserSettings.lobbyIdVisibility()typed accessor method for consistency.The
UserSettingsclass provides alobbyIdVisibility()method that handles the setting key directly. Replace the rawget()call with this method to match the established pattern across the codebase and improve maintainability.Note: The setting value is correct as-is—
truemeans "visible" (not hidden), matching the default behavior. Do not invert the value.Suggested fix
public open(id: string = "") { - this.lobbyIdVisible = this.userSettings.get( - "settings.lobbyIdVisibility", - true, - ); + this.lobbyIdVisible = this.userSettings.lobbyIdVisibility(); this.copySuccess = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/JoinPrivateLobbyModal.tssrc/client/styles.css
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/styles.css
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/JoinPrivateLobbyModal.ts
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/styles.css
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (1)
src/client/JoinPrivateLobbyModal.ts (1)
src/core/game/UserSettings.ts (1)
UserSettings(6-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Deploy to openfront.dev
- GitHub Check: Agent
🔇 Additional comments (5)
resources/lang/en.json (1)
397-397: LGTM!The updated text correctly reflects the expanded scope: lobby ID visibility now applies to both creating and joining private lobbies.
src/client/styles.css (1)
543-569: LGTM!The styling updates are consistent with the existing patterns in this file. The dark translucent theme with smooth transitions matches similar elements like
.lobby-id-buttonand.lobby-id-paste-button.src/client/JoinPrivateLobbyModal.ts (3)
14-25: LGTM!State management looks good. The
UserSettingsinstantiation follows the established codebase pattern.
269-274: LGTM!The method correctly extracts the lobby ID from URLs and synchronizes both the state and input element.
214-223: LGTM!The cleanup logic properly resets all state and clears the polling interval.
There was a problem hiding this comment.
Pull request overview
This PR enhances the streamer mode functionality by adding lobby ID visibility controls and copy functionality to the Join Private Lobby modal, bringing it to feature parity with the Create Private Lobby modal. The changes respect user preferences set in the settings menu for hiding lobby IDs.
Key Changes
- Added visibility toggle and copy functionality for lobby IDs in the Join Private Lobby modal when a lobby has been joined
- Updated the settings description text to reflect that the "Hidden Lobby IDs" setting now applies to both create and join private lobby modals
- Improved input field styling with a darker theme, enhanced borders, and smooth hover/focus transitions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/client/JoinPrivateLobbyModal.ts | Added state variables for lobby ID visibility and copy success, implemented conditional rendering to show visibility toggle and copy button after joining a lobby, integrated with UserSettings to respect user preferences |
| src/client/styles.css | Enhanced lobby ID input styling with darker background, refined borders, and added hover/focus state transitions |
| resources/lang/en.json | Updated settings description from "Hide Lobby ID in private lobby creation" to "Hide Lobby ID in private lobbies" to reflect the expanded functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ryanbarlow97
added
UI/UX
|
@iiamlewis do you want it toggled on by default? I don't really see an issue to it being on by default |
|
@ryanbarlow97 You're not asking me. But I'd definitely not want to have it turned off by default. That will confuse thousands (some asking on discord over and over, others just abandoning the idea of playing with their friends because they can't figure it out and move to the next game) just for saving a few knowing streamers one or two clicks. Tryout33 |
really, this is that confusing? it's a universal thing to have passwords/gameIDs hidden like this |
|
@ryanbarlow97 having once worked in customer service and also seeing the many easily answered questions/remarks in the OF discord, yes for some people it can actually be confusing. I'm weighing this against the few streamers the default on setting is meant to serve and from that conclude that it could be counterproductive. But we could choose to fafo it ;) |
No, I was just asking as I wanted a consensus, I didn't set it up this way anyway so not a big problem |
|
@ryanbarlow97 can we have it off by default and then toggleable to be turned on |
yes thats how it is now :) |
|
closing because i dont wanna worry about fixing it with the new ui |
Description:
Adds a slightly better streamer mode:
Updates text on already existing button within settings:
If enabled, by default the "Create Private Lobby" modal will have the ID hidden:
It now also has the same button copy/hidden button in the "Join Private Lobby" modal:
Hidden:
Displayed:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n