Repository navigation
fix: improve robustness and fix bugs across codebase - #8
Conversation
9a222dd to
037c0be
Compare
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving robustness across LiveStyle’s developer tooling and runtime by tightening parsing/merging behavior, making file operations more resilient, and adding clearer error handling for common failure cases.
Changes:
- Harden Mix tasks and runtime resolvers against missing/invalid inputs (e.g., invalid class atoms, unresolved refs).
- Improve correctness/robustness in CSS/media-query handling, shorthand expansion, and selector parsing.
- Make on-disk artifact writes more resilient (atomic temp-write+rename patterns, safer reads).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/mix/tasks/live_style.inspect.ex | Improves CLI robustness by halting with a clearer error when class names don’t map to existing atoms. |
| lib/mix/tasks/live_style.audit.ex | Improves accuracy of reported line numbers by using regex byte offsets. |
| lib/live_style/storage.ex | Adjusts stale lock cleanup timing and uses POSIX mtime for simpler lock age calculation. |
| lib/live_style/shorthand_behavior/flatten_shorthands.ex | Makes !important stripping safer by using suffix-based replacement. |
| lib/live_style/shorthand_behavior/accept_shorthands.ex | Changes overscroll-behavior expansion to avoid emitting nil values for invalid inputs. |
| lib/live_style/selector/condition.ex | Improves parsing to ignore @ inside []/() when splitting pseudo selectors from at-rules. |
| lib/live_style/runtime/ref_resolver.ex | Makes cross-module and dynamic resolution safer by checking module availability/exports. |
| lib/live_style/runtime/property_merger.ex | Reworks merge implementation to use a map+key-order list rather than repeated linear scans. |
| lib/live_style/runtime/dynamic.ex | Adds explicit argument count validation for dynamic class values with an actionable error message. |
| lib/live_style/media_query/transform.ex | Anchors media-query regexes to avoid transforming complex queries; improves iteration to avoid index-based access. |
| lib/live_style/manifest.ex | Simplifies sorted-list put/get with single-pass implementations and early-stop lookup. |
| lib/live_style/css_value/normalize.ex | Fixes timing normalization parsing for values that already include decimals; rounds seconds output. |
| lib/live_style/compiler/watch.ex | Prevents watch loop crashes on regeneration errors and switches hashing to :crypto.hash/2. |
| lib/live_style/compiler/module_data.ex | Adds atomic write pattern and safer reads for ETF files (gracefully handling corrupt/unreadable terms). |
| lib/live_style/compiler/css/writer/file.ex | Writes CSS output via temp file + rename to reduce partial-write risk. |
| lib/live_style/compiler/css/vars.ex | Tightens detection of “conditional lists” and avoids crashing on empty lists when extracting defaults. |
| lib/live_style/compiler/css/theme_classes.ex | Tightens detection of “conditional lists” to ensure all elements are {key, value} tuples. |
| lib/live_style/compiler/css/atomic_class.ex | Adds RTL generation support for fallback arrays and wraps output consistently in at-rules. |
| lib/live_style/class/include.ex | Adds include-cycle detection to prevent infinite recursion during include resolution. |
| lib/live_style/attrs.ex | Adjusts when :class is emitted so prop-class-only Attrs can still participate in component merging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = [] | ||
| result = if style && style != "", do: [{:style, style} | result], else: result | ||
| result = if class && class != "", do: [{:class, attrs} | result], else: result | ||
| has_class = class && class != "" |
There was a problem hiding this comment.
has_class can be nil when class is nil (because class && class != "" returns nil), and or only accepts booleans. This will raise BadBooleanError at runtime for attrs that have style set but no class. Consider making has_class a strict boolean (e.g., is_binary(class) and class != "") or use || with a boolean conversion.
| has_class = class && class != "" | |
| has_class = is_binary(class) and class != "" |
| defp find_top_level_at(selector, pos, bracket_depth, paren_depth) do | ||
| if pos >= byte_size(selector) do | ||
| nil | ||
| else | ||
| <<_::binary-size(pos), char, _::binary>> = selector | ||
|
|
||
| case char do | ||
| ?[ -> | ||
| find_top_level_at(selector, pos + 1, bracket_depth + 1, paren_depth) | ||
|
|
||
| ?] -> | ||
| find_top_level_at(selector, pos + 1, max(bracket_depth - 1, 0), paren_depth) | ||
|
|
||
| ?( -> | ||
| find_top_level_at(selector, pos + 1, bracket_depth, paren_depth + 1) | ||
|
|
||
| ?) -> | ||
| find_top_level_at(selector, pos + 1, bracket_depth, max(paren_depth - 1, 0)) |
There was a problem hiding this comment.
find_top_level_at/4 uses <<_::binary-size(pos), char, _::binary>> = selector on every recursion step, which re-matches from the start of the binary each time and can become O(n^2) on long selectors. Using :binary.at(selector, pos) (and keeping the depth bookkeeping) avoids the repeated prefix match and keeps the scan O(n).
| # Track insertion order as a list of normalized keys | ||
| acc_keys = Enum.map(acc, fn {k, _v} -> normalize_key(k) end) | ||
|
|
||
| # Merge each property class using map for O(1) ops, list for order | ||
| {merged_map, merged_keys} = | ||
| Enum.reduce(prop_classes, {acc_map, acc_keys}, fn | ||
| {prop, :__unset__}, {map, keys} -> | ||
| nk = normalize_key(prop) | ||
| {Map.delete(map, nk), List.delete(keys, nk)} | ||
|
|
||
| {prop, class}, {map, keys} -> | ||
| nk = normalize_key(prop) | ||
| existing? = Map.has_key?(map, nk) | ||
| new_map = Map.put(map, nk, {prop, class}) | ||
|
|
||
| new_keys = | ||
| if existing?, | ||
| do: keys, | ||
| else: keys ++ [nk] | ||
|
|
||
| {new_map, new_keys} | ||
| end) | ||
|
|
There was a problem hiding this comment.
The new merge algorithm still does keys ++ [nk] when adding a new property, which is O(length(keys)) per insert and can negate the intended performance gains for larger class lists. Consider accumulating keys in reverse with prepends and reversing once at the end, or using a queue-like structure, to keep inserts O(1) while preserving order.
| # Track insertion order as a list of normalized keys | |
| acc_keys = Enum.map(acc, fn {k, _v} -> normalize_key(k) end) | |
| # Merge each property class using map for O(1) ops, list for order | |
| {merged_map, merged_keys} = | |
| Enum.reduce(prop_classes, {acc_map, acc_keys}, fn | |
| {prop, :__unset__}, {map, keys} -> | |
| nk = normalize_key(prop) | |
| {Map.delete(map, nk), List.delete(keys, nk)} | |
| {prop, class}, {map, keys} -> | |
| nk = normalize_key(prop) | |
| existing? = Map.has_key?(map, nk) | |
| new_map = Map.put(map, nk, {prop, class}) | |
| new_keys = | |
| if existing?, | |
| do: keys, | |
| else: keys ++ [nk] | |
| {new_map, new_keys} | |
| end) | |
| # Track insertion order as a list of normalized keys (maintained in reverse for O(1) inserts) | |
| acc_keys = Enum.map(acc, fn {k, _v} -> normalize_key(k) end) | |
| acc_keys_rev = Enum.reverse(acc_keys) | |
| # Merge each property class using map for O(1) ops, list for order | |
| {merged_map, merged_keys_rev} = | |
| Enum.reduce(prop_classes, {acc_map, acc_keys_rev}, fn | |
| {prop, :__unset__}, {map, keys_rev} -> | |
| nk = normalize_key(prop) | |
| {Map.delete(map, nk), List.delete(keys_rev, nk)} | |
| {prop, class}, {map, keys_rev} -> | |
| nk = normalize_key(prop) | |
| existing? = Map.has_key?(map, nk) | |
| new_map = Map.put(map, nk, {prop, class}) | |
| new_keys_rev = | |
| if existing?, | |
| do: keys_rev, | |
| else: [nk | keys_rev] | |
| {new_map, new_keys_rev} | |
| end) | |
| merged_keys = Enum.reverse(merged_keys_rev) |
037c0be to
3150f65
Compare
3150f65 to
a5159eb
Compare
No description provided.