Skip to content

feat: Add suggestionSnippets field to review schema and prompts #1101

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

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

hoshinotsuyoshi
Copy link
Member

@hoshinotsuyoshi hoshinotsuyoshi commented Apr 3, 2025

  • Update tests in processSaveReview to include an empty suggestionSnippets array.
  • Enhance generateReview prompt documentation to define suggestionSnippets with filename and patch properties.
  • Extend reviewSchema to validate suggestionSnippets as an array of objects.
  • Update test fixtures to assert that suggestionSnippets are correctly included in the review report.

Issue

  • resolve:

Why is this change needed?

This change introduces the suggestionSnippets field to provide patch-level details for each suggestion in the review report. By including this field, reviewers can now receive specific file patch information along with general suggestions, enhancing the clarity and actionability of the feedback. (Note: The UI to display this information is not yet implemented.)

What would you like reviewers to focus on?

  • Verify that the new suggestionSnippets field is correctly integrated in the review schema and reflected in both tests and documentation.
  • Confirm that the schema validation correctly enforces the structure of suggestionSnippets (i.e., an array of objects containing filename and patch).
  • Ensure that test fixtures accurately assert the presence and format of suggestionSnippets in the review report.

Testing Verification

#1101 (comment)

Some tests are failed, but are not relevant to this PR.

スクリーンショット 2025-04-04 18 29 45

What was done

🤖 Generated by PR Agent at b8bfc8e

  • Introduced suggestionSnippets field in review schema for detailed suggestions.
  • Updated tests to validate suggestionSnippets integration and structure.
  • Enhanced prompt documentation to define suggestionSnippets usage.
  • Added test fixtures to assert suggestionSnippets correctness in reports.

Detailed Changes

Relevant files
Tests
processSaveReview.test.ts
Update tests to include `suggestionSnippets` field             

frontend/packages/jobs/src/functions/tests/processSaveReview.test.ts

  • Added suggestionSnippets field to test data for issues.
  • Ensured compatibility with the updated review schema.
  • +1/-0     
    fixture.yaml
    Add test fixtures for `suggestionSnippets` validation       

    frontend/packages/prompt-test/src/fixtures/github.com//pull/1033/fixture.yaml

  • Added test cases to validate suggestionSnippets field.
  • Verified file paths and snippet content in test assertions.
  • +8/-0     
    Documentation
    generateReview.ts
    Enhance prompt documentation for `suggestionSnippets`       

    frontend/packages/jobs/src/prompts/generateReview/generateReview.ts

  • Documented suggestionSnippets field in the prompt structure.
  • Explained its purpose and format for actionable suggestions.
  • +4/-0     
    Enhancement
    reviewSchema.ts
    Extend review schema to include `suggestionSnippets`         

    frontend/packages/jobs/src/prompts/generateReview/reviewSchema.ts

  • Added suggestionSnippets field to the review schema.
  • Defined it as an array of objects with filename and patch.
  • +6/-0     

    Additional Notes


    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @hoshinotsuyoshi hoshinotsuyoshi self-assigned this Apr 3, 2025
    Copy link

    changeset-bot bot commented Apr 3, 2025

    ⚠️ No Changeset found

    Latest commit: 85131cc

    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

    Copy link

    vercel bot commented Apr 3, 2025

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

    Name Status Preview Comments Updated (UTC)
    liam-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2025 10:03am
    liam-erd-sample ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2025 10:03am
    1 Skipped Deployment
    Name Status Preview Comments Updated (UTC)
    liam-docs ⬜️ Ignored (Inspect) Visit Preview Apr 4, 2025 10:03am

    Copy link

    supabase bot commented Apr 3, 2025

    Updates to Preview Branch (review-suggestion-patch-suggestion) ↗︎

    Deployments Status Updated
    Database Fri, 04 Apr 2025 10:01:03 UTC
    Services Fri, 04 Apr 2025 10:01:03 UTC
    APIs Fri, 04 Apr 2025 10:01:03 UTC

    Tasks are run on every commit but only new migration files are pushed.
    Close and reopen this PR if you want to apply changes from existing seed or migration files.

    Tasks Status Updated
    Configurations Fri, 04 Apr 2025 10:01:04 UTC
    Migrations Fri, 04 Apr 2025 10:01:04 UTC
    Seeding Fri, 04 Apr 2025 10:01:04 UTC
    Edge Functions Fri, 04 Apr 2025 10:01:04 UTC

    View logs for this Workflow Run ↗︎.
    Learn more about Supabase for Git ↗︎.

    Copy link
    Contributor

    github-actions bot commented Apr 4, 2025

    frontend/packages/prompt-test result:

    View results: https://app.promptfoo.dev/eval/f:4d45bcee-8391-48c5-9f56-69870146a07e

    ❌️ Promptfoo test failed

    ✅️ Successes ❌️ Failures ⚠️ Errors
    4 2 0

    - Update tests in processSaveReview to include an empty suggestionSnippets array.
    - Enhance generateReview prompt documentation to define suggestionSnippets with filename and patch properties.
    - Extend reviewSchema to validate suggestionSnippets as an array of objects.
    - Update test fixtures to assert that suggestionSnippets are correctly included in the review report.
    @hoshinotsuyoshi hoshinotsuyoshi force-pushed the review-suggestion-patch-suggestion branch from ac0e2bb to b8bfc8e Compare April 4, 2025 09:27
    Comment on lines +6 to +13
    - type: javascript
    # Test 1: Check if the snippet has the exact correct file path
    value: |
    JSON.parse(output).issues.find(issue => issue.kind === "Migration Safety").suggestionSnippets[0].filename === "frontend/packages/db/prisma/migrations/20250328105323_add_branch_name_to_knowledge_suggestion/migration.sql"
    - type: javascript
    # Test 2: Check if the snippet suggests setting a DEFAULT value for the branchName column
    value: |
    JSON.parse(output).issues.find(issue => issue.kind === "Migration Safety").suggestionSnippets[0].snippet.includes("ALTER TABLE \"KnowledgeSuggestion\" ADD COLUMN \"branchName\" TEXT NOT NULL DEFAULT '")
    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    I want to test those

    @hoshinotsuyoshi hoshinotsuyoshi marked this pull request as ready for review April 4, 2025 09:31
    @hoshinotsuyoshi hoshinotsuyoshi requested a review from a team as a code owner April 4, 2025 09:31
    @hoshinotsuyoshi hoshinotsuyoshi requested review from FunamaYukina, junkisai, MH4GF and NoritakaIkeda and removed request for a team April 4, 2025 09:31
    Copy link
    Contributor

    qodo-merge-for-open-source bot commented Apr 4, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 85131cc)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Documentation Inconsistency

    The field is named "suggestionSnippets" in the schema but the documentation refers to "snippet" property while the schema uses "snippet". Ensure naming is consistent across documentation and implementation.

    - "suggestionSnippets": An array of suggestion snippets for each issue kind in the "suggestions" field, each including:
      - "filename": The filename of the file that needs to be applied.
      - "snippet": The snippet of the file that needs to be applied.
        - For example, if DEFAULT value is needed for a column, the snippet should include the statement with the DEFAULT value.

    Copy link
    Contributor

    qodo-merge-for-open-source bot commented Apr 4, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 85131cc
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix field reference error

    The prompt description refers to "each issue kind in the 'suggestions' field",
    but there is no 'suggestions' field in the schema. It should refer to issues in
    the "issues" field instead.

    frontend/packages/jobs/src/prompts/generateReview/generateReview.ts [35-38]

    -+  - "suggestionSnippets": An array of suggestion snippets for each issue kind in the "suggestions" field, each including:
    ++  - "suggestionSnippets": An array of suggestion snippets for each issue in the "issues" field, each including:
     +    - "filename": The filename of the file that needs to be applied.
     +    - "snippet": The snippet of the file that needs to be applied.
     +      - For example, if DEFAULT value is needed for a column, the snippet should include the statement with the DEFAULT value.

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies an important error in the prompt description. The text incorrectly references a non-existent "suggestions" field when it should refer to the "issues" field, which could cause confusion for users and potential implementation errors.

    Medium
    • More

    Previous suggestions

    ✅ Suggestions up to commit b8bfc8e
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Align field naming conventions
    Suggestion Impact:The commit directly implemented the suggestion by changing the field name from 'patch' to 'snippet' in the schema definition, ensuring consistent naming across all files

    code diff:

    -          patch: string(),
    +          snippet: string(),

    The field name in the schema is patch but in the prompt and test it's referred
    to as snippet. Ensure consistent naming across all files to avoid confusion and
    potential bugs.

    frontend/packages/jobs/src/prompts/generateReview/reviewSchema.ts [34-38]

     +      suggestionSnippets: array(
     +        object({
     +          filename: string(),
    -+          patch: string(),
    ++          snippet: string(),
     +        }),
     +      ),
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion identifies a critical inconsistency between the schema definition and its usage elsewhere. The field is defined as "patch" in the schema but referred to as "snippet" in the prompt and tests, which would cause runtime errors or data mismatches.

    High
    General
    Improve test coverage

    The test is using an empty array for suggestionSnippets, but this might not
    properly test the functionality. Consider adding a mock snippet to ensure the
    feature works correctly.

    frontend/packages/jobs/src/functions/tests/processSaveReview.test.ts [99]

    -+            suggestionSnippets: [],
    ++            suggestionSnippets: [
    ++              {
    ++                filename: 'test-file.sql',
    ++                snippet: 'ALTER TABLE "Test" ADD COLUMN',
    ++              },
    ++            ],
    Suggestion importance[1-10]: 6

    __

    Why: Adding a mock snippet with realistic test data would improve test coverage by ensuring the new suggestionSnippets functionality works correctly. While not fixing a critical bug, this enhances test quality and helps prevent future regressions.

    Low
    Learned
    best practice
    Add validation for new fields to prevent empty or invalid data from being processed

    The new suggestionSnippets array is added without validation for its contents.
    Consider adding validation to ensure that the filename is a valid path and that
    the patch contains valid content. This will prevent unexpected behavior when
    processing these fields later.

    frontend/packages/jobs/src/prompts/generateReview/reviewSchema.ts [34-39]

     export const reviewSchema = object({
       bodyMarkdown: string(),
       issues: array(
         object({
           kind: KindEnum,
           severity: SeverityEnum,
           description: string(),
           suggestion: string(),
           suggestionSnippets: array(
             object({
    -          filename: string(),
    -          patch: string(),
    +          filename: string().min(1).refine(val => val.trim().length > 0, {
    +            message: 'Filename cannot be empty'
    +          }),
    +          patch: string().min(1).refine(val => val.trim().length > 0, {
    +            message: 'Patch cannot be empty'
    +          }),
             }),
           ),
         }),
       ),
       scores: array(
         object({
     ...
    Suggestion importance[1-10]: 6
    Low

    @hoshinotsuyoshi
    Copy link
    Member Author

    hoshinotsuyoshi commented Apr 4, 2025

    ah, not suggestionPatches, but suggestionSnippets . I re-wrote pr descriptions (and title)


    (edited )done

    @hoshinotsuyoshi hoshinotsuyoshi changed the title feat: Add suggestionPatches field to review schema and prompts feat: Add suggestionSnippets field to review schema and prompts Apr 4, 2025
    @hoshinotsuyoshi hoshinotsuyoshi marked this pull request as draft April 4, 2025 09:58
    @hoshinotsuyoshi
    Copy link
    Member Author

    hoshinotsuyoshi commented Apr 4, 2025

    sorry, I'm recheking frontend/packages/jobs/src/prompts/generateReview/reviewSchema.ts

    done

    Copy link
    Member

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

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

    LGTM 👍🏻

    @MH4GF MH4GF added this pull request to the merge queue Apr 4, 2025
    Merged via the queue into main with commit b9445e0 Apr 4, 2025
    19 checks passed
    @MH4GF MH4GF deleted the review-suggestion-patch-suggestion branch April 4, 2025 11:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants