Explain two undocumented traps in the bucket file listing and the form section hook - #1388
Explain two undocumented traps in the bucket file listing and the form section hook#1388mihow wants to merge 2 commits into
Conversation
list_files said "Returns an ObjectSummary object", which is wrong three ways: it is a generator, it yields tuples, and its last item is always (None, num_files_checked). Every caller already guards for that sentinel without saying why, so a new caller writing the obvious loop hits an AttributeError on the final iteration. The count it carries is load-bearing: test_connection() uses it to report an empty prefix separately from a prefix where the filter rejected every key. useSyncSectionStatus deliberately leaves setFormSectionStatus out of its effect deps. The callback is memoised on the form state it also replaces, so adding it loops forever. There is no react-hooks ESLint plugin configured to explain the omission or to catch a well-meant "fix", so record the reason. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for antenna-ssec canceled.
|
✅ Deploy Preview for antenna-preview canceled.
|
|
Warning Review limit reached
Next review available in: 47 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes clarify two existing contracts: S3 file-listing generator outputs and the React hook dependency exclusion for ChangesS3 Generator Contracts
Section Status Effect
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This PR only clarifies existing behavior in documentation and comments without changing runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Documents two “gotchas” that are currently encoded only in caller knowledge: the trailing marker yielded by S3 bucket listing helpers, and a deliberate React useEffect dependency omission that prevents an infinite render loop in multi-section forms.
Changes:
- Correct and expand
list_files()docstring to describe its generator contract, including the trailing(None, num_files_checked)marker. - Align
list_files_paginated()documentation with the same contract and point totest_connection()as a motivating caller. - Add an inline comment in
useSyncSectionStatusexplaining whysetFormSectionStatusis intentionally excluded from the effect deps.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
ui/src/utils/useSyncSectionStatus.ts |
Adds a short comment explaining a deliberate useEffect dependency omission that would otherwise tempt an “obvious” but looping change. |
ami/utils/s3.py |
Updates docstrings for bucket listing helpers to match actual yielded values and the trailing marker item relied on by callers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Yields ObjectTypeDef dicts instead of the ObjectSummary objects list_files yields; | ||
| the generator contract is otherwise the same, including the trailing | ||
| ``(None, num_files_checked)`` pair. See test_connection() for why that pair matters. |
There was a problem hiding this comment.
Claude says: Good catch, fixed in 551a394. The docstring now names the pair on both sides — (ObjectTypeDef, num_files_checked) rather than the (ObjectSummary, num_files_checked) pairs list_files yields — so the count is visible without having to infer it from the following sentence about the trailing pair.
Saying it yields ObjectTypeDef dicts reads as if the dict were the whole yielded value, which loses the count that sits alongside it. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Two places in the codebase quietly expect the next developer to already know something, and this pull request writes that knowledge down. Neither change affects behaviour — the only edits are a docstring and a two-line comment.
The first is in the storage layer that scans a bucket for new capture images. Its docstring describes a return value that does not match what the function actually produces, and it says nothing about a final marker item that the function always emits at the end. Every existing caller quietly works around that marker, so the code runs correctly today, but anyone writing a new caller from the docstring would get a crash on the last item. The second is a form hook used by the multi-section project settings forms, where a line that looks like an oversight is in fact deliberate: correcting it in the obvious way sends the form into an endless re-render loop, and there is no linter configured that would warn about either the omission or the "fix".
Both are the kind of thing that costs someone an afternoon exactly once, then gets forgotten again.
List of Changes
Returns an ObjectSummary object.line onlist_files()inami/utils/s3.pywith an accurate description of the(object, count)pairs it yields and the trailing(None, count)pair.list_files_paginated()docstring to state that it shares the generator contract and to referencetest_connection(), which uses the trailing pair's count to distinguish an empty location from one where the file filter matched nothing.useEffectinui/src/utils/useSyncSectionStatus.tsrecording thatsetFormSectionStatusis deliberately excluded, because it is memoised on the form state that it itself replaces.Notes for reviewers
ui/AGENTS.md. The file and its siblings carry no comments, so the bar for adding one is high — the justification here is that the trap is invisible:ui/.eslintrc.jsonconfigures noreact-hooksplugin, so neither the omission nor a well-meant correction produces a warning.pre-commit runon the changed Python file (black, isort, flake8, pyupgrade, django-upgrade) and, inui/,yarn format --check,yarn lintandyarn type-check— all clean.Summary by CodeRabbit