fix: ensure that footer replacing js is always executed on attach #24
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (1)
293-296: Consider adding error handling for the JavaScript executionWhile the changes look good, it might be beneficial to add some error handling for the JavaScript execution. This can help catch and log any potential issues that might occur during the DOM manipulation.
Consider wrapping the JavaScript execution in a try-catch block and logging any errors. Here's a suggested implementation:
- this.getElement().executeJs("setTimeout(() => this.shadowRoot.querySelector($0).innerHTML = $1)", ".chat-footer", "<slot name='footer'></slot>"); + try { + this.getElement().executeJs("setTimeout(() => this.shadowRoot.querySelector($0).innerHTML = $1)", ".chat-footer", "<slot name='footer'></slot>") + .then(result -> { + // Optional: Log success or perform additional actions + }) + .catch(error -> { + // Log the error or handle it appropriately + System.err.println("Error updating footer: " + error.getMessage()); + }); + } catch (Exception e) { + // Handle any synchronous exceptions + System.err.println("Exception while executing JavaScript: " + e.getMessage()); + }This change would make the code more robust by handling potential errors during the JavaScript execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (1 hunks)
🔇 Additional comments (2)
src/main/java/com/flowingcode/vaadin/addons/chatassistant/ChatAssistant.java (2)
293-296: LGTM! These changes address the custom footer persistence issue.The modifications to
setFooterComponentmethod effectively solve the problem described in issue #22. Here's how:
Moving the null check for
footerComponentensures that any existing footer is always removed before setting a new one. This prevents potential issues with multiple footers.The unconditional execution of the JavaScript to set the footer slot guarantees that the DOM is always updated with the correct footer, regardless of whether a new component is being set or not.
These changes ensure that the custom footer persists correctly when the chat assistant is re-attached, resolving the reported bug.
293-296: Verify usage of setFooterComponent across the codebaseWhile the changes to
setFooterComponentlook good and address the issue, it's important to ensure that this new behavior doesn't have any unintended consequences elsewhere in the codebase.Please run the following script to check for any other usages of
setFooterComponent:This will help us confirm that the new behavior of
setFooterComponentis appropriate for all its usages.
Fixes #22
Summary by CodeRabbit