feat: Add Checklist component to make our checklists clickable#498
Open
scode2277 wants to merge 4 commits into
Open
feat: Add Checklist component to make our checklists clickable#498scode2277 wants to merge 4 commits into
scode2277 wants to merge 4 commits into
Conversation
Deploying frameworks with
|
| Latest commit: |
5a83ab1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82b81dce.frameworks-573.pages.dev |
| Branch Preview URL: | https://chore-checklist-component.frameworks-573.pages.dev |
frameworks-volunteer
suggested changes
May 25, 2026
Collaborator
frameworks-volunteer
left a comment
There was a problem hiding this comment.
Model: z-ai/glm-5.1 Reasoning: medium Provider: openrouter
Security Review
- No hardcoded secrets, tokens, or API keys. localStorage keys use a safe pattern (
checklist:${id}:${slugify}) whereidis page-controlled andslugifystrips to[a-z0-9-]. - No XSS: Component uses
document.createElement/insertAdjacentElement-- neverinnerHTMLor untrusted HTML injection. - No eval, no unsafe deserialization, no path traversal.
- localStorage is wrapped in try/catch, handling QuotaExceededError and privacy mode. Only stores "true"/"false" booleans.
- No server interaction or auth concerns -- purely client-side UI.
QA Review
- Changes match PR description: new Checklist component, MDX wrapping, template guidance, GovernanceChecklistItem rename.
- BLOCKING: Unsigned commits. All 3 PR commits (047a750, 9517c4a, db0d88b) are unsigned. The repo requires
git commit -Sper AGENTS.md. Please rebase and re-sign. - CSS
:has()selector: Used inChecklist.csslines 24/32. Supported in all major browsers since late 2023 (Firefox 121+, Chrome 105+, Safari 15.4+). Acceptable for this audience, but older Firefox users will see missing bullets on non-checkbox items. - Template inconsistency: Commit db0d88b removes
Checklistfrom the template import (due to a JSX block-comment build error), but the guidance in 9517c4a says to add it. The template no longer demonstrates the import. Consider adding a separate non-JSX-comment note or example showing the Checklist import. - Duplicate slug collision: If two items under the same
<Checklist id>have identical text, their localStorage keys collide and share state. Consider appending the item's index toslugifyoutput, or documenting that item text must be unique within a block. - Blockquote-to-list-item conversions in telegram.mdx are correct per the template guidance.
- No leftover debug content, TODOs, or placeholder text.
Verdict
Requesting changes due to unsigned commits (blocking). Once signed, the CSS/template issues above are minor and can be addressed in a follow-up or at author's discretion.
Collaborator
|
@frameworks-volunteer review it again, i see everything ready to be merged, commits signed and everything... |
frameworks-volunteer
approved these changes
May 25, 2026
Collaborator
frameworks-volunteer
left a comment
There was a problem hiding this comment.
Model: z-ai/glm-5.1 Reasoning: medium Provider: openrouter
Re-review — PR #498
Previous review requested changes due to unsigned commits. All 3 commits now verified (reason: valid). CI checks pass (lint, sidebar-reminder, Cloudflare Pages).
Previously raised items
- Unsigned commits (blocking) — resolved. All commits verified.
- CSS
:has()selector — acceptable. Firefox 121+ (Dec 2023) has full support. - Template guidance — the JSX comment now explains when/how to add
Checklistto the import. The template itself doesn't use checklists, so omitting it from the actual import line is correct. - Duplicate slug collision — still present: two items with identical text under the same
<Checklist id>share a localStorage key. Low-risk (requires identical item text in the same block) and can be addressed in a follow-up if it surfaces in practice.
Current assessment
- Security: no issues. localStorage keys are namespace-scoped, no XSS vectors, no innerHTML, no eval.
- QA: changes match the PR description. 30 files touched, all checklist blocks wrapped with unique
idattributes.GovernanceChecklistItemrename is consistent across exports and usages. - CI: green.
Approving — the blocking issue is resolved and the remaining items are minor/non-blocking.
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
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.
I've created a component that makes our checklists actually clickable as throughout our content we have a lot of them and our contributors usually use them a lot. Also added it to our contributing guidelines + renamed the governance component to avoid confusion.
Check it out here -> https://chore-checklist-component.frameworks-573.pages.dev/guides/account-management/github
Frameworks PR Checklist
Thank you for contributing to the Security Frameworks! Before you open a PR, make sure to read information for contributors and take a look at the following checklist:
vocs.config.tsadding thedev: trueparameter