Feat/embed webui#224
Closed
yusing wants to merge 23 commits into
Closed
Conversation
Only patch the netip.Addr definition when it is present in swagger.json, avoiding a KeyError from newer or trimmed generator output. Prefer truthiness for x-nullable, tidy anyOf formatting, and end the file with a newline.
…lers Introduce the `handle <name>` command so rules can hand off a request to a handler registered in the in-process registry. Document it in the rules README and cover parsing plus end-to-end behavior in tests.
Introduce StaticProvider with a noop watcher: routes are returned from memory and the watcher never emits events, matching built-in use cases such as embedded WebUI assets. Document NewStaticProvider, Static in the package overview, and the Mermaid class diagram.
…ence When `Metadata.RootFS` is set, `FileServer` serves via `http.ServeFileFS` with nginx-like `try_files` for SPA: exact file, `index.html` under a directory, `.html` suffix, then the configured `Index` shell. Disk roots still require an absolute `Root` path. Add `ForceConflictWin` so built-in routes win `PreferOver`, skip `checkExists`, and drop the `godoxy-frontend` image rule-preset workaround from `validateRules`. Update the package README.
Register the built-in `webui` provider after other route providers, serving embedded assets from `webui.Dist` with aliases from `webui.aliases` or legacy `FRONTEND_ALIASES`. Log warnings when a provider or route would conflict and the embedded WebUI is preferred. Add `WebUIConfig` to top-level `Config`. Document behavior in the package README (diagram, security/performance notes, failure table).
Add godoxy-webui submodule and Dockerfile stages for install, schema gen, and Vite production build; ship dist with the Go binary. Makefile defaults WEBUI_DIR to ./webui, ensures dist for tests/builds, and adds build-webui. Remove godoxy-frontend from example compose files and GODOXY_FRONTEND_ ALIASES from env examples; document top-level webui.aliases in config.example.yml. Convert webui preset to block rules: dispatch API/auth to in-process handlers, tighten auth gating, and refresh preset docs; mirror handle api in webui_dev.
Introduce TEST_REGISTRY (default reg.i.sh) and update docker-build-test to
build ${TEST_REGISTRY}/godoxy, godoxy-agent, and godoxy-socket-proxy, then
push all three.
Contributor
|
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:
📝 WalkthroughWalkthroughEmbeds the Web UI into the GoDoxy binary, adds an in-process HTTP handler registry and a "handle" rule command, extends file-server/provider logic to serve embedded SPA assets with conflict resolution, updates build/CI to produce web UI artifacts, and simplifies several Docker/container API Swagger schemas. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as GoDoxy App
participant Rules as Rules Engine
participant Registry as Handler Registry
participant Handler as In-Process Handler
Client->>App: HTTP Request (/api/v1/...)
App->>Rules: Match rules for request
Rules->>Registry: Lookup "handle <name>"
Registry-->>Rules: Return handler
Rules->>Handler: Handler.ServeHTTP(w, r)
Handler-->>Client: Response (terminates rule flow)
sequenceDiagram
participant App as GoDoxy App
participant ConfigInit as state.Init
participant Providers as Provider Init
participant StaticProv as StaticProvider
participant FileSrv as FileServer (fs.FS)
participant Routes as Route Registry
App->>ConfigInit: state.Init(config bytes)
ConfigInit->>Providers: Load configured providers
ConfigInit->>StaticProv: initWebUIRoute() creates StaticProvider (aliases)
StaticProv->>FileSrv: Attach embedded fs.FS (SPA)
FileSrv->>FileSrv: Configure SPA fallback (exact → index → .html → shell)
StaticProv->>Routes: Register route with ForceConflictWin
Routes-->>App: Routes ready (conflict warnings logged if any)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/route/provider/docker_test.go (1)
360-385: 🧹 Nitpick | 🔵 TrivialRemove the duplicated Docker IP literal.
The fixture now uses
testDockerIP, but the local subtest still asserts through a second string with the same value. ReusingtestDockerIPin the expectation keeps the fixture and assertion from drifting apart later.♻️ Suggested cleanup
- privIP := "172.17.0.123" cont := &container.Summary{ Names: []string{"a"}, State: "running", NetworkSettings: &container.NetworkSettingsSummary{ @@ t.Run("local", func(t *testing.T) { r, ok := makeRoutes(cont)["a"] expect.True(t, ok) expect.NoError(t, r.Validate()) expect.Equal(t, r.Scheme, routeTypes.SchemeUDP) - expect.Equal(t, r.TargetURL().Hostname(), privIP) + expect.Equal(t, r.TargetURL().Hostname(), testDockerIP) expect.Equal(t, r.Port.Listening, 0) expect.Equal(t, r.Port.Proxy, int(privPort)) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/route/provider/docker_test.go` around lines 360 - 385, In TestStreamDefaultValues the test duplicates the Docker IP literal via the local variable privIP while the container fixture uses testDockerIP; update the assertion in the "local" subtest to compare r.TargetURL().Hostname() against testDockerIP (or remove the unused privIP) so the expectation references the single source of truth (testDockerIP) used when building cont; check the makeRoutes and r.TargetURL() usage to ensure no other assertions still use the duplicated literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/v1/docs/swagger.json`:
- Around line 6670-6672: Update the "signal" property description in the Swagger
schemas to fix the typo: change "If not value is set" to "If no value is set"
(ensure the full sentence reads that the default SIGTERM is used when no value
is set). Apply the same correction to both occurrences of the "signal"
description (the two request schemas where "signal" is defined).
In `@internal/api/v1/docs/swagger.yaml`:
- Around line 1672-1676: The parameter description containing "Signal (optional)
is the signal to send to the container..." has a typo: replace the phrase "If
not value is set" with "If no value is set" in both occurrences of that
description (the two "description" blocks that start with "Signal (optional)").
Update the description strings so the generated API docs read correctly.
In `@internal/config/state.go`:
- Around line 506-567: The initWebUIRoute function is over-complex due to the
nested loops that check provider conflicts for each alias; extract that
conflict-checking and logging into a small helper (e.g., checkWebUIAliasConflict
or aliasHasConflict) that accepts the alias string and state.providers.Range (or
state) and returns whether a conflict was found (and optionally the existing
provider name) so initWebUIRoute simply normalizes aliases, skips empties, calls
the helper to log any conflict, then proceeds to build the route; ensure the
helper uses the same logic (skip providerName "webui", call p.GetRoute(alias)
and log via state.tmpLog.Warn() with the same fields) and keep all other
behavior in initWebUIRoute unchanged.
In `@internal/config/types/config.go`:
- Around line 45-50: WebUIConfig currently allows empty strings in Aliases which
later causes runtime failures; add validation that rejects empty aliases by
updating/adding a validation step for WebUIConfig (e.g., implement
WebUIConfig.Validate or extend the existing config validation path) to iterate
over WebUIConfig.Aliases and return a descriptive error if any alias == "";
reference WebUIConfig and its Aliases field and ensure the error bubbles up
during config loading so Route.Validate later cannot encounter malformed empty
Alias values.
In `@internal/route/fileserver.go`:
- Around line 63-65: Extract the repeated literal "index.html" into a single
constant (e.g., const indexHTML = "index.html") and replace all occurrences in
this file: use indexHTML for the equality check against index, when appending to
indexCandidates, and where the other literal appears later in the same function;
ensure you reference the constant in comparisons and appends (replace uses
around index, indexCandidates, and the other check) rather than the raw string.
In `@internal/route/route.go`:
- Around line 770-775: The current conflict-resolution compares Rules before
honoring ForceConflictWin, so a route with ForceConflictWin can still lose; move
the ForceConflictWin checks to run before any Rules-based heuristic: in the
route comparison function where r and or (other route) are evaluated, check
r.ForceConflictWin vs or.ForceConflictWin first and return true/false
accordingly (use the existing r.ForceConflictWin and or.ForceConflictWin
fields), only falling back to the Rules heuristic (and other tie-breakers) if
neither side has ForceConflictWin set.
In `@internal/route/rules/do_test.go`:
- Around line 63-78: Add a test case to the table in
internal/route/rules/do_test.go that covers a whitespace-only handler name: add
an entry named like "handle_whitespace_name" with input set to `handle " "`
and wantErr set to ErrInvalidArguments so the new validator's normalization edge
case is locked down; place it alongside the existing "handle_valid",
"handle_missing_name", and "handle_too_many_args" cases.
In `@internal/route/rules/do.go`:
- Around line 177-185: The validate function in the do rule currently only
checks raw args[0]; normalize it (e.g., call strings.TrimSpace or the same
normalization used by the handler lookup) and verify the normalized name is not
empty before returning; if the normalized name is empty return
ErrInvalidArguments.Withf("empty handler name") (or similar) instead of
accepting quoted whitespace-only input so the error surfaces at config/parse
time; update the return to use the normalized string as parsedArgs.
- Around line 190-193: Replace the plain fmt.Errorf used when a handler is
missing with a gperr-built error: in the block that calls GetHandler(name) (and
currently returns fmt.Errorf("handler %q not found", name)), construct a
gperr.NewBuilder()/NewGroup() error (e.g.,
gperr.NewBuilder().PrependSubject("rule execution").NewGroup().Go(...)) or use
gperr.NewBuilder().Multiline() to produce a nested/pretty error that includes
the handler name, and return that instead of fmt.Errorf; keep the same condition
(if !ok) and ensure the returned error follows the file's existing gperr style
(use PrependSubject or similar to indicate the missing handler).
In `@internal/route/rules/handler_registry.go`:
- Around line 16-21: RegisterHandler currently overwrites existing handlers;
change it to fail fast on duplicates by using namedHandlers.LoadOrStore (or Load
then Store) to detect an existing entry and return an error instead of
overwriting; update the RegisterHandler signature from func RegisterHandler(name
string, handler http.Handler) to func RegisterHandler(name string, handler
http.Handler) error, keep normalizeHandlerName(name) and the nil/empty checks,
and if LoadOrStore reports an existing value return a clear error (e.g.,
"handler already registered: <name>") so callers can handle startup failures;
update all callers to handle the new error.
In `@Makefile`:
- Around line 9-10: Replace all hardcoded occurrences of "webui/..." in the
Makefile with the WEBUI_DIR (and DOCS_DIR where appropriate) so overriding
WEBUI_DIR actually changes build/probe paths; specifically, update targets and
recipe lines that reference paths like webui/src, webui/build, webui/wiki, etc.,
to use $(WEBUI_DIR)/... or $(DOCS_DIR) respectively, and ensure variable
definitions (WEBUI_DIR ?= $(shell pwd)/webui and DOCS_DIR ?= ${WEBUI_DIR}/wiki)
are used consistently throughout the Makefile instead of any literal "webui/"
strings.
- Around line 106-112: The docker-build-test target currently performs remote
writes by pushing images to ${TEST_REGISTRY}; remove the three docker push lines
from the docker-build-test target so it only builds the images, and add a
separate target (e.g. docker-push-test) that performs the docker push commands
or gate the pushes behind an explicit flag (e.g. MAKE PUSH=1) so local
invocations of docker-build-test never require registry credentials; modify the
Makefile to use the new target name docker-push-test (or check PUSH) and keep
docker-build-test limited to the build steps (referencing the docker-build-test
target and the TEST_REGISTRY variable to locate where the push logic should
move).
In `@scripts/fix-swagger-json.py`:
- Around line 10-21: Extract the literal "netip.Addr" into a clearly named
constant (e.g., NETIP_ADDR) at the top of the script and replace all three
occurrences in the block that checks/assigns data["definitions"]["netip.Addr"]
(the conditional that uses isinstance and the dictionary assignment) with that
constant; ensure you update both the membership check (data["definitions"]) and
the key access (data["definitions"][NETIP_ADDR]) so the script uses the single
constant everywhere.
---
Outside diff comments:
In `@internal/route/provider/docker_test.go`:
- Around line 360-385: In TestStreamDefaultValues the test duplicates the Docker
IP literal via the local variable privIP while the container fixture uses
testDockerIP; update the assertion in the "local" subtest to compare
r.TargetURL().Hostname() against testDockerIP (or remove the unused privIP) so
the expectation references the single source of truth (testDockerIP) used when
building cont; check the makeRoutes and r.TargetURL() usage to ensure no other
assertions still use the duplicated literal.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6279138a-b9e0-47fc-83ab-8830db4e922b
📒 Files selected for processing (35)
.env.example.gitmodulesAGENTS.mdDockerfileMakefilecompose.example.ymlconfig.example.ymlinternal/api/handler.gointernal/api/v1/docs/swagger.jsoninternal/api/v1/docs/swagger.yamlinternal/common/env.gointernal/config/README.mdinternal/config/state.gointernal/config/types/config.gointernal/route/README.mdinternal/route/fileserver.gointernal/route/fileserver_test.gointernal/route/provider/README.mdinternal/route/provider/docker_labels_test.gointernal/route/provider/docker_test.gointernal/route/provider/static.gointernal/route/route.gointernal/route/rules/README.mdinternal/route/rules/do.gointernal/route/rules/do_test.gointernal/route/rules/handler_registry.gointernal/route/rules/http_flow_block_test.gointernal/route/rules/presets/README.mdinternal/route/rules/presets/webui.ymlinternal/route/rules/presets/webui_dev.ymlrootless-compose.example.ymlrootless.env.examplescripts/fix-swagger-json.pyscripts/refresh-compat.shwebui
💤 Files with no reviewable changes (4)
- rootless.env.example
- .env.example
- rootless-compose.example.yml
- scripts/refresh-compat.sh
Comment on lines
6670
to
6672
| "signal": { | ||
| "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If no value is set, the default (SIGTERM) is used.", | ||
| "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If not value is set, the default (SIGTERM) is used.", | ||
| "type": "string" |
Contributor
There was a problem hiding this comment.
Fix the signal description typo in both request schemas.
If not value is set is grammatically incorrect and will leak into generated docs.
✏️ Proposed fix
- "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If not value is set, the default (SIGTERM) is used.",
+ "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If no value is set, the default (SIGTERM) is used.",
...
- "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If not value is set, the default (SIGTERM) is used.",
+ "description": "Signal (optional) is the signal to send to the container to (gracefully)\nstop it before forcibly terminating the container with SIGKILL after the\ntimeout expires. If no value is set, the default (SIGTERM) is used.",Also applies to: 6714-6716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/v1/docs/swagger.json` around lines 6670 - 6672, Update the
"signal" property description in the Swagger schemas to fix the typo: change "If
not value is set" to "If no value is set" (ensure the full sentence reads that
the default SIGTERM is used when no value is set). Apply the same correction to
both occurrences of the "signal" description (the two request schemas where
"signal" is defined).
Comment on lines
1672
to
1676
| description: |- | ||
| Signal (optional) is the signal to send to the container to (gracefully) | ||
| stop it before forcibly terminating the container with SIGKILL after the | ||
| timeout expires. If no value is set, the default (SIGTERM) is used. | ||
| timeout expires. If not value is set, the default (SIGTERM) is used. | ||
| type: string |
Contributor
There was a problem hiding this comment.
Fix the repeated signal-description typo.
If not value is set should be If no value is set in both operations; otherwise the generated API docs read broken in two places.
Also applies to: 1707-1711
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/v1/docs/swagger.yaml` around lines 1672 - 1676, The parameter
description containing "Signal (optional) is the signal to send to the
container..." has a typo: replace the phrase "If not value is set" with "If no
value is set" in both occurrences of that description (the two "description"
blocks that start with "Signal (optional)"). Update the description strings so
the generated API docs read correctly.
Comment on lines
+506
to
+567
| func (state *state) initWebUIRoute() error { | ||
| aliases := state.WebUI.Aliases | ||
| if len(aliases) == 0 { | ||
| aliases = common.FrontendAliasesLegacy | ||
| } | ||
| if len(aliases) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| routes := make(routeimpl.Routes, len(aliases)) | ||
| for _, alias := range aliases { | ||
| alias = strings.ToLower(strings.TrimSpace(alias)) | ||
| if alias == "" { | ||
| continue | ||
| } | ||
| for providerName, p := range state.providers.Range { | ||
| if providerName == "webui" { | ||
| continue | ||
| } | ||
| if existing, ok := p.GetRoute(alias); ok { | ||
| state.tmpLog.Warn(). | ||
| Str("alias", alias). | ||
| Str("existing_provider", existing.ProviderName()). | ||
| Msg("webui route conflicts with existing route; embedded webui route will be used") | ||
| } | ||
| } | ||
| routes[alias] = &routeimpl.Route{ | ||
| Scheme: routetypes.SchemeFileServer, | ||
| Root: "dist/client", | ||
| SPA: true, | ||
| Index: "_shell.html", | ||
| RuleFile: "embed://webui.yml", | ||
| HealthCheck: types.HealthCheckConfig{Disable: true}, | ||
| Homepage: &homepage.ItemConfig{ | ||
| Show: false, | ||
| }, | ||
| Metadata: routeimpl.Metadata{ | ||
| RootFS: webui.Dist(), | ||
| ForceConflictWin: true, | ||
| }, | ||
| InboundMTLSProfile: state.WebUI.InboundMTLSProfile, | ||
| Middlewares: state.WebUI.Middlewares, | ||
| AccessLog: state.WebUI.AccessLog, | ||
| } | ||
| } | ||
|
|
||
| if len(routes) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| webuiProvider := provider.NewStaticProvider("webui", routes) | ||
| if err := webuiProvider.LoadRoutes(); err != nil { | ||
| return err | ||
| } | ||
| if actual, loaded := state.providers.LoadAndStore(webuiProvider.String(), webuiProvider); loaded { | ||
| state.tmpLog.Warn(). | ||
| Str("provider", webuiProvider.String()). | ||
| Str("existing_type", string(actual.GetType())). | ||
| Msg("webui provider key already exists; replacing it with embedded webui route") | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting alias validation to reduce cognitive complexity.
SonarCloud flags this function's cognitive complexity at 16 (limit 15). The nested loops checking provider conflicts for each alias contribute significantly. Extracting the conflict-check logic into a helper would improve readability and satisfy the complexity threshold.
♻️ Proposed refactor to reduce complexity
+func (state *state) checkWebUIAliasConflicts(alias string) {
+ for providerName, p := range state.providers.Range {
+ if providerName == "webui" {
+ continue
+ }
+ if existing, ok := p.GetRoute(alias); ok {
+ state.tmpLog.Warn().
+ Str("alias", alias).
+ Str("existing_provider", existing.ProviderName()).
+ Msg("webui route conflicts with existing route; embedded webui route will be used")
+ }
+ }
+}
+
func (state *state) initWebUIRoute() error {
aliases := state.WebUI.Aliases
if len(aliases) == 0 {
aliases = common.FrontendAliasesLegacy
}
if len(aliases) == 0 {
return nil
}
routes := make(routeimpl.Routes, len(aliases))
for _, alias := range aliases {
alias = strings.ToLower(strings.TrimSpace(alias))
if alias == "" {
continue
}
- for providerName, p := range state.providers.Range {
- if providerName == "webui" {
- continue
- }
- if existing, ok := p.GetRoute(alias); ok {
- state.tmpLog.Warn().
- Str("alias", alias).
- Str("existing_provider", existing.ProviderName()).
- Msg("webui route conflicts with existing route; embedded webui route will be used")
- }
- }
+ state.checkWebUIAliasConflicts(alias)
routes[alias] = &routeimpl.Route{🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 506-506: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/state.go` around lines 506 - 567, The initWebUIRoute function
is over-complex due to the nested loops that check provider conflicts for each
alias; extract that conflict-checking and logging into a small helper (e.g.,
checkWebUIAliasConflict or aliasHasConflict) that accepts the alias string and
state.providers.Range (or state) and returns whether a conflict was found (and
optionally the existing provider name) so initWebUIRoute simply normalizes
aliases, skips empties, calls the helper to log any conflict, then proceeds to
build the route; ensure the helper uses the same logic (skip providerName
"webui", call p.GetRoute(alias) and log via state.tmpLog.Warn() with the same
fields) and keep all other behavior in initWebUIRoute unchanged.
Comment on lines
+63
to
65
| if index != "index.html" { | ||
| indexCandidates = append(indexCandidates, "index.html") | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract "index.html" to a constant to reduce duplication.
SonarCloud flags the literal "index.html" appearing 3 times (lines 63, 64, 88). Extracting to a constant improves maintainability.
♻️ Proposed fix
+const defaultIndexFile = "index.html"
+
func fsHandler(root fs.FS, spa bool, index string) http.Handler {
if !spa {
return http.FileServerFS(root)
}
index = strings.TrimPrefix(path.Clean("/"+index), "/")
indexCandidates := []string{index}
- if index != "index.html" {
- indexCandidates = append(indexCandidates, "index.html")
+ if index != defaultIndexFile {
+ indexCandidates = append(indexCandidates, defaultIndexFile)
}And at line 88:
return []string{
name,
- path.Join(name, "index.html"),
+ path.Join(name, defaultIndexFile),
name + ".html",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if index != "index.html" { | |
| indexCandidates = append(indexCandidates, "index.html") | |
| } | |
| const defaultIndexFile = "index.html" | |
| func fsHandler(root fs.FS, spa bool, index string) http.Handler { | |
| if !spa { | |
| return http.FileServerFS(root) | |
| } | |
| index = strings.TrimPrefix(path.Clean("/"+index), "/") | |
| indexCandidates := []string{index} | |
| if index != defaultIndexFile { | |
| indexCandidates = append(indexCandidates, defaultIndexFile) | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 63-63: Define a constant instead of duplicating this literal "index.html" 3 times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/route/fileserver.go` around lines 63 - 65, Extract the repeated
literal "index.html" into a single constant (e.g., const indexHTML =
"index.html") and replace all occurrences in this file: use indexHTML for the
equality check against index, when appending to indexCandidates, and where the
other literal appears later in the same function; ensure you reference the
constant in comparisons and appends (replace uses around index, indexCandidates,
and the other check) rather than the raw string.
Comment on lines
+190
to
+193
| handler, ok := GetHandler(name) | ||
| if !ok { | ||
| return fmt.Errorf("handler %q not found", name) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use gperr for the missing-handler path.
This error is part of rule execution flow, so it should use the same structured error package as the rest of this file instead of dropping back to fmt.Errorf.
♻️ Suggested change
handler, ok := GetHandler(name)
if !ok {
- return fmt.Errorf("handler %q not found", name)
+ return gperr.Errorf("handler %q not found", name)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/route/rules/do.go` around lines 190 - 193, Replace the plain
fmt.Errorf used when a handler is missing with a gperr-built error: in the block
that calls GetHandler(name) (and currently returns fmt.Errorf("handler %q not
found", name)), construct a gperr.NewBuilder()/NewGroup() error (e.g.,
gperr.NewBuilder().PrependSubject("rule execution").NewGroup().Go(...)) or use
gperr.NewBuilder().Multiline() to produce a nested/pretty error that includes
the handler name, and return that instead of fmt.Errorf; keep the same condition
(if !ok) and ensure the returned error follows the file's existing gperr style
(use PrependSubject or similar to indicate the missing handler).
Comment on lines
106
to
+112
| docker-build-test: | ||
| docker build -t godoxy . | ||
| docker build --build-arg=MAKE_ARGS=agent=1 -t godoxy-agent . | ||
| docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t godoxy-socket-proxy . | ||
| docker build -t ${TEST_REGISTRY}/godoxy . | ||
| docker build --build-arg=MAKE_ARGS=agent=1 -t ${TEST_REGISTRY}/godoxy-agent . | ||
| docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t ${TEST_REGISTRY}/godoxy-socket-proxy . | ||
| docker push ${TEST_REGISTRY}/godoxy | ||
| docker push ${TEST_REGISTRY}/godoxy-agent | ||
| docker push ${TEST_REGISTRY}/godoxy-socket-proxy |
Contributor
There was a problem hiding this comment.
Don't publish images from a build-only target by default.
docker-build-test now pushes three tags to ${TEST_REGISTRY} every time it runs. That turns a local verification target into a remote write, so local invocations will either fail without registry credentials or overwrite shared test tags unexpectedly.
Proposed fix
+PUSH ?= 0
+
docker-build-test:
docker build -t ${TEST_REGISTRY}/godoxy .
docker build --build-arg=MAKE_ARGS=agent=1 -t ${TEST_REGISTRY}/godoxy-agent .
docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t ${TEST_REGISTRY}/godoxy-socket-proxy .
- docker push ${TEST_REGISTRY}/godoxy
- docker push ${TEST_REGISTRY}/godoxy-agent
- docker push ${TEST_REGISTRY}/godoxy-socket-proxy
+ `@if` [ "$(PUSH)" = "1" ]; then \
+ docker push ${TEST_REGISTRY}/godoxy; \
+ docker push ${TEST_REGISTRY}/godoxy-agent; \
+ docker push ${TEST_REGISTRY}/godoxy-socket-proxy; \
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| docker-build-test: | |
| docker build -t godoxy . | |
| docker build --build-arg=MAKE_ARGS=agent=1 -t godoxy-agent . | |
| docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t godoxy-socket-proxy . | |
| docker build -t ${TEST_REGISTRY}/godoxy . | |
| docker build --build-arg=MAKE_ARGS=agent=1 -t ${TEST_REGISTRY}/godoxy-agent . | |
| docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t ${TEST_REGISTRY}/godoxy-socket-proxy . | |
| docker push ${TEST_REGISTRY}/godoxy | |
| docker push ${TEST_REGISTRY}/godoxy-agent | |
| docker push ${TEST_REGISTRY}/godoxy-socket-proxy | |
| PUSH ?= 0 | |
| docker-build-test: | |
| docker build -t ${TEST_REGISTRY}/godoxy . | |
| docker build --build-arg=MAKE_ARGS=agent=1 -t ${TEST_REGISTRY}/godoxy-agent . | |
| docker build --build-arg=MAKE_ARGS=socket-proxy=1 -t ${TEST_REGISTRY}/godoxy-socket-proxy . | |
| `@if` [ "$(PUSH)" = "1" ]; then \ | |
| docker push ${TEST_REGISTRY}/godoxy; \ | |
| docker push ${TEST_REGISTRY}/godoxy-agent; \ | |
| docker push ${TEST_REGISTRY}/godoxy-socket-proxy; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 106 - 112, The docker-build-test target currently
performs remote writes by pushing images to ${TEST_REGISTRY}; remove the three
docker push lines from the docker-build-test target so it only builds the
images, and add a separate target (e.g. docker-push-test) that performs the
docker push commands or gate the pushes behind an explicit flag (e.g. MAKE
PUSH=1) so local invocations of docker-build-test never require registry
credentials; modify the Makefile to use the new target name docker-push-test (or
check PUSH) and keep docker-build-test limited to the build steps (referencing
the docker-build-test target and the TEST_REGISTRY variable to locate where the
push logic should move).
Comment on lines
+10
to
21
| if "netip.Addr" in data["definitions"] and isinstance( | ||
| data["definitions"]["netip.Addr"], dict | ||
| ): | ||
| # MarshalText() | ||
| data["definitions"]["netip.Addr"] = { | ||
| "anyOf": [{ | ||
| "type": "string", | ||
| "format": "ipv4" | ||
| }, { | ||
| "type": "string", | ||
| "format": "ipv6" | ||
| }], | ||
| "anyOf": [ | ||
| {"type": "string", "format": "ipv4"}, | ||
| {"type": "string", "format": "ipv6"}, | ||
| ], | ||
| "x-nullable": False, | ||
| "x-omitempty": False | ||
| "x-omitempty": False, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract "netip.Addr" to a constant to reduce duplication.
SonarCloud flags the literal "netip.Addr" appearing 3 times. For a maintenance script this is minor, but extracting it improves readability.
♻️ Proposed fix
+NETIP_ADDR_KEY = "netip.Addr"
+
-if "netip.Addr" in data["definitions"] and isinstance(
- data["definitions"]["netip.Addr"], dict
+if NETIP_ADDR_KEY in data["definitions"] and isinstance(
+ data["definitions"][NETIP_ADDR_KEY], dict
):
# MarshalText()
- data["definitions"]["netip.Addr"] = {
+ data["definitions"][NETIP_ADDR_KEY] = {
"anyOf": [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if "netip.Addr" in data["definitions"] and isinstance( | |
| data["definitions"]["netip.Addr"], dict | |
| ): | |
| # MarshalText() | |
| data["definitions"]["netip.Addr"] = { | |
| "anyOf": [{ | |
| "type": "string", | |
| "format": "ipv4" | |
| }, { | |
| "type": "string", | |
| "format": "ipv6" | |
| }], | |
| "anyOf": [ | |
| {"type": "string", "format": "ipv4"}, | |
| {"type": "string", "format": "ipv6"}, | |
| ], | |
| "x-nullable": False, | |
| "x-omitempty": False | |
| "x-omitempty": False, | |
| } | |
| NETIP_ADDR_KEY = "netip.Addr" | |
| if NETIP_ADDR_KEY in data["definitions"] and isinstance( | |
| data["definitions"][NETIP_ADDR_KEY], dict | |
| ): | |
| # MarshalText() | |
| data["definitions"][NETIP_ADDR_KEY] = { | |
| "anyOf": [ | |
| {"type": "string", "format": "ipv4"}, | |
| {"type": "string", "format": "ipv6"}, | |
| ], | |
| "x-nullable": False, | |
| "x-omitempty": False, | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 10-10: Define a constant instead of duplicating this literal "netip.Addr" 3 times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/fix-swagger-json.py` around lines 10 - 21, Extract the literal
"netip.Addr" into a clearly named constant (e.g., NETIP_ADDR) at the top of the
script and replace all three occurrences in the block that checks/assigns
data["definitions"]["netip.Addr"] (the conditional that uses isinstance and the
dictionary assignment) with that constant; ensure you update both the membership
check (data["definitions"]) and the key access (data["definitions"][NETIP_ADDR])
so the script uses the single constant everywhere.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/types/config.go`:
- Line 50: Replace the permissive `dive,required` rule on the Aliases field with
a custom validator `alias_not_blank` that rejects strings that are only
whitespace: implement and register a validation function named "alias_not_blank"
(e.g., validator.RegisterValidation("alias_not_blank", func(fl
validator.FieldLevel) bool { s := fl.Field().String(); return
strings.TrimSpace(s) != "" })) and update the struct tag to
`validate:"dive,alias_not_blank"`; ensure you import strings and register this
validator during your validator initialization (the same place you set up other
validators) so initWebUIRoute no longer needs to ignore trimmed-empty aliases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b8e9919-7f84-45c3-9e1c-9d52fe86d1f4
📒 Files selected for processing (1)
internal/config/types/config.go
| Middlewares map[string]types.LabelMap `json:"middlewares,omitempty" extensions:"x-nullable"` | ||
| AccessLog *accesslog.RequestLoggerConfig `json:"access_log,omitempty" extensions:"x-nullable"` | ||
|
|
||
| Aliases []string `json:"aliases" validate:"dive,required"` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current validation tag and downstream trimming behavior (read-only).
rg -n --type=go -C2 'Aliases\s+\[\]string\s+`json:"aliases".*validate:' internal/config/types/config.go
rg -n --type=go -C3 'TrimSpace\(alias\)|initWebUIRoute|len\(aliases\)' internal/config/state.goRepository: yusing/godoxy
Length of output: 876
Reject whitespace-only aliases during validation.
The current validation validate:"dive,required" accepts whitespace-only strings (e.g., " "), which are trimmed to empty strings later in initWebUIRoute() and silently skipped. These invalid entries should be rejected during validation instead.
Introduce a custom validator alias_not_blank that rejects aliases that become empty after trimming whitespace, and update the validation tag accordingly:
Proposed fix
import (
"regexp"
+ "strings"
@@
- Aliases []string `json:"aliases" validate:"dive,required"`
+ Aliases []string `json:"aliases" validate:"dive,alias_not_blank"`
}
)
@@
serialization.MustRegisterValidation("non_empty_docker_keys", func(fl validator.FieldLevel) bool {
m := fl.Field().Interface().(map[string]types.DockerProviderConfig)
for k := range m {
if k == "" {
return false
}
}
return true
})
+ serialization.MustRegisterValidation("alias_not_blank", func(fl validator.FieldLevel) bool {
+ alias, ok := fl.Field().Interface().(string)
+ return ok && strings.TrimSpace(alias) != ""
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/config/types/config.go` at line 50, Replace the permissive
`dive,required` rule on the Aliases field with a custom validator
`alias_not_blank` that rejects strings that are only whitespace: implement and
register a validation function named "alias_not_blank" (e.g.,
validator.RegisterValidation("alias_not_blank", func(fl validator.FieldLevel)
bool { s := fl.Field().String(); return strings.TrimSpace(s) != "" })) and
update the struct tag to `validate:"dive,alias_not_blank"`; ensure you import
strings and register this validator during your validator initialization (the
same place you set up other validators) so initWebUIRoute no longer needs to
ignore trimmed-empty aliases.
Point ensure-webui-dist and build-webui at $(WEBUI_DIR) instead of a fixed webui/ path so WEBUI_DIR overrides apply consistently.
Move WebUIConfig out of config.go into its own file. Add Validate() to reject empty or whitespace-only aliases; drop struct tags in favor of this check.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 98-104: The ensure-webui-dist target currently skips generating
the WebUI when the godoxy variable equals "1", causing tests/builds to miss the
embedded asset check; update the Makefile so ensure-webui-dist always
verifies/produces $(WEBUI_DIR)/dist/client/_shell.html (remove the godoxy
conditional) or split into mode-specific targets and update test and build
prerequisites to depend on the unconditional artifact-check target; refer to the
ensure-webui-dist target, $(WEBUI_DIR), build-webui target, and the test target
when making the change.
- Around line 152-156: The build-webui recipe runs $(MAKE) gen-schema after cd
"$(WEBUI_DIR)", which causes make to look for gen-schema in the webui dir (where
no Makefile exists) and fail; fix this by invoking the gen-schema target from
the repo root instead of inside the webui context — e.g., run $(MAKE) gen-schema
before the cd or call make with an explicit directory (e.g., $(MAKE) -C
$(CURDIR) gen-schema) so the gen-schema target is executed in the root context;
update the build-webui target to call gen-schema from root while keeping bun i
and the vite build inside $(WEBUI_DIR).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1aea7d7-47b6-4226-ba3f-f477ec70d6e6
📒 Files selected for processing (3)
Makefileinternal/config/types/config.gointernal/config/types/webui_config.go
Comment on lines
+98
to
104
| ensure-webui-dist: | ||
| @if [ "${godoxy}" = "1" ] && [ ! -f "$(WEBUI_DIR)/dist/client/_shell.html" ]; then \ | ||
| $(MAKE) build-webui; \ | ||
| fi | ||
|
|
||
| test: | ||
| test: ensure-webui-dist | ||
| CGO_ENABLED=1 go test -v -race ${BUILD_FLAGS} ./internal/... |
Contributor
There was a problem hiding this comment.
Don't gate WebUI generation on godoxy=1.
This makes ensure-webui-dist a no-op for the non-default build modes, but test and build still depend on it. If those paths ever compile code that imports the embedded WebUI assets, a clean checkout will now fail because _shell.html never gets materialized. Make the artifact check unconditional, or move it behind mode-specific targets instead of a global prerequisite.
Also applies to: 158-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 98 - 104, The ensure-webui-dist target currently skips
generating the WebUI when the godoxy variable equals "1", causing tests/builds
to miss the embedded asset check; update the Makefile so ensure-webui-dist
always verifies/produces $(WEBUI_DIR)/dist/client/_shell.html (remove the godoxy
conditional) or split into mode-specific targets and update test and build
prerequisites to depend on the unconditional artifact-check target; refer to the
ensure-webui-dist target, $(WEBUI_DIR), build-webui target, and the test target
when making the change.
Comment on lines
+152
to
+156
| build-webui: | ||
| cd "$(WEBUI_DIR)" && \ | ||
| bun i --frozen-lockfile && \ | ||
| $(MAKE) gen-schema && \ | ||
| node ./node_modules/vite/bin/vite.js build |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^[[:space:]]*gen-schema:' -g 'Makefile' .
fd -a -t f 'Makefile' webuiRepository: yusing/godoxy
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for gen-schema more broadly (not just as Makefile target)
rg -i 'gen-schema' --no-heading --max-count=10
# Check if webui has package.json with scripts
if [ -f webui/package.json ]; then
echo "=== webui/package.json ==="
rg '"gen-schema"' webui/package.json
fi
# List webui directory structure
echo "=== webui directory structure ==="
fd -a -t f -d 2 . webui 2>/dev/null | head -20
# Check root Makefile around line 152-156
echo "=== Makefile gen-schema and build-webui context ==="
sed -n '140,170p' MakefileRepository: yusing/godoxy
Length of output: 977
🏁 Script executed:
#!/bin/bash
# Search for gen-schema target definition (not invocation)
rg -n '^gen-schema:' Makefile
# Also check entire Makefile to understand the structure
wc -l MakefileRepository: yusing/godoxy
Length of output: 70
🏁 Script executed:
#!/bin/bash
# Check if webui/package.json exists and has gen-schema script
cat webui/package.json 2>/dev/null | head -100 || echo "webui/package.json not found"
# Also check if gen-schema might be somewhere else entirely
echo "=== All references to gen-schema in repo ==="
rg -i 'gen-schema' -B 2 -A 2 | head -50Repository: yusing/godoxy
Length of output: 452
Invoke schema generation from the repo root.
After cd "$(WEBUI_DIR)", $(MAKE) gen-schema searches for the target in the webui directory context, where no Makefile exists. The gen-schema target is not defined anywhere in the codebase, so this recipe will fail. Move the schema generation outside the cd to invoke it from the root make context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 152 - 156, The build-webui recipe runs $(MAKE)
gen-schema after cd "$(WEBUI_DIR)", which causes make to look for gen-schema in
the webui dir (where no Makefile exists) and fail; fix this by invoking the
gen-schema target from the repo root instead of inside the webui context — e.g.,
run $(MAKE) gen-schema before the cd or call make with an explicit directory
(e.g., $(MAKE) -C $(CURDIR) gen-schema) so the gen-schema target is executed in
the root context; update the build-webui target to call gen-schema from root
while keeping bun i and the vite build inside $(WEBUI_DIR).
Cover initWebUIRoute when a static route already owns the alias: expect an embed://webui FileServerRoute and a conflict warning in the log. Smoke-test embedded dist with the webui rule preset: pages, API proxy, legacy auth redirect, SPA fallback, and static/hashed assets.
Restructure the Dockerfile into go-base and frontend-base, build non-main binaries in non-main-builder (no WebUI embed) and the main binary in main-builder (with webui dist). Expose scratch targets main, agent, and socket-proxy; pass inputs.target from the docker workflow; use those targets in Makefile docker-build-test; label the standalone socket-proxy.Dockerfile final stage.
…lers (#226) Introduce the `handle <name>` command so rules can hand off a request to a handler registered in the in-process registry. Document it in the rules README and cover parsing plus end-to-end behavior in tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Makefile (2)
98-104:⚠️ Potential issue | 🟠 MajorMake
ensure-webui-distunconditional for the test path.
testalways runs./internal/..., butensure-webui-distis still a no-op outsidegodoxy=1. On a clean checkout,make test agent=1/socket-proxy=1can now skip_shell.htmlgeneration and fail once the embedded WebUI tests compile or execute.Proposed fix
ensure-webui-dist: - `@if` [ "${godoxy}" = "1" ] && [ ! -f "$(WEBUI_DIR)/dist/client/_shell.html" ]; then \ + `@if` [ ! -f "$(WEBUI_DIR)/dist/client/_shell.html" ]; then \ $(MAKE) build-webui; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 98 - 104, The ensure-webui-dist target is conditional on godoxy and can be a no-op, causing tests invoked by the test target to run without generating required embedded WebUI artifacts; make the test target invoke ensure-webui-dist unconditionally (always run the ensure-webui-dist recipe before running CGO_ENABLED=1 go test ...), and adjust the ensure-webui-dist recipe so it always builds the WebUI (or at least checks/generates $(WEBUI_DIR)/dist/client/_shell.html) regardless of the godoxy variable so that tests depending on the embedded _shell.html cannot be skipped.
106-112:⚠️ Potential issue | 🟠 MajorKeep
docker-build-testbuild-only.This target still pushes to
${TEST_REGISTRY}by default, so a local smoke build now requires registry credentials and can overwrite shared test tags. Split the pushes into a separate target or gate them behind an explicit flag.Proposed fix
+PUSH ?= 0 + docker-build-test: docker build --target=main -t ${TEST_REGISTRY}/godoxy . docker build --target=agent --build-arg=MAKE_ARGS=agent=1 -t ${TEST_REGISTRY}/godoxy-agent . docker build --target=socket-proxy --build-arg=MAKE_ARGS=socket-proxy=1 -t ${TEST_REGISTRY}/godoxy-socket-proxy . - docker push ${TEST_REGISTRY}/godoxy - docker push ${TEST_REGISTRY}/godoxy-agent - docker push ${TEST_REGISTRY}/godoxy-socket-proxy + `@if` [ "$(PUSH)" = "1" ]; then \ + docker push ${TEST_REGISTRY}/godoxy; \ + docker push ${TEST_REGISTRY}/godoxy-agent; \ + docker push ${TEST_REGISTRY}/godoxy-socket-proxy; \ + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 106 - 112, The docker-build-test target is performing pushes which forces registry credentials and can overwrite shared tags; remove the docker push commands from the docker-build-test recipe and either create a new target (e.g., docker-push-test) that depends on docker-build-test to run the three docker push steps, or gate the pushes behind an explicit flag (e.g., require PUSH=1) so docker-build-test remains build-only; update the Makefile targets docker-build-test and (new) docker-push-test accordingly and ensure the new push logic references the same image names (${TEST_REGISTRY}/godoxy, ${TEST_REGISTRY}/godoxy-agent, ${TEST_REGISTRY}/godoxy-socket-proxy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/route/webui_smoke_test.go`:
- Around line 15-28: This test replaces package-level globals via
rules.RegisterHandler("api", ...) and rules.InitAuthHandler(...), but never
restores prior state; capture the current API handler and auth hook before
mutating (e.g. oldHandler := rules.RegisterHandler("api", current) or via
whatever getter your package provides) and oldAuth := currentAuthHook (or call
the existing getter), then defer restoring them with
rules.RegisterHandler("api", oldHandler) and rules.InitAuthHandler(oldAuth)
(handle nils correctly) so the original handler and auth hook are reinstated
after the smoke test.
- Around line 84-89: The test case named "underscored bundle asset embedded" is
pinned to a specific hashed asset path ("/assets/_-flIoYy1s.js"); change the
test to discover the JS asset at runtime instead of hardcoding the hash: use the
embedded filesystem used by the WebUI tests (e.g., read the embedded assets
directory via the package's embed.FS or the existing asset loader) to find a .js
filename that matches the underscored pattern and set path =
"/assets/"+foundName, or alternatively assert against a stable shell-exposed
path (index or manifest endpoint) rather than the hashed asset; update the test
case (the entry where path is set) to compute the path dynamically and keep
wantStatus/wantBody assertions the same.
---
Duplicate comments:
In `@Makefile`:
- Around line 98-104: The ensure-webui-dist target is conditional on godoxy and
can be a no-op, causing tests invoked by the test target to run without
generating required embedded WebUI artifacts; make the test target invoke
ensure-webui-dist unconditionally (always run the ensure-webui-dist recipe
before running CGO_ENABLED=1 go test ...), and adjust the ensure-webui-dist
recipe so it always builds the WebUI (or at least checks/generates
$(WEBUI_DIR)/dist/client/_shell.html) regardless of the godoxy variable so that
tests depending on the embedded _shell.html cannot be skipped.
- Around line 106-112: The docker-build-test target is performing pushes which
forces registry credentials and can overwrite shared tags; remove the docker
push commands from the docker-build-test recipe and either create a new target
(e.g., docker-push-test) that depends on docker-build-test to run the three
docker push steps, or gate the pushes behind an explicit flag (e.g., require
PUSH=1) so docker-build-test remains build-only; update the Makefile targets
docker-build-test and (new) docker-push-test accordingly and ensure the new push
logic references the same image names (${TEST_REGISTRY}/godoxy,
${TEST_REGISTRY}/godoxy-agent, ${TEST_REGISTRY}/godoxy-socket-proxy).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46154367-17d7-4ac3-b048-275b80f36fac
📒 Files selected for processing (6)
.github/workflows/docker-image.ymlDockerfileMakefileinternal/config/webui_route_test.gointernal/route/webui_smoke_test.gosocket-proxy.Dockerfile
Add ReplaceHandler (overwrite or delete) and GetAuthHandler so cleanup can restore prior api and auth handlers. Use ReplaceHandler for the api stub instead of RegisterHandler when the name may already exist.
…smoke Walk webui.Dist() for assets/_-<hash>.js instead of a fixed filename so the case keeps passing when Vite rewrites chunk hashes between builds.
Update the scc `cloc` target to scan `go,ts,tsx` and add `--not-match` for `.test.ts` and `.test.tsx`, mirroring the existing `_test.go` exclusion for Go.
|
Owner
Author
|
Merged in prep-0.29.0 branch |
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.
Summary by CodeRabbit
New Features
Removals
API
Docs / Build / Tests