Repository navigation
refactor(image): #314 dedup robustness — scroll fallback + shared attach helper - #335
Merged
Merged
Conversation
…ach helper Follow-up from the /code-review + /ponytail-review (ultra) pass on #314: - **Scroll fallback (correctness):** `_try_select_existing_by_filename` now scrolls the virtualised picker grid (`_scroll_picker_grid_until_rendered`, #287) when the match isn't in the initial DOM — an existing dup that's off-screen after a broad search is selected, not silently re-uploaded. (Matches `_select_existing_asset`; also mitigates a project-switch race where the grid re-renders.) - **DRY (reuse):** extracted `_attach_selected_tile` — the tile-click → wait-hidden → else-include → verify block that was duplicated in `_select_existing_asset` and the filename path. Both now call it (surface/detail/screenshot/timeout as params, so the media-UUID path's telemetry is byte-identical; its tests confirm). - **Contract clarity:** docstring now states a found-but-unattachable match RAISES (real UI failure surfaced, not a silent re-upload) and notes the exact-filename key misses names Flow sanitises. New unit test for the scroll-fallback path; `_select_existing_asset`'s existing tests pass unchanged (extraction behaviour-preserving). pyright 0/0/0, 306 transport tests green. Live e2e re-verify pending.
15990166686ll-crypto
pushed a commit
to 15990166686ll-crypto/gflow-cli
that referenced
this pull request
Aug 24, 2026
…ared attach helper (ffroliva#335) Follow-up from the /code-review + /ponytail-review (ultra) pass on ffroliva#314: - **Scroll fallback (correctness):** `_try_select_existing_by_filename` now scrolls the virtualised picker grid (`_scroll_picker_grid_until_rendered`, ffroliva#287) when the match isn't in the initial DOM — an existing dup that's off-screen after a broad search is selected, not silently re-uploaded. (Matches `_select_existing_asset`; also mitigates a project-switch race where the grid re-renders.) - **DRY (reuse):** extracted `_attach_selected_tile` — the tile-click → wait-hidden → else-include → verify block that was duplicated in `_select_existing_asset` and the filename path. Both now call it (surface/detail/screenshot/timeout as params, so the media-UUID path's telemetry is byte-identical; its tests confirm). - **Contract clarity:** docstring now states a found-but-unattachable match RAISES (real UI failure surfaced, not a silent re-upload) and notes the exact-filename key misses names Flow sanitises. New unit test for the scroll-fallback path; `_select_existing_asset`'s existing tests pass unchanged (extraction behaviour-preserving). pyright 0/0/0, 306 transport tests green. Live e2e re-verify pending.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up hardening for #314 (dedup i2i refs), from running the real
/code-review(high) and/ponytail:ponytail-review(ultra) on the merged PR #334. Addresses every finding those reviews surfaced:_try_select_existing_by_filenamenow scrolls the virtualised picker grid (_scroll_picker_grid_until_rendered, video i2v: accept an in-project asset (media UUID) as --initial-frame instead of forcing a local-file upload #287) when the match isn't in the initial DOM — so an existing duplicate that's off-screen after a broad search is selected, not silently re-uploaded. The sibling_select_existing_assetalready did this; the dedup helper now matches it. Also mitigates a_sync_picker_projectswitch-then-race._attach_selected_tile— the tile-click → wait-hidden → else-include → verify block that was duplicated between_select_existing_assetand the filename path. Both call it now (surface/detail/screenshot/timeout as params, so the media-UUID path's telemetry is byte-identical — its tests pass unchanged).Review to consensus
/code-review(high):[](clean)./ponytail:ponytail-review(ultra): "Lean. Ship." — the duplication it flagged pre-refactor is resolved. One iteration (the change was built to close the prior findings).Test plan
_select_existing_asset's existing tests pass unchanged → the extraction is behavior-preserving./gflow:check: ruff clean, pyright src 0/0/0, hygiene (594 files) + doc-links clean, 306 transport tests pass.ffroliva(project 9d5b9a86, forced classic, credit-free):reference_deduped_by_filename, 0image_uploaded,status: ok, real 45 KB image — dedup still fires end-to-end after the refactor.🤖 Generated with Claude Code