Merged
Conversation
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an HTML validation feature powered by html-validate. Server-side: a Nitro plugin formats and validates rendered HTML, generates HtmlValidateReport entries, stores them, injects a serialized report into responses, and emits runtime hooks; GET and DELETE handlers plus SSE wiring expose reports. Client-side: Nuxt plugins consume embedded data and SSE; new Vue components (card, page, issue/result/message) display and dismiss reports. New types, routes, storage, utilities, feature flag, README docs, playground example, and dependencies (html-validate, devalue, prettier) were added. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
client/app/components/FeatureCards/HtmlValidateCard.vue (2)
5-13: Consider using theResulttype for better type safety.The inline type
{ messages: unknown[] }on line 8 loses the type information available fromhtml-validate. Using the importedResulttype would provide better IDE support and catch potential issues if thehtml-validateAPI changes.♻️ Suggested improvement
<script setup lang="ts"> import type { HtmlValidateReport } from '../../../../src/runtime/html-validate/types' +import type { Result } from 'html-validate' const htmlValidateReports = useNuxtApp().$htmlValidateReports const errorCount = computed(() => htmlValidateReports.value.reduce( - (sum: number, report: HtmlValidateReport) => sum + report.results.reduce( - (rSum: number, result: { messages: unknown[] }) => rSum + result.messages.length, + (sum: number, report: HtmlValidateReport) => sum + report.results.reduce<number>( + (rSum, result: Result) => rSum + result.messages.length, 0, ), 0, ), ) </script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/components/FeatureCards/HtmlValidateCard.vue` around lines 5 - 13, The inline type for the reduce callback in the computed `errorCount` is using `{ messages: unknown[] }`, losing type safety; replace that inline type with the `Result` type from `html-validate` (ensure `Result` is imported alongside `HtmlValidateReport`), and update the reducer signature in `errorCount` to use `result: Result` so message lengths are typed correctly and IDE/type-checking benefits are restored.
42-42: Minor: Consider handling singular/plural for "error(s)".The badge always displays "errors" even when
errorCountis 1. For better UX, consider using conditional pluralization.♻️ Suggested fix
- {{ errorCount }} errors + {{ errorCount }} {{ errorCount === 1 ? 'error' : 'errors' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/app/components/FeatureCards/HtmlValidateCard.vue` at line 42, The template in HtmlValidateCard.vue currently renders "{{ errorCount }} errors" unconditionally; add conditional pluralization by creating a computed property or method (e.g., errorLabel or formatErrorCount) that returns "1 error" when errorCount === 1 and "<n> errors" otherwise, then replace the direct interpolation with that property/method in the template; ensure you reference the existing errorCount prop/data in the computed method so the displayed label updates reactively.README.md (1)
91-95: Consider adding more detail to the documentation.The new section is consistent with the README structure, but it's notably shorter than other feature sections. Consider adding:
- A brief explanation of what types of HTML issues are detected (e.g., accessibility, semantics, standards compliance)
- How validation results are reported (console warnings, DevTools UI)
- Any configuration options if available
This would help users understand the feature's scope without needing to explore the DevTools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 91 - 95, Expand the "HTML Validate integration" section (title: "HTML Validate integration") to match other feature sections by describing what kinds of issues `html-validate` detects (example categories: accessibility, semantics, standards compliance, broken markup), explain how results are reported to users (e.g., console warnings, DevTools UI with screenshot referenced), and list available configuration options or show a minimal example config and link to the `html-validate` docs for advanced rules; keep the existing screenshot and add a short note about when validation runs ("on every server-rendered response") and how to enable/disable or customize the integration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/app/components/HtmlValidateIssue.vue`:
- Around line 25-28: The dismiss function currently emits the UI removal before
the backend DELETE completes and does not handle errors; change dismiss so it
first awaits the $fetch DELETE to `${HTML_VALIDATE_ROUTE}/${props.report.id}`
(using the existing HTML_VALIDATE_ROUTE and props.report.id), wrap the await in
try/catch, and only call emit('dismiss', props.report.id) after the request
succeeds; on error, handle/log the failure (e.g., process an error notification
or console.error) instead of removing the item optimistically.
In `@src/runtime/html-validate/handlers/get.ts`:
- Around line 5-9: getAllHandler currently calls
storage.getItem<HtmlValidateReport> for each key but does not handle that
getItem can return null (TOCTOU), so the returned array may contain nulls;
update the logic in getAllHandler to filter out null results after Promise.all
(use a type guard like (r): r is HtmlValidateReport => r !== null or
filter(Boolean)) so the handler returns an array of HtmlValidateReport only and
preserves correct typing; keep references to storage.getKeys and
storage.getItem<HtmlValidateReport> when making the change.
In `@src/runtime/html-validate/handlers/nitro-handlers.plugin.ts`:
- Around line 31-35: The SSE delete handler is pushing the raw id as event.data
but clients expect JSON; update the 'hints:html-validate:deleted' hook (the
callback passed to nitro.hooks.hook in nitro-handlers.plugin.ts) to
JSON.stringify the payload before pushing to context.eventStream so event.data
is a JSON string that the client can JSON.parse (keep the event name
'hints:html-validate:deleted' and only change the data value).
In `@src/runtime/html-validate/nitro.plugin.ts`:
- Around line 45-50: The async side-effects are not awaited: ensure the
containing function is async (or returns a Promise) and await the persistence
and hook calls so the report isn't emitted before the durable write and failures
are observable; specifically, await storage.setItem(id, data) before modifying
response.body (using addBeforeBodyEndTag and stringify) and then await
nitro.hooks.callHook('hints:html-validate:report', data) so both storage.setItem
and nitro.hooks.callHook are awaited and any errors propagate.
- Around line 32-35: The formatter/validator calls inside the
nitro.hooks.hook('render:response', async (response, { event }) => { ... }) can
throw and must not break the response flow; wrap the formatting and validation
steps (the const formattedBody = await format(...) and const results = await
validator.validateString(formattedBody) calls) in a try/catch, and on error log
the error (using your existing logger or console.error) including context (e.g.,
event.url) and then return/continue without modifying response.body so the hook
fails open.
In `@src/runtime/html-validate/plugin.client.ts`:
- Around line 9-10: The current client setup uses
document.getElementById('hints-html-validate') and calls
parse(document.getElementById('hints-html-validate')!.textContent!) with
non-null assertions and no error handling, which can throw and break
initialization; update the plugin.client.ts logic to first safely retrieve the
element and its textContent (guarding for null/empty), wrap the parse(...) call
in a try-catch, and return early or skip setup if parsing fails or the payload
is invalid for the expected HtmlValidateReport shape to prevent crashes during
client setup.
In `@src/runtime/types.d.ts`:
- Line 11: The import path for the HTML validation types is misspelled: change
all occurrences of import('./html-validator/types') to
import('./html-validate/types') so TypeScript can resolve the module; update the
three locations referencing this in the declaration file (e.g., the
__hints_html_validation? type and the other two imports around the symbols on
lines corresponding to the other HTML validation properties) to use
'./html-validate/types'.
---
Nitpick comments:
In `@client/app/components/FeatureCards/HtmlValidateCard.vue`:
- Around line 5-13: The inline type for the reduce callback in the computed
`errorCount` is using `{ messages: unknown[] }`, losing type safety; replace
that inline type with the `Result` type from `html-validate` (ensure `Result` is
imported alongside `HtmlValidateReport`), and update the reducer signature in
`errorCount` to use `result: Result` so message lengths are typed correctly and
IDE/type-checking benefits are restored.
- Line 42: The template in HtmlValidateCard.vue currently renders "{{ errorCount
}} errors" unconditionally; add conditional pluralization by creating a computed
property or method (e.g., errorLabel or formatErrorCount) that returns "1 error"
when errorCount === 1 and "<n> errors" otherwise, then replace the direct
interpolation with that property/method in the template; ensure you reference
the existing errorCount prop/data in the computed method so the displayed label
updates reactively.
In `@README.md`:
- Around line 91-95: Expand the "HTML Validate integration" section (title:
"HTML Validate integration") to match other feature sections by describing what
kinds of issues `html-validate` detects (example categories: accessibility,
semantics, standards compliance, broken markup), explain how results are
reported to users (e.g., console warnings, DevTools UI with screenshot
referenced), and list available configuration options or show a minimal example
config and link to the `html-validate` docs for advanced rules; keep the
existing screenshot and add a short note about when validation runs ("on every
server-rendered response") and how to enable/disable or customize the
integration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/assets/devtools-html-validate.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
README.mdclient/app/components/FeatureCards/HtmlValidateCard.vueclient/app/components/HtmlValidateIssue.vueclient/app/components/HtmlValidateMessage.vueclient/app/components/HtmlValidateResult.vueclient/app/pages/html-validate.vueclient/app/pages/index.vueclient/app/plugins/html-validate.tsclient/app/utils/routes.tspackage.jsonplayground/nuxt.config.tsplayground/pages/html-validation.vuesrc/module.tssrc/runtime/core/types.tssrc/runtime/html-validate/handlers/delete.tssrc/runtime/html-validate/handlers/get.tssrc/runtime/html-validate/handlers/nitro-handlers.plugin.tssrc/runtime/html-validate/nitro.plugin.tssrc/runtime/html-validate/plugin.client.tssrc/runtime/html-validate/storage.tssrc/runtime/html-validate/types.tssrc/runtime/html-validate/utils.tssrc/runtime/types.d.tstest/fixtures/basic/.nuxtrc
💤 Files with no reviewable changes (1)
- playground/nuxt.config.ts
Comment on lines
+31
to
+35
| nitro.hooks.hook('hints:html-validate:deleted', (id) => { | ||
| context.eventStream.push( | ||
| { | ||
| data: id, | ||
| event: 'hints:html-validate:deleted', |
There was a problem hiding this comment.
Use JSON-serialized payload for deleted SSE events.
Line 34 sends raw id, but the client expects JSON (it calls JSON.parse(event.data)), so delete events fail to apply.
Suggested fix
nitro.hooks.hook('hints:html-validate:deleted', (id) => {
context.eventStream.push(
{
- data: id,
+ data: JSON.stringify(id),
event: 'hints:html-validate:deleted',
},
)
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| nitro.hooks.hook('hints:html-validate:deleted', (id) => { | |
| context.eventStream.push( | |
| { | |
| data: id, | |
| event: 'hints:html-validate:deleted', | |
| nitro.hooks.hook('hints:html-validate:deleted', (id) => { | |
| context.eventStream.push( | |
| { | |
| data: JSON.stringify(id), | |
| event: 'hints:html-validate:deleted', | |
| }, | |
| ) | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/html-validate/handlers/nitro-handlers.plugin.ts` around lines 31
- 35, The SSE delete handler is pushing the raw id as event.data but clients
expect JSON; update the 'hints:html-validate:deleted' hook (the callback passed
to nitro.hooks.hook in nitro-handlers.plugin.ts) to JSON.stringify the payload
before pushing to context.eventStream so event.data is a JSON string that the
client can JSON.parse (keep the event name 'hints:html-validate:deleted' and
only change the data value).
Comment on lines
+32
to
+35
| nitro.hooks.hook('render:response', async (response, { event }) => { | ||
| if (typeof response.body === 'string' && (response.headers?.['Content-Type'] || response.headers?.['content-type'])?.includes('html')) { | ||
| const formattedBody = await format(response.body, { plugins: [html], parser: 'html' }) | ||
| const results = await validator.validateString(formattedBody) |
There was a problem hiding this comment.
Fail open on formatter/validator errors in render:response.
Lines 34–35 can throw; without guarding, a tooling failure can impact page response flow.
Suggested fix
nitro.hooks.hook('render:response', async (response, { event }) => {
if (typeof response.body === 'string' && (response.headers?.['Content-Type'] || response.headers?.['content-type'])?.includes('html')) {
- const formattedBody = await format(response.body, { plugins: [html], parser: 'html' })
- const results = await validator.validateString(formattedBody)
+ let formattedBody: string
+ let results: Awaited<ReturnType<typeof validator.validateString>>
+ try {
+ formattedBody = await format(response.body, { plugins: [html], parser: 'html' })
+ results = await validator.validateString(formattedBody)
+ }
+ catch (error) {
+ // keep response path resilient if tooling fails
+ return
+ }
if (response.body && results.errorCount > 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| nitro.hooks.hook('render:response', async (response, { event }) => { | |
| if (typeof response.body === 'string' && (response.headers?.['Content-Type'] || response.headers?.['content-type'])?.includes('html')) { | |
| const formattedBody = await format(response.body, { plugins: [html], parser: 'html' }) | |
| const results = await validator.validateString(formattedBody) | |
| nitro.hooks.hook('render:response', async (response, { event }) => { | |
| if (typeof response.body === 'string' && (response.headers?.['Content-Type'] || response.headers?.['content-type'])?.includes('html')) { | |
| let formattedBody: string | |
| let results: Awaited<ReturnType<typeof validator.validateString>> | |
| try { | |
| formattedBody = await format(response.body, { plugins: [html], parser: 'html' }) | |
| results = await validator.validateString(formattedBody) | |
| } | |
| catch (error) { | |
| // keep response path resilient if tooling fails | |
| return | |
| } | |
| if (response.body && results.errorCount > 0) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/html-validate/nitro.plugin.ts` around lines 32 - 35, The
formatter/validator calls inside the nitro.hooks.hook('render:response', async
(response, { event }) => { ... }) can throw and must not break the response
flow; wrap the formatting and validation steps (the const formattedBody = await
format(...) and const results = await validator.validateString(formattedBody)
calls) in a try/catch, and on error log the error (using your existing logger or
console.error) including context (e.g., event.url) and then return/continue
without modifying response.body so the hook fails open.
Comment on lines
+45
to
+50
| storage.setItem(id, data) | ||
| response.body = addBeforeBodyEndTag( | ||
| response.body, | ||
| `<script id="hints-html-validate" type="application/json">${stringify(data)}</script>`, | ||
| ) | ||
| nitro.hooks.callHook('hints:html-validate:report', data) |
There was a problem hiding this comment.
Await persistence and hook emission to avoid racey report state.
On Line 45 and Line 50, async side-effects are not awaited. This can cause dropped/unobserved failures and timing issues (report emitted before durable store write).
Suggested fix
- storage.setItem(id, data)
+ await storage.setItem(id, data)
response.body = addBeforeBodyEndTag(
response.body,
`<script id="hints-html-validate" type="application/json">${stringify(data)}</script>`,
)
- nitro.hooks.callHook('hints:html-validate:report', data)
+ await nitro.hooks.callHook('hints:html-validate:report', data)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| storage.setItem(id, data) | |
| response.body = addBeforeBodyEndTag( | |
| response.body, | |
| `<script id="hints-html-validate" type="application/json">${stringify(data)}</script>`, | |
| ) | |
| nitro.hooks.callHook('hints:html-validate:report', data) | |
| await storage.setItem(id, data) | |
| response.body = addBeforeBodyEndTag( | |
| response.body, | |
| `<script id="hints-html-validate" type="application/json">${stringify(data)}</script>`, | |
| ) | |
| await nitro.hooks.callHook('hints:html-validate:report', data) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/html-validate/nitro.plugin.ts` around lines 45 - 50, The async
side-effects are not awaited: ensure the containing function is async (or
returns a Promise) and await the persistence and hook calls so the report isn't
emitted before the durable write and failures are observable; specifically,
await storage.setItem(id, data) before modifying response.body (using
addBeforeBodyEndTag and stringify) and then await
nitro.hooks.callHook('hints:html-validate:report', data) so both storage.setItem
and nitro.hooks.callHook are awaited and any errors propagate.
Member
|
do you think we can fully deprecate |
Member
Author
|
Yes I'm thinking of doing it after 1.0 release |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
📚 Description
html-validate integration