Summary

Only register action JS models if the action is being rendered.

Review Request #14800 — Created Feb. 5, 2026 and submitted March 9, 2026, 5:04 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change tackles this in two ways:

All rendered action IDs are tracked in the PageState, making it
easy to determine which actions have been placed on the page in some
way.
A new BaseAction.should_register() has been added that returns
whether registration of the model should take place. By default,
this will only return True if the action has been rendered (which
has the benefit of inheriting the result of should_render()).
Subclasses can override this to always register, or to limit to
certain URLs or other criteria (but should always respect
should_render()).

With this, actions are now only registered on the pages in which they're
used, by default, without limiting more advanced use cases.

Testing Done

Unit tests pass.

Verified that admin actions weren't showing up on other pages, but were
showing up in the admin UI.

Commits

Summary	ID
Only register action JS models if the action is being rendered. With the new action framework changes that separate out action model registration from rendering, we were now registering every single action on every page, whether or not they were being used. This meant that any user would end up seeing actions for other pages, including the admin page, which certainly wouldn't be correct. While there are cases where an action may be worth registering when not rendered, in most cases we want to avoid this behavior. This change tackles this in two ways: 1. All rendered action IDs are tracked in the `PageState`, making it easy to determine which actions have been placed on the page in some way. 2. A new `BaseAction.should_register()` has been added that returns whether registration of the model should take place. By default, this will only return `True` if the action has been rendered (which has the benefit of inheriting the result of `should_render()`). Subclasses can override this to always register, or to limit to certain URLs or other criteria (but should always respect `should_render()`). With this, actions are now only registered on the pages in which they're used, by default, without limiting more advanced use cases.	118414adf2ae171e631d0b8d5053ea89b41f83e2

Summary

Only register action JS models if the action is being rendered.

With the new action framework changes that separate out action model registration from rendering, we were now registering every single action on every page, whether or not they were being used. This meant that any user would end up seeing actions for other pages, including the admin page, which certainly wouldn't be correct. While there are cases where an action may be worth registering when not rendered, in most cases we want to avoid this behavior. This change tackles this in two ways: 1. All rendered action IDs are tracked in the `PageState`, making it easy to determine which actions have been placed on the page in some way. 2. A new `BaseAction.should_register()` has been added that returns whether registration of the model should take place. By default, this will only return `True` if the action has been rendered (which has the benefit of inheriting the result of `should_render()`). Subclasses can override this to always register, or to limit to certain URLs or other criteria (but should always respect `should_render()`). With this, actions are now only registered on the pages in which they're used, by default, without limiting more advanced use cases.

118414adf2ae171e631d0b8d5053ea89b41f83e2

Issues

Description	From	Last Updated
Can you add a test for an action that overrides should_register to always register the action?	david	March 5, 2026, 2:57 p.m.
do not use bare 'except' Column: 9 Error code: E722	reviewbot	Feb. 5, 2026, 5:08 p.m.
Perhaps this instead? You use setdefault just below in templatetags/actions.py rendered_action_ids: set[str] = page_state.extra_data.setdefault( 'rb-actions-rendered', set())	david	March 3, 2026, 12:35 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/actions/renderers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
do not use bare 'except'

Column: 9
Error code: E722
```

Change Summary:

Fixed a bare except.

Commits:

	Summary	ID
	Only register action JS models if the action is being rendered. With the new action framework changes that separate out action model registration from rendering, we were now registering every single action on every page, whether or not they were being used. This meant that any user would end up seeing actions for other pages, including the admin page, which certainly wouldn't be correct. While there are cases where an action may be worth registering when not rendered, in most cases we want to avoid this behavior. This change tackles this in two ways: 1. All rendered action IDs are tracked in the `PageState`, making it easy to determine which actions have been placed on the page in some way. 2. A new `BaseAction.should_register()` has been added that returns whether registration of the model should take place. By default, this will only return `True` if the action has been rendered (which has the benefit of inheriting the result of `should_render()`). Subclasses can override this to always register, or to limit to certain URLs or other criteria (but should always respect `should_render()`). With this, actions are now only registered on the pages in which they're used, by default, without limiting more advanced use cases.	335dc50fa6f11ccd1c320574f04c6d28acbe4954
	Only register action JS models if the action is being rendered. With the new action framework changes that separate out action model registration from rendering, we were now registering every single action on every page, whether or not they were being used. This meant that any user would end up seeing actions for other pages, including the admin page, which certainly wouldn't be correct. While there are cases where an action may be worth registering when not rendered, in most cases we want to avoid this behavior. This change tackles this in two ways: 1. All rendered action IDs are tracked in the `PageState`, making it easy to determine which actions have been placed on the page in some way. 2. A new `BaseAction.should_register()` has been added that returns whether registration of the model should take place. By default, this will only return `True` if the action has been rendered (which has the benefit of inheriting the result of `should_render()`). Subclasses can override this to always register, or to limit to certain URLs or other criteria (but should always respect `should_render()`). With this, actions are now only registered on the pages in which they're used, by default, without limiting more advanced use cases.	118414adf2ae171e631d0b8d5053ea89b41f83e2

Diff:

Revision 2 (+306 -2)

Show changes

	reviewboard/actions/base.py
	reviewboard/actions/renderers.py
	reviewboard/actions/templatetags/actions.py
	reviewboard/actions/tests/test_base_action.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

Can you add a test for an action that overrides should_register to always register the action?

chipx86 March 3, 2026, 12:42 p.m.

I don't really know what this would do. Wouldn't make sense in the Action tests, since it'd just be testing its own overridden function. The only place this is called and used is in templatetags, and we don't actually have any tests for any of that. That's a batch of work that'd need to be done.

reviewboard/actions/renderers.py (Diff revision 2)

The issue has been dropped. Show all issues

Perhaps this instead? You use setdefault just below in templatetags/actions.py

rendered_action_ids: set[str] = page_state.extra_data.setdefault(
  'rb-actions-rendered', set())

chipx86 March 3, 2026, 12:42 p.m.

The typical page will have dozens of actions, so we'd only need to construct the set() once. The try/except should be faster and less wasteful.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.1.x (b7749c0)