Skip to content
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

added reasoning to labeling queues #200

Merged
merged 1 commit into from
Nov 13, 2024
Merged

added reasoning to labeling queues #200

merged 1 commit into from
Nov 13, 2024

Conversation

skull8888888
Copy link
Collaborator

@skull8888888 skull8888888 commented Nov 13, 2024

Important

Adds optional reasoning to labels in queues, updates CSS background color, and modifies queue component for reasoning input.

  • Behavior:
    • Adds optional reasoning field to labels in removeQueueItemSchema in route.ts.
    • Updates addedLabels state in queue.tsx to include reasoning.
    • Allows users to input reasoning for labels using DefaultTextarea in queue.tsx.
  • CSS:
    • Changes --background color in globals.css from 0 0% 8% to 0 0% 4%.

This description was created by Ellipsis for 415ac37. It will automatically update as commits are pushed.

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.

❌ Changes requested. Reviewed everything up to 415ac37 in 1 minute and 4 seconds

More details
  • Looked at 129 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/app/api/projects/[projectId]/queues/[queueId]/remove/route.ts:21
  • Draft comment:
    Ensure that the reasoning field is properly validated and sanitized to prevent security issues such as XSS or SQL injection.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/app/globals.css:38
  • Draft comment:
    Ensure that the background color change aligns with the overall design requirements.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The CSS change modifies the background color, which might affect the overall theme or design. Ensure that this change aligns with the design requirements.

Workflow ID: wflow_x5H1CS9y779mgUIh


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

placeholder="Reasoning (optional)"
value={label.reasoning || ''}
onChange={(e) => {
setAddedLabels(prev => prev.map(l => l.labelClass.id === label.labelClass.id ? { ...l, reasoning: e.target.value } : l));
Copy link
Contributor

Choose a reason for hiding this comment

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

Using labelClass.id for updates can lead to incorrect behavior if there are multiple labels with the same labelClass.id. Consider using the index for updates instead.

Suggested change
setAddedLabels(prev => prev.map(l => l.labelClass.id === label.labelClass.id ? { ...l, reasoning: e.target.value } : l));
setAddedLabels(prev => prev.map((l, i) => i === index ? { ...l, reasoning: e.target.value } : l));

@skull8888888 skull8888888 merged commit e56e79f into dev Nov 13, 2024
1 check passed
@skull8888888 skull8888888 deleted the reasoning branch November 13, 2024 07:01
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.

1 participant