Skip to content

Conversation

repl6669
Copy link
Contributor

@repl6669 repl6669 commented Feb 14, 2025

Description

This PR aims to generalize how attribute data fields are handled in Filament and fix #2091 and #2071.

  1. Component state for all fields is set to field value by using the ->getValue() method.
  2. Instance of the field with its value set is returned after dehydration

Todo

  • Test that fields are rendered correctly on ProductEditPage
  • Test that the form is saved correctly for all Lunar fields as well as custom, user defined fields
    • Test custom RepeaterField
    • Test custom BuilderField

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability and normalization of attribute form fields (including repeater and builder) so values render and save consistently.
  • Documentation

    • Clarified public API surface for storefront session management in facade docs (types and methods).
  • Tests

    • Added comprehensive tests covering rendering and persistence of attribute fields.
    • Added test stubs and synthesizers for Builder and Repeater field types.
  • Chores

    • Updated .gitignore to exclude a local IDE configuration file.

Copy link

vercel bot commented Feb 14, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
lunar-docs Error Error Sep 12, 2025 2:07pm

@alecritson
Copy link
Collaborator

@repl6669 Is this still being worked on?

@repl6669
Copy link
Contributor Author

repl6669 commented May 2, 2025

Hello @alecritson, I will give it some more time this weekend. I just need to find out how to properly test it.

@repl6669 repl6669 force-pushed the feat/generalize-attribute-data-handling-in-filament branch from de8f71e to 739d28c Compare May 13, 2025 21:07
@repl6669 repl6669 reopened this Aug 18, 2025
Copy link
Contributor

coderabbitai bot commented Aug 18, 2025

Walkthrough

Normalizes AttributeData state handling and generalizes dehydration (removing FileFieldType special-casing), updates StorefrontSession PHPDoc annotations, adds builder/repeater field stubs and synthesizer, expands product edit tests, and adds /.phpactor.json to .gitignore.

Changes

Cohort / File(s) Summary
Attribute state handling
packages/admin/src/Support/Forms/AttributeData.php
Normalize component state: unwrap FieldType values via getValue(), filter/reindex arrays, and coerce raw state into expected FieldType during dehydration; removes special-case check for FileFieldType.
Storefront session PHPDoc
packages/core/src/Facades/StorefrontSession.php
Revise facade PHPDoc: explicit typed accessors and init/reset methods for Channel/Currency/Customer/Groups; documentation-only changes.
Feature tests (Product edit)
tests/admin/Feature/.../EditProductTest.php
Add synthesizeLivewireProperties pre-mount calls; add tests for rendering and saving builder and repeater attribute fields and broader attribute coverage.
Test stubs: FieldTypes (JSON-backed)
tests/admin/Stubs/FieldTypes/BuilderField.php, tests/admin/Stubs/FieldTypes/RepeaterField.php
Add BuilderField and RepeaterField test stubs implementing FieldType + JsonSerializable with array validation/normalization.
Test stubs: Filament FieldTypes + Synthesizer
tests/admin/Stubs/Support/FieldTypes/BuilderField.php, tests/admin/Stubs/Support/FieldTypes/RepeaterField.php, tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php
Add Filament components for builder/repeater with format/mutate callbacks and a BuilderSynth for normalized get/set/unset operations.
Repo config
.gitignore
Ignore /.phpactor.json.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Handle custom attribute file field type saving by not relying on strict FileFieldType comparison in AttributeData (#2091)

Out-of-scope changes

Code Change Explanation
.gitignore updated to ignore /.phpactor.json (.gitignore) Repository configuration change unrelated to fixing AttributeData/file-field handling.
PHPDoc-only updates to StorefrontSession facade (packages/core/src/Facades/StorefrontSession.php) Documentation changes to facade annotations do not relate to the file-field saving issue.

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@repl6669 repl6669 marked this pull request as ready for review August 18, 2025 22:25
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (13)
.gitignore (1)

6-6: Root-only ignore for Phpactor config — confirm scope; consider YAML variant

Using a leading slash limits ignoring .phpactor.json to the repo root. If that’s intended, this is fine. If you want to ignore it anywhere in the tree (e.g., in nested packages/fixtures), drop the leading slash. Also, consider whether .phpactor.yml is used in your workflows and should be ignored as well for consistency.

Optional change if you want non-root matching:

-/.phpactor.json
+.phpactor.json
packages/core/src/Facades/StorefrontSession.php (2)

19-24: Add @mixin annotation to StorefrontSession facade

All init*, forget and getSessionKey methods have been confirmed on StorefrontSessionManager. Adding a @mixin will help IDEs infer the full API without needing to enumerate every method.

• File: packages/core/src/Facades/StorefrontSession.php
– In the class-level docblock, add:

 /**
+ * @mixin \Lunar\Managers\StorefrontSessionManager
  * @method static \Lunar\Models\Contracts\Channel getChannel()
  * @method static \Lunar\Managers\StorefrontSessionManager setChannel(\Lunar\Models\Contracts\Channel $channel)
  * @method static \Illuminate\Support\Collection getCustomerGroups()
  * …
  * @method static void initCustomer()
  * @method static void forget()
  * @method static string getSessionKey()

11-14: Document concrete Collection value-type for customer groups

We’ve verified that StorefrontSessionManager::__construct() and resetCustomerGroups() always initialize $this->customerGroups to a new Collection, so getCustomerGroups() never returns null. It’s safe to tighten the facade PHPDoc with generics for better IDE/static-analysis support:

File: packages/core/src/Facades/StorefrontSession.php

- * @method static \Illuminate\Support\Collection getCustomerGroups()
+ * @method static \Illuminate\Support\Collection<int, \Lunar\Models\Contracts\CustomerGroup> getCustomerGroups()
- * @method static \Lunar\Managers\StorefrontSessionManager setCustomerGroups(\Illuminate\Support\Collection $customerGroups)
+ * @method static \Lunar\Managers\StorefrontSessionManager setCustomerGroups(\Illuminate\Support\Collection<int, \Lunar\Models\Contracts\CustomerGroup> $customerGroups)
packages/admin/src/Support/Forms/AttributeData.php (2)

66-68: Use the imported contract consistently

Minor consistency: use the FieldTypeContract alias here as well.

-                if ($state instanceof \Lunar\Base\FieldType) {
+                if ($state instanceof FieldTypeContract) {
                     return $state->getValue();
                 }

16-16: Nit: fix Dropdown alias typo for clarity

The alias is spelled DrodownFieldType which might be confusing when scanning the file.

-use Lunar\FieldTypes\Dropdown as DrodownFieldType;
+use Lunar\FieldTypes\Dropdown as DropdownFieldType;
@@
-        DrodownFieldType::class => Dropdown::class,
+        DropdownFieldType::class => Dropdown::class,

Also applies to: 31-31

tests/admin/Stubs/FieldTypes/RepeaterField.php (1)

18-35: Optional: be lenient with null/blank inputs in setValue

For test ergonomics, consider treating null/blank as [] instead of throwing, aligning with some core field types’ behavior.

-    public function setValue($value)
+    public function setValue($value)
     {
-        if (! is_array($value)) {
+        if ($value === null) {
+            $value = [];
+        }
+        if (! is_array($value)) {
             throw new FieldTypeException(self::class.' value must be an array.');
         }
 
         $this->value = json_encode($value);
     }
tests/admin/Stubs/Support/FieldTypes/BuilderField.php (1)

24-73: LGTM: UI-layer normalization complements dehydration

The format/dehydrate callbacks mirror the domain stub’s normalize logic, keeping the component state predictable. With the proposed change in AttributeData, we avoid double-filtering of scalar arrays while still reindexing builder items.

If you adopt the AttributeData normalization tweak, please rerun the product edit tests that exercise builder/repeater to confirm no regressions. I can help expand coverage to include File multiple and ListField cases.

tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (2)

26-38: Avoid double-normalization; let FieldType::setValue() own normalization

You're pre-normalizing in the nested set() path, but the CoreBuilderField::setValue() already normalizes comprehensively (including stripping empty/null data, enforcing shape). Relying on the FieldType normalization avoids duplicate work and keeps all rules in one place.

Suggested change:

     public function set(&$target, $key, $value)
     {
         if ($key === '' || $key === null) {
-            $target->setValue(is_array($value) ? $value : []);
+            // Let the FieldType perform full normalization.
+            $target->setValue(is_array($value) ? $value : []);
 
             return;
         }
 
         $fieldValue = (array) $target->getValue();
         Arr::set($fieldValue, $key, $value);
 
-        $target->setValue($this->normalize($fieldValue));
+        // Delegate normalization to the FieldType implementation.
+        $target->setValue($fieldValue);
     }

40-47: Mirror set() simplification in unset()

Same rationale as set(): pre-normalizing here is redundant given CoreBuilderField::setValue() normalizes fully, including re-indexing.

Suggested change:

     public function unset(&$target, $index)
     {
         $fieldValue = (array) $target->getValue();
 
         Arr::forget($fieldValue, $index);
 
-        $target->setValue($this->normalize($fieldValue));
+        $target->setValue($fieldValue);
     }
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (4)

78-206: Pre-synthesize here too for consistency and to reduce flakiness

This test registers custom field types (Repeater/Builder). While it only asserts rendering, pre-synthesizing keeps behavior consistent across tests and avoids subtle issues with nested state resolution.

Suggested insertion:

     \Lunar\Admin\Support\Facades\AttributeData::registerFieldType(
         \Lunar\Tests\Admin\Stubs\FieldTypes\RepeaterField::class,
         \Lunar\Tests\Admin\Stubs\Support\FieldTypes\RepeaterField::class,
     );
+    // Preload property synthesizers for dynamic field types
+    \Lunar\Admin\Support\Facades\AttributeData::synthesizeLivewireProperties();

Additionally, since this page renders variant pricing contexts, you already added a default Currency in this test — good. Consider applying the same to the other new tests for consistency (see suggestions below).


264-368: Add a default Currency to mirror the other test’s environment

The EditProduct page often expects a default Currency for variant-related UI. You created it in the “render all fields” test but not here. Add it to avoid environment-dependent failures.

Suggested insertion after the TaxClass factory:

     \Lunar\Models\Language::factory()->create(['default' => true]);
     \Lunar\Models\TaxClass::factory()->create(['default' => true]);
+    \Lunar\Models\Currency::factory()->create(['default' => true]);

Optional: there’s a fair bit of setup duplication across tests (language/tax/currency, groups, attributables). If this grows, consider extracting test helpers/factories to keep tests compact.


369-486: Pre-synthesize and add default Currency for parity

  • Registering the Repeater field type is correct, but, like above, pre-synthesizing reduces surprises in nested array handling.
  • Add a default Currency to keep the environment aligned with your other test and the page’s expectations.

Suggested changes:

     \Lunar\Admin\Support\Facades\AttributeData::registerFieldType(
         \Lunar\Tests\Admin\Stubs\FieldTypes\RepeaterField::class,
         \Lunar\Tests\Admin\Stubs\Support\FieldTypes\RepeaterField::class,
     );
+    \Lunar\Admin\Support\Facades\AttributeData::synthesizeLivewireProperties();
     \Lunar\Models\Language::factory()->create([
         'default' => true,
     ]);
 
     \Lunar\Models\TaxClass::factory()->create([
         'default' => true,
     ]);
+    \Lunar\Models\Currency::factory()->create([
+        'default' => true,
+    ]);

Note: Great end-to-end assertion of saved state for both product and variant repeaters.


1-486: Add a regression test for custom File field type (covers linked issue #2091)

Given the PR’s objective to let custom file attribute types save correctly (removing strict class checks), please add a test that exercises a custom File subtype end-to-end. This will prevent regressions and explicitly validates the fix for #2091.

I can draft it if helpful. Sketch:

  • Define a stub CustomFileField extends \Lunar\FieldTypes\File (no changes).
  • Register it with AttributeData to the same Filament file component used by core File.
  • Create a product + attribute of type CustomFileField.
  • Pre-synthesize, mount EditProduct, fill with new CustomFileField('foo/bar.jpg'), save.
  • Assert $product->attr('custom_file') === 'foo/bar.jpg'.

Would you like me to open a follow-up PR with the test and stubs?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ecb4b46 and 5bba165.

📒 Files selected for processing (9)
  • .gitignore (1 hunks)
  • packages/admin/src/Support/Forms/AttributeData.php (3 hunks)
  • packages/core/src/Facades/StorefrontSession.php (1 hunks)
  • tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (3 hunks)
  • tests/admin/Stubs/FieldTypes/BuilderField.php (1 hunks)
  • tests/admin/Stubs/FieldTypes/RepeaterField.php (1 hunks)
  • tests/admin/Stubs/Support/FieldTypes/BuilderField.php (1 hunks)
  • tests/admin/Stubs/Support/FieldTypes/RepeaterField.php (1 hunks)
  • tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/admin/Stubs/FieldTypes/RepeaterField.php (2)
packages/core/src/Exceptions/FieldTypeException.php (1)
  • FieldTypeException (5-8)
tests/admin/Stubs/Support/FieldTypes/RepeaterField.php (1)
  • RepeaterField (10-22)
tests/admin/Stubs/Support/FieldTypes/RepeaterField.php (2)
tests/admin/Stubs/FieldTypes/RepeaterField.php (1)
  • RepeaterField (9-43)
packages/core/src/Models/Attribute.php (1)
  • Attribute (35-94)
tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (3)
packages/admin/src/Support/Synthesizers/AbstractFieldSynth.php (1)
  • AbstractFieldSynth (8-41)
tests/admin/Stubs/FieldTypes/BuilderField.php (4)
  • BuilderField (9-69)
  • getValue (23-26)
  • normalize (37-61)
  • setValue (28-35)
tests/admin/Stubs/Support/FieldTypes/BuilderField.php (1)
  • BuilderField (10-75)
tests/admin/Stubs/Support/FieldTypes/BuilderField.php (2)
tests/admin/Stubs/FieldTypes/BuilderField.php (1)
  • BuilderField (9-69)
tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (1)
  • BuilderSynth (9-55)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (5)
packages/admin/src/Support/Forms/AttributeData.php (3)
  • AttributeData (28-117)
  • synthesizeLivewireProperties (111-116)
  • registerFieldType (92-97)
tests/admin/Stubs/FieldTypes/RepeaterField.php (1)
  • RepeaterField (9-43)
packages/core/src/Models/AttributeGroup.php (1)
  • AttributeGroup (23-59)
packages/core/src/Models/Attribute.php (1)
  • Attribute (35-94)
tests/admin/TestCase.php (1)
  • asStaff (88-91)
tests/admin/Stubs/FieldTypes/BuilderField.php (3)
packages/core/src/Exceptions/FieldTypeException.php (1)
  • FieldTypeException (5-8)
tests/admin/Stubs/Support/FieldTypes/BuilderField.php (1)
  • BuilderField (10-75)
tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (1)
  • normalize (49-54)
packages/admin/src/Support/Forms/AttributeData.php (7)
packages/admin/src/Support/FieldTypes/TextField.php (1)
  • TextField (11-38)
packages/admin/src/Support/FieldTypes/File.php (1)
  • getFilamentComponent (14-43)
packages/admin/src/Support/FieldTypes/Vimeo.php (1)
  • getFilamentComponent (14-23)
packages/core/src/Base/Traits/HasTranslations.php (1)
  • translate (17-40)
packages/core/src/FieldTypes/Toggle.php (2)
  • getValue (51-54)
  • setValue (61-68)
packages/core/src/FieldTypes/File.php (2)
  • getValue (59-62)
  • setValue (69-76)
packages/core/src/FieldTypes/Vimeo.php (2)
  • getValue (51-54)
  • setValue (61-68)
🔇 Additional comments (9)
packages/core/src/Facades/StorefrontSession.php (2)

9-10: Facade PHPDoc Signature Confirmed
Both StorefrontSessionInterface::setChannel() and StorefrontSessionManager::setChannel() accept only a ChannelContract instance (no string overload), so the facade’s PHPDoc is already correct. No |string union is needed.


15-18: Accessors signatures align with docs

All currency/customer methods match the documented return types and support fluent chaining:

  • Interface (StorefrontSessionInterface):
    • getCurrency(): Currency
    • getCustomer(): ?Customer
    • setCurrency(Currency): static
    • setCustomer(Customer): static

  • Implementation (StorefrontSessionManager):
    • getCurrency(): CurrencyContract – always initialized in the constructor via initCurrency(), so never null at runtime.
    • getCustomer(): ?CustomerContract – initialized in initCustomer(), returns null only when no session value is set.
    • setCurrency()/setCustomer() both return static, enabling fluent chaining.

  • Facade (StorefrontSession): docblock annotations reflect the above exactly.

No changes required.

packages/admin/src/Support/Forms/AttributeData.php (1)

44-51: Simplified component resolution and label application looks good

The one-liner panel field-type resolution and chaining label() read clean and align with Filament patterns.

tests/admin/Stubs/Support/FieldTypes/RepeaterField.php (1)

14-21: LGTM: focused Filament stub for Repeater

Clear schema, sensible defaults, and collapsed by default. This pairs well with the generalized state handling.

tests/admin/Stubs/FieldTypes/BuilderField.php (1)

28-35: LGTM: solid normalization for builder payloads

The normalization enforces the expected shape and strips null/empty values. Matches the UI stub behavior and should make assertions deterministic.

Also applies to: 37-61

tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (3)

9-14: Solid Synthesizer target/key setup

$key and $targetClass are correctly specialized for the builder stub. This ensures Livewire routes property ops to this synth only for your CoreBuilderField.


15-24: Balanced get() behavior with root/nested support

Returning a normalized full array at root and nested values via Arr::get() is appropriate for a Repeater-like structure. This aligns with Livewire's expected get semantics.


49-54: normalize() scope is intentionally minimal — OK

A shallow filter + reindex is fine if you keep normalization logic centralized in the FieldType. After applying the above refactors, this method will effectively be used only by get() to present stable state, which is reasonable.

tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (1)

54-56: Good call: synthesize Livewire properties prior to mounting

Pre-loading synthesizers reduces flakiness when interacting with FieldType instances in form state.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (4)

78-85: Reset field-type registrations to avoid cross-test leakage.

registerFieldType mutates global state. Add a teardown to restore the original map (or expose a reset API on AttributeData) so later tests don’t inherit test-only mappings.

If a reset API doesn’t exist, I can add one (e.g., AttributeData::resetToCoreFieldTypes()) and wire it to the facade for tests. Want me to open a follow-up?


126-142: Builder type is used but not registered in this test.

Definitions include a Builder field, but only Repeater is registered. If you intend to verify the actual Builder UI renders (not the Text fallback), register it here too.

     \Lunar\Admin\Support\Facades\AttributeData::registerFieldType(
         \Lunar\Tests\Admin\Stubs\FieldTypes\RepeaterField::class,
         \Lunar\Tests\Admin\Stubs\Support\FieldTypes\RepeaterField::class,
     );
+    \Lunar\Admin\Support\Facades\AttributeData::registerFieldType(
+        \Lunar\Tests\Admin\Stubs\FieldTypes\BuilderField::class,
+        \Lunar\Tests\Admin\Stubs\Support\FieldTypes\BuilderField::class,
+    );

144-167: DRY the attribute creation for product and variant.

Both loops are identical aside from attributable_type/group. Extract a small helper to reduce duplication and speed up future edits.

+    $createAttributes = function (string $type, int $groupId) use ($definitions, $product) {
+        foreach ($definitions as $idx => $def) {
+            $attribute = \Lunar\Models\Attribute::factory()->create([
+                'attribute_type' => $type,
+                'attribute_group_id' => $groupId,
+                'position' => $idx + 1,
+                'name' => ['en' => ucfirst($def['handle'])],
+                'handle' => $def['handle'],
+                'section' => 'main',
+                'type' => $def['type'],
+                'required' => false,
+                'system' => false,
+                'searchable' => false,
+                'configuration' => $def['configuration'],
+            ]);
+            \Illuminate\Support\Facades\DB::table('lunar_attributables')->insert([
+                'attribute_id' => $attribute->id,
+                'attributable_type' => 'product_type',
+                'attributable_id' => $product->productType->id,
+            ]);
+        }
+    };
-    // Create attributes for the product and map them via product type
-    foreach ($definitions as $idx => $def) { ... }
+    $createAttributes('product', $productGroup->id);
-    // Create attributes for the variant and map them via product type
-    foreach ($definitions as $idx => $def) { ... }
+    $createAttributes('product_variant', $variantGroup->id);

Also applies to: 169-192


257-259: Use the imported Livewire alias consistently.

Elsewhere you use the imported Livewire class; here it’s fully qualified.

-    \Livewire\Livewire::test(\Lunar\Admin\Filament\Resources\ProductResource\Pages\EditProduct::class, [
+    Livewire::test(\Lunar\Admin\Filament\Resources\ProductResource\Pages\EditProduct::class, [
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5bba165 and f7630a1.

📒 Files selected for processing (4)
  • packages/admin/src/Support/Forms/AttributeData.php (4 hunks)
  • tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (3 hunks)
  • tests/admin/Stubs/FieldTypes/RepeaterField.php (1 hunks)
  • tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/admin/Stubs/FieldTypes/RepeaterField.php
  • tests/admin/Stubs/Support/Synthesizers/BuilderSynth.php
  • packages/admin/src/Support/Forms/AttributeData.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (3)
packages/admin/src/Support/Forms/AttributeData.php (3)
  • AttributeData (28-117)
  • synthesizeLivewireProperties (111-116)
  • registerFieldType (92-97)
tests/admin/Stubs/FieldTypes/RepeaterField.php (1)
  • RepeaterField (9-46)
tests/admin/Stubs/Support/FieldTypes/RepeaterField.php (1)
  • RepeaterField (10-22)
🔇 Additional comments (5)
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php (5)

54-56: Good pre-mount synthesizer call.

Calling AttributeData::synthesizeLivewireProperties() before mounting prevents Livewire/Filament state desyncs for custom field types. Looks correct.


201-210: Assertions look good.

Presence checks for both product (attribute_data.) and variant (variant.) fields cover the matrix well.


269-373: Builder test is comprehensive and aligned with the new dehydration rules.

Registers the type, synthesizes, asserts presence, saves both product and variant, and verifies persistence. Solid coverage.


375-381: Confirm whether a default Currency is required to render the EditProduct form.

Some tests create a default Currency to ensure variant pricing sections render. If that’s a hidden dependency, add it here too for consistency.

Apply if needed:

+    \Lunar\Models\Currency::factory()->create(['default' => true]);

126-134: ListField mapping is already correct in AttributeData

The alias ListFieldFieldType::class on line 32 resolves to the fully qualified class Lunar\FieldTypes\ListField, so your mapping

ListFieldFieldType::class => ListField::class,

already uses the proper key. No changes are needed here.

Likely an incorrect or invalid review comment.

Comment on lines +92 to +100
\Lunar\Models\Currency::factory()->create([
'default' => true,
]);

// Ensure pricing-related defaults exist for variant rendering consistency
\Lunar\Models\Currency::factory()->create([
'default' => true,
]);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Duplicate default currency creation can cause inconsistent “default” resolution.

You create a default Currency twice in this test. This can lead to ambiguous defaults and brittle behavior depending on query ordering.

Apply this diff to remove the duplicate:

-    // Ensure pricing-related defaults exist for variant rendering consistency
-    \Lunar\Models\Currency::factory()->create([
-        'default' => true,
-    ]);
📝 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.

Suggested change
\Lunar\Models\Currency::factory()->create([
'default' => true,
]);
// Ensure pricing-related defaults exist for variant rendering consistency
\Lunar\Models\Currency::factory()->create([
'default' => true,
]);
\Lunar\Models\Currency::factory()->create([
'default' => true,
]);
🤖 Prompt for AI Agents
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php
around lines 92 to 100: the test creates a default Currency twice which can make
"default" resolution ambiguous; remove the duplicate factory call so only one
Currency is created with 'default' => true (keep the comment about ensuring
pricing defaults if needed) to avoid ambiguous defaults and brittle test
behavior.

Comment on lines +139 to +141
['handle' => 'file', 'type' => \Lunar\FieldTypes\File::class, 'configuration' => []],
['handle' => 'repeater', 'type' => \Lunar\Tests\Admin\Stubs\FieldTypes\RepeaterField::class, 'configuration' => []],
['handle' => 'builder', 'type' => \Lunar\Tests\Admin\Stubs\FieldTypes\BuilderField::class, 'configuration' => []],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add a save-path test for a custom File-based field type (covers #2091).

This PR aims to generalize dehydration for custom file types. Please add a focused test that saves a custom file attribute type (not strictly equal to FileFieldType::class) and asserts the value is persisted.

I can provide a minimal stub + test if you want.

🤖 Prompt for AI Agents
In
tests/admin/Feature/Filament/Resources/ProductResource/Pages/EditProductTest.php
around lines 139–141, add a focused test that verifies saving a custom
File-based field type persists correctly: create a minimal stub class that
mimics a File field (extend or implement the same base/contract used by
File::class), register that stub in the fields array (replace or append the
existing 'file' entry), write a test that posts the form payload with the custom
field value, triggers the resource save, then reloads the product and asserts
the attribute/database column contains the expected saved value; ensure the stub
is autoloadable or placed in the tests stubs namespace and the test cleans up
any temporary filesystem state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Attribute File Field Type Fails to Save Due to Strict Type Comparison in AttributeData
4 participants