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.
src/AsyncHttpClient.h
Outdated
| 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.
![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".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps library version to 2.1.1; adds automated tagging and enhanced release-note generation workflows; introduces scripts to sync/verify version fields; renames CookieJar → AsyncCookieJar; adds a reentrancy guard in AsyncHttpClient; ensures transport close() invokes callbacks before closing. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Repo as Repository (files)
participant Git as Git/Remote (origin)
GH->>Repo: Trigger on push to main affecting manifests
GH->>Repo: Read `library.json` (jq -> version)
GH->>Repo: Validate version vs `library.properties` and `src/HttpCommon.h`
alt versions mismatch
GH-->>GH: Emit error annotation, abort
else versions match
GH->>Git: Check if tag v<version> exists
alt tag exists
GH-->>GH: Emit notice "tag exists", skip
else tag missing
GH->>Git: Create annotated tag v<version>
GH->>Git: Push tag to origin
GH-->>GH: Emit notice "tag created"
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-release.sh (1)
13-17:⚠️ Potential issue | 🟡 MinorUse strict
jqextraction to properly detect missing/null versions.The current
jq -r '.version'at line 13 outputs the literal string"null"when the version key is missing from library.json. The check[[ -n "${VERSION_JSON}" ]]passes because"null"is non-empty, allowing the failure to go undetected. This leaves VERSION_JSON containing the string"null"instead of failing cleanly.Use the
-eflag with a proper filter to fail if the version is missing or invalid:Suggested fix
-VERSION_JSON=$(jq -r '.version' library.json) +VERSION_JSON=$(jq -er '.version | select(type=="string" and length>0)' library.json)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-release.sh` around lines 13 - 17, The VERSION_JSON extraction can silently return the string "null"; update the VERSION_JSON assignment to invoke jq with the -e flag and a filter that yields the version or empties (e.g. use the alternative operator to return empty when .version is null/missing) so jq exits non-zero on missing/invalid versions; keep the existing VERSION_PROP check and ensure the script fails when jq returns non-zero (so VERSION_JSON will never be the literal "null").
🧹 Nitpick comments (1)
src/AsyncHttpClient.cpp (1)
38-38: Stack size increased from 2048 to 4096 words.This doubles the auto-loop task stack to 16KB on ESP32. While this addresses potential stack overflows (particularly during TLS operations with deep call stacks), it also doubles memory consumption for this task.
Consider documenting the rationale for this increase (e.g., observed stack overflow, TLS requirements) or making this configurable via a build-time macro if memory is constrained on some targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AsyncHttpClient.cpp` at line 38, The hardcoded stack size value "4096" in AsyncHttpClient.cpp should be made configurable and documented: replace the literal with a build-time macro (e.g., ASYNC_HTTP_CLIENT_TASK_STACK_WORDS or AUTO_LOOP_STACK_WORDS) and add a brief comment explaining why the stack was increased (observed stack overflow/TLS deep call stacks) and noting memory trade-offs; also add a default fallback value (4096) via `#ifndef/`#define so builds that don't define the macro keep the current behavior. Use the symbol/context around where the task is created (the auto-loop/task creation call that currently passes 4096) to locate and update the code and add the explanatory comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-version.sh`:
- Around line 23-96: The script currently logs "✗ ... not found" for missing
source-of-truth files (library.json, library.properties, src/HttpCommon.h) but
continues and can report success; change the file-check branches so the script
fails fast: when any of the if [[ -f ... ]] checks finds the file missing (the
branches that echo "✗ ... not found"), immediately exit with a non-zero status
(e.g., exit 1) or set a global failure flag and at the end call exit 1 if set;
update the echo lines accordingly so success ("✓ ...") is only reached when the
corresponding Python block ran and the script exits non-zero on any missing
file.
---
Outside diff comments:
In `@scripts/verify-release.sh`:
- Around line 13-17: The VERSION_JSON extraction can silently return the string
"null"; update the VERSION_JSON assignment to invoke jq with the -e flag and a
filter that yields the version or empties (e.g. use the alternative operator to
return empty when .version is null/missing) so jq exits non-zero on
missing/invalid versions; keep the existing VERSION_PROP check and ensure the
script fails when jq returns non-zero (so VERSION_JSON will never be the literal
"null").
---
Nitpick comments:
In `@src/AsyncHttpClient.cpp`:
- Line 38: The hardcoded stack size value "4096" in AsyncHttpClient.cpp should
be made configurable and documented: replace the literal with a build-time macro
(e.g., ASYNC_HTTP_CLIENT_TASK_STACK_WORDS or AUTO_LOOP_STACK_WORDS) and add a
brief comment explaining why the stack was increased (observed stack
overflow/TLS deep call stacks) and noting memory trade-offs; also add a default
fallback value (4096) via `#ifndef/`#define so builds that don't define the macro
keep the current behavior. Use the symbol/context around where the task is
created (the auto-loop/task creation call that currently passes 4096) to locate
and update the code and add the explanatory comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7debfd9d-210e-419f-a5f2-dd0009a2063c
📒 Files selected for processing (15)
.github/workflows/auto-tag.yml.github/workflows/release.ymllibrary.jsonlibrary.propertiesscripts/sync-version.shscripts/verify-release.shsrc/AsyncCookieJar.cppsrc/AsyncCookieJar.hsrc/AsyncHttpClient.cppsrc/AsyncHttpClient.hsrc/HttpCommon.hsrc/TcpTransport.cppsrc/TlsTransport.cpptest/test_cookies/test_main.cpptest/test_redirects/test_main.cpp
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.Summary by CodeRabbit
New Features
Bug Fixes
Chores