fix(settings): enhance navigation handling and normalize payload stru… - #1513
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes enhance settings window navigation by introducing URL-based navigation interception, message queuing, and payload validation. The main process now validates navigation requests early, computes appropriate target URLs, and can reload the settings window with queued messages before the window fully initializes. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Process<br/>(windowPresenter)
participant Validate as Payload<br/>Validation
participant Window as Settings<br/>Window
participant Queue as Message<br/>Queue
Main->>Validate: tryNavigateSettingsWindowByUrl(args)
Validate->>Validate: toSettingsNavigationPayload(args[0])
alt Valid & matches 'settings-provider'
Validate->>Main: payload extracted
Main->>Main: getSettingsWindowTargetUrl(navigation)
Main->>Window: loadURL(targetUrl)
activate Window
Window-->>Main: success/failure
deactivate Window
Main->>Queue: queue {channel, args:[navigation]}
else Invalid or filtered out
Validate-->>Main: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/presenter/windowPresenter/index.ts (2)
16-19:⚠️ Potential issue | 🟡 MinorValidate
routeNamebefore casting it toSettingsNavigationPayload.Line 1544 accepts any string and then casts it to the route union, so malformed IPC payloads can still be queued/sent as “typed” navigation. Reject route names that are not in the shared settings navigation registry.
🛡️ Proposed fix
import { + SETTINGS_NAVIGATION_ITEMS, resolveSettingsNavigationPath, type SettingsNavigationPayload } from '@shared/settingsNavigation' @@ - if (typeof candidate.routeName !== 'string') { + if ( + typeof candidate.routeName !== 'string' || + !SETTINGS_NAVIGATION_ITEMS.some((item) => item.routeName === candidate.routeName) + ) { return null }Also applies to: 1544-1564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/windowPresenter/index.ts` around lines 16 - 19, The handler currently casts an arbitrary string variable routeName to SettingsNavigationPayload and enqueues/sends it without validation; change that code to validate routeName against the shared registry before casting by calling resolveSettingsNavigationPath(routeName) (or checking membership in the registry it uses) and ensure the result is non-undefined/valid; if resolveSettingsNavigationPath returns nothing (or the name is not in the allowed set) then reject/log and return early and do not construct/cast the SettingsNavigationPayload or enqueue the navigation. Update the block that creates the SettingsNavigationPayload (the variable named routeName and the cast to SettingsNavigationPayload) to perform this runtime check and handle invalid input safely.
1479-1489:⚠️ Potential issue | 🟠 MajorHandle stale pending messages after hash-only URL navigations
When
tryNavigateSettingsWindowByUrlloads a hash-only URL (same base path, different hash), Electron triggersdid-start-navigationwithisSameDocument=true, which causeshandleSettingsWindowNavigationStartto return early. Since same-document navigations don't reload the renderer, no READY event is emitted. The message queued beforeloadURLremains pending and can replay against stale state later.Ensure pending messages are flushed when the window was already ready before a same-document navigation, and clean up orphaned messages on load failure.
🔁 Proposed guard
this.pendingSettingsMessages.push({ channel, args: [navigation] }) + const wasReady = this.settingsWindowReady console.log(`Reloading settings window to target URL: ${targetUrl}`) console.info('[Startup][Settings][Main] loadURL start', targetUrl) void this.settingsWindow.webContents .loadURL(targetUrl) .then(() => { @@ + if (wasReady && this.settingsWindowReady) { + this.flushPendingSettingsMessages() + } + console.info( `[Startup][Settings][Main] loadURL end windowId=${this.settingsWindow.id} target=${targetUrl}` ) }) .catch((error) => { + this.pendingSettingsMessages = this.pendingSettingsMessages.filter( + (message) => message.channel !== channel || message.args[0] !== navigation + ) console.error(`Failed to reload settings window for navigation: ${targetUrl}`, error) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/windowPresenter/index.ts` around lines 1479 - 1489, When handleSettingsWindowNavigationStart sees a same-document navigation (isSameDocument === true) for the active settings window, if settingsWindowReady was true before navigation you must flush/clear any queued pending settings messages (the same logic the READY event handler uses to dispatch queued messages) and mark the window as not ready so messages do not replay against stale renderer state; likewise, in tryNavigateSettingsWindowByUrl ensure that on load failure (the promise rejection or navigation error path after calling loadURL) you also clear the pending-queue to avoid orphaned messages. Locate the queued message handling code that runs when the READY event is received and invoke or reuse that to flush/clear the queue from handleSettingsWindowNavigationStart and the loadURL error path, updating settingsWindowReady appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/presenter/windowPresenter/index.ts`:
- Around line 16-19: The handler currently casts an arbitrary string variable
routeName to SettingsNavigationPayload and enqueues/sends it without validation;
change that code to validate routeName against the shared registry before
casting by calling resolveSettingsNavigationPath(routeName) (or checking
membership in the registry it uses) and ensure the result is
non-undefined/valid; if resolveSettingsNavigationPath returns nothing (or the
name is not in the allowed set) then reject/log and return early and do not
construct/cast the SettingsNavigationPayload or enqueue the navigation. Update
the block that creates the SettingsNavigationPayload (the variable named
routeName and the cast to SettingsNavigationPayload) to perform this runtime
check and handle invalid input safely.
- Around line 1479-1489: When handleSettingsWindowNavigationStart sees a
same-document navigation (isSameDocument === true) for the active settings
window, if settingsWindowReady was true before navigation you must flush/clear
any queued pending settings messages (the same logic the READY event handler
uses to dispatch queued messages) and mark the window as not ready so messages
do not replay against stale renderer state; likewise, in
tryNavigateSettingsWindowByUrl ensure that on load failure (the promise
rejection or navigation error path after calling loadURL) you also clear the
pending-queue to avoid orphaned messages. Locate the queued message handling
code that runs when the READY event is received and invoke or reuse that to
flush/clear the queue from handleSettingsWindowNavigationStart and the loadURL
error path, updating settingsWindowReady appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cdb9bab-8979-40c5-9e63-6e9b0e167367
📒 Files selected for processing (2)
src/main/presenter/windowPresenter/index.tssrc/renderer/api/SettingsClient.ts
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.
…cture
Summary by CodeRabbit
Bug Fixes
Improvements