fix: prepends /v1 to all user/template urls and added proxies#2659
Merged
fix: prepends /v1 to all user/template urls and added proxies#2659
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds an internal proxy handler to the API legacy router and migrates legacy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (frontend)
participant Legacy as Legacy API\n(legacyRouter)
participant V1 as V1 API\n(internal /v1 handlers)
participant DB as Database
Client->>Legacy: HTTP request to /user/... or /pricing
alt route configured as 301
Legacy-->>Client: 301 redirect to /v1/...
else proxied route
Legacy->>V1: HTTP forward to http://127.0.0.1:<PORT>/v1/... (preserve path/query/body)
V1->>DB: query/update
DB-->>V1: result
V1-->>Legacy: response
Legacy-->>Client: proxied response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/deploy-web/src/components/sdl/EditDescriptionForm.tsx (1)
39-51:⚠️ Potential issue | 🟡 MinorMissing error handling could leave the form in a broken state.
If the API call fails,
isSavingremainstrueindefinitely, preventing further user interaction. Consider adding error handling similar to other forms in this PR.🛡️ Proposed fix
const onSubmit = async (data: FormValues) => { setIsSaving(true); - await consoleApiHttpClient.post("/v1/user/saveTemplateDesc", { - id: id, - description: data.description - }); - - enqueueSnackbar(<Snackbar title="Description saved!" iconVariant="success" />, { - variant: "success" - }); - - onSave(data.description); + try { + await consoleApiHttpClient.post("/v1/user/saveTemplateDesc", { + id: id, + description: data.description + }); + + enqueueSnackbar(<Snackbar title="Description saved!" iconVariant="success" />, { + variant: "success" + }); + + onSave(data.description); + } catch { + enqueueSnackbar(<Snackbar title="Error saving description" iconVariant="error" />, { + variant: "error" + }); + } finally { + setIsSaving(false); + } };apps/deploy-web/src/components/user/UserSettingsForm.tsx (1)
72-86:⚠️ Potential issue | 🟡 MinorMissing error handling for username availability check.
If the API call fails,
isCheckingAvailabilityremainstrue, showing a perpetual spinner. Consider adding error handling.🛡️ Proposed fix
const timeoutId = setTimeout(async () => { setIsCheckingAvailability(true); - const response = await consoleApiHttpClient.get(`/v1/user/checkUsernameAvailability/${username}`); - - setIsCheckingAvailability(false); - setIsAvailable(response.data.isAvailable); + try { + const response = await consoleApiHttpClient.get(`/v1/user/checkUsernameAvailability/${username}`); + setIsAvailable(response.data.isAvailable); + } catch { + setIsAvailable(null); + } finally { + setIsCheckingAvailability(false); + } }, 500);apps/api/src/user/routes/user-templates/user-templates.router.ts (1)
84-116:⚠️ Potential issue | 🟠 MajorOpenAPI response codes don’t match 204 handlers.
These routes return
204but declare200in the OpenAPI responses, which can break client generation and contract validation. Align the response codes (or change handlers to return 200 with a body).✅ Suggested fix (align OpenAPI responses to 204)
responses: { - 200: { + 204: { description: "Template description saved successfully" }, description: "Unauthorized" } } responses: { - 200: { + 204: { description: "Template deleted successfully" }, description: "Invalid template ID" }, description: "Unauthorized" } } responses: { - 200: { + 204: { description: "Template added to favorites" }, description: "Invalid template ID" }, description: "Unauthorized" } } responses: { - 200: { + 204: { description: "Template removed from favorites" }, description: "Invalid template ID" }, description: "Unauthorized" } }Also applies to: 145-173, 196-224, 226-254
🤖 Fix all issues with AI agents
In `@apps/api/src/user/controllers/user/user.controller.ts`:
- Around line 57-69: The assertion in updateSettings checks
this.authService.currentUser?.userId but the code uses
this.authService.currentUser.id; update the assert to check the correct property
(this.authService.currentUser?.id) or make both use the same property name so
they match, ensuring the assert refers to the actual identifier used by
authService.currentUser before calling this.userService.updateUserDetails.
- Around line 77-81: In subscribeToNewsletter the assertion checks
authService.currentUser?.userId but the code then reads
authService.currentUser.id, causing an inconsistency; update the assertion and
variable usage to the same property (prefer authService.currentUser?.id) or
rename the extracted variable to match the asserted property so the assert and
the userId assignment both reference the same field (refer to
subscribeToNewsletter and authService.currentUser).
In `@apps/api/src/user/routes/user-settings/user-settings.router.ts`:
- Around line 56-63: The backend marks bio, youtubeUsername, twitterUsername and
githubUsername as nullable but the frontend currently makes them optional;
update the frontend UserSettings type and the form Zod schema so those four
fields are required and accept null (e.g. in the UserSettings interface change
the properties to string | null, and in the UserSettingsForm.tsx schema change
bio, youtubeUsername, twitterUsername, githubUsername to
z.string().max(...).nullable()); also ensure the form default values populate
these fields (explicitly null when empty) so getValues()/submission matches the
backend nullable contract.
🧹 Nitpick comments (4)
apps/api/src/routers/legacyRouter.ts (3)
201-227: Inconsistent redirect status codes.The new user routes use hardcoded
301(Permanent Redirect) while the rest of the file uses theredirectStatusCodeconstant set to302(Temporary Redirect) on line 6.If
301is intentional for these user routes (since/v1/paths are now canonical), consider either:
- Updating
redirectStatusCodeto301if all legacy routes should use permanent redirects, or- Creating a separate constant like
permanentRedirectStatusCode = 301to make the intent explicit.♻️ Proposed refactor using a named constant
const redirectStatusCode = 302; // Temporary Redirect +const permanentRedirectStatusCode = 301; // Permanent RedirectThen replace all hardcoded
301values withpermanentRedirectStatusCode.
237-238: Comment mentions hardcoded port but code uses environment variable.The comment says "http://127.0.0.1:3000/v1/*" but the actual implementation uses
process.env.PORT || 3000. Update the comment to reflect the dynamic port behavior.📝 Proposed comment fix
-/** - * Local http proxy to http://127.0.0.1:3000/v1/* - */ +/** + * Local http proxy to http://127.0.0.1:${PORT}/v1/* + */
240-244: Consider adding error handling and timeout to the proxy function.The
fetchcall has no error handling or timeout. If the internal v1 endpoint is slow or unavailable, requests could hang indefinitely or fail with unhandled errors.♻️ Proposed improvement with timeout and error handling
function proxyToV1(c: Context) { const originalUrl = new URL(c.req.url); const url = `http://127.0.0.1:${process.env.PORT || 3000}/v1${originalUrl.pathname}${originalUrl.search}`; - return fetch(new Request(url, c.req.raw)); + return fetch(new Request(url, c.req.raw), { signal: AbortSignal.timeout(30000) }) + .catch(() => c.json({ error: "Internal proxy error" }, 502)); }apps/deploy-web/src/queries/useTemplateQuery.spec.tsx (1)
90-90: Consider usinguseUserFavoriteTemplates.namefor consistency.As per coding guidelines, use
<Subject>.namein the root describe suite description to enable automated refactoring tools. The same applies touseAddFavoriteTemplate(line 272) anduseRemoveFavoriteTemplate(line 315).♻️ Suggested fix
- describe("useUserFavoriteTemplates", () => { + describe(useUserFavoriteTemplates.name, () => {Similar changes for lines 272 and 315.
ygrishajev
previously approved these changes
Feb 3, 2026
stalniy
force-pushed
the
fix/user-template-urls
branch
2 times, most recently
from
a96dbb4 to
c8d1dd0
Compare
Contributor
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 (2)
apps/api/src/user/routes/user-settings/user-settings.router.ts (2)
81-85:⚠️ Potential issue | 🟡 MinorResponse status code mismatch with OpenAPI spec.
The route handler returns
c.body(null, 204)(No Content), but the OpenAPIresponsesdefinition declares200as the success response. This inconsistency may cause issues with API documentation and client code generation.Update either the response definition to
204or the handler to return200:🐛 Proposed fix to align OpenAPI spec with implementation
responses: { - 200: { - description: "Settings updated successfully" + 204: { + description: "Settings updated successfully" },
134-137:⚠️ Potential issue | 🟡 MinorSame response status code mismatch in subscribeToNewsletter.
The handler returns
204but the OpenAPI spec declares200. Apply the same fix here:🐛 Proposed fix
responses: { - 200: { - description: "Subscribed successfully" + 204: { + description: "Subscribed successfully" },
🧹 Nitpick comments (1)
apps/deploy-web/src/queries/useTemplateQuery.spec.tsx (1)
90-90: Consider using function.namefor describe block consistency.Some describe blocks use hardcoded strings while others use
.namereferences. For consistency and to support automated refactoring tools, consider updating these:
- Line 90:
"useUserFavoriteTemplates"→useUserFavoriteTemplates.name- Line 272:
"useAddFavoriteTemplate"→useAddFavoriteTemplate.name- Line 315:
"useRemoveFavoriteTemplate"→useRemoveFavoriteTemplate.name- Line 358:
"useTemplates"→useTemplates.nameAs per coding guidelines: Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references.Also applies to: 272-272, 315-315, 358-358
stalniy
force-pushed
the
fix/user-template-urls
branch
from
c8d1dd0 to
a939298
Compare
stalniy
force-pushed
the
fix/user-template-urls
branch
from
a939298 to
2e5fd76
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
unification and small payload fix
Summary by CodeRabbit
New Features
Refactor
Tests