-
-
Notifications
You must be signed in to change notification settings - Fork 0
Description
Hey Donovan ๐
I did an audit of the codebase and found a handful of issues โ nothing catastrophic but a few things worth fixing. Organized by severity below with suggested fixes for each.
๐ High โ preferred_type returns nil for EXMODZ-only mods
File: app/models/mod.rb (lines 48โ52)
def preferred_type
return :pak if pak?
:zip if zip?
endIf a mod only has an .exmodz file (no .pak or .zip), this returns nil. In _mod.html.erb, the download button only renders when preferred_type is truthy, so EXMODZ-only mods get no download button on the index listing.
Suggested fix:
def preferred_type
return :pak if pak?
return :zip if zip?
:exmodz if exmodz?
end๐ก Medium โ CSP not configured
File: config/initializers/content_security_policy.rb
The entire CSP config is commented out, so no Content-Security-Policy headers are sent. Given the app sanitizes markdown well and has no auth, this isn't urgent โ but a basic CSP would add defense-in-depth.
Suggested starter config:
Rails.application.configure do
config.content_security_policy do |policy|
policy.default_src :self, :https
policy.img_src :self, :https, :data, "raw.githubusercontent.com"
policy.script_src :self
policy.style_src :self, :unsafe_inline
end
end๐ก Medium โ Tool#filename crashes on nil URL
File: app/models/tool.rb (line 39)
def filename
url.split("/").last
endNo nil guard. If a tool in Firestore has no fileURL, this raises NoMethodError. The Mod model's filename method already has a nil guard โ this one just needs the same treatment.
Suggested fix:
def filename
url&.split("/")&.last
end๐ก Medium โ author_slug crashes on nil author
File: app/models/concerns/displayable.rb (line 17)
def author_slug
author.parameterize
endIf a Firestore document is missing the author field, nil.parameterize raises NoMethodError. Since author_slug is called during author filtering and in every mod row on the index page, a single bad record could crash the whole listing.
Suggested fix:
def author_slug
author&.parameterize || "unknown"
end๐ข Low โ .env file committed to repo
File: .env
The .gitignore has .env.* which catches .env.development etc. but doesn't catch the base .env file. The contents are just project IDs that are also in config/deploy.yml, so nothing sensitive is exposed โ but it's still good practice to exclude it.
Suggested fix:
.env
.env.*Then git rm --cached .env to stop tracking it.
๐ข Low โ _mod.html.erb uses mod.author instead of mod.author_slug in URL
File: app/views/mods/_mod.html.erb (line 3)
data-mods-path-param="<%= mod_detail_path(author: mod.author, slug: mod.slug) %>"This generates URLs with spaces like /mods/Donovan%20Young/mod-name instead of the cleaner /mods/donovan-young/mod-name. It doesn't cause 404s (the show action parameterizes the param before matching), just produces ugly URLs. The show.html.erb template already uses author_slug correctly.
Suggested fix:
data-mods-path-param="<%= mod_detail_path(author: mod.author_slug, slug: mod.slug) %>"๐ข Low โ Search debounce at 200ms
File: app/javascript/controllers/mods_controller.js (line 30)
this.timeout = setTimeout(() => {
event.target.form.requestSubmit();
}, 200)200ms is close to average inter-keystroke timing, so Turbo Frame requests fire after nearly every pause. Bumping to 400โ500ms would reduce unnecessary requests while still feeling responsive.
๐ข Low โ Markdown renderer objects recreated per call
File: app/helpers/application_helper.rb
def markdown(text)
coderayified = CodeRayify.new(filter_html: true, hard_wrap: true)
markdown_to_html = Redcarpet::Markdown.new(coderayified, options)
sanitize(markdown_to_html.render(text))
endA new CodeRayify and Redcarpet::Markdown instance is created on every call. On the tools index this runs once per tool. Minor perf improvement available by memoizing as a module-level constant.
๐ข Low โ require_master_key commented out in production
File: config/environments/production.rb (line 26)
# config.require_master_key = trueThe app can start without the master key. In practice Kamal injects it via secrets, so it's low risk โ but enabling this would catch misconfiguration early.
๐ข Low โ Firestore client not memoized
File: app/models/concerns/firestorable.rb
def self.firestore
# ...
Google::Cloud::Firestore.new(credentials: credentials.to_h)
endCreates a new gRPC client on every call. The 5-minute cache on Mod.all/Tool.all mitigates this, but memoizing with @firestore ||= would be cleaner.
Summary
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | ๐ High | mod.rb |
EXMODZ-only mods โ no download button on index |
| 2 | ๐ก Medium | content_security_policy.rb |
CSP not configured |
| 3 | ๐ก Medium | tool.rb |
filename crashes on nil URL |
| 4 | ๐ก Medium | displayable.rb |
author_slug crashes on nil author |
| 5 | ๐ข Low | .env |
Committed to repo (.gitignore misses it) |
| 6 | ๐ข Low | _mod.html.erb |
Raw author name in URL (cosmetic) |
| 7 | ๐ข Low | mods_controller.js |
200ms search debounce |
| 8 | ๐ข Low | application_helper.rb |
Markdown renderer not memoized |
| 9 | ๐ข Low | production.rb |
require_master_key disabled |
| 10 | ๐ข Low | firestorable.rb |
Firestore client not memoized |
Happy to open PRs for any of these if that would be helpful. Nice project! ๐ฎ