PROD-9616

[jitendrabanjara1991]

Jira Issue:

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

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

[claude]

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

---

Code Review

• Gather context (PR diff, CLAUDE.md)
• Understand the request (code review)
• Review changed files
• Post review feedback

Summary

This PR fixes a real bug: deleting (or spamming) an activity from inside the "view more comments" modal only removed the modal's copy of the <li>, leaving the modal open as an empty shell while the feed's copy went stale. The fix removes the feed copy, closes the modal via its real close button (falling back to hiding the wrapper if a theme override renamed the control), and scrolls the feed to the deleted post's neighbor.

The four commits show a genuine two-pass self-review: the third commit ("Harden the modal-delete flow per review") already fixes the exact issues I'd normally flag — prev()/next() → prevAll()/nextAll().first() for robustness against a "load more" row, excluding the .activity-popup/.bb-rl-activity-popup template placeholder <li> from anchor selection, resetting activityPinHasUpdates before the synthetic close-button click (to stop activitySyncOnModalClose's unconditional refreshActivities() from clobbering the scroll-anchor), and fixing the previously-dead media-path gate (the old gate checked for a selector that a prior .remove() call had already emptied). The fourth commit correctly extends the pin-flag reset to the media-delete path in legacy buddypress-media.js only, and its message explains why ReadyLaunch doesn't need it — I verified this independently: ReadyLaunch's activitySyncOnModalClose (readylaunch/js/buddypress-activity.js:2465) has no branch that reads activityPinHasUpdates at all, so there's nothing to reset there. Confirmed correct, not an oversight.

Things verified while reviewing

• All 4 .min.js files were rebuilt (checked via distinctive selector strings like bb-modal-activity-header, since local var names get mangled by the minifier).
• The .activity-popup / .bb-rl-activity-popup exclusion correctly targets the hidden template <li> in activity-loop.php:35 / readylaunch/activity/activity-loop.php:50, not a real feed item.
• The new block sits before the existing redirect early-return (single-activity-page delete) and after the comment-count bookkeeping, in both legacy and ReadyLaunch files — consistent placement.
• Indentation (tabs) and general style match the surrounding code; the unguarded parseInt(...) (no radix arg) matches the existing convention at buddypress-activity.js:3597, not a new issue.
• Spam actions share the same removal code path as delete, so it's correct that this block isn't delete-only gated.

Minor/non-blocking observations

• src/bp-templates/bp-nouveau/js/buddypress-activity.js:1301 and the ReadyLaunch equivalent build the close-button selector fresh ($('#activity-modal .bb-modal-activity-header .bb-close-action-popup')) rather than reusing an already-scoped jQuery object; harmless given the single modal instance on the page, just a very minor duplication vs. the media.js version which correctly scopes from $activityModal.
• No new PHP was touched and no new public JS functions were added, so the @since BuddyBoss [BBVERSION] docblock requirement doesn't apply here.

I did not run npm run lint-js / composer lint-php or the Jest/PHPUnit suites in this environment — network/tool restrictions prevented npm install/dependency fetch. Given this is JS-only and the logic traces through cleanly by hand, I'd still recommend a manual smoke test in a browser: delete an activity from inside the modal (a) with a prior pin toggle in the same modal session, and (b) via the "delete last photo" media path, to confirm the scroll-anchor isn't overridden by a feed reload and the modal closes cleanly in both legacy and ReadyLaunch modes.

Overall: solid, well-reasoned fix with good edge-case handling. No blocking issues found.

[jitendrabanjara1991]

@claude
Please review PR

[claude]

Code Review

• Gather context (PR diff, CLAUDE.md)
• Understand the request (code review)
• Review changed files
• Post review feedback

Summary

This PR fixes a real bug: deleting (or spamming) an activity from inside the "view more comments" modal only removed the modal's copy of the <li>, leaving the modal open as an empty shell while the feed's copy went stale. The fix removes the feed copy, closes the modal via its real close button (falling back to hiding the wrapper if a theme override renamed the control), and scrolls the feed to the deleted post's neighbor. This is functionally the same change set as when I reviewed it earlier today — I re-verified it independently rather than reusing that pass.

Verified correct

• is_comment guard is right: ajaxData.is_comment is only set when activity_comment_id is present (buddypress-activity.js:1206-1207), so the new block correctly fires only for top-level activity deletes, not comment deletes. Comment-count bookkeeping and the new block are mutually exclusive branches, so their relative ordering doesn't matter functionally.
• Single-activity-page redirect is unaffected: the response.data.redirect early return (buddypress-activity.js:1228-1230) fires before the new block, so that path is untouched, as claimed.
• Template placeholder <li> exclusion is correct: activity-loop.php:35 and readylaunch/activity/activity-loop.php:50 show the hidden clone-source <li> carries activity-popup / bb-rl-activity-popup respectively — excluding it from prevAll()/nextAll() anchor selection is necessary and correctly done. (It has no data-bp-activity-id, so it can't be matched as $feedCopy itself, but it could otherwise be picked as a bogus scroll anchor.)
• activityPinHasUpdates reset is necessary in legacy, correctly scoped to legacy: in buddypress-activity.js:4365-4372, legacy's activitySyncOnModalClose calls bp.Nouveau.refreshActivities() unconditionally whenever activityPinHasUpdates is true — regardless of whether the feed item still exists — which would clobber the scroll-anchor set by the new code. I checked ReadyLaunch's equivalent (readylaunch/js/buddypress-activity.js:2465-2515) and confirmed it has no activityPinHasUpdates branch at all; its only conditional refresh is gated on $pageActivitylistItem.length > 0, and since the feed copy is already removed before the close button is triggered, that branch can't fire either way. So the fourth commit's decision to only add the pin-flag reset to legacy buddypress-media.js (not ReadyLaunch) is correct, not an oversight.
• Media-delete path selectors line up: buddypress-media.js:1413-1424 removes by body #buddypress .activity-list li#activity-{id}, which matches both the feed and modal copies (both use .activity-list and the modal template markup in activity-modal.php:20-21 confirms the shared class). The gate 0 === $activityModal.find('.bb-modal-activity-body .activity-list li').length is checked after those removals, so it accurately reflects whether the modal was emptied.
• Scroll-offset code matches existing convention: the new adminBar/scrollTop logic at buddypress-activity.js:1311-1312 is a near-exact match of the existing pattern at buddypress-activity.js:3595-3597, including the unguarded parseInt() (no radix) — consistent with existing style, not a new issue.
• All 4 .min.js files were rebuilt: confirmed new selector strings (bb-modal-activity-header, activityPinHasUpdates, bb-rl-modal-activity-header) are present in the corresponding minified output.

Minor/non-blocking observations

• buddypress-activity.js:1301 and the ReadyLaunch equivalent rebuild the close-button selector fresh ($('#activity-modal .bb-modal-activity-header .bb-close-action-popup')) rather than reusing an already-scoped jQuery object; harmless given the single modal instance per page, just a very minor duplication vs. the media.js version, which scopes from $activityModal.
• No new PHP was touched and no new public JS functions were added, so the @since BuddyBoss [BBVERSION] docblock requirement doesn't apply here.

I did not run npm run lint-js / composer lint-php or the Jest/PHPUnit suites in this environment (network/tool restrictions prevent npm install/dependency fetch). Given this is JS-only and the logic traces through cleanly by hand — including tracing the exact conditional paths in activitySyncOnModalClose for both legacy and ReadyLaunch — I'd still recommend a manual smoke test in a browser: delete an activity from inside the modal (a) with a prior pin toggle in the same modal session, and (b) via the "delete last photo" media path, in both legacy and ReadyLaunch modes.

Overall: solid, well-reasoned fix with good edge-case handling. No blocking issues found.

View job run

[jitendrabanjara1991]

@claude
Check regarding. - buddypress-activity.js:1301 and the ReadyLaunch equivalent rebuild the close-button selector fresh ($('#activity-modal .bb-modal-activity-header .bb-close-action-popup')) rather than reusing an already-scoped jQuery object; harmless given the single modal instance per page, just a very minor duplication vs. the media.js version, which scopes from $activityModal.

"Confirmed real, cosmetic only — activity.js has no cached modal object to scope from (its gate uses closest().length directly), media.js scopes because it already held the object for its gate. Accepting the variance; will align if these blocks are touched again."

[claude]

Tasks

• Gather context (PR diff, prior review comments)
• Verify the claim about buddypress-activity.js:1301 vs buddypress-media.js selector scoping
• Post findings

Verification

Confirmed — the explanation is accurate. Checked all four files directly:

buddypress-activity.js (legacy):

• The modal gate at line 1287 is if ( ! ajaxData.is_comment && li_parent.closest( '#activity-modal' ).length ) — it evaluates .closest(...).length inline as a boolean condition and never assigns the jQuery result to a variable.
• So at line 1301, there is nothing cached to reuse — $( '#activity-modal .bb-modal-activity-header .bb-close-action-popup' ) has to be built fresh.

buddypress-media.js (legacy):

• Line 1423: var $activityModal = $( '#activity-modal:visible' ); — the gate itself needs the modal object ($activityModal.find(...) at line 1424), so it's assigned to a variable as a natural side effect of the gate check.
• Line 1425 then reuses that same $activityModal to scope the close-button lookup: $activityModal.find( '.bb-modal-activity-header .bb-close-action-popup' ).

ReadyLaunch equivalents follow the identical pattern:

• readylaunch/js/buddypress-activity.js:3091 — same inline .closest( '#bb-rl-activity-modal' ).length boolean gate, no cached object; line 3100 rebuilds the selector fresh, same as legacy.
• readylaunch/js/buddypress-media.js:964-966 — same $activityModal cache-and-reuse pattern as legacy media.js.

So the root cause is structural, not an oversight: activity.js's gate only ever needed a boolean (.length), while media.js's gate needed the actual modal object to run .find() against — that object then happened to be reusable for the close-button lookup. Aligning activity.js to also cache a $activityModal-style variable would be a valid future cleanup, but it's a no-op today (single modal instance per page, both selectors resolve identically). Agreed this is fine to leave as-is and revisit only if these blocks are touched again.

View job run