fix: incorrect tabber id counts with nested tabbers #215
Conversation
WalkthroughThe pull request introduces a subtle modification to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
includes/Tabber.php (1)
54-56: Consider tracking parent-child tabber relationships.The current implementation treats all tabbers independently. To ensure correct counting with nested tabbers, consider tracking parent-child relationships.
Example approach:
private static $tabberStack = []; public static function parserHook( ?string $input, array $args, Parser $parser, PPFrame $frame ) { // ... existing code ... $parentCount = end(self::$tabberStack) ?? null; self::$tabberStack[] = $count; $html = self::render( $input, $count, $args, $parser, $frame ); array_pop(self::$tabberStack); // ... rest of the code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Tabber.php(1 hunks)
🔇 Additional comments (2)
includes/Tabber.php (2)
Line range hint
147-148: Verify recursiveTagParse behavior with nested tabbers.The
getTabContentmethod usesrecursiveTagParsewhich can trigger nested tabber parsing. We should ensure that the count increment fix works correctly in this context.✅ Verification successful
Count increment fix is safe with nested tabbers ✅
The codebase deliberately uses
recursiveTagParse()with a safe recursion depth limit, as confirmed by issue #27. The count increment fix works correctly in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any special handling for nested parsing # or related issues in the codebase. # Look for recursiveTagParse usage and related comments/issues rg -B 5 -A 5 'recursiveTagParse' # Search for any nested parsing related issues rg -l 'nested.*tab|recursive.*parse' .github/Length of output: 5039
Script:
#!/bin/bash # Search for test cases and nested tabber handling rg -B 3 -A 3 "nested.*tabber" tests/ rg -B 3 -A 3 "test.*nested" tests/ # Look up issue #27 details gh issue view 27 || true # Search for any special handling or comments about nested tabbers ast-grep --pattern 'class Tabber { $$$ // $$$nested$$$ $$$ }' rg -B 3 -A 3 "nested" includes/Length of output: 1250
55-55: Verify if this fix addresses nested tabber counts correctly.While changing
++$countto$count + 1prevents modifying the count before storing, we should verify if this fully resolves the nested tabber count issue.Let's verify the fix with a test case containing nested tabbers:
Additionally, consider adding a test case that specifically validates nested tabber behavior:
// Test case suggestion $input = "<tabber> Tab1 = <tabber> NestedTab1 = Content1 NestedTab2 = Content2 </tabber> Tab2 = Content </tabber>";✅ Verification successful
Fix for nested tabber count is correct and properly implemented.
The implementation correctly handles nested tabber counts by:
- Incrementing the count atomically before content parsing
- Using extension data to maintain state
- Supporting nested tabbers through parser functions
The change from
++$countto$count + 1ensures each tabber (including nested ones) gets its own unique count, as the original count value remains unmodified during the parsing of nested content.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test cases or examples with nested tabbers # to verify if the count increment fix works correctly. # Look for nested tabber test cases rg -A 10 '<tabber.*<tabber' tests/Length of output: 83
Script:
#!/bin/bash # Find test and documentation files echo "=== Looking for test directories ===" fd -t d -i "test|spec|phpunit" echo -e "\n=== Looking for test files ===" fd -e test.php -e Test.php -e spec.php echo -e "\n=== Looking for documentation ===" fd -i "readme|documentation|wiki" -e md -e txt echo -e "\n=== Searching for tabber patterns in all files ===" rg -l "tabber"Length of output: 2195
Script:
#!/bin/bash # Check documentation and implementation echo "=== README.md content ===" cat README.md echo -e "\n=== Recent CHANGELOG entries ===" cat CHANGELOG.md echo -e "\n=== Tabber.php implementation ===" rg -B 5 -A 10 "recursiveTagParse|appendExtensionData" includes/Tabber.php echo -e "\n=== Searching for nested tabber mentions ===" rg -i "nest.*tabber"Length of output: 42073
Fix for the first issue in #207.
Tabber count is updated after
render, but duringrender,recursiveTagParseis called. With nested tabbers, this causes the first nested one to use the still unupdated count, which it then shares with the parent tabber.Summary by CodeRabbit