Draft
Conversation
…veRecord references. Add Sequel migration check.
…ate naming, block length suppression, and style fixes.
Contributor
Reviewer's GuideThis PR replaces ActiveRecord with Sequel throughout the application, refactoring models, migrations, Rake tasks, configuration, controllers, tests, and dependencies to use Sequel’s DSL, plugins, and migration system. Sequence diagram for bin creation and validation (Sequel)sequenceDiagram
actor User
participant AppController as "ApplicationController"
participant BinModel as "Bin (Sequel::Model)"
participant DB as "Database"
User->>AppController: POST /
AppController->>BinModel: Bin.new(params)
AppController->>BinModel: bin.save
BinModel->>BinModel: validate()
BinModel->>DB: INSERT bin
DB-->>BinModel: Success/ValidationFailed
BinModel-->>AppController: bin or errors
AppController-->>User: JSON response (id/url or errors)
Entity relationship diagram for the updated bins tableerDiagram
bins {
STRING id PK "primary key"
TEXT payload
DATETIME created_at
DATETIME updated_at
DATETIME expire_date
BOOLEAN has_password
}
Class diagram for the refactored Bin model (ActiveRecord → Sequel)classDiagram
class Bin {
+String id
+Text payload
+DateTime created_at
+DateTime updated_at
+DateTime expire_date
+Boolean has_password
+expired?()
+password?()
+has_password?()
+self.cleanup()
+self.expired()
+before_create()
-generate_unique_id()
validate()
}
note for Bin "Now inherits from Sequel::Model and uses Sequel plugins"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In spec/application_controller_spec, you’re passing retention: '#{72460}' literally instead of evaluating it, so the test never actually sends the intended minute value—replace that with (72460).to_s or proper interpolation.
- The
RemoveIdFromBinsandRenameRandomIdToIdmigrations are no‐ops for SQLite and only add confusion—consider deleting or consolidating them so your migration chain accurately represents real schema changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In spec/application_controller_spec, you’re passing retention: '#{7*24*60}' literally instead of evaluating it, so the test never actually sends the intended minute value—replace that with (7*24*60).to_s or proper interpolation.
- The `RemoveIdFromBins` and `RenameRandomIdToId` migrations are no‐ops for SQLite and only add confusion—consider deleting or consolidating them so your migration chain accurately represents real schema changes.
## Individual Comments
### Comment 1
<location> `app/models/bin.rb:17-29` </location>
<code_context>
+ validates_max_length AppConfig.calc_max_length, :payload
+
+ # Validate expire_date is not more than 7 days in the future
+ if expire_date && expire_date > (Time.now.utc + (7 * 24 * 60 * 60))
+ errors.add(:expire_date, "can't be bigger than 7 days")
+ end
</code_context>
<issue_to_address>
**suggestion:** Use a constant for the 7-day limit to improve maintainability.
Defining a constant for the 7-day limit will make future updates easier and prevent discrepancies across the codebase.
```suggestion
SEVEN_DAY_LIMIT_SECONDS = 7 * 24 * 60 * 60
set_allowed_columns :payload, :has_password, :expire_date
# Validation
def validate
super
validates_presence [:payload, :expire_date]
validates_max_length AppConfig.calc_max_length, :payload
# Validate expire_date is not more than 7 days in the future
if expire_date && expire_date > (Time.now.utc + SEVEN_DAY_LIMIT_SECONDS)
errors.add(:expire_date, "can't be bigger than 7 days")
end
end
```
</issue_to_address>
### Comment 2
<location> `app/models/bin.rb:43` </location>
<code_context>
- ActiveRecord::Base.transaction do
- batch.lock.delete_all
- end
+ where { expire_date < Time.now.utc }.delete
+ end
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Bulk delete may bypass model hooks.
Consider using .destroy on each record if you rely on model hooks for cleanup or logging, as .delete will not trigger them.
</issue_to_address>
### Comment 3
<location> `db/migrate/20240326191856_remove_id_from_bins.rb:3` </location>
<code_context>
- remove_column :bins, :id
+Sequel.migration do
+ change do
+ # This migration is intentionally left blank. Dropping a primary key column is not supported in SQLite.
end
end
</code_context>
<issue_to_address>
**suggestion:** Leaving migrations blank can cause confusion for future maintainers.
Please clarify in documentation or the migration filename why this migration is blank, or remove it if unnecessary for schema history.
</issue_to_address>
### Comment 4
<location> `spec/bin_model_spec.rb:14` </location>
<code_context>
it 'does not save a new bin without a payload' do
bin = Bin.new(payload: '')
- expect(bin.save).to be false
+ expect { bin.save }.to raise_error(Sequel::ValidationFailed)
+ end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing edge cases for payload length and invalid types.
Please add tests for cases where the payload exceeds the maximum length and where expire_date has an invalid type to improve validation coverage.
</issue_to_address>
### Comment 5
<location> `spec/application_controller_spec.rb:25-27` </location>
<code_context>
it 'saves a new bin with a retention time of 7 days' do
- post '/', bin: { payload: 'Hello, World!' }, retention: '10080'
+ post '/', bin: { payload: 'Hello, World!' }, retention: '#{7*24*60}'
expect(last_response.status).to eq(200)
expect(Bin.count).to eq(1)
end
</code_context>
<issue_to_address>
**issue (testing):** Retention time test uses string interpolation in a string literal.
Change '#{7*24*60}' to (7*24*60).to_s so the correct retention value is used.
</issue_to_address>
### Comment 6
<location> `spec/bin_model_spec.rb:56` </location>
<code_context>
+ expect(Bin.expired.all).to eq []
end
it 'has a has_password? method' do
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for has_password default value and toggling.
Add tests to confirm has_password defaults to false, can be set to true, and that password? and has_password? methods reflect both states.
</issue_to_address>
### Comment 7
<location> `app/models/bin.rb:6` </location>
<code_context>
- validate :expire_date_cannot_be_bigger_than_7_days
- has_secure_token :id
- self.primary_key = :id
+class Bin < Sequel::Model
+ plugin :validation_helpers
+ plugin :timestamps, update_on_create: true
</code_context>
<issue_to_address>
**issue (complexity):** Consider using Sequel plugins and dataset modules to simplify model setup and reduce custom code.
```ruby
class Bin < Sequel::Model
# Generates a urlsafe id for you
plugin :secure_token, field: :id, length: 8
# Only load what you actually need
plugin :timestamps, update_on_create: true
plugin :validation_helpers
plugin :whitelist_security, allowed: %i[payload has_password expire_date]
set_primary_key :id
# Validations
def validate
super
validates_presence [:payload, :expire_date]
validates_max_length AppConfig.calc_max_length, :payload
# use Rails‐style time helpers if you prefer
if expire_date && expire_date > 7.days.from_now.utc
errors.add(:expire_date, "can't exceed 7 days")
end
end
# Instance
def expired?
expire_date < Time.now.utc
end
# Class / dataset methods
dataset_module do
def expired
where { expire_date < Time.now.utc }
end
def cleanup!
expired.delete
end
end
end
```
- Replaced manual `before_create!`/`generate_unique_id!` with `plugin :secure_token`
- Merged `set_allowed_columns` into a single `plugin :whitelist_security` call
- Collapsed your two class methods into a `dataset_module`
- Dropped the redundant `password?`/`alias has_password?` (Sequel returns booleans)
</issue_to_address>
### Comment 8
<location> `config/environment.rb:20` </location>
<code_context>
+DB = Sequel.connect(ENV['DATABASE_URL'] || 'sqlite://db/development.sqlite3')
+DB.loggers << Logger.new($stdout) if ENV['RACK_ENV'] == 'development'
+
+# Determine if we're running in a context where we should skip migration checks and model loading
+running_rake = defined?(Rake) && Rake.application.top_level_tasks.any?
+running_db_task = running_rake && Rake.application.top_level_tasks.any? { |task| task.start_with?('db:') }
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting migration check logic into a dedicated initializer and simplifying config/environment.rb to improve clarity.
Here’s one way to collapse all of that branching into two focused pieces—the core “wiring” in config/environment.rb and a single initializer for your migration‐check logic—while preserving exactly the same behavior:
1. Extract migration logic into config/initializers/migration_check.rb:
```ruby
# config/initializers/migration_check.rb
require 'sequel/extensions/migration'
module DBMigrationVerifier
def self.ensure_up_to_date!(db)
return if skip_db_checks?
Sequel::Migrator.check_current(db, 'db/migrate')
rescue Sequel::Migrator::NotCurrentError
warn <<~MSG
#{'='*80}
ERROR: Database migrations are pending!
Your database schema is not up to date.
Please run:
bundle exec rake db:migrate
#{'='*80}
MSG
exit(1)
end
def self.skip_db_checks?
ENV['RACK_ENV'] == 'test' ||
(defined?(Rake) && Rake.application.top_level_tasks.any? { |t| t.start_with?('db:') })
end
end
```
2. Simplify config/environment.rb to just wire up Sequel, call the initializer and load your code:
```ruby
# config/environment.rb
require 'bundler/setup'
require 'sinatra/base'
require 'sinatra/json'
require 'sinatra/config_file'
require 'sequel'
require 'yaml'
require 'logger'
require 'i18n'
require 'i18n/backend/fallbacks'
# 1) connect DB and set logger
DB = Sequel.connect(ENV['DATABASE_URL'] || 'sqlite://db/development.sqlite3')
DB.loggers << Logger.new($stdout) if ENV['RACK_ENV'] == 'development'
# 2) run pending‐migration check
require_relative 'initializers/migration_check'
DBMigrationVerifier.ensure_up_to_date!(DB)
# 3) load the rest of the app
require_relative '../app/helpers/helpers'
require_relative '../app/controllers/application_controller'
require_relative '../app/models/bin'
```
—This moves all of your boolean‐flag logic out of the main environment file, collapses it into a single `skip_db_checks?` helper, and keeps `config/environment.rb` focused on “load & wire,” improving readability while preserving every branch and check you need.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request migrates the application from ActiveRecord to Sequel ORM. The migration includes updating database migrations, model definitions, test suite configuration, and all database interactions throughout the codebase.
- Migration of all database migrations from ActiveRecord to Sequel syntax
- Replacement of ActiveRecord models with Sequel models including custom ID generation
- Updated test suite to use database_cleaner-sequel and adjusted test expectations for Sequel validation behavior
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/spec_helper.rb | Configures Sequel database connection, runs migrations before tests, and sets up DatabaseCleaner for Sequel |
| spec/bin_model_spec.rb | Updates test assertions to handle Sequel validation exceptions and query methods |
| spec/application_controller_spec.rb | Adjusts test data and calculations for compatibility without ActiveSupport helpers |
| db/migrate/*.rb | Converts all migration files from ActiveRecord to Sequel syntax |
| config/environment.rb | Establishes Sequel database connection and migration checks |
| config.ru | Removes ActiveRecord migration check |
| app/models/bin.rb | Completely rewrites Bin model using Sequel plugins and validation patterns |
| app/controllers/application_controller.rb | Updates database queries to use Sequel syntax |
| Rakefile | Implements Sequel-based database rake tasks |
| Gemfile & Gemfile.lock | Replaces activerecord and sinatra-activerecord with sequel and database_cleaner-sequel |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
whotwagner
requested changes
Nov 9, 2025
The schema_migrations table migration is already handled by: 1. db/migrate/20260108000001_fix_schema_migrations_for_sequel.rb (proper migration) 2. migration_check.rb helper functions (bootstrap compatibility check) This file was doing the same thing and is not needed.
…lizer and remove migration file
* origin: (38 commits) docs: restrict docker-compose web port to localhost Refactor docker-compose.yml: remove unused version and bind port to localhost Replace backticks with exec to enable stdout logging Use setup-ruby@v1, github runner again Fix blacksmith runner name Update GitHub Actions workflow to use specific runner and fix Ruby version formatting Upgrade Ruby version to 4.0.1 and update related configurations Update test scenario name for clarity and remove unnecessary blank line Refactor JavaScript code: use let/const, improve comparisons, and minor cleanups Improve focus logic and password field handling in application.js Update public/javascripts/application.js Add feature specs to verify focus behavior on important UI elements Improve focus management for password and textarea inputs Focus message textarea on page load Disable version display in footer by default in config.yml Bump sinatra-contrib from `074d876` to `9e5c4ec` Bump dalli from 3.2.8 to 4.0.0 Bump activerecord from 8.1.1 to 8.1.2 Refactor versioning: move version constant to code, simplify versioning and build script Increase textarea width and improve message counter styling ...
|
|
||
| desc 'Drop the database (deletes SQLite file)' | ||
| task :drop do | ||
| db_path = DB.uri.split(':///').last |
There was a problem hiding this comment.
The URI parsing assumes the database URL has the format "sqlite:///path", but this will fail with "sqlite://path" (two slashes instead of three). Consider using URI(db.uri).path or db.opts[:database] for more reliable database path extraction.
Suggested change
| db_path = DB.uri.split(':///').last | |
| db_path = DB.opts[:database] |
Comment on lines
+23
to
+24
| running_rake = defined?(Rake) && Rake.application.top_level_tasks.any? | ||
| running_db_task = running_rake && Rake.application.top_level_tasks.any? { |task| task.start_with?('db:') } |
There was a problem hiding this comment.
The running_rake variable checks Rake.application.top_level_tasks.any?, but this will raise a NameError if Rake is not defined. The outer check defined?(Rake) only verifies that the constant exists, not that Rake.application is initialized. Consider using defined?(Rake) && defined?(Rake.application) && Rake.application.top_level_tasks.any? or wrap the check in a begin-rescue block.
Suggested change
| running_rake = defined?(Rake) && Rake.application.top_level_tasks.any? | |
| running_db_task = running_rake && Rake.application.top_level_tasks.any? { |task| task.start_with?('db:') } | |
| rake_app = defined?(Rake) && defined?(Rake.application) ? Rake.application : nil | |
| running_rake = rake_app && rake_app.respond_to?(:top_level_tasks) && rake_app.top_level_tasks.any? | |
| running_db_task = running_rake && rake_app.top_level_tasks.any? { |task| task.start_with?('db:') } |
| def expire_date_cannot_be_bigger_than_7_days | ||
| return unless expire_date | ||
| # Allow mass-assignment for these columns | ||
| SEVEN_DAY_LIMIT_SECONDS = 7 * 24 * 60 * 60 |
There was a problem hiding this comment.
The constant SEVEN_DAY_LIMIT_SECONDS is defined but only used once within the same class. Consider either using it in more places where 7 days is referenced (e.g., in comments or error messages) or inlining it at the point of use for clarity, since the calculation 7 * 24 * 60 * 60 is self-documenting.
| class Bin < Sequel::Model | ||
| plugin :validation_helpers | ||
| plugin :timestamps, update_on_create: true | ||
| plugin :whitelist_security |
There was a problem hiding this comment.
The whitelist_security plugin is used with set_allowed_columns, but the plugin name is outdated terminology. Sequel 5.47.0+ recommends using the allowlist_security plugin instead, which has the same functionality but uses more inclusive language. Consider updating to plugin :allowlist_security and set_allowed_columns (which works with both).
Suggested change
| plugin :whitelist_security | |
| plugin :allowlist_security |
Comment on lines
+65
to
+67
| loop do | ||
| random_id = SecureRandom.urlsafe_base64(8).tr('-_', 'az') | ||
| break random_id unless Bin[random_id] |
There was a problem hiding this comment.
The generate_unique_id method uses Bin[random_id] inside a loop to check for ID collisions. This performs a database query on each iteration. While collisions should be rare with the current ID generation scheme, consider adding an index on the id column (which should already exist as it's the primary key) and potentially using a more efficient collision check strategy or increasing the ID length to reduce collision probability.
Suggested change
| loop do | |
| random_id = SecureRandom.urlsafe_base64(8).tr('-_', 'az') | |
| break random_id unless Bin[random_id] | |
| # Generate multiple candidate IDs per iteration and check them in a single query | |
| loop do | |
| candidate_ids = Array.new(10) do | |
| SecureRandom.urlsafe_base64(12).tr('-_', 'az') | |
| end | |
| existing_ids = Bin.where(id: candidate_ids).select_map(:id) | |
| available_id = (candidate_ids - existing_ids).first | |
| return available_id if available_id |
… update specs Agent-Logs-Url: https://github.com/aha-oida/aha-secret/sessions/07caa785-7ea0-4c88-b7a9-4a999cc434da Co-authored-by: armins <168588+armins@users.noreply.github.com>
Resolved conflicts: - .github/workflows/ruby.yml: take main's rate-limit step isolation and screenshot upload; drop deprecated MEMCACHE/CI env vars - Gemfile: keep sequel + database_cleaner-sequel, drop activerecord; bump faker to ~> 3.6.1 from main - Gemfile.lock: regenerated after resolution - app/lib/app_config/loader.rb: take main's refactored private helper methods (same logic, cleaner structure) - config.ru: drop legacy MEMCACHE-based test rate-limit block; rate limit spec now driven by AHA_SECRET_RATE_LIMIT env var - spec/spec_helper.rb: keep Sequel DatabaseCleaner setup from HEAD, add fileutils require from main
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
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.
Summary by Sourcery
Replace ActiveRecord with Sequel as the ORM across the application
Enhancements:
Tests: