Summary

Set renderedInline on the AbstractReviewable model.

Review Request #14627 — Created Oct. 2, 2025 and updated Oct. 7, 2025, 8:25 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.1.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

We have a renderedInline attribute on both the AbstractReviewable model
and AbstractReviewableView JS objects for our review UIs. This
attribute says whether the review UI is being rendered inline or not. We
were properly setting this on AbstractReviewableView, however we
weren't setting it on AbstractReviewable. This change fixes that.

Testing Done

Confirmed that the attribute is set on the AbstractReviewable,
viewed an inline review UI and a standalone one.

Commits

Summary	ID
Set renderedInline on the AbstractReviewable model. We have a `renderedInline` attribute on both the `AbstractReviewable` model and `AbstractReviewableView` JS objects for our review UIs. This attribute says whether the review UI is being rendered inline or not. We were properly setting this on `AbstractReviewableView`, however we weren't setting it on `AbstractReviewable`. This change fixes that.	467fac07bcea53144fea3c6f8516a61defdbe4cf

Summary

Set renderedInline on the AbstractReviewable model.

We have a `renderedInline` attribute on both the `AbstractReviewable` model and `AbstractReviewableView` JS objects for our review UIs. This attribute says whether the review UI is being rendered inline or not. We were properly setting this on `AbstractReviewableView`, however we weren't setting it on `AbstractReviewable`. This change fixes that.

467fac07bcea53144fea3c6f8516a61defdbe4cf

Issues

Description	From	Last Updated
I don't think we need a version changed comment here. This is a bug fix rather than an API change.	david	Oct. 5, 2025, 11:34 p.m.
Seems like a better fix would be to include this in the data returned from ReviewUI.get_js_model_data	david	Oct. 3, 2025, 7:47 a.m.

flake8 passed.

JSHint passed.

Fix it!

reviewboard/static/rb/js/reviews/models/abstractReviewableModel.ts (Diff revision 1)

The issue has been resolved. Show all issues

I don't think we need a version changed comment here. This is a bug fix rather than an API change.

reviewboard/templates/reviews/ui/default.html (Diff revision 1)

An issue was opened. Show all issues

Seems like a better fix would be to include this in the data returned from ReviewUI.get_js_model_data

maubin Oct. 6, 2025, 12:04 a.m.

I prefered this fix because we have the inline information readily available in ReviewUI.build_render_context, and we do the same thing when initializing the JS view class a few lines above this.
If I wanted to include it in ReviewUI.get_js_model_data, I'd have to pull the inline information out of the request again in there, or set a ReviewUI._inline variable when we pull the info out of the request in  ReviewUI.render_to_response. I suppose it would be a good idea to have a ReviewUI._inline variable, so that any methods on the review UI can easily access the inline information. Thoughts?

david Oct. 7, 2025, 8:25 a.m.

It's probably best to have that state stored, although it does feel slightly icky because whether we're rendering inline is theoretically only known for the duration of the render_to_* method. In practice, a file attachment is either a regular attachment or it's connected to a diff, so there's no actual case where we'd have that state change, even if the ReviewUI lives across multiple requests.