Skip to content

Add AI Review dialog to CLI#750

Merged
seveibar merged 5 commits intomainfrom
codex/add-ai-review-button-in-file-menu
Jun 14, 2025
Merged

Add AI Review dialog to CLI#750
seveibar merged 5 commits intomainfrom
codex/add-ai-review-button-in-file-menu

Conversation

@seveibar
Copy link
Contributor

@seveibar seveibar commented Jun 13, 2025

image

Summary

  • add AiReviewDialog component
  • expose an AI Review menu item in CLI File menu
  • enable requesting and viewing AI reviews for boards

Testing

  • npm run -s build

https://chatgpt.com/codex/tasks/task_b_684ca1d8cc00832eb07e70367f6a9d98

@vercel
Copy link

vercel bot commented Jun 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
runframe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2025 5:16pm

Comment on lines +26 to +29
<div
className="rf-h-64 rf-overflow-y-auto rf-prose rf-max-w-none rf-mt-2"
dangerouslySetInnerHTML={{ __html: html }}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The current implementation renders markdown content directly as HTML using dangerouslySetInnerHTML without sanitization, which creates a potential XSS vulnerability since the content comes from an external API.

Consider adding a sanitization step before rendering:

import DOMPurify from 'dompurify';

// Then in the component:
const sanitizedHtml = useMemo(
  () => DOMPurify.sanitize(marked.parse(review.ai_review_text || "")),
  [review.ai_review_text]
);

// And use sanitizedHtml in the dangerouslySetInnerHTML

Alternatively, use a markdown library that handles sanitization internally, such as react-markdown.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +15 to +21
export const AiReviewListView = ({
packageName,
onSelectReview,
}: {
packageName: string | null
onSelectReview: (review: AiReview) => void
}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The component name AiReviewListView doesn't match its filename ListAiReviewsView.tsx. For better code organization and maintainability, consider either:

  1. Renaming the file to AiReviewListView.tsx to match the exported component, or
  2. Renaming the component to ListAiReviewsView to match the file

Consistent naming between files and their exports makes the codebase more navigable and reduces confusion during development.

Suggested change
export const AiReviewListView = ({
packageName,
onSelectReview,
}: {
packageName: string | null
onSelectReview: (review: AiReview) => void
}) => {
export const ListAiReviewsView = ({
packageName,
onSelectReview,
}: {
packageName: string | null
onSelectReview: (review: AiReview) => void
}) => {

Spotted by Diamond (based on custom rules)

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +8 to +14
export const AiReviewViewView = ({
review,
onBack,
}: {
review: AiReview
onBack: () => void
}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The component name AiReviewViewView contains redundant "View" repetition. Consider renaming to something clearer like AiReviewDetail or simply AiReviewView to better reflect its purpose while maintaining naming consistency with other components in this module.

Suggested change
export const AiReviewViewView = ({
review,
onBack,
}: {
review: AiReview
onBack: () => void
}) => {
export const AiReviewView = ({
review,
onBack,
}: {
review: AiReview
onBack: () => void
}) => {

Spotted by Diamond (based on custom rules)

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +48 to +49
timeout = setTimeout(loadAiReviewText, 1000)
return () => clearTimeout(timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

The cleanup function only clears the timeout set in the recursive calls, but not the initial timeout. This creates a potential memory leak when the component unmounts. Consider initializing the timeout variable outside the function and modifying the cleanup to ensure all timeouts are properly cleared:

let timeout: any;
function loadAiReviewText() {
  // function implementation
  timeout = setTimeout(loadAiReviewText, 1000);
}
timeout = setTimeout(loadAiReviewText, 1000);
return () => clearTimeout(timeout);

This ensures that whether the component unmounts during the initial timeout or during any subsequent recursive calls, the active timeout will be properly cleared.

Suggested change
timeout = setTimeout(loadAiReviewText, 1000)
return () => clearTimeout(timeout)
let timeout: NodeJS.Timeout
function loadAiReviewText() {
// function implementation
timeout = setTimeout(loadAiReviewText, 1000)
}
timeout = setTimeout(loadAiReviewText, 1000)
return () => clearTimeout(timeout)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@seveibar seveibar merged commit 7786b1c into main Jun 14, 2025
6 checks passed
@seveibar seveibar deleted the codex/add-ai-review-button-in-file-menu branch June 14, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant