feat: display changelog modal on new version to improve awareness #2403
Conversation
WalkthroughThe NewsButton component is refactored to extract modal-opening logic into a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NewsButton
participant Storage as localStorage
participant Modal as news-modal
alt Scenario 1: Version Check
NewsButton->>NewsButton: checkForNewVersion()
NewsButton->>Storage: persist current version
Note over NewsButton: isActive = true<br/>(new version found)
NewsButton->>NewsButton: schedule openNewsModel() in 500ms
end
alt Scenario 2: Button Click
User->>NewsButton: click
NewsButton->>NewsButton: handleClick()
NewsButton->>Storage: persist version
NewsButton->>NewsButton: isActive = false
NewsButton->>NewsButton: openNewsModel() immediately
end
NewsButton->>Modal: query for element
alt Modal Found
NewsButton->>Modal: open()
else Modal Not Found
NewsButton->>NewsButton: log "no newsModal"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/client/components/NewsButton.ts (2)
40-47: Method name has a typo (optional fix).The method is named
openNewsModel()but should beopenNewsModal()to match the element name "news-modal" and class "NewsModal". The typo is consistent throughout, so it doesn't break functionality, but fixing it would improve clarity.Rename the method:
- private openNewsModel() { + private openNewsModal() { const newsModal = document.querySelector("news-modal") as NewsModal; if (newsModal) { newsModal.open(); } else { console.log("no newsModal"); } }And update the call sites at lines 24 and 37:
- this.openNewsModel(); + this.openNewsModal();
44-46: Consider using console.warn for missing modal (optional).If the news modal element is not found in the DOM, this is likely an error condition. Using
console.warnorconsole.errorwould make it more visible in production logs.} else { - console.log("no newsModal"); + console.warn("news-modal element not found in DOM"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/components/NewsButton.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
🔇 Additional comments (1)
src/client/components/NewsButton.ts (1)
37-38: Good refactoring for code reuse.Extracting the modal-opening logic into a dedicated method improves maintainability.
| if (this.isActive) { | ||
| setTimeout(() => { | ||
| this.openNewsModel(); | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
Race condition and missing cleanup for the timeout.
Two issues here:
- If the user clicks the button before 500ms completes,
openNewsModel()will be called twice (once from timeout, once from click handler). - If the component disconnects before 500ms, the callback still executes. This could attempt to query the DOM when the component is gone.
Consider storing the timeout reference and clearing it on disconnect or click:
+ private autoOpenTimeout?: number;
+
connectedCallback() {
super.connectedCallback();
this.checkForNewVersion();
}
+
+ disconnectedCallback() {
+ super.disconnectedCallback();
+ if (this.autoOpenTimeout !== undefined) {
+ clearTimeout(this.autoOpenTimeout);
+ }
+ }
private checkForNewVersion() {
try {
const lastSeenVersion = localStorage.getItem("version");
this.isActive = lastSeenVersion !== version;
if (this.isActive) {
- setTimeout(() => {
+ this.autoOpenTimeout = setTimeout(() => {
this.openNewsModel();
}, 500);
}
} catch (error) {
// Fallback to NOT showing notification if localStorage fails
this.isActive = false;
}
localStorage.setItem("version", version);
}
private handleClick() {
+ if (this.autoOpenTimeout !== undefined) {
+ clearTimeout(this.autoOpenTimeout);
+ this.autoOpenTimeout = undefined;
+ }
localStorage.setItem("version", version);
this.isActive = false;
this.openNewsModel();
}🤖 Prompt for AI Agents
In src/client/components/NewsButton.ts around lines 22-26, the setTimeout
creates a race and cleanup issue: store the returned timeout ID when scheduling
openNewsModel(), clear that timeout when the button is clicked (to prevent
duplicate invocation) and also clear it in the component's disconnect/teardown
lifecycle method; additionally, before calling openNewsModel() inside the
timeout callback, verify the component is still connected/active to avoid DOM
access after unmount. Ensure the timeout ID is a property on the component
instance so both click handler and disconnect can access and clear it.
| // Fallback to NOT showing notification if localStorage fails | ||
| this.isActive = false; | ||
| } | ||
| localStorage.setItem("version", version); |
There was a problem hiding this comment.
Version marked as seen before the modal actually opens.
The current flow marks the version as seen immediately, even though the modal is scheduled to open 500ms later. If the user closes the tab before the timeout completes, they'll never see the changelog, but the version will still be marked as seen.
Move the version persistence to happen only when the modal actually opens:
private checkForNewVersion() {
try {
const lastSeenVersion = localStorage.getItem("version");
this.isActive = lastSeenVersion !== version;
if (this.isActive) {
setTimeout(() => {
this.openNewsModel();
+ localStorage.setItem("version", version);
}, 500);
}
} catch (error) {
// Fallback to NOT showing notification if localStorage fails
this.isActive = false;
}
- localStorage.setItem("version", version);
}Or alternatively, only persist when the user interacts (clicks the button), which is already handled in handleClick.
🤖 Prompt for AI Agents
In src/client/components/NewsButton.ts around line 31, the code persists the
current version to localStorage immediately (localStorage.setItem("version",
version)) even though the changelog modal is opened after a 500ms timeout; move
that persistence so it only runs when the modal actually opens or when the user
explicitly interacts. Remove the immediate localStorage.setItem call and instead
set the version in localStorage inside the setTimeout callback (where you set
showModal = true / open the modal) or inside the modal-open handler (or rely on
the existing handleClick persistence) so the version is only marked as seen when
the modal is displayed or the user clicks.
|
Althought RabbitAI is correct, it's very unlikely given the very short delay, and I think the add code is not worth the complexity. But I am happy to add if it's deemed necessary |
3 participants
Description:
Following discussion on discord, some people (including me) thought it would be nice to show the changelog whenever there is a new version, as people seemed not to be aware of the gameplay change
Kapture.2025-11-06.at.21.32.24.mp4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sorikairo