Skip to content

Add confirmation dialog before closing host lobby modal 🔧#3364

Merged
evanpelle merged 2 commits intomainfrom
feat/lobby-close-confirmation
Mar 6, 2026
Merged

Add confirmation dialog before closing host lobby modal 🔧#3364
evanpelle merged 2 commits intomainfrom
feat/lobby-close-confirmation

Conversation

@FloPinguin
Copy link
Contributor

Description:

Show a confirm prompt when the user tries to close the host lobby via click-outside or Escape key, preventing accidental lobby exits. Happened to many people and also DougDoug...

Does not show the prompt when the user clicks the arrow button because it's probably intentional.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

FloPinguin

Show a confirm prompt when the user tries to close the host lobby
via click-outside or Escape key, preventing accidental lobby exits.

- Add confirmBeforeClose() guard to BaseModal (Escape key)
- Check confirmBeforeClose() in Navigation.ts (click-outside)
- Override in HostLobbyModal with translated confirmation message
- Add leave_confirmation translation key
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c7359c02-4a18-4fb9-ae0f-2209c7501c0b

📥 Commits

Reviewing files that changed from the base of the PR and between cd333bf and 52e34f8.

📒 Files selected for processing (1)
  • src/client/components/BaseModal.ts

Walkthrough

Adds a close-confirmation lifecycle hook and its use: BaseModal gains confirmBeforeClose(), HostLobbyModal implements it to show a translated confirmation dialog, Navigation checks the hook and aborts closing when it returns false, and a new localization key host_modal.leave_confirmation is added.

Changes

Cohort / File(s) Summary
Localization
resources/lang/en.json
Added host_modal.leave_confirmation with message text. Adjusted surrounding JSON punctuation as needed.
Modal base
src/client/components/BaseModal.ts
Added public confirmBeforeClose(): boolean lifecycle method (defaults to true). Integrated guard into Escape-key close and underlying modal close flow to abort closing when it returns false.
Host modal
src/client/HostLobbyModal.ts
Implemented confirmBeforeClose(): boolean to call confirm(translateText("host_modal.leave_confirmation")) and return the user's choice.
Navigation
src/client/Navigation.ts
Before closing an open modal on main-area navigation, checks confirmBeforeClose() (if present) and aborts close when it returns false; otherwise proceeds with existing leave/close logic.

Sequence Diagram

sequenceDiagram
    actor User
    participant Nav as Navigation
    participant Modal as HostLobbyModal
    participant Trans as Translation

    User->>Nav: Click to navigate away / main-area click
    Nav->>Modal: call confirmBeforeClose()
    Modal->>Trans: translateText("host_modal.leave_confirmation")
    Trans-->>Modal: returns localized string
    Modal->>User: show confirm(dialog)
    User-->>Modal: confirm OK or Cancel
    Modal-->>Nav: returns true or false

    alt confirmed (true)
        Nav->>Modal: proceed with close
        Modal->>Nav: close and cleanup
    else cancelled (false)
        Nav->>Nav: abort close, keep modal open
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🚪 A tiny guard before goodbye,
A prompt that asks the user why,
One key, one click, one little test,
Confirm your leave, then take your rest.
Small change, calm exit — peace of mind.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a confirmation dialog before closing the host lobby modal.
Description check ✅ Passed The description is detailed and directly related to the changeset, explaining the motivation, implementation approach, and confirming completion of required tasks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ryanbarlow97 ryanbarlow97 added UI/UX UI/UX changes including assets, menus, QoL, etc. Bugfix Fixes a bug labels Mar 6, 2026
@ryanbarlow97 ryanbarlow97 added this to the v30 milestone Mar 6, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/client/components/BaseModal.ts`:
- Around line 99-106: The onClose callback registered in firstUpdated()
currently calls BaseModal.close() directly and thus bypasses the
confirmBeforeClose() guard used for Escape key handling; update the onClose
handler (the callback passed to the o-modal component in firstUpdated()) to call
confirmBeforeClose() first and only invoke BaseModal.close() when
confirmBeforeClose() returns true (return false / skip calling close() when the
guard returns false) so backdrop clicks respect the same confirmation logic as
Escape key presses.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 71e3a42d-0def-4806-9f93-5bf62071e099

📥 Commits

Reviewing files that changed from the base of the PR and between 6154b77 and cd333bf.

📒 Files selected for processing (4)
  • resources/lang/en.json
  • src/client/HostLobbyModal.ts
  • src/client/Navigation.ts
  • src/client/components/BaseModal.ts

@github-project-automation github-project-automation bot moved this from Triage to Development in OpenFront Release Management Mar 6, 2026
Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

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

thanks!

@github-project-automation github-project-automation bot moved this from Development to Final Review in OpenFront Release Management Mar 6, 2026
@evanpelle evanpelle merged commit 0dc520b into main Mar 6, 2026
10 checks passed
@evanpelle evanpelle deleted the feat/lobby-close-confirmation branch March 6, 2026 04:39
@github-project-automation github-project-automation bot moved this from Final Review to Complete in OpenFront Release Management Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix Fixes a bug UI/UX UI/UX changes including assets, menus, QoL, etc.

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants