Skip to content

chore: remove active_storage_s3 feature flag (#7627)#7630

Open
Shruti2110-coder wants to merge 10 commits into
CircuitVerse:masterfrom
Shruti2110-coder:remove-active-storage-s3#7627
Open

chore: remove active_storage_s3 feature flag (#7627)#7630
Shruti2110-coder wants to merge 10 commits into
CircuitVerse:masterfrom
Shruti2110-coder:remove-active-storage-s3#7627

Conversation

@Shruti2110-coder

@Shruti2110-coder Shruti2110-coder commented Jul 8, 2026

Copy link
Copy Markdown

Description

Removes the active_storage_s3 Flipper feature flag now that the Active Storage migration to S3 is complete and stable in production. Also fully removes the now-unused kt-paperclip gem and all remaining references to Paperclip across the codebase and test suite.

Related Issue

Fixes #7627

Type of Change

  • Chore (refactor / cleanup, no functional changes)

Changes Made

  • Removed active_storage_s3 entry from config/initializers/flipper.rb
  • Removed test setup/teardown referencing the flag in spec/rails_helper.rb
  • Removed duplicate specs that only covered the flag-disabled state
  • Removed Flipper.disable(:active_storage_s3) calls from API request specs
  • Removed the unused kt-paperclip gem from the Gemfile
  • Removed dead Paperclip.options[:command_path] config from config/environments/production.rb
  • Removed require "paperclip/matchers" and config.include Paperclip::Shoulda::Matchers from spec/spec_helper.rb

How Has This Been Tested?

Full suite: bundle exec rspec945 examples, 0 failures, 5 pending (pending specs are pre-existing, unrelated xdescribe-skipped notification specs)

Screenshots (if applicable)

N/A — backend/config cleanup, no UI changes.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (none needed)
  • My changes generate no new warnings
  • Existing tests updated/cleaned up to reflect removed flag/gem
  • New and existing unit tests pass locally with my changes (945 examples, 0 failures)
  • Any dependent changes have been merged and published

AI Declaration

This PR was developed with the assistance of an AI coding assistant (Claude), which helped with:

  • Debugging RuboCop lint failures (trailing whitespace/blank lines in spec_helper.rb)
  • Resolving the Gemfile.lock merge conflict with upstream/master
  • Verifying image upload/Active Storage functionality locally (manual testing walkthrough)

All code changes were reviewed and tested locally by me before pushing.

Summary by CodeRabbit

  • Refactor

    • Removed the legacy image attachment dependency and related Paperclip-only test matchers.
    • Stopped enabling the S3-backed image storage feature flag by default.
  • Tests

    • Updated controller/request specs to focus on currently supported (feature-enabled) image behavior.
    • Updated RSpec setup to align feature-flag defaults.
  • Chores

    • Added Coveralls to the test setup.
    • Cleaned up production configuration formatting.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This change removes the active_storage_s3 Flipper feature flag from default and test configuration, along with disabled-path controller and request-spec coverage. It also removes the kt-paperclip dependency and Paperclip matcher setup from test configuration, and deletes a blank line in the production configuration.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR also removes Paperclip gem/references and tweaks production config, which are outside #7627. Move Paperclip cleanup and config-only edits to a separate PR, or add them to the linked issue if intended.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The requested active_storage_s3 flag removal is implemented in app and test setup.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: removing the active_storage_s3 feature flag.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tachyons

tachyons commented Jul 8, 2026

Copy link
Copy Markdown
Member

Thanks, could you remove paperclip gem as well . Also make sure that you use PR template for description

@Shruti2110-coder

Copy link
Copy Markdown
Author

Hi @tachyons, thanks for the feedback! I've completed both requests
Let me know if there's anything else you'd like adjusted!

@safedep-bot

safedep-bot commented Jul 18, 2026

Copy link
Copy Markdown

vet Summary Report

This report is generated by vet

Policy Checks

  • ✅ Vulnerability
  • ✅ Malware
  • ✅ License
  • ✅ Popularity
  • ✅ Maintenance
  • ✅ Security Posture
  • ✅ Threats

Malicious Package Analysis

Active malicious package analysis was disabled. Learn more about enabling active package analysis

Malicious Package Analysis Report
Ecosystem Package Version Status Report
  • ℹ️ 0 packages have been actively analyzed for malicious behaviour.
  • ✅ No malicious packages found.

@tachyons

tachyons commented Jul 18, 2026

Copy link
Copy Markdown
Member
  • Ensure CI is green
  • Follow our PR template with AI declaration
  • Run the app locally and verify everything related to image uploads and display without any issues

@Shruti2110-coder

Copy link
Copy Markdown
Author

Hi @tachyons — all set:

  • ✅ Lint fixed and CI conflicts resolved (branch shows "Able to merge")
  • ✅ Tested image uploads locally — preview generates and displays correctly, no errors in logs
  • ✅ Added AI Declaration section to the PR description

CI checks just need workflow approval on your end. Let me know if anything else is needed!

Comment thread cv-frontend-vue
@tachyons

Copy link
Copy Markdown
Member

@Shruti2110-coder could you upload a screencsst showing that image preview upload and profile image upload are working fine and they work in previews including circuitverse contest page ?

@Shruti2110-coder

Copy link
Copy Markdown
Author
Screen.Recording.2026-07-24.at.10.16.57.AM.mov
Screen.Recording.2026-07-24.at.10.20.40.AM.mov

Recorded a walkthrough covering the areas you flagged:

Clip 1: Profile picture upload — working correctly ✅

Clip 2: Contest page — submitted a circuit, thumbnail/preview displays correctly on the contest entries page ✅

Note: I also tested the simulator's Image Annotation upload feature, and it currently fails due to an unrelated pre-existing bug — the hardcoded Imgur API Client-ID is dead/revoked (already tracked in #7342, not caused by this PR).

@tachyons

Copy link
Copy Markdown
Member

Could you fix the merge conflicts

@tachyons
tachyons enabled auto-merge (squash) July 24, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove feature flag active_storage_s3

3 participants