Skip to content

chore: add e18e lint plugin - #1019

Merged
43081j merged 9 commits into
mainfrom
jg/lint-perf
Feb 5, 2026
Merged

chore: add e18e lint plugin#1019
43081j merged 9 commits into
mainfrom
jg/lint-perf

Conversation

@43081j

Copy link
Copy Markdown
Contributor

This also disables no-console since pretty much everything in this
repo can run server-side, where console logs are valuable when done
right.

@vercel

vercelBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
npmx.devReadyReadyPreview, CommentFeb 5, 2026 2:47pm
2 Skipped Deployments
ProjectDeploymentActionsUpdated (UTC)
docs.npmx.devIgnoredIgnoredPreviewFeb 5, 2026 2:47pm
npmx-lunariaIgnoredIgnoredFeb 5, 2026 2:47pm

Request Review

@codecov

codecovBot commented Feb 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@coderabbitai

coderabbitaiBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds @e18e/eslint-plugin as a devDependency and updates .oxlintrc.json to register several e18e/* rules as error and disable no-console for specific file groups. Removes scripts/lint.ts and updates CI: install root deps (no scripts) and run pnpm lint. Small code edits: replace spread+map with Array.from(resolved.values(), ...) in server/utils/dependency-analysis.ts; extract a top-level prefixId(tagName, attribs) used by sanitizeHtml in server/utils/readme.ts; switch /readme/i.test(pathname) in test/e2e/test-utils.ts; add @e18e/eslint-plugin to knip.ts ignore list; mutate authors via Object.assign in server/api/atproto/author-profiles.get.ts.

Possibly related PRs

🚥 Pre-merge checks | ❌ 1
❌ Failed checks (1 inconclusive)
Check nameStatusExplanationResolution
Description check❓ InconclusiveThe PR description addresses one key aspect of the changeset—disabling no-console—but omits several significant changes including adding the e18e lint plugin, refactoring dependency analysis code, and restructuring linting infrastructure.Expand the description to cover the full scope of changes, particularly the addition of the e18e lint plugin and the refactoring of the linting workflow infrastructure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch jg/lint-perf

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

This also disables `no-console` since pretty much everything in this
repo can run server-side, where console logs are valuable when done
right.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
server/utils/readme.ts (1)

448-452: ⚠️ Potential issue | 🟡 Minor

The section and article transforms are ineffective.

The prefixId transform is applied to section and article tags, but these tags are not included in ALLOWED_TAGS (lines 102-139). Since sanitize-html strips tags not in the allowed list, these transforms will never execute.

Either add section and article to ALLOWED_TAGS if they should be permitted, or remove them from transformTags to avoid confusion.

🔧 Option A: Remove ineffective transforms
 div: prefixId,
p: prefixId,
span: prefixId,
- section: prefixId,- article: prefixId,
},
🔧 Option B: Add tags to ALLOWED_TAGS (around line 139)
 'button',
+ 'section',+ 'article',
]
🧹 Nitpick comments (2)
.oxlintrc.json (1)

22-22: Consider removing the empty overrides array.

The empty overrides array serves no functional purpose. If it's a placeholder for future configuration, consider adding a comment to clarify intent; otherwise, it can be removed to reduce noise.

🧹 Suggested removal
- "overrides": [],
scripts/lint.ts (1)

14-16: Pre-existing issue: error message is inconsistent with actual command.

The error message references pnpm info ${dependencyName} version, but the actual command executed is npm pkg get devDependencies.${dependencyName}. Consider updating the message for accurate debugging.

📝 Suggested fix
 if (result.status) {
- throw new Error(`Command failed: pnpm info ${dependencyName} version`)+ throw new Error(`Command failed: npm pkg get devDependencies.${dependencyName}`)
}

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/lint.ts (1)

19-21: ⚠️ Potential issue | 🟡 Minor

Error message does not match the actual command.

The error message references pnpm info but the command executed on line 15 is npm pkg get. This would be misleading when debugging failures.

🐛 Proposed fix
 if (result.status) {
- throw new Error(`Command failed: pnpm info ${dependencyName} version`)+ throw new Error(`Command failed: npm pkg get devDependencies.${dependencyName}`)
}

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
scripts/lint.ts (1)

19-21: ⚠️ Potential issue | 🟡 Minor

Error message does not match the actual command.

The error message references pnpm info but the command being executed is npm pkg get. This will cause confusion when debugging failures.

🐛 Proposed fix
 if (result.status) {
- throw new Error(`Command failed: pnpm info ${dependencyName} version`)+ throw new Error(`Command failed: npm pkg get devDependencies.${dependencyName}`)
}

Comment threadscripts/lint.ts Outdated
Comment thread.github/workflows/ci.yml
Comment thread.oxlintrc.json Outdated
},
"rules": {
"no-console": "warn",
"no-console": "off",

@serhalpserhalpFeb 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I never understood the purpose of warns, so I'm good with this 😁. Longer term should we be using a li'l logger module and turning this to "error"? (maybe keep off for scripts/)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

we really need to review logging overall.

only truly error-level things should be logged at error level, for example. this isn't currently the case iirc

i'll start a discussion around this 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would it be possible to disable this just on a per-directory case? so we disable it for server/ and cli/ but keep it for app/ ?

@serhalpserhalp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@43081j

Copy link
Copy Markdown
ContributorAuthor

FYI this does mean the lint job can often take longer now (if it is the first to populate the cache)

but having the workaround lint.ts was causing a lot of friction and means we can't use lint plugins. we should revisit how we can speed it back up

@danielroe

Copy link
Copy Markdown
Member

@43081j are you happy with those small tweaks?

@43081j

Copy link
Copy Markdown
ContributorAuthor

looks good to me 👍

@43081j
43081j added this pull request to the merge queueFeb 5, 2026
Merged via the queue into main with commit 18bcb8aFeb 5, 2026
20 checks passed
@43081j
43081j deleted the jg/lint-perf branch February 5, 2026 14:56
@coderabbitaicoderabbitaiBot mentioned this pull request Feb 7, 2026
Sign up for freeto 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.

3 participants

@43081j@danielroe@serhalp