Reduce repeated option reads during dashboard navigation URL resolution

[codefora-dev]

All Submissions

• The code follows WordPress coding standards.
• The code satisfies the intended behavior and requirements.
• The code includes appropriate inline documentation.
• PHPCS tests were run locally.

---

Changes proposed in this Pull Request

This pull request reduces repeated work during Dokan dashboard navigation URL resolution by introducing request-scoped optimizations:

1. Adds request-scoped memoization to dokan_get_navigation_url() to avoid repeated option reads and permalink computation within a single request.
2. Avoids repeated dashboard navigation rebuilds inside VendorNavMenuChecker::maybe_rewrite_to_react_route() by using a request-scoped react_route => url lookup map populated during menu conversion.

These changes do not introduce persistent caching and do not alter URL structure or menu discovery behavior.

---

How to test the changes in this Pull Request

1. Visit any page that triggers multiple calls to dokan_get_navigation_url() during a single request (e.g. Dokan dashboard navigation rendering).
2. Verify that returned navigation URLs remain unchanged.
3. Optionally instrument function calls or option reads to confirm reduced repeated execution within the same request.

Summary by CodeRabbit

• Refactor
  • Optimized navigation URL resolution and lookup performance through request-scoped caching and memoization, reducing computational overhead on navigation-heavy operations and improving overall app responsiveness.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Two performance optimization changes are introduced: VendorNavMenuChecker now uses a request-scoped cache for react route mappings with lazy population, while dokan_get_navigation_url adds static memoization to avoid recomputing URLs on repeated calls within a request.

Changes

Cohort / File(s)	Summary
Request-Scoped React Route Mapping includes/VendorNavMenuChecker.php	Introduces react_route_url_map instance variable for fast per-request lookups. convert_to_react_menu now populates the map during menu processing. maybe_rewrite_to_react_route implements fast-path lookup with lazy map building via filter removal/re-attachment pattern. Shifts from on-demand post-processing to request-scoped lazy-populated caching.
URL Function Memoization includes/functions.php	Adds static memoization cache to dokan_get_navigation_url keyed by name and new_url. Normalizes inputs, defers URL construction, caches filtered results to eliminate recomputation on repeated calls.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Cache it, stash it, keep it near,
No need to build the map each year,
Fast lookups hop through memory's land,
Request-scoped magic, oh how grand!
Memoization makes the code run light. ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main optimization objective: reducing repeated option reads during navigation URL resolution, which aligns with both commits and code changes.
Description check	✅ Passed	The description covers all critical sections: clear explanation of changes, testing instructions, and relevant checklist items. Most optional sections like changelog and screenshots are appropriately omitted for a performance optimization PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

• 📝 Generate docstrings

---

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]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @includes/functions.php:
- Line 2030: The current cache key assignment that builds $key by concatenating
$name, a pipe and the flag can collide if $name contains a pipe; update the $key
construction (the line assigning $key) to use a collision-safe encoding—e.g.
replace the plain concatenation with a deterministic hash of the two components
(hash of $name, a null separator, and the boolean flag) or use a structured
serializer (serialize or json_encode of an array of $name and $new_url) so the
separator cannot collide, and keep the rest of the logic unchanged.

🧹 Nitpick comments (1)

includes/VendorNavMenuChecker.php (1)

85-106: Well-structured map population during menu conversion.

The refactored convert_to_react_menu() method cleanly builds the route-to-URL map during menu conversion. The logic properly validates inputs before populating the map, and recursively handles submenus.

One minor suggestion: consider adding a docblock comment explaining that this method now has the side effect of populating $this->react_route_url_map in addition to its primary purpose of converting menu items.

📝 Optional: Add docblock note about side effect

 /**
  * Convert menu items to react menu items
+ *
+ * @internal Also populates $this->react_route_url_map as a side effect for fast lookups.
  *
  * @since 4.0.0
  *

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4e2b84c and 4911c16.

📒 Files selected for processing (2)

• includes/VendorNavMenuChecker.php
• includes/functions.php

🧰 Additional context used

🧬 Code graph analysis (1)

includes/VendorNavMenuChecker.php (1)

includes/functions-dashboard-navigation.php (1)

• dokan_get_dashboard_nav (31-158)

🔇 Additional comments (4)

includes/VendorNavMenuChecker.php (2)

44-51: LGTM: Request-scoped URL map property.

The new $react_route_url_map property provides a clean way to cache route-to-URL mappings for the request lifetime, avoiding repeated navigation rebuilds.

---

121-133: Good defensive programming with early returns.

The input validation and early return conditions properly handle edge cases:

• Normalizing $name to a trimmed string
• Returning early for empty/special names
• Skipping rewrite for $new_url or URLs with hash fragments

These checks prevent unnecessary processing and potential errors.

includes/functions.php (2)

2027-2028: Handle non-scalar name values properly.

If $name is not scalar (e.g., an array or object), it won't be cast to string on line 2027, which will cause issues when constructing the cache key on line 2030 (concatenation with a non-string will produce a warning or unexpected results).

🐛 Proposed fix to handle all name values

-	$name    = is_scalar( $name ) ? (string) $name : '';
+	$name    = is_scalar( $name ) && ! is_bool( $name ) ? (string) $name : '';
 	$new_url = (bool) $new_url;

This ensures boolean values are also excluded from string casting (they would become '1' or empty string), and non-scalar values default to empty string, which is a safer fallback.

Likely an incorrect or invalid review comment.

---

2052-2052: Filter caching is acceptable.

The dokan_get_navigation_url filter is applied once per cache key and results are cached. The only extension using this filter (VendorNavMenuChecker) has a deterministic callback that checks against a static map populated once per request. The callback behavior does not depend on call count or request variables, making caching appropriate for this use case.

[codefora-dev]

This PR addresses the issue described in #3077.

Reproduction steps, before/after measurements, and screenshots are documented there.

The fix removes a hot path caused by repeated nav resolution within a single request while preserving Dokan’s runtime behavior (no cross-request caching).

[MdAsifHossainNadim]

Hi @codefora-dev, before moving this PR forward, we need to ensure full test coverage for the changes introduced here.

Please run all test cases in the following file locally, without skipping any tests, and confirm that they pass successfully:
dokan/tests/php/src/ReactUrlGenerationTest.php

Once the tests are run and verified, please share the results here so we can proceed.

[codefora-dev]

Hi @MdAsifHossainNadim — I ran the PHPUnit suite locally for this PR and confirmed the test passes.

Command:

./vendor/bin/phpunit tests/php/src/ReactUrlGenerationTest.php

Output:

Installing WooCommerce...
Installing Dokan...

PHPUnit 9.6.31 by Sebastian Bergmann and contributors.

. 1 / 1 (100%)

OK (1 test, 1 assertion)

No failures or fatals observed. Let me know if you want additional coverage added before merge.

[codefora-dev]

Also a small note about the PHPUnit environment:

In the test bootstrap I had to explicitly instantiate VendorNavMenuChecker. The WordPress PHPUnit runner does not execute the normal dashboard/admin lifecycle where this class is usually registered, so its filters are never attached during unit tests. Without this, dokan_get_navigation_url() does not rewrite to React routes and the assertion fails.

For clarity, this is the only bootstrap addition used during tests:

// Ensure react-route rewrite filters exist during tests.
if ( class_exists( '\WeDevs\Dokan\VendorNavMenuChecker' ) ) {
new \WeDevs\Dokan\VendorNavMenuChecker();
}

This mirrors runtime behavior and is scoped strictly to the test bootstrap — it does not affect production execution.