Break lazy-frame away-links out to _top so they stop Oopsie-ing - #2461
Merged
Conversation
maebeale
marked this pull request as ready for review
August 31, 2026 19:41
maebeale
force-pushed
the
maebeale/revert-bulk-payments-oopsie
branch
from
August 31, 2026 20:18
5b2e52f to
181eb11
Compare
maebeale
changed the title
Collaborator
Author
|
@jmilljr24 as far as i can tell the issue w bulk payments is fixed and there isn't an easy revert option. |
Collaborator
|
This needs a deeper look so please wait. |
An audit of every `*_results` lazy frame turned up 12 away-navigating links across 7 frames that loaded their destination *into* the results frame — the same frame-missing "Oopsie!" the bulk-payments page hit. Fix at the two roots (the credited_author_link byline helper and the shared registrant roster partial) plus the admin activity/visit one-offs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a shared expect_frame_breakout matcher and specs asserting the fixed away-links render with data-turbo-frame="_top" (verified red without the fix). Fill the three frames that had no frame-header request spec at all — payments, users, video_recordings — so a nil-association 500 in those frames is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ywhere person_profile_button / organization_profile_button (and the events compact profile-button partial) always link to a profile page — an away-navigation — so break them out to _top by default instead of relying on each caller to remember. Callers can still override the frame target. Same safe-by-default move as credited_author_link. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ip colors)
The untinted-section assertion checked `not_to include("bg-blue-100")`, but that
class is also a legitimate author-chip color (ChipHelper::CHIP_COLORS), so the
example flipped with record-id parity — green locally, red on CI when the comment
author's id mapped to the blue chip. Assert on `admin-only`, the wash's own marker,
which is unambiguous.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Main's #2453 added these always-away-link helpers (edit_person_path / user_path) but left the _top breakout to each caller. Match person_profile_button and default it in the helper so they can't Oopsie a lazy results frame; callers keep the override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The four card-button helpers (person_profile_button, organization_profile_button, person_edit_button, user_button) now default data-turbo-frame=_top, so the explicit pass at each frame call site is redundant. Remove it; the standalone events profile-button partial keeps its own link_to _top. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maebeale
force-pushed
the
maebeale/revert-bulk-payments-oopsie
branch
from
September 1, 2026 03:24
eab0519 to
36e142f
Compare
jmilljr24
requested changes
Sep 1, 2026
jmilljr24
left a comment
jmilljr24
left a comment
Collaborator
There was a problem hiding this comment.
I get the intent behind this but I really don't think its the smart solution across the board. Rails is convention of configuration and this throws that out the window. _top should be used explicitly when needed. I know you've said turbo isn't your strong suit so we can chat through this if you want but at this point my vote is no.
Collaborator
Author
|
@jmilljr24 cool i'll remove it from the helpers and make this add any missing top's |
Reverses the earlier approach: the card-button/byline helpers (person_profile_button, organization_profile_button, person_edit_button, user_button, credited_author_link) no longer inject data-turbo-frame=_top. Each call site that renders inside a lazy results frame passes _top itself — matching the codebase's existing explicit convention. Helper specs now assert the helpers forward a caller's data-turbo-frame; request specs assert the in-frame call sites (people, professional licenses, workshop-variation and monthly-report bylines) actually emit the breakout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…call Revert the incidental guard-clause refactor of credited_author_link so the only application_helper change is removing data-turbo-frame=_top from the person_edit_button call in credited_author_edit_button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@jmilljr24 i updated this pr to remove the top from any helpers, and added a few missing top's w tests |
jmilljr24
approved these changes
Sep 1, 2026
maebeale
changed the title
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
2 participants
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.
🤖 suggested review level: 3 Read 📖 explicit
_topbreakout on lazy-frame away-links across a few views + guard specsFacilitators clicking a name, org, byline, or icon inside several filterable tables hit the "Oopsie!" box instead of the page they wanted — the same Turbo frame-missing bug the bulk-payments page had, now swept across every lazy
*_resultsframe.What was broken
An audit of all ~35 lazy results frames found 12 away-navigating links in 7 frames that loaded their destination into the results frame (no
_topbreakout) → frame-missing → Oopsie:comments_results,person_comments_results).credited_author_link) on the workshop-variations and monthly-reports tables.Fix — explicit
_topat each call site that needs itEvery away-link that renders inside a lazy frame carries
data: { turbo_frame: "_top" }at the call site, matching the codebase's existing convention. Helpers stay plain — no magic default baked intoperson_profile_button/person_edit_button/user_button/organization_profile_button/credited_author_link; they just forward whateverdata:a caller passes._registrant_rosterpartial so every away-link breaks out uniformly wherever it's reused.Tests
_topbreakout — fast and deterministic (no browser), and they'd have caught all 12 (roster, comment chip, ahoy, payments, people, professional licenses, and the workshop-variation + monthly-report bylines).data-turbo-frameonto the link.payments,users,video_recordings.Follow-up (not in this PR)
The deeper root is that Turbo traps in-frame links by default and the repo relies on remembering
_topat each call site. A safer design would betarget="_top"on the frame itself (safe-by-default), already used byevents/bulk_payments. Converting the rest is a deliberate, repo-wide call (pagination/sort drivers need re-tethering), so it's flagged as a separate follow-up rather than done here.