feat: show criticalityReason context for safe schema changes based on usage#8182
feat: show criticalityReason context for safe schema changes based on usage#8182jenishasubedi05 wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Windows path separators in build scripts, adds the criticalityReason field to the schema changes UI, configures a named volume for ClickHouse in Docker Compose, and updates dependency configurations. However, several issues need to be addressed: temporary bundled .mjs files containing local absolute paths should be removed, the prebuild script skip in package.json should be reverted, JSX whitespace and indentation in errors-and-changes.tsx should be cleaned up, and the Byte Order Mark (BOM) in schema-v1.graphql must be removed to avoid parsing errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import { readFileSync } from "fs"; | ||
| import { join, resolve } from "path"; | ||
| import { getPackagesSync } from "@manypkg/get-packages"; | ||
| var __injected_dirname__ = "C:\\Users\\College\\Desktop\\console\\configs\\tsup"; |
There was a problem hiding this comment.
These bundled .mjs files (e.g., dev.config.node.bundled_*.mjs) appear to be temporary build artifacts generated during development. They contain hardcoded local absolute paths (e.g., C:\\Users\\College\\Desktop\\console...) and should not be committed to the repository. Please remove these files and ensure they are covered by .gitignore.
| "local:setup": "docker compose -f ./docker/docker-compose.dev.yml up -d --build --remove-orphans && pnpm --filter @hive/migrations db:init", | ||
| "postinstall": "node ./scripts/patch-manifests.js && pnpm env:sync && node ./scripts/turborepo-cleanup.js && pnpm cargo:fix", | ||
| "prebuild": "rimraf deploy-tmp && rimraf packages/**/deploy-tmp", | ||
| "prebuild": "echo skip", |
There was a problem hiding this comment.
Changing the "prebuild" script to "echo skip" disables the cleanup of deploy-tmp directories. This can lead to stale build artifacts or bloated deployments. Please revert this change or explain why it is necessary.
| "prebuild": "echo skip", | |
| "prebuild": "rimraf deploy-tmp && rimraf packages/**/deploy-tmp", |
| {change.isSafeBasedOnUsage && change.criticalityReason && ( | ||
| <span className="text-xs text-muted-foreground ml-1"> | ||
| ({change.criticalityReason}) | ||
| </span> | ||
| )} | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Clean up the indentation of the closing </span> tag and remove the unnecessary empty lines/whitespace added within the JSX block to keep the code clean and maintainable.
| {change.isSafeBasedOnUsage && change.criticalityReason && ( | |
| <span className="text-xs text-muted-foreground ml-1"> | |
| ({change.criticalityReason}) | |
| </span> | |
| )} | |
| {change.isSafeBasedOnUsage && change.criticalityReason && ( | |
| <span className="text-xs text-muted-foreground ml-1"> | |
| ({change.criticalityReason}) | |
| </span> | |
| )} |
| type Query { | ||
| hello: String | ||
| legacyField: String | ||
| } |
There was a problem hiding this comment.
The file starts with a Byte Order Mark (BOM) character (\ufeff). UTF-8 BOMs are discouraged as they can cause parsing issues with some GraphQL tools and CLI parsers. Please save the file as standard UTF-8 without BOM.
| type Query { | |
| hello: String | |
| legacyField: String | |
| } | |
| type Query { | |
| hello: String | |
| legacyField: String | |
| } |
…tion, revert prebuild
|
Hi! I've addressed all the review feedback from the bot:
Ready for another look when you have time! |
1 participant
Background
Closes #6892
When a schema change is marked as "safe" due to conditional breaking change settings (usage threshold or client exclusion), the schema check view gives no explanation of why. Developers have to manually cross-reference the target's Conditional Breaking Changes configuration to understand why a change passed.
Description
This PR surfaces the existing
criticalityReasonfield through to the frontend UI.Changes:
packages/services/api/src/modules/schema/providers/inspector.ts— Addedreason: change.criticality.reasonto pass the criticality reason through the modelpackages/web/app/src/components/target/history/errors-and-changes.tsx— AddedcriticalityReasonto both GraphQL fragments and displays it inline next to "Safe based on usage data" in the schema check results viewChecklist
pnpm graphql:generaterun after GraphQL changes