Bug fixes#24
Conversation
Greptile SummaryThis is a comprehensive bug-fix release (v1.6.2) addressing 15 distinct issues across the
Confidence Score: 4/5This PR is safe to merge; all changes are targeted bug fixes with matching test coverage, and no regressions were identified in the core sync, STI sharing, or thread-safety paths. The changes are well-scoped and well-tested, covering 15 distinct fixes without introducing new architectural complexity. The most impactful change — the autosave cycle-detection fix that replaced lib/support_table_data.rb warrants a second look around the attribute-helper tracking logic for STI subclasses and the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([sync_table_data!]) --> B{table_exists?}
B -- No --> C[return empty array]
B -- Yes --> D[Build canonical_data from data files]
D --> E{delete_missing AND canonical_data empty AND rows exist?}
E -- Yes --> F[raise ArgumentError - guard against table wipe]
E -- No --> G[Query existing records where key IN canonical_data.keys]
G --> H[Begin transaction]
H --> I[Update existing records from canonical_data]
I --> J[Create new records for remaining canonical_data entries]
J --> K{delete_missing?}
K -- Yes --> L[destroy_all records not in synced_ids]
K -- No --> M[return changes array]
L --> M
H -- RecordInvalid --> N[raise ValidationError]
H -- RecordNotUnique --> O{retried?}
O -- Yes --> P[re-raise error]
O -- No --> Q[retried = true]
Q --> D
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A([sync_table_data!]) --> B{table_exists?}
B -- No --> C[return empty array]
B -- Yes --> D[Build canonical_data from data files]
D --> E{delete_missing AND canonical_data empty AND rows exist?}
E -- Yes --> F[raise ArgumentError - guard against table wipe]
E -- No --> G[Query existing records where key IN canonical_data.keys]
G --> H[Begin transaction]
H --> I[Update existing records from canonical_data]
I --> J[Create new records for remaining canonical_data entries]
J --> K{delete_missing?}
K -- Yes --> L[destroy_all records not in synced_ids]
K -- No --> M[return changes array]
L --> M
H -- RecordInvalid --> N[raise ValidationError]
H -- RecordNotUnique --> O{retried?}
O -- Yes --> P[re-raise error]
O -- No --> Q[retried = true]
Q --> D
Reviews (1): Last reviewed commit: "Bug fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| class_eval <<~RUBY, __FILE__, __LINE__ + 1 | ||
| def #{method_name} | ||
| #{attribute_name} == #{attribute_value.inspect} | ||
| #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) | ||
| end | ||
| RUBY |
There was a problem hiding this comment.
type_for_attribute(...).cast(...) on every single invocation to cast a value that is a compile-time constant (the raw value from the data file). While not incorrect, this repeatedly looks up and invokes the type caster for the same literal input. The cast result could instead be pre-computed at method-definition time and inlined as a constant in the generated Ruby, avoiding the runtime overhead on every predicate call. This matters most for predicate-heavy hot paths.
| class_eval <<~RUBY, __FILE__, __LINE__ + 1 | |
| def #{method_name} | |
| #{attribute_name} == #{attribute_value.inspect} | |
| #{attribute_name} == self.class.type_for_attribute(#{attribute_name.inspect}).cast(#{attribute_value.inspect}) | |
| end | |
| RUBY | |
| cast_value = type_for_attribute(attribute_name).cast(attribute_value) | |
| class_eval <<~RUBY, __FILE__, __LINE__ + 1 | |
| def #{method_name} | |
| #{attribute_name} == #{cast_value.inspect} | |
| end | |
| RUBY |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| data = if Psych::VERSION.to_f >= 3.1 | ||
| YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) | ||
| else | ||
| # Positional arguments for Psych < 3.1 (Ruby 2.5). | ||
| YAML.safe_load(file_data, [Date, Time], [], true) | ||
| end |
There was a problem hiding this comment.
to_f
Psych::VERSION.to_f truncates everything after the first decimal point, so "3.1.0" and "3.10.0" both produce 3.1. In practice Psych hasn't released a 3.10.x, but the comparison is inherently imprecise. Consider using Gem::Version for a semantically correct comparison: Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1").
| support_table_attribute_helpers_map.each do |attribute_name, defined_methods| | ||
| attribute_method_name = "#{method_name}_#{attribute_name}" | ||
| next if defined_methods.include?(attribute_method_name) | ||
|
|
||
| define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name]) | ||
| defined_methods << attribute_method_name | ||
| if defined_methods.include?(attribute_method_name) | ||
| define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name], redefine: true) | ||
| else | ||
| define_support_table_instance_attribute_helper(attribute_method_name, attributes[attribute_name]) | ||
| defined_methods << attribute_method_name | ||
| end | ||
| end |
There was a problem hiding this comment.
support_table_attribute_helpers_map returns the parent's @support_table_attribute_helpers hash for STI subclasses. The defined_methods << attribute_method_name mutation therefore writes directly into the parent class's tracking array while holding the parent's mutex — correct for the common case. However, if define_support_table_named_instance_methods is ever invoked on an STI subclass (e.g. via a subclass calling named_instance_attribute_helpers), the write to @support_table_attribute_helpers on line 195 creates a new, independent copy on the subclass, while defined_methods from the previous support_table_attribute_helpers_map call still points into the parent's array. After that point the subclass's map and the parent's map diverge. This is unlikely in practice since STI subclasses should not register their own attribute helpers, but a clarifying comment would help.
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughSupport-table synchronization now handles empty or missing data safely, retries concurrent uniqueness conflicts, and wraps validation failures. STI subclasses share cached state, named-instance helpers can be redefined, YAML supports aliases and date/time values, dependency discovery is updated, and related tests and release documentation are added. ChangesSupport table data behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SupportTableData
participant DataFiles
participant ActiveRecordTable
Caller->>SupportTableData: sync_table_data!
SupportTableData->>DataFiles: read canonical rows
SupportTableData->>ActiveRecordTable: find or insert records
ActiveRecordTable-->>SupportTableData: RecordNotUnique on concurrent insert
SupportTableData->>ActiveRecordTable: retry synchronization once
ActiveRecordTable-->>Caller: synchronized records or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/support_table_data.rb (1)
209-230: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftCritical:
support_table_data/sync_table_data!silently creates a phantom row and skips the intended override when a later data file overrides a named row's attribute without repeating the key attribute.
support_table_datamerges rows solely byattributes[support_table_key_attribute](defaulting to"id"). Butdefine_support_table_named_instances(used for named-instance/attribute helpers) merges by the YAML top-level name key instead. These two merge strategies diverge whenever an override file specifies only a subset of attributes for a named row without repeating the key attribute — exactly the pattern introduced by this PR's own fixtures:# spec/data/sizes_override.yml large: label: LargeSince this entry has no
id,support_table_datacomputeskey_value = attributes["id"].to_s→"", which doesn't match the"3"key already populated fromsizes.yml. The result:Size.sync_table_data!will (1) leave the real "large" row'slabelas"Big"(the override is never applied to the DB row) and (2) silently insert a phantom row withlabel: "Large"and every other attributenil.Meanwhile
Size.large_label(built from the name-keyed merge) correctly returns"Large", masking the discrepancy — the "redefines attribute helpers…" test passes even though the actual synced record is wrong. None of the current specs assertSize.count/the DB value oflabelon the "large" record aftersync_table_data!, so this goes unnoticed.Suggested direction: have
support_table_datareuse the name-keyed merge (likedefine_support_table_named_instances) for Hash-structured files — merge attributes by the YAML top-level name first, then compute the final key attribute from the merged result — before falling back to key-attribute merging for flat array-style (JSON/CSV) records. Also worth adding a regression test assertingSize.sync_table_data!produces exactly 3 rows and that the "large" row'slabelis"Large".🐛 Suggested direction (not a drop-in fix — needs verification against JSON/CSV interplay)
def support_table_data - data = {} - support_table_data_files.each do |data_file_path| - file_data = support_table_parse_data_file(data_file_path) - file_data = file_data.values if file_data.is_a?(Hash) - file_data = Array(file_data).flatten - file_data.each do |attributes| - key_value = attributes[support_table_key_attribute].to_s - existing = data[key_value] - if existing - existing.merge!(attributes) - else - data[key_value] = attributes - end - end - end - - data.values + merged_by_name = {} + data = {} + + support_table_data_files.each do |data_file_path| + file_data = support_table_parse_data_file(data_file_path) + if file_data.is_a?(Hash) + file_data.each do |name, attributes| + next unless attributes.is_a?(Hash) + existing = merged_by_name[name.to_s] + merged_by_name[name.to_s] = existing ? existing.merge(attributes) : attributes + end + else + Array(file_data).flatten.each do |attributes| + key_value = attributes[support_table_key_attribute].to_s + existing = data[key_value] + data[key_value] = existing ? existing.merge(attributes) : attributes + end + end + end + + merged_by_name.each_value do |attributes| + key_value = attributes[support_table_key_attribute].to_s + existing = data[key_value] + data[key_value] = existing ? existing.merge(attributes) : attributes + end + + data.values end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/support_table_data.rb` around lines 209 - 230, Update support_table_data to merge Hash-structured file records by their top-level YAML name, reusing the same name-keyed behavior as define_support_table_named_instances, so partial overrides inherit the existing key attribute before final records are keyed. Preserve key-attribute merging for flat array-style JSON/CSV records, and add regression coverage verifying Size.sync_table_data! leaves exactly three rows and updates the large row’s label to “Large”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/support_table_data.rb`:
- Around line 483-507: Rename the class-level mutex instance variable used by
support_table_mutex from `@mutex` to the concern-specific `@support_table_mutex`,
and update all initialization and access points consistently. Preserve the
existing superclass fallback behavior for STI subclasses.
- Around line 458-464: Update the Psych version check in the YAML loading branch
to compare Gem::Version instances rather than using Psych::VERSION.to_f, using
the documented 3.1.0.pre1 threshold and preserving the existing safe_load
branches and arguments.
In `@lib/support_table_data/documentation/source_file.rb`:
- Line 10: Update the generated-documentation handling around YARD_COMMENT_REGEX
and its source.sub/source.match callers to process every existing YARD block,
not only the first. Remove all matching generated blocks while preserving
intervening user code, then emit exactly one regenerated block.
---
Outside diff comments:
In `@lib/support_table_data.rb`:
- Around line 209-230: Update support_table_data to merge Hash-structured file
records by their top-level YAML name, reusing the same name-keyed behavior as
define_support_table_named_instances, so partial overrides inherit the existing
key attribute before final records are keyed. Preserve key-attribute merging for
flat array-style JSON/CSV records, and add regression coverage verifying
Size.sync_table_data! leaves exactly three rows and updates the large row’s
label to “Large”.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd33f181-6177-4306-8a10-a6690bc998b4
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mdVERSIONlib/support_table_data.rblib/support_table_data/documentation/source_file.rblib/support_table_data/railtie.rblib/support_table_data/tasks/utils.rbspec/data/sizes.ymlspec/data/sizes_override.ymlspec/models.rbspec/models/size.rbspec/support_table_data/documentation/source_file_spec.rbspec/support_table_data_spec.rb
| require "date" unless defined?(Date) | ||
| data = if Psych::VERSION.to_f >= 3.1 | ||
| YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) | ||
| else | ||
| # Positional arguments for Psych < 3.1 (Ruby 2.5). | ||
| YAML.safe_load(file_data, [Date, Time], [], true) | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider Gem::Version instead of .to_f for the Psych version check.
Psych::VERSION.to_f >= 3.1 works for the realistic Psych 3.x/4.x/5.x version history, but .to_f is a known-fragile pattern for version comparisons in general (e.g. "3.10".to_f == 3.1, which would compare incorrectly against a real "3.2"). The documented, more robust idiom for this exact check is Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1.0.pre1").
♻️ Suggested refactor
- data = if Psych::VERSION.to_f >= 3.1
+ data = if Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1.0.pre1")
YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true)
else
# Positional arguments for Psych < 3.1 (Ruby 2.5).
YAML.safe_load(file_data, [Date, Time], [], true)
end📝 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.
| require "date" unless defined?(Date) | |
| data = if Psych::VERSION.to_f >= 3.1 | |
| YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) | |
| else | |
| # Positional arguments for Psych < 3.1 (Ruby 2.5). | |
| YAML.safe_load(file_data, [Date, Time], [], true) | |
| end | |
| require "date" unless defined?(Date) | |
| data = if Gem::Version.new(Psych::VERSION) >= Gem::Version.new("3.1.0.pre1") | |
| YAML.safe_load(file_data, permitted_classes: [Date, Time], aliases: true) | |
| else | |
| # Positional arguments for Psych < 3.1 (Ruby 2.5). | |
| YAML.safe_load(file_data, [Date, Time], [], true) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/support_table_data.rb` around lines 458 - 464, Update the Psych version
check in the YAML loading branch to compare Gem::Version instances rather than
using Psych::VERSION.to_f, using the documented 3.1.0.pre1 threshold and
preserving the existing safe_load branches and arguments.
|
|
||
| # The class level state used by the concern is stored in instance variables on the | ||
| # class where the concern was included. These readers fall back to the superclass | ||
| # so that single table inheritance subclasses share the state defined on their | ||
| # base class rather than crashing on uninitialized instance variables. | ||
|
|
||
| def support_table_mutex | ||
| @mutex || (superclass.include?(SupportTableData) ? superclass.send(:support_table_mutex) : nil) | ||
| end | ||
|
|
||
| def support_table_data_files | ||
| @support_table_data_files || (superclass.include?(SupportTableData) ? superclass.send(:support_table_data_files) : []) | ||
| end | ||
|
|
||
| def support_table_instance_names_map | ||
| @support_table_instance_names || (superclass.include?(SupportTableData) ? superclass.send(:support_table_instance_names_map) : {}) | ||
| end | ||
|
|
||
| def support_table_attribute_helpers_map | ||
| @support_table_attribute_helpers || (superclass.include?(SupportTableData) ? superclass.send(:support_table_attribute_helpers_map) : {}) | ||
| end | ||
|
|
||
| def support_table_dependency_names | ||
| @support_table_dependencies || (superclass.include?(SupportTableData) ? superclass.send(:support_table_dependency_names) : []) | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Solid cascading-state design; minor naming nit on @mutex.
The superclass-fallback pattern correctly lets STI subclasses share the base class's mutex/caches without re-running included do. One small nit: @mutex is a fairly generic ivar name for a class-level instance variable mixed into arbitrary ActiveRecord::Base subclasses; a more specific name like @support_table_mutex would reduce any (admittedly remote) risk of colliding with another concern/mixin using the same generic name on the same class.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/support_table_data.rb` around lines 483 - 507, Rename the class-level
mutex instance variable used by support_table_mutex from `@mutex` to the
concern-specific `@support_table_mutex`, and update all initialization and access
points consistently. Preserve the existing superclass fallback behavior for STI
subclasses.
| BEGIN_YARD_COMMENT = "# Begin YARD docs for support_table_data" | ||
| END_YARD_COMMENT = "# End YARD docs for support_table_data" | ||
| YARD_COMMENT_REGEX = /^(?<indent>[ \t]*)#{BEGIN_YARD_COMMENT}.*^[ \t]*#{END_YARD_COMMENT}$/m | ||
| YARD_COMMENT_REGEX = /^(?<indent>[ \t]*)#{BEGIN_YARD_COMMENT}.*?^[ \t]*#{END_YARD_COMMENT}$/m |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Handle all duplicated generated-doc blocks.
The non-greedy regex preserves user code, but source.sub(...) and source.match(...) still process only the first block. A file with duplicated YARD sections therefore retains stale generated documentation after cleanup or regeneration. Remove all existing generated blocks while preserving user code, then emit exactly one block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/support_table_data/documentation/source_file.rb` at line 10, Update the
generated-documentation handling around YARD_COMMENT_REGEX and its
source.sub/source.match callers to process every existing YARD block, not only
the first. Remove all matching generated blocks while preserving intervening
user code, then emit exactly one regenerated block.
1 participant
Fixed
sync_table_data!withdelete_missing: truenow raises anArgumentErrorinstead of deleting every row in the table when the data files contain no rows (for example, when a data file was accidentally emptied or truncated).record.active?) now cast the data file value to the attribute type before comparing. Previously the raw file value was compared to the cast database attribute, so predicates silently returnedfalsewhenever the types differed (guaranteed for CSV data files, where all values are strings).NoMethodErrorfrominstance_names,instance_keys,protected_instance?, and other class methods. Subclasses now share the support table state defined on their base class.named_instance_attribute_helperscan now be called again with an attribute that was already registered without raising anArgumentError.Psych::AliasesNotEnabledorPsych::DisallowedClasserrors.protected_instance?no longer returns stale results when data files are added after the protected keys were first computed.sync_table_data!now retries once onActiveRecord::RecordNotUniqueerrors caused by concurrent syncs inserting the same rows from another process.sync_table_data!now returns an empty array instead ofnilwhen the table does not exist.named_instancenow raises a clearActiveRecord::RecordNotFounderror for undefined named instances instead of querying the database for anilkey (which could silently return a row with aNULLkey value).config.support_table.auto_sync = falsebefore the gem is loaded is no longer overwritten back totrueby the Railtie.SupportTableData.sync_all!to eager load models.Summary by CodeRabbit
Bug Fixes
auto_sync: falseconfiguration values.Documentation
Version