diff --git a/graphlink_app/graphlink_agents_tools.py b/graphlink_app/graphlink_agents_tools.py index 259b5d9..51b8fac 100644 --- a/graphlink_app/graphlink_agents_tools.py +++ b/graphlink_app/graphlink_agents_tools.py @@ -4,6 +4,7 @@ from PySide6.QtCore import QThread, Signal import graphlink_config as config import api_provider +from graphlink_chart_data import ChartDataError, canonicalize_chart_data class ChartDataAgent: @@ -525,87 +526,12 @@ def validate_chart_data(self, data, chart_type): and an error message if validation fails. """ try: - if not isinstance(data, dict): - return False, "Chart payload must be a JSON object" - data['type'] = str(data.get('type') or chart_type).strip().lower() - if data['type'] != chart_type: - data['type'] = chart_type - data['title'] = str(data.get('title') or f"{chart_type.title()} Chart").strip() - - if chart_type == 'sankey': - if 'flows' not in data: - return False, "Missing required field for sankey chart: flows" - if not isinstance(data.get('flows'), list): - return False, "'flows' must be a list of objects" - if not data['flows']: - return False, "Sankey charts require at least one flow" - for i, flow in enumerate(data['flows']): - if not all(k in flow for k in ['source', 'target', 'value']): - return False, f"Flow item at index {i} is missing a source, target, or value." - if not isinstance(flow['value'], (int, float)): - return False, f"Flow value at index {i} must be a number." - if flow['value'] <= 0: - return False, f"Flow value at index {i} must be greater than zero." - if not str(flow['source']).strip() or not str(flow['target']).strip(): - return False, f"Flow item at index {i} must have a non-empty source and target." - - elif chart_type == 'histogram': - if 'values' not in data: - return False, "Missing required field for histogram: values" - data.setdefault('bins', 10) - data.setdefault('xAxis', 'Value') - data.setdefault('yAxis', 'Frequency') - if not isinstance(data['bins'], (int, float)): - return False, "Bins must be a number" - if int(data['bins']) < 2: - return False, "Histogram bins must be at least 2" - if not isinstance(data.get('values'), list) or len(data['values']) < 2: - return False, "Histogram charts require at least two values" - - elif chart_type in ['bar', 'line']: - if 'labels' not in data or 'values' not in data: - return False, f"Missing required fields for {chart_type} chart: labels and values" - data.setdefault('xAxis', 'Category' if chart_type == 'bar' else 'Sequence') - data.setdefault('yAxis', 'Value') - if not isinstance(data.get('labels', []), list): - return False, "Labels must be a list" - if len(data['labels']) != len(data['values']): - return False, "Labels and values must have the same length" - if not data['labels']: - return False, f"{chart_type.title()} charts require at least one labeled value" - if chart_type == 'line' and len(data['values']) < 2: - return False, "Line charts require at least two data points" - if any(not str(label).strip() for label in data['labels']): - return False, "Labels must be non-empty strings" - - elif chart_type == 'pie': - if 'labels' not in data or 'values' not in data: - return False, "Missing required fields for pie chart: labels and values" - if not isinstance(data.get('labels', []), list): - return False, "Labels must be a list" - if len(data['labels']) != len(data['values']): - return False, "Labels and values must have the same length" - if not data['labels']: - return False, "Pie charts require at least one labeled value" - if any(not str(label).strip() for label in data['labels']): - return False, "Labels must be non-empty strings" - - # Ensure all 'values' are numeric for non-Sankey charts. - if chart_type != 'sankey': - try: - if isinstance(data['values'], list): - data['values'] = [float(v) for v in data['values']] - except (ValueError, TypeError): - return False, "All values must be numeric" - if chart_type == 'pie' and any(v <= 0 for v in data['values']): - return False, "Pie chart values must be greater than zero" - if chart_type == 'histogram': - data['bins'] = max(2, int(data['bins'])) - + canonical = canonicalize_chart_data(data, chart_type) + data.clear() + data.update(canonical) return True, None - - except Exception as e: - return False, f"Validation error: {str(e)}" + except (ChartDataError, TypeError, ValueError) as exc: + return False, str(exc) def process_sankey_data(self, raw_data): """ diff --git a/graphlink_app/graphlink_canvas/graphlink_canvas_base.py b/graphlink_app/graphlink_canvas/graphlink_canvas_base.py index df95fef..8675dcf 100644 --- a/graphlink_app/graphlink_canvas/graphlink_canvas_base.py +++ b/graphlink_app/graphlink_canvas/graphlink_canvas_base.py @@ -195,6 +195,7 @@ def iter_scene_connection_lists(scene): "group_summary_connections", "html_connections", "artifact_connections", + "chart_connections", ) for name in list_names: diff --git a/graphlink_app/graphlink_canvas/graphlink_canvas_chart_item.py b/graphlink_app/graphlink_canvas/graphlink_canvas_chart_item.py index b77978f..493e32d 100644 --- a/graphlink_app/graphlink_canvas/graphlink_canvas_chart_item.py +++ b/graphlink_app/graphlink_canvas/graphlink_canvas_chart_item.py @@ -20,6 +20,7 @@ from PySide6.QtGui import QPainter, QColor, QBrush, QPen, QFont, QPainterPath, QImage, QLinearGradient from graphlink_config import get_current_palette, get_graph_node_colors +from graphlink_chart_data import ChartDataError, canonicalize_chart_data class ChartItem(QGraphicsItem): @@ -35,6 +36,8 @@ class ChartItem(QGraphicsItem): MIN_HEIGHT = 320 DEFAULT_WIDTH = 680 DEFAULT_HEIGHT = 500 + MAX_WIDTH = 2400 + MAX_HEIGHT = 1800 RESIZE_DEBOUNCE_MS = 90 EXPORT_SCALE = 3.0 RESIZE_GRID = 20 @@ -50,8 +53,15 @@ def __init__(self, data, pos, parent=None, parent_content_node=None): """ super().__init__(parent) self.setPos(pos) - self.data = data - self.title = data.get("title", "Chart") + self.persistent_id = None + self.source_node = None + self.data_error = None + try: + self.data = canonicalize_chart_data(data) + except (ChartDataError, TypeError, ValueError) as exc: + self.data = dict(data) if isinstance(data, dict) else {} + self.data_error = str(exc) + self.title = str(self.data.get("title") or "Chart") self.parent_content_node = parent_content_node self.setFlag(QGraphicsItem.GraphicsItemFlag.ItemIsMovable) self.setFlag(QGraphicsItem.GraphicsItemFlag.ItemIsSelectable) @@ -64,8 +74,13 @@ def __init__(self, data, pos, parent=None, parent_content_node=None): self.resize_handle_hovered = False self.resizing = False self.chart_image = QImage() + self._render_cache = {} self.aspect_ratio_locked = True self._base_aspect_ratio = self.DEFAULT_WIDTH / self.DEFAULT_HEIGHT + self.resize_start_aspect_ratio = self._base_aspect_ratio + self.font_family = "Segoe UI" + self.font_size = 10 + self.font_color = QColor("#dddddd") self.figure = Figure(figsize=(6, 4), dpi=160) self.canvas = FigureCanvasAgg(self.figure) @@ -129,22 +144,25 @@ def _snap_dimension(self, value): def _clamp_size(self, width, height, preserve_aspect=None): preserve_aspect = self.aspect_ratio_locked if preserve_aspect is None else preserve_aspect - width = max(self.MIN_WIDTH, float(width)) - height = max(self.MIN_HEIGHT, float(height)) + width = min(self.MAX_WIDTH, max(self.MIN_WIDTH, float(width))) + height = min(self.MAX_HEIGHT, max(self.MIN_HEIGHT, float(height))) if preserve_aspect: - aspect_ratio = self._base_aspect_ratio if self._base_aspect_ratio > 0 else (self.DEFAULT_WIDTH / self.DEFAULT_HEIGHT) + aspect_ratio = self.resize_start_aspect_ratio if self.resizing else self._base_aspect_ratio + aspect_ratio = aspect_ratio if aspect_ratio > 0 else (self.DEFAULT_WIDTH / self.DEFAULT_HEIGHT) width_from_height = height * aspect_ratio height_from_width = width / aspect_ratio if abs(width_from_height - width) < abs(height_from_width - height): - width = width_from_height + width = min(self.MAX_WIDTH, width_from_height) + height = width / aspect_ratio else: - height = height_from_width + height = min(self.MAX_HEIGHT, height_from_width) + width = height * aspect_ratio width = self._snap_dimension(width) height = self._snap_dimension(height) - width = max(self.MIN_WIDTH, float(width)) - height = max(self.MIN_HEIGHT, float(height)) + width = min(self.MAX_WIDTH, max(self.MIN_WIDTH, float(width))) + height = min(self.MAX_HEIGHT, max(self.MIN_HEIGHT, float(height))) return width, height def set_chart_size(self, width, height, preserve_aspect=None, rerender=True): @@ -273,87 +291,7 @@ def _wrap_label(self, text, width=14, max_lines=2): return "\n".join(lines) def _sanitize_chart_data(self): - chart_type = self._chart_type() - cleaned = { - "type": chart_type, - "title": str(self.data.get("title", "Chart")).strip() or "Chart", - } - - if chart_type in {"bar", "line", "pie"}: - labels = [] - values = [] - for raw_label, raw_value in zip(self.data.get("labels", []), self.data.get("values", [])): - number = self._coerce_number(raw_value) - label = "" if raw_label is None else str(raw_label).strip() - if not label or number is None: - continue - if chart_type == "pie" and number <= 0: - continue - labels.append(label) - values.append(number) - - if not labels or not values: - raise ValueError("The chart data does not contain enough valid labeled values.") - - cleaned["labels"] = labels - cleaned["values"] = values - cleaned["xAxis"] = str(self.data.get("xAxis", "")).strip() - cleaned["yAxis"] = str(self.data.get("yAxis", "")).strip() - return cleaned - - if chart_type == "histogram": - values = [ - number - for number in (self._coerce_number(value) for value in self.data.get("values", [])) - if number is not None - ] - if len(values) < 2: - raise ValueError("Histogram charts need at least two numeric values.") - bins = self.data.get("bins", 10) - bins = int(bins) if isinstance(bins, (int, float)) else 10 - cleaned["values"] = values - cleaned["bins"] = max(3, min(24, bins, len(values))) - cleaned["xAxis"] = str(self.data.get("xAxis", "")).strip() - cleaned["yAxis"] = str(self.data.get("yAxis", "Frequency")).strip() or "Frequency" - return cleaned - - if chart_type == "sankey": - flows = [] - raw_flows = self.data.get("flows", []) - if not raw_flows and isinstance(self.data.get("data"), dict): - nodes = self.data["data"].get("nodes", []) - links = self.data["data"].get("links", []) - raw_flows = [] - for link in links: - source_idx = link.get("source") - target_idx = link.get("target") - if not isinstance(source_idx, int) or not isinstance(target_idx, int): - continue - if source_idx >= len(nodes) or target_idx >= len(nodes): - continue - raw_flows.append( - { - "source": nodes[source_idx].get("name", f"Node {source_idx}"), - "target": nodes[target_idx].get("name", f"Node {target_idx}"), - "value": link.get("value"), - } - ) - - for flow in raw_flows: - source = str(flow.get("source", "")).strip() - target = str(flow.get("target", "")).strip() - value = self._coerce_number(flow.get("value")) - if not source or not target or value is None or value <= 0: - continue - flows.append({"source": source, "target": target, "value": value}) - - if not flows: - raise ValueError("Sankey charts need at least one valid flow.") - - cleaned["flows"] = flows - return cleaned - - raise ValueError(f"Unsupported chart type: {chart_type or 'unknown'}") + return canonicalize_chart_data(self.data, self._chart_type()) def _prepare_standard_axes(self, ax, theme, x_label="", y_label="", x_grid=False, y_grid=True): ax.set_facecolor(theme["surface"].name()) @@ -371,6 +309,24 @@ def _prepare_standard_axes(self, ax, theme, x_label="", y_label="", x_grid=False if y_label: ax.set_ylabel(y_label, fontsize=9, color=theme["muted"].name(), labelpad=8) + def _set_categorical_ticks(self, ax, positions, labels): + """Show a readable subset of labels while retaining every data point.""" + if not positions: + return + max_ticks = max(4, min(12, int(max(1.0, self._chart_rect().width()) / 72))) + stride = max(1, math.ceil(len(positions) / max_ticks)) + tick_indexes = list(range(0, len(positions), stride)) + if tick_indexes[-1] != len(positions) - 1: + tick_indexes.append(len(positions) - 1) + tick_positions = [positions[index] for index in tick_indexes] + tick_labels = [self._wrap_label(labels[index], 14, 2) for index in tick_indexes] + ax.set_xticks(tick_positions) + ax.set_xticklabels( + tick_labels, + rotation=25 if len(tick_indexes) > 5 else 0, + ha="right" if len(tick_indexes) > 5 else "center", + ) + def _render_bar_chart(self, ax, chart_data, theme): values = chart_data["values"] labels = chart_data["labels"] @@ -387,8 +343,7 @@ def _render_bar_chart(self, ax, chart_data, theme): zorder=3, ) self._prepare_standard_axes(ax, theme, chart_data["xAxis"], chart_data["yAxis"], y_grid=True) - ax.set_xticks(positions) - ax.set_xticklabels([self._wrap_label(label, 12, 2) for label in labels]) + self._set_categorical_ticks(ax, positions, labels) ax.margins(x=0.04) min_value = min(values) @@ -442,8 +397,7 @@ def _render_line_chart(self, ax, chart_data, theme): color=self._mpl_rgba(theme["primary"], 0.15), zorder=2, ) - ax.set_xticks(positions) - ax.set_xticklabels([self._wrap_label(label, 12, 2) for label in labels]) + self._set_categorical_ticks(ax, positions, labels) ax.margins(x=0.04) value_span = max(values) - min(values) @@ -465,10 +419,16 @@ def _render_line_chart(self, ax, chart_data, theme): def _render_pie_chart(self, ax, chart_data, theme): values = chart_data["values"] labels = chart_data["labels"] + if len(values) > 10: + ranked = sorted(zip(values, labels), reverse=True) + top = ranked[:9] + remainder = sum(value for value, _ in ranked[9:]) + values = [value for value, _ in top] + [remainder] + labels = [label for _, label in top] + ["Other"] colors = [theme["cycle"][index % len(theme["cycle"])] for index in range(len(values))] total = sum(values) - self.figure.subplots_adjust(left=0.06, right=0.78, top=0.92, bottom=0.08) + self.figure.subplots_adjust(left=0.06, right=0.66, top=0.92, bottom=0.08) wedges, _ = ax.pie( values, startangle=90, @@ -501,14 +461,14 @@ def _render_pie_chart(self, ax, chart_data, theme): legend_labels = [] for label, value in zip(labels, values): percentage = (value / total) * 100 if total else 0 - wrapped = self._wrap_label(label, 20, 2).replace("\n", " ") + wrapped = textwrap.shorten(" ".join(str(label).split()), width=24, placeholder="...") legend_labels.append(f"{wrapped} {percentage:.1f}%") legend = ax.legend( wedges, legend_labels, loc="center left", - bbox_to_anchor=(0.98, 0.5), + bbox_to_anchor=(1.01, 0.5), frameon=False, fontsize=8, handlelength=1.2, @@ -782,14 +742,44 @@ def _figure_to_image(self): return QImage(buffer, width, height, width * 4, QImage.Format.Format_RGBA8888).copy() def _render_chart_to_image(self, export_scale=None): - self.title = self.data.get("title", "Chart") + self.title = str(self.data.get("title") or "Chart").strip() or "Chart" + palette = get_current_palette() + palette_signature = tuple( + repr(getattr(palette, name, "")) for name in ("AI_NODE", "USER_NODE", "SELECTION") + ) + cache_key = ( + export_scale, + round(self.width, 1), + round(self.height, 1), + self.font_family, + self.font_size, + repr(self.font_color), + palette_signature, + repr(self.data), + ) + cached = self._render_cache.get(cache_key) + if cached is not None: + return cached.copy() if export_scale is None: self._set_figure_geometry() else: self._set_export_figure_geometry(export_scale) - theme = self._build_theme(get_current_palette()) + theme = self._build_theme(palette) + try: + with matplotlib.rc_context({"font.family": [self.font_family]}): + image = self._render_chart_content(theme) + if not image.isNull(): + self._render_cache[cache_key] = image.copy() + if len(self._render_cache) > 8: + self._render_cache.pop(next(iter(self._render_cache))) + return image + except Exception as exc: + self._render_error_image(str(exc)) + return self.chart_image + + def _render_chart_content(self, theme): try: self.figure.clear() self.figure.patch.set_facecolor(theme["surface"].name()) @@ -818,6 +808,20 @@ def _render_chart_to_image(self, export_scale=None): self._render_error_image(str(exc)) return self.chart_image + def update_font_settings(self, family, size, color): + self.font_family = str(family or "Segoe UI") + self.font_size = max(6, min(32, int(size))) + self.font_color = QColor(color) + self.generate_chart() + + def to_context_text(self): + """Return bounded structured data for chart-to-chart generation context.""" + try: + canonical = self._sanitize_chart_data() + except (ChartDataError, TypeError, ValueError): + canonical = self.data + return str(canonical) + def _image_target_rect(self, rect, image): if image.isNull(): return rect @@ -851,12 +855,18 @@ def _notify(self, message, level="success", duration_ms=5000): def export_png(self, file_path=None, scale=None): file_path = file_path or self._desktop_export_path() scale = self.EXPORT_SCALE if scale is None else scale - image = self._render_chart_to_image(export_scale=scale) - if image.isNull(): - raise ValueError("Chart export failed because no image could be rendered.") - if not image.save(file_path, "PNG"): - raise IOError(f"Could not save chart image to {file_path}") - return file_path + try: + image = self._render_chart_to_image(export_scale=scale) + if image.isNull(): + raise ValueError("Chart export failed because no image could be rendered.") + parent_directory = os.path.dirname(os.path.abspath(file_path)) + if parent_directory: + os.makedirs(parent_directory, exist_ok=True) + if not image.save(file_path, "PNG"): + raise IOError(f"Could not save chart image to {file_path}") + return file_path + finally: + self.generate_chart() def generate_chart(self): """ @@ -909,16 +919,16 @@ def paint(self, painter, option, widget=None): painter.setPen(Qt.PenStyle.NoPen) painter.drawPath(header_path) - painter.setPen(QPen(QColor("#ffffff"))) - painter.setFont(QFont("Segoe UI", 10, QFont.Weight.Bold)) + painter.setPen(QPen(self.font_color)) + painter.setFont(QFont(self.font_family, self.font_size, QFont.Weight.Bold)) painter.drawText(header_rect.adjusted(12, 0, -120, 0), Qt.AlignmentFlag.AlignVCenter, self.title) badge_rect = QRectF(self.width - 102, 8, 90, 24) painter.setPen(QPen(self._with_alpha(QColor("#ffffff"), 65), 1)) painter.setBrush(QBrush(node_colors["badge_fill"])) painter.drawRoundedRect(badge_rect, 12, 12) - painter.setPen(QPen(QColor("#ffffff"))) - painter.setFont(QFont("Segoe UI", 8, QFont.Weight.DemiBold)) + painter.setPen(QPen(self.font_color)) + painter.setFont(QFont(self.font_family, max(6, self.font_size - 2), QFont.Weight.DemiBold)) painter.drawText(badge_rect, Qt.AlignmentFlag.AlignCenter, self._badge_text()) chart_panel = chart_rect.adjusted(-8, -8, 8, 8) @@ -965,7 +975,7 @@ def mousePressEvent(self, event): self.resizing = True self.resize_start_pos = event.pos() self.resize_start_size = QSizeF(self.width, self.height) - self.resize_start_aspect_ratio = self._base_aspect_ratio + self.resize_start_aspect_ratio = self.width / self.height if self.height else self._base_aspect_ratio event.accept() return diff --git a/graphlink_app/graphlink_chart_data.py b/graphlink_app/graphlink_chart_data.py new file mode 100644 index 0000000..6a29c54 --- /dev/null +++ b/graphlink_app/graphlink_chart_data.py @@ -0,0 +1,194 @@ +"""Canonical chart-data validation shared by generation, rendering, and storage.""" + +from __future__ import annotations + +import math + + +SUPPORTED_CHART_TYPES = frozenset({"bar", "line", "pie", "histogram", "sankey"}) +MAX_SERIES_POINTS = 300 +MAX_SANKEY_FLOWS = 300 +MAX_LABEL_LENGTH = 160 +MAX_TITLE_LENGTH = 200 + + +class ChartDataError(ValueError): + """Raised when a chart payload cannot be converted to the canonical schema.""" + + +def _finite_number(value, field_name): + if isinstance(value, bool): + raise ChartDataError(f"{field_name} must be a finite number") + try: + number = float(value) + except (TypeError, ValueError) as exc: + raise ChartDataError(f"{field_name} must be a finite number") from exc + if not math.isfinite(number): + raise ChartDataError(f"{field_name} must be a finite number") + return number + + +def _clean_text(value, field_name, *, default="", max_length=MAX_LABEL_LENGTH): + text = " ".join(str(value if value is not None else "").split()).strip() + if not text: + if default: + return default + raise ChartDataError(f"{field_name} must be non-empty") + if len(text) > max_length: + raise ChartDataError(f"{field_name} is too long (maximum {max_length} characters)") + return text + + +def _legacy_sankey_flows(payload): + nested = payload.get("data") + if not isinstance(nested, dict): + return None + nodes = nested.get("nodes", []) + links = nested.get("links", []) + if not isinstance(nodes, list) or not isinstance(links, list): + return None + + names = [] + for index, node in enumerate(nodes): + if isinstance(node, dict): + node_name = node.get("name", f"Node {index}") + else: + node_name = node + names.append(str(node_name).strip()) + + flows = [] + for link in links: + if not isinstance(link, dict): + raise ChartDataError("Sankey links must be objects") + source = link.get("source") + target = link.get("target") + if isinstance(source, int) and 0 <= source < len(names): + source = names[source] + if isinstance(target, int) and 0 <= target < len(names): + target = names[target] + flows.append({"source": source, "target": target, "value": link.get("value")}) + return flows + + +def _assert_acyclic(flows): + adjacency = {} + for flow in flows: + adjacency.setdefault(flow["source"], set()).add(flow["target"]) + adjacency.setdefault(flow["target"], set()) + + visiting = set() + visited = set() + + def visit(node): + if node in visiting: + raise ChartDataError("Sankey charts cannot contain cycles") + if node in visited: + return + visiting.add(node) + for target in sorted(adjacency.get(node, ())): + visit(target) + visiting.remove(node) + visited.add(node) + + for node in sorted(adjacency): + visit(node) + + +def canonicalize_chart_data(data, chart_type=None): + """Return a new, bounded chart payload or raise :class:`ChartDataError`. + + The function intentionally does not mutate ``data``. Callers that retain the + legacy in-place API can copy the returned mapping back into their payload. + """ + if not isinstance(data, dict): + raise ChartDataError("Chart payload must be a JSON object") + + expected_type = str(chart_type or data.get("type") or "").strip().lower() + if expected_type not in SUPPORTED_CHART_TYPES: + raise ChartDataError(f"Unsupported chart type: {expected_type or 'unknown'}") + + actual_type = str(data.get("type") or expected_type).strip().lower() + if actual_type != expected_type: + raise ChartDataError(f"Chart payload type must be {expected_type}") + + result = { + "type": expected_type, + "title": _clean_text( + data.get("title"), + "Chart title", + default=f"{expected_type.title()} Chart", + max_length=MAX_TITLE_LENGTH, + ), + } + + if expected_type in {"bar", "line", "pie"}: + labels = data.get("labels") + values = data.get("values") + if not isinstance(labels, list) or not isinstance(values, list): + raise ChartDataError("Labels and values must both be lists") + if not labels or not values: + raise ChartDataError(f"{expected_type.title()} charts require at least one data point") + if len(labels) != len(values): + raise ChartDataError("Labels and values must have the same length") + if len(labels) > MAX_SERIES_POINTS: + raise ChartDataError(f"Charts support at most {MAX_SERIES_POINTS} data points") + if expected_type == "line" and len(labels) < 2: + raise ChartDataError("Line charts require at least two data points") + + result["labels"] = [_clean_text(label, f"Label at index {index}") for index, label in enumerate(labels)] + result["values"] = [ + _finite_number(value, f"Value at index {index}") for index, value in enumerate(values) + ] + if expected_type == "pie" and any(value <= 0 for value in result["values"]): + raise ChartDataError("Pie chart values must be greater than zero") + result["xAxis"] = _clean_text( + data.get("xAxis"), + "X-axis label", + default="Category" if expected_type == "bar" else "Sequence", + max_length=MAX_LABEL_LENGTH, + ) + result["yAxis"] = _clean_text(data.get("yAxis"), "Y-axis label", default="Value", max_length=MAX_LABEL_LENGTH) + return result + + if expected_type == "histogram": + values = data.get("values") + if not isinstance(values, list) or len(values) < 2: + raise ChartDataError("Histogram charts require at least two values") + if len(values) > MAX_SERIES_POINTS: + raise ChartDataError(f"Histograms support at most {MAX_SERIES_POINTS} values") + bins = _finite_number(data.get("bins", 10), "Histogram bins") + if bins < 2: + raise ChartDataError("Histogram bins must be at least 2") + result["values"] = [_finite_number(value, f"Value at index {index}") for index, value in enumerate(values)] + result["bins"] = max(2, min(int(bins), 24, len(values))) + result["xAxis"] = _clean_text(data.get("xAxis"), "X-axis label", default="Value", max_length=MAX_LABEL_LENGTH) + result["yAxis"] = _clean_text(data.get("yAxis"), "Y-axis label", default="Frequency", max_length=MAX_LABEL_LENGTH) + return result + + raw_flows = data.get("flows") + if raw_flows is None: + raw_flows = _legacy_sankey_flows(data) + if not isinstance(raw_flows, list) or not raw_flows: + raise ChartDataError("Sankey charts require at least one flow") + if len(raw_flows) > MAX_SANKEY_FLOWS: + raise ChartDataError(f"Sankey charts support at most {MAX_SANKEY_FLOWS} flows") + + aggregated = {} + for index, flow in enumerate(raw_flows): + if not isinstance(flow, dict): + raise ChartDataError(f"Sankey flow at index {index} must be an object") + source = _clean_text(flow.get("source"), f"Sankey source at index {index}") + target = _clean_text(flow.get("target"), f"Sankey target at index {index}") + value = _finite_number(flow.get("value"), f"Sankey value at index {index}") + if value <= 0: + raise ChartDataError(f"Sankey value at index {index} must be greater than zero") + key = (source, target) + aggregated[key] = aggregated.get(key, 0.0) + value + + result["flows"] = [ + {"source": source, "target": target, "value": value} + for (source, target), value in sorted(aggregated.items()) + ] + _assert_acyclic(result["flows"]) + return result + diff --git a/graphlink_app/graphlink_scene.py b/graphlink_app/graphlink_scene.py index 525df49..c141886 100644 --- a/graphlink_app/graphlink_scene.py +++ b/graphlink_app/graphlink_scene.py @@ -62,6 +62,7 @@ def __init__(self, window): self.artifact_nodes = [] self.gitlink_nodes = [] self.chart_nodes = [] + self.chart_connections = [] self.transient_layout_items = [] self._connection_index = {} self._scene_change_pending = False @@ -173,6 +174,7 @@ def _all_connection_lists(self): self.html_connections, self.artifact_connections, self.gitlink_connections, + self.chart_connections, ] def register_connection(self, connection): @@ -245,6 +247,21 @@ def _update_all_node_fonts(self): self.update_connections() self.scene_changed.emit() + def resolve_chart_parent(self, node): + """Return the nearest conversational owner for a chart source.""" + conversational = set(self._all_conversational_nodes()) + current = node + visited = set() + while current is not None and id(current) not in visited: + if current in conversational: + return current + visited.add(id(current)) + current = ( + getattr(current, "parent_content_node", None) + or getattr(current, "parent_node", None) + ) + return None + def find_items(self, text): """ Searches all nodes for a given text string. @@ -285,20 +302,7 @@ def find_items(self, text): elif isinstance(node, CodeSandboxNode): content = node.get_prompt() + "\n" + node.get_requirements() + "\n" + node.get_code() + "\n" + node.output_display.toPlainText() elif isinstance(node, ChartItem): - labels = node.data.get("labels", []) if isinstance(node.data, dict) else [] - flows = node.data.get("flows", []) if isinstance(node.data, dict) else [] - flow_text = "\n".join( - f"{flow.get('source', '')} -> {flow.get('target', '')}: {flow.get('value', '')}" - for flow in flows if isinstance(flow, dict) - ) - content = "\n".join( - part for part in ( - str(node.data.get("title", "")) if isinstance(node.data, dict) else "", - str(node.data.get("type", "")) if isinstance(node.data, dict) else "", - "\n".join(str(label) for label in labels), - flow_text, - ) if part - ) + content = node.to_context_text() if hasattr(node, "to_context_text") else str(node.data) if text in content.lower(): matches.append(node) @@ -314,8 +318,7 @@ def update_search_highlight(self, matched_nodes): Args: matched_nodes (list): A list of nodes that should be highlighted. """ - all_nodes = (self._all_conversational_nodes() + self.code_nodes + self.document_nodes + - self.image_nodes + self.thinking_nodes) + all_nodes = self._all_layout_nodes() for node in all_nodes: is_match = node in matched_nodes if getattr(node, 'is_search_match', False) != is_match: @@ -555,13 +558,21 @@ def add_navigation_pin(self, pos): self.pins.append(pin) return pin - def add_chart(self, data, pos, parent_content_node=None): + def add_chart(self, data, pos, parent_content_node=None, source_node=None): """Adds a new ChartItem to the scene.""" - chart = ChartItem(data, pos, parent_content_node=parent_content_node) - strategy = "content" if parent_content_node is not None else "general" + source_node = source_node or parent_content_node + resolved_parent = self.resolve_chart_parent(parent_content_node or source_node) + chart = ChartItem(data, pos, parent_content_node=resolved_parent) + chart.source_node = source_node if source_node in self._all_layout_nodes() else resolved_parent + strategy = "content" if resolved_parent is not None else "general" chart.setPos(self.find_free_position(pos, chart, strategy=strategy)) self.chart_nodes.append(chart) self.addItem(chart) + if chart.source_node is not None and chart.source_node is not chart and chart.source_node.scene() == self: + connection = ContentConnectionItem(chart.source_node, chart) + self.addItem(connection) + self.chart_connections.append(connection) + self.register_connection(connection) self.scene_changed.emit() return chart @@ -976,7 +987,8 @@ def update_connections(self): for conn_list in [self.content_connections, self.document_connections, self.image_connections, self.thinking_connections, self.system_prompt_connections, self.pycoder_connections, self.code_sandbox_connections, self.web_connections, self.conversation_connections, self.group_summary_connections, - self.html_connections, self.artifact_connections, self.gitlink_connections]: + self.html_connections, self.artifact_connections, self.gitlink_connections, + self.chart_connections]: for conn in conn_list: conn.update_path() if hasattr(conn, 'sync_visibility_mode'): @@ -1151,7 +1163,20 @@ def organize_nodes(self): self.scene_changed.emit() def _remove_associated_chart_nodes(self, parent_node): - charts_to_remove = [chart for chart in self.chart_nodes if getattr(chart, 'parent_content_node', None) == parent_node] + owners = {parent_node} + charts_to_remove = [] + changed = True + while changed: + changed = False + for chart in self.chart_nodes: + chart_parent = getattr(chart, "parent_content_node", None) + chart_source = getattr(chart, "source_node", None) + if chart in charts_to_remove: + continue + if chart_parent in owners or chart_source in owners: + charts_to_remove.append(chart) + owners.add(chart) + changed = True for chart in charts_to_remove: chart_parent = chart.parentItem() if isinstance(chart_parent, Frame) and chart in chart_parent.nodes: @@ -1166,6 +1191,7 @@ def _remove_associated_chart_nodes(self, parent_node): chart_parent.updateGeometry() else: self.deleteContainer(chart_parent) + self._remove_connections_for_node(chart, [self.chart_connections]) if hasattr(chart, "dispose"): chart.dispose() if chart.scene() == self: @@ -1373,6 +1399,7 @@ def deleteSelectedItems(self): self.deleteContainer(parent) else: parent.updateGeometry() + self._remove_connections_for_node(item, [self.chart_connections]) if hasattr(item, "dispose"): item.dispose() self.removeItem(item) @@ -1532,6 +1559,7 @@ def clear(self): self.html_connections.clear() self.artifact_connections.clear() self.gitlink_connections.clear() + self.chart_connections.clear() if hasattr(self, 'window') and self.window: self.window.current_node = None diff --git a/graphlink_app/graphlink_session/deserializers.py b/graphlink_app/graphlink_session/deserializers.py index f37f276..1b26657 100644 --- a/graphlink_app/graphlink_session/deserializers.py +++ b/graphlink_app/graphlink_session/deserializers.py @@ -65,6 +65,17 @@ def _resolve_node_ref(self, data, id_key, index_key, all_nodes_map): return nodes_by_id[node_id] return all_nodes_map.get(data.get(index_key)) + def _resolve_chart_ref(self, data, id_key, index_key, all_nodes_map): + node_id = data.get(id_key) + if node_id: + for mapping in ( + getattr(self, "_nodes_by_id", None) or {}, + getattr(self, "_charts_by_id", None) or {}, + ): + if node_id in mapping: + return mapping[node_id] + return all_nodes_map.get(data.get(index_key)) + def _deserialize_basic_connection(self, data, scene, all_nodes_map, connection_cls, target_list_name): start_node = self._resolve_node_ref(data, "start_node_id", "start_node_index", all_nodes_map) end_node = self._resolve_node_ref(data, "end_node_id", "end_node_index", all_nodes_map) @@ -78,12 +89,17 @@ def _deserialize_basic_connection(self, data, scene, all_nodes_map, connection_c return connection def deserialize_chart(self, data, scene, all_nodes_map): - parent_node = all_nodes_map.get(data.get("parent_node_index")) + if not isinstance(data, dict) or not isinstance(data.get("data"), dict): + raise ValueError("Chart record is missing a data object") + parent_node = self._resolve_chart_ref(data, "parent_node_id", "parent_node_index", all_nodes_map) + source_node = self._resolve_chart_ref(data, "source_node_id", "parent_node_index", all_nodes_map) chart = scene.add_chart( data["data"], QPointF(data["position"]["x"], data["position"]["y"]), parent_content_node=parent_node, + source_node=source_node, ) + chart.persistent_id = data.get("id") or chart.persistent_id chart.aspect_ratio_locked = bool(data.get("aspect_ratio_locked", True)) if "size" in data: @@ -586,8 +602,16 @@ def restore_chat(self, chat, notes_data, pins_data): notes_map = self._load_notes(scene, notes_data) charts_map = {} + self._charts_by_id = {} for index, chart_data in enumerate(chat_data.get("charts", [])): - charts_map[index] = self.deserialize_chart(chart_data, scene, all_nodes_map) + try: + chart = self.deserialize_chart(chart_data, scene, all_nodes_map) + except Exception as exc: + self._set_status_message(f"Skipped invalid chart {index + 1}: {exc}", "warning") + continue + charts_map[index] = chart + if isinstance(chart_data, dict) and chart_data.get("id"): + self._charts_by_id[chart_data["id"]] = chart # Frame/container item references are positions in the SAVE-side item # space (all payload nodes, then notes, then charts, then frames - see diff --git a/graphlink_app/graphlink_session/manager.py b/graphlink_app/graphlink_session/manager.py index dd6fea4..6a32924 100644 --- a/graphlink_app/graphlink_session/manager.py +++ b/graphlink_app/graphlink_session/manager.py @@ -54,6 +54,8 @@ def mark_context_switch(self): this. See doc/ARCHITECTURE_REVIEW_FINDINGS.md. """ self._context_epoch += 1 + if self.window and hasattr(self.window, "invalidate_chart_requests"): + self.window.invalidate_chart_requests() def load_chat(self, chat_id): chat = self.db.load_chat(chat_id) diff --git a/graphlink_app/graphlink_session/serializers.py b/graphlink_app/graphlink_session/serializers.py index a8fb034..13b1beb 100644 --- a/graphlink_app/graphlink_session/serializers.py +++ b/graphlink_app/graphlink_session/serializers.py @@ -8,6 +8,7 @@ from graphlink_plugins.graphlink_plugin_gitlink import GitlinkNode from graphlink_pycoder import PyCoderNode from graphlink_web import WebNode +from graphlink_chart_data import ChartDataError, canonicalize_chart_data from graphlink_session.content_codec import ( encode_image_bytes, @@ -345,12 +346,23 @@ def serialize_note(self, note): def serialize_chart(self, chart, all_nodes_list): parent_node = getattr(chart, "parent_content_node", None) parent_node_index = all_nodes_list.index(parent_node) if parent_node in all_nodes_list else None + try: + chart_data = canonicalize_chart_data(chart.data) + data_error = None + except (ChartDataError, TypeError, ValueError) as exc: + chart_data = dict(getattr(chart, "data", {}) or {}) + data_error = str(exc) + source_node = getattr(chart, "source_node", None) return { - "data": chart.data, + "id": self._node_persistent_id(chart), + "data": chart_data, "position": {"x": chart.pos().x(), "y": chart.pos().y()}, "size": {"width": chart.width, "height": chart.height}, "aspect_ratio_locked": getattr(chart, "aspect_ratio_locked", True), "parent_node_index": parent_node_index, + "parent_node_id": self._node_persistent_id(parent_node) if parent_node is not None else None, + "source_node_id": self._node_persistent_id(source_node) if source_node is not None else None, + "data_error": data_error, } def serialize_chat_data(self): diff --git a/graphlink_app/graphlink_window_actions.py b/graphlink_app/graphlink_window_actions.py index cf1e441..2547c4f 100644 --- a/graphlink_app/graphlink_window_actions.py +++ b/graphlink_app/graphlink_window_actions.py @@ -604,6 +604,8 @@ def add_fragment(label, text): except Exception: text_value = "" add_fragment("Visible Text", text_value) + if hasattr(node, "to_context_text"): + add_fragment("Structured Chart Data", node.to_context_text()) for label, attr_name in ( ("Prompt", "prompt"), @@ -784,36 +786,67 @@ def generate_chart(self, node, chart_type): ) return self._show_loading_animation(anchor_node=node) - self.chart_thread = ChartWorkerThread(chart_source_text, chart_type) - self.chart_thread.finished.connect(lambda data, emitted_chart_type, source_node=node: self.handle_chart_data(data, emitted_chart_type, source_node)) - self.chart_thread.error.connect(self.handle_error) - self.chart_thread.finished.connect(self.chart_thread.deleteLater) - self.chart_thread.error.connect(self.chart_thread.deleteLater) - self.chart_thread.start() + self._chart_generation_token = getattr(self, "_chart_generation_token", 0) + 1 + request_token = self._chart_generation_token + source_scene = node.scene() + thread = ChartWorkerThread(chart_source_text, chart_type) + self.chart_thread = thread + thread.finished.connect( + lambda data, emitted_chart_type, source_node=node, origin_scene=source_scene, token=request_token: + self.handle_chart_data(data, emitted_chart_type, source_node, token, origin_scene) + ) + thread.error.connect( + lambda message, token=request_token: self._handle_chart_error(message, token) + ) + thread.finished.connect(thread.deleteLater) + thread.error.connect(thread.deleteLater) + thread.start() except Exception as e: self.handle_error(f"Error generating chart: {str(e)}") - - def handle_chart_data(self, data, chart_type, source_node=None): + + def invalidate_chart_requests(self): + """Invalidate results from chart workers that belong to another chat.""" + self._chart_generation_token = getattr(self, "_chart_generation_token", 0) + 1 + + def _handle_chart_error(self, message, request_token): + if request_token == getattr(self, "_chart_generation_token", request_token): + self.handle_error(message) + + def handle_chart_data(self, data, chart_type, source_node=None, request_token=None, source_scene=None): + is_current_request = request_token is None or request_token == getattr(self, "_chart_generation_token", request_token) try: + if not is_current_request: + return chart_data = json.loads(data) if "error" in chart_data: self.notification_banner.show_message(chart_data["error"], 15000, "error") return scene = self.chat_view.scene() - if source_node and source_node.scene(): - chart_pos = QPointF(source_node.scenePos().x() + 450, source_node.scenePos().y()) - elif self.current_node and self.current_node.scene(): - chart_pos = QPointF(self.current_node.scenePos().x() + 450, self.current_node.scenePos().y()) - else: - chart_pos = QPointF(0, 0) - chart = scene.add_chart(chart_data, chart_pos, parent_content_node=source_node) + if source_node is None or (source_scene is not None and source_scene is not scene) or source_node.scene() is not scene: + self.notification_banner.show_message( + "Chart generation finished after its source chat was closed; the result was discarded.", + 8000, + "warning", + ) + return + chart_pos = QPointF(source_node.scenePos().x() + 450, source_node.scenePos().y()) + parent_node = scene.resolve_chart_parent(source_node) + if parent_node is None: + raise ValueError("The chart source is no longer attached to a conversational node.") + chart = scene.add_chart( + chart_data, + chart_pos, + parent_content_node=parent_node, + source_node=source_node, + ) self.current_node = chart self.chat_view.reveal_item(chart) self.save_chat() except Exception as e: self.handle_error(f"Error creating chart: {str(e)}") finally: - self._clear_loading_animation() + if is_current_request: + self._clear_loading_animation() def generate_image(self, node): try: diff --git a/graphlink_app/tests/test_chart_nodes.py b/graphlink_app/tests/test_chart_nodes.py new file mode 100644 index 0000000..6f576b4 --- /dev/null +++ b/graphlink_app/tests/test_chart_nodes.py @@ -0,0 +1,139 @@ +"""Regression coverage for chart data, rendering, and scene lifecycle behavior.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from PySide6.QtCore import QPointF + +from graphlink_agents_tools import ChartDataAgent +from graphlink_canvas.graphlink_canvas_chart_item import ChartItem +from graphlink_chart_data import ChartDataError, canonicalize_chart_data +from graphlink_scene import ChatScene +from graphlink_session.deserializers import SceneDeserializer +from graphlink_session.serializers import SceneSerializer + + +def _bar_data(**overrides): + data = { + "type": "bar", + "title": "Sales", + "labels": ["A", "B", "C"], + "values": [1, 2, 3], + } + data.update(overrides) + return data + + +def _scene(): + return ChatScene(MagicMock()) + + +def test_canonical_chart_data_rejects_wrong_containers_and_non_finite_numbers(): + with pytest.raises(ChartDataError, match="both be lists"): + canonicalize_chart_data(_bar_data(values={"A": 1}), "bar") + with pytest.raises(ChartDataError, match="finite"): + canonicalize_chart_data(_bar_data(values=[1, float("nan"), 3]), "bar") + + +def test_canonical_sankey_data_aggregates_duplicates_and_rejects_cycles(): + canonical = canonicalize_chart_data( + { + "type": "sankey", + "flows": [ + {"source": "A", "target": "B", "value": 1}, + {"source": "A", "target": "B", "value": 2}, + ], + } + ) + assert canonical["flows"] == [{"source": "A", "target": "B", "value": 3.0}] + + with pytest.raises(ChartDataError, match="cannot contain cycles"): + canonicalize_chart_data( + { + "type": "sankey", + "flows": [ + {"source": "A", "target": "B", "value": 1}, + {"source": "B", "target": "A", "value": 1}, + ], + } + ) + + +def test_agent_validator_uses_canonical_contract(): + agent = ChartDataAgent() + valid, error = agent.validate_chart_data(_bar_data(values="123"), "bar") + assert not valid + assert "both be lists" in error + + valid, error = agent.validate_chart_data( + {"type": "sankey", "flows": [{"source": "A", "target": "B", "value": float("nan")}]}, + "sankey", + ) + assert not valid + assert "finite" in error + + +def test_chart_scene_adds_provenance_connection_and_cascades_delete(): + scene = _scene() + parent = scene.add_chat_node("source", preferred_pos=QPointF(0, 0)) + chart = scene.add_chart(_bar_data(), QPointF(450, 0), parent_content_node=parent) + + assert chart.parent_content_node is parent + assert chart.source_node is parent + assert len(scene.chart_connections) == 1 + assert scene.chart_connections[0].start_node is parent + assert scene.chart_connections[0].end_node is chart + + scene.delete_chat_node(parent) + assert chart not in scene.chart_nodes + assert not scene.chart_connections + + +def test_chart_render_is_bounded_for_dense_labels_and_invalid_data_gets_placeholder(): + dense = _bar_data( + labels=[f"Category {index} with a long label" for index in range(50)], + values=list(range(1, 51)), + ) + chart = ChartItem(dense, QPointF(0, 0)) + assert not chart.chart_image.isNull() + + invalid = ChartItem(_bar_data(values="12"), QPointF(0, 0)) + assert invalid.data_error + assert not invalid.chart_image.isNull() + + +def test_chart_resize_uses_resize_start_ratio_and_has_upper_bound(): + chart = ChartItem(_bar_data(), QPointF(0, 0)) + chart.resizing = True + chart.resize_start_aspect_ratio = 2.0 + chart.set_chart_size(10000, 10000, preserve_aspect=True, rerender=False) + chart.resizing = False + + assert chart.width <= chart.MAX_WIDTH + assert chart.height <= chart.MAX_HEIGHT + assert chart.width / chart.height == pytest.approx(2.0, rel=0.01) + + +def test_chart_serialization_persists_stable_identity_and_parent_reference(): + window = MagicMock() + scene = ChatScene(window) + window.chat_view.scene.return_value = scene + parent = scene.add_chat_node("source", preferred_pos=QPointF(0, 0)) + chart = scene.add_chart(_bar_data(), QPointF(450, 0), parent_content_node=parent) + + serializer = SceneSerializer(window) + payload = serializer.serialize_chart(chart, [parent]) + + assert payload["id"] == chart.persistent_id + assert payload["parent_node_id"] == parent.persistent_id + assert payload["data"]["values"] == [1.0, 2.0, 3.0] + + restored_window = MagicMock() + restored_scene = ChatScene(restored_window) + restored_window.chat_view.scene.return_value = restored_scene + deserializer = SceneDeserializer(restored_window) + restored = deserializer.deserialize_chart(payload, restored_scene, {0: parent}) + assert restored.persistent_id == chart.persistent_id + assert restored.parent_content_node is None or restored.parent_content_node is parent + diff --git a/graphlink_app/tests/test_scene_node_list_helpers.py b/graphlink_app/tests/test_scene_node_list_helpers.py index 5dca961..3b3bcce 100644 --- a/graphlink_app/tests/test_scene_node_list_helpers.py +++ b/graphlink_app/tests/test_scene_node_list_helpers.py @@ -10,10 +10,8 @@ These tests populate a real ChatScene (constructible headlessly - its __init__ only needs a `window` reference, which it just stores) with representative items in every -relevant list and verify the refactored methods still consider exactly the same nodes -valid/matched as the original inline concatenations would have, including the -deliberate differences between call sites (e.g. update_search_highlight/ -update_connections exclude chart_nodes; find_items/nodeMoved include them). +relevant list and verify the refactored methods treat chart nodes consistently with +the other searchable/layout content nodes. """ import sys @@ -114,8 +112,8 @@ def test_does_not_match_unrelated_text(self): assert artifact not in results -class TestUpdateSearchHighlightExcludesChartNodes: - def test_syncs_flag_on_plugin_and_content_nodes_but_not_chart_nodes(self): +class TestUpdateSearchHighlightIncludesChartNodes: + def test_syncs_flag_on_plugin_content_and_chart_nodes(self): scene = _make_scene() artifact = _tag(is_search_match=False, update=lambda: None) code_item = _tag(is_search_match=False, update=lambda: None) @@ -128,9 +126,7 @@ def test_syncs_flag_on_plugin_and_content_nodes_but_not_chart_nodes(self): assert artifact.is_search_match is True assert code_item.is_search_match is True - # chart_nodes are deliberately excluded from this method's node set (matches - # the pre-refactor inline concatenation, which never included chart_nodes here) - assert chart_item.is_search_match is False + assert chart_item.is_search_match is True class TestNodeMovedTypeValidation: