Skip to content

Conversation

@felixfeng33
Copy link
Collaborator

Checklist

  • yarn typecheck
  • yarn lint:fix
  • yarn test
  • yarn brl
  • yarn changeset
  • ui changelog

@codesandbox
Copy link

codesandbox bot commented Nov 1, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@changeset-bot
Copy link

changeset-bot bot commented Nov 1, 2025

⚠️ No Changeset found

Latest commit: 88fce74

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Nov 1, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
plate Ready Ready Preview Comment Nov 1, 2025 9:01am

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Nov 1, 2025
@claude
Copy link
Contributor

claude bot commented Nov 1, 2025

Claude finished @felixfeng33's task —— View job


PR Review: test

I've completed my review of this pull request. Here's my task list:

  • Read and analyze the changed files
  • Review code quality and adherence to project standards
  • Check if changeset is required
  • Verify if changelog update is needed
  • Provide comprehensive review feedback

Review Summary

This PR makes minimal changes to the development page, replacing a component with an embedded iframe. While the changes are simple, there are several concerns that should be addressed.

Issues Found

🔴 Critical Issues

  1. PR Title and Description

    • The PR title "test" is not descriptive
    • No description provided explaining the purpose of these changes
    • This violates project contribution guidelines
  2. Security Concern - External Content Embedding

    • The PR adds an iframe embedding content from https://evergarden.ai/iframe/nh7b9bwz6r72ntgr5e95ye5brs7tjacp
    • No context provided about what this external service is
    • Embedding external content without proper security considerations can expose users to:
      • Cross-site scripting (XSS) attacks
      • Data leakage
      • Third-party tracking
    • Missing security attributes on the iframe (e.g., sandbox, allow, CSP headers)

🟡 Code Quality Issues

  1. Inline Styles

    <iframe height="500px" width="40%" src="..." ></iframe>
    • Using inline styles instead of CSS classes or styled components
    • Inconsistent with the project's Tailwind CSS approach
  2. No Alt Text or Loading State

    • The iframe lacks accessibility attributes
    • No loading or error states for when the iframe fails to load
  3. Development Code in Main Branch

    • This appears to be test/experimental code being merged to main
    • The /dev route suggests this is for development purposes only

✅ Changeset and Changelog Status

  • Changeset: Not required (no package changes detected)
  • Changelog: Not required (no registry changes detected)
  • The changeset bot correctly identified that no changeset is needed, which contradicts the checked box in the PR checklist

Recommendations

  1. Immediate Actions Required:

    • Provide a clear PR title and description
    • Explain the purpose of embedding evergarden.ai content
    • Add security attributes to the iframe or use a safer embedding method
  2. Security Improvements:

    <iframe 
      className="h-[500px] w-2/5"
      src="https://evergarden.ai/iframe/nh7b9bwz6r72ntgr5e95ye5brs7tjacp"
      sandbox="allow-scripts allow-same-origin"
      loading="lazy"
      title="Evergarden AI Embed"
    />
  3. Consider Alternatives:

    • If this is for testing, use environment variables to conditionally render
    • If this is a permanent feature, properly integrate it with error handling and loading states
    • Document why this external service is being used

File Changes Review

  1. apps/www/next-env.d.ts

    • Auto-generated change by Next.js (harmless)
    • Path updated from ./.next/types/routes.d.ts to ./.next/dev/types/routes.d.ts
  2. apps/www/src/app/dev/page.tsx

    • Commented out <MarkdownStreamDemo />
    • Added iframe with external content
    • Concerns noted above

Conclusion

This PR appears to be a quick test or experiment that's not ready for production. The lack of context, security considerations, and proper implementation suggests this should either be:

  • Moved to a feature branch for experimentation
  • Properly implemented with security measures and documentation
  • Clarified if this is intended for production use

Please address the critical security concerns and provide proper context before this can be approved.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 1 to +4
/// <reference types="next" />
/// <reference types="next/image-types/global" />
/// <reference types="next/navigation-types/compat/navigation" />
import "./.next/types/routes.d.ts";
import "./.next/dev/types/routes.d.ts";

Choose a reason for hiding this comment

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

P1 Badge Restore typed-route import to generated path

The change in next-env.d.ts now imports ./.next/dev/types/routes.d.ts, but Next generates typed route definitions under ./.next/types/routes.d.ts for both next dev and next build. The new path does not exist in a normal build so tsc and yarn typecheck will fail with “Cannot find module './.next/dev/types/routes.d.ts'”. This file is auto‑generated and should keep the default path to avoid build breakage.

Useful? React with 👍 / 👎.

Comment on lines 4 to +7
return (
<main>
{/* <EditorViewClient /> */}
<MarkdownStreamDemo />
{/* <MarkdownStreamDemo /> */}

Choose a reason for hiding this comment

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

P1 Badge Remove unused MarkdownStreamDemo import

page.tsx still imports MarkdownStreamDemo even though its usage is commented out. With the repo’s eslint configuration, this unused import will trigger a no-unused-vars error and cause yarn lint to fail. Either drop the import or render the component.

Useful? React with 👍 / 👎.

@dosubot dosubot bot added the javascript Pull requests that update javascript code label Nov 1, 2025
@zbeyens zbeyens marked this pull request as draft November 1, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update javascript code size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants