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}
|
@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
…r single table inheritance, improve error handling, and update YARD documentation format
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