refactor: rename Comment 'level' field to 'variant' #361
Conversation
Implements GitHub issue #336 Changes: - Renamed 'level' field to 'variant' in Comment interface - Updated database schema: renamed EvaluationComment.level to EvaluationComment.variant - Created Prisma migration: 20251121132349_rename_level_to_variant - Updated all plugins (math, spelling, fact-check, fallacy-check, forecast) - Updated all UI components to use CommentVariantIndicator component - Exported CommentVariant type from @roast/ai package - Updated all tests to use 'variant' instead of 'level' - Updated JobOrchestrator to write 'variant' to database - Removed all inline imports, using proper import statements - Fixed fallacy-review tool to use shared CommentVariant type The field 'variant' better describes its purpose: visual presentation style rather than importance/severity level.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request systematically renames the comment field from Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Analysis Plugin
participant Builder as CommentBuilder
participant API as API/Export Layer
participant UI as UI Components
Plugin->>Builder: build({variant: 'warning', ...})
Builder->>Builder: validate variant is CommentVariant
Builder->>API: return Comment {variant, header, ...}
API->>API: export/transform to variant field
API->>UI: pass comment data with variant
UI->>UI: CommentVariantIndicator({variant})
UI->>UI: render colored badge based on variant
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
Removed migration directory with wrong year (2024 instead of 2025). Only keeping: 20251121132349_rename_level_to_variant
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts (1)
185-222: Update comments to reflectvariantterminology.The method was renamed from
getLevel()togetVariant(), but the internal comments still reference "level" (e.g., Line 185: "base level on confidence gap"). Update these comments to use "variant" for consistency.Apply this diff to update the comments:
private getVariant(): CommentVariant { - // If we have both author and our forecast, base level on confidence gap + // If we have both author and our forecast, base variant on confidence gap if (this.ourForecast && this.extractedForecast.authorProbability !== undefined) {apps/web/src/components/CommentVariantIndicator.tsx (1)
82-92: Simplify the icon cloning logic to improve type safety.The current icon cloning logic is complex and uses unsafe type assertions (
any). The check on Line 88 (iconContent.type === 'span') is problematic because React elements don't havetypeas a string property in this way.Consider simplifying by determining icon vs. text content earlier in the switch statement:
- // Clone icon element with size class if it's an icon component - let content = iconContent; - if (typeof iconContent === 'object' && iconContent && 'type' in iconContent) { - const IconComponent = iconContent.type as any; - if (IconComponent === XMarkIcon || IconComponent === CheckIcon || IconComponent === CheckCircleIcon) { - content = <IconComponent className={`${iconSizeClass} text-white`} />; - } else if (iconContent.type === 'span') { - // For text content, add text size - content = <span className={`text-white font-bold leading-none ${textSizeClass}`}>{(iconContent as any).props.children}</span>; - } - } + // Apply size classes to icon content + const content = typeof iconContent === 'object' && 'type' in iconContent + ? React.cloneElement(iconContent as React.ReactElement, { + className: `${iconSizeClass} text-white` + }) + : iconContent;Then in the switch cases, create text spans directly with size classes:
case "warning": bgColor = isHovered ? "bg-orange-500" : "bg-orange-400"; - iconContent = <span className="text-white font-bold leading-none">!</span>; + iconContent = <span className={`text-white font-bold leading-none ${textSizeClass}`}>!</span>; break;internal-packages/ai/src/analysis-plugins/plugins/math/index.ts (1)
160-168: Update return type to useCommentVariantfor consistency.The
getLevel()method should returnCommentVarianttype instead of the hardcoded union type for better maintainability and consistency with the refactoring.Apply this diff:
- private getLevel(): "error" | "warning" | "info" | "success" { + private getLevel(): CommentVariant { if (this.verificationResult.status === "verified_true") { return "success"; } else if (this.verificationResult.status === "verified_false") { return "error"; } else { return "info"; } }Also add the import at the top of the file:
import type { Comment, + CommentVariant, ToolChainResult, } from "../../../shared/types";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
apps/web/src/app/api/evaluators/[agentId]/export-data/route.ts(1 hunks)apps/web/src/app/docs/[docId]/export/ExportClient.tsx(3 hunks)apps/web/src/app/docs/[docId]/export/page.tsx(1 hunks)apps/web/src/app/monitor/evals/page.tsx(3 hunks)apps/web/src/app/tools/utils/toolExamples.ts(3 hunks)apps/web/src/application/workflows/evaluation/exportXml.ts(3 hunks)apps/web/src/application/workflows/evaluation/types.ts(3 hunks)apps/web/src/components/AgentDetail/tabs/EvaluationsTab.tsx(1 hunks)apps/web/src/components/AgentDetail/types.ts(2 hunks)apps/web/src/components/CommentVariantIndicator.tsx(1 hunks)apps/web/src/components/DocumentWithEvaluations/components/CommentModal.tsx(3 hunks)apps/web/src/components/DocumentWithEvaluations/components/CommentModalOptimized.tsx(3 hunks)apps/web/src/components/DocumentWithEvaluations/components/EvaluationAnalysisSection.tsx(1 hunks)apps/web/src/components/DocumentWithEvaluations/components/EvaluationView.tsx(4 hunks)apps/web/src/components/DocumentWithEvaluations/components/PositionedComment.tsx(2 hunks)apps/web/src/components/EvaluationComments.tsx(4 hunks)apps/web/src/infrastructure/export/document-export-service.ts(2 hunks)apps/web/src/models/Document.ts(1 hunks)apps/web/src/shared/types/commentTypes.ts(3 hunks)apps/web/src/shared/types/validationSchemas.ts(2 hunks)apps/web/src/shared/utils/commentToYaml.ts(1 hunks)internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/builder.ts(3 hunks)internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/markdown.ts(2 hunks)internal-packages/ai/src/analysis-plugins/plugins/fact-check/fact-check-e2e.integration.vtest.ts(4 hunks)internal-packages/ai/src/analysis-plugins/plugins/fact-check/fact-check-location.vtest.ts(3 hunks)internal-packages/ai/src/analysis-plugins/plugins/fact-check/index.ts(1 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/FallacyIssue.ts(2 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/builder.ts(3 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/markdown.ts(2 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/fallacy-check-iteration.vtest.ts(2 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/index.ts(1 hunks)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/realistic-test.llm.vtest.ts(4 hunks)internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts(4 hunks)internal-packages/ai/src/analysis-plugins/plugins/math/index.ts(5 hunks)internal-packages/ai/src/analysis-plugins/plugins/math/math.vtest.ts(1 hunks)internal-packages/ai/src/analysis-plugins/plugins/spelling/index.ts(2 hunks)internal-packages/ai/src/analysis-plugins/utils/CommentBuilder.ts(3 hunks)internal-packages/ai/src/index.ts(1 hunks)internal-packages/ai/src/shared/types.ts(2 hunks)internal-packages/ai/src/tools/fallacy-review/index.ts(2 hunks)internal-packages/ai/src/tools/fallacy-review/types.ts(2 hunks)internal-packages/ai/src/tools/generated-schemas.ts(3 hunks)internal-packages/ai/src/tools/link-validator/linkHighlightGenerator.ts(1 hunks)internal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql(1 hunks)internal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sql(1 hunks)internal-packages/db/prisma/schema.prisma(1 hunks)internal-packages/jobs/src/core/JobOrchestrator.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
internal-packages/db/prisma/migrations/*_*/migration.sql
📄 CodeRabbit inference engine (CLAUDE.md)
internal-packages/db/prisma/migrations/*_*/migration.sql: Always create Prisma migration files; do not run manual SQL outside migration files
Migration directories must be timestamped_name and contain a migration.sql file for each change
Files:
internal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sqlinternal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql
internal-packages/db/prisma/migrations/**
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all new Prisma migration files are committed with PRs (common issue: missing migrations)
Files:
internal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sqlinternal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql
🧠 Learnings (2)
📚 Learning: 2025-10-10T21:01:01.979Z
Learnt from: CR
Repo: quantified-uncertainty/roast-my-post PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T21:01:01.979Z
Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Always create Prisma migration files; do not run manual SQL outside migration files
Applied to files:
internal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sqlinternal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql
📚 Learning: 2025-10-10T21:01:01.979Z
Learnt from: CR
Repo: quantified-uncertainty/roast-my-post PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-10T21:01:01.979Z
Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Migration directories must be timestamped_name and contain a migration.sql file for each change
Applied to files:
internal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sqlinternal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql
🧬 Code graph analysis (23)
internal-packages/ai/src/shared/types.ts (1)
internal-packages/ai/src/index.ts (1)
CommentVariant(124-124)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/fallacy-check-iteration.vtest.ts (1)
apps/web/src/models/Document.ts (1)
comments(378-404)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/FallacyIssue.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/models/Document.ts (1)
apps/web/src/shared/types/commentTypes.ts (1)
getCommentProperty(110-117)
internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/markdown.ts (3)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/markdown.ts (1)
getVariant(44-46)internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts (1)
getVariant(184-222)internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
internal-packages/ai/src/tools/fallacy-review/types.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/components/CommentVariantIndicator.tsx (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/application/workflows/evaluation/exportXml.ts (2)
internal-packages/ai/src/index.ts (1)
CommentVariant(124-124)internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/app/docs/[docId]/export/ExportClient.tsx (2)
internal-packages/ai/src/index.ts (1)
CommentVariant(124-124)internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/builder.ts (3)
internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/markdown.ts (1)
getVariant(162-176)internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/markdown.ts (1)
getVariant(44-46)internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts (1)
getVariant(184-222)
apps/web/src/application/workflows/evaluation/types.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/app/monitor/evals/page.tsx (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/markdown.ts (2)
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/FallacyIssue.ts (1)
FallacyIssue(12-295)internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
internal-packages/ai/src/analysis-plugins/utils/CommentBuilder.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/components/AgentDetail/types.ts (2)
internal-packages/ai/src/index.ts (1)
CommentVariant(124-124)internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/components/DocumentWithEvaluations/components/PositionedComment.tsx (1)
apps/web/src/components/CommentVariantIndicator.tsx (1)
CommentVariantIndicator(19-102)
apps/web/src/components/EvaluationComments.tsx (2)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)apps/web/src/components/CommentVariantIndicator.tsx (1)
CommentVariantIndicatorCompact(107-146)
apps/web/src/components/DocumentWithEvaluations/components/EvaluationView.tsx (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/components/DocumentWithEvaluations/components/CommentModalOptimized.tsx (1)
apps/web/src/components/CommentVariantIndicator.tsx (1)
CommentVariantIndicator(19-102)
apps/web/src/components/DocumentWithEvaluations/components/CommentModal.tsx (1)
apps/web/src/components/CommentVariantIndicator.tsx (1)
CommentVariantIndicator(19-102)
internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
apps/web/src/shared/types/commentTypes.ts (1)
internal-packages/ai/src/shared/types.ts (1)
CommentVariant(12-12)
internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/builder.ts (1)
internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/markdown.ts (1)
getVariant(162-176)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Application
- GitHub Check: Docker Build Test
- GitHub Check: Integration Tests
- GitHub Check: Build and Push Docker Images (Dockerfile.worker, -worker, worker)
- GitHub Check: Build and Push Docker Images (Dockerfile, main)
🔇 Additional comments (75)
internal-packages/ai/src/analysis-plugins/plugins/fact-check/fact-check-location.vtest.ts (3)
86-102: LGTM! Property rename correctly applied.The guard condition and property accesses have been correctly updated from
comment.locationtocomment.highlight. The test logic properly verifies that chunk-relative offsets are converted to document-absolute offsets.
121-127: LGTM! Destructuring updated correctly.The guard condition and destructuring have been properly updated to use
comment.highlightinstead ofcomment.location, ensuring the test correctly validates handling of chunks without position metadata.
151-151: LGTM! Field rename correctly implemented.The filter condition has been correctly updated from
c.level !== 'debug'toc.variant !== 'debug', consistent with the PR objective to rename the 'level' field to 'variant' throughout the codebase.apps/web/src/models/Document.ts (1)
141-161: Comment variant mapping looks correctUsing
getCommentProperty(comment, 'variant', null)keeps the newvariantfield consistent with howheader,source, andmetadataare handled and aligns with thelevel→variantrename. No issues from this change in the transformer.apps/web/src/app/tools/utils/toolExamples.ts (1)
365-447: LGTM! Example data properly updated.All
levelfields have been consistently renamed tovariantwith valid enum values ('warning', 'nitpick', 'info', 'error') that align with the newCommentVariantschema.apps/web/src/infrastructure/export/document-export-service.ts (2)
46-64: LGTM! Export mapping updated correctly.The
extractCommentsmethod now properly mapscomment.variantinstead ofcomment.level, ensuring exported data uses the new field name.
124-146: LGTM! Markdown badge rendering updated.The Markdown exporter now correctly reads
comment.variantfor badge generation, maintaining consistency with the data model changes.internal-packages/ai/src/analysis-plugins/plugins/math/math.vtest.ts (1)
161-166: LGTM! Test updated to use new field.The test assertion correctly uses
c.variant === 'error'instead of the oldc.level === 'error', maintaining test validity after the rename.apps/web/src/shared/types/validationSchemas.ts (2)
24-40: LGTM! Validation schema updated with proper typing.The
CommentValidationSchemanow uses the centralizedCommentVariantSchemaenum, providing consistent validation across the application.
8-9: Remove duplicate database migration file and verify enum constraint strategy.The migrations show two identical files (
20241121_rename_level_to_variant/migration.sqland20251121132349_rename_level_to_variant/migration.sql) both renaming the column from "level" to "variant". Only one migration should exist; the older 2024-dated file should be removed.Additionally, the Prisma schema defines
variantasString?without database-level enum constraints. While the Zod validation schema properly enforces the six values ('error', 'warning', 'nitpick', 'info', 'success', 'debug'), consider whether database-level constraints would strengthen data integrity. UI component handling is confirmed across EvaluationComments.tsx, CommentVariantIndicator.tsx, and related components.⛔ Skipped due to learnings
Learnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/** : Ensure all new Prisma migration files are committed with PRs (common issue: missing migrations)Learnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Always create Prisma migration files; do not run manual SQL outside migration filesLearnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Migration directories must be timestamped_name and contain a migration.sql file for each changeapps/web/src/components/AgentDetail/types.ts (2)
1-1: LGTM! Proper import added.The
CommentVarianttype is correctly imported from@roast/aito support strongly-typed variant fields.
44-54: Excellent type safety improvement!Changing from
level?: string | nulltovariant?: CommentVariant | nullprovides compile-time type checking and prevents invalid variant values. This is a significant improvement over the loosely-typed string.internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/markdown.ts (2)
4-4: LGTM! Type import added.The
CommentVarianttype import enables proper typing for thegetVariantfunction return value.
159-176: Public API change handled correctly.The function rename from
getLeveltogetVariantis a breaking change, but based on the related code snippets, all callers (e.g.,builder.ts) have been updated accordingly. The function logic remains unchanged, only the naming and return type annotation have been updated to reflect the new terminology.internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/realistic-test.llm.vtest.ts (2)
152-161: LGTM! Test updated to use variant field.The test correctly groups results by
variantinstead oflevel, with proper filtering logic and console output.
228-267: LGTM! Remaining test cases updated consistently.Tests 3 and 4 properly use
byVariantvariable naming and correctly filter/display variant information.internal-packages/ai/src/analysis-plugins/plugins/fact-check/comments/builder.ts (2)
5-12: LGTM! Import updated correctly.The import statement now references
getVariantinstead ofgetLevel, aligning with the function rename inmarkdown.ts.
32-56: LGTM! Comment builder updated properly.The builder function now:
- Calls
getVariant(fact)instead ofgetLevel(fact)- Stores result in
variantvariable- Passes
variantfield toCommentBuilder.build()All changes are consistent with the refactoring.
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/index.ts (1)
206-213: LGTM! Correct field rename with appropriate fallback.The change from
leveltovariantis correct and includes a sensible default value of'warning'when the variant is absent.internal-packages/ai/src/index.ts (1)
121-129: LGTM! Correct export addition.Adding
CommentVariantto the exported types makes the new type available to consumers of the@roast/aipackage, which is necessary for the refactoring.internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/FallacyIssue.ts (2)
1-2: LGTM! Correct import addition.The
CommentVarianttype import is necessary for the renamed method's return type.
62-108: LGTM! Method renamed correctly with updated documentation.The rename from
getCommentLevel()togetCommentVariant()is executed correctly:
- Documentation updated to reflect new name
- Return type changed to
CommentVariant- Implementation logic preserved (only name/type changed)
internal-packages/db/prisma/schema.prisma (1)
157-172: LGTM! Schema field renamed correctly.The field rename from
leveltovariantin theEvaluationCommentmodel is correct and aligns with the migration SQL. The optionalString?type is preserved appropriately.internal-packages/jobs/src/core/JobOrchestrator.ts (1)
379-393: LGTM! Database write path updated correctly.The change from
leveltovariantin the comment persistence logic is correct and aligns with the updated Prisma schema. The fallback tonullappropriately handles optional values.internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/markdown.ts (2)
1-1: LGTM! Correct type import.The
CommentVarianttype import is necessary for the renamed function's return type.
41-46: LGTM! Function renamed correctly with improved documentation.The rename from
getLevel()togetVariant()is executed correctly:
- Documentation enhanced to describe "visual variant" (more descriptive)
- Return type changed to
CommentVariant- Implementation updated to call the renamed
getCommentVariant()methodinternal-packages/db/prisma/migrations/20251121132349_rename_level_to_variant/migration.sql (1)
1-2: Duplicate migration detected - this will cause migration conflicts.This migration file has identical SQL to
internal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql. Having two migrations that perform the same schema change will cause issues:
- The first migration will succeed
- The second migration will fail because the column "level" no longer exists
- This breaks the migration chain and prevents database updates
Resolution: Remove one of the duplicate migration files. Based on timestamps, keep the one with the proper format (
20251121132349_rename_level_to_variant) and remove20241121_rename_level_to_variant(which also has an incomplete timestamp format).Note: As per coding guidelines, migration directories must be
timestamped_nameformat with proper timestamps.⛔ Skipped due to learnings
Learnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Migration directories must be timestamped_name and contain a migration.sql file for each changeLearnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/*_*/migration.sql : Always create Prisma migration files; do not run manual SQL outside migration filesLearnt from: CR Repo: quantified-uncertainty/roast-my-post PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-10-10T21:01:01.979Z Learning: Applies to internal-packages/db/prisma/migrations/** : Ensure all new Prisma migration files are committed with PRs (common issue: missing migrations)apps/web/src/application/workflows/evaluation/exportXml.ts (3)
2-2: LGTM! Clean import addition.The CommentVariant import from @roast/ai is properly added to support the field rename.
26-26: Type definition updated correctly.The comment schema now uses the strongly-typed
CommentVariantinstead of a loose string type, improving type safety.
126-128: XML emission updated consistently.The XML output now emits
<variant>tags instead of<level>, maintaining the same conditional logic and structure.internal-packages/ai/src/shared/types.ts (2)
9-12: Well-defined type with clear semantics.The
CommentVarianttype provides a clear set of visual variants for comment display. The six variants cover the full spectrum from errors to debug information.
57-57: Interface updated with improved type safety.The Comment interface now uses the strongly-typed
CommentVariantinstead of a generic string, which will help catch invalid variant values at compile time.apps/web/src/shared/utils/commentToYaml.ts (1)
30-32: YAML serialization updated correctly.The YAML output now emits
variant:instead oflevel:, keeping the serialization format consistent with the schema changes.apps/web/src/app/monitor/evals/page.tsx (2)
13-13: Import added for type safety.CommentVariant import enables proper typing when mapping comment data to the EvaluationContent component.
48-48: Type handling is appropriate for database data.The interface defines
variantasstring | null(reflecting the database schema's loose typing), then casts toCommentVariant | nullat usage (Line 280). This approach safely handles database data while providing type safety to downstream components.Also applies to: 280-280
internal-packages/ai/src/tools/fallacy-review/types.ts (2)
5-5: Import added correctly.CommentVariant import enables the type to be used in the ReviewComment interface.
20-21: Interface updated with clear semantics.The field rename to
variantwith the JSDoc comment "Visual variant for display" better communicates the field's purpose compared to the previouslevelnaming.apps/web/src/app/api/evaluators/[agentId]/export-data/route.ts (1)
236-236: Export mapping updated consistently.The agent data export now includes
variantinstead oflevelin the comment mappings, ensuring exported data matches the new schema.internal-packages/ai/src/analysis-plugins/plugins/spelling/index.ts (2)
317-318: Debug logging updated consistently.The debug log now uses
variantfield name, maintaining the same conditional logic for error categorization.
336-336: CommentBuilder parameter updated correctly.The
variantparameter replaceslevelin the CommentBuilder call, with the same error-type-based logic (grammar → warning, spelling → error).internal-packages/ai/src/tools/link-validator/linkHighlightGenerator.ts (1)
190-190: Comment object updated with correct field name.The generated link highlights now use
variantinstead oflevel, with the grade-based logic correctly mapping to valid CommentVariant values (success/warning/error).apps/web/src/components/DocumentWithEvaluations/components/CommentModal.tsx (1)
23-23: LGTM! Clean refactoring from level to variant.The changes successfully replace the previous level-based indicator logic with the new variant-based approach:
- Properly imports and uses the new
CommentVariantIndicatorcomponent- Correctly accesses
comment.variantwith appropriate fallback to "info"- Maintains consistent UI presentation while centralizing variant display logic
Also applies to: 94-94, 174-174
apps/web/src/components/DocumentWithEvaluations/components/EvaluationAnalysisSection.tsx (1)
132-132: LGTM! Correct field rename in comment mapping.The change properly updates the field name from
leveltovariantin the comment mapping forEvaluationComments. The null coalescing operator is appropriately maintained.internal-packages/ai/src/analysis-plugins/plugins/fact-check/index.ts (1)
469-469: LGTM! Correct variant field usage.The update properly replaces
level: "debug"withvariant: "debug"in theCommentBuilder.buildcall, maintaining the type constraint withas const.internal-packages/ai/src/tools/fallacy-review/index.ts (1)
25-25: LGTM! Schema and output updated correctly.Both the Zod schema definition and the formatted comment output have been properly updated to use
variantinstead oflevel. The enum values ('error', 'warning', 'nitpick', 'info', 'success', 'debug') are comprehensive and consistent with the variant indicator logic.Also applies to: 69-69
apps/web/src/app/docs/[docId]/export/page.tsx (1)
108-108: LGTM! Export data mapping updated correctly.The comment export mapping has been properly updated to include
variantinstead oflevel, ensuring the exported data structure aligns with the new field name.apps/web/src/components/DocumentWithEvaluations/components/CommentModalOptimized.tsx (1)
15-15: LGTM! Consistent variant refactoring.The changes mirror the approach in
CommentModal.tsx, correctly:
- Importing and using
CommentVariantIndicator- Accessing
comment.variantwith fallback to "info"- Replacing the previous level-based indicator logic
Also applies to: 143-143, 222-222
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/fallacy-check-iteration.vtest.ts (1)
62-71: LGTM! Test suite properly updated for variant field.The test file has been comprehensively updated:
- Comment grouping logic now filters by
variantinstead oflevel- Output strings updated to reflect "By Variant" terminology
- All variant types (error, warning, nitpick, success, info) are covered
- Logging and assertions remain functionally equivalent
Also applies to: 96-106
apps/web/src/components/AgentDetail/tabs/EvaluationsTab.tsx (1)
258-258: LGTM! Comment mapping updated correctly.The field has been properly renamed from
leveltovariantin the comment mapping passed toEvaluationContent, maintaining the null coalescing operator for safe handling of undefined values.internal-packages/ai/src/tools/generated-schemas.ts (2)
1-7: LGTM! Auto-generated file properly updated.The schema hash has been updated to reflect the rename from
leveltovariant, indicating the schemas were properly regenerated. The warning comment about manual editing is clear and appropriate.
2298-2320: Consistent field renaming in tool schema.The
fallacy-reviewinput schema correctly renames the field fromleveltovariantwhile maintaining the same enum values and updating the required properties list. The change is consistent with the overall refactoring.apps/web/src/application/workflows/evaluation/types.ts (2)
1-1: Proper type import for improved type safety.Adding the
CommentVariantimport from@roast/aienables type-safe usage of the variant field throughout the application.
20-20: Enhanced type safety with CommentVariant.The change from
level?: string | nulltovariant?: CommentVariant | nullprovides better type safety by constraining the values to the specific enum options ('error' | 'warning' | 'nitpick' | 'info' | 'success' | 'debug'). This will help catch errors at compile time.Also applies to: 78-78
internal-packages/ai/src/analysis-plugins/utils/CommentBuilder.ts (3)
1-1: Type-safe variant field in builder options.The import of
CommentVariantand its use inCommentBuildOptionsensures type safety when building comments. This prevents invalid variant values at compile time.Also applies to: 31-31
65-65: Validation updated consistently.The validation error message correctly references "Variant" instead of "Level", ensuring clear error messages for developers.
71-71: Correct field name in returned Comment object.The returned comment object properly uses the
variantfield, maintaining consistency with the type definitions and the overall refactoring.internal-packages/ai/src/analysis-plugins/plugins/fact-check/fact-check-e2e.integration.vtest.ts (4)
48-54: Test assertions properly updated for refactored structure.The test correctly updates field access from
plugin→source,location→highlight, andlevel→variant. The regex pattern properly validates variant values.
95-95: Error detection test updated correctly.The assertion properly checks for
variant === 'error'to detect false claims, maintaining the same test logic with the new field name.
139-150: Location validation updated to use highlight.The test correctly validates the
highlightobject instead oflocation, checkingstartOffset,endOffset, andquotedTextfields. The substring extraction logic properly uses the highlight offsets.
201-201: Trivial claims test maintains correct logic.The assertion correctly checks for
variantmatching 'debug' or 'info' for trivial claims, ensuring low-importance claims are categorized appropriately.apps/web/src/app/docs/[docId]/export/ExportClient.tsx (2)
7-7: Type-safe Comment interface with CommentVariant.The import and interface update provide type safety for the export functionality, ensuring only valid variant values are exported.
Also applies to: 17-17
185-185: Export logic correctly uses variant field.The export logic properly checks for and exports the
variantfield in expanded mode, maintaining consistency with the refactoring. The conditional export pattern matches other metadata fields.apps/web/src/components/DocumentWithEvaluations/components/EvaluationView.tsx (3)
12-12: Clear function rename with improved type safety.The rename from
getLevelHighlightColortogetVariantHighlightColorbetter reflects the new terminology, and theCommentVariantparameter type ensures type safety for the color mapping logic.Also applies to: 27-28
106-106: Debug comment filtering updated correctly.The filter logic correctly checks
comment.variant !== "debug"to exclude debug comments when the debug flag is false, maintaining the same filtering behavior.
255-255: Highlight color computation uses renamed function.The highlight color is correctly computed using the updated function name and the
comment.variantfield, ensuring proper visual differentiation of comment types.apps/web/src/shared/types/commentTypes.ts (3)
8-8: Type safety improved with CommentVariant.The import and field type update from
string | nulltoCommentVariant | nullprovides compile-time validation of variant values, preventing invalid values from being used.Also applies to: 24-24
15-15: Omit type correctly updated.The
Omittype properly excludes 'variant' instead of 'level' from the AIComment type, maintaining consistency with the refactored field name.
85-85: Database mapping correctly handles variant field.The
aiCommentToDatabaseCommentfunction properly spreads thevariantfield when converting from AI comments to database format, maintaining the same conditional spreading pattern used for other optional fields.internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/comments/builder.ts (1)
8-8: LGTM! Consistent renaming fromleveltovariant.The refactoring is clean and consistent across import, variable declaration, and field usage.
Also applies to: 34-34, 51-51
internal-packages/ai/src/analysis-plugins/plugins/forecast/index.ts (1)
4-4: LGTM! Method and field successfully renamed tovariant.The import, method signature, and field usages are correctly updated to use
variantterminology.Also applies to: 184-184, 263-263, 706-706
apps/web/src/components/CommentVariantIndicator.tsx (2)
1-102: LGTM! Well-structured component with comprehensive variant support.The
CommentVariantIndicatorcomponent provides good support for all variant types with configurable sizes and hover states.
107-146: LGTM! Compact variant is consistent and appropriate for evaluation comments.The compact indicator correctly maps all variants and uses appropriate icons and colors.
internal-packages/ai/src/analysis-plugins/plugins/math/index.ts (1)
139-139: LGTM! Field name successfully updated fromleveltovariant.All usages of the field have been consistently renamed across the math plugin.
Also applies to: 378-382, 760-760, 816-816, 871-871
apps/web/src/components/EvaluationComments.tsx (2)
14-15: LGTM! Successfully integratedCommentVariantIndicatorCompact.The component imports and type definitions are correctly updated to use the new variant-based indicator component.
Also applies to: 26-26, 74-74
128-139: LGTM! Badge styling correctly supports all variant types includingnitpick.The variant badge logic comprehensively handles all variant types with appropriate color schemes, including the newly added
nitpickvariant.
| import type { Comment } from "@roast/ai"; | ||
|
|
||
| import IndexFingerIcon from "../../../../public/app-icons/indexFinger.svg"; | ||
| import { CommentVariantIndicator } from "@/components/CommentVariantIndicator"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excellent refactoring to use shared CommentVariantIndicator.
Replacing the duplicated indicator logic with the CommentVariantIndicator component eliminates code duplication and ensures consistent variant rendering across the UI. The component receives appropriate props (variant, size, isHovered, className) for proper display.
Also applies to: 120-120
🤖 Prompt for AI Agents
In
apps/web/src/components/DocumentWithEvaluations/components/PositionedComment.tsx
at lines 16 and 120, the file already imports CommentVariantIndicator at line 16
but line 120 still contains duplicated indicator rendering logic; replace that
duplicated logic at line 120 with the shared CommentVariantIndicator component,
passing the same props used elsewhere (variant, size, isHovered, className) so
both places use the single component and remove the duplicated code.
There was a problem hiding this comment.
Actually it was my idea to introduce such component:)
There was a problem hiding this comment.
@NykolaySavin That's great! 🎉 It's an excellent design decision. Introducing the CommentVariantIndicator component really improves code maintainability by eliminating the duplicated indicator logic across multiple UI components. The component's API is clean and flexible with the size, isHovered, and className props, making it easy to use consistently throughout the codebase. Well done on this refactoring!
ヽ(^◇^*)/
| const byLevel = { | ||
| error: result.comments.filter(c => c.level === 'error'), | ||
| warning: result.comments.filter(c => c.level === 'warning'), | ||
| error: result.comments.filter(c => c.variant === 'error'), | ||
| warning: result.comments.filter(c => c.variant === 'warning'), | ||
| }; | ||
|
|
||
| console.log(`By Level: ${byLevel.error.length} errors, ${byLevel.warning.length} warnings\n`); | ||
| console.log(`By Variant: ${byLevel.error.length} errors, ${byLevel.warning.length} warnings\n`); |
There was a problem hiding this comment.
Inconsistent variable naming.
The variable is named byLevel but filters by variant field. For consistency with the rest of the file (lines 153-159, 228-233, 262-267), this should be renamed to byVariant.
Apply this diff:
- const byLevel = {
- error: result.comments.filter(c => c.variant === 'error'),
- warning: result.comments.filter(c => c.variant === 'warning'),
+ const byVariant = {
+ error: result.comments.filter(c => c.variant === 'error'),
+ warning: result.comments.filter(c => c.variant === 'warning'),
};
- console.log(`By Variant: ${byLevel.error.length} errors, ${byLevel.warning.length} warnings\n`);
+ console.log(`By Variant: ${byVariant.error.length} errors, ${byVariant.warning.length} warnings\n`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const byLevel = { | |
| error: result.comments.filter(c => c.level === 'error'), | |
| warning: result.comments.filter(c => c.level === 'warning'), | |
| error: result.comments.filter(c => c.variant === 'error'), | |
| warning: result.comments.filter(c => c.variant === 'warning'), | |
| }; | |
| console.log(`By Level: ${byLevel.error.length} errors, ${byLevel.warning.length} warnings\n`); | |
| console.log(`By Variant: ${byLevel.error.length} errors, ${byLevel.warning.length} warnings\n`); | |
| const byVariant = { | |
| error: result.comments.filter(c => c.variant === 'error'), | |
| warning: result.comments.filter(c => c.variant === 'warning'), | |
| }; | |
| console.log(`By Variant: ${byVariant.error.length} errors, ${byVariant.warning.length} warnings\n`); |
🤖 Prompt for AI Agents
internal-packages/ai/src/analysis-plugins/plugins/fallacy-check/realistic-test.llm.vtest.ts
around lines 194 to 199: the variable is named `byLevel` but the rest of the
file uses `byVariant`; rename the variable to `byVariant` and update all
immediate references in this block (e.g., the console.log) to `byVariant` so the
naming is consistent with lines 153-159, 228-233, and 262-267.
internal-packages/db/prisma/migrations/20241121_rename_level_to_variant/migration.sql
Outdated
Show resolved
Hide resolved
|
@OAGr how can I run DB migration for testing environment? Probably this is the issue, why document pages fail here (all good on local) |
|
"@OAGr how can I run DB migration for testing environment? Probably this is the issue, why document pages fail here (all good on local)" -> We only have one staging DB, which we use for all feature branches. There's no great way to test it online. If you're pretty sure this will work, then remove the "Draft" status, and we can push to main / staging. Then that should trigger a DB migration. A bit hacky, but setting up feature-specific DBs would be a fair bit more work. |
User description
Implements GitHub issue #336
Changes:
PR Type
Enhancement, Tests
Description
Renamed Comment
levelfield tovariantacross codebaseUpdated database schema and Prisma migration
Created reusable
CommentVariantIndicatorcomponentUpdated all plugins to use
variantfieldUpdated all tests and type definitions
Diagram Walkthrough
File Walkthrough
18 files
Define CommentVariant typeExport CommentVariant typeUpdate ExtendedComment interfaceAdd CommentVariantSchema validationRename getLevel to getVariant functionUpdate fact-check builder to use variantRename getCommentLevel to getCommentVariantRename getLevel to getVariant functionUpdate fallacy-check builder to use variantRename getLevel to getVariant methodUpdate math plugin to use variantUpdate spelling plugin to use variantUpdate CommentBuilder to use variantUpdate JobOrchestrator to write variantCreate reusable CommentVariantIndicator componentReplace inline level logic with componentUse CommentVariantIndicator componentUse CommentVariantIndicatorCompact component2 files
Rename level to variant columnDatabase migration for column rename1 files
Update tests to use variant field26 files
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.