From 9d39f417115e10df61d67f758fdbf6e02f41de25 Mon Sep 17 00:00:00 2001
From: Peng Ding <pding.dp@foxmail.com>
Date: Mon, 14 Sep 2026 20:53:33 -0500
Subject: [PATCH] fix: address review feedback on profiler flamegraph/icicle

- Raise ValueError when sort_by/limit passed with flamegraph/icicle
- Remove dead zoomStack code from JS (reset renders from root directly)
- Unify total time source: use total_tt consistently in flame HTML
- Add edge tuple format comment for CPython version differences
- Clarify tooltip label: "Cumulative (path)" vs ambiguous "Cumulative"
- Add empty-profile and sort_by/limit rejection tests for flamegraph
---
 profiler/profiler.py                  | 13 +++++++------
 profiler/test_profiler_correctness.py | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/profiler/profiler.py b/profiler/profiler.py
index a15851d..be071df 100644
--- a/profiler/profiler.py
+++ b/profiler/profiler.py
@@ -261,6 +261,8 @@ def output_html(
             )
 
         if style in ("flamegraph", "icicle"):
+            if sort_by is not None or limit is not None:
+                raise ValueError("sort_by and limit only apply to style='table'")
             tree = self._extract_call_tree()
             doc = self._build_flame_html(tree, title, inverted=(style == "icicle"))
         else:
@@ -357,6 +359,7 @@ def _build_node(
             for callee_key, edge in callees.items():
                 if callee_key in visited:
                     continue
+                # edge is (cc,nc,tt,ct) or (nc,ct) depending on CPython version
                 child_ct = edge[3] if len(edge) == 4 else edge[1]
                 children.append(
                     _build_node(callee_key, visited | {callee_key}, child_ct)
@@ -391,7 +394,8 @@ def _build_flame_html(
         """Build a self-contained flamegraph or icicle chart HTML document."""
         import json as _json
 
-        total_time = self.total_time or 1e-9
+        # Use CPU total_tt (same as _extract_call_tree) for consistency
+        total_time = cast(Any, self._stats).total_tt or 1e-9
         style_name = "Icicle Chart" if inverted else "Flamegraph"
 
         return (
@@ -689,7 +693,6 @@ def _build_table_html(self, rows: list[dict[str, Any]], title: str) -> str:
     return COLORS[Math.abs(h)%COLORS.length];
   }
 
-  var zoomStack=[];
   var inverted=container.classList.contains('inverted');
 
   function flatten(nodes,depth,parentLeft,parentWidth,totalTime,arr){
@@ -747,9 +750,8 @@ def _build_table_html(self, rows: list[dict[str, Any]], title: str) -> str:
     }
   }
 
-  function onFrameClick(e){
+  function onFrameClick(){
     var node=this._node;
-    zoomStack.push({data:FLAME_DATA,total:TOTAL_TIME});
     render([node],node.cumtime);
   }
 
@@ -757,7 +759,7 @@ def _build_table_html(self, rows: list[dict[str, Any]], title: str) -> str:
     var el=this;
     tooltip.innerHTML='<b>'+escH(el.getAttribute('data-name'))+'</b><br>'
       +escH(el.getAttribute('data-file'))+'<br>'
-      +'Cumulative: '+el.getAttribute('data-cumtime')+'s ('
+      +'Cumulative (path): '+el.getAttribute('data-cumtime')+'s ('
       +el.getAttribute('data-pct')+'%)<br>'
       +'Calls: '+el.getAttribute('data-calls');
     tooltip.style.display='block';
@@ -775,7 +777,6 @@ def _build_table_html(self, rows: list[dict[str, Any]], title: str) -> str:
   }
 
   document.getElementById('reset-zoom').addEventListener('click',function(){
-    zoomStack=[];
     render(FLAME_DATA,TOTAL_TIME);
   });
 
diff --git a/profiler/test_profiler_correctness.py b/profiler/test_profiler_correctness.py
index 3fb7760..651b8a0 100644
--- a/profiler/test_profiler_correctness.py
+++ b/profiler/test_profiler_correctness.py
@@ -579,6 +579,25 @@ def test_flamegraph_subtitle(self):
         assert "Flamegraph" in html
         assert "Total time:" in html
 
+    def test_flamegraph_empty_profile(self):
+        with Profiler() as p:
+            pass
+        html = p.output_html(style="flamegraph")
+        assert "<!DOCTYPE html>" in html
+        assert "FLAME_DATA" in html
+
+    def test_flamegraph_sort_by_raises(self):
+        with Profiler() as p:
+            _busy_work()
+        with pytest.raises(ValueError, match="sort_by and limit"):
+            p.output_html(style="flamegraph", sort_by="tottime")
+
+    def test_flamegraph_limit_raises(self):
+        with Profiler() as p:
+            _busy_work()
+        with pytest.raises(ValueError, match="sort_by and limit"):
+            p.output_html(style="flamegraph", limit=10)
+
 
 # -- TestIcicleOutput --------------------------------------------------------