Skip to content

feat: remove browser index chat feature #615

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 6 commits into
base: dev
Choose a base branch
from
Open

feat: remove browser index chat feature #615

wants to merge 6 commits into from

Conversation

olzhik11
Copy link
Collaborator

@olzhik11 olzhik11 commented Jun 9, 2025

  • remove chat feature
  • add redirects with external domains

Important

Remove chat feature and add support for external domain redirects.

  • Feature Removal:
    • Remove chat feature by deleting route.ts, page.tsx, layout.tsx, and other related files in frontend/app/chat.
    • Remove chat-related components like chat, messages, session-context, and others in frontend/components/chat.
  • Redirects:
    • Add NEXTAUTH_ALLOWED_REDIRECT_DOMAINS to .env.local.example for external domain redirects.
    • Update auth.ts to handle allowed redirect domains using isRedirectDomainAllowed().
  • Misc:
    • Update README.md to remove references to the chat feature.
    • Modify checkout/page.tsx and sign-in/page.tsx to adjust URLs for the removed chat feature.

This description was created by Ellipsis for af6a928. You can customize this summary. It will automatically update as commits are pushed.

@olzhik11 olzhik11 self-assigned this Jun 9, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.

35 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 0cf5f3f in 1 minute and 36 seconds. Click for details.
  • Reviewed 3071 lines of code in 35 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/auth/sign-in.tsx:32
  • Draft comment:
    Omiting the 'redirect: false' flag in the signIn call means the function may auto-redirect, preventing error handling code from executing. Consider adding 'redirect: false' if you intend to manage sign-in errors in the component.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code has proper error handling both for the signIn call result and for caught exceptions. The PR author deliberately removed redirect:false, suggesting they want the default redirect behavior. The error handling would still work for errors that occur before any redirect. Without seeing the next-auth documentation or implementation, I can't be 100% certain about the redirect timing, but the comment seems speculative. I don't have access to the next-auth documentation to verify the exact behavior of signIn with and without redirect:false. The comment could be correct about the timing of redirects vs error handling. While the exact behavior is uncertain, the code change appears intentional and the error handling looks comprehensive. The comment is speculative without strong evidence that it would cause issues. Delete the comment as it's speculative and suggests reverting an intentional change without strong evidence of problems.
2. frontend/components/auth/sign-up.tsx:32
  • Draft comment:
    Similar to sign-in, omitting 'redirect: false' in the signIn call within the sign-up component may cause automatic redirection, which can bypass error handling. Evaluate whether this behavior is intended.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code has proper error handling both for the signIn result and for caught exceptions. The comment assumes removing redirect:false will bypass this, but looking at the code, the error handling would still work - the try-catch would catch errors, and the result.ok check would still run before any redirect happens. Without seeing the next-auth implementation details, I can't be 100% certain about the redirection timing. I don't have visibility into the next-auth library's internal behavior around redirection timing. The comment author may have specific knowledge about how next-auth handles redirects. Even with uncertainty about next-auth internals, the code shows clear error handling patterns that would catch issues before any redirect. The comment is speculative without strong evidence. Delete the comment. While it raises a theoretical concern, there's no strong evidence that removing redirect:false actually breaks error handling, and the code shows proper error handling patterns.
3. frontend/lib/auth.ts:63
  • Draft comment:
    The open-redirect prevention function 'isRedirectDomainAllowed' uses allowed domains from NEXTAUTH_ALLOWED_REDIRECT_DOMAINS. Ensure these domains are maintained in sync with deployment needs to avoid open redirect vulnerabilities.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is purely informative, reminding the author to keep domains in sync with deployment needs. It doesn't provide a specific code suggestion or ask for a specific test to be written.
4. frontend/lib/auth.ts:162
  • Draft comment:
    Consider explicitly specifying a JWT signing algorithm in the jwt.sign call for improved security and clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/components/auth/sign-in.tsx:75
  • Draft comment:
    The error message displayed on sign-in always shows a default message. Consider showing the actual error returned (if available) to aid debugging.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_iH9jjTUV2DhAC2mG

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@@ -93,12 +93,12 @@ export default async function CheckoutPage(props: {
const successUrl =
typeParam === "workspace"
? `${process.env.NEXT_PUBLIC_URL}/workspace/${workspaceId}?sessionId={CHECKOUT_SESSION_ID}&workspaceName=${urlEncodedWorkspaceName}&lookupKey=${lookupKey}`
: `${process.env.NEXT_PUBLIC_URL}/chat?sessionId={CHECKOUT_SESSION_ID}&userId=${userId}&lookupKey=${lookupKey}`;
: `https://index.lmnr.ai/chat?sessionId={CHECKOUT_SESSION_ID}&userId=${userId}&lookupKey=${lookupKey}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider externalizing the hardcoded domain https://index.lmnr.ai into an environment variable for both successUrl and cancelUrl. This would improve flexibility when deploying to different environments.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 9a31885 in 56 seconds. Click for details.
  • Reviewed 32 lines of code in 1 files
  • Skipped 1 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:64
  • Draft comment:
    Removed the 'Index - SOTA browser agent' section including chat UI details. Ensure all references (e.g., image files and URLs) to the removed chat feature are cleaned up elsewhere if needed.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_cWCOic88slI1nPxi

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@@ -93,12 +93,12 @@ export default async function CheckoutPage(props: {
const successUrl =
typeParam === "workspace"
? `${process.env.NEXT_PUBLIC_URL}/workspace/${workspaceId}?sessionId={CHECKOUT_SESSION_ID}&workspaceName=${urlEncodedWorkspaceName}&lookupKey=${lookupKey}`
: `${process.env.NEXT_PUBLIC_URL}/chat?sessionId={CHECKOUT_SESSION_ID}&userId=${userId}&lookupKey=${lookupKey}`;
: `https://index.lmnr.ai/chat?sessionId={CHECKOUT_SESSION_ID}&userId=${userId}&lookupKey=${lookupKey}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we have it here?

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed ff3b3d0 in 2 minutes and 39 seconds. Click for details.
  • Reviewed 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/app/sign-in/page.tsx:24
  • Draft comment:
    Removed fallback for NEXTAUTH_URL. If the env var is undefined (common in local or misconfigured environments), the URL comparison may fail. Consider validating or providing a safe default.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code appears to have proper error handling. If NEXTAUTH_URL is undefined, the origin comparison will fail, causing url.origin !== currentOrigin, which is fine - it will just skip the special case redirect. If URL parsing fails entirely, it falls back to "/onboarding". The original fallback to localhost was probably not a good practice anyway, as it could mask configuration issues. Maybe there's a specific reason they had the localhost fallback that I'm not aware of. The comment suggests this is common in local environments. Even for local environments, it's better to explicitly configure NEXTAUTH_URL than to have magic fallbacks. The code handles the undefined case gracefully. The comment should be deleted. The code handles the undefined case safely, and removing the fallback is actually better practice.
2. frontend/app/sign-up/page.tsx:24
  • Draft comment:
    Removed fallback for NEXTAUTH_URL. Without a default, undefined values may break the redirect logic. Verify that NEXTAUTH_URL is always set, or add explicit handling.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already has error handling - if NEXTAUTH_URL is undefined, the URL.origin comparison will fail and the catch block will handle it by setting callbackUrl to "/onboarding". The original localhost fallback wasn't great practice anyway since it could mask configuration issues in production. The current code actually handles the undefined case more cleanly. Maybe there's a security risk if NEXTAUTH_URL is undefined in production? Or maybe the error handling is too broad? The error handling is appropriate - if origin comparison fails for any reason (including undefined NEXTAUTH_URL), defaulting to "/onboarding" is safe. Having NEXTAUTH_URL properly configured is a deployment concern, not a code issue. The comment should be deleted. The code handles undefined NEXTAUTH_URL gracefully through its error handling, and removing the localhost fallback is actually better practice.

Workflow ID: wflow_hG9aRXe6nz89qCPj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed af6a928 in 2 minutes and 45 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/landing/landing-header.tsx:43
  • Draft comment:
    The link change correctly redirects to an external URL. However, using Next.js's Link component for external URLs may not be ideal. Consider using a standard tag for external links to ensure proper semantics and avoid potential client-side routing issues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment raises a theoretical concern but doesn't point to any actual problems. Next.js Link components work fine with external URLs - this is a documented use case. The code is consistent with the rest of the file's link handling. Changing just this one link would actually reduce consistency. The current implementation includes proper security attributes (rel="noopener noreferrer"). The comment might have a point about semantic HTML purity. There could be edge cases where Link behavior with external URLs causes issues that I'm not aware of. The theoretical benefits don't outweigh the value of consistency, and Next.js officially supports external URLs in Link components. No actual problems are demonstrated. Delete this comment. It suggests a change that would reduce code consistency without providing clear benefits or evidence of actual problems.

Workflow ID: wflow_jiZud4KYIwmD6ZEN

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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