Rock: Project Elevate - Part 1

[shervElmi]

Linear

• Handoff: https://linear.app/strategy11/document/handoff-693db9ae1792
• Design: https://www.figma.com/design/DQbZMgeNmJWkxOWYfHjDZX/Formidable-System?node-id=1-20&t=KdWMt6UNRG8kIbcG-1

Screen recording demo

https://share.cleanshot.com/GBMXWNjn

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

js/frm_testing_mode.js (1)

2-2: Consider excluding minified files from code review.

This appears to be a minified/built JavaScript file rather than source code. Reviewing minified files is not practical because they're generated artifacts where parameter names and code structure are intentionally obfuscated for size optimization.

Recommendations:

• Review the source file at js/src/frm_testing_mode.js instead
• Consider marking build artifacts like *.min.js or files in distribution directories as generated using .gitattributes to exclude them from PR diffs
• Configure your linter to skip minified files

Note on static analysis warnings: The Biome warnings (fallthrough cases, variable hoisting, function reassignment, etc.) are false positives—these patterns are expected and intentional in minified code.

js/src/admin/addon-state.js (2)

108-108: Consider only showing the success modal for install/activate actions

showUpgradeModalSuccess() is currently invoked for every addon action, including deactivate/uninstall. Even though the helper no-ops when #frm_upgrade_modal is absent, it might still be safer/clearer to gate it on install/activation actions so we never show a “Great! Everything’s ready to go!” state for non-upgrade flows.

You could tighten this with something like:

-	showUpgradeModalSuccess();
+	if ( [ 'frm_install_addon', 'frm_activate_addon', 'frm_activate_addon_license' ].includes( action ) ) {
+		showUpgradeModalSuccess();
+	}

(adjust the allowed actions list to match the real set used in this flow).

---

213-249: showUpgradeModalSuccess is solid; a couple of robustness tweaks to consider

The new helper is a nice improvement: it avoids innerHTML, keeps the existing image, and cleanly encapsulates the success state (class toggle + copy + icon).

Two small follow-ups you might consider:

1. Clear all addon status messages, not just the first

   Earlier in afterAddonInstall you treat .frm-addon-status as potentially multiple elements (using querySelectorAll + forEach), but here you only clear the first one:

   const frmAddonStatus = document.querySelector( '.frm-addon-status' );
   if ( frmAddonStatus ) {
   	frmAddonStatus.textContent = '';
   }

   To keep behavior consistent if more than one exists:

• const frmAddonStatus = document.querySelector( '.frm-addon-status' );
• if ( frmAddonStatus ) {

•   frmAddonStatus.textContent = '';
  

• }

• document.querySelectorAll( '.frm-addon-status' ).forEach(

•   status => {
  

•   	status.textContent = '';
  

•   }
  

• );

2. **Fallback if the circled icon has no existing `<svg>`**

Right now the checkmark only appears if there is already an `<svg>` inside `.frm-circled-icon`:

```js
circledIcon.querySelector( 'svg' )?.replaceWith( svg( { href: '#frm_checkmark_icon' } ) );

If the markup ever changes and that <svg> is missing, no icon will be shown. A slightly more defensive pattern:

-	const circledIcon = upgradeModal.querySelector( '.frm-circled-icon' );
-	if ( circledIcon ) {
-		circledIcon.classList.add( 'frm-circled-icon-green' );
-		circledIcon.querySelector( 'svg' )?.replaceWith( svg( { href: '#frm_checkmark_icon' } ) );
-	}
+	const circledIcon = upgradeModal.querySelector( '.frm-circled-icon' );
+	if ( circledIcon ) {
+		circledIcon.classList.add( 'frm-circled-icon-green' );
+		const newIcon = svg( { href: '#frm_checkmark_icon' } );
+		const existingIcon = circledIcon.querySelector( 'svg' );
+		if ( existingIcon ) {
+			existingIcon.replaceWith( newIcon );
+		} else {
+			circledIcon.append( newIcon );
+		}
+	}

Neither of these is blocking, but they make the helper a bit more future-proof if the DOM structure evolves or multiple status elements are present.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b06060 and 3f8b08f.

📒 Files selected for processing (4)

• classes/helpers/FrmFieldsHelper.php (1 hunks)
• css/frm_testing_mode.css (1 hunks)
• js/frm_testing_mode.js (1 hunks)
• js/src/admin/addon-state.js (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• classes/helpers/FrmFieldsHelper.php

🧰 Additional context used

🧬 Code graph analysis (2)

js/src/admin/addon-state.js (2)

js/src/admin/upgrade-popup.js (2)

• frmDom (3-3)
• upgradeMessage (31-31)

js/src/frm_testing_mode.js (1)

• frmDom (34-34)

js/frm_testing_mode.js (3)

js/src/admin/addon-state.js (2)

• a (198-198)
• frmDom (3-3)

js/src/admin/upgrade-popup.js (1)

• frmDom (3-3)

js/src/frm_testing_mode.js (1)

• frmDom (34-34)

🪛 Biome (2.1.2)

css/frm_testing_mode.css

[error] 3-3: Unexpected shorthand property background after background-color

(lint/suspicious/noShorthandPropertyOverrides)

---

[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.

text-align is already defined here.

Remove or rename the duplicate property to ensure consistent styling.

(lint/suspicious/noDuplicateProperties)

---

[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.

min-width is already defined here.

Remove or rename the duplicate property to ensure consistent styling.

(lint/suspicious/noDuplicateProperties)

---

[error] 3-3: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.

border-radius is already defined here.

Remove or rename the duplicate property to ensure consistent styling.

(lint/suspicious/noDuplicateProperties)

js/frm_testing_mode.js

[error] 2-2: This case is falling through to the next case.

Add a break or return statement to the end of this case to prevent fallthrough.

(lint/suspicious/noFallthroughSwitchClause)

---

[error] 2-2: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

---

[error] 2-2: Do not use the e variable name as a label

The variable is declared here

Creating a label with the same name as an in-scope variable leads to confusion.

(lint/suspicious/noLabelVar)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: Cypress

🔇 Additional comments (3)

css/frm_testing_mode.css (1)

2-3: I'll verify the review comment by examining the actual CSS file to confirm the duplicated Bootstrap tooltip CSS block, the stray closing comment marker, and the property override issues.
<function_calls>


#!/bin/bash

First, let's read the CSS file to understand its structure

wc -l css/frm_testing_mode.css


</function_calls>
<function_calls>


#!/bin/bash

Read the first 100 characters to understand the structure

head -c 500 css/frm_testing_mode.css


</function_calls>
<function_calls>


#!/bin/bash

Let's examine the file more carefully to find patterns

Check for the presence of closing comment markers

grep -o '*/' css/frm_testing_mode.css | wc -l


</function_calls>
<function_calls>


#!/bin/bash

Let's search for the tooltip CSS pattern to see if it's duplicated

First, count occurrences of ".tooltip{"

grep -o '.tooltip{' css/frm_testing_mode.css | wc -l


</function_calls>
<function_calls>


#!/bin/bash

Let's extract a meaningful chunk around the tooltip definitions

Use a Python script to parse and analyze the minified CSS

python3 << 'EOF'
import re

with open('css/frm_testing_mode.css', 'r') as f:
content = f.read()

Find all positions of ".tooltip{" in the file

tooltip_positions = []
for match in re.finditer(r'.tooltip{', content):
tooltip_positions.append(match.start())

print(f"Total occurrences of '.tooltip{{': {len(tooltip_positions)}")
print(f"Positions: {tooltip_positions}")

Look for the stray closing comment marker

comment_positions = []
for match in re.finditer(r'*/', content):
comment_positions.append(match.start())

print(f"\nTotal occurrences of '*/': {len(comment_positions)}")

Check if there's a */ before the first .tooltip{

if tooltip_positions and comment_positions:
first_tooltip = tooltip_positions[0]
relevant_comments = [p for p in comment_positions if p < first_tooltip + 500]
if relevant_comments:
print(f"Comment markers before/near first tooltip: {relevant_comments}")
# Show context around first comment near tooltip
for cm_pos in relevant_comments:
start = max(0, cm_pos - 50)
end = min(len(content), cm_pos + 50)
print(f"\nContext around position {cm_pos}:")
print(content[start:end])

EOF


</function_calls>

[Crabcyborg]

@lauramekaj1 It looks like an e2e test in deleteForms is failing here.

Can you look into this?

Thank you!

[Crabcyborg]

Thanks @shervElmi!

Once we figure out the failing e2e test, I think we can merge this.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

js/src/admin/upgrade-popup.js (1)

48-51: Guard against undefined data-learn-more attribute.

As flagged in the previous review, if link.dataset.learnMore is undefined, assigning it directly to href can create malformed URLs. This affects existing upgrade triggers that don't define a learn-more URL.

Apply this diff to guard against undefined values:

 const learnMoreLink = container.querySelector( '.frm-learn-more' );
 if ( learnMoreLink ) {
-	learnMoreLink.href = link.dataset.learnMore;
+	const learnMore = link.dataset.learnMore;
+	if ( learnMore ) {
+		learnMoreLink.href = learnMore;
+		learnMoreLink.style.display = 'inline-block';
+	} else {
+		learnMoreLink.style.display = 'none';
+	}
 }

🧹 Nitpick comments (2)

js/frm_testing_mode.js (2)

2-2: Guard .frm-learn-more href when data-learn-more is missing

In addOneClick, the learn‑more link’s href is always overwritten with e.dataset.learnMore (even when the attribute is missing/empty), which can result in href="undefined" or an empty/relative URL.

At the source level (e.g., js/src/admin/upgrade-popup.js), consider tightening this:

- const learnMoreLink = modal.querySelector('.frm-learn-more');
- if ( learnMoreLink ) {
-   learnMoreLink.href = trigger.dataset.learnMore;
- }
+ const learnMoreLink = modal.querySelector('.frm-learn-more');
+ if ( learnMoreLink && trigger.dataset.learnMore ) {
+   learnMoreLink.href = trigger.dataset.learnMore;
+ }

This preserves any default URL and avoids invalid links when data-learn-more is not provided.

---

2-2: Biome errors here are from transpiled/runtime code, not actionable bugs

The Biome findings (noFallthroughSwitchClause, noInvalidUseBeforeDeclaration, noLabelVar, multiple noFunctionAssign) all point at patterns in the bundled generator/runtime helpers on this line (e.g., the switch(e.n){...} state machine and reassigning o to a wrapped async function). These are typical of transpiled/minified bundles and are functionally correct.

Rather than modifying this generated file, it would be better to configure Biome to lint the source files (e.g., js/src/admin/upgrade-popup.js, js/src/admin/testing-mode.js) and/or exclude compiled bundles like js/frm_testing_mode.js from linting.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 55788d4 and 6878b01.

📒 Files selected for processing (2)

• js/frm_testing_mode.js (1 hunks)
• js/src/admin/upgrade-popup.js (6 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

js/src/admin/upgrade-popup.js (3)

js/src/admin/addon-state.js (1)

• frmDom (3-3)

js/src/admin/admin.js (3)

• frmDom (241-241)
• frmDom (242-242)
• frmDom (243-243)

js/src/frm_testing_mode.js (1)

• frmDom (34-34)

js/frm_testing_mode.js (1)

js/src/admin/upgrade-popup.js (1)

• frmDom (3-3)

🪛 Biome (2.1.2)

js/frm_testing_mode.js

[error] 2-2: This case is falling through to the next case.

Add a break or return statement to the end of this case to prevent fallthrough.

(lint/suspicious/noFallthroughSwitchClause)

---

[error] 2-2: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

---

[error] 2-2: Do not use the e variable name as a label

The variable is declared here

Creating a label with the same name as an in-scope variable leads to confusion.

(lint/suspicious/noLabelVar)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

---

[error] 2-2: Do not reassign a function declaration.

Reassigned here.

Use a local variable instead.

(lint/suspicious/noFunctionAssign)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: Cypress
• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: PHP 8 tests in WP trunk

🔇 Additional comments (5)

js/src/admin/upgrade-popup.js (4)

3-3: LGTM: SVG helper import.

The import follows the established pattern seen in other files (admin.js, addon-state.js) and is used appropriately throughout the file for SVG manipulation.

---

42-46: Good use of optional chaining for safety.

The optional chaining operator (?.) on line 45 properly guards against null when the SVG element doesn't exist, preventing potential runtime errors.

---

108-111: LGTM: Parent container display logic.

The code properly guards against null before setting the display style, and the flex layout adjustment aligns with the updated upgrade modal design.

---

121-121: LGTM: Modal width adjustment.

The 2px width increase appears intentional for design alignment.

js/frm_testing_mode.js (1)

2-2: Upgrade modal wiring and addon flows look consistent

Within this bundle, the updated upgrade‑modal logic (addOneClick, initUpgradeModal) and the addon flow (toggleAddonState, afterAddonInstall) appear internally consistent:

• Success state updates the modal message, icon, and addon status labels coherently.
• The new upsell image and learn‑more link integrate without breaking existing one‑click activation behavior.
• Test‑mode dropdown initialization and the async import of the upgrade‑modal module are unchanged in behavior.

No correctness or robustness issues beyond the learn‑more guard noted above.

[lauramekaj1]

@lauramekaj1 It looks like an e2e test in deleteForms is failing here.

Can you look into this?

Thank you!

@Crabcyborg I checked and it wasn’t related to functionality, so I updated the failing test and improved the selectors to make the flow stable.
Here is the PR with the update: #2617

Thank you!

[lauramekaj1]

@shervElmi After activating the Formidable Forms Pro plugin, I’m not able to access the add-ons from the form settings (Form Landing Pages, Conversational Forms, Form Abandonment). Please check the screen recording for reference: https://www.loom.com/share/f59bb84a5627451f865805d553964078

[shervElmi]

@shervElmi After activating the Formidable Forms Pro plugin, I’m not able to access the add-ons from the form settings (Form Landing Pages, Conversational Forms, Form Abandonment)...

Thanks so much Laura for catching this issue. 🚀 🙌🏻

It’s fixed now. Could you test it again?

[codecov]

Codecov Report

❌ Patch coverage is 85.79882% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.33%. Comparing base (e0a38d6) to head (1c11054).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
classes/helpers/FrmFieldsHelper.php	0.00%	12 Missing ⚠️
classes/helpers/FrmAppHelper.php	0.00%	6 Missing ⚠️
classes/controllers/FrmFormsController.php	0.00%	4 Missing ⚠️
classes/controllers/FrmSettingsController.php	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2576      +/-   ##
============================================
+ Coverage     26.17%   26.33%   +0.15%     
- Complexity     8831     8837       +6     
============================================
  Files           144      144              
  Lines         29798    29902     +104     
============================================
+ Hits           7799     7874      +75     
- Misses        21999    22028      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lauramekaj1]

Thanks so much Laura for catching this issue. 🚀 🙌🏻

It’s fixed now. Could you test it again?

@shervElmi I tested it and verified that the issue is fixed. Thank you!

[Crabcyborg]

Thank you @lauramekaj1!

🚀