feat: Add fullscreen launch setting (#1933) #2490
Conversation
|
|
WalkthroughAdds a new boolean configuration key Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Nitpick comments (1)
pkg/wconfig/defaultconfig/settings.json (1)
21-21: Consider improving placement for better organization.The setting is placed between
window:magnifiedblockblurprimarypxandwindow:magnifiedblockblursecondarypx, which groups it with magnification settings. Since this is a window behavior setting (likewindow:confirmcloseandwindow:savelastwindow), consider placing it near those settings instead.Suggested placement after line 23:
"window:confirmclose": true, "window:savelastwindow": true, +"window:fullscreenOnLaunch": false, "telemetry:enabled": true,And remove from current location:
"window:magnifiedblockblurprimarypx": 10, -"window:fullscreenOnLaunch": false, "window:magnifiedblockblursecondarypx": 2,Note: Remember to rename to
window:fullscreenonlaunchfor consistency with other window settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
emain/emain-window.ts(1 hunks)frontend/app/view/term/term-model.ts(2 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/defaultconfig/settings.json(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)schema/settings.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/term/term-model.ts (3)
frontend/util/util.ts (1)
fireAndForget(466-466)frontend/app/store/wshclientapi.ts (1)
RpcApi(572-572)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
⏰ 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). (1)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (4)
pkg/wconfig/settingsconfig.go (1)
119-119: Field type is appropriate.Using
boolinstead of*boolis correct here since the default value offalse(fullscreen disabled) aligns with the intended default behavior. This differs from settings likeWindowOpacitywherenilhas semantic meaning (use default opacity).Note: The json tag uses camelCase (
fullscreenOnLaunch), which should be changed to lowercase (fullscreenonlaunch) for consistency with other window settings, as noted in the review of metaconsts.go.schema/settings.json (1)
170-172: Schema definition is correct.The property definition follows the established pattern for boolean window settings and is placed appropriately within the window settings group.
Note: The property key should be renamed to
window:fullscreenonlaunchfor consistency with other compound window setting names, as flagged in metaconsts.go.frontend/types/gotypes.d.ts (1)
918-918: Type definition is correct.The optional boolean property follows the established pattern for window settings in the SettingsType interface.
Note: Rename to
"window:fullscreenonlaunch"?for consistency with other window setting names.frontend/app/view/term/term-model.ts (1)
28-28: Import addition is appropriate.The
fireAndForgetutility is correctly imported for the fire-and-forget RPC calls in the settings menu.
emain/emain-window.ts
Outdated
| const fullscreenOnLaunch = fullConfig?.settings["window:fullscreenOnLaunch"]; | ||
| if (fullscreenOnLaunch) { | ||
| this.once("show", () => { | ||
| this.setFullScreen(true); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Implementation is sound.
Using once("show", ...) ensures fullscreen is entered only on the first show event, which is the correct approach. The setting is evaluated at window construction time, and relaunchBrowserWindows (line 833) fetches a fresh config, so relaunches will respect updated settings.
Verify that the behavior works as expected across different scenarios:
Note: The config key should be renamed to window:fullscreenonlaunch for naming consistency.
🏁 Script executed:
#!/bin/bash
# Description: Find other window initialization or creation patterns that might bypass this logic
# Check for other places where windows are created or shown
rg -n "new WaveBrowserWindow|createBrowserWindow|\.show\(\)" --type ts -C 2
# Check for any fullscreen-related logic that might conflict
rg -n "setFullScreen|fullScreen|full-screen" --type ts -C 2Length of output: 7646
I need to verify the configuration key naming convention and check if the suggested rename is appropriate.
I need to check the config key naming patterns in the codebase to verify whether the suggestion to rename is appropriate.
Now let me check the codebase to verify the config key naming patterns used elsewhere.
Wait, I need the repository. Let me run this with the repository:
Based on my verification of the code and industry standards, I can now provide the final rewritten review comment.
Implementation is sound; remove the config key renaming suggestion.
The fullscreen logic using once("show", ...) correctly ensures the feature activates only on first display, and config refresh on relaunch respects updated settings. The window initialization pattern is consistent across all code paths.
However, the suggestion to rename "window:fullscreenOnLaunch" to "window:fullscreenonlaunch" is incorrect. JavaScript conventions favor camelCase for property and variable names, and the current key follows this standard. Removing the camelCase distinction would deviate from established JavaScript naming conventions.
[incorrect_review_comment]
🏁 Script executed:
#!/bin/bash
# Search for other config keys to verify naming convention
rg -n 'settings\[|config.*\[' --type ts | head -30Length of output: 3142
Config key naming inconsistency: rename to match codebase convention.
The config key "window:fullscreenOnLaunch" should be renamed to "window:fullscreenonlaunch" to match the codebase's established pattern. All other settings keys throughout the codebase use lowercase (e.g., "window:showmenubar", "window:savelastwindow", "window:confirmclose", "telemetry:enabled", "autoupdate:enabled"). The current key is the only one using camelCase and should be normalized.
This change needs to be applied in:
- Line 195 of
emain/emain-window.ts - Any configuration schema or defaults where this key is defined
🤖 Prompt for AI Agents
In emain/emain-window.ts around lines 195 to 200 the config key uses
"window:fullscreenOnLaunch" which is inconsistent with the codebase convention
of all-lowercase keys; change the lookup to "window:fullscreenonlaunch" and
update any configuration schema, defaults, and tests that define or reference
"window:fullscreenOnLaunch" to use "window:fullscreenonlaunch" so the key is
normalized across the project.
frontend/app/view/term/term-model.ts
Outdated
| const fullscreenOnLaunch = fullConfig?.settings?.["window:fullscreenOnLaunch"]; | ||
| fullMenu.push({ | ||
| label: "Launch On Fullscreen", | ||
| submenu: [ | ||
| { | ||
| label: "On", | ||
| type: "checkbox", | ||
| checked: fullscreenOnLaunch, | ||
| click: () => { | ||
| fireAndForget(async () => { | ||
| await RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenOnLaunch": true }); | ||
| }); | ||
| }, | ||
| }, | ||
| { | ||
| label: "Off", | ||
| type: "checkbox", | ||
| checked: !fullscreenOnLaunch, | ||
| click: () => { | ||
| fireAndForget(async () => { | ||
| await RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenOnLaunch": false }); | ||
| }); | ||
| }, | ||
| }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Global window setting placed in block-specific context menu.
This "Launch On Fullscreen" setting is a global window behavior that affects all future window launches, but it's placed in the terminal block's context menu. This creates UX confusion:
- Users accessing this from a block menu might expect it to be block-specific or terminal-specific
- The setting is not discoverable for users who don't interact with terminal blocks
- Other window-level settings (like "Native Title Bar", "Show Menu Bar") are typically in application preferences
Consider moving this to a more appropriate location such as:
- Application-level settings/preferences
- Window management menu
- System/global settings section
Additionally, simplify the click handlers—the async wrapper is unnecessary:
click: () => {
- fireAndForget(async () => {
- await RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenOnLaunch": true });
- });
+ fireAndForget(() => RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenOnLaunch": true }));
},Apply the same simplification to the "Off" option (lines 740-742).
Note: Rename the config key to window:fullscreenonlaunch for consistency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/app/view/term/term-model.ts around lines 721 to 746, this block adds
a global window setting ("Launch On Fullscreen") into a terminal block context
menu which is confusing and inconsistent; move this menu item out of the
block-specific context menu and add it instead to the application-level settings
or the window management/global settings menu, rename the config key from
"window:fullscreenOnLaunch" to "window:fullscreenonlaunch" for consistency, and
simplify the click handlers by removing the fireAndForget wrapper and the
unnecessary async arrow functions—call RpcApi.SetConfigCommand(TabRpcClient, {
"window:fullscreenonlaunch": true/false }) directly in both the On and Off
handlers.
pkg/wconfig/metaconsts.go
Outdated
| ConfigKey_WidgetShowHelp = "widget:showhelp" | ||
|
|
||
| ConfigKey_WindowClear = "window:*" | ||
| ConfigKey_WindowFullscreenOnLaunch = "window:fullscreenOnLaunch" |
There was a problem hiding this comment.
Inconsistent naming convention for config key.
The key uses camelCase (fullscreenOnLaunch), but other compound window settings use all lowercase without camelCase:
window:confirmclosewindow:savelastwindowwindow:magnifiedblocksizewindow:magnifiedblockblurprimarypx
For consistency, consider renaming to window:fullscreenonlaunch across all files in this PR.
Apply this diff to fix the naming:
- ConfigKey_WindowFullscreenOnLaunch = "window:fullscreenOnLaunch"
+ ConfigKey_WindowFullscreenOnLaunch = "window:fullscreenonlaunch"Note: This change must be applied consistently across all files: settingsconfig.go (json tag), defaultconfig/settings.json, schema/settings.json, gotypes.d.ts, emain-window.ts, and term-model.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ConfigKey_WindowFullscreenOnLaunch = "window:fullscreenOnLaunch" | |
| ConfigKey_WindowFullscreenOnLaunch = "window:fullscreenonlaunch" |
🤖 Prompt for AI Agents
In pkg/wconfig/metaconsts.go around line 73, the config key uses camelCase
"window:fullscreenOnLaunch" which is inconsistent with other window keys; rename
it to "window:fullscreenonlaunch" and update all references; change the json tag
in settingsconfig.go, replace occurrences in defaultconfig/settings.json and
schema/settings.json, update the TypeScript declaration in gotypes.d.ts, and fix
usages in emain-window.ts and term-model.ts so the new lowercase key is used
everywhere in this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/view/term/term-model.ts (1)
721-742: Global window setting placed in block-specific context menu (previously flagged).This setting controls application-wide window behavior on launch but is accessed through a terminal block's context menu, creating UX confusion as noted in the previous review. Users might expect block-specific behavior, and the setting is not discoverable outside terminal interactions.
🧹 Nitpick comments (1)
frontend/app/view/term/term-model.ts (1)
730-730: Consider removingfireAndForgetwrapper for consistency.Other similar click handlers in this file (lines 593, 604, 615, 629, 642, 674, 685, 702, 713) call
RpcApi.SetMetaCommandorRpcApi.SetConfigCommanddirectly without wrapping infireAndForget. For consistency with the existing patterns:click: () => { - fireAndForget(() => RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenonlaunch": true })); + RpcApi.SetConfigCommand(TabRpcClient, { "window:fullscreenonlaunch": true }); },Apply the same simplification to line 738.
Also applies to: 738-738
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
emain/emain-window.ts(1 hunks)frontend/app/view/term/term-model.ts(2 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/wconfig/defaultconfig/settings.json(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)schema/settings.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/wconfig/metaconsts.go
- frontend/types/gotypes.d.ts
- pkg/wconfig/defaultconfig/settings.json
- emain/emain-window.ts
- schema/settings.json
- pkg/wconfig/settingsconfig.go
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/view/term/term-model.ts (3)
frontend/util/util.ts (1)
fireAndForget(466-466)frontend/app/store/wshclientapi.ts (1)
RpcApi(572-572)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
|
@Mostafa-Khairy0 thanks for the submission! I think this context menu item would be better in the main wave menu (see emain-menu.ts) in the "View" menu rather than in the terminal context menu. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emain/emain-menu.ts (1)
295-315: WrapSetConfigCommandcalls infireAndForgetfor consistency.The click handlers call
SetConfigCommandwithout awaiting or error handling. Throughout this file, async operations in click handlers are wrapped withfireAndForget(see lines 51, 85, 98, 107, 114). This pattern ensures errors are caught and logged.Apply this diff:
{ label: "On", type: "radio", checked: fullscreenOnLaunch, click: () => { - RpcApi.SetConfigCommand(ElectronWshClient, { "window:fullscreenonlaunch": true }); + fireAndForget(() => RpcApi.SetConfigCommand(ElectronWshClient, { "window:fullscreenonlaunch": true })); }, }, { label: "Off", type: "radio", checked: !fullscreenOnLaunch, click: () => { - RpcApi.SetConfigCommand(ElectronWshClient, { "window:fullscreenonlaunch": false }); + fireAndForget(() => RpcApi.SetConfigCommand(ElectronWshClient, { "window:fullscreenonlaunch": false })); }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emain/emain-menu.ts(2 hunks)frontend/app/view/term/term-model.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/view/term/term-model.ts
🧰 Additional context used
🧬 Code graph analysis (1)
emain/emain-menu.ts (2)
frontend/app/store/wshclientapi.ts (1)
RpcApi(572-572)emain/emain-wsh.ts (1)
ElectronWshClient(74-74)
3 participants
New UI Setting: Launch On Fullscreen
A new option has been added to the block context menu allowing users to enable fullscreen mode automatically when Wave launches. This setting is persistent and can be toggled on a per-user basis.
How to Enable:
1. Right-click on any tile/block.
2. Open the settings menu.
3. Select Launch On Fullscreen → On.