feat: add support for named terminal add-ons #99
Conversation
WalkthroughThis update introduces a new server-side add-on system for the terminal component. It adds abstract base classes for server-side and client-aware add-ons, updates the terminal to support registration and retrieval of add-ons, and modifies an existing add-on to use the new structure. A corresponding property is also added on the client-side terminal element. Changes
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (4)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts (1)
165-165: Consider using a more specific type instead ofObject.The
addonsproperty is typed asObjectwhich is a very generic type. Consider using a more specific interface that describes the structure of the add-ons that can be registered.- addons : Object = {}; + addons : Record<string, ITerminalAddon> = {};The static analysis tool flagged the use of
Objectas a banned type, recommending to explicitly define the object shape for better type safety.🧰 Tools
🪛 Biome (1.9.4)
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java (1)
309-311: Generic signature can be simplified for type-safety
<T extends TerminalAddon> T getAddon(Class<? extends T> clazz)works but the wildcard is unnecessary and forces callers to up-cast. Prefer:-public <T extends TerminalAddon> T getAddon(Class<? extends T> clazz) +public <T extends TerminalAddon> T getAddon(Class<T> clazz)src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java (2)
3-3: Remove unused import
import com.vaadin.flow.dom.Element;is never referenced.
4-4: Avoid relying on internal Vaadin API
com.vaadin.flow.internal.JsonCodecis an internal class and may change without notice. Prefer the publicJsonSerializerutilities or plainJson.create...APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java(3 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java(4 hunks)src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java (1)
src/test/java/com/flowingcode/vaadin/addons/xterm/integration/XTermElement.java (1)
Element(32-101)
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🪛 GitHub Actions: Java CI with Maven
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java
[error] 322-322: Compilation error: illegal start of type at line 322, column 4.
🔇 Additional comments (7)
pom.xml (1)
7-7: Version bump is appropriate for a feature addition.The project version has been increased from 3.1.1-SNAPSHOT to 3.2.0-SNAPSHOT, which follows semantic versioning standards for adding new functionality.
src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java (2)
46-48: LGTM! PreserveStateAddon now integrates with the new add-on system.The class now extends
TerminalAddonwhile maintaining its existing interfaces, which properly integrates it with the new add-on management framework.
85-85: Correctly initializes the parent class.The
super(xterm)call passes the terminal instance to theTerminalAddonconstructor, ensuring proper registration with the terminal's add-on management system.src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java (2)
5-15: Well-documented class purpose and design.The Javadoc clearly explains the purpose of this abstract base class and its role in the add-on architecture. The documentation provides sufficient context for developers who will extend this class.
16-28:✅ Verification successful
Sound implementation of the base add-on class.
The class properly implements the foundational aspects of the add-on system:
- Takes an
XTermBaseinstance in the constructor- Validates the instance is not null using
Objects.requireNonNull- Registers itself with the terminal via
xterm.registerServerSideAddon(this)This design aligns with the PR objectives of enabling server-side retrieval of add-on instances.
🏁 Script executed:
#!/bin/bash # Verify that XTermBase implements the registerServerSideAddon method rg "void registerServerSideAddon\(" --type javaLength of output: 188
Approve: TerminalAddon constructor and registration verified
The
TerminalAddonconstructor correctly null-checks the providedXTermBaseinstance and callsregisterServerSideAddon(this)as intended.• Verified that
XTermBase.javadefinesfinal <T extends TerminalAddon> void registerServerSideAddon(T addon)ensuring the add-on registration is implemented.
No further changes required.
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java (1)
73-73: Consider thread-safe collection if add-ons may be accessed concurrently
ArrayListis not thread-safe. If add-ons can be registered/retrieved from different threads (e.g., UI access vs. background tasks), useCopyOnWriteArrayListor synchronize access.src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java (1)
75-84: Placeholder replacement may generate invalid JS
expression.replaceAll("\\$(\\d+)", "\\$1[$1]")turns$1into$1[1], e.g."$1"→"$1[1]", which is unlikely the intended transformation and produces a broken script. Re-evaluate the algorithm or add tests demonstrating correct behaviour.
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java(3 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java(4 hunks)src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java
- src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java
- src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java
🧰 Additional context used
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts (1)
165-165: Use a more specific type and add documentation for the addons property.The
Objecttype is too broad and doesn't provide enough type safety. Consider using a more specific interface or type that describes the structure of the add-ons object. Additionally, adding JSDoc documentation would help other developers understand the purpose and usage of this property.- addons : Object = {}; + /** + * Storage for client-side terminal add-ons that can be accessed by the server-side Java code. + * Used by ClientTerminalAddon to invoke JavaScript on client terminal add-ons. + * @example { webLinks: WebLinksAddon, fitAddon: FitAddon } + */ + addons : Record<string, ITerminalAddon> = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🧹 Nitpick comments (3)
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java (1)
5-5: Copyright year should match current release timelineThe copyright year has been updated to 2025, which is several years in the future from the current date. Consider adjusting it to reflect the actual release timeline.
- * Copyright (C) 2020 - 2025 Flowing Code + * Copyright (C) 2020 - 2024 Flowing Codesrc/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java (2)
60-62: Consider more descriptive exception message for getName() validationThe current implementation throws an
IllegalStateExceptionwithout a descriptive message whengetName()returns null. Adding a clear error message would help developers understand the issue.- if (getName() == null) { - throw new IllegalStateException(); - } + if (getName() == null) { + throw new IllegalStateException("getName() must return a non-null value"); + }
102-102: Add comment explaining the parameter replacement patternThe regex replacement on expression parameters is not immediately obvious. A code comment would help future maintainers understand this transformation.
+ // Convert $n placeholder syntax to $n[$n] to make it work with the apply() call and arguments array expression = expression.replaceAll("\\$(\\d+)", "\\$1[$1]");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java(3 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java(4 hunks)src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🔇 Additional comments (8)
pom.xml (1)
7-7: Version bump aligns with feature additionThe version update from 3.1.1-SNAPSHOT to 3.2.0-SNAPSHOT follows semantic versioning principles, correctly reflecting the addition of a new feature (server-side add-on management) without breaking existing functionality.
src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java (2)
46-48: LGTM: Proper integration with the new add-on architectureThe class now extends
TerminalAddonwhile maintaining its original interfaces, properly integrating it with the new server-side add-on framework.
85-85: LGTM: Appropriate super callThe constructor correctly calls
super(xterm)to initialize the baseTerminalAddonclass, which handles the registration of this add-on with the terminal instance.src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java (1)
25-52: Well-structured base class for the add-on systemThe
TerminalAddonabstract class provides a solid foundation for the server-side add-on framework. The implementation is clean, with:
- Comprehensive JavaDoc explaining the purpose and usage
- Proper null checks in the constructor
- Automatic registration with the terminal instance
- Implementation of Serializable for session state management
This abstraction will make it easier to create consistent add-ons that integrate well with the terminal component.
src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java (3)
73-73: LGTM: Good implementation of add-ons collectionThe implementation of the add-ons list as a private final field with proper initialization is a good practice.
288-311: LGTM: Well-structured method with comprehensive JavadocThe
getAddonmethod is well-designed with clear documentation, proper parameter validation, and effective use of Java streams to find the appropriate add-on instance.
313-327: LGTM: Good implementation of add-on registrationThe
registerServerSideAddonmethod correctly validates inputs, prevents duplicate registrations, and is properly marked asfinalsince it's called from constructors.src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java (1)
1-107: LGTM: Well-designed abstract class for client-side terminal add-onsThe
ClientTerminalAddonclass provides a solid foundation for implementing terminal add-ons with client-side JavaScript interaction. The implementation includes proper constructor validation, clear documentation, and a useful utility method for JavaScript execution.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts (1)
165-165: Add documentation for the addons property.The newly added property
addonsseems to be a key part of your new server-side add-on retrieval mechanism, as mentioned in the PR objectives. While I understand you prefer usingObjecttype (as noted in previous discussions), adding some documentation would help other developers understand the purpose of this property.- addons : Object = {}; + /** + * Storage for client-side terminal add-ons that can be accessed by the server-side Java code. + * Used by ClientTerminalAddon to invoke JavaScript on client terminal add-ons. + */ + addons : Object = {};🧰 Tools
🪛 Biome (1.9.4)
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java(3 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java(4 hunks)src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/com/flowingcode/vaadin/addons/xterm/PreserveStateAddon.java
- src/main/java/com/flowingcode/vaadin/addons/xterm/TerminalAddon.java
- src/main/java/com/flowingcode/vaadin/addons/xterm/ClientTerminalAddon.java
- src/main/java/com/flowingcode/vaadin/addons/xterm/XTermBase.java
🧰 Additional context used
🧠 Learnings (1)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts (1)
Learnt from: paodb
PR: FlowingCode/XTermConsoleAddon#99
File: src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts:165-165
Timestamp: 2025-05-15T19:10:55.084Z
Learning: In the XTermConsoleAddon project, using `Object` as a type for the `addons` property in `xterm-element.ts` is acceptable for the implementation, despite typical TypeScript best practices that would suggest more specific typing.
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-xterm/xterm-element.ts
[error] 165-165: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Related Issue: #98
This pull request provides a mechanism for server-side retrieval of add-on instances. While it doesn't automate the entire add-on setup process, it's a foundational step towards better overall add-on management.
Expectation:
The
getAddon()method indirectly supports this expectation, contingent on add-on design.By calling
xterm.getAddon(MySpecificAddon.class), developers can retrieve the server-side Java instance ofMySpecificAddon. If thisMySpecificAddonclass is designed to hold, manage, or provide access to its settings (which might mirror client-side settings or be purely server-side), then this PR enables to access those settings via methods defined on theMySpecificAddonJava object (e.g.,customAddon.getSettings(),customAddon.getSomeSpecificSetting()).The
getAddon()method itself does not automatically create or map client-side JavaScript settings objects into new Java objects if such a mapping isn't already part of the server-side add-on's own implementation. It provides the server-side add-on object; how that object exposes or interacts with settings is determined by the add-on's specific design.In essence, this PR provides a foundational piece for server-side add-on interaction, which can be leveraged for accessing settings if the add-ons are designed to expose them through their server-side Java API. The automation of client-side loading and direct mapping of purely client-side settings objects are considered out of scope for this particular change but could potentially build upon the mechanism introduced here.
Usage Example:
Summary by CodeRabbit
New Features
Other Changes