feat(dashboard): Focal point editor in shared asset preview dialog#4755
feat(dashboard): Focal point editor in shared asset preview dialog#4755grolmus wants to merge 9 commits into
Conversation
The standalone Asset detail route at `/assets/<id>` already lets the user
set a focal point on an asset. The same affordance was missing from the
shared `AssetPreview` dialog used by every `EntityAssets` gallery — so
opening a product or variant's featured asset gave you a preview with no
way to change its focal point.
This wires the existing `AssetFocalPointEditor` into `AssetPreview`:
- Side card gets a `FocusIcon` toggle plus a "Focal Point" coords readout
that shows the persisted value (or "Not set" when null).
- The main image is wrapped in `AssetFocalPointEditor` and switches into
edit mode on toggle. Confirming sends an inline
`UpdateAssetFocalPoint` mutation (defined here to avoid a
`lib → app/routes` import). On success we both:
1. seed a local `focalPointOverride` so the indicator updates
immediately, and
2. invoke a new `onAssetUpdated` callback so the parent
`EntityAssets` can update its own `assets` / `featuredAsset` /
`previewAsset` state. Re-opening the dialog on the same asset
now keeps the new value instead of regressing to the parent
query's stale snapshot.
- The mutation captures `assetId` at click time and the `onSuccess`
handler guards against that id so a late response after the user
navigated to a different asset can never write the focal point onto
the wrong asset's UI.
- Strings reuse the existing `Focal Point` / `Set Focal Point` msgids
from `assets_.$id.tsx` so locale catalogs don't grow duplicates.
The e2e regression test uploads a 1×1 transparent PNG via multipart
`createAssets` and exercises the full flow (set focal point, close
dialog, reopen, assert the new value persists). The e2e global-setup
now includes `AssetServerPlugin` (loaded dynamically to side-step
Playwright's Babel transform) so uploaded assets resolve to proper
http URLs instead of the testing-only `test-url/...` placeholder.
Reviewed by Nigel + vendurebot before commit; their HIGH / MEDIUM
feedback was applied (parent-sync callback, dead "Not set" branch
split, stale-closure guard, lingui casing, dialog-strings wrapped in
`<Trans>`, testid hooks for the featured-asset wrapper and the
focal-point editor confirm button, `typeof import(...)` for the
AssetServerPlugin dynamic import, `?? 0` index init).
Fixes vendurehq#4722
Encode the rules we keep re-deriving: - Plan + show diff before code on non-trivial changes - Run Nigel + vendurebot in parallel; apply HIGH/MEDIUM feedback - Tests are mandatory; verify pass-on-branch and fail-on-master - Manual smoke on the user's `localhost:5174` dashboard for UI changes - Commit only after explicit OK - Linear sync with PR link and verification summary - Recover, don't paper over (lost stash, killed port, polluted DB) Plus two gotchas we already hit this session: - Don't take port 3010 — it's the long-running dev backend. - Don't `git checkout HEAD -- file` on a branch without commits (HEAD resolves to master, working copy is wiped).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds focal-point editing to AssetPreview (local override state, GraphQL mutation, toasts, and AssetFocalPointEditor integration), exposes an onAssetUpdated callback through AssetPreviewDialog and synchronizes parent entity-assets state. E2E global setup registers an E2eAssetStorageStrategy and redirects importAssetsDir; a Playwright regression spec verifies focal-point persistence after closing/reopening the preview. Many locale .po files were updated with new focal-point/preview strings, and AGENTS.md received gotchas and a "Claude workflow" section. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
packages/dashboard/src/lib/components/shared/asset/asset-preview.tsx (1)
5-7: ⚡ Quick winUse the dashboard GraphQL import paths mandated by repo standards.
Please switch these to
@/graphql/api.jsand@/graphql/graphql.jsto keep module boundaries consistent in dashboard code.As per coding guidelines:
Import api from '@/graphql/api.js' and graphql from '@/graphql/graphql.js' for GraphQL queries and mutations.🤖 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 `@packages/dashboard/src/lib/components/shared/asset/asset-preview.tsx` around lines 5 - 7, Update the GraphQL import paths in the asset preview component: replace the current imports of api and graphql (symbols "api" and "graphql" in asset-preview.tsx) so they import from '`@/graphql/api.js`' and '`@/graphql/graphql.js`' respectively to follow dashboard module boundaries; keep the existing AssetFragment import unchanged unless further guidance requires it, then run a quick compile to ensure no unresolved imports remain.
🤖 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
`@packages/dashboard/e2e/tests/regression/issue-4722-asset-focal-point-in-preview-dialog.spec.ts`:
- Line 17: The afterEach teardown assumes the variable setup is always
initialized and can throw if beforeEach failed; update the afterEach hook to
guard against uninitialized setup by checking if setup is defined (e.g., if
(setup) or using optional chaining) before calling setup.teardown() or any
teardown methods, and ensure any async teardown is awaited only when setup
exists; apply this guard around uses of the setup variable referenced in the
afterEach block and similar cleanup code between lines ~106-135.
In `@packages/dashboard/src/lib/components/shared/asset/asset-preview.tsx`:
- Around line 211-220: AssetFocalPointEditor is being mounted with preserved
internal state across asset changes which can submit stale coordinates; to fix,
force a fresh editor instance whenever the active asset or edit session changes
by resetting the editor state or forcing a remount – e.g., derive a unique key
from activeAsset.id (or the session identifier) and pass it as the key prop to
AssetFocalPointEditor, or clear editorFocalPoint when activeAsset changes before
opening the editor so updateFocalPointMutation.mutate always receives
coordinates for the current asset.
---
Nitpick comments:
In `@packages/dashboard/src/lib/components/shared/asset/asset-preview.tsx`:
- Around line 5-7: Update the GraphQL import paths in the asset preview
component: replace the current imports of api and graphql (symbols "api" and
"graphql" in asset-preview.tsx) so they import from '`@/graphql/api.js`' and
'`@/graphql/graphql.js`' respectively to follow dashboard module boundaries; keep
the existing AssetFragment import unchanged unless further guidance requires it,
then run a quick compile to ensure no unresolved imports remain.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b17329cd-b39f-4fa5-bb92-ab9b067e6fef
📒 Files selected for processing (7)
AGENTS.mdpackages/dashboard/e2e/global-setup.tspackages/dashboard/e2e/tests/regression/issue-4722-asset-focal-point-in-preview-dialog.spec.tspackages/dashboard/src/lib/components/shared/asset/asset-focal-point-editor.tsxpackages/dashboard/src/lib/components/shared/asset/asset-preview-dialog.tsxpackages/dashboard/src/lib/components/shared/asset/asset-preview.tsxpackages/dashboard/src/lib/components/shared/entity-assets.tsx
michaelbromley
left a comment
There was a problem hiding this comment.
The feature implementation is in good shape. The wiring through AssetPreview is clean, the onAssetUpdated callback up to EntityAssets is the right call — it keeps the re-opened dialog consistent without forcing a parent refetch and avoids the regression where re-opening would show the stale focal point. The assetId-stamped mutation variable + matching guard in onSuccess is a nice defensive touch against late responses after the user navigates to a different asset. Reusing the existing Focal Point / Set Focal Point msgids from assets_.$id.tsx is the right call so the catalogs don't grow duplicates, and the data-testid hooks on entity-assets-featured, asset-preview-set-focal-point, asset-preview-focal-point-value, and asset-focal-point-editor-confirm are exactly the right anchors for the regression test. Good craft on the focal-point editor itself — happy to take that part as-is.
The change I'd like before merge is in the e2e test setup approach. The current setup pulls AssetServerPlugin into global-setup.ts via a dynamic import(...) (to dodge Playwright's Babel transform), provisions an assetUploadDir, rmSyncs it on each setup, and adds the full plugin to the merged Vendure config. This is what's causing all four e2e shards to fail in CI — @vendure/asset-server-plugin is not in the dashboard-e2e build scope (the workflow runs lerna run ci --scope @vendure/testing --include-dependencies, which doesn't reach asset-server-plugin because nothing in that dependency chain pulls it in), so the dynamic import resolves to a non-existent lib/index.js in CI:
Error: Cannot find package '/home/runner/work/vendure/vendure/node_modules/@vendure/asset-server-plugin/lib/index.js' imported from /home/runner/work/vendure/vendure/packages/dashboard/e2e/global-setup.ts
The underlying need is much smaller than the chosen fix. VendureImage does new URL(asset.preview) at vendure-image.tsx:165, and the default TestingAssetStorageStrategy.toAbsoluteUrl returns test-url/test-assets/<id> which isn't parseable as an absolute URL. The contract the test actually requires is "asset.preview parses as a URL" — nothing more. No image bytes need serving, no presets, no resizing, no disk I/O, no native sharp bindings.
A custom AssetStorageStrategy passed via assetOptions covers that contract in ~15 lines with no new dependencies, no CI workflow change, and no dynamic-import-with-cast workaround:
import { AssetStorageStrategy } from '@vendure/core';
import { Readable, Stream, Writable } from 'node:stream';
/**
* Minimal in-memory storage strategy used only by the dashboard e2e suite.
* Emits a parseable absolute URL so VendureImage's `new URL(asset.preview)`
* does not throw on assets created during tests. No bytes are persisted
* and no real HTTP server is required.
*/
class E2eAssetStorageStrategy implements AssetStorageStrategy {
toAbsoluteUrl(_req: any, identifier: string) {
return \`http://test-asset.local/\${identifier}\`;
}
writeFileFromBuffer(fileName: string) {
return Promise.resolve(\`test-assets/\${fileName}\`);
}
writeFileFromStream(fileName: string, data: Stream) {
return new Promise<string>((resolve, reject) => {
const w = new Writable({ write: (_c, _e, cb) => cb() });
data.pipe(w);
w.on('finish', () => resolve(\`test-assets/\${fileName}\`));
w.on('error', reject);
});
}
readFileToBuffer() { return Promise.resolve(Buffer.alloc(0)); }
readFileToStream() { const s = new Readable(); s.push(null); return Promise.resolve(s); }
fileExists() { return Promise.resolve(false); }
deleteFile() { return Promise.resolve(); }
}Then in mergeConfig:
const config = mergeConfig(defaultTestConfig, {
apiOptions: { port: VENDURE_PORT },
paymentOptions: { paymentMethodHandlers: e2ePaymentMethodHandlers },
assetOptions: {
assetStorageStrategy: new E2eAssetStorageStrategy(),
},
plugins: [CustomHistoryEntryPlugin], // drop AssetServerPlugin
customFields: e2eCustomFields,
});That should let you drop the AssetServerPlugin dynamic import, the assetUploadDir setup and teardown, and the as any cast on the plugin init. No CI build-scope changes are then needed and all four e2e shards should go green on the existing workflow.
A couple of small things to wrap up before merge:
-
i18n catalogs (
dashboard i18n syncfails): the new<Trans>strings (Asset,Preview of {asset.name},Edit focal point,Not set,Focal point updated,Failed to update focal point,Unknown error) need extracting. Runnpm run i18n:extract --workspace=@vendure/dashboardand commit the updated.pofiles. -
deploy failure: ignore — that's the standard Vercel-token-missing-on-fork-PRs infra issue, unrelated to this change.
The Cannot destructure property 'store' of useDialogRootContext(...) runtime error I hit locally testing this branch was the Dialog title context bug from #4739, which has now landed on master and is included via your branch update — confirmed the dialog renders cleanly with the merge in.
Nice work on the feature — looking forward to seeing this go in once the test setup is trimmed down.
Adds a key on AssetFocalPointEditor so it remounts when the active asset or edit session changes. Without it, navigating between assets in the preview dialog could submit the previous asset's stale focal-point coordinates. Also clarifies the mutation onSuccess comment: the parent is always notified (keyed by id), only the local on-screen override is guarded. Relates to vendurehq#4722
Replaces the AssetServerPlugin dynamic import (not in the dashboard-e2e build scope, which broke the CI shards), the assetUploadDir setup/teardown and the `as any` cast with a ~30-line in-memory E2eAssetStorageStrategy that emits a parseable absolute URL for VendureImage's `new URL(...)`. global-setup also points importExportOptions.importAssetsDir at the core e2e fixture images so the seeded "Laptop" product gets a real featured asset; the spec then reads its id and drives the UI instead of uploading an asset and creating a product at runtime. beforeEach resets the focal point to null so each run (incl. a retry reusing the global-setup DB) starts from a deterministic "Not set" baseline, asserted explicitly. Relates to vendurehq#4722
Adds the new msgids from the asset preview focal-point editor (source en filled, other locales left empty for the batch translation workflow). Relates to vendurehq#4722
|
Thanks for the detailed review @michaelbromley — addressed the e2e setup concern (and a bit more). Just pushed. E2e setup
i18n — ran Extra — added a No CI build-scope change needed; the four e2e shards should go green now. Verified: (Minor, pre-existing / out of scope: the dialog title doesn't update when navigating prev/next inside the dialog — happy to follow up separately.) |
…focal-points-on-variant-asset-preview # Conflicts: # packages/dashboard/src/i18n/locales/ar.po # packages/dashboard/src/i18n/locales/bg.po # packages/dashboard/src/i18n/locales/cs.po # packages/dashboard/src/i18n/locales/de.po # packages/dashboard/src/i18n/locales/en.po # packages/dashboard/src/i18n/locales/es.po # packages/dashboard/src/i18n/locales/fa.po # packages/dashboard/src/i18n/locales/fr.po # packages/dashboard/src/i18n/locales/he.po # packages/dashboard/src/i18n/locales/hr.po # packages/dashboard/src/i18n/locales/hu.po # packages/dashboard/src/i18n/locales/it.po # packages/dashboard/src/i18n/locales/ja.po # packages/dashboard/src/i18n/locales/nb.po # packages/dashboard/src/i18n/locales/ne.po # packages/dashboard/src/i18n/locales/nl.po # packages/dashboard/src/i18n/locales/pl.po # packages/dashboard/src/i18n/locales/pt_BR.po # packages/dashboard/src/i18n/locales/pt_PT.po # packages/dashboard/src/i18n/locales/ro.po # packages/dashboard/src/i18n/locales/ru.po # packages/dashboard/src/i18n/locales/sv.po # packages/dashboard/src/i18n/locales/tr.po # packages/dashboard/src/i18n/locales/uk.po # packages/dashboard/src/i18n/locales/zh_Hans.po # packages/dashboard/src/i18n/locales/zh_Hant.po
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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 `@packages/dashboard/e2e/global-setup.ts`:
- Around line 40-45: The promise returned by writeFileFromStream doesn't listen
for errors on the source stream (data), so source-stream errors never reject and
hang; update writeFileFromStream to attach an 'error' handler to the source
stream (data) that rejects the promise, and ensure you remove/cleanup listeners
as appropriate (also consider calling data.unpipe(w) on error) while keeping the
existing Writable (w) 'finish' and 'error' handling so any error from either
side rejects the promise with the actual error.
In `@packages/dashboard/src/i18n/locales/cs.po`:
- Around line 1289-1291: Populate the empty Czech translations (msgstr) for the
five new focal-point flow entries in
packages/dashboard/src/i18n/locales/cs.po—specifically fill in the translation
for msgid "Edit focal point" (used in asset-preview.tsx at the focal-point flow)
and the other four related focal-point msgid entries referenced at the other
locations so the Czech locale shows localized text instead of English; ensure
each msgstr contains the correct Czech phrase and preserve surrounding PO
formatting and encoding.
In `@packages/dashboard/src/i18n/locales/de.po`:
- Around line 1289-1291: Translate the new focal-point strings in de.po by
replacing the empty msgstr values for the focal-point-related msgid entries
(e.g., "Edit focal point" found alongside
src/lib/components/shared/asset/asset-preview.tsx:161) with appropriate German
translations, and do the same for the other focal-point occurrences indicated
(around entries at 2825-2827, 3071-3073, and 5005-5008) so the German UI no
longer falls back to English.
In `@packages/dashboard/src/i18n/locales/es.po`:
- Around line 1289-1299: The Spanish locale file is missing translations for new
UI strings used by AssetPreview and AssetPreviewDialog (e.g., msgid "Edit focal
point", "Asset", "Preview of {0}" and related success/error toasts), so update
the es.po entries by filling each empty msgstr with the correct Spanish phrasing
(preserve placeholders like {0} and any context/comment), and ensure the same
translations are applied for the other occurrences noted (additional ranges) so
the AssetPreview, AssetPreviewDialog and related toast messages display in
Spanish rather than falling back to English.
In `@packages/dashboard/src/i18n/locales/fa.po`:
- Around line 1289-1300: In packages/dashboard/src/i18n/locales/fa.po, fill the
empty msgstr values for the new focal-point related msgid entries (e.g., "Edit
focal point" from src/lib/components/shared/asset/asset-preview.tsx and "Asset"
from src/lib/components/shared/asset/asset-preview-dialog.tsx) with correct
Persian translations so the UI doesn’t fall back to English; apply the same fix
for the other ranges mentioned (2825-3073 and 5005-5008) by locating the
identical msgid strings in those sections and providing their proper fa
translations, then run your i18n extraction/validation to ensure no untranslated
msgstr remain.
In `@packages/dashboard/src/i18n/locales/fr.po`:
- Around line 1289-1300: Update the French .po entries that currently have empty
msgstr for the new focal-point UI strings: set msgstr for "Edit focal point",
"Asset", "Focal point updated", "Failed to update focal point", and "Preview of
{0}" to appropriate French translations (e.g., "Modifier le point focal",
"Ressource" or "Fichier", "Point focal mis à jour", "Échec de la mise à jour du
point focal", "Aperçu de {0}"), and ensure you also update the same strings at
the other occurrences mentioned (lines around 2825-2827, 3071-3073, 5005-5008)
so the French UI no longer falls back to English; keep placeholders like {0}
intact.
In `@packages/dashboard/src/i18n/locales/he.po`:
- Around line 1289-1299: The Hebrew locale file has empty msgstr entries for
several new msgid strings used in the focal-point flow and preview dialog (e.g.,
"Edit focal point", "Asset", "Focal point updated", "Failed to update focal
point", "Preview of {0}") so update packages/dashboard/src/i18n/locales/he.po by
providing proper Hebrew translations for those msgid keys and any other empty
msgstrs in the indicated ranges (lines referenced ~2825-3073 and ~5005-5008) to
ensure the dashboard doesn't fall back to English; keep placeholders (like {0})
intact in the translations and follow existing phrasing/format used elsewhere in
the file for consistency.
In `@packages/dashboard/src/i18n/locales/hr.po`:
- Around line 1289-1291: Several Croatian PO entries for the focal-point strings
are untranslated; update each msgstr for msgid "Edit focal point" (and the other
focal-point msgid occurrences noted) with a proper Croatian translation such as
"Uredi žarišnu točku" so the asset preview dialog/toasts are localized; ensure
you replace the empty msgstr values for all listed occurrences in the hr.po
file.
In `@packages/dashboard/src/i18n/locales/hu.po`:
- Around line 1289-1299: The Hungarian locale file hu.po is missing translations
for the new focal-point related strings so the UI/toasts fallback to English;
open hu.po and add appropriate Hungarian msgstr values for the msgid "Edit focal
point" (from src/lib/components/shared/asset/asset-preview.tsx) and for "Asset"
(from src/lib/components/shared/asset/asset-preview-dialog.tsx), and ensure any
other occurrences referenced (lines ~2815-2817, ~3061-3063, ~4992-4995) are
updated consistently; save the file so the asset focal-point UI/toasts use the
Hungarian translations.
In `@packages/dashboard/src/i18n/locales/it.po`:
- Around line 1289-1291: Replace the empty msgstr entries for the new
focal-point strings in the Italian PO catalog with proper Italian translations:
update msgid "Edit focal point" (and the other focal-point msgids referenced in
the review) by filling their msgstr with correct Italian text (e.g., "Modifica
punto focale" or equivalent) so the focal-point dialog and toast messages
display in Italian; ensure you update all occurrences noted in the review (the
other ranges containing focal-point entries) and keep the original msgid lines
unchanged.
In `@packages/dashboard/src/i18n/locales/ja.po`:
- Around line 1289-1299: Fill in the missing Japanese translations in
packages/dashboard/src/i18n/locales/ja.po for the new msgid entries: replace the
empty msgstr for "Edit focal point" (from
src/lib/components/shared/asset/asset-preview.tsx) and for "Asset" (from
src/lib/components/shared/asset/asset-preview-dialog.tsx) with appropriate
Japanese text; also update the same untranslated entries found in the other
ranges referenced (2825-3073 and 5005-5008) so the preview dialog labels/toasts
are localized consistently. Use clear, natural Japanese equivalents for each
msgid and keep the msgid strings unchanged.
In `@packages/dashboard/src/i18n/locales/nb.po`:
- Around line 1289-1299: Fill the missing Norwegian translations in nb.po for
the new focal-point and asset entries: set msgstr for "Edit focal point" to
"Rediger fokuspunkt" wherever that msgid appears, and set msgstr for "Asset" to
"Eiendel" (update all occurrences referenced in the comment: the entries with
msgid "Edit focal point" and "Asset"). Ensure you replace the empty msgstr
values (and any obsolete #~ blocks where appropriate) so the Norwegian locale
shows the translated text.
In `@packages/dashboard/src/i18n/locales/nl.po`:
- Around line 1293-1303: Add Dutch translations for the untranslated PO entries:
provide a proper Dutch msgstr for msgid "Edit focal point" and for msgid "Asset"
in the nl.po file (these appear as empty msgstr entries and also at the other
locations referenced); update the msgstr fields with the accurate Dutch phrases
(e.g., "Bijsnijdpunt bewerken" for "Edit focal point" and "Middel" or "Asset"
equivalent like "Asset" -> "Middel/Asset" as appropriate to the app terminology)
and ensure you repeat the same translations at the other occurrences (lines
referenced around 2819-2821, 3065-3067, 5000-5003) so the focal-point flow
displays in Dutch consistently.
In `@packages/dashboard/src/i18n/locales/pl.po`:
- Around line 1289-1291: The PO entries for the focal-point UI are missing
Polish translations: locate the msgid "Edit focal point" (from asset-preview.tsx
/ AssetPreview component) and all other focal-point-related msgid entries added
in this file and fill their corresponding msgstr with correct Polish
translations so the dialog/toasts show Polish text instead of falling back to
English.
In `@packages/dashboard/src/i18n/locales/pt_BR.po`:
- Around line 1289-1299: The pt_BR translations for the focal-point flow are
missing; update the msgstr entries for the msgid values "Edit focal point",
"Asset", "Preview of {0}", "Focal point updated", and "Failed to update focal
point" (and the same msgids at the other occurrences mentioned) in
packages/dashboard/src/i18n/locales/pt_BR.po by replacing the empty msgstr with
appropriate Brazilian Portuguese translations: "Editar ponto focal" for "Edit
focal point", "Arquivo" for "Asset", "Pré-visualização de {0}" for "Preview of
{0}", "Ponto focal atualizado" for "Focal point updated", and "Falha ao
atualizar o ponto focal" for "Failed to update focal point"; ensure you update
every occurrence referenced (including the other ranges) so the UI no longer
falls back to English.
In `@packages/dashboard/src/i18n/locales/pt_PT.po`:
- Around line 1289-1291: The pt_PT.po file contains empty msgstr values for new
focal-point/preview messages (e.g. msgid "Edit focal point" referenced from
src/lib/components/shared/asset/asset-preview.tsx at the given diff); populate
the corresponding msgstr entries with proper European Portuguese translations
for each msgid in the file (including the other occurrences noted around lines
1297-1299, 2825-2827, 3071-3073, 5005-5009), preserving punctuation and
placeholders, then run the i18n build/validation to ensure no syntax/encoding
errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 507f917c-53c1-4e92-b7f3-4fda0f0db046
📒 Files selected for processing (32)
packages/dashboard/e2e/global-setup.tspackages/dashboard/e2e/tests/catalog/custom-fields.spec.tspackages/dashboard/e2e/tests/regression/issue-4722-asset-focal-point-in-preview-dialog.spec.tspackages/dashboard/src/i18n/locales/ar.popackages/dashboard/src/i18n/locales/bg.popackages/dashboard/src/i18n/locales/cs.popackages/dashboard/src/i18n/locales/de.popackages/dashboard/src/i18n/locales/en.popackages/dashboard/src/i18n/locales/es.popackages/dashboard/src/i18n/locales/fa.popackages/dashboard/src/i18n/locales/fr.popackages/dashboard/src/i18n/locales/he.popackages/dashboard/src/i18n/locales/hr.popackages/dashboard/src/i18n/locales/hu.popackages/dashboard/src/i18n/locales/it.popackages/dashboard/src/i18n/locales/ja.popackages/dashboard/src/i18n/locales/nb.popackages/dashboard/src/i18n/locales/ne.popackages/dashboard/src/i18n/locales/nl.popackages/dashboard/src/i18n/locales/pl.popackages/dashboard/src/i18n/locales/pt_BR.popackages/dashboard/src/i18n/locales/pt_PT.popackages/dashboard/src/i18n/locales/ro.popackages/dashboard/src/i18n/locales/ru.popackages/dashboard/src/i18n/locales/sv.popackages/dashboard/src/i18n/locales/tr.popackages/dashboard/src/i18n/locales/uk.popackages/dashboard/src/i18n/locales/zh_Hans.popackages/dashboard/src/i18n/locales/zh_Hant.popackages/dashboard/src/lib/components/shared/asset/asset-preview.tsxpackages/dashboard/src/lib/components/shared/configurable-operation-utils.spec.tspackages/dashboard/src/lib/framework/form-engine/utils.spec.ts
💤 Files with no reviewable changes (10)
- packages/dashboard/src/lib/framework/form-engine/utils.spec.ts
- packages/dashboard/src/lib/components/shared/configurable-operation-utils.spec.ts
- packages/dashboard/src/i18n/locales/sv.po
- packages/dashboard/src/i18n/locales/ru.po
- packages/dashboard/src/lib/components/shared/asset/asset-preview.tsx
- packages/dashboard/src/i18n/locales/zh_Hant.po
- packages/dashboard/src/i18n/locales/uk.po
- packages/dashboard/src/i18n/locales/ro.po
- packages/dashboard/src/i18n/locales/tr.po
- packages/dashboard/src/i18n/locales/zh_Hans.po
✅ Files skipped from review due to trivial changes (1)
- packages/dashboard/e2e/tests/catalog/custom-fields.spec.ts
Fills the msgstr for the five new focal-point/preview strings (Asset,
Edit focal point, Focal point updated, Failed to update focal point,
Preview of {0}) across all managed locales, per CodeRabbit review.
Relates to vendurehq#4722
Observe the source stream in E2eAssetStorageStrategy.writeFileFromStream so a stream error rejects the promise instead of hanging global setup behind a Playwright timeout. Per CodeRabbit review. Relates to vendurehq#4722
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/dashboard/src/i18n/locales/hu.po (1)
1297-1299:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the asset label aligned with the rest of the Hungarian catalog.
Line 1299 translates
AssetasEszköz, but this locale already usesMédiafor the asset/media entity elsewhere. Leaving this as-is will make the preview dialog title inconsistent with the surrounding asset UI.📝 Suggested fix
#: src/lib/components/shared/asset/asset-preview-dialog.tsx:34 msgid "Asset" -msgstr "Eszköz" +msgstr "Média"🤖 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 `@packages/dashboard/src/i18n/locales/hu.po` around lines 1297 - 1299, The Hungarian translation for the "Asset" message in the locale file is inconsistent with the rest of the catalog; update the msgstr for msgid "Asset" (the entry tied to src/lib/components/shared/asset/asset-preview-dialog.tsx:34) from "Eszköz" to "Média" so the preview dialog title matches the existing asset/media terminology used elsewhere.
🤖 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 `@packages/dashboard/src/i18n/locales/nl.po`:
- Around line 1301-1303: Replace the incorrect Dutch translation for msgid
"Asset" (currently msgstr "Middel") with the consistent term used elsewhere in
this locale ("Bestand") so the singular label in asset-preview-dialog (msgid
"Asset") matches other strings like "Nieuw bestand", "Bestand bewerken" and
"Bestand succesvol bijgewerkt".
In `@packages/dashboard/src/i18n/locales/sv.po`:
- Around line 3071-3073: Update the Swedish msgstr for msgid "Failed to update
focal point" to follow the project's consistent error pattern "Misslyckades med
att uppdatera X" and use the indefinite noun form; replace the current
translation with e.g. "Misslyckades med att uppdatera en fokuspunkt" in
packages/dashboard/src/i18n/locales/sv.po so it matches other "Failed to update
X" entries.
---
Duplicate comments:
In `@packages/dashboard/src/i18n/locales/hu.po`:
- Around line 1297-1299: The Hungarian translation for the "Asset" message in
the locale file is inconsistent with the rest of the catalog; update the msgstr
for msgid "Asset" (the entry tied to
src/lib/components/shared/asset/asset-preview-dialog.tsx:34) from "Eszköz" to
"Média" so the preview dialog title matches the existing asset/media terminology
used elsewhere.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed176174-0006-4f98-832f-6a8e04155652
📒 Files selected for processing (25)
packages/dashboard/src/i18n/locales/ar.popackages/dashboard/src/i18n/locales/bg.popackages/dashboard/src/i18n/locales/cs.popackages/dashboard/src/i18n/locales/de.popackages/dashboard/src/i18n/locales/es.popackages/dashboard/src/i18n/locales/fa.popackages/dashboard/src/i18n/locales/fr.popackages/dashboard/src/i18n/locales/he.popackages/dashboard/src/i18n/locales/hr.popackages/dashboard/src/i18n/locales/hu.popackages/dashboard/src/i18n/locales/it.popackages/dashboard/src/i18n/locales/ja.popackages/dashboard/src/i18n/locales/nb.popackages/dashboard/src/i18n/locales/ne.popackages/dashboard/src/i18n/locales/nl.popackages/dashboard/src/i18n/locales/pl.popackages/dashboard/src/i18n/locales/pt_BR.popackages/dashboard/src/i18n/locales/pt_PT.popackages/dashboard/src/i18n/locales/ro.popackages/dashboard/src/i18n/locales/ru.popackages/dashboard/src/i18n/locales/sv.popackages/dashboard/src/i18n/locales/tr.popackages/dashboard/src/i18n/locales/uk.popackages/dashboard/src/i18n/locales/zh_Hans.popackages/dashboard/src/i18n/locales/zh_Hant.po
✅ Files skipped from review due to trivial changes (5)
- packages/dashboard/src/i18n/locales/nb.po
- packages/dashboard/src/i18n/locales/cs.po
- packages/dashboard/src/i18n/locales/hr.po
- packages/dashboard/src/i18n/locales/de.po
- packages/dashboard/src/i18n/locales/bg.po
| #: src/lib/components/shared/asset/asset-preview-dialog.tsx:34 | ||
| msgid "Asset" | ||
| msgstr "Middel" |
There was a problem hiding this comment.
Keep singular “Asset” aligned with the existing Dutch terminology.
msgstr "Middel" conflicts with the rest of this locale, where singular asset copy is translated as Bestand (Nieuw bestand, Bestand bewerken, Bestand succesvol bijgewerkt). This dialog will otherwise switch terms inside the same flow.
Suggested fix
#: src/lib/components/shared/asset/asset-preview-dialog.tsx:34
msgid "Asset"
-msgstr "Middel"
+msgstr "Bestand"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #: src/lib/components/shared/asset/asset-preview-dialog.tsx:34 | |
| msgid "Asset" | |
| msgstr "Middel" | |
| #: src/lib/components/shared/asset/asset-preview-dialog.tsx:34 | |
| msgid "Asset" | |
| msgstr "Bestand" |
🤖 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 `@packages/dashboard/src/i18n/locales/nl.po` around lines 1301 - 1303, Replace
the incorrect Dutch translation for msgid "Asset" (currently msgstr "Middel")
with the consistent term used elsewhere in this locale ("Bestand") so the
singular label in asset-preview-dialog (msgid "Asset") matches other strings
like "Nieuw bestand", "Bestand bewerken" and "Bestand succesvol bijgewerkt".
| #: src/lib/components/shared/asset/asset-preview.tsx:141 | ||
| msgid "Failed to update focal point" | ||
| msgstr "Det gick inte att uppdatera fokuspunkten" |
There was a problem hiding this comment.
Inconsistent translation pattern for error message.
The translation "Det gick inte att uppdatera fokuspunkten" deviates from the established pattern used throughout the file for "Failed to update X" messages. All similar error messages use the pattern "Misslyckades (med) att uppdatera X" with the indefinite form of the noun (e.g., lines 315, 476, 945, 1906).
🔄 Suggested fix for consistency
#: src/lib/components/shared/asset/asset-preview.tsx:141
msgid "Failed to update focal point"
-msgstr "Det gick inte att uppdatera fokuspunkten"
+msgstr "Misslyckades att uppdatera fokuspunkt"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #: src/lib/components/shared/asset/asset-preview.tsx:141 | |
| msgid "Failed to update focal point" | |
| msgstr "Det gick inte att uppdatera fokuspunkten" | |
| #: src/lib/components/shared/asset/asset-preview.tsx:141 | |
| msgid "Failed to update focal point" | |
| msgstr "Misslyckades att uppdatera fokuspunkt" |
🤖 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 `@packages/dashboard/src/i18n/locales/sv.po` around lines 3071 - 3073, Update
the Swedish msgstr for msgid "Failed to update focal point" to follow the
project's consistent error pattern "Misslyckades med att uppdatera X" and use
the indefinite noun form; replace the current translation with e.g.
"Misslyckades med att uppdatera en fokuspunkt" in
packages/dashboard/src/i18n/locales/sv.po so it matches other "Failed to update
X" entries.
Summary
The standalone Asset detail route at
/assets/<id>already lets the user set a focal point on an asset. The same affordance was missing from the sharedAssetPreviewdialog used by everyEntityAssetsgallery — so opening a product or variant's featured asset gave you a preview with no way to change its focal point. The reporter asked for the editor to be available from the variant assets edit panel; modifying the shared dialog covers both the product and variant code paths in a single change.Change
packages/dashboard/src/lib/components/shared/asset/asset-preview.tsxnow hosts the focal-point editor:FocusIcontoggle plus a "Focal Point" coords readout that shows the persisted value (or "Not set" when null).AssetFocalPointEditorand switches into edit mode on toggle. Confirming sends an inlineUpdateAssetFocalPointmutation (defined locally to avoid alib → app/routesimport — the equivalentassetUpdateDocumentalready lives in the assets route; hoisting it to a shared location is a worthwhile follow-up but out of scope here).focalPointOverrideso the indicator updates immediately AND invokes a newonAssetUpdatedcallback so the parentEntityAssetscan keep itsassets/featuredAsset/previewAssetstate in sync. Without that callback, re-opening the dialog on the same asset would silently regress to the stale value from the parent detail query.assetIdat click time and theonSuccesshandler guards against that id so a late response after the user navigated to a different asset can never write the focal point onto the wrong asset's UI.Focal Point/Set Focal Pointmsgids fromassets_.$id.tsxso locale.pocatalogs don't grow duplicates.AssetPreviewDialogpasses the callback through, andEntityAssets(the only current consumer) updates all three pieces of local state on the callback.packages/dashboard/src/lib/components/shared/asset/asset-focal-point-editor.tsxgets adata-testid="asset-focal-point-editor-confirm"on its "Set Focal Point" button so the e2e test can target it without colliding with the trigger'saria-label.packages/dashboard/src/lib/components/shared/entity-assets.tsxgets adata-testid="entity-assets-featured"on the featured-asset wrapper for the same reason.E2E setup change
The e2e backend was running without
AssetServerPlugin, which meant uploaded test assets resolved to atest-url/test-assets/...placeholder thatVendureImagecould not parse withnew URL(...). The product detail page crashed withFailed to construct 'URL': Invalid URLwhenever a real asset was attached.packages/dashboard/e2e/global-setup.tsnow configuresAssetServerPlugin, loaded viaawait import(...)because Playwright's Babel transform can't resolve the plugin's class-extends chain through@vendure/corestatically. Uploads go to__data__/assets/which is already inpackages/dashboard/.gitignore; the directory is wiped on each global setup so reruns start clean.Test plan
bunx tsc --noEmitonpackages/dashboard— net-1against baseline (theas anycast on theAssetServerPlugin.initreturn also fixes a pre-existingCustomHistoryEntryPlugintyping error)bunx prettier --check— cleancheck-lib-imports.js, format) passpackages/dashboard/e2e/tests/regression/issue-4722-asset-focal-point-in-preview-dialog.spec.tscreateAssets, attaches it to a fresh productdata-testid="entity-assets-featured")data-testid="asset-focal-point-editor-confirm")0.50, 0.50role="dialog"to be hidden, reopens, asserts the value still shows — the regression for the parent-sync fixFollow-ups (out of scope)
assetUpdateDocumentfromapp/routes/_authenticated/_assets/assets.graphql.tsintosrc/lib/graphql/(extending the selection set to includefocalPoint { x y }) and have both the assets route andasset-preview.tsximport it. Removes the duplicate mutation document and the duplicateUpdateAsset*operation name in generated types.data-testidon theAssetFocalPointEditor'sCancelbutton to mirror the new confirm-button testid.Fixes #4722
Need help on this PR? Tag
@codesmithwith what you need.