Re: [PR] fix(reports): paginate Slack recipient picker for large workspaces [superset]

via GitHub Tue, 14 Jul 2026 11:43:02 -0700


EnxDev commented on PR #41998:
URL: https://github.com/apache/superset/pull/41998#issuecomment-4972729960


   ## EnxDev's Review Agent — apache/superset#41998 · HEAD fe3a25a
   comment — solid, well-tested fix; the async picker + pagination are correct 
and CI is green. Two minor items plus an answer to the open payload thread.
   
   Verified: backend paginates the warm-cached filtered set and returns full 
`count` (`api.py`); the frontend now sends `search_string`/`exact_match` (was 
`searchString`/`exactMatch`, silently dropped) so search finally reaches the 
server; `AsyncSelect(search, page, pageSize) => {data, totalCount}` wiring 
matches the core signature; edit-mode `exact_match` resolve is bounded to saved 
ids; force-refresh remount discards the internal cache and re-fetches page 0. 
All 8 jest shards + backend suites pass.
   
   ### 🟡 Should-fix
   - **`superset/reports/api.py:692`** (answers @geido's payload thread) — the 
picker only consumes `id`/`name`/`is_private`/`is_member`, but each row still 
ships the full `SlackChannelSchema`. Pagination already bounds the row count; 
the remaining trim is projecting those four fields server-side before 
`self.response(...)`. Small win, but it directly closes the open thread.
   
   ### 🔵 Nits
   - **`NotificationMethod.tsx`** — `RefreshLabel 
disabled={isSlackChannelsLoading}` is effectively dead: the only 
`setIsSlackChannelsLoading(true)` lived in the deleted `updateSlackOptions`, so 
after mount the flag is stuck `false` and Force-refresh never disables the 
button. AsyncSelect's own dropdown spinner still fires on the remount, so 
impact is cosmetic — either drive the flag around `onRefreshSlackChannels` or 
drop it.
   - **Test gaps** — no coverage for the two new-ish paths: edit-mode id→name 
resolution (`resolveSavedRecipients`) and the force-refresh remount. Backend 
also doesn't test `page` supplied without `page_size` (falls through to the 
full list) — cheap to pin.
   - The bot type-hint comments (annotate `page`/`page_size`/`count`/`start`) 
are noise; mypy doesn't require local-variable annotations.
   
   ### 🙌 Praise
   - `allowNewOptions` paste-a-channel-ID as the guaranteed unblock for huge 
workspaces is the right call — SlackV2 send takes ID strings directly, so it 
works without any list membership.
   
   <!-- enxdev-review-agent:fe3a25a -->
   _Reviewed by EnxDev's Review Agent — @EnxDev · HEAD fe3a25a._
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] fix(reports): paginate Slack recipient picker for large workspaces [superset]

Reply via email to