font size slider for visualizer#256
Merged
ParticularlyPythonicBS merged 4 commits intounstablefrom Mar 27, 2026
Merged
Conversation
ParticularlyPythonicBS
added
Maintenance
WalkthroughAdds a "Visual Settings" font-size slider wired into the vis-network rendering path, exposes graph payloads to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Slider as Font Slider
participant Script as graph_script.js
participant DataSets as DataSets (nodes/edges)
participant Network as vis.Network
participant Debug as window.__graph
User->>Slider: adjust font-size
Slider->>Script: input handler -> set visualState.fontSize
Script->>DataSets: addWithCurrentFontSize(newNodes,newEdges) — apply node.font.size
Script->>Network: network.setOptions(...) (global node/edge fonts, edge align)
Script->>Network: network.redraw()
Network-->>User: re-rendered graph with updated label sizes
alt debug enabled
Script->>Debug: expose data, nodes, edges, options
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@temoa/utilities/graph_utils.py`:
- Around line 234-241: The iterator all_edges is being consumed by num_edges =
sum(1 for _ in all_edges) causing the later loop for edge_tuple in all_edges to
see no items; instead materialize the iterable at the start (e.g., convert
all_edges to a list) before counting so num_edges and the subsequent loop both
operate on the same concrete sequence—update references around num_edges,
layout_radius, jitter_radius, and the for edge_tuple in all_edges loop to use
the materialized collection.
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 35-42: The code attaches internal state to a global by assigning
window.__graph = { data, allNodesPrimary, allEdgesPrimary, allNodesSecondary,
allEdgesSecondary, optionsObject };; either remove this assignment before
production or gate it behind a clear flag/environment check and add a short
comment describing its debugging purpose. Update the block around the
window.__graph assignment (the symbol window.__graph and the variables data,
allNodesPrimary, allEdgesPrimary, allNodesSecondary, allEdgesSecondary,
optionsObject) to only export when a DEBUG/DEV_MODE flag is true or when
process.env.NODE_ENV !== 'production', and include a one-line comment stating
“Expose for debugging only — disable in production.”
In `@temoa/utilities/network_vis_templates/graph_template.html`:
- Around line 20-26: Summary: tighten the font-size slider range to a more
practical maximum. Update the input element with id "font-size-slider" to reduce
the max from 100 to 30 (and optionally add step="1" for finer control) while
keeping value="14" to match DEFAULT_VIS_OPTIONS; ensure any JS that reads the
slider (e.g., handlers referencing "font-size-slider") still accepts the new
range.
In `@tests/test_commodity_visualizer.py`:
- Around line 16-23: Replace the MagicMock usage in
tests/test_commodity_visualizer.py with a concrete NetworkModelData instance so
attribute lookups behave like the real class; instantiate NetworkModelData(),
then set network_data.physical_commodities = {'comm_inter'}, assign
network_data.source_commodities[(region, period)] = {'comm_source'} and
network_data.demand_commodities[(region, period)] = {'comm_demand'} and ensure
network_data.available_techs and network_data.tech_data use the class defaults
(or populate them minimally) before passing network_data into the
CommodityVisualizer test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 50-52: visualState.fontSize is hardcoded to 14 while optionsObject
(from options_json_str) may specify a different font size, causing a UI/render
mismatch; update the initialization so visualState.fontSize =
(optionsObject?.font_size ?? 14) and set the slider's initial value to that same
value during the UI setup (use the same slider variable or DOM element used
where the slider is created) so both the rendered network and the displayed
slider are synchronized; locate references to visualState, optionsObject, and
the slider initialization code (where the slider value is set) and replace the
hardcoded 14 with the derived value and apply it to the slider control.
b5cb1b8 to
5f7428f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/source/default/static/Network_Graph_utopia_1990.html`:
- Around line 20-26: The range input with id "font-size-slider" is missing the
step attribute; update the element (identified by id "font-size-slider") to
include step="1" to match graph_template.html and ensure consistent behavior
across templates.
5f7428f to
480c3b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_commodity_visualizer.py`:
- Around line 76-86: The test uses next(e for e in edges if ...) which raises
StopIteration with a poor message; change the lookup for edge_do, edge_oo, and
edge_dt to perform a safe search (e.g., use a list comprehension or a helper
like find_edge(edges, src, dst)) and then assert that the result is not None (or
that the list is non-empty) before checking ['dashes'] and ['color']; update the
assertions so a missing edge yields a clear failure message indicating which
source/destination pair (comm_inter->comm_demand, comm_source->comm_inter,
comm_source->comm_demand) was not found.
480c3b7 to
8611764
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/source/default/static/Network_Graph_utopia_1990.html (1)
20-26:⚠️ Potential issue | 🟡 MinorMissing
stepattribute on slider for consistency.The font-size slider is missing
step="1"which is present ingraph_template.html. This was flagged in a previous review.🔧 Proposed fix
- <input type="range" id="font-size-slider" min="6" max="100" value="14"> + <input type="range" id="font-size-slider" min="6" max="100" step="1" value="14">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/default/static/Network_Graph_utopia_1990.html` around lines 20 - 26, The font-size slider input with id "font-size-slider" is missing the step attribute; update the <input id="font-size-slider"> element to include step="1" (matching graph_template.html) so the slider increments by whole pixels for consistent behavior across templates.temoa/utilities/network_vis_templates/graph_script.js (1)
50-52:⚠️ Potential issue | 🟡 MinorSynchronize slider initial value with backend font size defaults.
visualState.fontSizeis hardcoded to 14, butoptionsObject(from the backend) may specify a different font size. This creates a potential mismatch where the network initializes with the size fromoptionsObjectwhile the slider shows 14. This was flagged in a previous review.🔧 Proposed fix to sync with backend defaults
+ const parsedFontSize = parseInt( + optionsObject?.nodes?.font?.size ?? optionsObject?.edges?.font?.size ?? 14, + 10, + ); + const defaultFontSize = Number.isFinite(parsedFontSize) ? parsedFontSize : 14; let visualState = { - fontSize: 14 + fontSize: defaultFontSize };And after line 60:
if (fontSizeSlider) fontSizeSlider.value = String(defaultFontSize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 50 - 52, The visualState.fontSize is hardcoded to 14 causing a mismatch with backend-provided defaults; update initialization so visualState.fontSize is set from the backend default (use the same variable that holds the backend value, e.g., defaultFontSize or optionsObject.fontSize) and after creating the fontSizeSlider set fontSizeSlider.value = String(defaultFontSize) (or the corresponding optionsObject value) so both the network rendering and the slider reflect the backend default; update references to visualState and fontSizeSlider to use that backend-derived value.tests/test_commodity_visualizer.py (1)
76-86: 🧹 Nitpick | 🔵 TrivialConsider explicit assertions for edge lookup to improve test failure messages.
Using
next(e for e in edges if ...)raisesStopIterationwith an unhelpful error message if no matching edge is found. This was previously flagged in review.♻️ Suggested improvement for clearer test failures
- edge_do = next(e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand')) + edge_do_list = [e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand')] + assert len(edge_do_list) == 1, "Expected 1 edge from comm_inter to comm_demand" + edge_do = edge_do_list[0] assert edge_do[2]['dashes'] is True assert edge_do[2]['color'] == '#d62728' - edge_oo = next(e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_inter')) + edge_oo_list = [e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_inter')] + assert len(edge_oo_list) == 1, "Expected 1 edge from comm_source to comm_inter" + edge_oo = edge_oo_list[0] assert edge_oo[2]['dashes'] is True assert edge_oo[2]['color'] == '#ff7f0e' - edge_dt = next(e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_demand')) + edge_dt_list = [e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_demand')] + assert len(edge_dt_list) == 1, "Expected 1 edge from comm_source to comm_demand" + edge_dt = edge_dt_list[0] assert edge_dt[2]['dashes'] is True assert edge_dt[2]['color'] == '#1f77b4'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commodity_visualizer.py` around lines 76 - 86, Replace the bare use of next(...) that can raise StopIteration with an explicit existence check before assertions: for each lookup of edges (the variables edge_do, edge_oo, edge_dt), first find the element using next(..., None) or assert any(...) to ensure a matching edge exists, assert the found value is not None with a helpful message (including the expected (source, target) pair), then assert its ['dashes'] and ['color'] values; reference the edges collection and the lookup expressions (e.g., next(e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand'))) when applying the checks so failures show which edge was missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 83-101: The updateVisualSettings function sets a global font via
network.setOptions but then forcibly updates per-node fonts with nodes.update
(likely to override per-node fonts created by addWithCurrentFontSize); either
remove the nodes.update block to avoid redundancy or keep it and add a
clarifying comment referencing addWithCurrentFontSize and visualState.fontSize
to explain that per-node font properties must be overwritten because they would
otherwise take precedence over the global setting; update or remove the
nodes.update(...) call and ensure network.setOptions({ nodes: { font: { size:
visualState.fontSize } }, edges: { font: { size: visualState.fontSize } } })
remains to apply the global change.
---
Duplicate comments:
In `@docs/source/default/static/Network_Graph_utopia_1990.html`:
- Around line 20-26: The font-size slider input with id "font-size-slider" is
missing the step attribute; update the <input id="font-size-slider"> element to
include step="1" (matching graph_template.html) so the slider increments by
whole pixels for consistent behavior across templates.
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 50-52: The visualState.fontSize is hardcoded to 14 causing a
mismatch with backend-provided defaults; update initialization so
visualState.fontSize is set from the backend default (use the same variable that
holds the backend value, e.g., defaultFontSize or optionsObject.fontSize) and
after creating the fontSizeSlider set fontSizeSlider.value =
String(defaultFontSize) (or the corresponding optionsObject value) so both the
network rendering and the slider reflect the backend default; update references
to visualState and fontSizeSlider to use that backend-derived value.
In `@tests/test_commodity_visualizer.py`:
- Around line 76-86: Replace the bare use of next(...) that can raise
StopIteration with an explicit existence check before assertions: for each
lookup of edges (the variables edge_do, edge_oo, edge_dt), first find the
element using next(..., None) or assert any(...) to ensure a matching edge
exists, assert the found value is not None with a helpful message (including the
expected (source, target) pair), then assert its ['dashes'] and ['color']
values; reference the edges collection and the lookup expressions (e.g., next(e
for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand'))) when
applying the checks so failures show which edge was missing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/source/default/static/Network_Graph_utopia_1990.htmltemoa/utilities/graph_utils.pytemoa/utilities/network_vis_templates/graph_script.jstemoa/utilities/network_vis_templates/graph_styles.csstemoa/utilities/network_vis_templates/graph_template.htmltemoa/utilities/visualizer.pytests/test_commodity_visualizer.py
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
2 times, most recently
from
555a358 to
6036036
Compare
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
3 times, most recently
from
f772f84 to
f61e425
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
temoa/utilities/network_vis_templates/graph_script.js (1)
50-52: 🧹 Nitpick | 🔵 TrivialHardcoded font size may mismatch backend configuration.
visualState.fontSizeis hardcoded to14, butoptionsObject(from backendoptions_json_str) could specify a different font size. While currently both are14, if the backend default changes, the slider's displayed value would not match the rendered font size on initial load.Consider initializing from
optionsObjectwith a fallback:♻️ Suggested improvement for synchronization
+ const defaultFontSize = optionsObject?.nodes?.font?.size ?? 14; let visualState = { - fontSize: 14 + fontSize: defaultFontSize };And synchronize the slider's initial value after line 60:
if (fontSizeSlider) fontSizeSlider.value = String(defaultFontSize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 50 - 52, visualState.fontSize is hardcoded to 14 causing mismatch if backend options_json_str specifies a different font size; change initialization of visualState (the visualState object) to read font size from optionsObject (e.g., optionsObject.fontSize or computed defaultFontSize) with a fallback to 14, and after slider creation ensure fontSizeSlider.value is set to that defaultFontSize so the slider and rendered font are synchronized.tests/test_commodity_visualizer.py (1)
76-86: 🧹 Nitpick | 🔵 TrivialConsider explicit assertions for clearer test failure messages.
The
next(e for e in edges if ...)pattern raisesStopIterationwith an unhelpful message if no matching edge exists. Using a list comprehension with an explicit assertion would provide clearer failure diagnostics.♻️ Suggested improvement for clearer test failures
- edge_do = next(e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand')) + edge_do_list = [e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand')] + assert len(edge_do_list) == 1, f"Expected 1 edge from comm_inter to comm_demand, found {len(edge_do_list)}" + edge_do = edge_do_list[0] assert edge_do[2]['dashes'] is True assert edge_do[2]['color'] == '#d62728' - edge_oo = next(e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_inter')) + edge_oo_list = [e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_inter')] + assert len(edge_oo_list) == 1, f"Expected 1 edge from comm_source to comm_inter, found {len(edge_oo_list)}" + edge_oo = edge_oo_list[0] assert edge_oo[2]['dashes'] is True assert edge_oo[2]['color'] == '#ff7f0e' - edge_dt = next(e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_demand')) + edge_dt_list = [e for e in edges if (e[0] == 'comm_source' and e[1] == 'comm_demand')] + assert len(edge_dt_list) == 1, f"Expected 1 edge from comm_source to comm_demand, found {len(edge_dt_list)}" + edge_dt = edge_dt_list[0] assert edge_dt[2]['dashes'] is True assert edge_dt[2]['color'] == '#1f77b4'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commodity_visualizer.py` around lines 76 - 86, Replace the fragile next(e for e in edges if ...) calls with a safer two-step pattern: first collect matches via a list comprehension (e.g., matches = [e for e in edges if e[0] == 'comm_inter' and e[1] == 'comm_demand']), assert that matches is not empty with an explicit message (so test failures show which edge is missing), then use matches[0] for subsequent property assertions on edge_do / edge_oo / edge_dt; alternatively use next(..., None) and assert the result is not None before checking ['dashes'] and ['color'] to avoid StopIteration and get clearer diagnostics.docs/source/default/static/Network_Graph_utopia_1990.html (1)
20-26:⚠️ Potential issue | 🟡 MinorMissing
stepattribute on slider creates inconsistency with template.The font-size slider here is missing
step="1"which is present ingraph_template.html(line 24). While browsers default tostep=1for range inputs, adding it maintains consistency across templates.🔧 Proposed fix
- <input type="range" id="font-size-slider" min="6" max="100" value="14"> + <input type="range" id="font-size-slider" min="6" max="100" step="1" value="14">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/default/static/Network_Graph_utopia_1990.html` around lines 20 - 26, The range input with id "font-size-slider" is missing the step attribute and should include step="1" to match the template; locate the <input> element with id "font-size-slider" and add the step="1" attribute so the element definition becomes consistent with graph_template.html's slider configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 35-45: The current guard using typeof process... never evaluates
true in browser, so either remove the block or replace the check with an
explicit browser-friendly debug flag; for example, detect a URL param or global
flag and only expose window.__graph when present: check
URLSearchParams(window.location.search).has('debugGraph') or a global
window.DEBUG_GRAPH before assigning window.__graph (refer to the existing
window.__graph, data, allNodesPrimary, allEdgesPrimary, allNodesSecondary,
allEdgesSecondary, optionsObject symbols).
---
Duplicate comments:
In `@docs/source/default/static/Network_Graph_utopia_1990.html`:
- Around line 20-26: The range input with id "font-size-slider" is missing the
step attribute and should include step="1" to match the template; locate the
<input> element with id "font-size-slider" and add the step="1" attribute so the
element definition becomes consistent with graph_template.html's slider
configuration.
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 50-52: visualState.fontSize is hardcoded to 14 causing mismatch if
backend options_json_str specifies a different font size; change initialization
of visualState (the visualState object) to read font size from optionsObject
(e.g., optionsObject.fontSize or computed defaultFontSize) with a fallback to
14, and after slider creation ensure fontSizeSlider.value is set to that
defaultFontSize so the slider and rendered font are synchronized.
In `@tests/test_commodity_visualizer.py`:
- Around line 76-86: Replace the fragile next(e for e in edges if ...) calls
with a safer two-step pattern: first collect matches via a list comprehension
(e.g., matches = [e for e in edges if e[0] == 'comm_inter' and e[1] ==
'comm_demand']), assert that matches is not empty with an explicit message (so
test failures show which edge is missing), then use matches[0] for subsequent
property assertions on edge_do / edge_oo / edge_dt; alternatively use next(...,
None) and assert the result is not None before checking ['dashes'] and ['color']
to avoid StopIteration and get clearer diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9aa4684-bbe9-496d-8348-d583fbecc57b
📒 Files selected for processing (7)
docs/source/default/static/Network_Graph_utopia_1990.htmltemoa/utilities/graph_utils.pytemoa/utilities/network_vis_templates/graph_script.jstemoa/utilities/network_vis_templates/graph_styles.csstemoa/utilities/network_vis_templates/graph_template.htmltemoa/utilities/visualizer.pytests/test_commodity_visualizer.py
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
from
f61e425 to
a3d2735
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
temoa/utilities/network_vis_templates/graph_script.js (1)
32-33:⚠️ Potential issue | 🟡 MinorGuard the compatibility parse or remove it.
Line 33 has an unguarded
JSON.parse(optionsRaw)that throws uncaught if the string is malformed. The graph-data parsing pattern (lines 6–17) demonstrates the appropriate fail-fast approach. Wrap the parse in try-catch and log errors consistently, or remove the string branch if it is no longer needed. Additionally, line 74 accessesoptionsObject.configurewithout null/undefined protection; this also requires fixing.🛡️ Proposed hardening
- const optionsObject = - typeof optionsRaw === "string" ? JSON.parse(optionsRaw) : optionsRaw; + let optionsObject = optionsRaw; + if (typeof optionsRaw === 'string') { + try { + optionsObject = JSON.parse(optionsRaw); + } catch (e) { + console.error('Failed to parse visualization options:', e); + return; + } + } + if (!optionsObject) { + console.error('Visualization options are missing. Halting script.'); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 32 - 33, The unguarded JSON.parse on optionsRaw can throw and optionsObject.configure is accessed without null checks; wrap the parse of optionsRaw in a try/catch (when typeof optionsRaw === "string") and log the parse error consistently with the file's existing logger, then fall back to a safe default (e.g., {} or optionsRaw if already an object). After creation, ensure any uses of optionsObject.configure (and similar properties) are guarded—use null/undefined checks or optional chaining and provide sensible defaults before using the value. Update the code around the optionsObject assignment and the place that reads optionsObject.configure to implement these guards and consistent error logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/utilities/graph_utils.py`:
- Around line 237-245: The code currently computes layout_radius from num_edges
and repeatedly assigns positions[edge_tuple.tech] inside the edge loop, causing
duplicate placement work and overwrites; change the logic to first build a
unique set/list of technologies to place (from edges in all_edges_list or
sectors_to_place) and iterate over that unique collection when creating
sector_anchors and populating positions, compute num_sectors/num_nodes from the
unique tech count (not total edges) to scale layout_radius/jitter_radius
appropriately, and before assigning positions in the placement loop check/skip
if positions already contains the tech (or only iterate unique techs) to avoid
reprocessing and memory churn (refer to positions, edge_tuple.tech,
sectors_to_place, all_edges_list, layout_radius, jitter_radius).
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 88-110: The slider currently calls
nodes.get()/nodes.update()/network.redraw() on every input, causing O(N) work
per event; throttle these heavy updates to one per animation frame by
introducing a pendingRaf (or rafId) variable and scheduling the actual render
work inside requestAnimationFrame. Keep quick work (parseInt of
fontSizeSlider.value and updating visualState.fontSize) in the input handler or
at the top of updateVisualSettings, but replace the direct
nodes.get()/nodes.update()/network.redraw() calls in updateVisualSettings with a
requestAnimationFrame wrapper: the input event should only call a scheduler that
sets pendingRaf = requestAnimationFrame(() => { pendingRaf = null; perform
nodes.get(), nodes.update(nodeUpdates), network.redraw(), and network.setOptions
as needed }); ensure the event listener for fontSizeSlider uses this scheduler
and cancels/ignores extra frames while one is pending to guarantee at most one
render per frame (refer to updateVisualSettings,
fontSizeSlider.addEventListener, nodes.get, nodes.update, network.redraw).
---
Duplicate comments:
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 32-33: The unguarded JSON.parse on optionsRaw can throw and
optionsObject.configure is accessed without null checks; wrap the parse of
optionsRaw in a try/catch (when typeof optionsRaw === "string") and log the
parse error consistently with the file's existing logger, then fall back to a
safe default (e.g., {} or optionsRaw if already an object). After creation,
ensure any uses of optionsObject.configure (and similar properties) are
guarded—use null/undefined checks or optional chaining and provide sensible
defaults before using the value. Update the code around the optionsObject
assignment and the place that reads optionsObject.configure to implement these
guards and consistent error logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6988dbd-e89b-473e-a394-0a315e87dda2
📒 Files selected for processing (5)
docs/source/default/static/Network_Graph_utopia_1990.htmltemoa/utilities/graph_utils.pytemoa/utilities/network_vis_templates/graph_script.jstemoa/utilities/network_vis_templates/graph_template.htmltests/test_commodity_visualizer.py
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
from
a3d2735 to
cae9241
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/utilities/network_vis_templates/graph_script.js (1)
83-95:⚠️ Potential issue | 🟡 MinorKeep the panel toggle independent from
configure.enabled.The header button is always rendered, but this block only binds its click handler when advanced physics controls are enabled. If a caller sets
configure.enabled = false, the panel can no longer be collapsed and the "Show Advanced Physics Controls" link becomes a dead control.🔧 Proposed fix
- if (optionsObject?.configure?.enabled) { - optionsObject.configure.container = visConfigContainer; - configHeader.addEventListener('click', () => { - const isCollapsed = configWrapper.classList.toggle('collapsed'); - configToggleButton.setAttribute('aria-expanded', !isCollapsed); - }); + configHeader.addEventListener('click', () => { + const isCollapsed = configWrapper.classList.toggle('collapsed'); + configToggleButton.setAttribute('aria-expanded', String(!isCollapsed)); + }); + if (optionsObject?.configure?.enabled) { + optionsObject.configure.container = visConfigContainer; advancedControlsToggle.addEventListener('click', function(e) { e.preventDefault(); const isHidden = visConfigContainer.style.display === 'none'; visConfigContainer.style.display = isHidden ? 'block' : 'none'; this.textContent = isHidden ? 'Hide Advanced Physics Controls' : 'Show Advanced Physics Controls'; }); + } else { + advancedControlsToggle.style.display = 'none'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 83 - 95, The collapse/collapse-button behavior is only bound when optionsObject?.configure?.enabled is true, which leaves the header button (configHeader/configToggleButton/configWrapper) non-functional if configure.enabled is false; move the configHeader.addEventListener block (toggling configWrapper.classList and updating configToggleButton aria-expanded) outside the configure.enabled conditional so the panel collapse works always, while keeping the advancedControlsToggle click handler (which shows/hides visConfigContainer and updates its textContent) bound only when optionsObject?.configure?.enabled is true and only if advancedControlsToggle and visConfigContainer exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/utilities/graph_utils.py`:
- Around line 258-263: tech_to_sector currently built with a dict comprehension
over all_edges_list captures the last seen sector for a tech, causing mismatch
with generate_technology_graph which expects the first non-empty sector; replace
the comprehension with an explicit loop over all_edges_list that sets
tech_to_sector[edge.tech] only if edge.tech is not already present and
edge.sector is non-empty (i.e., preserve the first non-empty sector
encountered), referencing tech_to_sector, all_edges_list, unique_techs_to_place
and generate_technology_graph to locate and align the behavior.
In `@tests/test_commodity_visualizer.py`:
- Around line 20-50: The test currently misses a commodity that is only present
in driven_techs, so the blue-border "Connected to Driven Tech" styling isn't
exercised; add a new EdgeTuple (e.g., Commodity('comm_only_driven')) to the
driven_techs list and ensure this commodity is not included in other_orphans or
demand_orphans (use the same EdgeTuple structure as existing entries with
region, Technology('tech_driven'), Vintage(2020), Commodity('comm_demand'),
sector=Sector('Sx')) so the test asserts the blue styling path for
comm_only_driven.
---
Outside diff comments:
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 83-95: The collapse/collapse-button behavior is only bound when
optionsObject?.configure?.enabled is true, which leaves the header button
(configHeader/configToggleButton/configWrapper) non-functional if
configure.enabled is false; move the configHeader.addEventListener block
(toggling configWrapper.classList and updating configToggleButton aria-expanded)
outside the configure.enabled conditional so the panel collapse works always,
while keeping the advancedControlsToggle click handler (which shows/hides
visConfigContainer and updates its textContent) bound only when
optionsObject?.configure?.enabled is true and only if advancedControlsToggle and
visConfigContainer exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e670639e-3f93-4641-9a79-449a047c827f
📒 Files selected for processing (5)
docs/source/default/static/Network_Graph_utopia_1990.htmltemoa/utilities/graph_utils.pytemoa/utilities/network_vis_templates/graph_script.jstemoa/utilities/network_vis_templates/graph_template.htmltests/test_commodity_visualizer.py
Comment on lines
+258
to
+263
| # 3. Place each unique technology node near its sector's anchor point with jitter | ||
| # Create a mapping of tech to its primary sector from the edges | ||
| tech_to_sector = {edge.tech: edge.sector for edge in all_edges_list if edge.tech} | ||
|
|
||
| for tech in unique_techs_to_place: | ||
| primary_sector = tech_to_sector.get(tech) |
There was a problem hiding this comment.
Preserve the first non-empty sector for each technology.
tech_to_sector = { ... } keeps the last sector seen for a tech, but generate_technology_graph later keeps the first one when it builds node metadata. If a technology shows up with mixed or missing sector values, the node can be positioned under one sector anchor and rendered as another.
🔧 Proposed fix
- tech_to_sector = {edge.tech: edge.sector for edge in all_edges_list if edge.tech}
+ tech_to_sector = {}
+ for edge in all_edges_list:
+ if edge.tech and edge.sector and edge.tech not in tech_to_sector:
+ tech_to_sector[edge.tech] = edge.sector🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/utilities/graph_utils.py` around lines 258 - 263, tech_to_sector
currently built with a dict comprehension over all_edges_list captures the last
seen sector for a tech, causing mismatch with generate_technology_graph which
expects the first non-empty sector; replace the comprehension with an explicit
loop over all_edges_list that sets tech_to_sector[edge.tech] only if edge.tech
is not already present and edge.sector is non-empty (i.e., preserve the first
non-empty sector encountered), referencing tech_to_sector, all_edges_list,
unique_techs_to_place and generate_technology_graph to locate and align the
behavior.
Comment on lines
+20
to
+50
| # Define some special items | ||
| demand_orphans = [ | ||
| EdgeTuple( | ||
| region, | ||
| Commodity('comm_inter'), | ||
| Technology('tech_demand_orphan'), | ||
| Vintage(2020), | ||
| Commodity('comm_demand'), | ||
| sector=Sector('S1'), | ||
| ) | ||
| ] | ||
| other_orphans = [ | ||
| EdgeTuple( | ||
| region, | ||
| Commodity('comm_source'), | ||
| Technology('tech_other_orphan'), | ||
| Vintage(2020), | ||
| Commodity('comm_inter'), | ||
| sector=Sector('S2'), | ||
| ) | ||
| ] | ||
| driven_techs = [ | ||
| EdgeTuple( | ||
| region, | ||
| Commodity('comm_source'), | ||
| Technology('tech_driven'), | ||
| Vintage(2020), | ||
| Commodity('comm_demand'), | ||
| sector=Sector('S3'), | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Add a commodity that's only styled by driven_techs.
This setup never asserts the blue-border node path. comm_source is also in other_orphans, so orange wins, and the remaining node assertions hit the red branch. A regression in the Connected to Driven Tech node styling would still pass this test.
Also applies to: 62-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_commodity_visualizer.py` around lines 20 - 50, The test currently
misses a commodity that is only present in driven_techs, so the blue-border
"Connected to Driven Tech" styling isn't exercised; add a new EdgeTuple (e.g.,
Commodity('comm_only_driven')) to the driven_techs list and ensure this
commodity is not included in other_orphans or demand_orphans (use the same
EdgeTuple structure as existing entries with region, Technology('tech_driven'),
Vintage(2020), Commodity('comm_demand'), sector=Sector('Sx')) so the test
asserts the blue styling path for comm_only_driven.
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
from
cae9241 to
64f0278
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_commodity_visualizer.py (1)
20-50: 🧹 Nitpick | 🔵 TrivialTest does not exercise the "Connected to Driven Tech" node styling path.
The current setup has overlapping commodities across all three special categories:
comm_sourceis in bothother_orphansanddriven_techs→ orange winscomm_demandis in bothdemand_orphansanddriven_techs→ red winsNo commodity is exclusively styled by
driven_techs, so a regression in the blue-border node path would go undetected.Consider adding a fourth commodity (e.g.,
comm_driven_only) that appears only indriven_techs:♻️ Suggested addition
+ # Add a commodity that only appears in driven_techs + network_data.physical_commodities.add(Commodity('comm_driven_only')) + driven_techs = [ EdgeTuple( region, Commodity('comm_source'), Technology('tech_driven'), Vintage(2020), Commodity('comm_demand'), sector=Sector('S3'), - ) + ), + EdgeTuple( + region, + Commodity('comm_driven_only'), + Technology('tech_driven_exclusive'), + Vintage(2020), + Commodity('comm_inter'), + sector=Sector('S3'), + ), ]Then add assertion for the blue border:
assert dg.nodes['comm_driven_only']['color']['border'] == '#1f77b4' assert dg.nodes['comm_driven_only']['borderWidth'] == 4 assert 'Connected to Driven Tech' in dg.nodes['comm_driven_only']['title']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_commodity_visualizer.py` around lines 20 - 50, The test never covers the "Connected to Driven Tech" styling because every commodity in driven_techs overlaps another category; add a new driven-only commodity by appending an EdgeTuple to driven_techs using Commodity('comm_driven_only') (e.g., EdgeTuple(region, Commodity('comm_driven_only'), Technology('tech_driven_only'), Vintage(2020), Commodity('comm_demand'), sector=Sector('Sx'))) so it appears only in driven_techs, then add assertions against dg.nodes['comm_driven_only'] that its color.border == '#1f77b4', borderWidth == 4, and that its title contains 'Connected to Driven Tech' to exercise the blue-border path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/utilities/graph_utils.py`:
- Around line 238-240: The sorting uses an unnecessary lambda; replace the
key=lambda t: str(t) with key=str in the unique_techs_to_place construction to
simplify the call that builds sorted({edge.tech for edge in all_edges_list if
edge.tech}); update the expression that assigns to unique_techs_to_place
accordingly (refer to unique_techs_to_place, all_edges_list, and edge.tech).
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Line 44: Update the inline comment that currently reads "// Expose for
debugging only — enable in production." to correctly state the intent by
changing it to "// Expose for debugging only — disable in production." so it
warns that the exposed debug surface should not be enabled in production; locate
the comment string in graph_script.js and replace it accordingly.
---
Duplicate comments:
In `@tests/test_commodity_visualizer.py`:
- Around line 20-50: The test never covers the "Connected to Driven Tech"
styling because every commodity in driven_techs overlaps another category; add a
new driven-only commodity by appending an EdgeTuple to driven_techs using
Commodity('comm_driven_only') (e.g., EdgeTuple(region,
Commodity('comm_driven_only'), Technology('tech_driven_only'), Vintage(2020),
Commodity('comm_demand'), sector=Sector('Sx'))) so it appears only in
driven_techs, then add assertions against dg.nodes['comm_driven_only'] that its
color.border == '#1f77b4', borderWidth == 4, and that its title contains
'Connected to Driven Tech' to exercise the blue-border path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1842cb8d-16bf-48de-8760-971773e2314a
📒 Files selected for processing (7)
docs/source/default/static/Network_Graph_utopia_1990.htmltemoa/utilities/graph_utils.pytemoa/utilities/network_vis_templates/graph_script.jstemoa/utilities/network_vis_templates/graph_styles.csstemoa/utilities/network_vis_templates/graph_template.htmltemoa/utilities/visualizer.pytests/test_commodity_visualizer.py
Comment on lines
+238
to
+240
| unique_techs_to_place = sorted( | ||
| {edge.tech for edge in all_edges_list if edge.tech}, key=lambda t: str(t) | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the lambda to key=str.
The lambda lambda t: str(t) is equivalent to just str.
♻️ Proposed simplification
- unique_techs_to_place = sorted(
- {edge.tech for edge in all_edges_list if edge.tech}, key=lambda t: str(t)
- )
+ unique_techs_to_place = sorted(
+ {edge.tech for edge in all_edges_list if edge.tech}, key=str
+ )🧰 Tools
🪛 Ruff (0.15.7)
[warning] 239-239: Lambda may be unnecessary; consider inlining inner function
Inline function call
(PLW0108)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/utilities/graph_utils.py` around lines 238 - 240, The sorting uses an
unnecessary lambda; replace the key=lambda t: str(t) with key=str in the
unique_techs_to_place construction to simplify the call that builds
sorted({edge.tech for edge in all_edges_list if edge.tech}); update the
expression that assigns to unique_techs_to_place accordingly (refer to
unique_techs_to_place, all_edges_list, and edge.tech).
| optionsObject = optionsRaw || {}; | ||
| } | ||
|
|
||
| // Expose for debugging only — enable in production. |
There was a problem hiding this comment.
Typo in comment: "enable in production" should be "disable in production".
The comment says "enable in production" but the intent is clearly to enable debugging only during development.
🔧 Proposed fix
- // Expose for debugging only — enable in production.
+ // Expose for debugging only — disable in production.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Expose for debugging only — enable in production. | |
| // Expose for debugging only — disable in production. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/utilities/network_vis_templates/graph_script.js` at line 44, Update the
inline comment that currently reads "// Expose for debugging only — enable in
production." to correctly state the intent by changing it to "// Expose for
debugging only — disable in production." so it warns that the exposed debug
surface should not be enabled in production; locate the comment string in
graph_script.js and replace it accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added slider for label font size to the visualizer, added tests for special items in legends, updated visualizer embed in the docs
Summary by CodeRabbit
New Features
Improvements
Style
Tests