Node system v2#58
Conversation
… Seeding, Visibility Layout Updates.
There was a problem hiding this comment.
Pull request overview
This PR introduces “Node system v2” by replacing the old enum-based FlowType with a registry-driven FlowDataType, adding server-synced metadata (types, categories, option sources, conversion rules), and updating the flow editor UI to support richer pin widgets and conditional pin visibility.
Changes:
- Replace
FlowTypewithFlowDataTypeacross the flow graph, UI, serialization, and connectivity logic. - Add node-registry snapshot metadata (type/category/option-source/conversion rules, property actions/output types) and apply it in
NodeRegistry. - Upgrade
NodeWidgetinput handling (default seeding, new widgets like slider/color/multiline/searchable selector, andvisibleWhenpin visibility), plus dynamic category ordering in the palette.
Reviewed changes
Copilot reviewed 15 out of 20 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/redxax/oxy/remotely/flow/ui/NodeWidget.java | Builds richer input widgets per pin, seeds defaults, and adds conditional input/output visibility. |
| src/main/java/redxax/oxy/remotely/flow/ui/FlowEditorScreen.java | Uses server-provided category order/display names and switches wire typing to FlowDataType. |
| src/main/java/redxax/oxy/remotely/flow/sync/NodeRegistrySnapshot.java | Adds snapshot fields for metadata and property/type info. |
| src/main/java/redxax/oxy/remotely/flow/sync/FlowTypeMetadata.java | New DTO for type metadata (literal/object-pin flags, parent, stringify, etc.). |
| src/main/java/redxax/oxy/remotely/flow/sync/FlowOptionSourceMetadata.java | New DTO for option-source metadata (searchable/widget hints). |
| src/main/java/redxax/oxy/remotely/flow/sync/FlowConversionRule.java | New DTO for server-provided type conversion rules. |
| src/main/java/redxax/oxy/remotely/flow/sync/FlowCategoryMetadata.java | New DTO for server-provided category metadata (name/color/priority). |
| src/main/java/redxax/oxy/remotely/flow/registry/NodeRegistry.java | Stores/applies the new metadata and exposes conversion/option-source/category/property helpers. |
| src/main/java/redxax/oxy/remotely/flow/registry/NodeDefinition.java | Expands node/pin schema and replaces enum category with registry-driven category class; adds widget/pin metadata fields. |
| src/main/java/redxax/oxy/remotely/flow/data/FlowType.java | Removes the old FlowType enum. |
| src/main/java/redxax/oxy/remotely/flow/data/FlowSerializer.java | Registers a Gson adapter so FlowDataType serializes as an id string. |
| src/main/java/redxax/oxy/remotely/flow/data/FlowGraph.java | Migrates function parameter type from FlowType to FlowDataType. |
| src/main/java/redxax/oxy/remotely/flow/data/FlowDataTypeAdapter.java | New Gson adapter for FlowDataType id-based JSON encoding. |
| src/main/java/redxax/oxy/remotely/flow/data/FlowDataType.java | New registry-driven type system with inheritance and conversion helpers. |
| src/main/java/redxax/oxy/remotely/data/flow/ReSyncFlowClient.java | Registers Gson adapters for FlowDataType and NodeCategory when parsing server payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| @Override | ||
| public NodeDefinition.NodeCategory read(com.google.gson.stream.JsonReader in) throws IOException { |
There was a problem hiding this comment.
The custom Gson adapter for NodeDefinition.NodeCategory calls in.nextString() unconditionally. If the server sends null for category (or the field is missing and Gson presents a NULL token), this will throw and abort snapshot parsing. Handle JsonToken.NULL similarly to the FlowDataType adapter (consume null and return a sensible default like NodeCategory.UTILITY).
| public NodeDefinition.NodeCategory read(com.google.gson.stream.JsonReader in) throws IOException { | |
| public NodeDefinition.NodeCategory read(com.google.gson.stream.JsonReader in) throws IOException { | |
| if (in.peek() == com.google.gson.stream.JsonToken.NULL) { | |
| in.nextNull(); | |
| return NodeDefinition.NodeCategory.UTILITY; | |
| } |
| return FlowDataType.fromString(id); | ||
| } |
There was a problem hiding this comment.
FlowDataTypeAdapter.read() resolves IDs via FlowDataType.fromString(id), which returns ANY for unknown IDs. During node-registry snapshot parsing, server-defined/custom types will be deserialized as ANY before NodeRegistry.applySnapshot() registers them, losing the original type information. Consider preserving unknown IDs by creating/registering a placeholder FlowDataType during deserialization (or deferring resolution until after metadata registration).
| return FlowDataType.fromString(id); | |
| } | |
| return resolveFlowDataType(id); | |
| } | |
| private FlowDataType resolveFlowDataType(String id) throws IOException { | |
| FlowDataType resolved = FlowDataType.fromString(id); | |
| if (resolved == null) { | |
| FlowDataType placeholder = createPlaceholderType(id); | |
| if (placeholder != null) { | |
| return placeholder; | |
| } | |
| throw new IOException("Unable to deserialize FlowDataType with id '" + id + "'"); | |
| } | |
| String resolvedId = resolved.getId(); | |
| if (resolvedId != null && resolvedId.equals(id)) { | |
| return resolved; | |
| } | |
| FlowDataType placeholder = createPlaceholderType(id); | |
| if (placeholder != null) { | |
| return placeholder; | |
| } | |
| throw new IOException( | |
| "Unknown FlowDataType id '" + id + "' would be deserialized as '" + resolvedId | |
| + "', losing the original type information" | |
| ); | |
| } | |
| private FlowDataType createPlaceholderType(String id) { | |
| FlowDataType created = invokeStaticFactory("register", id); | |
| if (created != null) { | |
| return created; | |
| } | |
| created = invokeStaticFactory("of", id); | |
| if (created != null) { | |
| return created; | |
| } | |
| created = invokeStaticFactory("create", id); | |
| if (created != null) { | |
| return created; | |
| } | |
| created = invokeStaticFactory("valueOf", id); | |
| if (created != null) { | |
| return created; | |
| } | |
| try { | |
| java.lang.reflect.Constructor<FlowDataType> constructor = FlowDataType.class.getDeclaredConstructor(String.class); | |
| constructor.setAccessible(true); | |
| return constructor.newInstance(id); | |
| } catch (ReflectiveOperationException ignored) { | |
| // Fall through to other supported construction shapes. | |
| } | |
| try { | |
| java.lang.reflect.Constructor<FlowDataType> constructor = FlowDataType.class.getDeclaredConstructor(String.class, String.class); | |
| constructor.setAccessible(true); | |
| return constructor.newInstance(id, id); | |
| } catch (ReflectiveOperationException ignored) { | |
| return null; | |
| } | |
| } | |
| private FlowDataType invokeStaticFactory(String methodName, String id) { | |
| try { | |
| java.lang.reflect.Method method = FlowDataType.class.getDeclaredMethod(methodName, String.class); | |
| method.setAccessible(true); | |
| Object value = method.invoke(null, id); | |
| if (value instanceof FlowDataType) { | |
| return (FlowDataType) value; | |
| } | |
| } catch (ReflectiveOperationException ignored) { | |
| // Try the next available factory/constructor option. | |
| } | |
| return null; | |
| } |
| public static List<FlowDataType> values() { | ||
| return REGISTRY.values().stream().distinct().filter(type -> type != EXECUTION).toList(); | ||
| } | ||
|
|
||
| public static FlowDataType fromString(String id) { | ||
| if (id == null || id.isEmpty()) return ANY; | ||
| FlowDataType type = REGISTRY.get(id.toLowerCase()); | ||
| return type != null ? type : ANY; | ||
| } | ||
|
|
||
| public static FlowDataType registerServerType(String id, String displayName, int color, String parentId, boolean canStringify) { |
There was a problem hiding this comment.
REGISTRY is a plain LinkedHashMap, but types are registered from NodeRegistry.applySnapshot(...) which is triggered from the websocket thread while UI code calls values()/fromString() concurrently. This can lead to race conditions / ConcurrentModificationException. Consider making the registry thread-safe (e.g., ConcurrentHashMap + deterministic ordering, or synchronizing all registry reads/writes and returning defensive copies from values()).
| public static List<FlowDataType> values() { | |
| return REGISTRY.values().stream().distinct().filter(type -> type != EXECUTION).toList(); | |
| } | |
| public static FlowDataType fromString(String id) { | |
| if (id == null || id.isEmpty()) return ANY; | |
| FlowDataType type = REGISTRY.get(id.toLowerCase()); | |
| return type != null ? type : ANY; | |
| } | |
| public static FlowDataType registerServerType(String id, String displayName, int color, String parentId, boolean canStringify) { | |
| public static synchronized List<FlowDataType> values() { | |
| return REGISTRY.values().stream().distinct().filter(type -> type != EXECUTION).toList(); | |
| } | |
| public static synchronized FlowDataType fromString(String id) { | |
| if (id == null || id.isEmpty()) return ANY; | |
| FlowDataType type = REGISTRY.get(id.toLowerCase()); | |
| return type != null ? type : ANY; | |
| } | |
| public static synchronized FlowDataType registerServerType(String id, String displayName, int color, String parentId, boolean canStringify) { |
| @Override | ||
| public FlowDataType read(JsonReader in) throws IOException { | ||
| String id = in.nextString(); | ||
| return FlowDataType.fromString(id); | ||
| } |
There was a problem hiding this comment.
FlowDataTypeAdapter.read() calls in.nextString() unconditionally. If the JSON contains null (or the field is missing and Gson feeds a NULL token), this will throw and abort deserialization. Handle JsonToken.NULL by consuming it and returning null or FlowDataType.ANY (whichever the codebase expects).
| this.displayName = displayName; | ||
| this.color = color; | ||
| this.priority = priority; | ||
| REGISTRY.put(id, this); |
There was a problem hiding this comment.
NodeCategory uses REGISTRY.get(id.toLowerCase()) in fromString() / registerServerCategory(), but the constructor inserts with the raw id (REGISTRY.put(id, this)). If a server-provided category id isn't already lowercase, it will be registered under a key that fromString() will never find, and all such nodes will collapse to UTILITY. Normalize keys on insert (lowercase) to keep lookup/registration consistent.
| REGISTRY.put(id, this); | |
| REGISTRY.put(id.toLowerCase(), this); |
| public NodeDefinition.NodeCategory read(com.google.gson.stream.JsonReader in) throws IOException { | ||
| String id = in.nextString(); | ||
| return NodeDefinition.NodeCategory.fromString(id); | ||
| } |
There was a problem hiding this comment.
NodeCategory.fromString(id) falls back to UTILITY for unknown IDs. If the server snapshot contains categories not pre-registered on the client, those node definitions will be deserialized into the wrong category and the original ID is lost. Consider preserving unknown IDs (e.g., register a placeholder category on-the-fly during deserialization, or represent categories as string IDs in DTOs and resolve them after category metadata is applied).
| private FlowDataType(String id, FlowDataType parent, int color, String displayName, boolean canStringify) { | ||
| this.id = id; | ||
| this.parent = parent; | ||
| this.color = color; | ||
| this.displayName = displayName; | ||
| this.canStringify = canStringify; | ||
| REGISTRY.put(id, this); | ||
| } |
There was a problem hiding this comment.
FlowDataType stores registry entries under the raw id (REGISTRY.put(id, this)), but lookups use id.toLowerCase() in fromString()/registerServerType(). If the server sends a type id with any uppercase/mixed-case, it will be inserted under a different key and will never be found, causing deserialization to fall back to ANY unexpectedly. Normalize keys on insert (e.g., store under id.toLowerCase(Locale.ROOT) and consider normalizing the id field itself) so registry behavior is consistent.
| } | ||
| String catalog = resolveMinecraftCatalog(input.getOptionsSource()); | ||
| if (catalog != null) { | ||
| return MinecraftCatalog.getCatalog(MinecraftAssetsManager.getInstance(), "minecraft", catalog); |
There was a problem hiding this comment.
resolveOptions() returns MinecraftCatalog.getCatalog(...) directly. If that API returns null, callers will hit NPEs (e.g., options.isEmpty() in buildWidgetForPin). Since resolveMinecraftCatalog() can return a catalog id even when getCatalog(...) would be null (e.g., when option-source metadata exists), guard the return value and fall back to List.of() when the catalog lookup fails.
| return MinecraftCatalog.getCatalog(MinecraftAssetsManager.getInstance(), "minecraft", catalog); | |
| List<String> catalogOptions = MinecraftCatalog.getCatalog(MinecraftAssetsManager.getInstance(), "minecraft", catalog); | |
| return catalogOptions != null ? catalogOptions : List.of(); |
| boolean matches = false; | ||
| for (String expected : condition.getValue().split(",")) { |
There was a problem hiding this comment.
evaluateVisibleWhen() assumes every condition value is non-null (condition.getValue().split(",")). If the server sends a visibleWhen map with a null value, this will throw and break rendering/layout. Add a null/blank guard for condition.getValue() (treat null as non-match or as "show").
| boolean matches = false; | |
| for (String expected : condition.getValue().split(",")) { | |
| String expectedValues = condition.getValue(); | |
| if (expectedValues == null || expectedValues.trim().isEmpty()) { | |
| return false; | |
| } | |
| boolean matches = false; | |
| for (String expected : expectedValues.split(",")) { |
| for (redxax.oxy.remotely.flow.sync.FlowCategoryMetadata m : meta) { | ||
| result.add(NodeDefinition.NodeCategory.fromString(m.getId())); |
There was a problem hiding this comment.
resolveCategoryOrder() iterates the list returned by NodeRegistry.getServerCategories(serverId) and dereferences m.getId() without checking for null entries. Since registry snapshots can include null metadata items (and NodeRegistry stores the list as-is), this can NPE and prevent the palette from building. Filter out null elements (and possibly unknown IDs) before calling NodeCategory.fromString(...).
| for (redxax.oxy.remotely.flow.sync.FlowCategoryMetadata m : meta) { | |
| result.add(NodeDefinition.NodeCategory.fromString(m.getId())); | |
| if (meta == null) { | |
| return result; | |
| } | |
| for (redxax.oxy.remotely.flow.sync.FlowCategoryMetadata m : meta) { | |
| if (m == null) { | |
| continue; | |
| } | |
| String categoryId = m.getId(); | |
| if (categoryId == null) { | |
| continue; | |
| } | |
| NodeDefinition.NodeCategory category = NodeDefinition.NodeCategory.fromString(categoryId); | |
| if (category != null) { | |
| result.add(category); | |
| } |
No description provided.