Color variations#357
Conversation
|
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 (15)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (10)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)messages/**/*.json📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a complete gear colorways (color variations) system enabling per-gear visual variants. It introduces a ChangesGear Colorways
Sequence Diagram(s)sequenceDiagram
participant Page as Gear Page
participant Carousel as GearImageCarousel
participant Dock as Dock + Modal
participant ImageModal as GearImageModal
participant Action as colorways/actions.ts
participant Service as colorways/service.ts
participant Data as colorways/data.ts
participant DB as Database
Page->>Carousel: colorways={item.colorways}
Page->>Dock: colorways={item.colorways ?? []}
Carousel->>Carousel: compute slides from colorway<br/>image URLs (or fallback)
Carousel->>Carousel: render color pills<br/>(disabled if no images)
Dock->>Action: actionCreateGearColorway<br/>/ actionReorderGearColorways / etc.
ImageModal->>Action: actionSetGearColorwayImage<br/>(imageType, imageUrl)
Action->>Service: validate role +<br/>normalize input
Service->>Data: transactional DB<br/>operation
Data->>DB: INSERT/UPDATE<br/>gear_colorways +<br/>audit_log
Data->>DB: mirror default<br/>colorway URLs<br/>onto gear
Action->>Action: revalidatePath<br/>(gear/browse/admin)
Action-->>Dock: updated colorways
Action-->>ImageModal: updated colorway
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)docs/gear-images/color-variations.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
src/components/gear/colorway-swatch.tsx (1)
6-15: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire an accessible name when the swatch is interactive.
labelis optional even when rendering a button. This allows unnamed controls for assistive tech. A discriminated prop type can enforcelabelwheninteractiveistrue.Also applies to: 51-58
🤖 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 `@src/components/gear/colorway-swatch.tsx` around lines 6 - 15, The ColorwaySwatchProps type definition allows the label prop to be optional even when interactive is true, which creates an accessibility issue for screen readers when rendering as a button. Refactor ColorwaySwatchProps to use a discriminated union type that creates two variants: one where interactive is false and label is optional, and another where interactive is true and label is required. Apply this type constraint throughout the component, including in the component rendering logic where the button is conditionally rendered based on the interactive prop, to ensure that a label is always provided when the swatch functions as an interactive button control.
🤖 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 `@messages/de.json`:
- Around line 244-301: The colorways.manager section in the German locale file
contains numerous English UI strings that need to be translated to German to
ensure locale consistency. Translate all the English strings in the
colorways.manager object (including title, descriptionExplicit,
descriptionImplicit, implicitTitle, implicitDescription, enableTitle, addTitle,
enable, add, name, namePlaceholder, nameRequired, editSwatch, swatchTitle,
colorA, colorB, hex, hue, saturationLightness, presets, black, gray, white,
lightPreview, darkPreview, apply, cancel, drag, delete, deleteTitle,
deleteDescription, confirmDelete, created, enabled, updated, reordered, deleted,
saveFailed, resetTitle, confirmResetTitle, defaultHint, and enableHint) to their
German equivalents so German users see consistent language throughout the UI.
In `@messages/es.json`:
- Around line 244-301: The colorways.manager section in messages/es.json
contains numerous English string values that need to be translated to Spanish.
Translate all the English text values in the colorways.manager object (including
strings like "Manage Colors", "Add, edit, and reorder the stored color
variations for this gear item", "Color name", "Hex color", "Hue", "Saturation
and lightness", "Consistent presets", and all other English labels and messages)
to their proper Spanish equivalents while maintaining the same key structure and
placeholders like {name}.
In `@messages/fr.json`:
- Around line 244-301: The colorways.manager section in the French locale file
contains English text values that need to be translated to French for consistent
language presentation to French users. Translate all the string values in the
manager object (keeping the property keys in English as they are identifiers) to
proper French equivalents. Focus on translating entries like title,
descriptionExplicit, descriptionImplicit, and all other value strings throughout
the manager object to provide a complete French localization experience without
mixing English and French in the same interface.
In `@messages/it.json`:
- Around line 244-301: The colorways.manager object in messages/it.json contains
numerous English string values that need to be translated to Italian for
consistent UI localization. Review all the string values within the
colorways.manager nested object (including keys like title, descriptionExplicit,
descriptionImplicit, implicitTitle, implicitDescription, enableTitle, addTitle,
enable, add, name, namePlaceholder, nameRequired, editSwatch, swatchTitle,
colorA, colorB, hex, hue, saturationLightness, presets, black, gray, white,
lightPreview, darkPreview, apply, cancel, drag, delete, deleteTitle,
deleteDescription, confirmDelete, created, enabled, updated, reordered, deleted,
saveFailed, resetTitle, resetImageChoice, keepGearImages, applyAndReset,
applyOtherColor, applyOtherColorDescription, chooseColorway, resetConfirmTitle,
keepGearImagesDescription, applyColorwayDescription, confirmReset,
confirmResetAction, resetComplete, defaultHint, and enableHint) and translate
each English string value to its appropriate Italian equivalent, ensuring
consistency with the existing Italian translations already present in the file.
In `@messages/ja.json`:
- Around line 244-301: The colorways.manager subtree in the Japanese
localization file has incomplete translations with many English strings
remaining untranslated. Review all the key-value pairs within the
colorways.manager object and translate the remaining English values to Japanese,
ensuring consistency with the Japanese translations already present in the
object such as resetDescription, resetImageChoice, and keepGearImages. This
includes translating strings like title, descriptionExplicit, implicitTitle,
namePlaceholder, swatchTitle, and all other English text values to provide a
complete Japanese user experience for the color management interface.
In `@messages/ms.json`:
- Around line 246-300: The colorways.manager section in messages/ms.json
contains numerous English strings that need to be translated to Malay for
consistent localization. For each English string value in the colorways.manager
object (including keys like "title", "descriptionExplicit",
"descriptionImplicit", "enable", "add", "nameRequired", "deleteTitle",
"deleteDescription", "confirmDelete", "resetTitle", "resetConfirmTitle",
"confirmReset", "confirmResetAction", "defaultHint", and "enableHint"), replace
the English text with the appropriate Malay translation to ensure the Malay
locale displays correctly throughout the UI.
In `@messages/zh.json`:
- Around line 246-300: The zh.json file contains English values that should be
translated to Chinese for the colorways.manager locale block. Identify all
English strings in this section (including keys like confirmReset,
resetComplete, defaultHint, and enableHint that show "Reset colors", "Color
variations reset", and similar English phrases) and replace them with
appropriate Chinese translations to maintain consistency with the rest of the
Chinese localization file.
In `@src/components/gear/gear-tools-dock/dock-buttons.tsx`:
- Around line 165-170: Replace the hardcoded aria-label="Manage Images" string
in the button element with a translation key variable (similar to the pattern
used with colorwaysManageLabel). Create a new translation key for image
management functionality and thread it into the component props or state where
other localized strings are accessed, then use that key variable in the
aria-label attribute instead of the hardcoded string to ensure the label is
properly localized across all supported locales.
In `@src/components/ui/color-picker.tsx`:
- Around line 42-49: The DEFAULT_LABELS object in color-picker.tsx contains
hardcoded English user-facing strings that bypass localization. Replace all
string values in the DEFAULT_LABELS constant (trigger, hex, alpha,
saturationLightness, hue, opacity) with corresponding translation keys from your
i18n system instead of hardcoded English text. This ensures the color picker
labels can be properly localized for different languages as per the project's
localization guidelines.
In `@src/components/ui/example-usage.tsx`:
- Around line 97-110: The Color Picker section in the example-usage.tsx file
contains hardcoded user-facing strings that need to be localized. Replace the
hardcoded strings "Color Picker", "Accent Color", and "Overlay Color" with
translation keys from the project's i18n system. Update the h2 element
containing the section title and the p elements containing the field labels to
use translation key references instead of literal strings, ensuring consistency
across all supported locales.
In `@src/server/admin/colorways/actions.ts`:
- Around line 19-21: The current revalidatePath calls only invalidate the
default locale. To fix this, import locales from the i18n config file and update
the three revalidatePath calls for `/gear/${result.gearSlug}`, `/browse`, and
`/admin/gear` by wrapping them in a loop that iterates through all supported
locales. Each revalidatePath call should be modified to include the locale
prefix in the path so that cache is properly invalidated across all locales, not
just the default one.
In `@src/server/admin/colorways/data.ts`:
- Around line 40-49: The mirrorDefaultColorway function currently
unconditionally sets ogImageUrl to null when preserveOg is not explicitly true,
which silently clears existing OG images when called from reorder,
delete-default, and reset/apply paths that don't pass options. Modify the
conditional logic in the set statement to only include ogImageUrl in the update
when options.ogImageUrl is explicitly provided (i.e., during front-image
uploads). Otherwise, omit ogImageUrl from the update entirely to preserve the
existing value. This ensures that paths calling mirrorDefaultColorway without
options don't inadvertently clear the OG image.
In `@tests/playwright/colorways.spec.ts`:
- Around line 14-16: The E2E test has hardcoded English role names like
"Available colors" in page.getByRole calls and navigates to a locale-agnostic
URL path that will fail when tests run in non-English environments. Update the
page.goto call to include an explicit English locale prefix by changing the path
from `/gear/${colorwayGearSlug}` to `/en/gear/${colorwayGearSlug}` to ensure all
subsequent role-name assertions remain stable and deterministic regardless of
the system locale.
- Around line 30-31: Between pressing the Escape key in the first action and
clicking the "Manage Images" button, add an explicit wait to ensure the "Manage
Colors" dialog is hidden before proceeding. Use a waitFor assertion on the
dialog element to confirm it is no longer visible before allowing the next click
action to execute, which will eliminate timing flakes caused by animation delays
in the dialog dismissal.
---
Nitpick comments:
In `@src/components/gear/colorway-swatch.tsx`:
- Around line 6-15: The ColorwaySwatchProps type definition allows the label
prop to be optional even when interactive is true, which creates an
accessibility issue for screen readers when rendering as a button. Refactor
ColorwaySwatchProps to use a discriminated union type that creates two variants:
one where interactive is false and label is optional, and another where
interactive is true and label is required. Apply this type constraint throughout
the component, including in the component rendering logic where the button is
conditionally rendered based on the interactive prop, to ensure that a label is
always provided when the swatch functions as an interactive button control.
🪄 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: fc8bb907-bfb9-4fcd-b381-956c7013e3e2
📒 Files selected for processing (40)
docs/gear-images/color-variations.mddocs/gear-images/gear-image-plan.mddocs/gear-specification-system.mddocs/server-structure.mddrizzle/0022_puzzling_husk.sqldrizzle/meta/0022_snapshot.jsondrizzle/meta/_journal.jsonmessages/de.jsonmessages/en.jsonmessages/es.jsonmessages/fr.jsonmessages/it.jsonmessages/ja.jsonmessages/ms.jsonmessages/zh.jsonsrc/app/[locale]/(pages)/gear/[slug]/page.tsxsrc/app/[locale]/(pages)/gear/_components/gear-image-carousel.tsxsrc/components/gear/colorway-swatch-editor.tsxsrc/components/gear/colorway-swatch.tsxsrc/components/gear/gear-tools-dock/dock-buttons.tsxsrc/components/gear/gear-tools-dock/gear-item-dock.client.tsxsrc/components/gear/gear-tools-dock/gear-item-dock.tsxsrc/components/gear/manage-colorways-modal.tsxsrc/components/modals/gear-image-modal.tsxsrc/components/ui/color-picker.tsxsrc/components/ui/example-usage.tsxsrc/server/admin/colorways/actions.tssrc/server/admin/colorways/data.tssrc/server/admin/colorways/service.tssrc/server/db/schema.tssrc/server/gear/data.tssrc/types/gear.tstests/playwright/colorways.spec.tstests/unit/color-picker.test.tstests/unit/colorway-swatch.test.tstests/unit/gear-colorways-data-wiring.test.tstests/unit/gear-colorways-service.test.tstests/unit/gear-image-carousel.test.tstests/unit/gear-image-modal-wiring.test.tstests/unit/manage-colorways-modal-wiring.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Include or update automated tests (vitest and/or playwright) covering the primary path and at least one meaningful edge case when introducing or modifying behavior
Files:
tests/unit/manage-colorways-modal-wiring.test.tstests/unit/gear-colorways-data-wiring.test.tstests/playwright/colorways.spec.tstests/unit/colorway-swatch.test.tstests/unit/gear-colorways-service.test.tstests/unit/gear-image-carousel.test.tstests/unit/gear-image-modal-wiring.test.tstests/unit/color-picker.test.ts
**/{app,src}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Replace any new hardcoded user-facing strings with translation keys for all locales
Files:
src/types/gear.tssrc/components/gear/colorway-swatch.tsxsrc/components/ui/example-usage.tsxsrc/components/gear/gear-tools-dock/gear-item-dock.client.tsxsrc/components/gear/gear-tools-dock/gear-item-dock.tsxsrc/server/admin/colorways/actions.tssrc/app/[locale]/(pages)/gear/[slug]/page.tsxsrc/components/gear/colorway-swatch-editor.tsxsrc/server/gear/data.tssrc/server/db/schema.tssrc/server/admin/colorways/service.tssrc/app/[locale]/(pages)/gear/_components/gear-image-carousel.tsxsrc/components/ui/color-picker.tsxsrc/server/admin/colorways/data.tssrc/components/modals/gear-image-modal.tsxsrc/components/gear/gear-tools-dock/dock-buttons.tsxsrc/components/gear/manage-colorways-modal.tsx
messages/**/*.json
📄 CodeRabbit inference engine (AGENTS.md)
Maintain translation key parity: if a key is added, removed, or renamed in
messages/en.json, apply the same key change across all locale files inmessages/directory
Files:
messages/es.jsonmessages/en.jsonmessages/de.jsonmessages/fr.jsonmessages/ms.jsonmessages/zh.jsonmessages/it.jsonmessages/ja.json
src/server/db/schema.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/server/db/schema.ts: Make all schema changes insrc/server/db/schema.ts; never modify the database directly without going through the schema
All schema changes must be backwards compatible by default, using deprecations instead of deletions
Use Drizzle's type-safe schema definitions for database schema insrc/server/db/schema.ts
Files:
src/server/db/schema.ts
🪛 ast-grep (0.44.0)
tests/unit/manage-colorways-modal-wiring.test.ts
[warning] 4-7: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(
path.join(process.cwd(), "src/components/gear/manage-colorways-modal.tsx"),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
tests/unit/gear-colorways-data-wiring.test.ts
[warning] 6-6: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(projectRoot, file), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
tests/unit/colorway-swatch.test.ts
[warning] 28-34: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(
path.join(
process.cwd(),
"src/components/gear/colorway-swatch-editor.tsx",
),
"utf8",
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (27)
messages/en.json (1)
234-301: LGTM!docs/gear-images/color-variations.md (1)
1-29: LGTM!docs/gear-images/gear-image-plan.md (1)
1-3: LGTM!docs/gear-specification-system.md (1)
9-12: LGTM!docs/server-structure.md (1)
17-17: LGTM!Also applies to: 30-30, 40-40
tests/unit/color-picker.test.ts (1)
1-255: LGTM!tests/unit/colorway-swatch.test.ts (1)
1-44: LGTM!tests/unit/gear-colorways-data-wiring.test.ts (1)
1-34: LGTM!tests/unit/gear-colorways-service.test.ts (1)
1-107: LGTM!tests/unit/gear-image-carousel.test.ts (1)
58-83: LGTM!Also applies to: 153-208
tests/unit/gear-image-modal-wiring.test.ts (1)
3-3: LGTM!Also applies to: 17-19, 35-69
tests/unit/manage-colorways-modal-wiring.test.ts (1)
1-37: LGTM!src/server/admin/colorways/service.ts (1)
1-174: LGTM!src/components/gear/colorway-swatch-editor.tsx (1)
57-207: LGTM!src/components/gear/manage-colorways-modal.tsx (1)
85-682: LGTM!drizzle/0022_puzzling_husk.sql (2)
1-26: LGTM!
27-27: 🩺 Stability & AvailabilityRemove the
IF NOT EXISTSsuggestion—this index is not being recreated.The index
gear_publication_state_idxappears only in migration 0022 and does not exist in any earlier migration (0000–0021). The index is being created for the first time, so there is no risk of a duplicate relation error.drizzle/meta/_journal.json (1)
158-164: LGTM!src/server/db/schema.ts (1)
1-1: LGTM!Also applies to: 17-17, 75-82, 648-673, 1230-1234, 1408-1415, 1816-1824, 2263-2266
src/types/gear.ts (1)
13-13: LGTM!Also applies to: 46-46, 65-65
src/server/gear/data.ts (1)
32-32: LGTM!Also applies to: 57-57, 121-128, 288-298, 309-309, 425-427, 671-671, 1016-1022, 1041-1043, 1398-1401, 1438-1438
src/components/modals/gear-image-modal.tsx (1)
3-6: LGTM!Also applies to: 27-28, 37-37, 48-48, 77-82, 98-145, 206-231, 245-262, 334-350, 434-434, 519-555
src/app/[locale]/(pages)/gear/_components/gear-image-carousel.tsx (1)
5-18: LGTM!Also applies to: 30-41, 51-166, 168-205, 206-221, 223-255
src/app/[locale]/(pages)/gear/[slug]/page.tsx (1)
25-51: LGTM!Also applies to: 105-110, 141-143, 272-272, 339-339, 379-379, 517-519
src/components/gear/gear-tools-dock/gear-item-dock.tsx (1)
7-16: LGTM!Also applies to: 35-35, 47-50, 65-65
src/components/gear/gear-tools-dock/gear-item-dock.client.tsx (1)
4-8: LGTM!Also applies to: 17-28, 41-41, 94-97, 126-126, 135-135, 186-191, 208-208
src/components/gear/gear-tools-dock/dock-buttons.tsx (1)
7-7: LGTM!Also applies to: 21-21, 40-40, 60-65, 89-94, 157-164, 180-207, 219-219, 256-256, 287-287, 318-318, 457-458
1 participant
No description provided.