Skip to content

feat(v3): add desktop-only "Start in fullscreen" system option #938

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.

Sign up for GitHub

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

Merged
byrongamatos merged 2 commits into from
Jul 13, 2026
Merged

feat(v3): add desktop-only "Start in fullscreen" system option #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8012a8
feat(v3): add desktop-only "Start in fullscreen" system option
gionnibgud Jul 13, 2026
3050c7b
feat(v3): reword the fullscreen setting + note the macOS launch caveat
gionnibgud Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions static/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@
setAvOffsetMs,
setInstrumentPathway,
setupAppUpdates,
setupWindowOptions,
syncDefaultArrangementPin,
} from './js/settings.js';
import {
Expand Down Expand Up @@ -1497,7 +1498,7 @@
// NOT leave. Space/Enter stay on native activation of the focused button
// (Leave by default), so the keyboard "leave" is Space/Enter.
function onKey(e) {
if (e.key === 'Escape') { e.preventDefault(); e.stopImmediatePropagation(); close(false); }

Check warning on line 1501 in static/app.js

View workflow job for this annotation

GitHub Actions / ci / lint 

File has too many lines (2326). Maximum allowed is 1500
}
document.addEventListener('keydown', onKey, true);
leaveBtn.addEventListener('click', () => close(true));
Expand Down
42 changes: 42 additions & 0 deletions static/js/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export async function loadSettings() {
// failed fetch below still leaves the desktop updater wired up.
// setupAppUpdates() is idempotent via _appUpdatesWired.
setupAppUpdates();
setupWindowOptions();

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Protect settings hydration from a synchronous getter throw.

Promise.resolve(winApi.getStartFullscreen()) evaluates the getter before creating the promise. A synchronous bridge exception therefore escapes setupWindowOptions(). Since loadSettings() calls it at Line 103, the outer catch in static/app.js only logs the error and skips the remaining settings hydration.

Proposed fix 
-    Promise.resolve(winApi.getStartFullscreen()).then(function (on) {
+    Promise.resolve().then(function () {
+        return winApi.getStartFullscreen();
+    }).then(function (on) {
         cb.checked = !!on;
     }).catch(function () { /* leave unchecked on error */ });

Also applies to: 196-200

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@static/js/settings.js` at line 103, Update setupWindowOptions to protect the
synchronous winApi.getStartFullscreen() getter invocation before wrapping its
result in a promise, ensuring bridge exceptions are caught within the settings
hydration flow. Preserve the existing handling for successful values and apply
the same protection to the corresponding logic around lines 196-200.

const resp = await fetch('/api/settings');
const data = await resp.json();
// Null-guard the form fields: on the v3 tabbed settings page the markup is
Expand Down Expand Up @@ -167,6 +168,47 @@ export async function loadSettings() {
hwcInitSettingsUI();
}

// ── Window options (desktop-only) ────────────────────────────────────────
// Desktop-only window preferences (start-in-fullscreen, …). The whole block
// stays hidden in the plain web / Docker app; unhide + wire only when the
// feedBack-desktop bridge (window.feedBackDesktop.window) exposes the getter
// and setter. Persistence lives desktop-side because only the Electron main
// process can read the pref at window-creation time — core just proxies.
export let _windowOptionsWired = false;

export function setupWindowOptions() {
const block = document.getElementById('window-options-block');
if (!block) return;
const winApi = window.feedBackDesktop?.window;
// Per-method capability check: a partial/older bridge may expose `window`
// without this shape. Leave the block hidden rather than half-wiring it.
if (!winApi
|| typeof winApi.getStartFullscreen !== 'function'
|| typeof winApi.setStartFullscreen !== 'function') {
return;
}

block.classList.remove('hidden');

const cb = document.getElementById('setting-start-fullscreen');
if (!cb) return;

// Hydrate from the desktop-persisted value. The getter may be sync or
// async (IPC round-trip); Promise.resolve normalises both.
Promise.resolve(winApi.getStartFullscreen()).then(function (on) {
cb.checked = !!on;
}).catch(function () { /* leave unchecked on error */ });
Comment on lines +198 to +200

@coderabbitai coderabbitai Bot Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Prevent late hydration from overwriting a user toggle.

When the getter is asynchronous, the checkbox is enabled immediately. A user can change it before the getter resolves, after which cb.checked = !!on silently replaces the user’s choice. Disable the control until hydration completes or skip applying the result after a user change.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@static/js/settings.js` around lines 198 - 200, Update the getStartFullscreen
hydration flow around cb so an asynchronous result cannot overwrite a user
toggle: either disable the checkbox until the promise settles, or track user
interaction and skip applying the resolved value after a change. Preserve the
existing unchecked fallback on getter errors and ensure the control is usable
once hydration completes.


// Guard only the listener against double-binding; unhide + re-hydrate
// stay idempotent so re-entering Settings refreshes the checkbox.
if (!_windowOptionsWired) {
_windowOptionsWired = true;
cb.addEventListener('change', function () {
try { winApi.setStartFullscreen(cb.checked); } catch (_) { /* best-effort */ }
});
}
}

export const APP_UPDATE_CHANNELS = ['stable', 'rc', 'beta', 'alpha'];

export let _appUpdatesWired = false;
Expand Down
15 changes: 15 additions & 0 deletions static/v3/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,21 @@ <h3>Gameplay Settings</h3>
<a href="https://github.com/got-feedback/feedback-desktop/releases" target="_blank" rel="noopener" class="text-accent hover:text-accent-light underline">download new versions from GitHub Releases</a>.
</p>
</div>
<!-- Window options — desktop-only; setupWindowOptions() unhides. -->
<div id="window-options-block" class="hidden">
<div class="fb-srow">
<div class="fb-srow-main">
<div class="fb-srow-title">Fullscreen</div>
<div class="fb-srow-desc">Run fee[dB]ack in fullscreen mode. On macOS, changes take effect on the next launch.</div>
</div>
<div class="fb-srow-control">
<label class="fb-switch">
<input type="checkbox" id="setting-start-fullscreen">
<span class="fb-switch-track"></span>
</label>
</div>
</div>
</div>
<!-- Library folder path -->
<div class="fb-srow fb-srow-stack">
<div class="fb-srow-main">
Expand Down
Loading