Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s release automation/versioning and performs an internal naming refactor by renaming CookieJar to AsyncCookieJar, alongside a few stability fixes in request processing and transport shutdown.
Changes:
- Added GitHub Actions automation for auto-tagging on version bumps and enhanced release notes generation (including changelog extraction + structured summary).
- Introduced
scripts/sync-version.shand bumped version metadata to2.1.1acrosslibrary.json,library.properties, andsrc/HttpCommon.h. - Renamed
CookieJartoAsyncCookieJaracross the codebase; added request-queue reentrancy guard and adjusted transport close behavior to reduce callback hazards.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_redirects/test_main.cpp | Updates include to renamed AsyncCookieJar header. |
| test/test_cookies/test_main.cpp | Updates include to renamed AsyncCookieJar header. |
| src/TlsTransport.cpp | Clears AsyncClient callbacks on close to avoid late callback invocation. |
| src/TcpTransport.cpp | Clears AsyncClient callbacks on close to avoid late callback invocation. |
| src/HttpCommon.h | Version bump to 2.1.1. |
| src/AsyncHttpClient.h | Renames friend/ptr type to AsyncCookieJar; adds _inTryDequeue guard flag. |
| src/AsyncHttpClient.cpp | Uses AsyncCookieJar; increases auto-loop stack; adds tryDequeue recursion guard + safer loop iteration. |
| src/AsyncCookieJar.h | Renames class and include guards to AsyncCookieJar. |
| src/AsyncCookieJar.cpp | Renames implementation symbols/includes to AsyncCookieJar. |
| scripts/sync-version.sh | New helper script to sync versions across metadata files. |
| library.properties | Version bump to 2.1.1. |
| library.json | Version bump to 2.1.1. |
| .github/workflows/release.yml | Adds changelog extraction and structured release body generation. |
| .github/workflows/auto-tag.yml | New workflow to validate version consistency and auto-create vX.Y.Z tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Extract changelog for this version | ||
| id: changelog | ||
| run: | | ||
| VERSION=$(grep '"version"' library.json | sed -E 's/.*"version" *: *"([^"]+)".*/\1/') | ||
| echo "version=${VERSION}" >> "$GITHUB_OUTPUT" | ||
|
|
There was a problem hiding this comment.
In the changelog extraction step, VERSION=$(grep '"version"' library.json | ...) will match multiple "version" keys in library.json (the top-level library version and dependency versions). This can produce a multi-line VERSION value and break release naming/notes. Use jq -r .version or at least grep -m1 with a stricter pattern targeting only the top-level version.
| if [[ -f library.json ]]; then | ||
| sed -i -E "s/\"version\" *: *\"[^\"]+\"/\"version\": \"${NEW_VERSION}\"/" library.json | ||
| echo " ✓ library.json" | ||
| else | ||
| echo " ✗ library.json not found" | ||
| fi | ||
|
|
||
| # 2. library.properties | ||
| if [[ -f library.properties ]]; then | ||
| sed -i -E "s/^version=.*/version=${NEW_VERSION}/" library.properties | ||
| echo " ✓ library.properties" | ||
| else | ||
| echo " ✗ library.properties not found" | ||
| fi | ||
|
|
||
| # 3. src/HttpCommon.h | ||
| if [[ -f src/HttpCommon.h ]]; then | ||
| sed -i -E "s/#define ESP_ASYNC_WEB_CLIENT_VERSION \"[^\"]+\"/#define ESP_ASYNC_WEB_CLIENT_VERSION \"${NEW_VERSION}\"/" src/HttpCommon.h | ||
| echo " ✓ src/HttpCommon.h" |
There was a problem hiding this comment.
sed -i behaves differently between GNU sed (Linux) and BSD sed (macOS requires -i ''). Since this script is meant to be run locally, consider making the in-place edit portable (e.g., detect BSD vs GNU sed, or use a small Python/JQ snippet for replacements) to avoid breaking contributors on macOS.
| bool _inTryDequeue = false; // reentrancy guard to prevent cleanup → tryDequeue recursion | ||
| std::unique_ptr<AsyncCookieJar> _cookieJar; |
There was a problem hiding this comment.
_inTryDequeue is accessed from multiple code paths that can run on different tasks/threads when ESP32 auto-loop is enabled (e.g., auto-loop task + request/callback paths), but it’s a plain bool. Reads/writes to a non-atomic flag without consistently holding _reqMutex is a C++ data race (undefined behavior). Consider making this an std::atomic_bool (or guarding all accesses with lock()/unlock()), and use an atomic test-and-set/exchange pattern so the reentrancy check is safe.
| void AsyncHttpClient::tryDequeue() { | ||
| if (_inTryDequeue) | ||
| return; // prevent recursion via executeRequest → triggerError → cleanup → tryDequeue | ||
| _inTryDequeue = true; | ||
| while (true) { |
There was a problem hiding this comment.
tryDequeue() sets/reads _inTryDequeue without any synchronization. With the ESP32 auto-loop task enabled, this can be called concurrently (and also from callback stacks), so this is a data race. Use an atomic guard (e.g., exchange(true) / store(false)) or take _reqMutex before checking/setting the flag so recursion prevention doesn’t introduce undefined behavior.
| # 1. library.json | ||
| if [[ -f library.json ]]; then | ||
| sed -i -E "s/\"version\" *: *\"[^\"]+\"/\"version\": \"${NEW_VERSION}\"/" library.json | ||
| echo " ✓ library.json" | ||
| else |
There was a problem hiding this comment.
The sed replacement for library.json matches any line containing a "version" key, which will also rewrite dependency versions (e.g. the AsyncTCP dependency’s "version": "^3.4.8"). This will corrupt library.json when syncing versions. Update the script to only modify the top-level .version field (e.g., parse JSON with jq/Python, or restrict the match to the first occurrence / an anchored top-level key).
![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: 7a8fc9df91
ℹ️ 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".
|
|
||
| # 1. library.json | ||
| if [[ -f library.json ]]; then | ||
| sed -i -E "s/\"version\" *: *\"[^\"]+\"/\"version\": \"${NEW_VERSION}\"/" library.json |
There was a problem hiding this comment.
The sed replacement matches every "version" key in library.json, not just the package’s top-level version field. In this repo that also rewrites dependencies[].version (e.g., AsyncTCP), so running scripts/sync-version.sh 2.2.0 would silently change the dependency constraint to 2.2.0 and can break dependency resolution/builds in subsequent releases.
Useful? React with 👍 / 👎.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Comment |
This pull request introduces several improvements focused on release automation, version consistency, and a significant internal rename for clarity. The main highlights include adding a new auto-tag workflow, enhancing the release workflow with changelog extraction and AI-style summaries, and renaming the
CookieJarclass toAsyncCookieJarthroughout the codebase for clarity and consistency.Release automation and version management:
.github/workflows/auto-tag.yml) that automatically creates a Git tag when a version bump is detected, ensuring the version is consistent acrosslibrary.json,library.properties, andsrc/HttpCommon.h..github/workflows/release.yml) to extract the changelog for the current version, generate an AI-style release summary, and use the new version as the release name. [1] [2]scripts/sync-version.sh) to synchronize version numbers across all source-of-truth files, enforcing semantic versioning and reducing human error.Internal refactoring and naming consistency:
CookieJarclass and related files toAsyncCookieJarfor improved clarity and alignment with the library's asynchronous design. This includes renaming files, updating class names, and modifying all usages throughout the codebase. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Version bump:
2.1.1in bothlibrary.jsonandlibrary.propertiesas part of the release process. [1] [2]Minor improvements:
AsyncHttpClientto 4096 words for improved stability.AsyncHttpClient::cleanupmethod to prevent recursion during request cleanup, improving reliability.