feat: Upgrade ESLint packages and improve diff handling #621
Conversation
🦋 Changeset detectedLatest commit: 1c5ddfe The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change updates dependency versions for various linting, formatting, and related packages across multiple package manifests and constants. It introduces a granular overwrite and diff application mechanism during file generation, replacing the previous force-overwrite approach. Several utility and test files are refactored to use non-mutating array reversal. The snapshot diff application is now integrated per-file during output writing. Changes
Sequence Diagram(s)sequenceDiagram
participant Generator as Generator
participant Snapshot as SnapshotManifest
participant Writer as writeGeneratorOutput
participant DiffUtil as applySnapshotToFileContents
Generator->>Writer: generateForDirectory(..., overwriteOptions)
loop For each file
Writer->>DiffUtil: applyDiff(relativePath, generatedContents)
alt File is deleted in snapshot
DiffUtil-->>Writer: undefined (skip file)
else File is binary
DiffUtil-->>Writer: original contents
else File is modified
DiffUtil->>Snapshot: read diff file
DiffUtil->>DiffUtil: applyPatch(generatedContents, diff)
DiffUtil-->>Writer: patched contents
else
DiffUtil-->>Writer: original contents
end
Writer->>Writer: skipFile?(relativePath)
alt skipFile returns true
Writer-->>Generator: skip writing file
else
Writer->>Generator: write file with (possibly patched) contents
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/project-builder-server/src/sync/generate-for-directory.ts (1)
104-128: Well-designed callback-based overwrite options architecture.The new
overwriteOptionsstructure withapplyDiffandskipFilecallbacks provides excellent separation of concerns:
applyDiff: Handles per-file snapshot application with proper error loggingskipFile: Uses ignore patterns for file filtering- Proper async handling and error reporting
The warning message for invalid patches is helpful for user feedback.
Consider extracting the
applyDiffcallback to a separate function to improve readability:+const createApplyDiffCallback = ( + snapshot: any, + diffsPath: string, + logger: Logger, +) => + async (relativePath: string, generatedContents: Buffer | string) => { + const result = await applySnapshotToFileContents( + relativePath, + generatedContents, + snapshot, + diffsPath, + ); + if (result === false) { + logger.warn( + `Snapshot for ${relativePath} was not applied because the patch was invalid. Please verify the new output and run snapshot add once the changes have been verified.`, + ); + } + return result; + }; const overwriteOptions: OverwriteOptions = { enabled: !!overwrite, - applyDiff: - snapshot && - (async (relativePath, generatedContents) => { - const result = await applySnapshotToFileContents( - relativePath, - generatedContents, - snapshot, - resolvedSnapshotDirectory.diffsPath, - ); - if (result === false) { - logger.warn( - `Snapshot for ${relativePath} was not applied because the patch was invalid. Please verify the new output and run snapshot add once the changes have been verified.`, - ); - } - return result; - }), + applyDiff: snapshot && createApplyDiffCallback( + snapshot, + resolvedSnapshotDirectory.diffsPath, + logger, + ), skipFile: (relativePath) => { if (!ignorePatterns) { return false; } return ignorePatterns.ignores(relativePath); }, };
| const { applyDiff } = context.overwriteOptions; | ||
| const generatedContentsWithDiff = applyDiff | ||
| ? await applyDiff(relativePath, formattedContents) | ||
| : normalizeBufferString(formattedContents); | ||
| if (generatedContentsWithDiff) { | ||
| const previousWorkingBuffer = await previousWorkingCodebase?.readFile( | ||
| previousRelativePath ?? relativePath, | ||
| ); | ||
| if ( | ||
| previousWorkingBuffer?.equals( | ||
| normalizeBufferString(generatedContentsWithDiff), | ||
| ) | ||
| ) { | ||
| return { | ||
| relativePath, | ||
| mergedContents: undefined, | ||
| generatedContents: normalizeBufferString(formattedContents), | ||
| previousRelativePath, | ||
| }; | ||
| } | ||
| return { | ||
| relativePath, | ||
| mergedContents: normalizeBufferString(generatedContentsWithDiff), | ||
| generatedContents: normalizeBufferString(formattedContents), | ||
| previousRelativePath, | ||
| }; | ||
| } else if (generatedContentsWithDiff === undefined) { | ||
| // If the file was purposely deleted, we skip the generation | ||
| return { | ||
| relativePath, | ||
| mergedContents: undefined, | ||
| generatedContents: normalizeBufferString(formattedContents), | ||
| previousRelativePath, | ||
| }; | ||
| } | ||
| return { | ||
| relativePath, | ||
| mergedContents: normalizeBufferString(formattedContents), | ||
| generatedContents: normalizeBufferString(formattedContents), | ||
| previousRelativePath, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the applyDiff callback error handling and return value semantics.
The implementation correctly handles the applyDiff callback with proper conditional logic:
- Applies diff when callback is provided
- Handles
undefinedreturn value as intentional file deletion/skipping - Compares buffer contents to avoid unnecessary writes
However, ensure that the applyDiff callback implementations properly handle edge cases and return the expected types (Buffer | string | undefined).
Let me verify how applyDiff is implemented and used across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Verify applyDiff callback implementations and usage patterns
# Search for applyDiff implementations
echo "=== applyDiff implementations ==="
ast-grep --pattern 'applyDiff: $$$'
echo -e "\n=== applyDiff callback usage ==="
rg -A 10 "applyDiff.*=>" --type ts
echo -e "\n=== OverwriteOptions interface definition ==="
ast-grep --pattern 'interface OverwriteOptions {
$$$
}'Length of output: 2500
I dug into how applyDiff’s false return is handled and found that the current snippet only distinguishes “truthy” diffs vs. undefined (deletion), and there is no explicit branch for false. To properly fall back to a standard 3-way merge when applyDiff errors out (i.e. returns false), we need to add an else branch that treats false as a signal to skip the overwrite and perform the default merge.
Please update packages/sync/src/output/prepare-generator-files/prepare-generator-file.ts accordingly:
- After the
else if (generatedContentsWithDiff === undefined)block, add:} else if (generatedContentsWithDiff === false) { // Diff application failed—fallback to standard merge return { relativePath, mergedContents: normalizeBufferString(formattedContents), generatedContents: normalizeBufferString(formattedContents), previousRelativePath, }; }
- Ensure this sits just before the closing
}of theapplyDiffhandling.
This patch makes the applyDiff: false case explicit and aligns with the documented semantics of OverwriteOptions.
🤖 Prompt for AI Agents
In packages/sync/src/output/prepare-generator-files/prepare-generator-file.ts
between lines 355 and 390, the code currently handles truthy diffs and undefined
(deletion) from applyDiff but lacks explicit handling for a false return value.
Add an else-if branch after the existing else-if for undefined to check if
generatedContentsWithDiff is false, and in that case return an object with
mergedContents and generatedContents both set to
normalizeBufferString(formattedContents), along with relativePath and
previousRelativePath. This will explicitly handle the false case by falling back
to the standard merge as intended.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync/src/utils/create-generator.ts (1)
102-102: LGTM! Simplification improves code clarity.The removal of the
statSynccheck simplifies the logic appropriately. SincegeneratorFileUrlis documented as "The file URL of the generator", usingpath.dirname()directly is the correct approach.Consider removing the unused
statSyncimport if it's no longer needed elsewhere in the file:-import { statSync } from 'node:fs';
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sync/src/utils/create-generator.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Import UI components from the@baseplate-dev/ui-componentspackage as shown in the provided examples.
Use both standalone and React Hook Form controller variants for form components from@baseplate-dev/ui-componentsas appropriate.
If a particular interface or type is not exported, change the file so it is exported.
**/*.{ts,tsx}: TypeScript with strict type checking
Always include return types on top-level functions including React components (React.ReactElement)
Include absolute paths in import statements via tsconfig paths (@src/is the alias forsrc/)
If a particular interface or type is not exported, change the file so it is exported
Files:
packages/sync/src/utils/create-generator.ts
**/*.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/code-style.mdc)
Node 16 module resolution - include file extensions in imports (
.js)
Files:
packages/sync/src/utils/create-generator.ts
**/*.{ts,tsx,js}
📄 CodeRabbit Inference Engine (.cursor/rules/code-style.mdc)
**/*.{ts,tsx,js}: Sort imports by group: external libs first, then local imports
Use camelCase for variables/functions, PascalCase for types/classes
Order functions such that functions are placed below the variables/functions they use
We use the prefer using nullish coalescing operator (??) ESLint rule instead of a logical or (||), as it is a safer operator
Use console.info/warn/error instead of console.log
Files:
packages/sync/src/utils/create-generator.ts
**/*
📄 CodeRabbit Inference Engine (.cursor/rules/code-style.mdc)
Use kebab-case for file names
Files:
packages/sync/src/utils/create-generator.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:09:23.016Z
Learning: Run `pnpm lint:affected` and `pnpm typecheck` before committing changes.
Learnt from: kingston
PR: halfdomelabs/baseplate#539
File: scripts/check-changesets.ts:70-79
Timestamp: 2025-05-08T12:56:11.723Z
Learning: In the check-changesets.ts script for monorepo validation, `npm pack ${pkg.name}@latest` is intentionally used to pack the most recently published version (not local changes) for comparison purposes to determine if changes require new changesets.
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: .cursor/rules/dev-commands.mdc:0-0
Timestamp: 2025-07-22T09:11:00.767Z
Learning: Lint affected files with auto-fixing from the repository root using `pnpm lint:only:affected -- --fix`
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: .cursor/rules/dev-commands.mdc:0-0
Timestamp: 2025-07-22T09:11:00.767Z
Learning: Lint a package with auto-fixing using `pnpm lint --fix` from within the package directory
packages/sync/src/utils/create-generator.ts (16)
Learnt from: kingston
PR: #521
File: packages/react-generators/src/generators/admin/admin-components/admin-components.generator.ts:55-61
Timestamp: 2025-05-05T06:35:13.300Z
Learning: In generators, paths should be merged with forward slashes (/) only since JavaScript generated files use POSIX style separators. The system handles conversion to Windows style separators at later stages when writing out files.
Learnt from: kingston
PR: #539
File: scripts/check-changesets.ts:10-13
Timestamp: 2025-05-08T12:56:59.222Z
Learning: Node.js 20.12.0 and above include globSync in the core node:fs module, so import { promises as fs, globSync } from 'node:fs'; is valid syntax in projects using these Node.js versions.
Learnt from: kingston
PR: #539
File: scripts/check-changesets.ts:10-13
Timestamp: 2025-05-08T12:56:59.222Z
Learning: Node.js 22.0.0 and later versions include both glob and globSync functionality in the core node:fs module, making import { promises as fs, globSync } from 'node:fs'; valid syntax.
Learnt from: kingston
PR: #606
File: plugins/plugin-auth/src/auth/core/generators/auth-apollo/auth-apollo.generator.ts:24-32
Timestamp: 2025-07-12T19:56:08.559Z
Learning: For generator functions and configuration object methods like those in createGeneratorTask, inferred return types are acceptable when the structure is clear from the implementation. ESLint rules handle enforcement of explicit return types where truly needed, so manual review for this is not necessary.
Learnt from: kingston
PR: #613
File: packages/react-generators/src/generators/core/react-components/templates/components/ui/circular-progress/circular-progress.tsx:0-0
Timestamp: 2025-07-16T17:15:56.714Z
Learning: For imports starting with $ (template aliases), do not require .js extensions as these are resolved differently during the generation process, unlike regular file imports which require explicit .js extensions under Node 16 module resolution.
Learnt from: kingston
PR: #505
File: packages/create-project/tsconfig.json:6-6
Timestamp: 2025-04-21T06:32:22.476Z
Learning: Since TypeScript 4.1, baseUrl is not required for paths mapping in tsconfig.json. Removing baseUrl and using explicit relative paths with "./" prefix (e.g., changing "@src/": ["src/"] to "@src/": ["./src/"]) prevents bare path imports from node_modules while maintaining path alias functionality.
Learnt from: kingston
PR: #428
File: packages/project-builder-server/src/sync/index.ts:5-5
Timestamp: 2025-01-23T09:12:46.178Z
Learning: Avoid importing directly from 'dist' directories. Instead, expose functionality through the package's public API and import from the main package entry point.
Learnt from: kingston
PR: #505
File: packages/create-project/tsconfig.json:6-6
Timestamp: 2025-04-21T06:32:22.476Z
Learning: Since TypeScript 4.1, baseUrl is not required for paths mapping when using explicit relative paths (with "./"). Removing baseUrl from tsconfig.json while updating paths to use relative paths (e.g., changing "@src/": ["src/"] to "@src/": ["./src/"]) prevents bare path imports from node_modules while maintaining path alias functionality.
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: .cursor/rules/code-style.mdc:0-0
Timestamp: 2025-07-22T09:10:31.413Z
Learning: Applies to **/*.{js,ts,tsx} : Node 16 module resolution - include file extensions in imports (.js)
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:09:23.016Z
Learning: Applies to **/*.{ts,tsx} : If a particular interface or type is not exported, change the file so it is exported.
Learnt from: kingston
PR: #539
File: scripts/check-changesets.ts:111-124
Timestamp: 2025-05-08T12:56:23.394Z
Learning: Node.js 22.0.0 and later versions provide native glob functionality via fs.glob and fs.globSync methods in the node:fs module, allowing pattern matching without external dependencies.
Learnt from: kingston
PR: #510
File: packages/project-builder-server/src/sync/conflict-file-monitor.test.ts:19-24
Timestamp: 2025-04-23T06:44:30.952Z
Learning: When testing file operations in this codebase, the pattern is to use Vitest automocks for 'node:fs' and 'node:fs/promises' (without explicit implementation replacement) while populating a virtual filesystem with vol.fromJSON() from memfs. File operations in tests are performed directly via vol.promises methods.
Learnt from: kingston
PR: #510
File: packages/project-builder-server/src/sync/conflict-file-monitor.test.ts:19-24
Timestamp: 2025-04-23T06:44:30.952Z
Learning: In this codebase, when testing file operations, Vitest's automocks for 'node:fs' and 'node:fs/promises' are used in conjunction with memfs, but without replacing the mock implementation explicitly in each test file. The virtual filesystem is populated using vol.fromJSON or similar methods, which works seamlessly with the automocks.
Learnt from: kingston
PR: #539
File: scripts/check-changesets.ts:111-124
Timestamp: 2025-05-08T12:56:23.394Z
Learning: Node.js 22.0.0 and later versions provide native glob functionality via fs.glob method in the node:fs/promises module, allowing pattern matching without requiring external dependencies.
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-22T09:11:16.930Z
Learning: Applies to **/*.test.{ts,tsx} : For file system operations in tests, use memfs and mock 'node:fs' and 'node:fs/promises'
Learnt from: CR
PR: halfdomelabs/baseplate#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-06-30T11:52:28.745Z
Learning: Applies to **/*.test.{ts,tsx} : For file system operations in tests, use memfs and mock 'node:fs' and 'node:fs/promises' with vi.mock
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: Lint
- GitHub Check: Test E2E
kingston
deleted the
kingston/eng-827-upgrade-eslint-import-x-to-resolve-vscode-context-issues
branch
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Tests
Chores