feat(ui): Serve pre-compressed assets#5133
Open
SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
Open
feat(ui): Serve pre-compressed assets#5133SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds Vite compression to emit gzip and brotli assets, updates the server to negotiate Accept-Encoding and serve precompressed or decompressed assets with appropriate headers, and expands tests to validate encoding negotiation and caching behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant FS as FileSystem
Client->>Server: GET /asset.js\nAccept-Encoding: br, gzip
Server->>Server: selectEncoding(Accept-Encoding)
alt br preferred and /asset.js.br exists
Server->>FS: open /asset.js.br
FS->>Server: br bytes
Server->>Client: 200 OK\nContent-Encoding: br\nVary: Accept-Encoding
else gzip preferred and /asset.js.gz exists
Server->>FS: open /asset.js.gz
FS->>Server: gzip bytes
Server->>Client: 200 OK\nContent-Encoding: gzip\nVary: Accept-Encoding
else gzip/br not accepted or not found, try uncompressed or decompress .gz
Server->>FS: open /asset.js or /asset.js.gz (for decompression)
FS->>Server: raw bytes (or gz bytes)
Server->>Client: 200 OK\nContent-Type: ...\n(Vary: Accept-Encoding if encoded variants considered)
else
Server->>Client: 404 Not Found or 500 on decompression error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🧹 Nitpick comments (3)
ui/app/src/Views.elm (1)
19-24: Finish thelibUrlcleanup.
viewno longer consumesmodel.libUrl, butui/app/src/Types.elm:13-28still carries the field andui/app/src/Main.elm:84-89,107-124still computes/passes it. Dropping those in the same refactor keeps the Elm model surface aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/Views.elm` around lines 19 - 24, The Model still defines and Main still computes/passes the libUrl field even though view no longer uses it; remove the libUrl field from the Model type in Types.elm (and any related type alias constructors) and update the code paths in Main.elm that build or update the Model to stop computing/passing libUrl (where the initial Model is created and where route/state is constructed/updated). Search for the Model type definition and any references to libUrl (e.g., places that set model.libUrl or pass libUrl into Model constructors) and delete those entries and their computations so the Model surface and Main functions align with the new view signature.ui/web.go (1)
33-46: KeepfileTypesaligned with the Vite compression list.The comment on Line 35 says this table must match
ui/app/vite.config.mjs, but the Vite side includes.jsonand.mjswhile this map cannot serve either. If one of those files is emitted intodist,/assets/*will 404 it.💡 Minimal sync fix
".html": {"text/html; charset=utf-8", true}, ".ico": {"image/vnd.microsoft.icon", true}, ".js": {"text/javascript; charset=utf-8", true}, + ".json": {"application/json", true}, + ".mjs": {"text/javascript; charset=utf-8", true}, ".svg": {"image/svg+xml", true},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` around lines 33 - 46, The fileTypes map is missing entries that Vite emits (.json and .mjs), causing 404s for those assets; update the fileTypes map (variable name fileTypes) to include ".json" with contentType "application/json; charset=utf-8" and ".mjs" with contentType "text/javascript; charset=utf-8", and set their varyEncoding booleans to match the existing JS/CSS entries (true) so the map stays aligned with ui/app/vite.config.mjs compression list.ui/web_test.go (1)
193-217: AssertVary: Accept-Encodingfor negotiated assets.These subtests verify
Content-Encoding, but not the header caches use to keep gzip/br/identity variants separate. A regression there would still pass this suite while breaking shared-cache behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web_test.go` around lines 193 - 217, The tests iterate files/encodings and call fetchWithEncoding and checkCachingHeaders but never assert the Vary header; update each subtest inside the switch (cases encGzip, encBrotli, encNone) to assert that res.Header().Get("Vary") includes "Accept-Encoding" (use the same require helpers in the file), referencing the existing symbols router, fetchWithEncoding, files, encGzip, encBrotli, encNone and checkCachingHeaders so negotiated assets are verified to advertise Vary: Accept-Encoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 29-36: The build ordering bug is that ui-elm is a sibling
prerequisite of common-build/common-test/common-lint so make -j may run them
concurrently; fix this by making ui-elm a prerequisite (or an order-only
prerequisite) of the common targets instead of a sibling of
build/test/lint—i.e., update the Makefile so common-build, common-test, and
common-lint list ui-elm as a dependency (or use the order-only | operator with
ui-elm) so that ui-elm completes before running
common-build/common-test/common-lint; adjust the build, test, and lint targets
to depend only on the common-* targets as needed.
In `@ui/app/index.html`:
- Around line 10-11: Index.html currently loads /src/main.js but doesn't
canonicalize a missing trailing slash for non-root route prefixes, causing built
assets to resolve under /assets/* instead of /{route-prefix}/assets/*; restore
slash canonicalization by adding an inline check in index.html (before the
<script type="module" src="/src/main.js">) that detects when location.pathname
equals the configured route prefix without a trailing slash and then redirects
(location.replace) to the same path with a trailing slash (or dynamically set
document.baseURI/document.query to include the prefixed base), ensuring all
hashed JS/CSS assets resolve under the prefixed /{route-prefix}/assets/*.
In `@ui/app/src/main.js`:
- Around line 6-14: Guard against malformed or missing localStorage values
before passing flags to Elm.Main.init: read
localStorage.getItem('firstDayOfWeek') and getItem('groupExpandAll') into
variables, and if they are null or not valid JSON, fall back to sensible
defaults (e.g., undefined or false) rather than calling JSON.parse directly;
similarly handle defaultCreator (which may be null) so Elm receives stable
primitives. Implement this by validating/parsing the strings with a small
safeParse helper or try/catch and then pass the sanitized values into the flags
object used in Elm.Main.init.
---
Nitpick comments:
In `@ui/app/src/Views.elm`:
- Around line 19-24: The Model still defines and Main still computes/passes the
libUrl field even though view no longer uses it; remove the libUrl field from
the Model type in Types.elm (and any related type alias constructors) and update
the code paths in Main.elm that build or update the Model to stop
computing/passing libUrl (where the initial Model is created and where
route/state is constructed/updated). Search for the Model type definition and
any references to libUrl (e.g., places that set model.libUrl or pass libUrl into
Model constructors) and delete those entries and their computations so the Model
surface and Main functions align with the new view signature.
In `@ui/web_test.go`:
- Around line 193-217: The tests iterate files/encodings and call
fetchWithEncoding and checkCachingHeaders but never assert the Vary header;
update each subtest inside the switch (cases encGzip, encBrotli, encNone) to
assert that res.Header().Get("Vary") includes "Accept-Encoding" (use the same
require helpers in the file), referencing the existing symbols router,
fetchWithEncoding, files, encGzip, encBrotli, encNone and checkCachingHeaders so
negotiated assets are verified to advertise Vary: Accept-Encoding.
In `@ui/web.go`:
- Around line 33-46: The fileTypes map is missing entries that Vite emits (.json
and .mjs), causing 404s for those assets; update the fileTypes map (variable
name fileTypes) to include ".json" with contentType "application/json;
charset=utf-8" and ".mjs" with contentType "text/javascript; charset=utf-8", and
set their varyEncoding booleans to match the existing JS/CSS entries (true) so
the map stays aligned with ui/app/vite.config.mjs compression list.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a680f60-4c3f-4133-a973-3606d414d908
⛔ Files ignored due to path filters (9)
ui/app/lib/bootstrap-4.6.2-dist/css/bootstrap.min.css.mapis excluded by!**/*.mapui/app/lib/font-awesome-4.7.0/fonts/FontAwesome.otfis excluded by!**/*.otfui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.eotis excluded by!**/*.eotui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.svgis excluded by!**/*.svgui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.ttfis excluded by!**/*.ttfui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.woffis excluded by!**/*.woffui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.woff2is excluded by!**/*.woff2ui/app/package-lock.jsonis excluded by!**/package-lock.jsonui/app/public/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (19)
MakefileREADME.mdui/app/.gitignoreui/app/Makefileui/app/index.htmlui/app/lib/bootstrap-4.6.2-dist/css/bootstrap.min.cssui/app/lib/font-awesome-4.7.0/css/font-awesome.cssui/app/lib/font-awesome-4.7.0/css/font-awesome.min.cssui/app/package.jsonui/app/script.jsui/app/src/Main.elmui/app/src/Types.elmui/app/src/Updates.elmui/app/src/Views.elmui/app/src/assets/elm-datepicker.cssui/app/src/main.jsui/app/vite.config.mjsui/web.goui/web_test.go
💤 Files with no reviewable changes (5)
- ui/app/src/Main.elm
- ui/app/src/Updates.elm
- ui/app/src/Types.elm
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.css
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.min.css
Content-Encoding now varies per request based on Accept-Encoding. Assets are stored only in compressed form (.gz level 9, .br quality 11), which decreases the binary size from ~42.8 MB (v0.31.1) to ~39.7 MB. Clients that accept neither gzip nor brotli receive a response decompressed in-memory from the .gz variant on every request. Adds vite-plugin-compression2 as a build dependency. Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
enable-compression
branch
from
1179122 to
6cb401d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/web.go (1)
133-141: Type assertions could panic if underlying implementation changes.The
f.(io.ReadSeeker)assertions on lines 139, 149, and 172 assumeembed.FSfiles implementio.ReadSeeker. While currently true, using the comma-ok idiom would be more defensive.♻️ Optional defensive refactor
- http.ServeContent(w, req, filePath, time.Time{}, f.(io.ReadSeeker)) + rs, ok := f.(io.ReadSeeker) + if !ok { + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + http.ServeContent(w, req, filePath, time.Time{}, rs)Also applies to: 143-151, 167-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` around lines 133 - 141, The direct type assertions f.(io.ReadSeeker) in the appFS.Open -> http.ServeContent paths can panic if the returned file doesn't implement io.ReadSeeker; change each assertion (the ones around appFS.Open and subsequent http.ServeContent calls) to use the comma-ok form, and if the file is not an io.ReadSeeker, read its contents into a bytes.Reader (or similar) that implements io.ReadSeeker and pass that to http.ServeContent; keep the existing setCachePolicy and header handling, and ensure you still defer f.Close() where appropriate and handle any read errors before calling http.ServeContent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/web.go`:
- Around line 33-46: fileTypes map is missing entries for ".mjs" and ".json" so
requests for those compressed-by-vite assets return 404; update the fileTypes
map to include ".mjs" with contentType "text/javascript; charset=utf-8" and
varyEncoding true, and ".json" with contentType "application/json;
charset=utf-8" and varyEncoding true so the server recognizes and serves those
assets (match the Vite compression pattern in vite.config.mjs).
---
Nitpick comments:
In `@ui/web.go`:
- Around line 133-141: The direct type assertions f.(io.ReadSeeker) in the
appFS.Open -> http.ServeContent paths can panic if the returned file doesn't
implement io.ReadSeeker; change each assertion (the ones around appFS.Open and
subsequent http.ServeContent calls) to use the comma-ok form, and if the file is
not an io.ReadSeeker, read its contents into a bytes.Reader (or similar) that
implements io.ReadSeeker and pass that to http.ServeContent; keep the existing
setCachePolicy and header handling, and ensure you still defer f.Close() where
appropriate and handle any read errors before calling http.ServeContent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb2a6fea-a43b-4505-9f4d-3a8505d0c78f
⛔ Files ignored due to path filters (1)
ui/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
ui/app/package.jsonui/app/vite.config.mjsui/web.goui/web_test.go
✅ Files skipped from review due to trivial changes (1)
- ui/app/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/vite.config.mjs
- ui/web_test.go
Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
enable-compression
branch
from
da1cb3d to
1d0631d
Compare
Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
enable-compression
branch
from
098716e to
86c678f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/web.go (1)
49-55:⚠️ Potential issue | 🟡 MinorRespect explicit
identityrejections.
encNonecurrently means both "serve identity" and "nothing acceptable". That makes headers likeAccept-Encoding: identity;q=0, gzip;q=0, br;q=0andAccept-Encoding: *;q=0fall through to the decompression path, so we return a representation the client explicitly refused. Please return a distinct "not acceptable" state fromselectEncodingand map it to406 Not Acceptableinstead of treating it as the uncompressed fallback. Adding those cases toTestSelectEncodingwould also lock this down.💡 Example shape of the fix
type encoding int const ( encNone encoding = iota encGzip encBrotli + encNotAcceptable ) func selectEncoding(header string) encoding { - brotli, gzip, wildcard := effectUnseen, effectUnseen, effectUnseen + brotli, gzip, identity, wildcard := effectUnseen, effectUnseen, effectUnseen, effectUnseen for part := range strings.SplitSeq(header, ",") { encAndQ := strings.SplitN(strings.TrimSpace(part), ";", 2) @@ switch strings.TrimSpace(encAndQ[0]) { case "br": brotli = effect case "gzip": gzip = effect + case "identity": + identity = effect case "*": wildcard = effect } } @@ + if identity == effectReject || (wildcard == effectReject && identity == effectUnseen) { + return encNotAcceptable + } return encNone }+ case encNotAcceptable: + http.Error(w, http.StatusText(http.StatusNotAcceptable), http.StatusNotAcceptable) + return case encNone: if f, err := appFS.Open(filePath + ".gz"); err == nil {Also applies to: 65-97, 153-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` around lines 49 - 55, selectEncoding currently conflates "serve identity" and "nothing acceptable" (encNone); add a distinct sentinel (e.g., encNotAcceptable) to the encoding type, update selectEncoding to return encNotAcceptable for cases like Accept-Encoding: identity;q=0 or Accept-Encoding: *;q=0 when no acceptable encoding exists, and change the caller(s) that handle selectEncoding to map encNotAcceptable to an HTTP 406 Not Acceptable response instead of falling back to uncompressed; also add test cases to TestSelectEncoding covering explicit identity rejection and wildcard rejection to lock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/web.go`:
- Around line 49-55: selectEncoding currently conflates "serve identity" and
"nothing acceptable" (encNone); add a distinct sentinel (e.g., encNotAcceptable)
to the encoding type, update selectEncoding to return encNotAcceptable for cases
like Accept-Encoding: identity;q=0 or Accept-Encoding: *;q=0 when no acceptable
encoding exists, and change the caller(s) that handle selectEncoding to map
encNotAcceptable to an HTTP 406 Not Acceptable response instead of falling back
to uncompressed; also add test cases to TestSelectEncoding covering explicit
identity rejection and wildcard rejection to lock behavior.
Spaceman1701
approved these changes
Apr 3, 2026
Contributor
Spaceman1701
left a comment
Spaceman1701
left a comment
There was a problem hiding this comment.
Lgtm, thanks for making those changes!
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
2 participants
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.
Pull Request Checklist
Which user-facing changes does this PR introduce?
Summary by CodeRabbit