Fix section fields incorrectly merging into the previous row after reload - #2694
Conversation
…r fields to check section_size before opening section
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSimplifies field wrapper state handling and corrects closure behavior for nested sections. Updates the section-overflow test to verify that an oversized section starts a new row. ChangesField wrapper logic
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change is a localized fix to keep sections on the correct row after reload, with no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Hi @lauramekaj1, I am adding you as a reviewer for a deep review, and I would appreciate it if you could test complex groups with sections, without sections, and with complex layout columns. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/helpers/FrmFieldGridHelper.php (1)
207-208: Correct initialization of tracking flags.The flags properly capture the state of the first field when a new wrapper begins.
One minor observation:
is_group_startandis_section_startare not reset inclose_field_wrapper(), unlikecurrent_list_sizeandcurrent_field_count. This works becausebegin_field_wrapper()always sets them before use, but explicit resets would improve symmetry with other state variables.🔎 Optional: Reset flags in close_field_wrapper for consistency
private function close_field_wrapper() { $this->maybe_close_section_helper(); echo '</ul></li>'; $this->parent_li = false; $this->current_list_size = 0; $this->current_field_count = 0; + $this->is_group_start = false; + $this->is_section_start = false; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmFieldGridHelper.php
⏰ 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). (1)
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/helpers/FrmFieldGridHelper.php (2)
63-79: LGTM!The new tracking flags are well-documented with clear examples explaining the intended behavior. The PHPDoc comments provide good context for future maintainers.
175-197: Logic flow is correct for the bug fix.The refactored method properly handles the section wrapping issue:
- Short-circuits when there is no parent list item (no wrapper to close).
- Adds special handling for divider fields when a section isn't already open.
- Lets section_helper take precedence when present.
The condition on line 184 correctly ensures dividers start on a new row when fields aren't explicitly grouped (
!is_group_start), which addresses the reported issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
classes/helpers/FrmFieldGridHelper.php (3)
176-184: Core logic looks correct for the stated objective.The early exit and divider handling properly enforce that sections start new rows unless explicitly grouped. The condition at line 183 ensures dividers only remain in the same row when
is_group_startis true (first field hadfrm_first), which matches the PR's goal of fixing unintended merging after reload.Consider adding an inline comment above line 182 explaining the three conditions for better maintainability:
// Close the current row before a section if: // 1. The section would overflow the grid, OR // 2. The section explicitly starts a new row (frm_first), OR // 3. The current row wasn't intentionally started as a group if ( 'divider' === $this->field->type && ! $this->section_is_open ) {
194-194: Consider adding a comment to clarify the section-start condition.The extended condition
( $this->is_section_start && ! $this->is_group_start )ensures fields after a section start a new row unless explicitly grouped. While the logic is sound, an inline comment would improve maintainability.🔎 Suggested clarification comment
+ // Also close if the row started with a section but wasn't marked as an intentional group return ! $this->can_support_current_layout() || $this->is_frm_first || ( $this->is_section_start && ! $this->is_group_start );
205-206: Flag initialization looks correct.The flags properly capture the state of the first field in each row. The logic works because flags are overwritten when
begin_field_wrapper()is called for a new row.For defensive coding, consider resetting these flags in
close_field_wrapper()(around line 293) to make the lifecycle explicit and prevent potential future issues if the logic changes:private function close_field_wrapper() { $this->maybe_close_section_helper(); echo '</ul></li>'; $this->parent_li = false; $this->current_list_size = 0; $this->current_field_count = 0; $this->is_group_start = false; $this->is_section_start = false; }This makes the state management more explicit, though the current implementation is functionally correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmFieldGridHelper.php
⏰ 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 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (1)
classes/helpers/FrmFieldGridHelper.php (1)
63-79: LGTM! Clear documentation with helpful examples.The new flags are well-documented with clear intent. The example for
is_group_starteffectively illustrates the grouping behavior.
|
@shervElmi I tested it and it's working as expected. Thank you! |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
 |
|
Overall Grade |
Security  Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Aug 25, 2026 12:22p.m. | Review ↗ |
| JavaScript |  |
Aug 25, 2026 12:22p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I added some unit tests and fix a bug.
I think this is good to merge now.
🚀
3 participants
Fixes https://github.com/Strategy11/formidable-pro/issues/3820
This update fixes an issue in the form builder where adding a section after a partially-sized field could cause the section to appear in the same row after refreshing the page. Now, sections correctly start on a new row when needed, keeping the builder layout consistent before and after reload.
Summary by CodeRabbit