chore(ui): Allow creating UI artefacts inside container#5102
Open
SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
Open
chore(ui): Allow creating UI artefacts inside container#5102SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
SoloJacobs wants to merge 3 commits intoprometheus:mainfrom
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplace Docker-based frontend/tooling with npm/Vite local builds; remove bundled Font Awesome CSS; embed and serve prebuilt app/dist with encoding-aware content negotiation and caching; extract operational/debug endpoints into a new weboperations package; update CI to use root .nvmrc and npm caching; adjust Makefile and main wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant Server as Alertmanager (ui/web.go)
participant Assets as Embedded app/dist
participant Decompress as Decompressor
Browser->>Server: GET /assets/app.js\nAccept-Encoding: br, gzip
activate Server
Server->>Assets: Lookup /assets/app.js
Assets-->>Server: file exists
alt Brotli variant exists
Server->>Assets: Read app.js.br
Assets-->>Server: br bytes
Server-->>Browser: 200\nContent-Encoding: br\nVary: Accept-Encoding
else Gzip variant exists
Server->>Assets: Read app.js.gz
Assets-->>Server: gz bytes
Server-->>Browser: 200\nContent-Encoding: gzip\nVary: Accept-Encoding
else Only gz stored but client requests identity
Server->>Assets: Read app.js.gz
Assets-->>Server: gz bytes
Server->>Decompress: decompress gz in-memory
Decompress-->>Server: uncompressed bytes
Server-->>Browser: 200\n[uncompressed bytes]
end
deactivate Server
sequenceDiagram
participant User as Browser
participant Index as index.html
participant MainJS as /src/main.js
participant Elm as Elm.Main
participant Storage as localStorage
User->>Index: Load /
Index->>MainJS: load module
MainJS->>Storage: read firstDayOfWeek, groupExpandAll, defaultCreator
Storage-->>MainJS: return values
MainJS->>Elm: init(flags)
Elm-->>MainJS: subscribe ports
Elm->>MainJS: persistFirstDayOfWeek(value)
MainJS->>Storage: write firstDayOfWeek
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
SoloJacobs
force-pushed
the
example-ui
branch
9 times, most recently
from
feb0289 to
92c42fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
ui/app/CONTRIBUTING.md (1)
11-21: Clarify command execution directory for new contributors.At Line 14 and Line 20, explicitly stating these commands should run from
ui/appwould reduce ambiguity.📝 Suggested doc tweak
To set up your environment and install all dependencies, run: ```sh +# from ui/app npm ciTo run the tests:
+# from ui/app make test</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@ui/app/CONTRIBUTING.mdaround lines 11 - 21, Update the CONTRIBUTING.md so
the two command examples explicitly state they must be executed from the ui/app
directory: add a short note like "from ui/app" immediately above thenpm ci
example and themake testexample (the sections showing thenpm ciandmake testcommands) so contributors know the correct working directory when running
those commands.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 60-67: The workflow calls npm in the container (step id
cache-paths) before ensuring Node/npm exists; either ensure the container image
includes Node/npm or explicitly install Node/npm inside the job before the
cache-paths step so npm config get cache succeeds; update the job to add an
installation step (or switch the container image) that provides Node/npm, and
keep the actions/cache@v4 step and path (${{ steps.cache-paths.outputs.npm-cache
}}) after that installation so the cache lookup is reliable in the container.In
@Makefile:
- Around line 30-46: The ui Elm generated Data files are only a prerequisite of
the assets target, so build/test/lint that depend on the ui-elm target can run
without src/Data present; update the Makefile so the ui-elm target depends on
$(FRONTEND_DIR)/src/Data (i.e., change the ui-elm rule to "ui-elm:
$(FRONTEND_DIR)/src/Data" or add that prerequisite) so that invoking
build/test/lint will ensure the OpenAPI-generated Data modules exist before
running "cd$(FRONTEND_DIR) && $ (MAKE) build" (you can then remove the redundant
dependency from the assets rule if desired).In
@template/Makefile:
- Around line 4-6: The Makefile target email.tmpl lists a prerequisite
inline-css.js but the recipe calls node ./inline-css (missing the .js
extension); update the recipe for the email.tmpl target to explicitly run the
same file named in the prerequisite (use node ./inline-css.js) or alternatively
change the prerequisite to match the executed script name—ensure the symbols
inline-css.js (prerequisite) and the recipe invocation (node ./inline-css.js)
are consistent.In
@ui/app/Makefile:
- Around line 41-44: The .build_stamp rule currently only depends on
$(ELM_FILES), index.html, and vite.config.mjs, so changes to the JS/CSS entry or
package config won't trigger rebuilds; update the .build_stamp dependency list
to include package.json, src/main.js (or your actual JS entry), and
src/assets/elm-datepicker.css (or other asset files) so that changes to those
files cause npm run build to run and .build_stamp to be updated; modify the
Makefile rule for .build_stamp to add these filenames alongside $(ELM_FILES),
index.html, and vite.config.mjs.- Around line 48-54: The Makefile recipe uses DOCKER_RUN_CURRENT_USER to start a
container named openapi-gen but lacks failure handling, allowing subsequent
docker cp operations to silently copy from an old container; update the recipe
(the block that runs openapi-gen, trap, and docker cp) to abort on error by
enabling set -e (or chain commands with &&) immediately before the docker run,
or check the docker run exit status and only proceed if it succeeded, and ensure
the temporary container is removed on both success and failure (keep the trap
but only run docker cp when the run returned success) so openapi-gen failures
cannot cause stale-copy corruption.In
@ui/app/vite.config.mjs:
- Around line 5-19: The Vite config uses defineConfig(exported object) but lacks
a relative base, causing built asset URLs to be absolute; update the object
passed to defineConfig (the export default defineConfig call) to include base:
"./" at top level so assets are emitted as relative URLs and resolve correctly
when ui.Register mounts the app on a prefixed path; ensure this change is
applied alongside the existing plugins (elm(), compression(...)) in the
vite.config module.In
@ui/web_test.go:
- Around line 152-159: The helper checkCachingHeaders currently treats two
different Cache-Control policies as interchangeable and never asserts Vary, so
update checkCachingHeaders to accept an explicit expectedCacheControl string and
a wantVary bool, assert that res.Header().Get("Cache-Control") equals
expectedCacheControl (not Contains), and when wantVary is true assert
res.Header().Get("Vary") == "Accept-Encoding" (or empty when false); then update
callers (tests that exercise "/", "/favicon.ico", and "/assets/*") to pass
"no-cache, no-store, must-revalidate" for root/favicon, "public,
max-age=31536000, immutable" for assets, and set wantVary appropriately based on
whether the response is compressed.In
@ui/web.go:
- Around line 185-187: The handler for r.Get("/assets/*path") sets a long-lived
Cache-Control header before calling serve, which causes 404/500 responses to be
cached; change the flow so the Cache-Control header is only added for successful
asset responses—either move the header assignment into serve and set it only
when the file is found and served (i.e., when you would write a 200 OK), or have
the handler check existence (using the same http.FileSystem/Open or os.Stat
logic serve uses) and only set Cache-Control after confirming the file exists;
ensure the 404 and error paths (the logic in serve that writes 404/500) do not
set the immutable max-age header.In
@weboperations/weboperations.go:
- Around line 31-38: The POST /-/reload handler can hang; change the local reply
channel to a buffered channel (errc := make(chan error, 1)), remove the defer
close(errc) (handler must not close a channel it doesn't own), and replace the
direct send and receive with context-aware selects that also listen on
req.Context().Done(); specifically, use select { case reloadCh <- errc: ... case
<-req.Context().Done(): return } for queuing the reload and another select {
case err := <-errc: ... case <-req.Context().Done(): return } for waiting for
the reply so both enqueue and reply are cancellation-safe.
Nitpick comments:
In@ui/app/CONTRIBUTING.md:
- Around line 11-21: Update the CONTRIBUTING.md so the two command examples
explicitly state they must be executed from the ui/app directory: add a short
note like "from ui/app" immediately above thenpm ciexample and themake testexample (the sections showing thenpm ciandmake testcommands) so
contributors know the correct working directory when running those commands.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `77ceba66-ab1f-485f-8bf8-870d44f0e778` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5f3188d4ef00ec05a1f75b8f1b0ddbbd7730ead6 and 92c42fbea6a5c61cd019546728e31a98e00169bb. </details> <details> <summary>⛔ Files ignored due to path filters (10)</summary> * `template/package-lock.json` is excluded by `!**/package-lock.json` * `ui/app/lib/bootstrap-4.6.2-dist/css/bootstrap.min.css.map` is excluded by `!**/*.map` * `ui/app/lib/font-awesome-4.7.0/fonts/FontAwesome.otf` is excluded by `!**/*.otf` * `ui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.eot` is excluded by `!**/*.eot` * `ui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.svg` is excluded by `!**/*.svg` * `ui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.ttf` is excluded by `!**/*.ttf` * `ui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.woff` is excluded by `!**/*.woff` * `ui/app/lib/font-awesome-4.7.0/fonts/fontawesome-webfont.woff2` is excluded by `!**/*.woff2` * `ui/app/package-lock.json` is excluded by `!**/package-lock.json` * `ui/app/public/favicon.ico` is excluded by `!**/*.ico` </details> <details> <summary>📒 Files selected for processing (31)</summary> * `.github/workflows/ci.yml` * `.github/workflows/ui-ci.yml` * `.nvmrc` * `Makefile` * `README.md` * `cmd/alertmanager/main.go` * `template/.gitignore` * `template/Dockerfile` * `template/Makefile` * `template/package.json` * `ui/Dockerfile` * `ui/app/.gitignore` * `ui/app/CONTRIBUTING.md` * `ui/app/Makefile` * `ui/app/index.html` * `ui/app/lib/bootstrap-4.6.2-dist/css/bootstrap.min.css` * `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` * `ui/app/package.json` * `ui/app/script.js` * `ui/app/src/Main.elm` * `ui/app/src/Types.elm` * `ui/app/src/Updates.elm` * `ui/app/src/Views.elm` * `ui/app/src/assets/elm-datepicker.css` * `ui/app/src/main.js` * `ui/app/vite.config.mjs` * `ui/web.go` * `ui/web_test.go` * `weboperations/weboperations.go` * `weboperations/weboperations_test.go` </details> <details> <summary>💤 Files with no reviewable changes (7)</summary> * ui/app/src/Main.elm * ui/app/src/Updates.elm * ui/Dockerfile * template/Dockerfile * ui/app/lib/font-awesome-4.7.0/css/font-awesome.min.css * ui/app/src/Types.elm * ui/app/lib/font-awesome-4.7.0/css/font-awesome.css </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ui/app/Makefile (1)
12-12: Add PHONY declarations forallandtesttargets.The static analysis tool correctly flags that
all(Line 12) andtest(Line 24) should be declared as.PHONYtargets since they don't produce files with those names. This prevents unexpected behavior if files namedallortestare ever created.Suggested fix
-.PHONY: build +.PHONY: all build test build: .build_stampAlso applies to: 38-39, 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/Makefile` at line 12, Add a .PHONY declaration for the make targets that do not produce files (at minimum include all and test) so make won’t treat those names as real files; update the Makefile to add a line like “.PHONY: all test” (and include any other non-file-producing targets referenced elsewhere in the file corresponding to the 38-39 and 58-58 occurrences) placed near the top with other meta-targets.ui/web.go (1)
139-139: Type assertionf.(io.ReadSeeker)may panic if the embedded FS changes.While
embed.FSfiles currently implementio.ReadSeeker, this is an unchecked type assertion. A defensive check would prevent a runtime panic if Go's embed implementation changes in the future.Defensive type assertion example
-http.ServeContent(w, req, filePath, time.Time{}, f.(io.ReadSeeker)) +rs, ok := f.(io.ReadSeeker) +if !ok { + http.Error(w, "internal error: file not seekable", http.StatusInternalServerError) + return +} +http.ServeContent(w, req, filePath, time.Time{}, rs)Also applies to: 149-149, 172-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` at line 139, The direct type assertion f.(io.ReadSeeker) can panic; change to a safe check and fallback: attempt a type assertion (rs, ok := f.(io.ReadSeeker)) and call http.ServeContent with rs if ok, otherwise read the file into memory (io.ReadAll(f)), wrap the bytes with bytes.NewReader(...) (which implements io.ReadSeeker) and pass that to http.ServeContent; ensure f is closed after open/read. Apply the same defensive pattern to the other occurrences referenced (the other http.ServeContent calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/Makefile`:
- Line 12: Add a .PHONY declaration for the make targets that do not produce
files (at minimum include all and test) so make won’t treat those names as real
files; update the Makefile to add a line like “.PHONY: all test” (and include
any other non-file-producing targets referenced elsewhere in the file
corresponding to the 38-39 and 58-58 occurrences) placed near the top with other
meta-targets.
In `@ui/web.go`:
- Line 139: The direct type assertion f.(io.ReadSeeker) can panic; change to a
safe check and fallback: attempt a type assertion (rs, ok := f.(io.ReadSeeker))
and call http.ServeContent with rs if ok, otherwise read the file into memory
(io.ReadAll(f)), wrap the bytes with bytes.NewReader(...) (which implements
io.ReadSeeker) and pass that to http.ServeContent; ensure f is closed after
open/read. Apply the same defensive pattern to the other occurrences referenced
(the other http.ServeContent calls).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ecf2950-c3cf-4bf6-8ed8-3160292886d6
⛔ 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 (25)
.github/workflows/ci.yml.github/workflows/ui-ci.yml.nvmrcMakefileREADME.mdcmd/alertmanager/main.goui/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.goweboperations/weboperations.goweboperations/weboperations_test.go
💤 Files with no reviewable changes (5)
- ui/app/src/Updates.elm
- ui/app/src/Main.elm
- ui/app/src/Types.elm
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.min.css
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.css
✅ Files skipped from review due to trivial changes (3)
- ui/app/.gitignore
- ui/app/package.json
- weboperations/weboperations_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ui-ci.yml
- cmd/alertmanager/main.go
- README.md
- ui/app/vite.config.mjs
- .github/workflows/ci.yml
- ui/app/src/main.js
- ui/web_test.go
- Makefile
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ui/web_test.go (1)
85-123: AddContent-Typeassertions while you're already walking every embedded file.This suite is the safety net for the new hard-coded
fileTypesmap, but it only checks status, caching, and content encoding. A MIME typo inui/web.gowould still return 200 here and slip through.Also applies to: 174-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web_test.go` around lines 85 - 123, walkEmbeddedFiles currently builds expected encodings per URL but doesn't assert Content-Type; update it (and the test loop that uses its result) to include and check the expected MIME for each file: while iterating fileTypes (via fileTypes[ext] and fileType.varyEncoding) capture fileType.mime and either return it alongside the encodings from walkEmbeddedFiles or assert it immediately; then in the request assertions (the test block that iterates the returned URLs/encodings around lines 174-215) verify the response header "Content-Type" equals fileType.mime for each url/encoding (use the same fileTypes lookup by extension to derive expected MIME). Ensure references to encGzip, encBrotli, encNone remain unchanged.
🤖 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 39-46: The assets target currently lists the phony ui-elm
prerequisite first which runs the Vite build against checked-in ui/app/src/Data
before $(FRONTEND_DIR)/src/Data is regenerated; change the prerequisite order so
the generated artifact $(FRONTEND_DIR)/src/Data (with its dependency on
api/v2/openapi.yaml) is listed before ui-elm (and keep template/email.tmpl),
ensuring Make updates src/Data first and only then runs the phony ui-elm build;
update the assets rule to depend on $(FRONTEND_DIR)/src/Data before ui-elm and
leave the existing $(FRONTEND_DIR)/src/Data: rule intact.
In `@ui/app/Makefile`:
- Around line 35-36: The Makefile target dev-server currently runs Elm Reactor
which cannot handle bundler-dependent imports in main.js; update the dev-server
recipe to start Vite instead (e.g., run the project's Vite dev command such as
"npx vite" or the equivalent npm/pnpm/yarn script like "npm run dev") so CSS and
npm package imports and the Elm build are served by the bundler; replace the
"npx elm reactor" invocation in the dev-server rule with the Vite start command
and ensure any package manager script name used matches the project's
package.json.
In `@ui/web.go`:
- Around line 64-96: selectEncoding currently conflates "no encoding (identity)
allowed" with "identity explicitly rejected"; add tracking for identity by
introducing an identity variable (initialized to effectUnseen) like
brotli/gzip/wildcard and set it when encountering "identity" (or
"identity;q=..."). At the end, if identity==effectReject or
(wildcard==effectReject && identity==effectUnseen) return a distinct sentinel
(e.g., encNotAcceptable or encRejectIdentity) instead of encNone; update callers
that use selectEncoding (and the encoding type) to handle this new sentinel and
respond with a 406 or similar. Ensure you reference selectEncoding, encoding,
encNone, brotli/gzip/wildcard, and the new identity and encNotAcceptable symbols
in your changes.
- Around line 123-127: The handler currently treats unknown file extensions by
returning http.Error with StatusInternalServerError; change this so that when
the extension lookup (ext := strings.ToLower(path.Ext(filePath)); fileType, ok
:= fileTypes[ext]) fails (ok == false) you call http.NotFound(w, r) and return
instead of sending an internal server error, so unknown/unsupported asset
requests (e.g. /assets/foo.txt) fall through as a 404 like other missing assets;
locate this logic around the ext/fileTypes check in the request handler (the
block using ext, fileTypes and http.Error) and replace the error response with
http.NotFound.
- Around line 30-31: The embed directive "//go:embed app/dist" referenced by the
variable "asset embed.FS" points to a non-existent folder and breaks
compilation; fix it by either committing a placeholder assets directory with the
expected files (so the embed pattern has matches) or change the embed directive
to reference an existing path/pattern that is present in the repository (or gate
the embed behind a build tag and provide a developer-friendly stub) so that the
variable "asset" is only built when matching files exist.
---
Nitpick comments:
In `@ui/web_test.go`:
- Around line 85-123: walkEmbeddedFiles currently builds expected encodings per
URL but doesn't assert Content-Type; update it (and the test loop that uses its
result) to include and check the expected MIME for each file: while iterating
fileTypes (via fileTypes[ext] and fileType.varyEncoding) capture fileType.mime
and either return it alongside the encodings from walkEmbeddedFiles or assert it
immediately; then in the request assertions (the test block that iterates the
returned URLs/encodings around lines 174-215) verify the response header
"Content-Type" equals fileType.mime for each url/encoding (use the same
fileTypes lookup by extension to derive expected MIME). Ensure references to
encGzip, encBrotli, encNone remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e22df37d-5a23-43f7-a16c-5a7367be2bdc
⛔ 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/Types.elm
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.min.css
- ui/app/lib/font-awesome-4.7.0/css/font-awesome.css
- ui/app/src/Updates.elm
✅ Files skipped from review due to trivial changes (3)
- ui/app/.gitignore
- ui/app/package.json
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/vite.config.mjs
- ui/app/src/main.js
SoloJacobs
added
waiting-for-user-input
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/app/Makefile`:
- Line 12: The Makefile's phony targets are missing: declare the targets all and
test as .PHONY to prevent accidental file name collisions (add a .PHONY: all
test line); also consider consolidating with the existing .PHONY declaration for
build (reference the targets "all", "test", and "build") so all phony targets
are declared together for maintainability.
In `@ui/app/src/main.js`:
- Around line 10-12: Wrap JSON.parse calls for the persisted flags to prevent
boot-time SyntaxError: replace direct
JSON.parse(localStorage.getItem('firstDayOfWeek')) and
JSON.parse(localStorage.getItem('groupExpandAll')) with a safe parse (either a
small try/catch around each parse or a shared safeParse helper) that returns a
sensible default (e.g., null/false) when parsing fails or value is null; keep
defaultCreator as-is (localStorage.getItem('defaultCreator')) and ensure the
code uses the fallback values when storage is invalid.
In `@ui/web_test.go`:
- Around line 71-77: The test loop is fetching the index URL (`urlPath`) instead
of each asset (`assetPath`), so change the call in the t.Run body to use
assetPath with fetchWithEncoding(router, assetPath, encNone) and ensure the
goroutine captures the correct iteration value (e.g., keep the existing
assetPath := path.Join(...) or rebind inside the loop before t.Run) so each
subtest actually verifies the individual asset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5df670c-37fc-47fe-bcdd-567e76850ff2
⛔ Files ignored due to path filters (10)
template/package-lock.jsonis excluded by!**/package-lock.jsonui/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 (31)
.github/workflows/ci.yml.github/workflows/ui-ci.yml.nvmrcMakefileREADME.mdcmd/alertmanager/main.gotemplate/.gitignoretemplate/Dockerfiletemplate/Makefiletemplate/package.jsonui/Dockerfileui/app/.gitignoreui/app/CONTRIBUTING.mdui/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.goweboperations/weboperations.goweboperations/weboperations_test.go
💤 Files with no reviewable changes (7)
- template/Dockerfile
- ui/app/src/Updates.elm
- ui/app/src/Main.elm
- ui/Dockerfile
- 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
✅ Files skipped from review due to trivial changes (5)
- template/.gitignore
- ui/app/.gitignore
- ui/app/CONTRIBUTING.md
- template/package.json
- ui/app/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/alertmanager/main.go
- .github/workflows/ci.yml
- ui/app/vite.config.mjs
- weboperations/weboperations.go
- ui/app/index.html
- .github/workflows/ui-ci.yml
- README.md
- Makefile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/web.go (1)
187-188: Consider explicit path validation for defense-in-depth clarity.Lines 187–188 construct the file path via
path.Join("assets", route.Param(...)). Whilepath.Joinwill collapse../segments (e.g.,../index.html→index.html), the subsequentappFS.Open()call on line 139/145/151/157 is protected byfs.Sub(asset, "app/dist")on line 118, which prevents directory escape. However, this protection is implicit and fragile; if the code were refactored away fromfs.Sub, the traversal path becomes exploitable.Adding explicit validation at the route handler boundary (checking for
..or usingfs.ValidPath()) would make the safety explicit and maintainable:Optional improvement
r.Get("/assets/*path", func(w http.ResponseWriter, req *http.Request) { + assetPath := route.Param(req.Context(), "path") + if !fs.ValidPath(assetPath) { + http.NotFound(w, req) + return + } - serve(w, req, path.Join("assets", route.Param(req.Context(), "path")), true) + serve(w, req, path.Join("assets", assetPath), true) })A test case for
/assets/../index.htmlwould also prevent regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` around lines 187 - 188, The handler building asset paths uses path.Join("assets", route.Param(req.Context(), "path")) (in the route GET "/assets/*path" and serve function) but relies implicitly on fs.Sub/appFS.Open for safety; add explicit validation at the route boundary by rejecting path parameters containing ".." or using fs.ValidPath() before calling serve, returning a 400/403 for invalid values to ensure defense-in-depth; update the serve invocation to only receive validated, cleanedPath and add a unit/integration test that requests "/assets/../index.html" to assert the server rejects it.
🤖 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_test.go`:
- Around line 62-75: fetchIndexAndAssets builds asset URLs with path.Join which
breaks for absolute asset references; replace that logic by parsing the base URL
(url.Parse) from urlPath and resolving each asset reference with
base.ResolveReference(url.Parse(match[1])) so absolute and relative asset
href/src values are handled correctly. Update fetchIndexAndAssets to build
assetPath using url.Parse + ResolveReference, ensure error handling for Parse
failures, and add the necessary import for net/url; keep the existing
fetchWithEncoding calls and test assertions unchanged.
---
Nitpick comments:
In `@ui/web.go`:
- Around line 187-188: The handler building asset paths uses path.Join("assets",
route.Param(req.Context(), "path")) (in the route GET "/assets/*path" and serve
function) but relies implicitly on fs.Sub/appFS.Open for safety; add explicit
validation at the route boundary by rejecting path parameters containing ".." or
using fs.ValidPath() before calling serve, returning a 400/403 for invalid
values to ensure defense-in-depth; update the serve invocation to only receive
validated, cleanedPath and add a unit/integration test that requests
"/assets/../index.html" to assert the server rejects it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3376ef2-c892-486f-a3df-4268a7ab0687
⛔ Files ignored due to path filters (1)
ui/app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
README.mdui/app/.gitignoreui/app/Makefileui/app/package.jsonui/app/vite.config.mjsui/web.goui/web_test.go
✅ Files skipped from review due to trivial changes (3)
- ui/app/.gitignore
- ui/app/vite.config.mjs
- ui/app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/Makefile
SoloJacobs
force-pushed
the
example-ui
branch
2 times, most recently
from
3e0cf27 to
02b283d
Compare
3 tasks
The GOPATH layout was only necessary before Go modules were introduced in Go 1.11. Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
This makes it so `make` will no longer skip targets, if files such as `ui/app/all` exist. More importantly, it will stop this from comming up in AI code reviews. Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
example-ui
branch
10 times, most recently
from
41daa5e to
9661487
Compare
SoloJacobs
changed the title
In previous commit, I replaced the dockerized `npm` builds with one that runs locally. This was necessary to build our our `UI` via `promu`. This commit adds back the capability to use `docker` to build our UI. Moreover, it introduces two new capabilities: * `podman` can be used to build `elm`. * `CA_BUNDLE` can be used to to pass a certificate bundle into the container. Fixes: prometheus#2848 Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
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
1 participant
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.
In previous commit, I replaced the dockerized
npmbuilds with one that runs locally. This was necessary to build our ourUIviapromu.podmancan be used to buildelm.CA_BUNDLEcan be used to to pass a certificate bundle into the container.Fixes: #2848