Allowed flexible visibility settings for email editor

[9larsons]

no ref

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f27c5076-c17b-4e02-a0c6-cf366a9ea228

📥 Commits

Reviewing files that changed from the base of the PR and between 4063239 and e3e300c.

📒 Files selected for processing (1)

• packages/koenig-lexical/test/unit/EmailEditor.test.js

✅ Files skipped from review due to trivial changes (1)

• packages/koenig-lexical/test/unit/EmailEditor.test.js

---

Walkthrough

getEmailEditorCardConfig now validates cardConfig.visibilitySettings against an allowlist containing EMAIL_ONLY and NONE: if the input contains one of those values it is preserved; otherwise the function falls back to the default EMAIL_EDITOR_CARD_CONFIG.visibilitySettings. Previously visibilitySettings was always overwritten with the default. A new unit test file was added covering defaulting, pass-through of allowed values, rejection/normalization of unsupported or invalid values, normalization of image.allowedWidths, and preservation of unrelated properties.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The description is minimal ('no ref') and does not meaningfully describe the changeset or its purpose.	Provide a meaningful description explaining the purpose of adding flexible visibility settings validation, why this change was needed, and any relevant context.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: adding flexible validation for visibility settings in the email editor component, which aligns with the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch viz-email-editor

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

packages/koenig-lexical/test/unit/EmailEditor.test.js (1)

1-3: Consider consistent imports for Vitest globals.

You import expect explicitly from vitest but use describe and it as implicit globals. For consistency, either import all three or rely on Vitest's globals for all.

♻️ Option A: Import all Vitest globals explicitly

-import {expect} from 'vitest';
+import {describe, it, expect} from 'vitest';
 import {getEmailEditorCardConfig, EMAIL_EDITOR_CARD_CONFIG} from '../../src/components/EmailEditor';
 import {VISIBILITY_SETTINGS} from '../../src/utils/visibility';

♻️ Option B: Rely on Vitest globals entirely

-import {expect} from 'vitest';
 import {getEmailEditorCardConfig, EMAIL_EDITOR_CARD_CONFIG} from '../../src/components/EmailEditor';
 import {VISIBILITY_SETTINGS} from '../../src/utils/visibility';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/koenig-lexical/test/unit/EmailEditor.test.js` around lines 1 - 3,
The test mixes explicit and implicit Vitest globals—update the imports in
EmailEditor.test.js to be consistent by importing all used globals (e.g., change
the top line to "import {expect, describe, it} from 'vitest';") so that expect,
describe, and it are all imported explicitly; alternatively, remove the explicit
expect import and rely on Vitest globals for all three—update the import
statement around expect/describe/it and ensure the test uses the chosen approach
consistently.

packages/koenig-lexical/src/components/EmailEditor.jsx (1)

35-43: Silent coercion of invalid visibility settings may surprise callers.

When WEB_AND_EMAIL or WEB_ONLY is passed, it's silently coerced to EMAIL_ONLY without any warning. While the tests document this behavior, callers who mistakenly pass these values won't receive any feedback that their setting was ignored.

Consider adding a development-mode warning (e.g., console.warn) when a non-allowed value is provided, to help developers catch configuration mistakes early.

💡 Optional: Add a dev warning for ignored values

 const ALLOWED_EMAIL_EDITOR_VISIBILITY = [
     VISIBILITY_SETTINGS.EMAIL_ONLY,
     VISIBILITY_SETTINGS.NONE
 ];

 export function getEmailEditorCardConfig(cardConfig = {}) {
+    if (cardConfig.visibilitySettings && !ALLOWED_EMAIL_EDITOR_VISIBILITY.includes(cardConfig.visibilitySettings)) {
+        console.warn(`[EmailEditor] visibilitySettings "${cardConfig.visibilitySettings}" is not allowed. Falling back to "${EMAIL_EDITOR_CARD_CONFIG.visibilitySettings}".`);
+    }
     const visibilitySettings = ALLOWED_EMAIL_EDITOR_VISIBILITY.includes(cardConfig.visibilitySettings)
         ? cardConfig.visibilitySettings
         : EMAIL_EDITOR_CARD_CONFIG.visibilitySettings;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/koenig-lexical/src/components/EmailEditor.jsx` around lines 35 - 43,
getEmailEditorCardConfig silently coerces non-allowed visibility values (like
WEB_AND_EMAIL or WEB_ONLY) to the default; update getEmailEditorCardConfig to
emit a development-only warning when cardConfig.visibilitySettings is present
but not in ALLOWED_EMAIL_EDITOR_VISIBILITY by checking NODE_ENV (e.g.,
process.env.NODE_ENV !== 'production') and calling console.warn with a clear
message referencing the provided value and that
EMAIL_EDITOR_CARD_CONFIG.visibilitySettings will be used; reference
ALLOWED_EMAIL_EDITOR_VISIBILITY, getEmailEditorCardConfig,
EMAIL_EDITOR_CARD_CONFIG and VISIBILITY_SETTINGS to locate the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/koenig-lexical/src/components/EmailEditor.jsx`:
- Around line 35-43: getEmailEditorCardConfig silently coerces non-allowed
visibility values (like WEB_AND_EMAIL or WEB_ONLY) to the default; update
getEmailEditorCardConfig to emit a development-only warning when
cardConfig.visibilitySettings is present but not in
ALLOWED_EMAIL_EDITOR_VISIBILITY by checking NODE_ENV (e.g., process.env.NODE_ENV
!== 'production') and calling console.warn with a clear message referencing the
provided value and that EMAIL_EDITOR_CARD_CONFIG.visibilitySettings will be
used; reference ALLOWED_EMAIL_EDITOR_VISIBILITY, getEmailEditorCardConfig,
EMAIL_EDITOR_CARD_CONFIG and VISIBILITY_SETTINGS to locate the change.

In `@packages/koenig-lexical/test/unit/EmailEditor.test.js`:
- Around line 1-3: The test mixes explicit and implicit Vitest globals—update
the imports in EmailEditor.test.js to be consistent by importing all used
globals (e.g., change the top line to "import {expect, describe, it} from
'vitest';") so that expect, describe, and it are all imported explicitly;
alternatively, remove the explicit expect import and rely on Vitest globals for
all three—update the import statement around expect/describe/it and ensure the
test uses the chosen approach consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9f7b7f23-2bbf-418f-937a-1765c1efbde9

📥 Commits

Reviewing files that changed from the base of the PR and between b6bab06 and 41fe7ec.

📒 Files selected for processing (2)

• packages/koenig-lexical/src/components/EmailEditor.jsx
• packages/koenig-lexical/test/unit/EmailEditor.test.js