feat(v3): add desktop-only "Start in fullscreen" system option - #938
Conversation
Adds a "Start in fullscreen" toggle to the Settings → System panel,
addressing the desktop request in feedBack-desktop#97: users want the
app to launch fullscreen without hitting the OS hotkey every time.
The block ships hidden and is gated exactly like the App-updates block:
setupWindowOptions() only unhides + wires it when the feedBack-desktop
bridge exposes window.feedBackDesktop.window.{getStartFullscreen,
setStartFullscreen}. Web/Docker builds have no such bridge, so the
section never appears there. Persistence lives desktop-side because
only the Electron main process can read the pref at window-creation
time — core just proxies through the bridge.
The desktop bridge + launch behaviour land in a follow-up
feedBack-desktop PR.
Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a desktop-only “Window options” settings block with a “Start in fullscreen” checkbox. Settings initialization hydrates the checkbox through the desktop bridge and persists changes with capability checks, error handling, and idempotent listener wiring. ChangesWindow options
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant Settings
participant DOM
participant DesktopBridge
App->>Settings: loadSettings()
Settings->>Settings: setupWindowOptions()
Settings->>DOM: Reveal supported window options
Settings->>DesktopBridge: getStartFullscreen()
DesktopBridge-->>Settings: Return fullscreen state
Settings->>DOM: Set checkbox state
DOM->>Settings: Notify checkbox change
Settings->>DesktopBridge: setStartFullscreen()
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@static/js/settings.js`:
- Around line 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.
- 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.
In `@static/v3/index.html`:
- Around line 760-766: Give the checkbox with id setting-start-fullscreen an
accessible name by associating the visible “Start in fullscreen” title via a
matching label for attribute, or by adding an equivalent aria-label to the
input; keep the existing switch styling and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e07d455d-7653-4525-b8c1-462ec60ef122
📒 Files selected for processing (3)
static/app.jsstatic/js/settings.jsstatic/v3/index.html
| // failed fetch below still leaves the desktop updater wired up. | ||
| // setupAppUpdates() is idempotent via _appUpdatesWired. | ||
| setupAppUpdates(); | ||
| setupWindowOptions(); |
There was a problem hiding this comment.
🩺 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.
| Promise.resolve(winApi.getStartFullscreen()).then(function (on) { | ||
| cb.checked = !!on; | ||
| }).catch(function () { /* leave unchecked on error */ }); |
There was a problem hiding this comment.
🎯 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.
| <div class="fb-srow-title">Start in fullscreen</div> | ||
| <div class="fb-srow-desc">Launch the desktop app in fullscreen every time.</div> | ||
| </div> | ||
| <div class="fb-srow-control"> | ||
| <label class="fb-switch"> | ||
| <input type="checkbox" id="setting-start-fullscreen"> | ||
| <span class="fb-switch-track"></span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give the checkbox an accessible name.
The visible title is a sibling of the <label>, while the label contains only the switch track. Screen readers therefore may announce an unnamed checkbox. Associate the title with the input using for/id, or add aria-label="Start in fullscreen" (and optionally aria-describedby for the description).
🤖 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/v3/index.html` around lines 760 - 766, Give the checkbox with id
setting-start-fullscreen an accessible name by associating the visible “Start in
fullscreen” title via a matching label for attribute, or by adding an equivalent
aria-label to the input; keep the existing switch styling and behavior
unchanged.
Retitle the toggle "Fullscreen" (from "Start in fullscreen") and reword the description to "Run fee[dB]ack in fullscreen mode. On macOS, changes take effect on the next launch." The macOS note is honest about a native-fullscreen limitation: AppKit drops the first programmatic fullscreen-enter on a window created windowed, so on macOS the desktop side applies the pref at next launch rather than live. Windows/Linux apply it live on the first toggle. The note is self-scoping text (no platform-detection code needed). Signed-off-by: gionnibgud <gionnibgud@gmail.com>
|
Companion desktop PR: got-feedBack/feedBack-desktop#105 — adds the |
|
Thanks @coderabbitai — evaluated all three; skipping each, with rationale: 1. Sync getter throw ( 2. Late hydration overwriting a user toggle — skip. The race window is a single IPC round-trip on Settings entry, and the toggle is its own sole writer — the 3. Checkbox accessible name — valid, but out of scope here. The finding is correct, but it's a page-wide pattern: every sibling toggle in Settings ( Net: this PR stays consistent with the established settings patterns; no changes from this review. |
What
Adds a Start in fullscreen toggle to Settings → System (between App updates and Library folder path).
Addresses the desktop request in got-feedBack/feedBack-desktop#97 — users want the app to launch fullscreen without pressing the OS hotkey (⌃⌘F) every time.
How it stays desktop-only
The block ships
hiddenand is gated exactly like the existing App-updates block.setupWindowOptions()(instatic/js/settings.js, called fromloadSettings()) only unhides + wires it when the feedBack-desktop bridge exposes:feedBackDesktopbridge → the function returns early → the section never appears.Persistence lives desktop-side because only the Electron main process can read the pref at window-creation time; core just proxies through the bridge (getter normalised via
Promise.resolveto allow sync or async IPC).Scope
Core UI + bridge proxy only. The desktop bridge (
feedBackDesktop.window) and the actual launch-in-fullscreen behaviour land in a follow-up feedBack-desktop PR. Until then this is inert in every shipping build.Testing
#window-options-blockis present in the DOM butdisplay:none(no bridge) — nothing new renders. ManuallyclassList.remove('hidden')in devtools confirms the row layout.tailwind.min.cssrebuild.🤖 Generated with Claude Code
Summary by CodeRabbit