Skip to content

Feature | Redirection #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: staging
Choose a base branch
from

Conversation

mukezhz
Copy link
Collaborator

@mukezhz mukezhz commented Apr 9, 2025

  • added support for redirection

image

@mukezhz mukezhz requested review from avashForReal and Copilot April 9, 2025 11:17
@mukezhz mukezhz self-assigned this Apr 9, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 66 out of 71 changed files in this pull request and generated 1 comment.

Files not reviewed (5)
  • package.json: Language not supported
  • pnpm-lock.yaml: Language not supported
  • prisma/migrations/20250407063823_add_role_permissions/migration.sql: Language not supported
  • prisma/migrations/20250409071302_add_redirect_url/migration.sql: Language not supported
  • prisma/schema.prisma: Language not supported
Comments suppressed due to low confidence (1)

src/app/api/domain/add/route.ts:77

  • [nitpick] Consider using consistent naming for redirection fields. The schema uses 'redirectTo' for the request payload, while the database and API response refer to it as 'redirectUrl'. Aligning these names could improve clarity.
redirectUrl: reqPayload.enableRedirection && reqPayload.redirectTo ? reqPayload.redirectTo.trim() : null,

@@ -72,6 +145,7 @@ export async function POST(request: NextRequest) {
{ status: 400 }
);
}
console.error("error...", err)
Copy link
Preview

Copilot AI Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The catch block prints 'err' even though the error variable is named 'error'. Rename 'err' to 'error' to correctly log the caught error.

Suggested change
console.error("error...", err)
console.error("error...", error)

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@mukezhz mukezhz changed the base branch from main to staging April 9, 2025 11:19
@mukezhz mukezhz requested a review from Copilot April 9, 2025 11:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.

Files not reviewed (2)
  • prisma/migrations/20250409071302_add_redirect_url/migration.sql: Language not supported
  • prisma/schema.prisma: Language not supported

@mukezhz mukezhz requested a review from Copilot April 10, 2025 03:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • prisma/migrations/20250409071302_add_redirect_url/migration.sql: Language not supported
  • prisma/schema.prisma: Language not supported
Comments suppressed due to low confidence (1)

src/components/proxies/add-proxy-dialog.tsx:46

  • [nitpick] Using '0' as the port value for redirection might lead to unintended behavior if downstream logic does not expect a port of '0'. Please verify that this value is handled correctly or consider using an alternative indicator.
if (values.enableRedirection) { processedValues.destinationAddress = ''; processedValues.port = '0'; }

Comment on lines +57 to +58

return issues.length === 0;
Copy link
Preview

Copilot AI Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The superRefine callback in Zod is intended for adding custom issues and should not return a value. Remove the return statement to adhere to the expected callback signature.

Suggested change
return issues.length === 0;

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

@mukezhz mukezhz requested a review from Copilot April 15, 2025 17:08
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • prisma/migrations/20250409071302_add_redirect_url/migration.sql: Language not supported
  • prisma/schema.prisma: Language not supported
Comments suppressed due to low confidence (1)

src/lib/prisma.ts:11

  • [nitpick] Consider removing the commented-out query logging code if it is no longer needed to improve code clarity.
        // ? ["query", "error", "warn"]

Comment on lines +26 to +29
if (data.enableRedirection && data.redirectTo) {
if (!domainRegex.test(data.redirectTo.trim())) {
ctx.addIssue({
path: ["redirectTo"],
Copy link
Preview

Copilot AI Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When redirection is enabled, the schema currently only refines the redirectTo field if a value is provided; consider enforcing a non-empty redirectTo when enableRedirection is true to avoid misconfiguration.

Suggested change
if (data.enableRedirection && data.redirectTo) {
if (!domainRegex.test(data.redirectTo.trim())) {
ctx.addIssue({
path: ["redirectTo"],
if (data.enableRedirection) {
if (!data.redirectTo || data.redirectTo.trim() === "") {
ctx.addIssue({
path: ["redirectTo"],
message: "redirectTo is required when enableRedirection is true",
code: z.ZodIssueCode.custom,
});
issues.push("redirectTo");
} else if (!domainRegex.test(data.redirectTo.trim())) {
ctx.addIssue({
path: ["redirectTo"],

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants