Repository navigation
fix: stop mutating the shared PagePresenter when wrapping the index block - #3
Merged
Merged
Conversation
…lock IndexAsTableExtension#build wrote its footer wrapper back into the PagePresenter it was handed. ActiveAdmin builds one PagePresenter per `index do ... end` when it loads the resource, and get_page_presenter returns that same object on every render, so each render wrapped the previous wrapper: retained closures and block call depth grew by one per request until rendering an index raised SystemStackError (~6k renders in a long-lived process). The `footer_proc && page_presenter.block` guard did not stop it, because an already-wrapped block is still truthy. Build the wrapper as before, but render against a fresh PagePresenter sharing the original options hash. ActiveAdmin reads the presenter only through `[]` and `#block` on both 3.5 and 4.0, so rendering is unchanged. Prepending IndexAsTable#table_for to inject the option instead was rejected: ActiveAdmin 4 has no such method — its build calls insert_tag directly — and this gem supports both majors. The new spec renders the same index repeatedly and asserts PagePresenter#block is never replaced. Verified failing before this change and passing after, against activeadmin 3.5.1 and 4.0.0.beta23.
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix preserves rendering behavior, adds targeted regression coverage, and has no identified blocking issues.
No open issues 🎉
What changed in this PR
Prevents repeated index renders from mutating ActiveAdmin’s shared page presenter and accumulating wrappers that can cause stack overflow.
Changes:
- Renders with a fresh presenter while preserving options and footer behavior.
- Adds regression coverage for presenter block identity and repeated footer rendering.
| File | Description |
|---|---|
| spec/features/index_presenter_mutation_spec.rb | Tests presenter stability and footer rendering across repeated requests. |
| lib/activeadmin_table_footer/index_as_table_extension.rb | Wraps the index block in a fresh presenter instead of mutating shared state. |
Review effort: Balanced
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.
Problem
IndexAsTableExtension#buildwrote its footer wrapper back into thePagePresenterit was handed:ActiveAdmin builds one
PagePresenterperindex do ... endwhen it loads the resource, memoized inResource::PagePresenters#page_presenters, andget_page_presenterreturns that same object on every render. So each render wrapped the previous wrapper: retained closures and block call depth grew by one per request, until rendering an index raisedSystemStackError(~6k renders in a long-lived process). Thefooter_proc && page_presenter.blockguard did not stop it, because an already-wrapped block is still truthy.Fix
Build the wrapper as before, but render against a fresh
PagePresenterthat shares the original options hash, leaving the stored presenter untouched. ActiveAdmin reads the presenter only through[]and#blockon both 3.5 and 4.0, so rendering is unchanged.Prepending
IndexAsTable#table_forto inject the option instead was rejected: ActiveAdmin 4 has no such method — itsbuildcallsinsert_tagdirectly — and this gem supports both majors.Regression cover
spec/features/index_presenter_mutation_spec.rbrenders the same index repeatedly and assertsPagePresenter#blockis never replaced.Before the fix it fails with the user block swapped for the lambda this gem installs:
After the fix:
25 examples, 0 failureson both the default bundle (activeadmin 3.5.1) andgemfiles/activeadmin_4.0.gemfile(activeadmin 4.0.0.beta23).No version bump here — release separately, following the existing
fix/*thenrelease/*flow.