feat(journal): page-loop pageIndex=2..N until totleSize / short page#205
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughExtends SmartAccount.get_trip_journal() to page through journalLogV4 responses, normalizing pages (including misspelled totleSize), treating cloud 8153 as benign end-of-data mid-loop, optionally sleeping between pages, and validating accumulated counts (log WARNING or raise JournalTruncationError). ChangesJournal Pagination with Truncation Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pysmarthashtag/tests/test_journal_pagination.py (1)
231-261: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd type hints to the test and nested helper.
Both the test function and the nested
_dispatchhelper are missing type annotations. As per coding guidelines, Python typing is required for function signatures.✨ Proposed fix
`@pytest.mark.asyncio` -async def test_no_loop_when_first_page_short(smart_fixture: respx.Router): +async def test_no_loop_when_first_page_short(smart_fixture: respx.Router) -> None: """If page 1 is already short and totleSize matches, no second page is fetched.""" page1 = _page_body([_trip(1), _trip(2)], total=2, page_size=20) captured: list[httpx.Request] = [] - def _dispatch(request: httpx.Request) -> httpx.Response: + def _dispatch(request: httpx.Request) -> httpx.Response: captured.append(request)Note: The nested
_dispatchalready has a return type hint, so only the outer function needs updating.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 231 - 261, Add missing type hints on the test coroutine: change the signature of test_no_loop_when_first_page_short to include the fixture type and a return type, e.g. async def test_no_loop_when_first_page_short(smart_fixture: respx.Router) -> None:. Keep the nested helper _dispatch as-is (it already has def _dispatch(request: httpx.Request) -> httpx.Response) and do not modify its return annotation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pysmarthashtag/account.py`:
- Around line 64-69: In _unwrap_journal_page, handle string-encoded pagination
totals: locate symbols items_raw, pagination, total_raw and total in the
_unwrap_journal_page function and change the total extraction so it accepts str
as well as int/float (e.g., treat total_raw if isinstance(total_raw, (int,
float, str))), then convert to int with a safe parse (wrap int(total_raw) in
try/except catching ValueError/TypeError and fall back to None) so values like
"100" become 100 while preserving None on invalid input.
In `@pysmarthashtag/tests/test_journal_pagination.py`:
- Around line 218-229: Add a return type annotation to the test function
signature: update the async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) to
include -> None so the signature becomes async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None,
keeping the rest of the body unchanged; this satisfies the project typing
guideline for test functions.
- Around line 170-190: The test function test_mid_loop_other_error_propagates is
missing a return type annotation; add the Python typing return of None to its
signature (i.e., change "async def
test_mid_loop_other_error_propagates(smart_fixture: respx.Router):" to include
"-> None") so the function signature follows project typing guidelines and tools
like mypy; ensure only the signature is changed and no runtime behavior is
altered.
- Around line 113-139: The test function test_short_page_stops_loop is missing a
type hint for its smart_fixture parameter; update the function signature to
annotate smart_fixture with the respx.Router type (i.e., smart_fixture:
respx.Router) so it matches typing guidelines and static checks, and ensure
respx is imported where tests are defined if not already.
- Around line 141-168: The test function test_mid_loop_8153_breaks_loop
currently lacks a return type annotation; update its signature to include an
explicit return type (async def test_mid_loop_8153_breaks_loop(smart_fixture:
respx.Router) -> None:) so it conforms to the typing guideline; verify the
nested helper _dispatch already has its parameter and return types (request:
httpx.Request) -> httpx.Response and only adjust it if any annotations are
missing.
- Around line 192-216: Add explicit typing for the test's return type by
annotating the function signature of test_truncation_warn_when_count_disagrees
to return None (i.e., add "-> None"); keep the existing parameter annotations
smart_fixture: respx.Router and caplog: pytest.LogCaptureFixture as-is and
ensure any required test typing imports are available in the module.
---
Outside diff comments:
In `@pysmarthashtag/tests/test_journal_pagination.py`:
- Around line 231-261: Add missing type hints on the test coroutine: change the
signature of test_no_loop_when_first_page_short to include the fixture type and
a return type, e.g. async def test_no_loop_when_first_page_short(smart_fixture:
respx.Router) -> None:. Keep the nested helper _dispatch as-is (it already has
def _dispatch(request: httpx.Request) -> httpx.Response) and do not modify its
return annotation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e360742e-133a-45d0-8a05-e956ef0bc19a
📒 Files selected for processing (3)
pysmarthashtag/account.pypysmarthashtag/models.pypysmarthashtag/tests/test_journal_pagination.py
| @pytest.mark.asyncio | ||
| async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router): | ||
| """Mid-loop 8153 is benign end-of-data, not an error. | ||
|
|
||
| Page 2's ``code: 8153`` ("data unavailable") breaks the loop and | ||
| returns what's accumulated; the caller does not see an exception. | ||
| """ | ||
| page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2) | ||
|
|
||
| def _dispatch(request: httpx.Request) -> httpx.Response: | ||
| qs = parse_qs(urlparse(str(request.url)).query) | ||
| page_index = int((qs.get("pageIndex") or ["0"])[0]) | ||
| if page_index == 1: | ||
| return httpx.Response(200, json=page1) | ||
| return httpx.Response(200, json={"code": "8153", "message": "Connection interruption"}) | ||
|
|
||
| for base in (API_BASE_URL, API_BASE_URL_V2): | ||
| smart_fixture.get( | ||
| re.compile(re.escape(base + _JOURNAL_PATH + "TestVIN0000000001") + r".*") | ||
| ).mock(side_effect=_dispatch) | ||
|
|
||
| account = await prepare_account_with_vehicles() | ||
| # No raise — 8153 mid-loop is benign end-of-data. | ||
| merged = await account.get_trip_journal("TestVIN0000000001", page_size=2) | ||
|
|
||
| # Accumulated only page 1's 2 entries; page 2 was 8153. | ||
| assert len(merged["data"]["list"]) == 2 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints to the test and nested helper.
Both the test function and the nested _dispatch helper are missing type annotations. As per coding guidelines, Python typing is required for function signatures.
✨ Proposed fix
`@pytest.mark.asyncio`
-async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router):
+async def test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router) -> None:
"""Mid-loop 8153 is benign end-of-data, not an error.
Page 2's ``code: 8153`` ("data unavailable") breaks the loop and
returns what's accumulated; the caller does not see an exception.
"""
page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2)
- def _dispatch(request: httpx.Request) -> httpx.Response:
+ def _dispatch(request: httpx.Request) -> httpx.Response:
qs = parse_qs(urlparse(str(request.url)).query)Note: The nested _dispatch already has a return type hint, so only the outer function needs updating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 141 - 168, The
test function test_mid_loop_8153_breaks_loop currently lacks a return type
annotation; update its signature to include an explicit return type (async def
test_mid_loop_8153_breaks_loop(smart_fixture: respx.Router) -> None:) so it
conforms to the typing guideline; verify the nested helper _dispatch already has
its parameter and return types (request: httpx.Request) -> httpx.Response and
only adjust it if any annotations are missing.
Source: Coding guidelines
| @pytest.mark.asyncio | ||
| async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router): | ||
| """A non-8153 cloud error mid-loop propagates as ``HTTPStatusError``.""" | ||
| page1 = _page_body([_trip(1), _trip(2)], total=10, page_size=2) | ||
|
|
||
| def _dispatch(request: httpx.Request) -> httpx.Response: | ||
| qs = parse_qs(urlparse(str(request.url)).query) | ||
| page_index = int((qs.get("pageIndex") or ["0"])[0]) | ||
| if page_index == 1: | ||
| return httpx.Response(200, json=page1) | ||
| return httpx.Response(200, json={"code": "5000", "message": "Internal server error"}) | ||
|
|
||
| for base in (API_BASE_URL, API_BASE_URL_V2): | ||
| smart_fixture.get( | ||
| re.compile(re.escape(base + _JOURNAL_PATH + "TestVIN0000000001") + r".*") | ||
| ).mock(side_effect=_dispatch) | ||
|
|
||
| account = await prepare_account_with_vehicles() | ||
| with pytest.raises(httpx.HTTPStatusError): | ||
| await account.get_trip_journal("TestVIN0000000001", page_size=2) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hint for the test function.
The test function is missing a return type annotation. As per coding guidelines, Python typing is required for function signatures.
✨ Proposed fix
`@pytest.mark.asyncio`
-async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router):
+async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router) -> None:
"""A non-8153 cloud error mid-loop propagates as ``HTTPStatusError``."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 170 - 190, The
test function test_mid_loop_other_error_propagates is missing a return type
annotation; add the Python typing return of None to its signature (i.e., change
"async def test_mid_loop_other_error_propagates(smart_fixture: respx.Router):"
to include "-> None") so the function signature follows project typing
guidelines and tools like mypy; ensure only the signature is changed and no
runtime behavior is altered.
Source: Coding guidelines
| @pytest.mark.asyncio | ||
| async def test_truncation_warn_when_count_disagrees( | ||
| smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture | ||
| ): | ||
| """Truncation logs a WARNING by default; loop returns the partial result. | ||
|
|
||
| Routine polls keep going so the next iteration can pick up the | ||
| missing trips, instead of failing the whole refresh on a mismatch. | ||
| """ | ||
| # Cloud reports totleSize=10 but only delivers 1 short page → mismatch. | ||
| page1 = _page_body([_trip(1)], total=10, page_size=2) | ||
| _install_paginated_routes(smart_fixture, {1: page1}) | ||
|
|
||
| account = await prepare_account_with_vehicles() | ||
| with caplog.at_level(logging.WARNING, logger="pysmarthashtag.account"): | ||
| merged = await account.get_trip_journal("TestVIN0000000001", page_size=2) | ||
|
|
||
| assert len(merged["data"]["list"]) == 1 | ||
| assert merged["data"]["pagination"]["totleSize"] == 10 | ||
| assert any( | ||
| "page-loop truncation" in record.message | ||
| for record in caplog.records | ||
| if record.levelno == logging.WARNING | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints for the test function parameters.
The test function is missing type annotations for its parameters. As per coding guidelines, Python typing is required for function signatures.
✨ Proposed fix
`@pytest.mark.asyncio`
async def test_truncation_warn_when_count_disagrees(
- smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture
+ smart_fixture: respx.Router, caplog: pytest.LogCaptureFixture
-):
+) -> None:
"""Truncation logs a WARNING by default; loop returns the partial result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 192 - 216, Add
explicit typing for the test's return type by annotating the function signature
of test_truncation_warn_when_count_disagrees to return None (i.e., add "->
None"); keep the existing parameter annotations smart_fixture: respx.Router and
caplog: pytest.LogCaptureFixture as-is and ensure any required test typing
imports are available in the module.
Source: Coding guidelines
| @pytest.mark.asyncio | ||
| async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router): | ||
| """``raise_on_truncation=True`` upgrades the WARNING to a hard failure.""" | ||
| page1 = _page_body([_trip(1)], total=10, page_size=2) | ||
| _install_paginated_routes(smart_fixture, {1: page1}) | ||
|
|
||
| account = await prepare_account_with_vehicles() | ||
| with pytest.raises(JournalTruncationError): | ||
| await account.get_trip_journal( | ||
| "TestVIN0000000001", page_size=2, raise_on_truncation=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hint for the test function.
The test function is missing a return type annotation. As per coding guidelines, Python typing is required for function signatures.
✨ Proposed fix
`@pytest.mark.asyncio`
-async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router):
+async def test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None:
"""``raise_on_truncation=True`` upgrades the WARNING to a hard failure."""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pysmarthashtag/tests/test_journal_pagination.py` around lines 218 - 229, Add
a return type annotation to the test function signature: update the async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) to
include -> None so the signature becomes async def
test_truncation_raises_when_caller_opts_in(smart_fixture: respx.Router) -> None,
keeping the rest of the body unchanged; this satisfies the project typing
guideline for test functions.
Source: Coding guidelines
The previous get_trip_journal hard-coded pageIndex=1 and silently
dropped trips beyond the first page when totleSize > page_size. Refactor
the request into a private _fetch_journal_page helper and wrap it in a
loop that fetches pages 2, 3, ... until either:
* the cloud-reported totleSize is reached, OR
* the previous page came back short (cloud's "no more pages" signal)
Mid-loop code=8153 ("data unavailable") is treated as benign
end-of-data: break the loop and return what's accumulated. Page 1's
8153 still propagates as HTTPStatusError so existing callers with their
own benign-empty handling (e.g. get_vehicles) keep working unchanged.
After the loop, if the accumulated count disagrees with totleSize:
* default (raise_on_truncation=False): log a WARNING with both
numbers and return the partial list — routine polls keep going so
the next iteration can pick up the missing trips.
* raise_on_truncation=True: raise JournalTruncationError instead.
Backfill / archive callers opt into the hard failure since
silent data loss is worse than a hard failure that forces an
operator to look.
The returned dict preserves the original first-page response shape
(code/message/data) but with data.list containing the merged entries
from every page; data.pagination.totleSize carries the cloud's most
recent reported total. Existing TripJournal.from_response works
unchanged since it only reads data.list[0] + totleSize.
Adds page_gap_seconds kwarg (default 0.0) so polite-API callers can
insert a sleep between fetches without monkey-patching.
c438bc7 to
9beaeb9
Compare
|
Thanks @coderabbitai! Applied (
Declining (convention): The Ruff clean, full suite passes. |
|
Tip For best results, initiate chat on the files or code changes.
The Your convention argument on 🐇 ✨ ✏️ Learnings added
|
Builds on the trip-journal foundation merged in #194 (same author). That PR added the page-1 fetch; this PR teaches the same method to walk subsequent pages when the cloud reports more entries than fit on page 1.
Summary
account.get_trip_journalpreviously fetched onlypageIndex=1(server caps at 20 entries) and returned whatever the cloud sent, even whenpagination.totleSizereported more trips available. Accounts with deeper history saw silent truncation. This change loopspageIndex=2..Nuntillen(accumulated) >= totleSize, the previous page is short, or the cloud signals end-of-data via code8153mid-loop, then returns the page-1 dict withdata.listreplaced by the merged accumulation anddata.pagination.totleSizerefreshed. Single-page accounts (the common case) are byte-identical to the prior behaviour.What's new
pysmarthashtag/account.py—get_trip_journalgains two keyword-defaulted kwargs:data.listpreserving the cloud's desc-by-time ordering across pages._fetch_journal_pagefactored from the old single-page body. Same 3-retry semantics forSmartTokenRefreshNecessary/SmartHumanCarConnectionErroras before._is_benign_empty,_unwrap_journal_pageand constant_BENIGN_EMPTY_CODE = "8153".pysmarthashtag/models.py— newJournalTruncationError(SmartAPIError). SubclassesSmartAPIErrorso existing broad-except handlers still catch it; the distinct type lets backfill callers distinguish "transient cloud hiccup" from "page-loop terminated short" without string-matching messages.Behaviour
Loop termination conditions, evaluated before each
pageIndex=Nfetch:len(accumulated) >= totleSizefrom the page-1 response — stop, return merged.last_page_count < page_size) — stop. Cloud has no more entries even iftotleSizedisagrees.code == 8153— treat as end-of-data signal, stop without raising.httpx.HTTPStatusError.[]but totleSize not yet reached) — stop.Page-1 8153 still raises
httpx.HTTPStatusError. This asymmetry is deliberate:get_vehicleshas its own benign-empty handling around the page-1 call, so flipping page-1 semantics would silently shift error-handling responsibility. Mid-loop 8153 is the cloud's documented "no more data" signal, hence treated as a terminator.raise_on_truncationdecision tree whenlen(accumulated) != totleSizeafter the loop exits:raise_on_truncation=False(default): WARNING logged with VIN sanitized, partial merged response returned,totleSizepreserved from the cloud's reported value.raise_on_truncation=True:JournalTruncationErrorraised with the accumulated/expected counts in the message.Page-1 server failure still returns
{}— matches the siblingget_vehicle_ota_infopattern and the prior single-page contract. The one subtle delta: page-1 success with empty/Nonedata.listnow returns the raw first-page dict instead of entering the loop.TripJournal.from_responseinpysmarthashtag/vehicle/journal.pyhandles missing-list gracefully, so this is functionally equivalent.Backward compatibility
No caller breakage. Existing call sites passing
(vin),(vin, page_size=...), or(vin, page_size=..., window_days=...)work unchanged — the new kwargs are defaulted. Return shape is stilldictwithcode/message/success/data, anddata.list+data.pagination.totleSizepresent.TripJournal.from_responseonly readsdata.list[0]andpagination.totleSize, so the merged shape is invariant.total_tripsnow reflects the final cloud-reported total rather than the page-1 total — almost always identical; only differs if the cloud rotated total mid-loop, in which case the new value is strictly more correct.get_vehicles' internal call and its existing benign-empty handling (covered bytests/test_journal.py) are untouched.JournalTruncationErroris opt-in viaraise_on_truncation=True; default callers never see it.Why these defaults
raise_on_truncation=False: routine polling (consumer coordinators running every 60-300s) cannot afford a hard failure on a partial page — the next iteration picks up the missing trips. A WARNING surfaces the anomaly without breaking the user's setup. Backfill and archive callers — where silent loss IS worse than aborting — opt in withraise_on_truncation=True. This mirrors the existing pattern inget_vehicles, which catches journal errors with_LOGGER.debug(exc_info=True)rather than crashing the refresh for one bad vehicle.page_gap_seconds=0.0: the SDK is a transport library, not a rate-limiter. Polite-API pacing is policy that belongs to the consumer — coordinators, batch backfill scripts, and interactive CLIs all want different gaps. Defaulting to 0 keeps single-page accounts (the common case) identical to pre-loop behaviour: zero added latency, zero behaviour change. Consumers who want pacing pass an explicit value, matching the pattern used elsewhere in the codebase (no built-in retry backoff in the existing request-retry loops either). The guardif page_gap_seconds > 0beforeawait asyncio.sleep(...)is technically redundant —asyncio.sleep(0)is a documented event-loop yield — but it avoids the unnecessary scheduling round-trip on the default path.Test coverage
New file
pysmarthashtag/tests/test_journal_pagination.py(260 lines, 7 respx-driven scenarios, every while-loop branch exercised):test_loops_until_totleSize_reached— 2-page loop, asserts merged order preserves cloud desc-by-time ordering across page boundaries and thattotleSizematches.test_short_page_stops_loop— short page (1 item withpage_size=2) terminates the loop even whentotleSizeclaims more; verifies nopageIndex=2request is issued.test_mid_loop_8153_breaks_loop—pageIndex=2returnscode=8153; no exception, returns page-1 accumulation.test_mid_loop_other_error_propagates—pageIndex=2returnscode=5000; raiseshttpx.HTTPStatusError.test_truncation_warn_when_count_disagrees— default behaviour: WARNING logged, partial returned,totleSize=10preserved in the merged response.test_truncation_raises_when_caller_opts_in—raise_on_truncation=TrueraisesJournalTruncationError.test_no_loop_when_first_page_short— page-1 returns 2 of 2 (total reached + short): nopageIndex=2issued.Existing
tests/test_journal.pycontinues to cover end-to-endget_vehicles→ journal population and the page-1 8153 survival path, both still relevant after the refactor.Known coverage gaps, called out for transparency:
page_gap_secondsis not directly asserted (callsasyncio.sleepstraight through); theSmartTokenRefreshNecessary/SmartHumanCarConnectionErrorretry path inside_fetch_journal_pageis unchanged but uncovered (was uncovered before too); grant rotation mid-loop is not explicitly tested but relies on the existing inner retry. Happy to add any of these if reviewers want.Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests