ref(np): Dispatch provider renderers through a registry - #124857
Draft
hobzcalvin wants to merge 1 commit into
Draft
hobzcalvin wants to merge 1 commit into
hobzcalvin wants to merge 1 commit into
Conversation
Replaces the per-provider `get_renderer` overrides with a `NotificationRendererRegistry` keyed by provider and notification source. Renderers register themselves where they are defined, and `NotificationProvider.get_renderer` resolves the renderer from the notification's source, falling back to the provider's default renderer. Keying on source rather than category lets the Seer renderers be registered directly, replacing the nested source check the Slack provider needed to separate `SeerAgentWriteApprovalSlackRenderer` from `SeerSlackRenderer`. Also drops the unused `provider_key` attribute from the `NotificationRenderer` protocol. Co-authored-by: Cursor <cursoragent@cursor.com>
hobzcalvin
force-pushed
the
gp/add_renderer_registry
branch
from
September 17, 2026 23:15
6f19cf1 to
aa068fe
Compare
hobzcalvin
changed the base branch from
gv/add_msteams_issue_renderer
to
master
September 17, 2026 23:15
Comment on lines
+144
to
+145
| renderer = renderer_registry.get(provider_key=cls.key, source=data.source) | ||
| return renderer or cls.default_renderer |
Contributor
There was a problem hiding this comment.
SLACK_STAGING never resolves registered Slack renderers
get_renderer looks up by cls.key, but specialized Slack renderers only register under NotificationProviderKey.SLACK, so SlackStagingNotificationProvider always falls back to the default renderer for issue/metric/seer sources. Register those renderers for SLACK_STAGING as well, or fall back to the SLACK key when cls.key is staging.
Evidence
get_renderercallsrenderer_registry.get(provider_key=cls.key, source=data.source)and returnscls.default_rendererwhen that miss returnsNone.SlackStagingNotificationProvidersetskey = NotificationProviderKey.SLACK_STAGINGand inherits this baseget_renderer.- All Slack specialized renderers register only under
NotificationProviderKey.SLACK(issue, metric_alert, seer, seer_agent_write_approval). - Prior category-based Slack overrides were inherited by staging and returned the specialized renderers; source+provider_key lookup no longer does.
Identified by Warden · sentry-backend-bugs · R99-8QC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #112890 (
gv/add_msteams_issue_renderer).Summary
Each provider was hand-rolling the same category → renderer dispatch table inside its own
get_rendereroverride. This replaces those with aNotificationRendererRegistrykeyed by(provider_key, category).@renderer_registry.register(NotificationProviderKey.SLACK, NotificationCategory.ISSUE).NotificationProvider.get_rendereris now a single base implementation: look the pair up, fall back todefault_renderer. The Slack, Discord, and MS Teams overrides are gone, and with them the function-local renderer imports they needed to dodge the renderer → provider circular import.get_renderertakes only the notification data now; the category comes from the notification's registered template rather than the call site.provider_keyfrom theNotificationRendererprotocol — nothing read it.Renderer modules are imported in
sentry/notifications/apps.pyalongside the providers so registration happens at startup.Notes for review
get_rendereris kept as the override seam rather than having callers hit the registry directly; it is public API used by the notification debugger endpoint and a number of tests, and it is the one place the registry'sAnyis narrowed back to the provider's renderable type.NotificationCategory, which stays coarser than the data.SeerSlackRenderercovers seven sources and still branches internally on the data type. Keying onNotificationSourcewould let those split apart, but that is a larger change and not attempted here.Test plan
pytest tests/sentry/notifications/platform/ tests/sentry/integrations/slack/test_integration.py— 189 passing.Made with Cursor