Conversation
… file for comparison, ignore comments - they don't change the schema structurally
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: 0 B Total Size: 486 kB ℹ️ View Unchanged
|
…chema change check
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (e327cb4) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3359If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3359If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3359 |
github-actions
bot
added
the
schema-change
…t change taht shouldn't trigger check
github-actions
bot
removed
the
schema-change
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM - the --removeComments flag addition is straightforward; just remove the test comment from data-schema.ts before merging.
Extended reasoning...
Overview
This PR adds the --removeComments flag to the tsc command in the schema check CI job (.github/workflows/node-ci.yml), so that comment-only changes to data-schema.ts no longer trigger false-positive schema change alerts. The only other change is a leftover test comment in packages/perseus-core/src/data-schema.ts that was used to validate the fix.
Security risks
None. The change is to a CI workflow flag that controls TypeScript compiler output formatting. It does not affect authentication, permissions, data handling, or any runtime code paths.
Level of scrutiny
Low scrutiny is appropriate. This is a single-flag addition to a CI build command - mechanical and easy to verify. The --removeComments flag is a standard tsc option that strips comments from output, which is exactly the desired behavior for comparing schema structure.
Other factors
The inline bug report correctly identifies the test comment as an artifact that should be cleaned up. This is a minor nit that does not affect correctness - the comment is harmless in both the source file and the CI output (since --removeComments now strips it). All CI checks are passing. The PR is well-documented with screenshots showing both positive and negative test cases.
| */ | ||
| export type PerseusArticle = PerseusRenderer | PerseusRenderer[]; | ||
|
|
||
| // Comment change - shouldn't trigger schema check |
There was a problem hiding this comment.
🟡 Nit: The comment // Comment change - shouldn't trigger schema check on line 230 is a test artifact left over from validating the --removeComments feature and should be removed before merging.
Extended reasoning...
What the bug is
Line 230 of packages/perseus-core/src/data-schema.ts contains the comment // Comment change - shouldn't trigger schema check. This is a leftover test artifact from the PR author's test plan, not a meaningful code comment.
How it got there
The PR author added this comment as part of their test plan to verify that comment-only changes no longer trigger the schema change CI check after adding the --removeComments flag to the tsc command. The commit message for e327cb4 explicitly states: "Undo schema change and add comment change that shouldn't trigger check." The PR description also documents this: "I removed above change, added a comment-only change and the schema check cleared."
Step-by-step proof
- The diff shows the addition of
// Comment change - shouldn't trigger schema checkon line 230, right above theVersiontype definition. - The commit e327cb4 message says "Undo schema change and add comment change that shouldn't trigger check" — confirming it was added purely for testing.
- The comment text itself is meta-commentary about the CI process, not documentation about the
Versiontype it precedes. - The PR description explicitly describes adding this comment as a test step.
Impact
No functional impact since it's a comment, but data-schema.ts is the core Perseus schema file with a prominent warning at the top (lines 2-29) about being careful with changes. Leaving test debris in this file adds noise and is inconsistent with the care the file demands.
How to fix
Simply delete line 230 (// Comment change - shouldn't trigger schema check) from packages/perseus-core/src/data-schema.ts.
1 participant
Summary:
In #3357 the Schema Check triggered but the changes were all in code comments. None of these would alter the structure of any Perseus schema types and so it's a false positive and clutters the PR.
This PR changes the
tsccommand to strip comments so that the resulting comparison files are only code/types. This should eliminate these types of false positives, but retain type/schema safety.Issue: LEMS-3949
Test plan:
I added a change that should affect the schema and the schema check triggered.
I removed above change, added a comment-only change and the schema check cleared.
Click the "Edited" button on the Schema Change comment to see earlier runs of the check (as well as its output).
Success!