Add Firestore caching and SEO meta tags#74
Conversation
dyoung522
temporarily deployed
to
workflow-test
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
This PR combines #60 and #66 by adding short-lived caching around Firestore “fetch all” queries for Mod/Tool, and introducing dynamic page titles + meta/OG tags via content_for and the application layout.
Changes:
- Cache
Mod.all/Tool.allviaRails.cache.fetch(5-minute TTL) and add manual invalidation methods. - Configure production to use
:memory_storecaching. - Add dynamic
<title>, meta description, and OpenGraph tags with per-page overrides in key views.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/mod.rb |
Wraps .all in Rails.cache.fetch, adds fetch_all and expire_cache, adds RuboCop class-length disable/enable. |
app/models/tool.rb |
Wraps .all in Rails.cache.fetch, adds fetch_all and expire_cache. |
config/environments/production.rb |
Enables :memory_store cache store with a size limit. |
app/views/layouts/application.html.erb |
Adds dynamic title, meta description, and OG tags with content_for defaults. |
app/views/mods/index.html.erb |
Sets content_for(:title) and content_for(:meta_description) (includes mod count). |
app/views/mods/show.html.erb |
Sets per-mod title/description and og:image when available. |
app/views/tools/index.html.erb |
Sets tools index title and meta description. |
spec/models/mod_spec.rb |
Adds caching specs for Mod.all and Mod.expire_cache. |
spec/models/tool_spec.rb |
Adds caching specs for Tool.all and Tool.expire_cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/models/mod.rb
Outdated
|
|
||
| require "net/http" | ||
|
|
||
| # rubocop:disable Metrics/ClassLength -- Will shrink when Displayable concern (#63) is merged |
There was a problem hiding this comment.
This RuboCop note says "Will shrink when ... (#63) is merged" but #63 appears to be an issue reference (issues don't get merged). Consider updating the reference to the actual PR number (e.g., #72) or rewording to avoid implying an issue will be merged.
| # rubocop:disable Metrics/ClassLength -- Will shrink when Displayable concern (#63) is merged | |
| # rubocop:disable Metrics/ClassLength -- Will shrink after extracting Displayable concern (see #63) |
spec/models/tool_spec.rb
Outdated
| it "caches results from .all" do | ||
| described_class.all | ||
| expect(Rails.cache.exist?("firestore/tools")).to be true | ||
| end |
There was a problem hiding this comment.
The "caches results from .all" example only asserts that the cache key exists after a single call, which doesn't actually prove the caching behavior (i.e., that the Firestore fetch is skipped on subsequent calls). Consider stubbing/expecting described_class.fetch_all (or the Firestore collection .get) and calling .all twice, asserting the underlying fetch happens only once.
spec/models/tool_spec.rb
Outdated
| original_store = Rails.cache | ||
| Rails.cache = ActiveSupport::Cache::MemoryStore.new | ||
| example.run | ||
| ensure | ||
| Rails.cache = original_store |
There was a problem hiding this comment.
Reassigning Rails.cache globally in an around hook can create order-dependent failures if specs are ever run in parallel (or if other examples rely on the default :null_store). Prefer stubbing Rails.cache (e.g., allow(Rails).to receive(:cache).and_return(store)) or using ActiveSupport::Cache.lookup_store with stub_const/around in a way that doesn't mutate global state.
| original_store = Rails.cache | |
| Rails.cache = ActiveSupport::Cache::MemoryStore.new | |
| example.run | |
| ensure | |
| Rails.cache = original_store | |
| store = ActiveSupport::Cache::MemoryStore.new | |
| allow(Rails).to receive(:cache).and_return(store) | |
| example.run |
spec/models/mod_spec.rb
Outdated
| it "caches results from .all" do | ||
| described_class.all | ||
| expect(Rails.cache.exist?("firestore/mods")).to be true | ||
| end |
There was a problem hiding this comment.
The "caches results from .all" example only checks that the cache key exists after one call, which doesn't verify that subsequent calls avoid hitting Firestore. Consider calling .all twice while asserting described_class.fetch_all (or the Firestore .get) is invoked only once.
spec/models/mod_spec.rb
Outdated
| original_store = Rails.cache | ||
| Rails.cache = ActiveSupport::Cache::MemoryStore.new | ||
| example.run | ||
| ensure | ||
| Rails.cache = original_store |
There was a problem hiding this comment.
Reassigning Rails.cache in this around hook mutates global state for the duration of the example, which can become flaky if the suite is parallelized or other specs depend on the default cache store. Prefer stubbing Rails.cache to a MemoryStore for these examples rather than assigning to Rails.cache=.
| original_store = Rails.cache | |
| Rails.cache = ActiveSupport::Cache::MemoryStore.new | |
| example.run | |
| ensure | |
| Rails.cache = original_store | |
| memory_store = ActiveSupport::Cache::MemoryStore.new | |
| allow(Rails).to receive(:cache).and_return(memory_store) | |
| example.run |
| def self.all | ||
| Rails.cache.fetch("firestore/tools", expires_in: 5.minutes) do | ||
| fetch_all | ||
| end | ||
| end | ||
|
|
||
| def self.fetch_all | ||
| firestore.col("tools").get.filter_map do |tool| | ||
| new( |
There was a problem hiding this comment.
fetch_all looks like an internal helper used only by .all to populate the cache, but it's currently a public class method (expanding the public API surface and making it easy to bypass caching accidentally). Consider marking it as private_class_method (or prefixing it as an internal method) so callers consistently use .all/.expire_cache.
| def self.all | ||
| Rails.cache.fetch("firestore/mods", expires_in: 5.minutes) do | ||
| fetch_all | ||
| end | ||
| end | ||
|
|
||
| def self.fetch_all | ||
| firestore.col("mods").get.filter_map do |mod| |
There was a problem hiding this comment.
Same as Tool: fetch_all is only referenced internally by .all and is now part of the public API. To reduce accidental cache bypass and keep the external API smaller, consider making fetch_all a private_class_method (or otherwise clearly internal).
Caching (#60): - Add Rails.cache.fetch with 5-minute TTL to Mod.all and Tool.all - Add expire_cache class methods for manual cache invalidation - Enable memory_store cache in production (single-server app) - Add caching specs for both models SEO (#66): - Dynamic page titles via content_for(:title) - Meta description tags with sensible defaults - OpenGraph tags (og:title, og:description, og:type, og:site_name) - Per-mod og:image when image_url is present - Mods index shows total mod count in meta description Closes #60, closes #66
dyoung522
force-pushed
the
enhance/caching-and-seo
branch
from
84218fb to
df7ff4f
Compare
dyoung522
temporarily deployed
to
workflow-test
GitHub Actions
Inactive
- Make fetch_all a private_class_method on Mod and Tool to prevent accidental cache bypass - Improve caching specs: verify fetch_all is called only once across two .all calls (proves caching works) - Use allow(Rails).to receive(:cache) instead of reassigning Rails.cache to avoid global state mutation
dyoung522
temporarily deployed
to
workflow-test
GitHub Actions
Inactive
Combined PR for #60 and #66.
Firestore Caching (#60)
Mod.allandTool.allnow useRails.cache.fetchwith a 5-minute TTLexpire_cacheclass methods for manual invalidationmemory_storecache in production (appropriate for single-server deploy)SEO Meta Tags (#66)
<title>tags:Page Name | ProjectDaedalus<meta name="description">with context-aware defaultsog:title,og:description,og:type,og:site_name)og:imagewhenimage_urlis present234 specs passing, RuboCop clean.
Closes #60, closes #66