Skip to content

Conversation

@jamesbhobbs
Copy link
Contributor

@jamesbhobbs jamesbhobbs commented Oct 10, 2025

Summary

This PR adds lint-staged configuration with pre-commit hooks to standardize code quality tooling across Deepnote repositories, matching the setup from jupyterlab-deepnote PR #18.

Changes

Dependencies

  • Upgraded husky from ^8.0.3 to 9.1.7 (exact version for consistency)
  • Added lint-staged 16.2.3 as dev dependency
  • Added prepare script "prepare": "husky" for husky setup

Hook Configuration

  • Updated .husky/pre-commit: Replaced manual prettier logic with npm exec lint-staged
  • Added .husky/post-checkout: Runs npm install after checkout
  • Added .husky/pre-push: Prevents direct pushes to main branch

Lint-staged Configuration

"lint-staged": {
  "src/**/*.{ts,tsx}": [
    "eslint --cache --fix", 
    "prettier --write"
  ]
}

⚠️ Important Review Points

  1. File Pattern Change: Old pre-commit hook checked all .ts files in commit, new config targets src/**/*.{ts,tsx}. Verify this covers the intended scope.

  2. Husky Major Version Upgrade: Upgraded from 8.0.3 to 9.1.7. Test that hooks execute correctly with new version.

  3. Behavioral Change: Replaced custom prettier-checking logic with lint-staged. The old hook had fallback logic when prettier wasn't installed - verify lint-staged handles this appropriately.

  4. Local Testing: Could not verify locally due to GitHub Package Registry authentication issues with @deepnote/blocks dependency. Please test manually after CI passes.

Testing Checklist

  • Pre-commit hook runs on staged src/**/*.{ts,tsx} files
  • Post-checkout hook runs npm install correctly
  • Pre-push hook prevents main branch pushes
  • ESLint and Prettier execute via lint-staged
  • No regressions in existing workflow

Link to Devin run: https://app.devin.ai/sessions/145f89c8cbc942d2970c5bc43a90149c
Requested by: James Hobbs (@jamesbhobbs)

Summary by CodeRabbit

  • Chores
    • Updated development dependencies and enhanced automated code quality checks
    • Added safeguard to prevent direct pushes to the main branch

- Upgrade husky from 8.0.3 to 9.1.7
- Add lint-staged 16.2.3 for automated code quality checks
- Configure lint-staged to run eslint and prettier on src/**/*.{ts,tsx}
- Update pre-commit hook to use lint-staged
- Add post-checkout hook for automatic dependency installation
- Add pre-push hook to prevent direct pushes to main branch

This matches the setup in jupyterlab-deepnote for consistency.
@devin-ai-integration
Copy link

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

📝 Walkthrough

Walkthrough

Sets up Git hooks via Husky 9.1.7 to automate development workflows. The post-checkout hook runs npm ci to install dependencies deterministically after checkout. The pre-commit hook delegates to npm exec lint-staged for linting and formatting TypeScript files. The pre-push hook prevents direct pushes to the main branch. The prepare script initializes Husky, and .gitignore is expanded to exclude build artifacts, caches, and test-related files.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Git as Git
    participant Husky as Husky Hooks
    participant LintStaged as lint-staged
    participant Tools as ESLint/Prettier

    Dev->>Git: git checkout
    Git->>Husky: post-checkout
    Husky->>Husky: npm ci
    Husky-->>Dev: ✓ Dependencies installed

    Dev->>Git: git commit
    Git->>Husky: pre-commit
    Husky->>LintStaged: npm exec lint-staged
    LintStaged->>Tools: Run on changed *.ts/*.tsx files
    Tools-->>Git: ✓ Lint/format check
    Git-->>Dev: ✓ Commit allowed

    Dev->>Git: git push (to main)
    Git->>Husky: pre-push
    Husky->>Husky: Check branch == "main"
    Husky-->>Dev: ✗ Push blocked on main
Loading

Possibly related PRs

Suggested reviewers

  • andyjakubowski
  • saltenasl

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "chore: add lint-staged with pre-commit hooks for src/ directory" accurately captures the primary purpose of the PR: establishing lint-staged configuration and integrating it with Husky's pre-commit hook to run ESLint and Prettier on staged files in the src/ directory. The title is specific and clear about the main change, distinguishing it from generic phrasings. While the PR also includes auxiliary hooks (post-checkout and pre-push) and .gitignore updates, these are supporting infrastructure rather than the core objective. The title appropriately highlights the central functionality being added.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3143b2a and 8ec10fe.

📒 Files selected for processing (1)
  • .gitignore (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jamesbhobbs
PR: deepnote/deepnote#1
File: .husky/post-checkout:1-1
Timestamp: 2025-09-30T13:34:45.847Z
Learning: In the deepnote/deepnote repository, the .husky/post-checkout hook should remain minimal (just `pnpm install`) without explicit flags like `--frozen-lockfile` or guards. Explicit flags should only be used where needed, such as in CI workflows.
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Pre-commit: run `npm run format-fix`, `npm run lint`, and `npm run test:unittests` before committing
🔇 Additional comments (1)
.gitignore (1)

1-75: Verify .qlty/* patterns align with project tooling.

The reorganization adds .eslintcache (line 6)—good, necessary for ESLint caching during lint-staged runs. However, the .qlty/* patterns (lines 12–16) suggest integration with a Qlty code-quality tool that isn't mentioned in the PR objectives. Confirm these additions are in scope for this repository.

Also, .husky/ is correctly NOT ignored, so Husky hooks will be committed.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f59e68f and b54bd1c.

📒 Files selected for processing (4)
  • .husky/post-checkout (1 hunks)
  • .husky/pre-commit (1 hunks)
  • .husky/pre-push (1 hunks)
  • package.json (3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: jamesbhobbs
PR: deepnote/deepnote#1
File: .husky/post-checkout:1-1
Timestamp: 2025-09-30T13:34:45.847Z
Learning: In the deepnote/deepnote repository, the .husky/post-checkout hook should remain minimal (just `pnpm install`) without explicit flags like `--frozen-lockfile` or guards. Explicit flags should only be used where needed, such as in CI workflows.
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Pre-commit: run `npm run format-fix`, `npm run lint`, and `npm run test:unittests` before committing
📚 Learning: 2025-09-30T14:09:47.867Z
Learnt from: jamesbhobbs
PR: deepnote/deepnote#1
File: .husky/pre-push:1-5
Timestamp: 2025-09-30T14:09:47.867Z
Learning: In .husky/pre-push, the simpler branch name check (`git symbolic-ref --short HEAD`) is preferred over parsing stdin for remote refs, as the goal is preventing accidental manual errors rather than enforcing strict security. Admins can bypass it when needed.

Applied to files:

  • .husky/pre-push
📚 Learning: 2025-09-03T13:01:49.096Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Use `npm run format` to check code style

Applied to files:

  • .husky/pre-commit
📚 Learning: 2025-09-03T12:53:28.421Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-03T12:53:28.421Z
Learning: Run `npm run format-fix` before committing any changes

Applied to files:

  • .husky/pre-commit
📚 Learning: 2025-09-03T13:01:49.096Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: ALWAYS run `npm run format-fix` before committing changes

Applied to files:

  • .husky/pre-commit
📚 Learning: 2025-09-03T13:01:49.096Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Pre-commit: run `npm run format-fix`, `npm run lint`, and `npm run test:unittests` before committing

Applied to files:

  • .husky/pre-commit
📚 Learning: 2025-09-30T13:34:45.847Z
Learnt from: jamesbhobbs
PR: deepnote/deepnote#1
File: .husky/post-checkout:1-1
Timestamp: 2025-09-30T13:34:45.847Z
Learning: In the deepnote/deepnote repository, the .husky/post-checkout hook should remain minimal (just `pnpm install`) without explicit flags like `--frozen-lockfile` or guards. Explicit flags should only be used where needed, such as in CI workflows.

Applied to files:

  • .husky/post-checkout
📚 Learning: 2025-09-04T15:02:20.363Z
Learnt from: jamesbhobbs
PR: deepnote/deepnote#18363
File: .husky/post-checkout:1-1
Timestamp: 2025-09-04T15:02:20.363Z
Learning: pnpm has built-in checking that detects when dependencies need to be installed versus when they're already up to date, making manual package.json/pnpm-lock.yaml diff checking redundant in Husky hooks.

Applied to files:

  • .husky/post-checkout
📚 Learning: 2025-09-03T13:01:49.096Z
Learnt from: CR
PR: deepnote/vscode-extension#0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-09-03T13:01:49.096Z
Learning: Run `npm install` if `package.json` was changed

Applied to files:

  • .husky/post-checkout
🪛 GitHub Actions: CI
package.json

[error] 1-1: Command failed: npm ci --prefer-offline --no-audit. The lockfile is out of sync with package.json. Please run 'npm install' to update the lockfile and commit it.

🔇 Additional comments (4)
package.json (1)

2295-2300: Verify pattern coverage and pre-commit scope.

The lint-staged config targets only src/**/*.{ts,tsx}, excluding:

  • Build scripts in build/
  • Test files outside src/
  • Root-level .ts and .js files
  • Non-TypeScript files

Retrieved learnings indicate pre-commit should run npm run format-fix, npm run lint, and npm run test:unittests, but lint-staged only runs eslint and prettier on the restricted pattern. This may miss critical files and skip unit tests entirely.

Confirm this narrower scope is intentional and won't introduce regressions.

Based on learnings.

.husky/post-checkout (1)

1-1: LGTM—minimal approach aligns with best practices.

Runs npm install without extra flags, letting npm's built-in checks handle dependencies. Note: Retrieved learning mentions pnpm install for deepnote/deepnote, but npm appears correct for this vscode-extension repo.

Based on learnings.

.husky/pre-push (1)

1-5: LGTM—simple and effective guard.

Uses git symbolic-ref --short HEAD to prevent accidental pushes to main. Aligns with learning preference for simpler branch checks over stdin parsing.

Based on learnings.

.husky/pre-commit (1)

1-1: Verify divergence from pre-commit best practices.

Retrieved learnings state: "Pre-commit: run npm run format-fix, npm run lint, and npm run test:unittests before committing."

Current implementation only runs lint-staged (eslint + prettier on src/**/*.{ts,tsx}), omitting:

  • Unit tests (npm run test:unittests)
  • Full lint/format runs on non-src files

Confirm this reduced scope is intentional. If unit tests are now skipped pre-commit, document the rationale.

Based on learnings.

Update package-lock.json to match the package.json changes made in the previous commit.
This resolves the npm ci failures in CI where the lock file was out of sync with package.json.

All verification steps passed locally:
- npm run lint ✓
- npm run format ✓
- npm run typecheck ✓
- npm run compile ✓
- npm test ✓ (1329 passing)
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71%. Comparing base (f5cec10) to head (8ec10fe).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #38   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        520     520           
  Lines      38865   38865           
  Branches    4895    4895           
=====================================
  Hits       27705   27705           
  Misses      9537    9537           
  Partials    1623    1623           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 14, 2025
@jamesbhobbs jamesbhobbs marked this pull request as ready for review October 14, 2025 15:31
@jamesbhobbs jamesbhobbs enabled auto-merge (squash) October 14, 2025 15:35
Co-authored-by: Christoffer Artmann <Artgaard@gmail.com>
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 16, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 16, 2025
Copy link
Member

@saltenasl saltenasl left a comment

Choose a reason for hiding this comment

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

you used npm install which broke package-lock.json file, this repo uses npm ci

Artmann
Artmann previously approved these changes Oct 17, 2025
@Artmann Artmann dismissed stale reviews from coderabbitai[bot] and themself via 3143b2a October 17, 2025 10:09
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
package.json (1)

2132-2132: Fix prepare script for Husky v9.

Line 2132 still references "prepare": "husky" instead of "prepare": "husky install". For Husky v9 compatibility, this must be corrected.

-        "prepare": "husky",
+        "prepare": "husky install",

Also applies to: 2132-2132

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9732da2 and 3143b2a.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • .eslintcache (1 hunks)
  • package.json (3 hunks)
⏰ 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). (2)
  • GitHub Check: Build & Test
  • GitHub Check: copilot-setup-steps
🔇 Additional comments (2)
package.json (2)

2329-2329: Verify Husky v9 setup & hook execution.

Major version upgrade from ^8.0.3 to 9.1.7. Confirm that after applying the prepare script fix, npm run prepare installs hooks correctly and all .husky/ files remain executable. Test pre-commit, post-checkout, and pre-push hooks locally.

Also applies to: 2329-2329


2364-2369: Lint-staged config looks good.

File pattern src/**/*.{ts,tsx} correctly targets TypeScript files. ESLint runs with --cache (perf boost), then Prettier. Aligns with PR scope narrowing from prior broader linting.

@jamesbhobbs jamesbhobbs requested a review from Artmann October 17, 2025 10:43
@jamesbhobbs jamesbhobbs disabled auto-merge October 17, 2025 10:50
@jamesbhobbs jamesbhobbs merged commit 146cda4 into main Oct 17, 2025
10 checks passed
@jamesbhobbs jamesbhobbs deleted the devin/1760108720-add-lint-staged branch October 17, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants