Skip to content

Conversation

@ChanminK
Copy link

ONLY ON THE GODOT EXTENSION PAGE

Comment on lines +6 to +16
connect() {
// Banner in case we ever want to use it
// if (this.hasBannerTarget) {
// this.bannerTarget.classList.remove("hidden")
// }

// side-wide noti scooter use it if ya want
// if (!this.modalDismissed()) {
// this.openModal()
// }
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The sitewide_notice_controller.js has an empty connect() method, which prevents the sitewide notice modal from ever being displayed, rendering the feature non-functional.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The connect() method in sitewide_notice_controller.js is empty, with its logic commented out. As this method is the entry point for Stimulus controllers, the sitewide notice modal and banner will never be displayed to users. The controller is correctly attached to the DOM via the _sitewide_notice.html.erb partial on every page, but because the initialization logic in connect() is disabled, the modal remains hidden indefinitely. This renders the entire sitewide notice feature non-functional, despite all the supporting HTML and controller methods being in place.

💡 Suggested Fix

To re-enable the sitewide notice feature, uncomment the logic within the connect() method in sitewide_notice_controller.js. This will allow the controller to check if the modal has been dismissed and call openModal() on page load. If the intention was to permanently remove the feature, the controller and the corresponding partial rendering in the application layout should be removed entirely.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: app/javascript/controllers/sitewide_notice_controller.js#L6-L16

Potential issue: The `connect()` method in `sitewide_notice_controller.js` is empty,
with its logic commented out. As this method is the entry point for Stimulus
controllers, the sitewide notice modal and banner will never be displayed to users. The
controller is correctly attached to the DOM via the `_sitewide_notice.html.erb` partial
on every page, but because the initialization logic in `connect()` is disabled, the
modal remains hidden indefinitely. This renders the entire sitewide notice feature
non-functional, despite all the supporting HTML and controller methods being in place.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7509002

@JeffreyWangDev
Copy link
Contributor

It might be helpful to instead hardcode a warning, add a warning feature in the admin UI where you can set/remove a warning for specific pages/all pages, instead of hardcoding it.

@JeffreyWangDev
Copy link
Contributor

@scooterthedev
Copy link
Member

I'll take over this PR in the coming days. My idea to add onto @JeffreyWangDev's idea goes something along the lines of this:

  1. We have a page on the admin settings that are able to select/deselect which popups they want to show. For example, the Godot extension here.
  2. If we detect that they have used godot in the past, we will check there last 30 day's of hb's for godot, and:
    a. if it's found, check the user-agent and check if there using the old one or new one
    b. look back up to 60 days
  3. If old godot extension usage is found, we will display a popup like @ChanminK added to their homage forcing them to update. On top of this, we will show this banner for the extension page specifically always just in case.
  4. If we want to go further, we can add a "Check Validity" button on the popup which will check the last day's hb's and look for the new user-agent to make sure it's installed.

The only current issue with this is querying the db for the past ~60 days even for 2% of hackatime users is a lot. What we could do is see when there projects made in godot happened, and check +- 1 week of hb's from that date.

The good thing about this way of approaching it, is that it will be easily adjustable for other extensions when this happens to those too.

@JeffreyWangDev
Copy link
Contributor

Instead of checking the past 60 days, why not do a query like select * from heartbeats where language = godot limit 100, then check if the latest version is correct (have the admin page put in a regex to match). This should run better than pulling 60 days, cause if they are coding a lot, that's a lot of heartbeats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants