font size slider for visualizer#256
Open
ParticularlyPythonicBS wants to merge 4 commits intounstablefrom
Open
Conversation
ParticularlyPythonicBS
added
Maintenance
WalkthroughDynamic layout scaling for network graphs; swapped primary/secondary embedded graph payloads in static HTML; added a Label Font Size slider with per-node font-size wiring and CSS; exposed master graph data to Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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.
Comment on lines
20
to
26
| <div class="legend-section"> | ||
| <h4>Visual Settings</h4> | ||
| <div class="control-group"> | ||
| <label for="font-size-slider">Label Font Size</label> | ||
| <input type="range" id="font-size-slider" min="6" max="100" value="14"> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Font size slider integration.
The slider is well-integrated with proper labeling and accessibility attributes. The default value of 14 correctly matches DEFAULT_VIS_OPTIONS.
One consideration: a max of 100 is quite large for label fonts. If most users will stay within 6-30, you could consider tightening the range for finer control, but the current range does allow for extreme zoom-out scenarios where larger labels might be useful.
🤖 Prompt for AI Agents
In `@temoa/utilities/network_vis_templates/graph_template.html` around lines 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.
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.
Comment on lines
+50
to
+52
| let visualState = { | ||
| fontSize: 14 | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the graph_script.js file
find . -name "graph_script.js" -type fRepository: TemoaProject/temoa
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Read the graph_script.js file to understand the full context
wc -l ./temoa/utilities/network_vis_templates/graph_script.jsRepository: TemoaProject/temoa
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Read the relevant sections of the file
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | head -120 | tail -80Repository: TemoaProject/temoa
Length of output: 3994
🏁 Script executed:
#!/bin/bash
# Read the beginning of the file to understand how optionsObject is created
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | head -50Repository: TemoaProject/temoa
Length of output: 2122
🏁 Script executed:
#!/bin/bash
# Search for font size references and how optionsObject is used
rg -n "fontSize|font.*size" ./temoa/utilities/network_vis_templates/graph_script.jsRepository: TemoaProject/temoa
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Search for optionsObject usage and structure to see if it contains font settings
rg -n "optionsObject" ./temoa/utilities/network_vis_templates/graph_script.js | head -20Repository: TemoaProject/temoa
Length of output: 350
🏁 Script executed:
#!/bin/bash
# Look for where optionsRaw/options_json_str is generated, likely in a Python backend
find . -name "*.py" -type f | xargs rg -l "options_json_str|network_vis" | head -10Repository: TemoaProject/temoa
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file to understand how options_json_str is generated
wc -l ./temoa/utilities/visualizer.pyRepository: TemoaProject/temoa
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file to find options_json_str generation
rg -n "options_json_str|nodes.*font|edges.*font" ./temoa/utilities/visualizer.pyRepository: TemoaProject/temoa
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file around line 242 and look for options construction
cat -n ./temoa/utilities/visualizer.py | sed -n '200,280p'Repository: TemoaProject/temoa
Length of output: 3792
🏁 Script executed:
#!/bin/bash
# Look at the DEFAULT_VIS_OPTIONS to understand the structure
cat -n ./temoa/utilities/visualizer.py | sed -n '272,315p'Repository: TemoaProject/temoa
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Check if the slider has a default value in the HTML template
rg -n "font-size-slider" ./temoa/utilities/network_vis_templates/ -A 2 -B 2Repository: TemoaProject/temoa
Length of output: 1411
🏁 Script executed:
#!/bin/bash
# Verify scope - check that optionsObject is defined before line 50
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | sed -n '30,55p'Repository: TemoaProject/temoa
Length of output: 1021
Synchronize slider initial value with backend font size defaults to prevent UI/render mismatch.
visualState.fontSize and the slider are hardcoded to 14, but optionsObject (derived from backend options_json_str) may specify a different font size. When the network initializes, it uses the size from optionsObject, creating a mismatch where the actual render differs from the displayed slider value. Initialize both from optionsObject with a fallback to 14, and set the slider's initial value accordingly.
🛠️ Suggested fix
- let visualState = {
- fontSize: 14
- };
+ const parsedFontSize = parseInt(
+ optionsObject?.nodes?.font?.size ?? optionsObject?.edges?.font?.size ?? 14,
+ 10,
+ );
+ const defaultFontSize = Number.isFinite(parsedFontSize) ? parsedFontSize : 14;
+ let visualState = {
+ fontSize: defaultFontSize
+ };And at line 60:
const fontSizeSlider = document.getElementById('font-size-slider');
+ if (fontSizeSlider) fontSizeSlider.value = String(defaultFontSize);🤖 Prompt for AI Agents
In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 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.
Comment on lines
+20
to
+26
| <div class="legend-section"> | ||
| <h4>Visual Settings</h4> | ||
| <div class="control-group"> | ||
| <label for="font-size-slider">Label Font Size</label> | ||
| <input type="range" id="font-size-slider" min="6" max="100" value="14"> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Minor inconsistency: Missing step attribute on slider.
The font-size slider here is missing step="1" which is present in graph_template.html (line 24). While browsers default to step=1 for range inputs, adding it would maintain 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">📝 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
| <div class="legend-section"> | |
| <h4>Visual Settings</h4> | |
| <div class="control-group"> | |
| <label for="font-size-slider">Label Font Size</label> | |
| <input type="range" id="font-size-slider" min="6" max="100" value="14"> | |
| </div> | |
| </div> | |
| <div class="legend-section"> | |
| <h4>Visual Settings</h4> | |
| <div class="control-group"> | |
| <label for="font-size-slider">Label Font Size</label> | |
| <input type="range" id="font-size-slider" min="6" max="100" step="1" value="14"> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
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;
update the element (identified by id "font-size-slider") to include step="1" to
match graph_template.html and ensure consistent behavior across templates.
ParticularlyPythonicBS
force-pushed
the
visualizer_improvements
branch
from
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.
Comment on lines
+76
to
+86
| edge_do = next(e for e in edges if (e[0] == 'comm_inter' and e[1] == 'comm_demand')) | ||
| 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')) | ||
| 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')) | ||
| assert edge_dt[2]['dashes'] is True | ||
| assert edge_dt[2]['color'] == '#1f77b4' |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using explicit assertions for edge lookup to improve test failure messages.
Using next(e for e in edges if ...) raises StopIteration if no matching edge is found, which produces an unhelpful error message. Consider extracting a helper or using list comprehension with an explicit assertion.
♻️ 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
In `@tests/test_commodity_visualizer.py` around lines 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.
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