PROD-10021

[jitendrabanjara1991]

Jira Issue:

https://buddyboss.atlassian.net/browse/PROD-10021

General Note

Keep all conversations related to this PR in the associated Jira issue(s). Do NOT add comment on this PR or edit this PR’s description.

Notes to Developer

• Ensure the IDs (i.e. PROD-1) of all associated Jira issues are reference in this PR’s title
• Ensure that you have achieved the Definition of Done before submitting for review
• When this PR is ready for review, move the associate Jira issue(s) to “Needs Review” (or “Code Review” for Dev Tasks)

Notes to Reviewer

• Ensure that the Definition of Done have been achieved before approving a PR
• When this PR is approved, move the associated Jira issue(s) to “Needs QA” (or “Approved” for Dev Tasks)

[jitendrabanjara1991]

@claude
Please review PR from scratch

[claude]

Claude finished @jitendrabanjara1991's task in 4m 53s —— View job

---

PR Review: PROD-10021 — Integration 2.0 + Shared Admin-Common Layer

• Load tools
• Read PHP backend files
• Read JavaScript/React frontend files
• Read webpack config & build files
• Analyze for security, performance, coding standards
• Post final review

---

Summary

This PR introduces the Integrations marketplace admin page, a shared admin-common UI layer (header + Knowledge Base modal), and a server-side CORS proxy for the buddyboss.com integrations directory. The architecture is sound and mirrors the proven help-content proxy pattern. Security guardrails (SSRF prevention, sanitization) are properly implemented. Below are findings ranging from architectural to cosmetic.

---

Critical / High

1. Cross-bundle import of settings/utils/sanitize from integrations components

Files: src/js/admin/integrations/components/IntegrationCard.js:24, IntegrationDrawer.js:23

// IntegrationCard.js
import { safeUrl } from '../../settings/utils/sanitize';

// IntegrationDrawer.js
import { sanitizeHtml, safeUrl } from '../../settings/utils/sanitize';

integrations is a separate webpack bundle from settings. Relative imports across bundle boundaries cause webpack to bundle settings/utils/sanitize.js (and everything it depends on) twice — once into the settings build and once into the integrations build. This couples two independent apps at the source level.

sanitizeHtml and safeUrl should be exported from @bb/admin-common (add to src/js/admin/common/index.js and expose via the common bundle) so both apps get them from the shared external without duplication. Fix this →

---

2. IntegrationDrawer declares role="dialog" aria-modal="true" but has no focus management

File: src/js/admin/integrations/components/IntegrationDrawer.js:84

<div className="bb-integrations-drawer" role="dialog" aria-modal="true">

When aria-modal="true" is set, assistive technologies expect focus to be moved into the dialog on open and trapped inside it. Currently:

• Focus stays on the card that opened the drawer
• There is no focus trap
• There is no focus restoration on close

The KB modal (KnowledgeBaseModal.js) handles all three correctly and can serve as the reference. Fix this →

---

Medium

3. BBAdminHeader search results use array index as React key

File: src/js/admin/common/components/BBAdminHeader.js:172

{ searchResults.map( ( result, index ) => (
    <button key={ index } ...>

Using index as key breaks React's reconciliation when search results change — the list re-orders or changes length and wrong DOM nodes get updated. The result objects have a route property that uniquely identifies each result; use that instead:

<button key={ result.route || index } ...>

Fix this →

---

4. onSearch prop in Integrations App.js creates a new function on every render

File: src/js/admin/integrations/App.js:150–158

<BBAdminHeader
    onSearch={ ( query, signal ) => {
        const fd = new FormData();
        // ...
        return fetch( ... );
    } }

This inline function is recreated on every render, causing BBAdminHeader to re-render unnecessarily (its useEffect for search has onSearch as a dependency — a new reference every render triggers the effect). Wrap in useCallback with [] deps since adminData values are stable:

const handleHeaderSearch = useCallback( ( query, signal ) => {
    const fd = new FormData();
    fd.append( 'action', 'bb_admin_search_settings' );
    fd.append( 'nonce', adminData.searchNonce || '' );
    fd.append( 'query', query );
    return fetch( adminData.ajaxUrl, { method: 'POST', body: fd, credentials: 'same-origin', signal } )
        .then( ( r ) => r.json() )
        .then( ( r ) => ( r.success ? ( r.data?.results || [] ) : [] ) );
}, [] );

Fix this →

---

5. Redundant onKeyDown handler on a native <button> in IntegrationCard

File: src/js/admin/integrations/components/IntegrationCard.js:34–49

const handleKeyDown = ( e ) => {
    if ( 'Enter' === e.key || ' ' === e.key ) {
        e.preventDefault();
        open();
    }
};

<button type="button" ... onClick={ open } onKeyDown={ handleKeyDown }>

A native <button> already activates its onClick handler on both Enter and Space by default per the HTML spec. The onKeyDown handler is redundant and calling e.preventDefault() on Space can suppress browser scroll, which has no effect here but may surprise future readers. The handler and the open outer function declaration can be removed. Fix this →

---

6. KnowledgeBaseModal still accepts triggerRef prop but no caller passes it

File: src/js/admin/common/components/knowledge-base/KnowledgeBaseModal.js:41

export default function KnowledgeBaseModal( { triggerRef } ) {

The App.js refactor removed the kbTriggerRef (and the triggerRef pass-through) when switching from <Header> to <BBAdminHeader>. The modal's focus-restoration path now always falls through to previouslyFocusedRef (the fallback), so the triggerRef branch on line 60–62 is dead code:

if ( triggerRef && triggerRef.current ) {
    triggerRef.current.focus(); // never reached
}

This can be removed to clean up the public API and avoid confusion.

---

Low / Informational

7. build:admin command builds settings before common

File: package.json

"build:admin": "npm run build:admin:rl-onboarding && npm run build:admin:settings && npm run build:admin:integrations && npm run build:admin:common"

settings and integrations both depend on the common bundle (via @bb/admin-common externalized to window.bbAdminCommon). For a clean CI build, common should be built first. At runtime this is fine (PHP enqueues bb-admin-common before the app scripts), but a developer rebuilding a single target in isolation may get stale output. Suggested order: ... && npm run build:admin:common && npm run build:admin:settings && npm run build:admin:integrations.

8. unset($request) antipattern in PHP permissions check

File: src/bp-core/admin/classes/class-bb-rest-integrations-endpoint.php:169

public function get_item_permissions_check( $request ) {
    unset( $request );

Using unset() to silence "unused parameter" warnings is an antipattern; it mutates the local variable unnecessarily and is confusing. The standard WordPress approach is to either use the parameter or suppress with a PHPCS inline comment. The BB_REST_Help_Content_Endpoint sibling class can serve as reference for whichever style it uses.

9. Stale inline comment in bb-admin-integrations-page.php

File: src/bp-core/admin/bb-admin-integrations-page.php:104–106

// The React app reads window.bbIntegrationsData.{apiUrl,nonce,version}.

The wp_localize_script call actually localizes 7 keys (apiUrl, nonce, adminUrl, version, logoUrl, ipnRootId, ajaxUrl, searchNonce, settingsUrl). The comment only documents 3, which makes it misleading for anyone extending this block later.

10. decodeEntities defined twice in the codebase

File: src/js/admin/common/utils/kbApi.js:253

The file exports its own decodeEntities (for KB article titles) while IntegrationCard.js imports decodeEntities from @wordpress/html-entities. Two different implementations are now in play. The @wordpress/html-entities version is the canonical choice for WP admin apps; the hand-rolled one in kbApi.js can be replaced with the package import (it already handles numeric entities correctly).

11. Loading skeleton grid missing accessible announcement

File: src/js/admin/integrations/components/IntegrationGrid.js:18–24

The 8 skeleton <div> cards render with aria-busy="true" on the grid parent, which is correct. However, screen readers on some AT implementations do not reannounce the loading state when aria-busy flips from false to true. Adding a visually-hidden <span aria-live="polite">{ __('Loading integrations…', 'buddyboss') }</span> alongside the skeletons would make the loading state explicit for screen reader users.

---

What's Working Well

• SSRF guardrails in class-bb-rest-integrations-endpoint.php are thorough: double-slash rejection, .. traversal check, control-character rejection, fragment rejection, length cap, and a final wp_http_validate_url() pass. The host is never client-controlled.
• Two-tier cache in integrationsApi.js (12h server transient + versioned localStorage) is a clean pattern; the version-keyed CACHE_PREFIX ensures a plugin upgrade busts the client cache automatically.
• withCommonExternal in webpack.config.js correctly replaces (rather than appending to) the existing DependencyExtractionWebpackPlugin instance, avoiding the double-instance conflict.
• bb_register_integrations_rest_route correctly registers on rest_api_init (outside the is_admin() gate in bb_admin_settings_init), matching the existing help-content proxy pattern.
• Abort controller usage in both App.js files and IntegrationDrawer.js is consistent and correct — inflight requests are always cancelled on filter change or unmount.
• KnowledgeBaseModal updated to read bbAdminData || bbIntegrationsData so it works from both host apps without prop drilling the admin data.

[jitendrabanjara1991]

@claude
Please review PR from scratch

[claude]

Claude finished @jitendrabanjara1991's task in 4m 14s —— View job

---

PR Review: PROD-10021 — Integration 2.0 + Shared Admin-Common Layer

• Load tools
• Read PHP backend files
• Read JavaScript/React frontend files
• Read webpack config & build files
• Analyze for security, performance, coding standards
• Post final review

---

Summary

This PR introduces three related things: a shared admin-common React layer (header, KB modal, sanitizers) built as its own webpack bundle; an Integrations marketplace admin page; and a server-side CORS proxy mirroring the proven KB proxy pattern. The architecture is sound. Most issues from the previous review are resolved. A few remain, detailed below.

---

Previously Reported — Now Fixed ✅

#	Finding	Status
1	Cross-bundle import of settings/utils/sanitize from integrations	✅ Fixed — both consumers now import from @bb/admin-common
2	IntegrationDrawer missing focus management / focus trap	✅ Fixed — three useEffect hooks handle capture, restore, and Tab trap
3	BBAdminHeader search results use array index as React key	✅ Fixed — uses result.route || index
4	onSearch in Integrations App.js creates new function on every render	✅ Fixed — wrapped in useCallback
5	Redundant onKeyDown on native <button> in IntegrationCard	✅ Fixed — removed
6	Dead triggerRef prop on KnowledgeBaseModal	✅ Fixed — removed
7	build:admin builds settings before common	✅ Fixed — common now builds first

---

Medium

1. Settings App.js inline onSearch creates a new function on every render

File: src/js/admin/settings/App.js:374–378

<BBAdminHeader
    ...
    onSearch={ ( query, signal ) =>
        ajaxFetch( 'bb_admin_search_settings', { query }, { signal } ).then(
            ( response ) => ( response.success ? ( response.data?.results || [] ) : [] )
        )
    }
    onSelectResult={ ( result ) => setCurrentRoute( result.route ) }

The Integrations App.js wraps its equivalent handlers in useCallback( ..., [] ), which gives them stable references. The Settings App.js does not. BBAdminHeader has onSearch in its debounced-search useEffect dependency array — a new function reference on every parent render triggers an unnecessary re-subscription (clears the 300ms timer, re-attaches the AbortController, discards partial input). While typically benign in a single-user admin page, it introduces subtle UX jitter and diverges from the pattern already established in the integrations app.

Fix: wrap both handlers in useCallback:

const handleSearch = useCallback( ( query, signal ) =>
    ajaxFetch( 'bb_admin_search_settings', { query }, { signal } )
        .then( ( r ) => ( r.success ? ( r.data?.results || [] ) : [] ) ),
    [] // ajaxFetch is module-scoped, stable
);
const handleSelectResult = useCallback( ( result ) => setCurrentRoute( result.route ), [ setCurrentRoute ] );

Fix this →

---

2. kbApi.js still exports its own decodeEntities while integration components use @wordpress/html-entities

File: src/js/admin/common/utils/kbApi.js:253

// kbApi.js — hand-rolled implementation
export function decodeEntities( str ) { ... }

// IntegrationCard.js + IntegrationDrawer.js — WP canonical
import { decodeEntities } from '@wordpress/html-entities';

Two implementations with different coverage: the hand-rolled one in kbApi.js handles 8 named entities + numeric decimal/hex ranges; @wordpress/html-entities handles all named HTML5 entities. If buddyboss.com integration titles ever include –, «, ’, or any named entity beyond the hand-rolled set, titles in the KB landing/category views will render with literal &…; characters while the same entity in an integration card renders correctly.

The canonical fix is to replace the kbApi.js hand-rolled function with the @wordpress/html-entities import. The package is already in the dependency graph (both IntegrationCard.js and IntegrationDrawer.js import from it and it's a WP-externalized package).

Fix this →

---

Low

3. unset($request) and unset($e) antipatterns

Files: src/bp-core/admin/classes/class-bb-rest-integrations-endpoint.php:169, src/bp-core/admin/bb-admin-integrations-page.php:136

// Endpoint permissions check
public function get_item_permissions_check( $request ) {
    unset( $request ); // suppress "unused variable" warning

// IPN root ID resolution
} catch ( Throwable $e ) {
    unset( $e ); // suppress "unused variable" warning
}

Using unset() to silence unused-parameter/unused-variable warnings is a PHP code smell. It mutates the local scope and confuses readers who wonder if the unset has any semantic purpose. The standard WordPress approach is an inline PHPCS disable comment:

// For unused parameter:
public function get_item_permissions_check( $request ) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.Found

// For unused catch variable (PHP 8.0+ supports catch without a variable):
} catch ( Throwable $e ) { // @phpstan-ignore-next-line
    // Non-fatal; IPN root ID falls back to structural selector.
}

The sibling BB_REST_Help_Content_Endpoint can serve as the reference for whichever style the team prefers.

Fix this →

---

4. No thundering-herd guard on integrations proxy transient

File: src/bp-core/admin/classes/class-bb-rest-integrations-endpoint.php:204–215

$cached = get_transient( $transient_key );
if ( false !== $cached && is_array( $cached ) && isset( $cached['body'] ) ) {
    return rest_ensure_response( $cached );
}
$envelope = $this->fetch_remote( $target );
// ...
set_transient( $transient_key, $envelope, self::TRANSIENT_TTL );

The file's own docblock acknowledges this mirrors the KB proxy. The KB proxy has search-index locking to prevent a thundering herd (multiple concurrent cache misses all hitting the upstream). That risk is lower here — admin-only endpoint, distinct cache keys per URL, low concurrency — so no action is strictly required, but it's worth confirming whether the KB proxy actually has a lock that was intentionally omitted, or whether it was simply not needed and not omitted.

---

5. Double "the" typo in bb-admin-common-assets.php

File: src/bp-core/admin/bb-admin-common-assets.php:30

 * dependencies so the the layer ships once across all admin React apps.

Fix this →

---

Informational

6. commonConfig is the only webpack target not wrapped by withCommonExternal

File: src/js/admin/webpack.config.js

This is correct and intentional — the common bundle IS window.bbAdminCommon; it should not externalize itself. The withCommonExternal wrapper is only for consumer bundles (settings, integrations). Worth a one-line comment in webpack.config.js above the commonConfig declaration so future maintainers don't accidentally add it:

// NOTE: commonConfig intentionally omits withCommonExternal() — it IS bbAdminCommon,
// so it must not externalize itself. Consumer configs (settings, integrations) use withCommonExternal.
const commonConfig = { ... };

---

What's Working Well

• SSRF guardrails in class-bb-rest-integrations-endpoint.php are thorough: double-slash rejection, .. traversal, control-character check, fragment rejection, length cap, and a final wp_http_validate_url() pass. Host is never client-controlled.
• Two-tier cache in integrationsApi.js (12h server transient + versioned localStorage) is clean; the version-keyed CACHE_PREFIX busts the client cache automatically on plugin upgrades.
• Sanitizer consolidation is complete: settings/utils/sanitize.js is now a 1-line re-export shim; both apps get the sanitizer from @bb/admin-common with zero duplication.
• withCommonExternal in webpack.config.js correctly replaces (not appends to) the existing DependencyExtractionWebpackPlugin instance, avoiding the double-instance conflict that would corrupt .asset.php files.
• KnowledgeBaseModal correctly reads bbAdminData || bbIntegrationsData, making it host-agnostic without prop drilling.
• Abort controller usage throughout App.js, IntegrationDrawer.js, and integrationsApi.js is consistent — stale requests are always cancelled on filter change or unmount.
• Focus management in IntegrationDrawer is now complete with open-focus, close-restore, and Tab-trap matching the KnowledgeBaseModal pattern.
• build:admin order is correct: common → settings → integrations, ensuring no stale external at build time.

[jitendrabanjara1991]

@claude
Please review PR from scratch

[claude]

Claude finished @jitendrabanjara1991's task in 5m 3s —— View job

---

PR Review: PROD-10021 — Integration 2.0 + Shared Admin-Common Layer

• Load tools
• Read PHP backend files
• Read JavaScript/React frontend files
• Read webpack config & build files
• Compare against previous review findings
• Post final review

---

Summary

This is a fresh, from-scratch review of the full diff. The PR introduces an Integrations marketplace admin page, a shared admin-common React layer, and a server-side CORS proxy mirroring the KB proxy pattern. All 10 issues raised in the two prior review cycles have been resolved. This review identifies 3 issues still open from review 2 and 2 new findings not previously raised.

---

Previously Reported — Now Fixed ✅

#	Finding	Source Review
1	Cross-bundle import of settings/utils/sanitize from integrations	Review 1
2	IntegrationDrawer missing focus management / focus trap	Review 1
3	BBAdminHeader search results use array index as React key	Review 1
4	onSearch in Integrations App.js creates new function on every render	Review 1
5	Redundant onKeyDown on native <button> in IntegrationCard	Review 1
6	Dead triggerRef prop on KnowledgeBaseModal	Review 1
7	build:admin builds settings before common	Review 1
8	Settings App.js onSearch not wrapped in useCallback	Review 2
9	kbApi.js hand-rolled decodeEntities vs @wordpress/html-entities	Review 2
10	commonConfig webpack missing self-exclusion comment	Review 2

---

Still Open From Review 2

1. unset($request) and unset($e) antipatterns

Files: src/bp-core/admin/classes/class-bb-rest-integrations-endpoint.php:169, src/bp-core/admin/bb-admin-integrations-page.php:128

// Endpoint permissions check
public function get_item_permissions_check( $request ) {
    unset( $request ); // suppress "unused variable" warning

// IPN root ID resolution
} catch ( Throwable $e ) {
    unset( $e );
}

The standard approach for $request is an inline PHPCS disable comment; for $e the cleanest fix is a PHP 8.0 bare catch ( Throwable ) (no binding). Mutating the local scope with unset() is surprising to readers and accomplishes nothing functionally.

Fix this →

---

2. Double "the" typo in bb-admin-common-assets.php

File: src/bp-core/admin/bb-admin-common-assets.php:22

 * so the the layer ships once and is never duplicated across bundles.

Fix this →

---

New Findings

3. Protocol-relative URL bypass in isAllowedUrl

File: src/js/admin/common/utils/sanitize.js:256

function isAllowedUrl(url) {
    // Allow relative URLs
    if (url.startsWith('/') || url.startsWith('#') || url.startsWith('?')) {
        return true;  // ← '//evil.com/path' passes here
    }
    // scheme check never reached for protocol-relative URLs
    try {
        const parsed = new URL(url, window.location.origin);
        return ALLOWED_SCHEMES.includes(parsed.protocol);
    } catch (e) { return false; }
}

//evil.com/path starts with / and is treated as a "relative URL", bypassing the scheme allowlist. In the browser, <a href="//evil.com/path"> navigates to https://evil.com/path. For an admin-only page consuming trusted API data this risk is low today, but it is a latent bug in the shared sanitizer that affects every consumer (sanitizeHtml + safeUrl).

Fix: tighten the relative-URL check to exclude protocol-relative paths:

// Only true root-relative paths (one leading slash) are relative.
// Protocol-relative '//host/path' must fall through to the scheme check.
if ( (url.startsWith('/') && !url.startsWith('//'))
     || url.startsWith('#')
     || url.startsWith('?') ) {
    return true;
}

Fix this →

---

4. onClose inline arrow function causes Escape key listener churn in IntegrationDrawer

Files: src/js/admin/integrations/App.js:259, src/js/admin/integrations/components/IntegrationDrawer.js:116

// App.js — new reference every render
{ activeSlug && (
    <IntegrationDrawer
        slug={ activeSlug }
        initialTitle={ activeTitle }
        onClose={ () => setActiveSlug( null ) }  // ← inline
    />
) }

// IntegrationDrawer.js — onClose is in the dep array
useEffect( () => {
    const onKey = ( e ) => {
        if ( 'Escape' === e.key ) { onClose(); }
    };
    document.addEventListener( 'keydown', onKey );
    return () => document.removeEventListener( 'keydown', onKey );
}, [ onClose ] );  // ← re-runs every time App re-renders

Every re-render of AppInner produces a new onClose reference, which causes the IntegrationDrawer Escape key effect to fire its cleanup and re-register a fresh keydown listener. During typical usage (category dropdown changes, list load, debounced search) this means the listener is continuously torn down and rebuilt while the drawer is open. Add useCallback to App.js:

const handleDrawerClose = useCallback( () => setActiveSlug( null ), [] );

// also wrap onSelect for consistency:
const handleIntegrationSelect = useCallback( ( slug, title ) => {
    setActiveTitle( title || '' );
    setActiveSlug( slug );
}, [] );

Fix this →

---

What's Working Well

• SSRF guardrails in class-bb-rest-integrations-endpoint.php are thorough: double-slash rejection, .. traversal, control-character check, fragment rejection, length cap, and final wp_http_validate_url() pass. Host is never client-controlled.
• Two-tier cache in integrationsApi.js (12h server transient + versioned localStorage) is clean; the version-keyed CACHE_PREFIX busts the client cache automatically on plugin upgrades.
• Sanitizer consolidation is complete: settings/utils/sanitize.js is now a 1-line re-export shim; both apps source the sanitizer from @bb/admin-common with zero duplication.
• decodeEntities consolidation is complete: kbApi.js now imports from @wordpress/html-entities and re-exports it, so the whole admin surface decodes entities one consistent way.
• Settings App.js handleHeaderSearch / handleHeaderSelectResult are correctly wrapped in useCallback with stable deps, matching the integrations app.
• withCommonExternal in webpack.config.js correctly replaces (not appends to) the existing DependencyExtractionWebpackPlugin instance; the self-exclusion comment is now in place.
• build:admin order is correct: rl-onboarding → common → settings → integrations.
• IntegrationDrawer focus management is complete: open-focus, Tab-trap, and close-restore all implemented, matching the KnowledgeBaseModal reference pattern.
• REST route registration for the integrations proxy correctly hooks on rest_api_init (outside the is_admin() gate), matching the KB proxy pattern.
• Abort controller usage throughout is consistent — stale requests are always cancelled on filter change or unmount.

[jitendrabanjara1991]

@claude
Please review PR from scratch

[claude]

Claude finished @jitendrabanjara1991's task in 5m 28s —— View job

---

PR Review: PROD-10021 — Integration 2.0 + Shared Admin-Common Layer

• Load tools
• Read PHP backend files
• Read JavaScript/React frontend files
• Read webpack config & build files
• Compare against previous review findings
• Post final review

---

Summary

This is a fresh, from-scratch review of the full diff (4th cycle). All 14 issues raised across three prior reviews are resolved except one. One new medium finding is identified. The PR is otherwise production-ready.

---

Previously Reported — Now Fixed ✅

#	Finding	Source Review
1	Cross-bundle import of settings/utils/sanitize from integrations	Review 1
2	IntegrationDrawer missing focus management / focus trap	Review 1
3	BBAdminHeader search results using array index as React key	Review 1
4	onSearch in Integrations App.js creating new function on every render	Review 1
5	Redundant onKeyDown on native <button> in IntegrationCard	Review 1
6	Dead triggerRef prop on KnowledgeBaseModal	Review 1
7	build:admin building settings before common	Review 1
8	Settings App.js onSearch not wrapped in useCallback	Review 2
9	kbApi.js hand-rolled decodeEntities vs @wordpress/html-entities	Review 2
10	commonConfig webpack missing self-exclusion comment	Review 2
11	unset($request) and unset($e) antipatterns	Review 3
12	Protocol-relative URL bypass in isAllowedUrl	Review 3
13	onClose inline arrow causing Escape key listener churn	Review 3

---

Still Open from Review 3

1. Double "the" typo in bb-admin-common-assets.php

File: src/bp-core/admin/bb-admin-common-assets.php:21

 * the the layer ships once and is never duplicated across bundles.

Fix this →

---

New Findings

2. role="tablist" missing arrow-key navigation — tech-debt before Free/Pro tabs are re-enabled

File: src/js/admin/integrations/App.js:201–231

<div className="bb-integrations__tabs" role="tablist">
    <button type="button" role="tab" aria-selected={...} onClick={...}>All</button>
    <button type="button" role="tab" aria-selected={...} disabled>Free</button>
    <button type="button" role="tab" aria-selected={...} disabled>Pro</button>
</div>

The WAI-ARIA Tabs Pattern requires keyboard navigation via arrow keys — Tab moves focus to the active tab panel, and Left/Right Arrow cycles between tab items. Currently, there is no onKeyDown handler on the tablist. Today this is invisible: only the "All" tab is focusable (Free/Pro are native-disabled and outside the interactive accessibility tree), so there is no navigation to implement. However, the spec comment at line 196–200 explicitly states these tabs will be re-enabled once the API exposes the free/pro field (Q5). When that happens, keyboard navigation will silently be missing.

The fix is to add the arrow-key handler before unlocking those tabs — not necessarily now, but tracked before the Q5 API work lands:

const handleTabKeyDown = useCallback( ( e ) => {
    const tabs = e.currentTarget.querySelectorAll( '[role="tab"]:not([disabled])' );
    const index = Array.from( tabs ).indexOf( document.activeElement );
    if ( 'ArrowRight' === e.key && index < tabs.length - 1 ) {
        tabs[ index + 1 ].focus();
    } else if ( 'ArrowLeft' === e.key && index > 0 ) {
        tabs[ index - 1 ].focus();
    }
}, [] );

<div className="bb-integrations__tabs" role="tablist" onKeyDown={ handleTabKeyDown }>

Fix this →

---

What's Working Well

• All 13 previous review issues resolved. Specific callouts for the biggest fixes:
• Protocol-relative URL fix in isAllowedUrl (sanitize.js:258–264) is correct: url.startsWith('/') && !url.startsWith('//') now properly falls through to the scheme allowlist for //evil.com paths.
• onClose + onSelect useCallback wrappers in App.js (lines 150–154) are correct — both have empty deps and the Escape-key effect in IntegrationDrawer will now register exactly once and remain stable.
• IntegrationDrawer focus management is complete: open-focus on close button, Tab-trap within the panel, and close-restore to previously focused element. All three effects match the KnowledgeBaseModal reference.
• kbApi.js imports decodeEntities from @wordpress/html-entities (line 21) and re-exports it, eliminating the hand-rolled 8-entity implementation; the whole admin surface now decodes entities one consistent way.
• integrationsApi.js two-tier cache (12h server transient + versioned localStorage) is well-structured; CACHE_PREFIX is keyed to the plugin version so a platform upgrade automatically busts the client cache.
• SSRF guardrails in class-bb-rest-integrations-endpoint.php remain thorough: double-slash rejection, .. traversal, control-character check, fragment rejection, length cap, and wp_http_validate_url() final pass. Host is never client-controlled.
• bb-admin-settings-init.php and class-bp-admin.php integrate the new Integrations page cleanly, following the same patterns as the Settings page (file-existence guards, add_submenu_page, rest_api_init hook for the proxy route).
• bb-admin-integrations-page.php excludes the integrations page from the legacy bb-tab-header in bp-core-admin-actions.php — no double header bar.
• build:admin order is correct: rl-onboarding → common → settings → integrations.

---

[jitendrabanjara1991]

@claude
Please review PR from scratch. also check below comment

On #2 — I deliberately did not add the handler now (it'd be dead code until Free/Pro unlock, and the reviewer themselves said
"not necessarily now"). The requirement is now captured right where the future dev will re-enable the tabs, so it can't be
silently missed. If you'd rather have the handler wired in now anyway (it's ~8 lines, harmless), say the word and I'll add it.

[claude]

Claude finished @jitendrabanjara1991's task in 4m 21s —— View job

---

PR Review: PROD-10021 — Integration 2.0 + Shared Admin-Common Layer

• Load tools
• Read PHP backend files
• Read JavaScript/React frontend files
• Check open findings from review 4
• Analyze for new issues
• Post final review

---

Summary

Fresh read of all 53 changed files (5th cycle). All 15 issues raised across four prior review cycles are resolved. No new critical or medium issues found.

---

Previously Reported — All Fixed ✅

#	Finding	Source
1	Cross-bundle import of settings/utils/sanitize from integrations	Review 1
2	IntegrationDrawer missing focus management / focus trap	Review 1
3	BBAdminHeader search results using array index as React key	Review 1
4	onSearch in Integrations App.js creating new function on every render	Review 1
5	Redundant onKeyDown on native <button> in IntegrationCard	Review 1
6	Dead triggerRef prop on KnowledgeBaseModal	Review 1
7	build:admin building settings before common	Review 1
8	Settings App.js onSearch not wrapped in useCallback	Review 2
9	kbApi.js hand-rolled decodeEntities vs @wordpress/html-entities	Review 2
10	commonConfig webpack missing self-exclusion comment	Review 2
11	unset($request) and unset($e) antipatterns	Review 3
12	Protocol-relative URL bypass in isAllowedUrl	Review 3
13	onClose inline arrow causing Escape key listener churn	Review 3
14	Double "the" typo in bb-admin-common-assets.php	Review 4
15	role="tablist" missing arrow-key navigation	Review 4

---

On issue 15 — tablist arrow-key navigation

The developer's decision to defer the handler is accepted. The comment at App.js:196–202 is a three-step checklist co-located with the code that changes:

{ /* Tier tabs — Figma layout. Only "All" is wired today; Free/Pro
     are disabled (not no-op) until the API exposes a free/pro
     field (Q5), so a visible filter never silently returns the
     same results. Re-enable by: (1) removing `disabled`, (2) handling
     `tier` in the list query, and (3) adding WAI-ARIA arrow-key
     navigation (onKeyDown on the tablist — Left/Right cycles tabs);
     a11y nav is unnecessary while only one tab is focusable. */ }

With only one focusable tab, the WAI-ARIA Tabs Pattern's arrow-key requirement is not currently violated. The prior review explicitly said "not necessarily now." The comment ensures the future developer cannot unlock the tabs without seeing the a11y requirement. This is an acceptable resolution.

---

Informational

catch (Throwable $e) with $e unread — bb-admin-integrations-page.php:127

} catch ( Throwable $e ) {
    // Non-fatal: IPN service unavailable — $ipn_root_id stays empty …
    $ipn_root_id = '';
}

The unset($e) antipattern is gone (fix from Review 3). The variable is still bound but never read, which PHP itself is fine with. PHPCS/WordPress standards don't flag unused catch bindings, so this won't break CI. PHPStan level 4+ would flag it; the fix for PHP 7.4 (can't use bare catch (Throwable)) is an inline suppression comment. Not a blocker — noting for completeness in case the project runs PHPStan.

---

What's Working Well — Final State

• SSRF guardrails (class-bb-rest-integrations-endpoint.php) are complete: double-slash rejection, .. traversal, control-character check, fragment rejection, length cap, wp_http_validate_url() final pass. Host is never client-controlled.
• Protocol-relative URL fix (sanitize.js:258–263): url.startsWith('/') && !url.startsWith('//') correctly falls through to the scheme allowlist for //evil.com paths.
• Two-tier cache in integrationsApi.js (12h server transient + versioned localStorage): version-keyed CACHE_PREFIX busts client cache on plugin upgrade.
• Sanitizer consolidation: settings/utils/sanitize.js is a 1-line re-export shim; both apps source sanitizers from @bb/admin-common with zero duplication.
• decodeEntities consolidation: kbApi.js imports from @wordpress/html-entities and re-exports it; the whole admin surface decodes entities one consistent way.
• withCommonExternal in webpack.config.js correctly replaces (not appends to) the existing DependencyExtractionWebpackPlugin; the self-exclusion comment is in place above commonConfig.
• build:admin order (package.json:59): rl-onboarding → common → settings → integrations — correct.
• handleDrawerClose and handleIntegrationSelect both wrapped in useCallback(…, []) — Escape-key listener in IntegrationDrawer registers exactly once and stays stable.
• IntegrationDrawer focus management: open-focus on close button, Tab-trap within panel, close-restore to opener element — all three match the KnowledgeBaseModal reference.
• IntegrationGrid loading state: aria-busy="true" on the grid + visually-hidden aria-live="polite" span confirms the loading state to screen readers.
• BBAdminHeader search keys use result.route || index — correct reconciliation under result-set changes.
• REST proxy route registered on rest_api_init (outside the is_admin() gate) — correct, matching the KB proxy pattern.

This PR is ready to approve.