Added self-update check mechanism#12
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a background update-check subsystem: a worker fetches remote JSON via WinHTTP, compares remote FileVersion with the local executable version, notifies the UI via a custom Windows message, persists a dismissal timestamp, and links required Windows libs in the build. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Main UI Thread
participant Worker as UpdateCheckSession Worker
participant Remote as Remote Server
participant WinAPI as Windows APIs
UI->>Worker: Construct UpdateCheckSession(hwnd, dismissedUnix)
Worker->>Worker: Check dismissedUnix (24h window)
par Fetch remote JSON & get local version
Worker->>Remote: HTTPS GET latest JSON (WinHTTP)
Remote-->>Worker: JSON with "FileVersion"
Worker->>WinAPI: Query local executable FileVersion
end
Worker->>Worker: Parse versions, compare VersionQuad
alt Remote is newer
Worker->>Worker: Store pending local & remote strings (mutex)
Worker->>UI: Post WM_UPDATE_CHECK_READY (if HWND still current)
else No update / suppressed / error
Worker-->>Worker: Exit without notifying
end
UI->>UI: On WM_UPDATE_CHECK_READY call UpdateCheck_PopResultForUi()
UI-->>UI: Show ImGui modal with versions
alt Download selected
UI->>Remote: Open download URL (UpdateCheck_GetLatestDownloadUrlW)
else Not today selected
UI->>UI: Save current time to updateDismissedUnix and persist config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main.cpp (1)
754-762: Keep the update URL in one place.This ZIP URL duplicates the buildbot location already hardcoded in
src/update_check.cpp(kJsonHost/kJsonPath). If those drift, the dialog will announce an update but open the wrong artifact. Prefer a shared source of truth or return the download URL in the metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 754 - 762, The Download update button handler in src/main.cpp currently hardcodes the ZIP URL via the ShellExecuteW call; instead obtain the download URL from the shared update logic in src/update_check.cpp (do not duplicate kJsonHost/kJsonPath). Replace the hardcoded URL with a single source of truth: either expose a function like GetLatestDownloadUrl() or a shared constant (e.g., kDownloadUrl) from update_check.cpp and call that from the ImGui::Button handler (the block containing ImGui::Button and ShellExecuteW) so the dialog always opens the same artifact returned by the update metadata.
🤖 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/update_check.cpp`:
- Around line 116-139: ExtractJsonFileVersion currently accepts unterminated
JSON string values; change it to verify the closing quote exists and that the
parsed substring is properly delimited (reject when scan hits end-of-input
without finding the closing '"'), and ensure it does not return true for empty
or partial matches. Also harden HttpGetUtf8 so that any WinHttpReadData failure
(even after some bytes were buffered) causes the function to return false
instead of success — propagate/read the WinHttpReadData result and only report
success when the entire response was read without read errors. Update both
ExtractJsonFileVersion and HttpGetUtf8 accordingly so truncated responses like
`"FileVersion":"1.2.3` are rejected.
---
Nitpick comments:
In `@src/main.cpp`:
- Around line 754-762: The Download update button handler in src/main.cpp
currently hardcodes the ZIP URL via the ShellExecuteW call; instead obtain the
download URL from the shared update logic in src/update_check.cpp (do not
duplicate kJsonHost/kJsonPath). Replace the hardcoded URL with a single source
of truth: either expose a function like GetLatestDownloadUrl() or a shared
constant (e.g., kDownloadUrl) from update_check.cpp and call that from the
ImGui::Button handler (the block containing ImGui::Button and ShellExecuteW) so
the dialog always opens the same artifact returned by the update metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16dc844c-7464-408b-ad28-c585b1ea5f3f
📒 Files selected for processing (4)
CMakeLists.txtsrc/main.cppsrc/update_check.cppsrc/update_check.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/update_check.cpp (1)
305-309:⚠️ Potential issue | 🟠 MajorGive the update worker an owner instead of detaching it.
This fire-and-forget thread can outlive the window and keep running while
g_resultMutex,g_pendingLocal,g_pendingRemote, and thehwndSlotpointee are tearing down. Clearing the slot onWM_DESTROYonly avoids stale posts; it does not coordinate shutdown of the WinHTTP work itself. Please store the worker in app state and join/cancel it during shutdown instead of detaching here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update_check.cpp` around lines 305 - 309, StartBackgroundUpdateCheck currently detaches a fire-and-forget std::thread that can outlive the window and access globals like g_resultMutex, g_pendingLocal, g_pendingRemote and the hwndSlot pointee during teardown; instead, make the update worker owned by application state (store a std::thread or preferably a std::jthread member) and use a cancellation mechanism (std::atomic<bool> or std::stop_token) passed into RunUpdateCheck so the worker can exit cooperatively; on shutdown/join path join the thread (or request stop and join) before destroying the globals and clearing hwndSlot to guarantee no concurrent access. Ensure the stored thread is referenced instead of detaching in StartBackgroundUpdateCheck and that shutdown code signals cancellation and joins the worker prior to resource teardown.
🤖 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.cpp`:
- Around line 744-747: The modal uses
ImGui::BeginPopupModal(kUpdateAvailablePopupId, &g_showUpdateAvailable, ...) so
clicking the built-in close (X) only clears g_showUpdateAvailable and does not
persist the 24h suppression; updateDismissedUnix is only written by the "Not
today" button. Fix by either removing the address parameter (call
BeginPopupModal without the bool pointer) so the built-in close is not shown, or
detect any dismissal via g_showUpdateAvailable becoming false and set
updateDismissedUnix the same way the "Not today" button does; update the logic
around ImGui::BeginPopupModal, kUpdateAvailablePopupId, g_showUpdateAvailable,
and the updateDismissedUnix write site so all close paths persist the 24h
suppression.
In `@src/update_check.cpp`:
- Around line 44-76: ParseVersionString currently accepts empty components
because dots commit the accumulator even when no digits were read; fix by
tracking whether the current segment had any digits and reject on a dot (or
end-of-string) if none were seen. In the loop over s, add a boolean (e.g.
sawDigit) set to true when a numeric char is processed; when encountering '.'
check sawDigit and return false if false, then reset sawDigit and cur and
increment part as before; after the loop ensure sawDigit is true before
assigning the last component to out (a/b/c/d) and return false if not. Use the
existing symbols part, cur, out, and the loop over s to locate where to apply
this change.
---
Duplicate comments:
In `@src/update_check.cpp`:
- Around line 305-309: StartBackgroundUpdateCheck currently detaches a
fire-and-forget std::thread that can outlive the window and access globals like
g_resultMutex, g_pendingLocal, g_pendingRemote and the hwndSlot pointee during
teardown; instead, make the update worker owned by application state (store a
std::thread or preferably a std::jthread member) and use a cancellation
mechanism (std::atomic<bool> or std::stop_token) passed into RunUpdateCheck so
the worker can exit cooperatively; on shutdown/join path join the thread (or
request stop and join) before destroying the globals and clearing hwndSlot to
guarantee no concurrent access. Ensure the stored thread is referenced instead
of detaching in StartBackgroundUpdateCheck and that shutdown code signals
cancellation and joins the worker prior to resource teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3f261e2-09d8-4db2-9b11-83b7018c24bb
📒 Files selected for processing (3)
src/main.cppsrc/update_check.cppsrc/update_check.h
Comment on lines
+744
to
+747
| if (ImGui::BeginPopupModal( | ||
| kUpdateAvailablePopupId, | ||
| &g_showUpdateAvailable, | ||
| ImGuiWindowFlags_AlwaysAutoResize | ImGuiWindowFlags_NoResize)) |
Contributor
There was a problem hiding this comment.
The modal has a dismissal path that doesn't persist suppression.
Only the "Not today" button writes updateDismissedUnix, but BeginPopupModal(..., &g_showUpdateAvailable, ...) automatically provides a close button. When users click that X button, the modal closes without recording the 24h suppression, so the notification may reappear immediately on the next launch. Either remove the close button parameter here or treat any close as "Not today".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.cpp` around lines 744 - 747, The modal uses
ImGui::BeginPopupModal(kUpdateAvailablePopupId, &g_showUpdateAvailable, ...) so
clicking the built-in close (X) only clears g_showUpdateAvailable and does not
persist the 24h suppression; updateDismissedUnix is only written by the "Not
today" button. Fix by either removing the address parameter (call
BeginPopupModal without the bool pointer) so the built-in close is not shown, or
detect any dismissal via g_showUpdateAvailable becoming false and set
updateDismissedUnix the same way the "Not today" button does; update the logic
around ImGui::BeginPopupModal, kUpdateAvailablePopupId, g_showUpdateAvailable,
and the updateDismissedUnix write site so all close paths persist the 24h
suppression.
Comment on lines
+44
to
+76
| for (char ch : s) | ||
| { | ||
| if (ch == '.') | ||
| { | ||
| if (part >= 4) | ||
| return false; | ||
| if (part == 0) | ||
| out.a = static_cast<uint16_t>(cur); | ||
| else if (part == 1) | ||
| out.b = static_cast<uint16_t>(cur); | ||
| else if (part == 2) | ||
| out.c = static_cast<uint16_t>(cur); | ||
| ++part; | ||
| cur = 0; | ||
| continue; | ||
| } | ||
| if (ch < '0' || ch > '9') | ||
| return false; | ||
| cur = cur * 10u + static_cast<uint32_t>(ch - '0'); | ||
| if (cur > 65535u) | ||
| return false; | ||
| } | ||
| if (part == 0) | ||
| out.a = static_cast<uint16_t>(cur); | ||
| else if (part == 1) | ||
| out.b = static_cast<uint16_t>(cur); | ||
| else if (part == 2) | ||
| out.c = static_cast<uint16_t>(cur); | ||
| else if (part == 3) | ||
| out.d = static_cast<uint16_t>(cur); | ||
| else | ||
| return false; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Reject empty version components in ParseVersionString().
Inputs like .1, 1..2, and 1.2.3. still parse as valid because each . commits the current accumulator even when that segment had no digits. That lets malformed manifest data participate in the update comparison.
Possible fix
bool ParseVersionString(std::string_view s, VersionQuad& out)
{
out = {};
int part = 0;
uint32_t cur = 0;
+ bool sawDigit = false;
for (char ch : s)
{
if (ch == '.')
{
- if (part >= 4)
+ if (!sawDigit || part >= 3)
return false;
if (part == 0)
out.a = static_cast<uint16_t>(cur);
else if (part == 1)
out.b = static_cast<uint16_t>(cur);
else if (part == 2)
out.c = static_cast<uint16_t>(cur);
++part;
cur = 0;
+ sawDigit = false;
continue;
}
if (ch < '0' || ch > '9')
return false;
+ sawDigit = true;
cur = cur * 10u + static_cast<uint32_t>(ch - '0');
if (cur > 65535u)
return false;
}
+ if (!sawDigit)
+ return false;
if (part == 0)
out.a = static_cast<uint16_t>(cur);
else if (part == 1)
out.b = static_cast<uint16_t>(cur);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/update_check.cpp` around lines 44 - 76, ParseVersionString currently
accepts empty components because dots commit the accumulator even when no digits
were read; fix by tracking whether the current segment had any digits and reject
on a dot (or end-of-string) if none were seen. In the loop over s, add a boolean
(e.g. sawDigit) set to true when a numeric char is processed; when encountering
'.' check sawDigit and return false if false, then reset sawDigit and cur and
increment part as before; after the loop ensure sawDigit is true before
assigning the last component to out (a/b/c/d) and return false if not. Use the
existing symbols part, cur, out, and the loop over s to locate where to apply
this change.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/update_check.cpp (1)
154-261: Consider RAII wrappers for WinHTTP handles.The function correctly handles all error paths and cooperative cancellation. The manual handle cleanup is correct but verbose. If you're using WIL elsewhere in the project,
wil::unique_winhttp_hinternetcould simplify cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/update_check.cpp` around lines 154 - 261, Summary: Replace manual WinHTTP handle management in HttpGetUtf8 with RAII wrappers to simplify cleanup and reduce verbosity. Fix: include the WIL winhttp header (wil/winhttp.h), change HINTERNET hSession/hConnect/hRequest to wil::unique_winhttp_hinternet (or wil::unique_hinternet equivalent), remove the closeSession lambda and all explicit WinHttpCloseHandle calls, and let the unique handles auto-close on scope exit; keep the same error checks, timeouts (WinHttpSetTimeouts using the unique hSession.get()), and stop_token checks, and ensure any APIs that require raw HINTERNET use .get() when calling WinHttpConnect/WinHttpOpenRequest/WinHttpSendRequest/WinHttpReceiveResponse/WinHttpReadData/WinHttpQueryHeaders so behavior is unchanged.
🤖 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/update_check.cpp`:
- Around line 154-261: Summary: Replace manual WinHTTP handle management in
HttpGetUtf8 with RAII wrappers to simplify cleanup and reduce verbosity. Fix:
include the WIL winhttp header (wil/winhttp.h), change HINTERNET
hSession/hConnect/hRequest to wil::unique_winhttp_hinternet (or
wil::unique_hinternet equivalent), remove the closeSession lambda and all
explicit WinHttpCloseHandle calls, and let the unique handles auto-close on
scope exit; keep the same error checks, timeouts (WinHttpSetTimeouts using the
unique hSession.get()), and stop_token checks, and ensure any APIs that require
raw HINTERNET use .get() when calling
WinHttpConnect/WinHttpOpenRequest/WinHttpSendRequest/WinHttpReceiveResponse/WinHttpReadData/WinHttpQueryHeaders
so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 823ac08e-df44-4dc8-b39a-7ea8dc4fc0f1
📒 Files selected for processing (3)
src/main.cppsrc/update_check.cppsrc/update_check.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.cpp
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.
Summary by CodeRabbit