mirror of
https://github.com/ZSeven-W/openpencil.git
synced 2026-06-01 03:14:29 +07:00
fix(ai): image search skips placeholders already filled with image fill
Previous fix left `role: 'image-placeholder'` on the frame even after
its fill was swapped to `[{type:'image', url, mode:'crop'}]` — the role
is what makes "this slot is meant to hold a photo" semantics survive
into history / codegen / downstream tooling, so stripping it would
trade one regression for another.
But that meant any follow-up generation (which calls
`resetImageSearchQueue` to clear `queuedNodeIds`) would re-walk the
tree, re-enqueue the same placeholder via role match, and overwrite
the already-good photo with whatever the next search returned.
`isUnfilledImagePlaceholderFrame` now gates every read: role match AND
fill is not already `type: 'image'`. Used in three places:
- `collectImageSearchTargets` only collects unfilled placeholders.
- `enqueueImageForSearch` early-returns if the caller passes an
already-filled placeholder (defense-in-depth for direct callers).
- `processQueue`'s re-check uses it instead of a plain
`isImagePlaceholderFrame`, so even a stale queue entry from before
someone else filled the frame gets dropped.
4 new tests in image-search-pipeline.test.ts cover the predicate
(default solid fill = unfilled; missing/empty fill = unfilled; image
fill = filled; non-placeholder role = always false) and a regression
test in `collectImageSearchTargets` that keeps the already-filled
placeholder out of the result while still picking up its sibling.
This commit is contained in:
parent
3f5bae0324
commit
d57f9816c2
2 changed files with 129 additions and 7 deletions
|
|
@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach } from 'vitest';
|
|||
import {
|
||||
inferAspectRatio,
|
||||
isImagePlaceholderFrame,
|
||||
isUnfilledImagePlaceholderFrame,
|
||||
collectImageSearchTargets,
|
||||
} from '../image-search-pipeline';
|
||||
import { useDocumentStore } from '@/stores/document-store';
|
||||
|
|
@ -64,6 +65,58 @@ describe('isImagePlaceholderFrame', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('isUnfilledImagePlaceholderFrame', () => {
|
||||
it('returns true for placeholder frame with the default solid gray fill', () => {
|
||||
const frame = {
|
||||
id: 'p',
|
||||
type: 'frame',
|
||||
role: 'image-placeholder',
|
||||
width: 200,
|
||||
height: 140,
|
||||
fill: [{ type: 'solid', color: '#F1F5F9' }],
|
||||
children: [],
|
||||
} as PenNode;
|
||||
expect(isUnfilledImagePlaceholderFrame(frame)).toBe(true);
|
||||
});
|
||||
|
||||
it('returns true when fill is missing or empty', () => {
|
||||
const noFill = {
|
||||
id: 'p',
|
||||
type: 'frame',
|
||||
role: 'image-placeholder',
|
||||
width: 200,
|
||||
height: 140,
|
||||
} as PenNode;
|
||||
const emptyFill = { ...noFill, fill: [] } as PenNode;
|
||||
expect(isUnfilledImagePlaceholderFrame(noFill)).toBe(true);
|
||||
expect(isUnfilledImagePlaceholderFrame(emptyFill)).toBe(true);
|
||||
});
|
||||
|
||||
it('returns false once the placeholder has been filled with an image', () => {
|
||||
const filled = {
|
||||
id: 'p',
|
||||
type: 'frame',
|
||||
role: 'image-placeholder',
|
||||
width: 200,
|
||||
height: 140,
|
||||
fill: [{ type: 'image', url: 'https://cdn/burger.jpg', mode: 'crop' }],
|
||||
children: [],
|
||||
} as PenNode;
|
||||
expect(isUnfilledImagePlaceholderFrame(filled)).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false for non-placeholder frames regardless of fill', () => {
|
||||
const card = {
|
||||
id: 'c',
|
||||
type: 'frame',
|
||||
role: 'card',
|
||||
fill: [{ type: 'solid', color: '#FFFFFF' }],
|
||||
children: [],
|
||||
} as PenNode;
|
||||
expect(isUnfilledImagePlaceholderFrame(card)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('collectImageSearchTargets', () => {
|
||||
beforeEach(() => {
|
||||
// Fresh empty doc per test
|
||||
|
|
@ -167,4 +220,42 @@ describe('collectImageSearchTargets', () => {
|
|||
it('returns empty when root id missing', () => {
|
||||
expect(collectImageSearchTargets('nonexistent')).toEqual([]);
|
||||
});
|
||||
|
||||
it('does NOT collect placeholder frames that already have an image fill', () => {
|
||||
// A previous scan painted this placeholder; the role stays
|
||||
// (semantic survival) but the fill is now `type:'image'`. A
|
||||
// follow-up scan must NOT re-enqueue it.
|
||||
const filledPlaceholder = {
|
||||
id: 'ph-old',
|
||||
type: 'frame',
|
||||
role: 'image-placeholder',
|
||||
width: 320,
|
||||
height: 200,
|
||||
fill: [{ type: 'image', url: 'https://cdn/saved.jpg', mode: 'crop' }],
|
||||
children: [],
|
||||
} as PenNode;
|
||||
const newPlaceholder = {
|
||||
id: 'ph-new',
|
||||
type: 'frame',
|
||||
role: 'image-placeholder',
|
||||
width: 200,
|
||||
height: 140,
|
||||
fill: [{ type: 'solid', color: '#F1F5F9' }],
|
||||
children: [],
|
||||
} as PenNode;
|
||||
const root = {
|
||||
id: 'root',
|
||||
type: 'frame',
|
||||
width: 360,
|
||||
height: 800,
|
||||
children: [filledPlaceholder, newPlaceholder],
|
||||
} as PenNode;
|
||||
useDocumentStore.setState({
|
||||
document: { children: [root], variables: [], themes: [], pages: [] },
|
||||
} as never);
|
||||
|
||||
const targets = collectImageSearchTargets('root');
|
||||
expect(targets).toHaveLength(1);
|
||||
expect(targets[0].node.id).toBe('ph-new');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -29,6 +29,27 @@ export function isImagePlaceholderFrame(node: PenNode | undefined | null): boole
|
|||
return (node as PenNode & { role?: string }).role === 'image-placeholder';
|
||||
}
|
||||
|
||||
/**
|
||||
* True when the frame is a placeholder AND still carries the gray
|
||||
* solid fill (i.e. has not yet been filled by a previous scan). The
|
||||
* filled-frame flag we leave on the node is the fill itself: once a
|
||||
* scan succeeds it becomes `[{type:'image', url, mode:'crop'}]`. We
|
||||
* INTENTIONALLY don't strip `role: 'image-placeholder'` on fill — the
|
||||
* role is what makes "this frame is meant to hold a photo" semantics
|
||||
* survive into history, codegen, and downstream tooling — but we do
|
||||
* gate every search-pipeline read on the fill check so a future scan
|
||||
* (e.g. from a follow-up generation that resets queuedNodeIds and
|
||||
* re-walks the tree) doesn't re-fetch and replace an already-good photo.
|
||||
*/
|
||||
export function isUnfilledImagePlaceholderFrame(node: PenNode | undefined | null): boolean {
|
||||
if (!isImagePlaceholderFrame(node)) return false;
|
||||
const fill = (node as PenNode & { fill?: unknown }).fill;
|
||||
if (!Array.isArray(fill) || fill.length === 0) return true;
|
||||
const first = fill[0] as { type?: unknown } | undefined;
|
||||
// Already painted with an image fill = filled, skip.
|
||||
return first?.type !== 'image';
|
||||
}
|
||||
|
||||
interface ImageSearchTarget {
|
||||
node: PenNode;
|
||||
kind: 'image' | 'placeholder-frame';
|
||||
|
|
@ -44,10 +65,14 @@ export function collectImageSearchTargets(rootId: string): ImageSearchTarget[] {
|
|||
if (node.type === 'image') {
|
||||
if (isPlaceholderSrc((node as ImageNode).src)) targets.push({ node, kind: 'image' });
|
||||
} else if (isImagePlaceholderFrame(node)) {
|
||||
targets.push({ node, kind: 'placeholder-frame' });
|
||||
// Don't descend into placeholder children (icon_font + label) — they
|
||||
// get wiped on fill anyway, and treating them as separate targets
|
||||
// would just be wasted work.
|
||||
// Only enqueue if not already painted with an image fill — see
|
||||
// `isUnfilledImagePlaceholderFrame` for the rationale on keeping the
|
||||
// role around after fill. Either way, don't descend into children:
|
||||
// unfilled placeholder children (icon_font + label) get wiped on
|
||||
// fill, and filled placeholders don't have children anymore.
|
||||
if (isUnfilledImagePlaceholderFrame(node)) {
|
||||
targets.push({ node, kind: 'placeholder-frame' });
|
||||
}
|
||||
return;
|
||||
}
|
||||
if ('children' in node && Array.isArray(node.children)) {
|
||||
|
|
@ -132,7 +157,7 @@ export function enqueueImageForSearch(node: PenNode): void {
|
|||
if (node.type === 'image') {
|
||||
if (!isPlaceholderSrc((node as ImageNode).src)) return;
|
||||
kind = 'image';
|
||||
} else if (isImagePlaceholderFrame(node)) {
|
||||
} else if (isUnfilledImagePlaceholderFrame(node)) {
|
||||
kind = 'placeholder-frame';
|
||||
} else {
|
||||
return;
|
||||
|
|
@ -184,8 +209,14 @@ async function processQueue(): Promise<void> {
|
|||
if (currentNode) {
|
||||
if (item.kind === 'image' && currentNode.type === 'image') {
|
||||
stillNeedsFill = isPlaceholderSrc((currentNode as ImageNode).src);
|
||||
} else if (item.kind === 'placeholder-frame' && isImagePlaceholderFrame(currentNode)) {
|
||||
stillNeedsFill = true;
|
||||
} else if (item.kind === 'placeholder-frame') {
|
||||
// Strict re-check: the frame must STILL be a placeholder AND not
|
||||
// already filled with an image. Without the fill check, a
|
||||
// follow-up generation that resets `queuedNodeIds` and re-runs
|
||||
// `scanAndFillImages` on the same tree would re-search the
|
||||
// already-good photo and overwrite it with whatever the new
|
||||
// search returns.
|
||||
stillNeedsFill = isUnfilledImagePlaceholderFrame(currentNode);
|
||||
}
|
||||
}
|
||||
if (!stillNeedsFill) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue