fix(image): reuse memory-cached renditions on mount (OK-63873) - #126
Conversation
Local Codex CLI reviewOpen the review session in Slack The review runs in an isolated temporary worktree. It may make bounded reproduction changes there, but never modifies the original project checkout or running systems, and continues in this session when the PR receives a substantive commit. |
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude review session: https://claude.ai/code/session_01Hjx1BHahasA5T9Nz3RXYZx |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1a9cfdd to
e4ca102
Compare
|
Addressed the three review findings (branch force-pushed, now based on #125 so the 3.0.157 release carries the P1 — probe key vs. preload / laid-out request
P2 — tests
P1 — version
|
|
Base is back on |
e4ca102 to
fa29148
Compare
2c42bcc to
d7b3aaf
Compare
…-63873) On iOS a freshly mounted OneKeyImage probes the memory cache from identityDidChange, before its first layout. The SDWebImage key carries the decode thumbnail size derived from the host view bounds, which are still empty at that point, so the probe never matched the entry the prewarm (or the previous mount) stored and every new token row painted an icon skeleton for at least one frame even when the logo was already in memory. Derive the probe size from the resizeWidth hint while the bounds are empty (a square, exactly what the preload uses for a width-only source), so the mount frame can show the cached image; a laid-out view keeps using its real bounds.
d7b3aaf to
3abd450
Compare
react-native-image and react-native-native-list were the only two Nitro packages without a package .gitignore, so their nitrogen/generated trees were committed. Every prop change then dragged a dozen generated C++, Kotlin, Swift and ObjC++ files into the diff — 117 files in total, all of them reproducible from the .nitro.ts spec. Ignore nitrogen/ in both packages, matching the other 23 Nitro packages and the scaffolding template, and drop the tracked output. react-native- image already ran `yarn nitrogen` in its release script; native-list did not, and its prepare step copies nitrogen into lib, so add the codegen step there too. Both packages keep `nitrogen` in their npm `files`, so the published tarballs are unchanged. Regenerating on top of the deleted tree reproduces the committed files byte for byte apart from trailing whitespace an editor had stripped. The image fix and this cleanup are recorded under Unreleased; no version bump, so the next release picks them up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3abd450 to
1f208d9
Compare
`round` clips the native view to an oval on Android (`addOval`) and to a `min(width, height) / 2` corner radius on iOS (`updateRoundMask`), but the RN wrapper that hosts a custom `placeholder` or `fallback` only ever clipped to the caller's own `borderRadius`. With `round` and no `borderRadius` the wrapper had no radius at all, so `overflow: hidden` clipped to a rectangle and the overlays were painted square over a round image — against the contract the README states, that image, placeholder and fallback share the same clipping. Give the overlay container and the drawn border overlay a percentage radius when `round` is set. It says "circle" without measuring the view, it is exactly Android's oval, and it matches iOS for the square icons `round` exists for. All the per-corner radii are set too, so a caller's `borderTopLeftRadius` cannot survive the round clip and leave one square corner. Tests assert the container clip overrides a caller's per-corner radius and that the border overlay follows; both fail without the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Cursor review failed. Remote error: |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
huhuanming
changed the title
|
Cursor review failed. Remote error: |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
3 participants
Problem
On iOS, a freshly mounted
OneKeyImagecould paint a skeleton for 3–5 frames even when the image was already decoded in memory. The commoncachePolicyarrived asnil, and a pre-layout request could derive a different thumbnail key from the later render or preload, so the synchronous probe missed and entered the asynchronous image pipeline.A second case remained after exact-key probing: the same raw image is requested at several view sizes. SDWebImage and Glide include decode size in their resource identity, so an already decoded larger or smaller rendition was ignored while the exact rendition loaded. The result was an avoidable blank/skeleton frame during account switches and native-list reuse.
The package also had no authoritative document for lifecycle, cache identity, event ownership, safety limits, or accepted iOS/Android differences, and the repository had no always-on native-module development rule requiring such a spec.
Fix
Exact and cross-size memory reuse
memory-diskdefault during the iOS probe.resizeWidth/resizeHeight, including the rendercontentFitand preload-compatiblecoverkey.resizeHeightto the Nitro view contract and reset it on reuse.disk, andnonepolicies do not participate.onlyRetrieveFromCache(true)and accepts only an immediate memory callback. A square target never reuses a circle-transformed bitmap; a circular target may reuse an untransformed bitmap because the host clips it.There is no fixed three-size cache family: TOS has five layout tiers with standard/high-density renditions, while the registry records up to eight variants actually used by a source family.
Observable behavior
onLoadoronError, followed byonLoadEnd.cacheType: memory. Android can display the exact entry immediately while the normal Glide request remains terminal.Specs and development requirements
native-views/react-native-image/docs/SPEC.mdas the authoritative Native Image contract: scope, definitions, API/defaults, lifecycle and event order, cache identity, cross-size selection, TOS tiers, safety limits, platform differences, conformance map, and acceptance matrix.docs/NATIVE_MODULE_DEVELOPMENT.mdrequiring repository-template initialization, a packagedocs/SPEC.md, defined boundaries/defaults/lifecycle/platform differences/limits, and a post-change code-versus-spec audit.AGENTS.mdas the always-loaded instruction entry, withCLAUDE.mdimporting the same rules, so detailed requirements stay in one document.yarn create:module/yarn create:viewgenerators.Verification
android.text.TextUtils.isEmptyis not mocked. The memory-variant and signature tests are not among those failures.git diff --check, and relative-link validation pass.