Adds support for path prefix configuration #139
Conversation
Introduces configurable path prefix to serve application under subpath (e.g., /pgbackweb) Implements validation for path prefixes in environment variables Updates all redirects and routes to use the configured path prefix This allows deploying the app at a custom URL path while maintaining correct functionality.
WalkthroughAdds PBW_PATH_PREFIX env var and validation; initializes a global path prefix at startup; introduces thread-safe pathutil (Set/Get/Build); updates router to mount routes and static assets under the prefix; replaces literal routes/assets with pathutil.BuildPath across middleware, handlers, components, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App (startup)
participant PathU as pathutil
participant Echo as Echo
participant Router as Router (baseGroup)
participant Handler as Handler
Note over App,PathU: Startup
App->>PathU: SetPathPrefix(env.PBW_PATH_PREFIX)
App->>Router: MountRouter(app, servs, env)
Note over User,Router: Incoming request to {prefix}/...
User->>Echo: GET {prefix}/dashboard
Echo->>Router: route via baseGroup
Router->>Handler: dispatch handler
alt Handler issues redirect
Handler->>PathU: BuildPath("/auth/login")
PathU-->>Handler: "{prefix}/auth/login"
Handler-->>User: 302 Redirect -> "{prefix}/auth/login"
else Serve resource
Handler-->>User: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/util/pathutil/pathutil_test.go (1)
13-13: Remove unusedshouldSkipfield.The
shouldSkipfield is defined in the test struct but never used in the test logic.Apply this diff:
tests := []struct { name string prefix string path string expected string - shouldSkip bool // Skip if prefix was already set }{internal/validate/path_prefix.go (1)
38-38: Consider adding vertical tab and form feed to whitespace check.The whitespace validation covers the most common cases. For completeness, you could include
\v(vertical tab) and\f(form feed) in the character set, though these are extremely rare in practice.- if strings.ContainsAny(pathPrefix, " \t\n\r") { + if strings.ContainsAny(pathPrefix, " \t\n\r\v\f") {internal/util/pathutil/pathutil.go (1)
30-35: Clarify or validate that path must start with "/".The current implementation assumes
pathstarts with "/". If called with a path like"dashboard"and prefix"/api", it would produce"/apidashboard"instead of"/api/dashboard".While all current call sites appear to use leading slashes, consider making this requirement explicit or adding defensive handling.
Option 1: Document the requirement.
// BuildPath constructs a full path by prepending the configured path prefix -// to the given path. If no prefix is configured, returns the path as-is. +// to the given path. The path argument must start with "/". +// If no prefix is configured, returns the path as-is. // // Examples:Option 2: Add defensive handling.
func BuildPath(path string) string { if pathPrefix == "" { return path } + // Ensure path starts with "/" to avoid malformed URLs + if path != "" && !strings.HasPrefix(path, "/") { + path = "/" + path + } return pathPrefix + path }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.md(2 hunks)cmd/app/main.go(3 hunks)internal/config/env.go(1 hunks)internal/config/env_validate.go(1 hunks)internal/util/pathutil/pathutil.go(1 hunks)internal/util/pathutil/pathutil_test.go(1 hunks)internal/validate/path_prefix.go(1 hunks)internal/validate/path_prefix_test.go(1 hunks)internal/view/middleware/require_auth.go(2 hunks)internal/view/middleware/require_no_auth.go(2 hunks)internal/view/router.go(2 hunks)internal/view/web/auth/create_first_user.go(3 hunks)internal/view/web/auth/login.go(3 hunks)internal/view/web/auth/logout.go(3 hunks)internal/view/web/dashboard/backups/create_backup.go(2 hunks)internal/view/web/dashboard/databases/create_database.go(2 hunks)internal/view/web/dashboard/destinations/create_destination.go(2 hunks)internal/view/web/dashboard/webhooks/create_webhook.go(2 hunks)internal/view/web/router.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
internal/view/web/router.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/router.go (3)
internal/view/web/router.go (1)
MountRouter(16-50)internal/view/api/router.go (1)
MountRouter(13-22)internal/config/env.go (1)
Env(10-16)
internal/view/web/dashboard/databases/create_database.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/config/env.go (3)
internal/service/destinations/destinations.go (1)
env(10-15)internal/service/auth/auth.go (1)
env(14-17)internal/service/executions/executions.go (1)
env(10-15)
internal/view/web/dashboard/backups/create_backup.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/middleware/require_no_auth.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/validate/path_prefix_test.go (1)
internal/validate/path_prefix.go (1)
PathPrefix(21-43)
internal/util/pathutil/pathutil_test.go (1)
internal/util/pathutil/pathutil.go (2)
BuildPath(30-35)GetPathPrefix(19-21)
internal/config/env_validate.go (1)
internal/validate/path_prefix.go (1)
PathPrefix(21-43)
cmd/app/main.go (2)
internal/util/pathutil/pathutil.go (1)
SetPathPrefix(12-16)internal/view/router.go (1)
MountRouter(15-34)
internal/util/pathutil/pathutil.go (4)
internal/util/strutil/create_path_test.go (1)
TestCreatePath(9-55)internal/util/strutil/create_path.go (1)
CreatePath(8-30)internal/util/strutil/remove_leading_slash_test.go (1)
TestRemoveLeadingSlash(5-50)internal/util/strutil/remove_trailing_slash.go (1)
RemoveTrailingSlash(4-10)
internal/view/web/auth/login.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/middleware/require_auth.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/validate/path_prefix.go (1)
internal/util/strutil/create_path.go (1)
CreatePath(8-30)
internal/view/web/auth/create_first_user.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/destinations/create_destination.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/create_webhook.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/auth/logout.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
🔇 Additional comments (21)
internal/view/web/dashboard/databases/create_database.go (1)
7-7: LGTM!The import and usage of
pathutil.BuildPathfor the redirect target correctly implements the path prefix feature. The change is consistent with the PR's objective to enable configurable subpath deployment.Also applies to: 45-45
internal/view/web/dashboard/destinations/create_destination.go (1)
5-5: LGTM!The path prefix integration is correctly implemented. The redirect now honors the configured
PBW_PATH_PREFIXviapathutil.BuildPath.Also applies to: 49-49
internal/view/web/dashboard/webhooks/create_webhook.go (1)
9-9: LGTM!The redirect target construction is correctly updated to use
pathutil.BuildPath, ensuring the webhook creation flow respects the configured path prefix.Also applies to: 58-58
internal/view/web/auth/create_first_user.go (1)
9-9: LGTM!Both redirect paths in the first-user creation flow now correctly use
pathutil.BuildPath, ensuring proper behavior when the app is served under a subpath.Also applies to: 33-33, 139-139
internal/view/web/dashboard/backups/create_backup.go (1)
10-10: LGTM!The backup creation handler now correctly constructs the redirect target with
pathutil.BuildPath, aligning with the path prefix feature.Also applies to: 74-74
internal/view/web/auth/login.go (1)
8-8: LGTM!Both redirect paths in the login flow are now correctly constructed using
pathutil.BuildPath, ensuring the authentication flow works properly under a configured subpath.Also applies to: 32-32, 112-112
internal/config/env_validate.go (1)
19-21: LGTM!The path prefix validation is correctly integrated, ensuring
PBW_PATH_PREFIXmeets the required format before the application starts. The error message clearly communicates the validation rules.internal/view/middleware/require_no_auth.go (1)
6-6: LGTM!The middleware correctly uses
pathutil.BuildPathto construct the redirect target, ensuring both HTMX and standard HTTP redirects respect the configured path prefix.Also applies to: 17-19
internal/view/web/auth/logout.go (1)
4-4: LGTM! Clean integration of path prefix support.The redirect paths now correctly use
pathutil.BuildPath()to respect the configured path prefix. The changes are minimal, focused, and maintain the existing logic.Also applies to: 19-19, 32-32
internal/view/middleware/require_auth.go (1)
7-7: LGTM! Middleware redirects now respect path prefix.The authentication flow correctly uses
pathutil.BuildPath()for both redirect scenarios (first-user creation and login). The implementation is consistent with other redirect usages across the codebase.Also applies to: 33-40
internal/config/env.go (1)
15-15: LGTM! Path prefix field added correctly.The new
PBW_PATH_PREFIXfield is properly configured with an empty default, making the path prefix feature opt-in. The field naming and tags follow the existing conventions in the struct.README.md (1)
82-82: LGTM! Clear documentation for path prefix feature.The README provides clear guidance on the
PBW_PATH_PREFIXconfiguration with:
- Practical example in the Docker Compose snippet
- Explicit validation rules (must start with
/, must not end with/)- Clear use case explanation (serving under a subpath)
Also applies to: 123-125
internal/view/web/router.go (1)
8-8: LGTM! Root path handler now prefix-aware.All redirect paths in the root handler correctly use
pathutil.BuildPath():
- Dashboard redirect for authenticated users
- First-user creation for new installations
- Login page for unauthenticated users
The implementation is consistent and maintains the existing conditional logic.
Also applies to: 25-25, 39-39, 42-42
internal/validate/path_prefix_test.go (1)
5-78: LGTM! Comprehensive validation test coverage.The test suite thoroughly validates the path prefix rules:
- Valid cases: empty string, simple paths, nested paths
- Invalid cases: missing leading slash, trailing slash, only slash, whitespace characters
The table-driven approach makes the test cases clear and maintainable.
internal/view/router.go (1)
6-6: LGTM! Clean architecture for path prefix support.The router refactoring elegantly introduces path prefix support:
- Signature update:
MountRouternow acceptsenvto accessPBW_PATH_PREFIX- Base group creation: All routes derived from
baseGroup, which applies the prefix- Comprehensive coverage: Static files, API routes, and web routes all respect the prefix
When
PBW_PATH_PREFIXis empty (default),app.Group("")creates a group at the root level, maintaining backward compatibility.Also applies to: 15-19, 27-33
internal/validate/path_prefix.go (1)
21-43: LGTM! Clean and well-documented validation logic.The validation rules are appropriate for path prefix use cases. The function correctly handles empty strings, leading/trailing slashes, and whitespace.
cmd/app/main.go (3)
11-11: LGTM! Necessary import for path prefix initialization.
22-23: LGTM! Path prefix initialized at the right time.The initialization occurs after environment validation and before router mounting, which is the correct sequence.
51-51: LGTM! Router signature updated to accept environment config.This change aligns with the router needing access to
PBW_PATH_PREFIXfor base path configuration.internal/util/pathutil/pathutil.go (2)
5-16: LGTM! Thread-safe initialization with sync.Once.The use of
sync.Onceensures idempotent, thread-safe initialization. The happens-before guarantee means that subsequent reads inGetPathPrefixandBuildPathare safe without additional synchronization, given thatSetPathPrefixis called during startup before concurrent operations.
18-21: LGTM! Simple and correct getter.
Updates tests to properly manage global state: - Uses defer pattern to restore original values after tests - Resets sync.Once to allow multiple SetPathPrefix calls in tests - Removes shouldSkip field from test cases as it's no longer needed
Refactors URL handling to allow serving the app from a subdirectory. Updates all static file paths and dashboard navigation links to use a utility function that prepends the configured path prefix when generating URLs.
Consistently uses the pathutil.BuildPath function to build URLs across various web components instead of hardcoding them.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/view/web/dashboard/databases/list_databases.go (1)
69-69: Apply path prefix consistently to all hard-coded paths.Lines 69 and 108-110 contain hard-coded paths that should also use
pathutil.BuildPathto maintain consistency with the path prefix configuration. Currently, only line 77 uses the path builder, which creates an inconsistency.Apply this diff to fix the inconsistency:
component.OptionsDropdownA( nodx.Href( - fmt.Sprintf("/dashboard/executions?database=%s", database.ID), + pathutil.BuildPath(fmt.Sprintf("/dashboard/executions?database=%s", database.ID)), ), nodx.Target("_blank"),if pagination.HasNextPage { trs = append(trs, nodx.Tr( - htmx.HxGet(fmt.Sprintf( - "/dashboard/databases/list?page=%d", pagination.NextPage, - )), + htmx.HxGet(pathutil.BuildPath(fmt.Sprintf( + "/dashboard/databases/list?page=%d", pagination.NextPage, + ))), htmx.HxTrigger("intersect once"), htmx.HxSwap("afterend"),Also applies to: 108-110
🧹 Nitpick comments (2)
internal/view/web/dashboard/databases/delete_database.go (1)
31-31: Path prefix support correctly applied.The change correctly wraps the URL with
pathutil.BuildPathto support subpath deployments.Consider using
fmt.Sprintffor clearer path construction:- htmx.HxDelete(pathutil.BuildPath("/dashboard/databases/"+databaseID.String())), + htmx.HxDelete(pathutil.BuildPath(fmt.Sprintf("/dashboard/databases/%s", databaseID))),This approach separates the format string from the data and is more maintainable.
internal/view/web/dashboard/destinations/list_destinations.go (1)
75-75: Consider usingfmt.Sprintffor path construction.The
pathutil.BuildPathusage is correct and properly handles the path prefix. However, for consistency with other path constructions in this file (lines 67, 134-136), consider usingfmt.Sprintfinstead of string concatenation.Apply this diff for improved readability:
- htmx.HxPost(pathutil.BuildPath("/dashboard/destinations/"+destination.ID.String()+"/test")), + htmx.HxPost(pathutil.BuildPath(fmt.Sprintf("/dashboard/destinations/%s/test", destination.ID))),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
internal/view/web/auth/create_first_user.go(4 hunks)internal/view/web/auth/login.go(4 hunks)internal/view/web/dashboard/backups/create_backup.go(4 hunks)internal/view/web/dashboard/backups/delete_backup.go(2 hunks)internal/view/web/dashboard/backups/duplicate_backup.go(2 hunks)internal/view/web/dashboard/backups/edit_backup.go(2 hunks)internal/view/web/dashboard/backups/index.go(2 hunks)internal/view/web/dashboard/backups/manual_run.go(2 hunks)internal/view/web/dashboard/databases/delete_database.go(2 hunks)internal/view/web/dashboard/databases/index.go(2 hunks)internal/view/web/dashboard/databases/list_databases.go(2 hunks)internal/view/web/dashboard/destinations/delete_destination.go(2 hunks)internal/view/web/dashboard/destinations/index.go(2 hunks)internal/view/web/dashboard/destinations/list_destinations.go(2 hunks)internal/view/web/dashboard/executions/restore_execution.go(3 hunks)internal/view/web/dashboard/executions/soft_delete_execution.go(2 hunks)internal/view/web/dashboard/profile/close_all_sessions.go(2 hunks)internal/view/web/dashboard/profile/update_user.go(2 hunks)internal/view/web/dashboard/webhooks/create_webhook.go(4 hunks)internal/view/web/dashboard/webhooks/delete_webhook.go(2 hunks)internal/view/web/dashboard/webhooks/duplicate_webhook.go(2 hunks)internal/view/web/dashboard/webhooks/edit_webhook.go(3 hunks)internal/view/web/dashboard/webhooks/index.go(2 hunks)internal/view/web/dashboard/webhooks/run_webhook.go(2 hunks)internal/view/web/layout/dashboard_header.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/view/web/auth/create_first_user.go
- internal/view/web/auth/login.go
🧰 Additional context used
🧬 Code graph analysis (23)
internal/view/web/dashboard/executions/soft_delete_execution.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/destinations/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/edit_webhook.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/backups/edit_backup.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/destinations/list_destinations.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/databases/list_databases.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/databases/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/executions/restore_execution.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/backups/create_backup.go (2)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/databases/delete_database.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/delete_webhook.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/profile/close_all_sessions.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/backups/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/destinations/delete_destination.go (2)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)internal/view/web/dashboard/destinations/edit_destination.go (1)
editDestinationButton(51-187)
internal/view/web/layout/dashboard_header.go (2)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)internal/view/web/layout/dashboard.go (1)
Dashboard(14-43)
internal/view/web/dashboard/backups/delete_backup.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/duplicate_webhook.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/create_webhook.go (3)
internal/view/web/respondhtmx/respond.go (1)
Redirect(78-82)internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)internal/view/web/dashboard/destinations/create_destination.go (1)
h(23-49)
internal/view/web/dashboard/profile/update_user.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/backups/manual_run.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/run_webhook.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/backups/duplicate_backup.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
🔇 Additional comments (35)
internal/view/web/layout/dashboard_header.go (1)
4-4: LGTM! Path prefix integration is correct.The addition of
pathutil.BuildPathwrapping for both HTMX endpoint URLs correctly implements the path prefix feature. The external Discord link at line 37 is appropriately left unwrapped since it's an absolute external URL.Also applies to: 32-32, 44-44
internal/view/web/dashboard/executions/restore_execution.go (3)
10-10: LGTM!The import addition is correctly placed and necessary for the path prefix functionality.
111-111: LGTM!The path construction correctly applies the configured path prefix to the restore execution endpoint.
226-226: LGTM!The path construction correctly applies the configured path prefix to the restore form endpoint.
internal/view/web/dashboard/webhooks/duplicate_webhook.go (2)
4-4: LGTM!The import of
pathutilis necessary for theBuildPathcall on line 31 and aligns with the PR objective to support configurable path prefixes.
31-31: LGTM!The change to use
pathutil.BuildPathfor the webhook duplication route is correct and aligns with the PR objective to support configurable path prefixes. The path construction is straightforward and consistent with the pattern established elsewhere in the codebase.internal/view/web/dashboard/databases/list_databases.go (2)
11-11: LGTM: Import added for path prefix support.The import of
internal/util/pathutilis correctly added to support the new path prefix functionality.
77-77: LGTM: Path prefix correctly applied to HTMX endpoint.The usage of
pathutil.BuildPathcorrectly applies the configured path prefix to the test connection endpoint.internal/view/web/dashboard/databases/delete_database.go (1)
4-4: LGTM!The import addition is correct and necessary for the path prefix support introduced in this PR.
internal/view/web/dashboard/profile/close_all_sessions.go (2)
5-5: LGTM!The import addition is necessary for using
pathutil.BuildPathand correctly integrates with the path prefix feature.
19-19: LGTM!The change correctly applies the path prefix to the logout endpoint using
pathutil.BuildPath, ensuring the button action works correctly when the application is served under a custom subpath.internal/view/web/dashboard/webhooks/run_webhook.go (2)
7-7: LGTM!The import of
pathutilis correctly added to support the path prefix configuration introduced in this PR.
45-52: LGTM!The migration from a hard-coded path to
pathutil.BuildPathcorrectly implements the configurable path prefix support. The URL construction properly incorporates the webhook ID and maintains the expected route structure.internal/view/web/dashboard/webhooks/edit_webhook.go (3)
9-9: LGTM: Import added to support path prefix functionality.The pathutil import is correctly added to enable the use of
pathutil.BuildPathfor constructing prefixed URLs.
105-105: LGTM: Form submission URL now supports path prefix.The hard-coded URL has been correctly wrapped with
pathutil.BuildPath, ensuring the form submission endpoint respects the configured path prefix (e.g.,/pgbackweb/dashboard/webhooks/{id}/edit).
130-130: LGTM: HTMX GET URL now supports path prefix.The hard-coded URL has been correctly wrapped with
pathutil.BuildPath, ensuring the HTMX request endpoint respects the configured path prefix (e.g.,/pgbackweb/dashboard/webhooks/{id}/edit).internal/view/web/dashboard/destinations/list_destinations.go (3)
11-11: LGTM!The import addition is necessary for using
pathutil.BuildPathand follows the project's import conventions.
67-67: Verify: Missingpathutil.BuildPathfor link URL.The URL at line 67 does not use
pathutil.BuildPath, which may break the path prefix feature for this link. Please verify if this path should also be prefixed.If path prefix support is needed here, apply this diff:
component.OptionsDropdownA( - nodx.Href( - fmt.Sprintf("/dashboard/executions?destination=%s", destination.ID), - ), + nodx.Href(pathutil.BuildPath( + fmt.Sprintf("/dashboard/executions?destination=%s", destination.ID), + )), nodx.Target("_blank"),
134-136: Verify: Missingpathutil.BuildPathfor pagination URL.The HTMX pagination URL at lines 134-136 does not use
pathutil.BuildPath, which may break the path prefix feature for pagination. Please verify if this path should also be prefixed.If path prefix support is needed here, apply this diff:
trs = append(trs, nodx.Tr( - htmx.HxGet(fmt.Sprintf( - "/dashboard/destinations/list?page=%d", pagination.NextPage, - )), + htmx.HxGet(pathutil.BuildPath(fmt.Sprintf( + "/dashboard/destinations/list?page=%d", pagination.NextPage, + ))), htmx.HxTrigger("intersect once"),internal/view/web/dashboard/destinations/delete_destination.go (2)
4-4: LGTM!The pathutil import is correctly added to support the path prefix functionality.
32-32: No hard-coded paths remain
No htmx attributes or HTTP route definitions use raw string literals; pathutil.BuildPath is applied consistently.internal/view/web/dashboard/executions/soft_delete_execution.go (2)
4-4: LGTM!The import addition is necessary for the
BuildPathcall on line 32 and aligns with the PR objective to support configurable path prefixes.
32-32: Approve code changes Verified that all Go handlers use pathutil.BuildPath and no unwrapped route paths remain.internal/view/web/dashboard/webhooks/delete_webhook.go (1)
4-4: LGTM! Path prefix integration is correct.The import of
pathutiland the use ofBuildPathto construct the webhook deletion URL are correct. The path format follows the expected pattern (leading slash, no trailing slash).Also applies to: 31-31
internal/view/web/dashboard/webhooks/create_webhook.go (1)
9-9: LGTM! Multiple path prefix integrations are correct.All three uses of
pathutil.BuildPathfor webhook creation flows are correct:
- Redirect after successful creation (line 58)
- Form submission endpoint (line 90)
- Lazy-loaded form content (line 115)
Path formats are consistent and follow the expected pattern.
Also applies to: 58-58, 90-90, 115-115
internal/view/web/dashboard/destinations/index.go (1)
7-7: LGTM! Path prefix integration with query string is correct.The use of
pathutil.BuildPathwith the query string parameter is correct. TheBuildPathfunction simply prepends the path prefix, so query strings are preserved as expected.Also applies to: 59-59
internal/view/web/dashboard/databases/index.go (1)
7-7: LGTM! Path prefix integration is correct.The import and use of
pathutil.BuildPathfor the databases list endpoint with query parameter are correct. Consistent with the pattern used in other index pages.Also applies to: 46-46
internal/view/web/dashboard/backups/edit_backup.go (1)
9-9: LGTM! Dynamic path construction is correct.The import and use of
pathutil.BuildPathwith the dynamically constructed backup edit path are correct. The string concatenation properly builds the path with the backup ID.Also applies to: 94-94
internal/view/web/dashboard/profile/update_user.go (1)
7-7: LGTM! Path prefix integration is correct.The import and use of
pathutil.BuildPathfor the profile update form submission are correct. The path format is proper.Also applies to: 52-52
internal/view/web/dashboard/backups/duplicate_backup.go (1)
4-4: LGTM! Dynamic path construction is correct.The import and use of
pathutil.BuildPathwith the dynamically constructed backup duplication path are correct. The string concatenation properly builds the path with the backup ID.Also applies to: 31-31
internal/view/web/dashboard/backups/delete_backup.go (1)
4-4: LGTM! Dynamic path construction is correct.The import and use of
pathutil.BuildPathwith the dynamically constructed backup deletion path are correct. Consistent with other backup action buttons.Also applies to: 31-31
internal/view/web/dashboard/backups/create_backup.go (1)
10-10: LGTM! Path prefix support correctly integrated.The import of
pathutiland the three uses ofBuildPathare correct:
- Line 74: redirect after backup creation
- Line 109: form POST endpoint
- Line 326: HTMX lazy-load endpoint
All paths are absolute and will be correctly prefixed.
Also applies to: 74-74, 109-109, 326-326
internal/view/web/dashboard/webhooks/index.go (1)
7-7: LGTM! Path prefix support correctly integrated.The import and use of
pathutil.BuildPathfor the webhooks list endpoint is correct. The path includes a query string, which is properly preserved byBuildPath.Also applies to: 49-49
internal/view/web/dashboard/backups/index.go (1)
7-7: LGTM! Path prefix support correctly integrated.The import and use of
pathutil.BuildPathfor the backups list endpoint is correct. The path includes a query string, which is properly preserved byBuildPath.Also applies to: 54-54
internal/view/web/dashboard/backups/manual_run.go (1)
6-6: LGTM! Path prefix support correctly integrated.The import and use of
pathutil.BuildPathfor the manual backup execution endpoint is correct. The path is dynamically constructed with the backup ID and will be properly prefixed.Also applies to: 31-31
Replaces string concatenation with fmt.Sprintf for building URLs This ensures consistent and more secure path handling across all dashboard operations like delete, edit, duplicate, etc.
Introduces consistent use of pathutil.BuildPath function for building URLs Adds proper path prefixing to image paths in support project data Improves URL generation across various dashboard components WIP on feature/path-prefix
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/view/web/dashboard/webhooks/webhook_executions.go (1)
3-23: Add missing pathutil import
Lines 91 and 261 callpathutil.BuildPath, butgithub.com/eduardolat/pgbackweb/internal/util/pathutilis not imported. Add this import to the file’s import block.
🧹 Nitpick comments (2)
internal/view/web/dashboard/webhooks/webhook_executions.go (1)
260-262: Consider consistent query parameter handling.The URL construction works correctly, but note the different approach compared to line 91. Here, the query parameter is included in the path passed to
BuildPath, while line 91 adds the query parameter after theBuildPathcall. Both approaches work, but using a consistent pattern throughout the codebase improves maintainability.For consistency with line 91, consider this approach:
htmx.HxGet( - pathutil.BuildPath("/dashboard/webhooks/"+webhookID.String()+"/executions?page=1"), + strutil.AddQueryParamToUrl( + pathutil.BuildPath("/dashboard/webhooks/"+webhookID.String()+"/executions"), + "page", "1", + ), ),internal/view/web/component/support_project.inc.js (1)
66-97: Consider immutability for data transformation.The method mutates the input
dataobject by reassigning properties. While this works in the current context (fresh fetches and cached data are isolated), a more defensive approach would be to return a new object:processData(data) { const result = { ...data }; if (result.referralLinks) { result.referralLinks = result.referralLinks.map((link) => ({ ...link, logo: this.prefixImagePath(link.logo), })); } if (result.sponsors) { result.sponsors = { ...result.sponsors }; // ... map gold, silver, bronze } return result; }However, given that the data is sourced from either a fresh fetch or localStorage (both isolated), the current approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/view/web/component/support_project.inc.js(3 hunks)internal/view/web/dashboard/backups/list_backups.go(2 hunks)internal/view/web/dashboard/databases/list_databases.go(3 hunks)internal/view/web/dashboard/destinations/list_destinations.go(3 hunks)internal/view/web/dashboard/executions/index.go(2 hunks)internal/view/web/dashboard/executions/list_executions.go(2 hunks)internal/view/web/dashboard/restorations/index.go(2 hunks)internal/view/web/dashboard/restorations/list_restorations.go(2 hunks)internal/view/web/dashboard/webhooks/list_webhooks.go(1 hunks)internal/view/web/dashboard/webhooks/webhook_executions.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/view/web/dashboard/backups/list_backups.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/view/web/dashboard/databases/list_databases.go
🧰 Additional context used
🧬 Code graph analysis (7)
internal/view/web/dashboard/executions/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/executions/list_executions.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/destinations/list_destinations.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/list_webhooks.go (2)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)internal/view/web/dashboard/webhooks/index.go (1)
indexPage(22-61)
internal/view/web/dashboard/restorations/list_restorations.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/restorations/index.go (1)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)
internal/view/web/dashboard/webhooks/webhook_executions.go (3)
internal/util/pathutil/pathutil.go (1)
BuildPath(30-35)internal/view/web/dashboard/webhooks/index.go (1)
indexPage(22-61)internal/view/web/dashboard/webhooks/edit_webhook.go (2)
editWebhookButton(123-146)editWebhookForm(97-121)
🔇 Additional comments (15)
internal/view/web/dashboard/restorations/list_restorations.go (2)
11-11: LGTM!The import of
pathutilis appropriate for supporting the path prefix configuration introduced in this PR.
113-113: LGTM!The use of
pathutil.BuildPathcorrectly applies the configured path prefix to the HTMX pagination URL, ensuring the link works when the app is deployed under a subpath. This change aligns with the PR objective and is consistent with similar updates throughout the codebase.internal/view/web/dashboard/restorations/index.go (1)
7-7: LGTM! Path prefix support correctly integrated.The addition of the
pathutilimport and the use ofpathutil.BuildPathto wrap the base URL path are correct. The query parameter logic remains unchanged and will correctly append parameters after the prefix is applied. This change aligns with the PR objective to support configurable path prefixes.Also applies to: 70-70
internal/view/web/dashboard/webhooks/list_webhooks.go (1)
94-98: No further action required
Both the initial load inindex.goand pagination inlist_webhooks.gonow usepathutil.BuildPath, ensuring consistent path prefix support.internal/view/web/dashboard/webhooks/webhook_executions.go (1)
90-94: LGTM! Correct path prefix integration for pagination.The URL construction correctly applies the path prefix before appending query parameters, which is the proper order for building prefixed URLs with query strings.
internal/view/web/dashboard/destinations/list_destinations.go (4)
11-11: LGTM!The import of
internal/util/pathutilis correctly added to support the centralized path builder used below.
66-68: LGTM!The URL construction for the executions link correctly uses
pathutil.BuildPathto incorporate the configured path prefix.
75-75: LGTM!The POST URL for testing the connection correctly uses
pathutil.BuildPathto incorporate the configured path prefix.
134-136: LGTM!The pagination next-page URL correctly uses
pathutil.BuildPathto incorporate the configured path prefix.internal/view/web/dashboard/executions/index.go (2)
7-7: LGTM!The import of
internal/util/pathutilis correctly added to support the centralized path builder used below.
65-65: LGTM!The base URL for the executions list correctly uses
pathutil.BuildPathto incorporate the configured path prefix, with query parameters appended afterward.internal/view/web/dashboard/executions/list_executions.go (2)
11-11: LGTM!The import of
internal/util/pathutilis correctly added to support the centralized path builder used below.
121-121: LGTM!The pagination next-page URL correctly uses
pathutil.BuildPathto incorporate the configured path prefix, with query parameters appended afterward.internal/view/web/component/support_project.inc.js (2)
99-128: LGTM! Consistent data processing.Both the cached and freshly fetched data paths now consistently apply
processDatato ensure logos are properly prefixed. The implementation correctly maintains the caching logic while adding the transformation layer.
61-61: Verifywindow.PBW_PATH_PREFIXis defined before use.
No assignment or injection for this global was found in any templates or scripts—ensure it’s initialized on the page beforesupport_project.inc.jsruns to avoidundefined/...paths.
| prefixImagePath(path) { | ||
| // If the path starts with / and doesn't start with http, add the path prefix | ||
| if (path && path.startsWith("/") && !path.startsWith("http")) { | ||
| return window.PBW_PATH_PREFIX + path; | ||
| } | ||
| return path; | ||
| }, |
There was a problem hiding this comment.
Handle protocol-relative URLs correctly.
The current check doesn't account for protocol-relative URLs (e.g., //cdn.example.com/logo.png). These URLs start with "//" and should not be prefixed, but the current logic would incorrectly prefix them.
Apply this diff to fix the issue:
prefixImagePath(path) {
- // If the path starts with / and doesn't start with http, add the path prefix
- if (path && path.startsWith("/") && !path.startsWith("http")) {
+ // If the path starts with / but not with // or http, add the path prefix
+ if (path && path.startsWith("/") && !path.startsWith("//") && !path.startsWith("http")) {
return window.PBW_PATH_PREFIX + path;
}
return path;
},📝 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.
| prefixImagePath(path) { | |
| // If the path starts with / and doesn't start with http, add the path prefix | |
| if (path && path.startsWith("/") && !path.startsWith("http")) { | |
| return window.PBW_PATH_PREFIX + path; | |
| } | |
| return path; | |
| }, | |
| prefixImagePath(path) { | |
| // If the path starts with / but not with // or http, add the path prefix | |
| if (path && path.startsWith("/") && !path.startsWith("//") && !path.startsWith("http")) { | |
| return window.PBW_PATH_PREFIX + path; | |
| } | |
| return path; | |
| }, |
🤖 Prompt for AI Agents
In internal/view/web/component/support_project.inc.js around lines 58 to 64, the
prefixImagePath function incorrectly prefixes protocol-relative URLs (those
starting with "//"); update the condition so it only prefixes when path starts
with a single "/" (i.e., path.startsWith("/") && !path.startsWith("//")) and
still exclude full URLs (e.g., keep the !path.startsWith("http") check or
combine checks) so protocol-relative and absolute URLs are not prefixed; ensure
the function returns the original path for "//..." and "http(s)://..." cases and
only prepends window.PBW_PATH_PREFIX for single-leading-slash local paths.
|
Absolutely amazing, LGTM, thanks for your incredible contribution! |
|
I'll fix the code rabbit suggestion and a little inconsistency, thanks for your work |
2 participants
Introduces configurable path prefix to serve application under subpath (e.g., /pgbackweb) Implements validation for path prefixes in environment variables Updates all redirects and routes to use the configured path prefix
This allows deploying the app at a custom URL path while maintaining correct functionality.
Summary by CodeRabbit
New Features
Documentation
Tests