Render fulfillment container only when applicable by jazairi · Pull Request #402 · MITLibraries/timdex-ui

jazairi · 2026-06-04T16:27:23Z

Why these changes are being introduced:

The fulfillment link container currently renders
as an empty div when no links are available. This
pollutes the DOM and affects spacing.

Relevant ticket(s):

USE-611

How this addresses that need:

Primo link removal moves further up in the control
flow, because that check is not applicable if the
parent element is gone.

Side effects of this change:

None.

Developer

Accessibility

ANDI or WAVE has been run in accordance to our guide.
This PR contains no changes to the view layer.
New issues flagged by ANDI or WAVE have been resolved.
New issues flagged by ANDI or WAVE have been ticketed (link in the Pull Request details above).
No new accessibility issues have been flagged.

New ENV

All new ENV is documented in README.
All new ENV has been added to Heroku Pipeline, Staging and Prod.
ENV has not changed.

Approval beyond code review

UXWS/stakeholder approval has been confirmed.
UXWS/stakeholder review will be completed retroactively.
UXWS/stakeholder review is not needed.

Additional context needed to review

E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.

Code Reviewer

Code

I have confirmed that the code works as intended.
Any CodeClimate issues have been fixed or confirmed as
added technical debt.

Documentation

The commit message is clear and follows our guidelines
(not just this pull request message).
The documentation has been updated or is unnecessary.
New dependencies are appropriate or there were no changes.

Testing

There are appropriate tests covering any new functionality.
No additional test coverage is required.

qltysh · 2026-06-04T16:28:29Z

❌ 1 blocking issue (2 total)

Tool	Category	Rule	Count
rubocop	Style	Line is too long. [123/120]	1	❌
qlty	Structure	Function with high complexity (count = 9): load	1

coveralls · 2026-06-04T16:29:43Z

Coverage Report for CI Build 27033899446

Coverage at 98.333% (no base build to compare)

Details

Coverage remained the same as the base build.
Patch coverage: 12 of 12 lines across 1 file are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	1440
Covered Lines:	1416
Line Coverage:	98.33%
Coverage Strength:	74.58 hits per line

💛 - Coveralls

Copilot

Pull request overview

This PR updates the Stimulus content-loader controller to avoid leaving empty fulfillment/link containers in the DOM when the fetched HTML response contains no meaningful content, reducing DOM pollution and preventing unintended spacing/layout artifacts in results.

Changes:

If the fetched HTML is empty/whitespace-only, remove the loader element instead of replacing it with empty output.
Preserve existing behavior that removes .primo-link elements when a .libkey-link is present.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Why these changes are being introduced: The fulfillment link container currently renders as an empty div when no links are available. This pollutes the DOM and affects spacing. Relevant ticket(s): - [USE-611](https://mitlibraries.atlassian.net/browse/USE-611) How this addresses that need: This removes the parent div if no fulfillment links are present. Side effects of this change: Primo link removal moves further up in the control flow, because that check is not applicable if the parent element is gone.

qltysh · 2026-06-04T17:41:36Z

+          // Remove result-get container (no fulfillment links)
+          parentElement.remove();
        }
      })


Function with high complexity (count = 7): load [qlty:function-complexity]

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

jazairi · 2026-06-04T19:57:23Z

+  stripHtmlComments(input) {
+    let previous
+    let output = input
+
+    do {
+      previous = output
+      output = output.replace(/<!--[\s\S]*?-->/g, '')
+    } while (output !== previous)
+
+    return output
+  }


The change Copilot is suggesting is what I originally had. It would override the existing code in this helper method, which was suggested in a prior Copilot review. 🙃

djanelle-mit

Final edit: I still see the result-get divs on the PR build. There was a couple cases where they were gone but I can't reproduce them now.

JPrevost

This doesn't seem to work. I still see the empty divs in Dave's example from the ticket. I suspect we are injecting them for records we never do lookups so this swap logic never comes into play. The issue may be we need to conditionally inject the divs to begin with (and still clean them up if they result in empty divs)?

It's quite early though so I'm marking this as comment and not request changes because I may not be understanding what is going on :)

JPrevost · 2026-06-05T12:13:25Z

+
+    do {
+      previous = output
+      output = output.replace(/<!--[\s\S]*?-->/g, '')


Are there actually HTML comments we need to strip? I see whitespace, but that's handled by the trim without this new method.

The deeper I go into this, the more confused I get. Yesterday I was seeing comments. Now I'm just seeing whitespace.

Ah, I see. This config setting is the culprit. I was looking at the changes in my local, so I was solving for a problem that doesn't actually exist in prod environments.

The good news is, trimming whitespace makes this much simpler.

…t fire

qltysh · 2026-06-05T18:48:31Z

+    end
+  end
+
+  test 'result_get? returns true when result has both full record and PDF links even if record_link feature is disabled' do


Line is too long. [123/120] [rubocop:Layout/LineLength]

JPrevost · 2026-06-09T19:36:31Z

I'm going to merge to unblock @djanelle-mit

jazairi requested a review from Copilot June 4, 2026 16:29

Copilot started reviewing on behalf of jazairi June 4, 2026 16:29 View session

mitlib temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 16:29 Inactive

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread app/javascript/controllers/content_loader_controller.js Outdated

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 16:34 Inactive

jazairi requested a review from Copilot June 4, 2026 16:34

Copilot started reviewing on behalf of jazairi June 4, 2026 16:34 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

jazairi force-pushed the use-611 branch from 9fc3771 to 9771b79 Compare June 4, 2026 17:40

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 17:40 Inactive

github-advanced-security AI found potential problems Jun 4, 2026

View reviewed changes

Comment thread app/javascript/controllers/content_loader_controller.js Fixed

qltysh Bot reviewed Jun 4, 2026

View reviewed changes

jazairi force-pushed the use-611 branch from 9771b79 to 8654cb1 Compare June 4, 2026 17:41

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 17:41 Inactive

Address security issue identified by CodeQL

97e04c5

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 17:50 Inactive

jazairi requested a review from Copilot June 4, 2026 19:02

Copilot started reviewing on behalf of jazairi June 4, 2026 19:03 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread app/javascript/controllers/content_loader_controller.js

Comment thread app/javascript/controllers/content_loader_controller.js Outdated

Remove semicolons

0fb3f13

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 4, 2026 19:51 Inactive

jazairi requested a review from Copilot June 4, 2026 19:51

Copilot started reviewing on behalf of jazairi June 4, 2026 19:51 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

jazairi requested review from JPrevost, djanelle-mit and matt-bernhardt June 4, 2026 19:58

Safely remove container

c8f65a3

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 00:02 Inactive

djanelle-mit reviewed Jun 5, 2026

View reviewed changes

JPrevost reviewed Jun 5, 2026

View reviewed changes

Disable annotations in development and simplify cleanup logic

57a4d80

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 16:05 Inactive

jazairi force-pushed the use-611 branch from 5d96791 to 57a4d80 Compare June 5, 2026 16:05

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 16:05 Inactive

jazairi requested review from JPrevost and djanelle-mit June 5, 2026 16:20

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 17:33 Inactive

qltysh Bot reviewed Jun 5, 2026

View reviewed changes

Comment thread app/helpers/results_helper.rb Outdated

Add helper method to handle cases where stimulus controller JS doesn'…

0c2f369

…t fire

jazairi force-pushed the use-611 branch from 0265e9d to 0c2f369 Compare June 5, 2026 18:46

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 18:47 Inactive

qltysh Bot reviewed Jun 5, 2026

View reviewed changes

rubocop

67f1232

jazairi temporarily deployed to timdex-ui-pi-use-611-oftuzrder June 5, 2026 18:51 Inactive

JPrevost self-assigned this Jun 8, 2026

JPrevost approved these changes Jun 8, 2026

View reviewed changes

JPrevost merged commit cfc3554 into main Jun 9, 2026
6 checks passed

JPrevost deleted the use-611 branch June 9, 2026 19:36

Uh oh!

Conversation

jazairi commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why these changes are being introduced:

Relevant ticket(s):

How this addresses that need:

Side effects of this change:

Developer

Accessibility

New ENV

Approval beyond code review

Additional context needed to review

Code Reviewer

Code

Documentation

Testing

Uh oh!

qltysh Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 blocking issue (2 total)

Uh oh!

coveralls commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 27033899446

Coverage at 98.333% (no base build to compare)

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

qltysh Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

jazairi Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

djanelle-mit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JPrevost left a comment

Choose a reason for hiding this comment

Uh oh!

JPrevost Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

jazairi Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

jazairi Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qltysh Bot Jun 5, 2026

Choose a reason for hiding this comment

jazairi commented Jun 4, 2026 •

edited

Loading

qltysh Bot commented Jun 4, 2026 •

edited

Loading

coveralls commented Jun 4, 2026 •

edited

Loading

djanelle-mit left a comment •

edited

Loading