Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional persistent web-link support: settings, renderer store and UI, IPC/preload changes, web-server constructor and factory to accept/reuse a security token, plus tests to cover persistence, generation, and storage of the token. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as LiveOverlayPanel
participant RStore as Renderer Store
participant Preload as Preload (window.maestro.live)
participant IPC as Main IPC Handler
participant Server as WebServer
participant SStore as Settings Store
UI->>RStore: setPersistentWebLink(true)
RStore->>Preload: persistCurrentToken()
Preload->>IPC: invoke "live:persistCurrentToken"
IPC->>Server: getSecurityToken()
Server-->>IPC: token
IPC->>SStore: set('webAuthToken', token)
SStore-->>RStore: persisted confirmation
UI->>RStore: setPersistentWebLink(false)
RStore->>SStore: set('webAuthToken', null)
Note over SStore,IPC: On startup
SStore-->>IPC: get('webAuthToken')
IPC->>Server: new WebServer(port, token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR adds a "Persistent Web Link" toggle to the Live overlay panel, allowing users to lock in their web server's security token across app restarts. The core mechanics are well-structured — the factory picks up the stored token on startup, the Key findings:
Confidence Score: 3/5
Last reviewed commit: 32f8800 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/renderer/components/SessionList/LiveOverlayPanel.tsx (1)
198-207: Consider providing user feedback when token persistence fails.Based on the settings store implementation in
src/renderer/stores/settingsStore.ts, thesetPersistentWebLinkaction callspersistCurrentToken()without awaiting or checking its return value. If the web server is not running when the user enables this toggle, the IPC call returns{ success: false, message: 'Web server is not running.' }but this failure is silently ignored.The toggle will show as enabled, but no token will actually be persisted until the server is manually started. Consider either:
- Disabling this toggle when the server is not running (similar to the cloudflared toggle pattern)
- Awaiting the
persistCurrentToken()result and showing a toast/warning if it fails🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList/LiveOverlayPanel.tsx` around lines 198 - 207, The persistent-web-link toggle in LiveOverlayPanel.tsx currently flips UI state via setPersistentWebLink without handling persistCurrentToken() failures; update the toggle handler inside the button (the onClick that calls setPersistentWebLink) to either (a) disable the control when the server is down by checking the same server-running state used for the cloudflared toggle, or (b) make setPersistentWebLink await the settingsStore.persistCurrentToken() result and, on failure (response.success === false), revert the toggle state and surface a user-visible notification (toast/warning) with the response.message; reference the setPersistentWebLink action and settingsStore.persistCurrentToken() when implementing this change.src/__tests__/main/web-server/web-server-factory.test.ts (1)
233-247: Make the generated-token assertion deterministic.This still passes if the factory stores one token and passes a different token to
WebServer, because it only checks “some string” on both sides. MockrandomUUID()or compare the persisted value directly toserver.securityTokenso the test actually pins the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/web-server/web-server-factory.test.ts` around lines 233 - 247, The test for createWebServerFactory is non-deterministic because it only asserts that both persisted and in-memory tokens are strings; either mock the UUID generator or assert equality between the stored token and the server instance token to pin the contract: update the test that calls createWebServerFactory/deps so that it either stubs crypto.randomUUID() (or whatever randomUUID helper is used) to return a fixed value and assert mockSettingsStore.set was called with that fixed value and that (server as any).securityToken equals it, or capture the argument passed to mockSettingsStore.set and assert it strictly equals (server as any).securityToken; reference createWebServerFactory, (server as any).securityToken, mockSettingsStore.set and randomUUID in your changes.
🤖 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/main/ipc/handlers/web.ts`:
- Around line 273-281: The handler for 'live:persistCurrentToken' currently
rejects persistence when webServer.isActive() is false, which blocks valid
tokens created in the WebServer constructor during startup; change the logic to
persist whenever a WebServer instance exists (check getWebServer() != null)
instead of requiring isActive(), or mirror the approach used in live:toggle by
awaiting the server readiness before persisting (e.g., await the same
readiness/start promise or call the WebServer method used there) so that
webServer.getSecurityToken() and settingsStore.set('webAuthToken', ...) run for
instances created during startup as well.
In `@src/renderer/components/SessionList/LiveOverlayPanel.tsx`:
- Around line 216-218: The caption in LiveOverlayPanel is misleading about when
persistence occurs; update the copy to accurately reflect the actual behavior of
setPersistentWebLink and persistCurrentToken (i.e., persistence happens
immediately if the server is running). Locate the text node in
LiveOverlayPanel.tsx that currently reads "Takes effect on next server start"
and replace it with a clearer message such as "Persists current token
immediately if server is running" (or similar phrasing) so it matches the
behavior of setPersistentWebLink(true) calling persistCurrentToken().
In `@src/renderer/stores/settingsStore.ts`:
- Around line 682-690: The setter setPersistentWebLink currently writes
persistentWebLink=true before calling window.maestro.live.persistCurrentToken();
change the flow so you await the IPC call and only persist the setting on
success: call window.maestro.live.persistCurrentToken() first (await its
Promise), and if it succeeds call set({ persistentWebLink: true }) and
window.maestro.settings.set('persistentWebLink', true); if it fails either do
not change the setting or explicitly roll it back (set persistentWebLink false
and/or set('webAuthToken', null) as needed) and surface/log the error. Keep
references to setPersistentWebLink, window.maestro.live.persistCurrentToken, and
window.maestro.settings.set in your changes so the logic is easy to locate.
- Around line 299-300: getSettingsActions() currently doesn't return the newly
added setPersistentWebLink (and should mirror setWebInterfaceUseCustomPort
behavior), so update getSettingsActions() to include and return
setPersistentWebLink in its returned actions object and ensure the
implementation calls window.maestro.settings.set(...) with the correct key/value
like the other setters; also verify any wrapper callers and the useSettings.ts
useEffect still load/persist settings via window.maestro.settings.set and
window.maestro.settings.get so the new action actually persists the change.
---
Nitpick comments:
In `@src/__tests__/main/web-server/web-server-factory.test.ts`:
- Around line 233-247: The test for createWebServerFactory is non-deterministic
because it only asserts that both persisted and in-memory tokens are strings;
either mock the UUID generator or assert equality between the stored token and
the server instance token to pin the contract: update the test that calls
createWebServerFactory/deps so that it either stubs crypto.randomUUID() (or
whatever randomUUID helper is used) to return a fixed value and assert
mockSettingsStore.set was called with that fixed value and that (server as
any).securityToken equals it, or capture the argument passed to
mockSettingsStore.set and assert it strictly equals (server as
any).securityToken; reference createWebServerFactory, (server as
any).securityToken, mockSettingsStore.set and randomUUID in your changes.
In `@src/renderer/components/SessionList/LiveOverlayPanel.tsx`:
- Around line 198-207: The persistent-web-link toggle in LiveOverlayPanel.tsx
currently flips UI state via setPersistentWebLink without handling
persistCurrentToken() failures; update the toggle handler inside the button (the
onClick that calls setPersistentWebLink) to either (a) disable the control when
the server is down by checking the same server-running state used for the
cloudflared toggle, or (b) make setPersistentWebLink await the
settingsStore.persistCurrentToken() result and, on failure (response.success ===
false), revert the toggle state and surface a user-visible notification
(toast/warning) with the response.message; reference the setPersistentWebLink
action and settingsStore.persistCurrentToken() when implementing this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1276a05-5aa2-4eb1-86c1-7c5d829592f7
📒 Files selected for processing (13)
src/__tests__/main/web-server/web-server-factory.test.tssrc/main/index.tssrc/main/ipc/handlers/web.tssrc/main/preload/web.tssrc/main/stores/defaults.tssrc/main/stores/types.tssrc/main/web-server/WebServer.tssrc/main/web-server/web-server-factory.tssrc/renderer/components/SessionList/LiveOverlayPanel.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/components/SettingCheckbox.tsxsrc/renderer/global.d.tssrc/renderer/stores/settingsStore.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/renderer/stores/settingsStore.ts (2)
299-299:⚠️ Potential issue | 🟡 MinorReturn
setPersistentWebLinkfromgetSettingsActions().Line 299 adds the action to the public store contract, but the non-React helper still skips it. Any caller using
getSettingsActions()can't toggle this setting yet. Based on learnings: "Ensure settings persist by verifying wrapper functions call window.maestro.settings.set() and loading code in useSettings.ts useEffect".Suggested fix
setLeaderboardRegistration: state.setLeaderboardRegistration, + setPersistentWebLink: state.setPersistentWebLink, setWebInterfaceUseCustomPort: state.setWebInterfaceUseCustomPort,Also applies to: 1817-1819
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/settingsStore.ts` at line 299, The public actions returned by getSettingsActions() currently omit setPersistentWebLink, so callers cannot toggle that setting; update getSettingsActions() to include setPersistentWebLink in its returned object and ensure the implementation calls the wrapper that persists the change (invoke window.maestro.settings.set(...) or the existing settingsStore setter used elsewhere). Also verify the helper/wrapper used by setPersistentWebLink mirrors other setters (loads current settings and writes the updated value) and that useSettings.ts useEffect still reads the persisted value so the UI reflects changes.
682-690:⚠️ Potential issue | 🟠 MajorPersist the token successfully before flipping the toggle on.
src/main/ipc/handlers/web.ts:269-280returns{ success: false }when the server is not running, andsettings:setreturnsfalseon persistence failures (src/main/ipc/handlers/persistence.ts:49-84). Because Lines 683-684 set and persistpersistentWebLinkbefore that result is checked, the toggle can stay ON even though no token was saved. Mirror the rollback pattern used insetPreventSleepEnabled: await/check the IPC results first, then commit local state. The off path also fire-and-forgets both writes, so rapid toggles can still interleavepersistentWebLinkandwebAuthTokenpersistence.Suggested fix
setPersistentWebLink: async (value) => { - set({ persistentWebLink: value }); - window.maestro.settings.set('persistentWebLink', value); - if (value) { - await window.maestro.live.persistCurrentToken(); - } else { - window.maestro.settings.set('webAuthToken', null); - } + const prev = get().persistentWebLink; + try { + if (value) { + const result = await window.maestro.live.persistCurrentToken(); + if (!result.success) { + throw new Error(result.message ?? 'Failed to persist current web token'); + } + const saved = await window.maestro.settings.set('persistentWebLink', true); + if (!saved) { + throw new Error('Failed to persist persistent web link state'); + } + set({ persistentWebLink: true }); + } else { + const cleared = await window.maestro.settings.set('webAuthToken', null); + const saved = await window.maestro.settings.set('persistentWebLink', false); + if (!cleared || !saved) { + throw new Error('Failed to clear persistent web link state'); + } + set({ persistentWebLink: false }); + } + } catch (error) { + set({ persistentWebLink: prev }); + throw error; + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/settingsStore.ts` around lines 682 - 690, The current setPersistentWebLink flips local state and then persists which can leave the toggle ON if persistence fails; change it to first call and await the IPC persistence functions (window.maestro.live.persistCurrentToken and window.maestro.settings.set for 'persistentWebLink'/'webAuthToken'), check their boolean success results, and only update local state via set({ persistentWebLink: ... }) when persistence succeeded (mirror the rollback pattern used in setPreventSleepEnabled). For the "off" path, await both settings.set calls (clear webAuthToken and persistentWebLink) instead of fire-and-forget to avoid interleaving, and if any persistence fails, ensure local state remains consistent with the persisted outcome (revert or leave unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/stores/settingsStore.ts`:
- Line 299: The public actions returned by getSettingsActions() currently omit
setPersistentWebLink, so callers cannot toggle that setting; update
getSettingsActions() to include setPersistentWebLink in its returned object and
ensure the implementation calls the wrapper that persists the change (invoke
window.maestro.settings.set(...) or the existing settingsStore setter used
elsewhere). Also verify the helper/wrapper used by setPersistentWebLink mirrors
other setters (loads current settings and writes the updated value) and that
useSettings.ts useEffect still reads the persisted value so the UI reflects
changes.
- Around line 682-690: The current setPersistentWebLink flips local state and
then persists which can leave the toggle ON if persistence fails; change it to
first call and await the IPC persistence functions
(window.maestro.live.persistCurrentToken and window.maestro.settings.set for
'persistentWebLink'/'webAuthToken'), check their boolean success results, and
only update local state via set({ persistentWebLink: ... }) when persistence
succeeded (mirror the rollback pattern used in setPreventSleepEnabled). For the
"off" path, await both settings.set calls (clear webAuthToken and
persistentWebLink) instead of fire-and-forget to avoid interleaving, and if any
persistence fails, ensure local state remains consistent with the persisted
outcome (revert or leave unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9ece753-ebc8-4a84-90ff-642812bb8caa
📒 Files selected for processing (5)
src/main/ipc/handlers/web.tssrc/main/preload/web.tssrc/renderer/components/SessionList/LiveOverlayPanel.tsxsrc/renderer/global.d.tssrc/renderer/stores/settingsStore.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/preload/web.ts
- src/main/ipc/handlers/web.ts
- src/renderer/global.d.ts
- src/renderer/components/SessionList/LiveOverlayPanel.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/stores/settingsStore.ts (1)
682-695:⚠️ Potential issue | 🟠 MajorPersist the token before saving
persistentWebLink=true.This still commits
persistentWebLinkbeforepersistCurrentToken()finishes. If the app quits or crashes in that gap, the next startup sees the flag enabled but no saved current token and generates a new URL, so the “lock current URL immediately” guarantee is still broken.Suggested ordering change
setPersistentWebLink: async (value) => { - set({ persistentWebLink: value }); - window.maestro.settings.set('persistentWebLink', value); if (value) { try { const result = await window.maestro.live.persistCurrentToken(); - if (!result.success) { - set({ persistentWebLink: false }); - window.maestro.settings.set('persistentWebLink', false); - } - } catch { - set({ persistentWebLink: false }); - window.maestro.settings.set('persistentWebLink', false); + if (!result.success) { + set({ persistentWebLink: false }); + window.maestro.settings.set('persistentWebLink', false); + return; + } + set({ persistentWebLink: true }); + window.maestro.settings.set('persistentWebLink', true); + } catch { + set({ persistentWebLink: false }); + window.maestro.settings.set('persistentWebLink', false); + return; } } else { + set({ persistentWebLink: false }); + window.maestro.settings.set('persistentWebLink', false); window.maestro.settings.set('webAuthToken', null); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/settingsStore.ts` around lines 682 - 695, The code sets persistentWebLink before persistCurrentToken completes, risking a saved flag without a persisted token; change setPersistentWebLink to first call window.maestro.live.persistCurrentToken() and await its result, and only on success call set({ persistentWebLink: true }) and window.maestro.settings.set('persistentWebLink', true); if persistCurrentToken fails or throws, ensure you call set({ persistentWebLink: false }) and window.maestro.settings.set('persistentWebLink', false) (and do not set the flag prior to awaiting), keeping the existing error handling around window.maestro.live.persistCurrentToken to revert state on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/stores/settingsStore.ts`:
- Around line 682-695: The code sets persistentWebLink before
persistCurrentToken completes, risking a saved flag without a persisted token;
change setPersistentWebLink to first call
window.maestro.live.persistCurrentToken() and await its result, and only on
success call set({ persistentWebLink: true }) and
window.maestro.settings.set('persistentWebLink', true); if persistCurrentToken
fails or throws, ensure you call set({ persistentWebLink: false }) and
window.maestro.settings.set('persistentWebLink', false) (and do not set the flag
prior to awaiting), keeping the existing error handling around
window.maestro.live.persistCurrentToken to revert state on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2a7482f-69b1-4ea1-9f85-d536d23f584b
📒 Files selected for processing (2)
src/main/ipc/handlers/web.tssrc/renderer/stores/settingsStore.ts
There was a problem hiding this comment.
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/renderer/stores/settingsStore.ts`:
- Around line 682-698: The enable path in setPersistentWebLink can complete
out-of-order and re-enable persistentWebLink after the user turned it off;
update setPersistentWebLink to ignore stale async completions by stamping each
call with a monotonic request id (or cancellation token) captured before calling
window.maestro.live.persistCurrentToken and stored in a module-level variable,
then after the await verify the captured id still matches the latest request id
before calling set(...) and window.maestro.settings.set(...); reference the
setPersistentWebLink function, window.maestro.live.persistCurrentToken, set, and
window.maestro.settings.set when implementing the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d26ea1d2-fabb-4d66-8d36-505572fea4ab
📒 Files selected for processing (1)
src/renderer/stores/settingsStore.ts
There was a problem hiding this comment.
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/renderer/stores/settingsStore.ts`:
- Around line 685-701: The stale-enable race can re-persist webAuthToken after a
user quickly disables; update setPersistentWebLink so that when an in-flight
persistCurrentToken() finishes but its requestSeq no longer matches
persistentWebLinkRequestSeq you explicitly clear the token in main before
returning (i.e., in the branch inside setPersistentWebLink where you currently
do "if (requestSeq !== persistentWebLinkRequestSeq) return;", call
window.maestro.settings.set('webAuthToken', null) then return). This ensures any
late/stale enable result cannot leave a persisted token behind; keep references
to persistentWebLinkRequestSeq and persistCurrentToken in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c15034a-5008-4e4a-82df-3fd8dbba5b3a
📒 Files selected for processing (1)
src/renderer/stores/settingsStore.ts
jSydorowicz21
force-pushed
the
feat/persistent-web-link
branch
7 times, most recently
from
ef3b686 to
4b750d3
Compare
Collaborator
|
This feels risky as default behavior though. With no auth, that key is what gives someone full access to your system. Can you paste some screenshots of what this looks like? Maybe we add a default off toggle for "persitent key" just like we have for custom port |
jSydorowicz21
force-pushed
the
feat/persistent-web-link
branch
4 times, most recently
from
aa7488b to
45c0ec7
Compare
Author
Heyo! I was still working on this, the default is off since it's better to play safe for those who may expose their instance to the public. It's set in src/main/stores/defaults.ts as persistentWebLink: false 
|
jSydorowicz21
force-pushed
the
feat/persistent-web-link
branch
2 times, most recently
from
2cbf3a5 to
9c130c7
Compare
Add persistentWebLink setting that preserves the web server's security token across restarts, keeping the access URL stable. Implementation: - WebServer constructor accepts optional securityToken parameter - web-server-factory validates stored tokens against UUID v4 regex, auto-regenerates on invalid/missing with graceful degradation - Two IPC handlers (persistCurrentToken / clearPersistentToken) with crash-safe write ordering and best-effort rollback on disk errors - Shared SettingsStoreInterface extracted to stores/types.ts - Renderer store action with optimistic UI, stale-request sequence counter inside Zustand closure, and rollback on soft/hard failures - LiveOverlayPanel toggle with isPersistPending guard and aria-busy Note: silent token regeneration notification intentionally omitted
- IPC handler tests: persistCurrentToken (success, null/inactive server, disk error), clearPersistentToken (crash-safe write order, disk error) - Factory tests: persistent-disabled, valid UUID reuse, invalid UUID rejection with v4 regex validation, null token fresh generation - Renderer store tests: optimistic enable/disable, soft/hard IPC failure rollback, rapid double-toggle (enable→disable and disable→enable) - Test setup: add live namespace mock with persistCurrentToken and clearPersistentToken
jSydorowicz21
force-pushed
the
feat/persistent-web-link
branch
from
9c130c7 to
e6d529b
Compare
Author
|
@pedramamini |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a toggle in the Live overlay panel that lets users lock in their
current web server URL. When enabled, the security token is saved to
settings and reused on future startups instead of generating a fresh
one each time. Handy for anyone sharing the link on a local network
who doesn't want it to break every time Maestro restarts.
Default state is off
Toggling ON immediately persists the running server's token (no restart
needed). Toggling OFF clears the stored token so a fresh one is
generated next time.
Summary by CodeRabbit
New Features
Accessibility
Settings
Tests