fix: app cannot fetch updates because of GitHub rate limits#243
Merged
vitonsky merged 7 commits intoMar 7, 2026
Conversation
📝 WalkthroughWalkthroughReplaces the GitHub releases updater with a host-configurable updater. Removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (5)
src/electron/updates/__tests__/AppUpdatesChecker.unit.test.ts (2)
86-109: Good error handling coverage.The "No updates cases" block effectively covers empty releases and server errors. Consider adding a test for network-level failures (fetch rejection) to ensure complete error path coverage.
Optional: Add network failure test
+ test('Returns null for network failures', async () => { + mockFetch.mockRejectedValue(new Error('Network error')); + + await expect( + updatesChecker.getUpdate({ + version: '0.0.1', + }), + ).resolves.toEqual(null); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/__tests__/AppUpdatesChecker.unit.test.ts` around lines 86 - 109, Add a test to cover network-level failures by mocking the fetch used by AppUpdatesChecker to reject (e.g., mockFetch.mockRejectedValueOnce(new Error('Network error'))) and verifying AppUpdatesChecker.getUpdate(...) resolves to null; place this new test inside the existing "No updates cases" describe alongside the tests using mockVersions and the server error mock so it exercises the network rejection path for getUpdate.
1-16: Consider adding explicit vitest imports.Similar to the other test file, explicit imports would improve clarity.
Suggested imports
import { AppUpdatesChecker, VersionObject } from '../AppUpdatesChecker'; +import { vi, describe, beforeEach, test, expect } from 'vitest'; const mockFetch = vi.fn();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/__tests__/AppUpdatesChecker.unit.test.ts` around lines 1 - 16, Add explicit vitest imports at the top of the test file so the test helpers are clear: import the vi mock helper and beforeEach from 'vitest' (used by mockFetch and beforeEach), then remove reliance on implicit globals; update the top of the file where AppUpdatesChecker and VersionObject are imported to also import { vi, beforeEach } from 'vitest' so symbols like vi.fn and beforeEach resolve explicitly.src/electron/updates/AppUpdatesChecker.ts (1)
4-8: Theprereleasefield is validated but unused.
VersionObjectSchemerequires aprereleaseboolean field, butgetUpdatedoesn't use it in the update detection logic. This is fine if planned for future use (e.g., allowing users to opt-out of prerelease updates), but could be removed if not needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/AppUpdatesChecker.ts` around lines 4 - 8, The VersionObjectScheme currently declares a prerelease boolean that's never used by the update detection logic in getUpdate; either remove the unused field from VersionObjectScheme (and any code that expects versionObj.prerelease) or wire it into getUpdate (e.g., respect prerelease when comparing/choosing updates). Pick one: if you don't support prerelease yet, delete the prerelease entry from VersionObjectScheme to keep the schema accurate; if you do want prerelease support, update getUpdate to inspect versionObj.prerelease and incorporate it into update selection logic.src/features/App/useGetAppUpdates.tsx (1)
72-73: URL override creates inconsistency with API response.The code mutates
newVersion.urlto a hardcoded download page URL, discarding the release-specific URL returned byAppUpdatesChecker. While this aligns with the PR objective to direct users to the site's download page, consider:
- The mutation pattern is fragile—if
AppVersionInfobecomes immutable (e.g.,readonlyproperties), this will break.- The
AppUpdatesChecker.getUpdate()already returns aurlfield that's now being ignored.A cleaner approach would be to either:
- Configure the download URL in
AppUpdatesCheckeror its config- Or create a new object instead of mutating
Option: Create new object instead of mutating
.then((newVersion) => { if (!newVersion) return null; - newVersion.url = 'https://deepink.io/download'; - + const versionWithDownloadUrl = { + ...newVersion, + url: 'https://deepink.io/download', + }; + showToast({ duration: null, position: 'top-right', containerStyle: { maxW: '350px', }, render(props) { return ( <AppToast title="New version is available" body="Update app to a latest version to get new features and improvements" actions={ <> <Button size="sm" variant="accent" onClick={() => { - window.open(newVersion.url); + window.open(versionWithDownloadUrl.url); localStorage.removeItem( ignoreFlagKey, ); props.onClose(); }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useGetAppUpdates.tsx` around lines 72 - 73, The current code mutates newVersion.url to a hardcoded download page which discards the release-specific URL from AppUpdatesChecker.getUpdate() and is fragile if AppVersionInfo becomes readonly; instead, stop mutating the returned object and either (A) have AppUpdatesChecker accept a configured downloadUrl so getUpdate() returns the desired URL, or (B) create a new object in useGetAppUpdates (e.g., const displayVersion = { ...newVersion, url: 'https://deepink.io/download' }) and use displayVersion for UI, leaving AppUpdatesChecker.getUpdate() and the original newVersion untouched.src/electron/updates/__tests__/AppUpdatesChecker.test.ts (1)
1-5: Consider adding explicit vitest imports for clarity.The test uses
viandbeforeEachwithout explicit imports. While Vitest supports globals mode, explicit imports improve code clarity and IDE support.Suggested imports
import { AppUpdatesChecker } from '../AppUpdatesChecker'; import versions from './versions.json'; +import { vi, beforeEach, test, expect } from 'vitest'; const mockFetch = vi.fn(); globalThis.fetch = mockFetch;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/__tests__/AppUpdatesChecker.test.ts` around lines 1 - 5, The test currently relies on Vitest globals (uses vi and beforeEach) without explicit imports; add explicit imports from 'vitest' (e.g., import { vi, beforeEach, afterEach, it, expect } from 'vitest') at the top of the test file so IDEs and readers can resolve symbols like vi, beforeEach and test helpers used alongside AppUpdatesChecker and the versions fixture; keep only the helpers you actually use to avoid unused-import lint warnings.
🤖 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/electron/updates/AppUpdatesChecker.ts`:
- Around line 37-38: Validate the version strings before calling semver.gt in
AppUpdatesChecker.ts: check semver.valid(latestVersion) and
semver.valid(context.version) (the variables from the diff) and handle invalid
cases explicitly (e.g., log a clear error or skip comparison) instead of relying
on the try-catch; only call semver.gt(latestVersion, context.version) when both
are valid to avoid TypeError and preserve clearer error messages.
---
Nitpick comments:
In `@src/electron/updates/__tests__/AppUpdatesChecker.test.ts`:
- Around line 1-5: The test currently relies on Vitest globals (uses vi and
beforeEach) without explicit imports; add explicit imports from 'vitest' (e.g.,
import { vi, beforeEach, afterEach, it, expect } from 'vitest') at the top of
the test file so IDEs and readers can resolve symbols like vi, beforeEach and
test helpers used alongside AppUpdatesChecker and the versions fixture; keep
only the helpers you actually use to avoid unused-import lint warnings.
In `@src/electron/updates/__tests__/AppUpdatesChecker.unit.test.ts`:
- Around line 86-109: Add a test to cover network-level failures by mocking the
fetch used by AppUpdatesChecker to reject (e.g.,
mockFetch.mockRejectedValueOnce(new Error('Network error'))) and verifying
AppUpdatesChecker.getUpdate(...) resolves to null; place this new test inside
the existing "No updates cases" describe alongside the tests using mockVersions
and the server error mock so it exercises the network rejection path for
getUpdate.
- Around line 1-16: Add explicit vitest imports at the top of the test file so
the test helpers are clear: import the vi mock helper and beforeEach from
'vitest' (used by mockFetch and beforeEach), then remove reliance on implicit
globals; update the top of the file where AppUpdatesChecker and VersionObject
are imported to also import { vi, beforeEach } from 'vitest' so symbols like
vi.fn and beforeEach resolve explicitly.
In `@src/electron/updates/AppUpdatesChecker.ts`:
- Around line 4-8: The VersionObjectScheme currently declares a prerelease
boolean that's never used by the update detection logic in getUpdate; either
remove the unused field from VersionObjectScheme (and any code that expects
versionObj.prerelease) or wire it into getUpdate (e.g., respect prerelease when
comparing/choosing updates). Pick one: if you don't support prerelease yet,
delete the prerelease entry from VersionObjectScheme to keep the schema
accurate; if you do want prerelease support, update getUpdate to inspect
versionObj.prerelease and incorporate it into update selection logic.
In `@src/features/App/useGetAppUpdates.tsx`:
- Around line 72-73: The current code mutates newVersion.url to a hardcoded
download page which discards the release-specific URL from
AppUpdatesChecker.getUpdate() and is fragile if AppVersionInfo becomes readonly;
instead, stop mutating the returned object and either (A) have AppUpdatesChecker
accept a configured downloadUrl so getUpdate() returns the desired URL, or (B)
create a new object in useGetAppUpdates (e.g., const displayVersion = {
...newVersion, url: 'https://deepink.io/download' }) and use displayVersion for
UI, leaving AppUpdatesChecker.getUpdate() and the original newVersion untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf4d1483-9462-42e8-9776-f16de76d845b
📒 Files selected for processing (8)
src/electron/updates/AppUpdatesChecker.tssrc/electron/updates/GitHubReleaseUpdatesChecker.test.tssrc/electron/updates/GitHubReleaseUpdatesChecker.tssrc/electron/updates/__tests__/AppUpdatesChecker.test.tssrc/electron/updates/__tests__/AppUpdatesChecker.unit.test.tssrc/electron/updates/__tests__/versions.jsonsrc/features/App/Settings/sections/GeneralSettings.tsxsrc/features/App/useGetAppUpdates.tsx
💤 Files with no reviewable changes (2)
- src/electron/updates/GitHubReleaseUpdatesChecker.test.ts
- src/electron/updates/GitHubReleaseUpdatesChecker.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/features/App/useGetAppUpdates.tsx (1)
72-119: Keep the update CTA aligned with the fetched release metadata.
AppUpdatesChecker.getUpdate()already returns the canonicalurlfrom/versions.json, but this hook drops it and always opens/download. That gives you two sources of truth for the same update and can send users somewhere different from the release metadata the checker just resolved. Prefer reusingnewVersion.url, or make/downloadthe only URL emitted byversions.json.🔗 Suggested simplification
- const updateUrl = 'https://deepink.io/download'; + const updateUrl = newVersion.url;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/App/useGetAppUpdates.tsx` around lines 72 - 119, The hook currently hardcodes updateUrl = 'https://deepink.io/download' and uses it in the toast and return value; replace that with the canonical URL from the release metadata (use newVersion.url) so the CTA and returned AppVersionInfo align with AppUpdatesChecker.getUpdate(), and update the window.open call, toast actions, and the returned object (version and url) to use newVersion.url instead of the hardcoded updateUrl; keep existing ignoreUpdate and ignoreFlagKey behavior unchanged.src/electron/updates/__tests__/AppUpdatesChecker.test.ts (1)
26-30: Assert the proxied endpoint in this regression test.This covers the version-comparison behavior, but it never proves that the checker actually requested the new site endpoint. A typo in
hostor/versions.jsonwould still leave this test green, which weakens the main regression this PR is meant to protect.📌 Suggested assertion
await expect( updatesChecker.getUpdate({ version: '0.0.1', }), ).resolves.toEqual(latestVersion); + expect(mockFetch).toHaveBeenNthCalledWith( + 1, + 'https://site.com/versions.json', + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/__tests__/AppUpdatesChecker.test.ts` around lines 26 - 30, The test currently verifies version comparison but doesn't assert the actual network call; update the test around updatesChecker.getUpdate to also assert that the proxied endpoint was requested (e.g., that the HTTP client/mocker was called with the configured host + '/versions.json' or that the test server received a request to '/versions.json'). Reference the same setup values used in the test (the configured host/URL and updatesChecker.getUpdate) and add an assertion that the fetch/mock call includes the exact path '/versions.json' to catch typos in the host or path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/electron/updates/__tests__/AppUpdatesChecker.test.ts`:
- Around line 26-30: The test currently verifies version comparison but doesn't
assert the actual network call; update the test around updatesChecker.getUpdate
to also assert that the proxied endpoint was requested (e.g., that the HTTP
client/mocker was called with the configured host + '/versions.json' or that the
test server received a request to '/versions.json'). Reference the same setup
values used in the test (the configured host/URL and updatesChecker.getUpdate)
and add an assertion that the fetch/mock call includes the exact path
'/versions.json' to catch typos in the host or path.
In `@src/features/App/useGetAppUpdates.tsx`:
- Around line 72-119: The hook currently hardcodes updateUrl =
'https://deepink.io/download' and uses it in the toast and return value; replace
that with the canonical URL from the release metadata (use newVersion.url) so
the CTA and returned AppVersionInfo align with AppUpdatesChecker.getUpdate(),
and update the window.open call, toast actions, and the returned object (version
and url) to use newVersion.url instead of the hardcoded updateUrl; keep existing
ignoreUpdate and ignoreFlagKey behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81dff6cb-9d1c-4092-9c65-74da6f8b5621
📒 Files selected for processing (2)
src/electron/updates/__tests__/AppUpdatesChecker.test.tssrc/features/App/useGetAppUpdates.tsx
vitonsky
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/electron/updates/AppUpdatesChecker.ts (1)
4-8: Unusedprereleasefield.The
prereleasefield is parsed but never used in the update selection logic. If prereleases should be excluded from stable update checks, consider filtering them out ingetUpdate. If this is intentional (e.g., for future use or showing all releases), this can be ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/electron/updates/AppUpdatesChecker.ts` around lines 4 - 8, The VersionObjectScheme currently includes a prerelease boolean that's never used; either remove prerelease from VersionObjectScheme if you don't intend to handle it, or update the update-selection logic in getUpdate to respect it (e.g., filter out items where parsedVersion.prerelease === true when performing stable update checks). Locate VersionObjectScheme and the getUpdate function and either drop the prerelease property from the schema or add a filter step in getUpdate that excludes prerelease releases before selecting the latest update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/electron/updates/AppUpdatesChecker.ts`:
- Around line 4-8: The VersionObjectScheme currently includes a prerelease
boolean that's never used; either remove prerelease from VersionObjectScheme if
you don't intend to handle it, or update the update-selection logic in getUpdate
to respect it (e.g., filter out items where parsedVersion.prerelease === true
when performing stable update checks). Locate VersionObjectScheme and the
getUpdate function and either drop the prerelease property from the schema or
add a filter step in getUpdate that excludes prerelease releases before
selecting the latest update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf3f2abd-6833-4149-beeb-dcaa1b20d823
📒 Files selected for processing (2)
src/electron/updates/AppUpdatesChecker.tssrc/electron/updates/__tests__/AppUpdatesChecker.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/electron/updates/tests/AppUpdatesChecker.unit.test.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
1 participant
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.
Fix #203
Now we use a static page on our site to check for updates.
Summary by CodeRabbit
New Features
Refactor
Tests