[WEB-4178] HTML notice fixes #612
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several modifications across multiple files in the core components. The changes primarily focus on type refinements, HTML content sanitization, and the introduction of a new Rails UJS links hook. The package version has been updated to a development version, signaling ongoing work. Key modifications include updating type definitions in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/core/hooks/use-rails-ujs-hooks.ts (2)
5-11: Consider implementing the data-confirm attribute handling.The
RailsUjsLinkinterface includes aconfirmproperty in the dataset, but the implementation doesn't handle confirmation dialogs.const handleClick = (event: MouseEvent): void => { const target = event.target as HTMLElement; const link = target.closest<RailsUjsLink>("a[data-method]"); if (!link) return; + + if (link.dataset.confirm && !window.confirm(link.dataset.confirm)) { + return; + } // Check if the clicked link is within this component's container
43-44: Consider supporting additional HTTP methods.The comment suggests future expansion, but currently only POST and DELETE methods are supported. Consider adding support for PUT and PATCH methods as they are common in REST APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(1 hunks)src/core/Flash.tsx(3 hunks)src/core/Meganav.tsx(2 hunks)src/core/Notice.tsx(4 hunks)src/core/hooks/use-rails-ujs-hooks.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/core/Notice.tsx
[error] 89-89: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (3)
src/core/Meganav.tsx (1)
86-93: LGTM! Well-structured type definition.The new
NoticeApiPropstype provides better type safety and clearer structure for notice configuration.src/core/Flash.tsx (2)
20-20: LGTM! Good use of Pick utility type.Using
Pickto restrict theitemsarray to only include necessary properties improves type safety.
207-207: Verify the removal of removeFlash prop.The changes suggest that
removeFlashis being added to the flash items array but then not used in the Flash component render. This could lead to memory leaks if the removal functionality is broken.Also applies to: 218-218
kennethkalmer
force-pushed
the
WEB-4178-html-notice-fixes
branch
from
177455e to
ba000bc
Compare
kennethkalmer
force-pushed
the
WEB-4178-html-notice-fixes
branch
from
7407f6e to
deeb09c
Compare
Jira Ticket Link / Motivation
A bit of TLC and some fixes for rendering HTML notices from the website's notice API. This is a relatively unused feature, will mostly be triggered by staff members, but still.
This depends on a companion PR in the website that will inject the CSRF tokens into the Voltaire responses so that we have these meta tags available everywhere we use notices (just like Rails does).
It also adds some additional smarts to the notice rendering to be able to handle Rails UJS-style markup, which we do return from the notice API.
Summary of changes
This pull request includes several changes to improve type safety, sanitize HTML content, and refactor the codebase. The most important changes include updating the
package.jsonversion, refining theFlashcomponent, sanitizing theNoticecomponent, and adding a custom hook for handling Rails UJS links.Version Update:
package.json: Updated the version from15.3.0to15.3.1-dev.b451a94.Flash Component Refinement:
src/core/Flash.tsx: UpdatedFlashesPropsto usePick<FlashProps, "type" | "content">[]and removed theremoveFlashprop from theFlashcomponent. [1] [2] [3]Notice Component Sanitization:
src/core/Notice.tsx: AddedDOMPurifyto sanitizebodyTextand useddangerouslySetInnerHTMLto render sanitized content. Introduced a custom hookuseRailsUjsLinksfor handling Rails UJS links. [1] [2] [3] [4]Custom Hook Addition:
src/core/hooks/use-rails-ujs-hooks.ts: Created a new custom hookuseRailsUjsLinksto manage Rails UJS links, including CSRF token handling and form submission for POST and DELETE methods.Meganav Component Refactoring:
src/core/Meganav.tsx: RefactoredMeganavPropsand related types, and added type import forNoticeProps. Removed unused properties fromMeganavPaths. [1] [2] [3]How do you manually test this?
The companion PR on the website has all the instructions and a prepared environment to test this.
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Version Update
Type Changes
FlashesPropstype to restrict flash message propertiesMeganavcomponent types, introducingNoticeApiPropsNoticeprops type publicly exportableNew Features
useRailsUjsLinkshook for handling Rails UJS linksImprovements
MeganavPathstype structure