fix: add /space redirect to /admin/space in nginx configmap

[wangty371]

Summary

Add a redirect rule in the nginx configmap template so that /space and /space/* correctly route to the octo-admin SPA's space management pages.

Closes #88

Root Cause

octo-web navigates to /space when the user clicks 「空间管理」. This path is handled by octo-admin's React Router with basename=/admin and route=/space, meaning the correct URL is /admin/space. Without the redirect, /space falls through to the location / handler and serves the octo-web SPA instead.

Change

One location block added to configmap-nginx.yaml:

location ~* ^/space(/.*)?$ {
    return 301 /admin/space$1;
}

Test plan

• Click 「空间管理」 in octo-web → browser redirects to /admin/space/... and shows the space admin UI
• Direct navigation to /space redirects to /admin/space
• /space/some-id/members redirects to /admin/space/some-id/members

🤖 Generated with Claude Code

[lml2468]

Review — PR #89 (c4e4141)

Summary

Adds nginx redirect from /space(/*)? to /admin/space(/*)? so direct /space URLs reach the octo-admin React Router correctly.

🔴 Blocking

Query strings are silently dropped on redirect (.github/...configmap-nginx.yaml:140)

return 301 /admin/space$1; discards $args. A user hitting /space?tab=settings gets redirected to /admin/space — the ?tab=settings is lost.

Fix — either append $is_args$args:

return 301 /admin/space$1$is_args$args;

Or use rewrite which preserves query strings automatically:

rewrite ^/space(/.*)?$ /admin/space$1 permanent;

💬 Non-blocking

Case-insensitive match is unnecessary (line 139)

~* makes /Space, /SPACE etc. also redirect. URL paths are case-sensitive by convention. Suggest ~ instead:

location ~ ^/space(/.*)?$ {

✅ Highlights

• Regex is well-scoped: matches /space and /space/... but not /spaces or /space-settings ✅
• Placement before location /admin/ is correct for nginx regex evaluation order ✅
• Comment explains the basename/route relationship clearly ✅

[Jerry-Xin]

Review — PR #89 (c4e4141)

Summary

Adds nginx redirect from /space(/*)? to /admin/space(/*)? so that octo-web's 「空间管理」navigation correctly reaches the octo-admin SPA.

🔴 Blocking

Query string silently dropped (configmap-nginx.yaml:140)

return 301 /admin/space$1; discards $args. Any query parameters (e.g. /space?tab=settings) are lost on redirect. Fix:

return 301 /admin/space$1$is_args$args;

Or switch to rewrite which preserves query strings by default:

rewrite ^/space(/.*)?$ /admin/space$1 permanent;

💬 Non-blocking

• ~* (case-insensitive) is unnecessary — URL paths are case-sensitive by convention. ~ would be more precise.
• Consider whether /space will ever need subpath preservation beyond React Router's client-side routing. If not, a simpler location = /space + location ^~ /space/ pair avoids regex overhead.

✅ Highlights

• Regex correctly scopes to /space and /space/... without matching /spaces or /space-settings ✅
• Placement before location /admin/ is correct per nginx evaluation order ✅
• PR body clearly explains the root cause (basename=/admin, route=/space mismatch) ✅

[lml2468]

Re-review — PR #89 (46104ae)

Previous Blocking Issues — ✅ All Resolved

1. Query string loss — fixed: $is_args$args appended ✅
2. Case-insensitive match — fixed: ~* → ~ ✅

Verification

• return 301 /admin/space$1$is_args$args; correctly preserves both path capture and query parameters
• ~ (case-sensitive) is appropriate for URL path matching
• Regex ^/space(/.*)?$ scope unchanged — still correctly scoped to /space and /space/... only

LGTM. Clean fix addressing both review items.

[Jerry-Xin]

Re-review — PR #89 (46104ae)

Both blockers from the previous round are resolved:

1. ✅ Query string preservation — $is_args$args appended
2. ✅ Case sensitivity — ~* → ~

Regex correctly scopes to /space and /space/.... Placement before location /admin/ is correct. Clean fix.

[yujiawei]

Code Review — PR #89 (octo-deployment)

Summary

This PR adds a single nginx redirect rule in helm/octo/templates/configmap-nginx.yaml so that requests to /space and /space/* are permanently redirected to /admin/space and /admin/space/*. The motivation is that octo-web links to /space for the "空间管理" entry point, while octo-admin's React Router serves the corresponding view at /admin/space (basename /admin, route /space). Without the redirect, /space falls through to the catch-all location / block and returns the octo-web SPA, which has no matching route.

The change is minimal, well-scoped, and directly addresses the reported routing bug (#88).

Verification

• ✅ Regex correctness: ^/space(/.*)?$ matches /space, /space/, /space/<id>, /space/<id>/members, and stops before /spaceship or /space-foo (must be end-of-path or followed by /). helm/octo/templates/configmap-nginx.yaml:139.
• ✅ Query string preservation: the rewrite target /admin/space$1$is_args$args keeps query strings intact (/space/x?y=1 → /admin/space/x?y=1). helm/octo/templates/configmap-nginx.yaml:140. Note: this is slightly more complete than what the PR description shows (which omits $is_args$args); the code is correct, the description is just simplified.
• ✅ Location-block ordering / no shadowing: nginx prefers the longest non-regex prefix match before evaluating regex locations. /space does not match any of the existing prefix locations (/api/, /v1/, /ws, /minio/, /admin/, /matter/, /summary/, /file…, /)… well, it would match the catch-all location /, but a regex location takes precedence over a prefix location / match, so the redirect wins. For /admin/space/..., the longest prefix /admin/ wins over the regex (prefix /admin/ is longer than the regex's effective prefix), so no redirect loop is possible. helm/octo/templates/configmap-nginx.yaml:142-149.
• ✅ Relative redirect target: the 301 response uses a path-only target, so the browser preserves the original scheme/host — no risk of HTTPS→HTTP downgrade behind a TLS-terminating ingress.
• ✅ Style consistency with the existing location = /admin { return 301 /admin/; } block above.
• ✅ Comments are clear and explain why (octo-admin basename) rather than what, which is the right choice here.
• ✅ No Helm template variables touched; this is plain nginx config inside an existing template.

Findings

P0 / P1

None.

P2 / Nits / Suggestions

1. 301 vs 302 (configmap-nginx.yaml:140). 301 is correct for a permanent routing decision, but browsers cache 301s aggressively (often until cache clear). If the team ever decides to host a top-level /space page (e.g. a marketing or landing page) on octo-web in the future, users with cached redirects would skip it. Given the current product structure this is unlikely to bite, so 301 is fine — just flagging for future awareness. If you want the freedom to repurpose /space later, switching to 302 would cost essentially nothing.

2. POST/PUT semantics (configmap-nginx.yaml:139-141). 301 (and 302) historically downgrade non-GET methods to GET in some clients, while 307/308 preserve the method. The route appears to be a UI navigation entry only (GET), so this is not a real concern, but worth noting if anyone later puts an API or form POST under /space.

3. PR description out of sync (cosmetic). The PR body shows the redirect as return 301 /admin/space$1; but the actual code uses return 301 /admin/space$1$is_args$args;. The code is the better version; consider updating the PR description for the audit trail. Non-blocking.

4. No automated test for nginx config (process). Routing changes in this configmap currently rely on manual verification per the PR test plan. For a 5-line redirect this is reasonable, but as the configmap grows it would be worth considering nginx -t-style validation in CI (rendering the chart with helm template and piping into a containerized nginx -t). Out of scope for this PR.

Verdict

The change is correct, minimal, and matches the documented intent. Manual test plan in the PR body is appropriate for the size of the change. No blocking concerns.