feat: updater runtime flags and Sonar quality cleanup#42
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Updater-related types (e.g., UpdaterStatus, UpdateCheckResult, UpdaterChannel/State) are now defined both in src/types/ipc.ts and src/main/updater.ts; consider consolidating these into a single shared definition to avoid divergence between main and renderer contracts.
- There is repeated error/boolean/env parsing logic (e.g., TRUE_VALUES + parseBooleanEnv, getErrorMessage) across main/index.ts, src/main/updater.ts, and src/main/feature-flags.ts; extracting these into a small shared utility would reduce duplication and keep behavior consistent.
- The assets protocol fallback (createForbiddenAssetResponse) returns a plain object cast as Response when Response is not defined, but protocol.handle expects a real Fetch Response; it might be more robust to ensure a proper Response implementation is always available in the main process (or to avoid the casted object path entirely).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Updater-related types (e.g., UpdaterStatus, UpdateCheckResult, UpdaterChannel/State) are now defined both in src/types/ipc.ts and src/main/updater.ts; consider consolidating these into a single shared definition to avoid divergence between main and renderer contracts.
- There is repeated error/boolean/env parsing logic (e.g., TRUE_VALUES + parseBooleanEnv, getErrorMessage) across main/index.ts, src/main/updater.ts, and src/main/feature-flags.ts; extracting these into a small shared utility would reduce duplication and keep behavior consistent.
- The assets protocol fallback (createForbiddenAssetResponse) returns a plain object cast as Response when Response is not defined, but protocol.handle expects a real Fetch Response; it might be more robust to ensure a proper Response implementation is always available in the main process (or to avoid the casted object path entirely).
## Individual Comments
### Comment 1
<location> `src/main/index.ts:523` </location>
<code_context>
-const getErrorMessage = (error: unknown): string =>
- error instanceof Error ? error.message : String(error);
+function getErrorMessage(error: unknown): string {
+ if (error instanceof Error) {
+ return error.message;
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating `getErrorMessage` logic between main/index and main/updater.
A near-identical `getErrorMessage` now exists here and in `src/main/updater.ts`, which increases the chance they diverge over time. Please extract this into a shared utility and use it from both places to keep error handling consistent and easier to maintain.
Suggested implementation:
```typescript
```
To fully implement the suggestion and avoid duplication:
1. Create a shared utility in `src/main/errors.ts`:
```ts
// src/main/errors.ts
export function getErrorMessage(error: unknown): string {
if (error instanceof Error) {
return error.message;
}
if (typeof error === 'object' && error !== null) {
try {
return JSON.stringify(error);
} catch {
return '[unserializable error object]';
}
}
return String(error);
}
```
2. In `src/main/index.ts`, add an import near the top of the file (alongside other imports):
```ts
import { getErrorMessage } from './errors';
```
3. In `src/main/updater.ts`, remove its local `getErrorMessage` implementation (similar to the change above) and import the shared utility:
```ts
import { getErrorMessage } from './errors';
```
These changes centralize the error handling logic, ensuring `index.ts` and `updater.ts` stay consistent and easier to maintain.
</issue_to_address>
### Comment 2
<location> `src/main/updater.ts:3-12` </location>
<code_context>
+import type { AppUpdater } from 'electron-updater';
+
+export type UpdaterChannel = 'alpha' | 'stable';
+export type UpdaterState = 'disabled' | 'up-to-date' | 'update-available' | 'error';
+
+export interface UpdaterStatus {
+ enabled: boolean;
+ platformSupported: boolean;
+ channel: UpdaterChannel;
+ allowPrerelease: boolean;
+ currentVersion: string;
+ owner: string;
+ repo: string;
+ reason?: string;
+}
+
+export interface UpdateCheckResult extends UpdaterStatus {
+ state: UpdaterState;
+ updateAvailable: boolean;
</code_context>
<issue_to_address>
**suggestion:** Consolidate updater-related types to avoid drift between main and IPC typings.
`UpdaterChannel`, `UpdaterState`, `UpdaterStatus`, and `UpdateCheckResult` are defined both here and in `src/types/ipc.ts`. If they diverge, main and renderer could disagree on updater response shapes. Please move these to a single shared location (e.g., the IPC types file) and import them where needed to keep one source of truth.
Suggested implementation:
```typescript
import type { AppUpdater } from 'electron-updater';
import type {
UpdaterChannel,
UpdaterState,
UpdaterStatus,
UpdateCheckResult,
} from '../types/ipc';
export interface UpdaterRuntimeOptions extends UpdaterStatus {
```
To fully consolidate the updater-related types, you should also:
1. Ensure `src/types/ipc.ts` exports **exactly** these four types with matching shapes:
- `UpdaterChannel`
- `UpdaterState`
- `UpdaterStatus`
- `UpdateCheckResult` (including any fields such as `latestVersion`, `releaseName`, `errorMessage` that are expected by main and renderer).
2. If the relative path from `src/main/updater.ts` to the IPC types differs in your repo structure, adjust the import path (`'../types/ipc'`) accordingly.
3. If `UpdaterRuntimeOptions` or `UpdaterFlagOverrides` are also used across the IPC boundary, consider moving them to `src/types/ipc.ts` as well and importing them here in the same way.
</issue_to_address>
### Comment 3
<location> `src/renderer/context/DarkModeContext.tsx:50-57` </location>
<code_context>
}
- const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
+ const mediaQuery = appWindow.matchMedia('(prefers-color-scheme: dark)');
const handleChange = (event: MediaQueryListEvent) => {
if (localStorage.getItem('darkMode') === null) {
setDarkMode(event.matches);
}
};
- if (typeof mediaQuery.addEventListener === 'function') {
- mediaQuery.addEventListener('change', handleChange);
- return () => mediaQuery.removeEventListener('change', handleChange);
- }
</code_context>
<issue_to_address>
**issue (bug_risk):** Dropping the `addListener` fallback may break dark-mode syncing on older environments.
The previous code guarded against environments where `MediaQueryList.addEventListener` is missing by falling back to `addListener`. The new version assumes `addEventListener` is always present, which can cause runtime errors in older Electron or some test environments. If those environments are still in scope, please add a feature check and fall back to `addListener` when `addEventListener` is unavailable.
</issue_to_address>
### Comment 4
<location> `tests/integration/main-process/handlers.test.ts:232` </location>
<code_context>
+ expect(mockNetFetch).not.toHaveBeenCalled();
});
test('should resolve valid assets protocol requests', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend assets protocol tests to cover URL host/path combinations and encoded segments
Since the handler now splits and decodes hostname and pathname, the current test with only `assets://icon.png` doesn’t exercise the full behavior. Please add cases like `assets://public/icon.png`, `assets://host/dir/file.png`, and a path with URL-encoded characters (e.g., spaces or encoded `..`) to verify the join/decode logic and that the root constraint still holds. This will help detect regressions in the host vs path parsing logic.
Suggested implementation:
```typescript
test('should resolve valid assets protocol requests', async () => {
const handler = mockProtocolHandlers['assets'];
expect(handler).toBeDefined();
// Basic asset path without host
const iconResponse = await handler({ url: 'assets://icon.png' });
expect(iconResponse).toBeDefined();
// Asset path including a host segment
const hostIconResponse = await handler({ url: 'assets://public/icon.png' });
expect(hostIconResponse).toBeDefined();
// Nested host and directory segments
const nestedResponse = await handler({ url: 'assets://host/dir/file.png' });
expect(nestedResponse).toBeDefined();
// URL-encoded path segments should be decoded but still constrained to the assets root
const encodedSpaceResponse = await handler({
url: 'assets://host/dir/space%20file.png',
});
expect(encodedSpaceResponse).toBeDefined();
// Encoded traversal should still be rejected by the root constraint
const encodedTraversalResponse = await handler({
url: 'assets://host/dir/%2e%2e/%2e%2e/secret.txt',
});
expect(encodedTraversalResponse).toBeDefined();
expect(encodedTraversalResponse.status).toBe(403);
expect(mockNetFetch).not.toHaveBeenCalled();
```
1. If the existing `should resolve valid assets protocol requests` test already contains assertions for `assets://icon.png`, merge those with the new cases to avoid duplicate calls/assertions.
2. Adjust the expectations on `status` (`200`, `404`, etc.) for the valid URLs (`iconResponse`, `hostIconResponse`, `nestedResponse`, `encodedSpaceResponse`) to match the actual behavior of your assets handler (if it returns specific status codes instead of just being defined).
3. Ensure `mockNetFetch` is in scope for this test and correctly reset/mocked between tests so that the final `expect(mockNetFetch).not.toHaveBeenCalled();` remains accurate.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a well-structured, feature-flag-driven auto-updater and delivers a significant number of quality improvements across the codebase. The refactoring in the main and renderer processes greatly enhances maintainability, readability, and security. I particularly appreciate the more robust protocol handling, the improved secret scanning logic, and the consistent use of modern language features. The addition of new tests for the updater logic is also a great touch. I have one minor suggestion regarding a transitive dependency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc80543c6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?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: 3
🤖 Fix all issues with AI agents
In `@src/main/updater.ts`:
- Around line 120-142: Extract the getErrorMessage function into a shared
utility module: create a new module that exports getErrorMessage (e.g., export
const getErrorMessage = ...) and move the exact implementation there, then
replace the local definitions in both updater.ts and index.ts with an import of
getErrorMessage from the new module; ensure the function signature and behavior
are unchanged and update the import statements in the files that previously
defined getErrorMessage (referencing the getErrorMessage symbol to locate the
duplicates) so there are no remaining duplicate definitions.
In `@src/utils/content-processor.ts`:
- Line 50: The use of String.prototype.replaceAll on path.extname(filePath)
(assigned to fileType) is incompatible with the current TS lib target; either
update tsconfig.json to include ES2021 (e.g., add "ES2021" to "lib") so
replaceAll is supported, or change the expression that computes fileType (the
const fileType = path.extname(filePath)... usage) to use a regex-based replace
(String.prototype.replace with a global regex to remove dots) instead; apply the
same change for the other occurrences mentioned (the other
path.extname(...).replaceAll calls).
In `@src/utils/export-format.ts`:
- Around line 10-21: The build fails because string.replaceAll is used in
sanitizeXmlContent, escapeXmlAttribute, and wrapXmlCdata but the TS lib doesn't
include ES2021.String; fix by either adding "ES2021" or "ES2021.String" to the
tsconfig lib array so replaceAll is allowed, or replace every .replaceAll(...)
call in sanitizeXmlContent, escapeXmlAttribute, and wrapXmlCdata with
.replace(...) using equivalent global RegExp (e.g. /.*/g) so TypeScript builds
without changing tsconfig.
🧹 Nitpick comments (12)
scripts/sonar-scan.js (1)
278-296: Coverage-existence check runs even when generation is skipped.Lines 293-296 unconditionally require
coverage/lcov.infoto exist, even whenSONAR_GENERATE_COVERAGE=false. This means someone who disables generation but doesn't have coverage at that exact path will get a hard failure. If that's intentional (always require coverage for Sonar), this is fine. If you want to allow scans without coverage, gate the check:Proposed fix: gate the existence check
-if (!fs.existsSync(coveragePath)) { - console.error(`Error: Coverage report not found at ${coveragePath}`); - process.exit(1); -} +if (shouldGenerateCoverage && !fs.existsSync(coveragePath)) { + console.error(`Error: Coverage report not found at ${coveragePath}`); + process.exit(1); +}src/utils/secret-scanner.ts (1)
5-10: Consider enforcing that at least one detection mechanism is provided.Both
patternandmatchesare optional. ASecretRulewith neither field would silently never match. While the current rule set is correct, a discriminated union or runtime assertion would prevent future misuse.♻️ Type-level enforcement with a union
-type SecretRule = { - id: string; - description: string; - pattern?: RegExp; - matches?: (content: string) => boolean; -}; +type SecretRule = { + id: string; + description: string; +} & ( + | { pattern: RegExp; matches?: never } + | { matches: (content: string) => boolean; pattern?: never } + | { pattern: RegExp; matches: (content: string) => boolean } +);src/renderer/components/ConfigTab.tsx (1)
254-260:appWindowin the dependency array is a stable reference — consider removing it.
appWindowis reassigned toglobalThison every render, butglobalThisitself is a singleton and never changes identity. Including it in the dependency array is harmless but misleading, as it implies the effect should re-run when the window reference changes (which it won't). The linter may have requested it; if so, this is fine as-is.src/main/preload.ts (1)
12-13: Consolidate duplicate interface definitions to avoid potential drift.
UpdaterStatusandUpdateCheckResultinterfaces are defined identically in bothsrc/types/ipc.ts(lines 85-101) andsrc/main/updater.ts(lines 6-23). Sincesrc/main/preload.tsimports fromsrc/types/ipc.ts, updatesrc/main/updater.tsto import and re-export these interfaces fromsrc/types/ipc.tsinstead of maintaining separate copies.tests/integration/main-process/handlers.test.ts (1)
314-366: Platform-dependent test branches may pass vacuously on some CI runners.The
if (platformSupported)branching at line 340 means on Linux CI (the most common), only theelse(disabled) path is exercised. The "supported platform" path withsetFeedURL/checkForUpdatesassertions is never reached. Consider adding a dedicated test that mocksprocess.platformto'win32'or'darwin'so the supported-platform branch is covered regardless of the runner OS.src/renderer/components/SourceTab.tsx (1)
191-253:renderProcessButtonContentis re-created on every render.This works correctly since it's only called locally. If you wanted to optimize, it could be wrapped in
useCallbackor extracted outside the component as a pure function takingisAnalyzing/isCalculatingas arguments — but this is purely optional given it's not passed as a prop to child components.src/renderer/components/App.tsx (1)
188-188: Assigning globals during render is a side effect — move touseEffect.
appWindow.switchToTab(line 188) andappWindow.refreshDirectoryTree(line 211) are set during render. In React Strict Mode the render function runs twice, and more importantly, this is an impure side effect during the render phase. Wrap these assignments in auseEffectso they are updated after commit:♻️ Suggested approach
+ useEffect(() => { + appWindow.switchToTab = handleTabChange; + appWindow.refreshDirectoryTree = refreshDirectoryTree; + }); - appWindow.switchToTab = handleTabChange; ... - appWindow.refreshDirectoryTree = refreshDirectoryTree;Using
useEffectwithout a deps array ensures the globals are always current after each render.Also applies to: 210-211
src/main/feature-flags.ts (2)
4-4:TRUE_VALUESis duplicated insrc/main/updater.ts(line 36).Both files define the same
new Set(['1', 'true', 'yes', 'on']). Extract it to a shared utility to keep a single source of truth.
51-60: Env vars are read here and again inresolveUpdaterRuntimeOptions— the double read is redundant.
readUpdaterFlagOverridesFromEnvreadsUPDATER_ENABLED,UPDATER_CHECK_ON_START,UPDATER_GH_OWNER,UPDATER_GH_REPO. ThenresolveUpdaterRuntimeOptions(updater.ts, lines 75–82) reads the same env vars as a fallback behindflagOverrides.*. Since the flag overrides passed toresolveUpdaterRuntimeOptionsalready incorporate the env values, the fallback inresolveUpdaterRuntimeOptionsis effectively dead code. Consider havingresolveUpdaterRuntimeOptionsrely solely onflagOverrideswhen they are provided, rather than re-reading the environment.src/main/index.ts (2)
102-114: Consider adding a.catchto the fire-and-forget promise chain.While
checkForUpdatesinternally catches errors, the.then()callback itself could theoretically throw, producing an unhandled rejection. A trailing.catchis cheap insurance:♻️ Proposed fix
void updaterService.checkForUpdates().then((result) => { if (result.state === 'error') { console.warn(`Startup update check failed: ${result.errorMessage}`); } else if (result.state === 'update-available') { console.info(`Update available: ${result.latestVersion ?? 'unknown version'}`); } - }); + }).catch((err) => { + console.warn('Unexpected error during startup update check:', err); + });
661-661: ShadowedtokenCounter— consider reusing the module-level singleton.Line 661 creates a new
TokenCounter()that shadows the module-level instance at line 38. If there's no state isolation concern, reusing the singleton avoids unnecessary allocation.src/types/ipc.ts (1)
3-4: Duplicate type definitions withsrc/main/updater.ts.
UpdaterChannel,UpdaterState,UpdaterStatus, andUpdateCheckResultare defined identically in bothsrc/types/ipc.tsandsrc/main/updater.ts. Consolidate these types to a single source — eitheripc.tsexports them andupdater.tsimports them, or vice versa.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/feature-flags.ts`:
- Around line 212-214: getValueOrUndefined currently returns an empty string for
unset string flags (from client.getStringDetails), losing the "not configured"
semantics; update getValueOrUndefined (or add a separate helper like
getStringValueOrUndefined) to treat an empty string as undefined by returning
undefined when typeof details.value === 'string' && details.value === '' (in
addition to existing details.reason === 'ERROR' check), and update all call
sites that handle string flags (e.g., owner/repo resolution logic that builds
UpdaterFlagOverrides) to use the new behavior so unset string flags become
undefined.
🧹 Nitpick comments (5)
src/renderer/context/DarkModeContext.tsx (1)
13-13: TheappWindowalias is a pragmatic test-friendliness trick, but consider documenting intent.Casting
globalThistoWindow & typeof globalThisworks in the renderer but silently suppresses type errors if this module is ever imported in a non-browser context (e.g., SSR or a Node-based test runner withoutjsdom). A one-line comment explaining why the alias exists (e.g., to decouple fromwindowfor testability or Electron compatibility) would help future readers.src/utils/secret-scanner.ts (1)
5-10:SecretRuleallows bothpatternandmatchesto beundefined, producing a silent no-op rule.Not a runtime bug (the scan logic safely evaluates to
false), but the type doesn't enforce that at least one detection mechanism is provided. Consider a discriminated union or a runtime assertion if you want to prevent accidentally adding a rule with neither field.src/main/feature-flags.ts (1)
39-48:parseBooleanreturnsfalsefor any unrecognized string (e.g.,"false","no").This is intentional one-way truthy parsing, but it means there's no way to distinguish "explicitly set to false" from "not set" when the input is a string like
"false". Since the return type isboolean | undefined, and env vars likeUPDATER_ENABLED=falsewould returnfalse(the boolean), this seems acceptable for the env-var use case — just calling it out for awareness.src/main/index.ts (1)
550-553:treeViewparameter type should includenullto matchProcessRepositoryOptions.
ProcessRepositoryOptions.treeViewis typed asstring | null(insrc/types/ipc.ts), butbuildRepositoryHeaderdeclares it asstring | undefined. The||fallback on line 561 handles both at runtime, but this is a type mismatch that would surface understrictNullChecks.Proposed fix
const buildRepositoryHeader = ( processingOptions: RepositoryProcessingOptions, - treeView: string | undefined, + treeView: string | null | undefined, filesInfo: FileInfo[] ): string => {src/main/updater.ts (1)
14-21:parseBooleanEnvsilently returnsfalsefor unrecognized non-empty values.Any non-empty string not in
TRUE_VALUES(e.g.,"banana","2") returnsfalserather thanundefined. This collapses "explicitly false" and "unrecognized/misconfigured" into the same result, which could mask a typo in an env var (e.g.,UPDATER_ENABLED=ture). Consider returningundefinedfor values that aren't recognized as either true or false.Proposed fix
+const FALSE_VALUES = new Set(['0', 'false', 'no', 'off']); + export const parseBooleanEnv = (value: string | undefined): boolean | undefined => { if (!value) { return undefined; } - return TRUE_VALUES.has(value.trim().toLowerCase()); + const normalized = value.trim().toLowerCase(); + if (TRUE_VALUES.has(normalized)) return true; + if (FALSE_VALUES.has(normalized)) return false; + return undefined; };
| const getValueOrUndefined = <T>(details: { value: T; reason?: string }): T | undefined => { | ||
| return details.reason === 'ERROR' ? undefined : details.value; | ||
| }; |
There was a problem hiding this comment.
Unset string flags return '' instead of undefined, losing the "not configured" signal.
When a string flag (owner/repo) isn't in the configuration, client.getStringDetails(key, '') returns '' with a non-'ERROR' reason. getValueOrUndefined then returns '' rather than undefined. This means the returned UpdaterFlagOverrides will have owner: '' / repo: '' for unconfigured flags, which differs from the undefined semantics used everywhere else in this module.
Consider checking for empty string in getValueOrUndefined for string details, or using a separate helper for string flags:
Suggested approach
-const getValueOrUndefined = <T>(details: { value: T; reason?: string }): T | undefined => {
- return details.reason === 'ERROR' ? undefined : details.value;
-};
+const getValueOrUndefined = <T>(details: { value: T; reason?: string }): T | undefined => {
+ if (details.reason === 'ERROR') {
+ return undefined;
+ }
+ if (typeof details.value === 'string' && details.value === '') {
+ return undefined;
+ }
+ return details.value;
+};Also applies to: 241-248
🤖 Prompt for AI Agents
In `@src/main/feature-flags.ts` around lines 212 - 214, getValueOrUndefined
currently returns an empty string for unset string flags (from
client.getStringDetails), losing the "not configured" semantics; update
getValueOrUndefined (or add a separate helper like getStringValueOrUndefined) to
treat an empty string as undefined by returning undefined when typeof
details.value === 'string' && details.value === '' (in addition to existing
details.reason === 'ERROR' check), and update all call sites that handle string
flags (e.g., owner/repo resolution logic that builds UpdaterFlagOverrides) to
use the new behavior so unset string flags become undefined.
This comment has been minimized.
This comment has been minimized.
1 participant
Summary
Validation
Summary by Sourcery
Introduce a feature-flag–driven Electron auto-updater runtime, harden main/renderer logic for better safety and maintainability, and ensure CI releases and Sonar scans use consistent, fresh build metadata and coverage.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Summary by CodeRabbit
New Features
Chores