Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This also disables `no-console` since pretty much everything in this repo can run server-side, where console logs are valuable when done right.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 | 🟡 MinorThe
sectionandarticletransforms are ineffective.The
prefixIdtransform is applied tosectionandarticletags, but these tags are not included inALLOWED_TAGS(lines 102-139). Sincesanitize-htmlstrips tags not in the allowed list, these transforms will never execute.Either add
sectionandarticletoALLOWED_TAGSif they should be permitted, or remove them fromtransformTagsto 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 emptyoverridesarray.The empty
overridesarray 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 isnpm 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}`) }
There was a problem hiding this comment.
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 | 🟡 MinorError message does not match the actual command.
The error message references
pnpm infobut the command executed on line 15 isnpm 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}`) }
There was a problem hiding this comment.
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 | 🟡 MinorError message does not match the actual command.
The error message references
pnpm infobut the command being executed isnpm 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}`) }
.oxlintrc.json
Outdated
| }, | ||
| "rules": { | ||
| "no-console": "warn", | ||
| "no-console": "off", |
There was a problem hiding this comment.
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/)
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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/ ?
|
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 |
|
@43081j are you happy with those small tweaks? |
|
looks good to me 👍 |
3 participants
This also disables
no-consolesince pretty much everything in thisrepo can run server-side, where console logs are valuable when done
right.