[ENG-77] Add formatting option for attribute select #497
Conversation
trangdoan982
force-pushed
the
eng-77/attribute-display
branch
from
e666721 to
b1fc8f3
Compare
|
FYI if you post the raw loom URL, GitHub embeds it :) EG: |
There was a problem hiding this comment.
Super cool! This looks great, unfortunately, we won't be able to allow the user to run arbitrary JavaScript as there is too much security risk. 😔 We use to do this awhile back in smartblocks but it got nixed: https://github.com/RoamJs/smartblocks/blob/main/docs/070-developer-docs.md#migrating-from-javascript-commands
Still, I do like the idea of having some presets ... so if you want to keep that in there but just use/default to regex as the custom format, I'm down with exploring that.
|
@mdroidian update: only allow user to either:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce a new text formatting feature in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TabsPanel
participant Formatter
participant TemplateMap
User->>TabsPanel: Select formatting template & enter custom regex
TabsPanel->>Formatter: Invoke formatText(text, settings)
Formatter->>TemplateMap: Determine transformation based on selected template
TemplateMap-->>Formatter: Execute transformation function
Formatter-->>TabsPanel: Return formatted text
TabsPanel-->>User: Display updated text
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (4)
src/features/attributeSelect.tsx (4)
40-70: The template implementations look good, but consider some improvements.The format templates are well structured with both transformation functions and descriptions. A few suggestions:
The "Remove Double Brackets" regex might be too restrictive with its start/end anchors (
^$), which will only work on strings that are exactly[[text]].The "Capitalize Words" function has limitations with certain words (like "don't" becoming "Don'T").
const TEMPLATE_MAP = { "No styling": { transform: (text: string) => text, description: "No styling" }, "Remove Double Brackets": { - transform: (text: string) => text.replace(/^\[\[(.*?)\]\]$/g, '$1'), + transform: (text: string) => text.replace(/\[\[(.*?)\]\]/g, '$1'), description: "Removes [[text]] format" }, "Convert to Uppercase": { transform: (text: string) => text.toUpperCase(), description: "Makes text all caps" }, "Capitalize Words": { - transform: (text: string) => text.split(' ').map(word => word.charAt(0).toUpperCase() + word.slice(1)).join(' '), + transform: (text: string) => text.split(' ').map(word => { + if (!word) return ''; + return word.charAt(0).toUpperCase() + word.slice(1).toLowerCase(); + }).join(' '), description: "Makes first letter of each word uppercase" }, // ... rest of the code }
74-93: Good implementation of the formatting function with error handling.The
applyFormattingfunction handles both standard templates and custom regex patterns with appropriate error catching. However, consider a couple of improvements:
- Use a stricter type for
templateNameparameter- Provide a way to surface errors to users beyond console logs
-const applyFormatting = ( - text: string, - templateName: string, +const applyFormatting = ( + text: string, + templateName: TemplateName | string, customPattern?: string, customReplacement?: string ): string => { try { const template = TEMPLATE_MAP[templateName as TemplateName]; if (!template) return text; if (templateName === "Custom Format" && customPattern) { return template.transform(text, customPattern, customReplacement); } return template.transform(text); } catch (e) { console.error("Error in transform function:", e); return text; } };
734-783: The custom format UI is comprehensive, but could use validation feedback.The UI for custom regex formatting is well implemented with input fields and a preview. Consider adding immediate validation feedback for regex patterns rather than just logging errors to console.
You could add a state variable to track regex validity and provide visual feedback:
const [customPattern, setCustomPattern] = useState(initialCustomPattern); const [customReplacement, setCustomReplacement] = useState(initialCustomReplacement); +const [isValidRegex, setIsValidRegex] = useState(true); // Then in the onChange handler: onChange={(e) => { const newValue = e.target.value; setCustomPattern(newValue); + try { + if (newValue) new RegExp(newValue); + setIsValidRegex(true); + } catch (e) { + setIsValidRegex(false); + } setInputSetting({ blockUid: attributeUid, key: "customPattern", value: newValue, }); }} // Then add visual feedback: +<input + className={`bp3-input font-mono text-sm w-full ${!isValidRegex ? 'bp3-intent-danger' : ''}`} + // ... +/> +{!isValidRegex && ( + <div className="text-red-500 text-xs mt-1"> + Invalid regular expression + </div> +)}
770-782: The preview section is a helpful user experience touch.Showing users a live preview of how their regex will transform text is a great feature. Consider making the example more dynamic or providing multiple examples that cover common use cases.
<div className="bg-gray-100 p-2 rounded text-sm"> <div className="font-bold mb-1">Preview:</div> <div> - <span className="font-bold">Input:</span> <span className="font-mono">[[Example]]</span> + <span className="font-bold">Input:</span> <span className="font-mono">[[Example]]</span> </div> <div> <span className="font-bold">Output:</span> <span className="font-mono"> {customPattern ? applyFormatting("[[Example]]", "Custom Format", customPattern, customReplacement) : "[[Example]]"} </span> </div> + <div className="mt-2"> + <span className="font-bold">Input:</span> <span className="font-mono">Page [[Reference]]</span> + </div> + <div> + <span className="font-bold">Output:</span> <span className="font-mono"> + {customPattern ? + applyFormatting("Page [[Reference]]", "Custom Format", customPattern, customReplacement) : + "Page [[Reference]]"} + </span> + </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/attributeSelect.tsx(7 hunks)
🔇 Additional comments (5)
src/features/attributeSelect.tsx (5)
124-161: Well-implemented formatting logic with good error handling.The
formatDisplayTextfunction properly retrieves configuration settings and applies the formatting with error handling. The organization is clear and readable.
173-173: Great use of the formatting function.Using the new
transformItemprop to apply formatting for display is a clean way to integrate the new functionality.
563-588: Good state management with useMemo for initial values.Using
useMemoto compute the initial values from settings is an efficient approach. The default values are also sensible.
686-732: Well-designed UI with helpful tooltips.The Display Format UI has a clean layout with good user guidance through tooltips. The dropdown implementation using
MenuItemSelectis appropriate.
787-797: Button placement is appropriate.Keeping the "Find All Current Values" button as part of the formatting UI section maintains a logical flow in the interface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/features/attributeSelect.tsx (2)
737-743: Consider mapping templates programmatically.Rather than manually creating list items for each template, you could map over the TEMPLATE_MAP entries, which would be more maintainable if templates are added or removed.
-{Object.entries(TEMPLATE_MAP).map(([name, { description }]) => ( - <li key={name}> - <span className="font-mono">{name}:</span>{" "} - {description} - </li> -))} +{Object.entries(TEMPLATE_MAP).map(([name, { description }]) => ( + <li key={name}> + <span className="font-mono">{name}:</span>{" "} + {description} + </li> +))}
134-170: 🛠️ Refactor suggestionConsider caching formatConfig query results.
The formatConfig useMemo performs multiple database queries for each attribute. Since this function runs whenever the attributeName changes and is used in transformItem (which runs for every item in the list), it could impact performance.
Apply this refactoring to improve performance:
- const formatConfig = useMemo(() => { + const formatConfig = useMemo(() => { try { const configUid = getPageUidByPageTitle(CONFIG); - const attributesNode = getSubTree({ - key: "attributes", - parentUid: configUid, - }); - const attributeUid = getSubTree({ - key: attributeName, - parentUid: attributesNode.uid, - }).uid; + // Get the attributeUid once + const attributesNode = getSubTree({ key: "attributes", parentUid: configUid }); + const attributeUid = getSubTree({ key: attributeName, parentUid: attributesNode.uid }).uid; + // Get all settings at once with a single query if possible return { templateName: getSettingValueFromTree({ key: "template", parentUid: attributeUid, }) || "No styling", customPattern: getSettingValueFromTree({ key: "customPattern", parentUid: attributeUid, }), customReplacement: getSettingValueFromTree({ key: "customReplacement", parentUid: attributeUid, }) }; } catch (e) { console.error("Error getting format config:", e); return { templateName: "No styling", customPattern: undefined, customReplacement: undefined }; } }, [attributeName]);
🧹 Nitpick comments (1)
src/features/attributeSelect.tsx (1)
753-817: Consider extracting the Custom Format UI to a separate component.The Custom Format section is quite large and could be extracted into its own component to improve code maintainability.
+const CustomFormatEditor = ({ + customPattern, + setCustomPattern, + customReplacement, + setCustomReplacement, + isValidRegex, + setIsValidRegex, + attributeUid +}) => { + return ( + <div className="space-y-2"> + <FormGroup label="Pattern (regex)" className="m-0"> + <input + className={`bp3-input font-mono text-sm w-full ${!isValidRegex ? 'bp3-intent-danger' : ''}`} + placeholder="E.g., \[\[(.*?)\]\]" + value={customPattern} + onChange={(e) => { + const newValue = e.target.value; + setCustomPattern(newValue); + try { + if (newValue) new RegExp(newValue); + setIsValidRegex(true); + } catch (e) { + setIsValidRegex(false); + } + setInputSetting({ + blockUid: attributeUid, + key: "customPattern", + value: newValue, + }); + }} + /> + {!isValidRegex && ( + <div className="text-red-500 text-xs mt-1"> + Invalid regular expression + </div> + )} + </FormGroup> + + <FormGroup label="Replacement" className="m-0"> + <input + className="bp3-input font-mono text-sm w-full" + placeholder="E.g., $1" + value={customReplacement} + onChange={(e) => { + const newValue = e.target.value; + setCustomReplacement(newValue); + setInputSetting({ + blockUid: attributeUid, + key: "customReplacement", + value: newValue, + }); + }} + /> + </FormGroup> + + <div className="bg-gray-100 p-2 rounded text-sm"> + <div className="font-bold mb-1">Preview:</div> + <div> + <span className="font-bold">Input:</span> <span className="font-mono">[[Example]]</span> + </div> + <div> + <span className="font-bold">Output:</span> <span className="font-mono"> + {customPattern ? + applyFormatting({ + text: "[[Example]]", + templateName: "Custom Format", + customPattern, + customReplacement + }) : + "[[Example]]"} + </span> + </div> + </div> + </div> + ); +}; // Then in the TabsPanel component: {selectedTemplate === "Custom Format" && ( <CustomFormatEditor customPattern={customPattern} setCustomPattern={setCustomPattern} customReplacement={customReplacement} setCustomReplacement={setCustomReplacement} isValidRegex={isValidRegex} setIsValidRegex={setIsValidRegex} attributeUid={attributeUid} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/attributeSelect.tsx(7 hunks)
🔇 Additional comments (9)
src/features/attributeSelect.tsx (9)
40-73: Well-structured template system for text formatting.The TEMPLATE_MAP implementation provides a clean, maintainable way to define text transformations. The inclusion of transform functions with descriptions makes the code self-documenting.
75-82: Good type definitions for template types and formatting parameters.Using TypeScript types effectively ensures type safety throughout the application. The explicit type definitions make the code more maintainable and help prevent errors.
84-103: Well-implemented formatting function with proper error handling.The applyFormatting function has good error handling for both general exceptions and specific template scenarios. The function is designed to be resilient and never crash, with helpful console error logging.
172-177: Clean implementation of formatText with memoization.Good use of useMemo to avoid unnecessary recalculations of the formatted text when the dependencies haven't changed.
704-820: Comprehensive UI for formatting options with good user guidance.The UI implementation for the formatting options is well-designed with:
- Informative tooltips explaining the feature
- Clear organization of options
- Live preview for custom regex patterns
- Validation feedback for regex errors
This provides an excellent user experience for both basic and advanced users.
579-606: Good state initialization from persisted settings.The component correctly initializes state from saved settings and provides defaults when needed. The use of useMemo to compute initial values just once is efficient.
799-816: Nice touch with the live preview.The preview section is an excellent UX feature that helps users understand how their regex pattern will transform text. This is particularly valuable for those who might not be familiar with regex syntax.
189-189: Improved text transformation in list items.The change from the previous implementation to using the
formatTextfunction ensures consistent formatting across the application.
755-779:❓ Verification inconclusive
Ensure regex validation is consistent with usage.
The validation logic checks if a pattern can be used to create a RegExp object, but doesn't verify that it would work as expected with the replacement pattern. Consider adding more comprehensive validation.
For regex patterns involving capture groups, ensure users understand how to reference them in the replacement string. For example, if a user creates a pattern with capture groups (
()), they would need to use$1,$2, etc. in the replacement string to reference those groups.
🌐 Web query:
What are common mistakes when using JavaScript regex replacements with capture groups?💡 Result:
Common mistakes when using JavaScript regex replacements with capture groups often stem from syntax errors, misnumbered references, and incorrect assumptions about regex behavior. Here are key issues to avoid:
1. Invalid Capture Group Names
Naming groups with non-identifier characters (e.g., numbers or symbols) triggers errors. Named groups must use valid JavaScript identifiers:
/(?<1>\d+)/; // SyntaxError: Invalid capture group name [1]Valid named group:
/(?<group1>\d+)/; // Correct [1][8]2. Incorrect Group Syntax
Using
(?without a valid group specifier (e.g.,?:,?=, or?<) causes errors:/Hello(?|!)/; // SyntaxError: invalid regexp group [2]Fix by escaping characters or using valid syntax:
/Hello(\?|!)/; // Correct [2]3. Misnumbered Backreferences
Referencing nonexistent groups (e.g.,
$4when only 3 groups exist) fails silently or causes unexpected results. Example from a failed replacement:// Intended to swap "one two three" to "three two one" let fixRegex = /(\w+)\s(\w)+\s(\w+)/; // Incorrectly captures middle word let replaceText = "$3 $2 $1"; // May reference undefined groups [9]Correct approach with proper grouping:
let fixRegex = /(\w+) (\w+) (\w+)/; let replaceText = "$3 $2 $1"; // Outputs "three two one" [9][3]4. Unanchored Patterns Causing Partial Matches
Regex patterns that don’t anchor to the start (
^) and end ($) of the string may unexpectedly match substrings:/(\d+)\s\1\s\1/.test("42 42 42 100"); // Returns true unintentionally [4]Anchored solution:
/^(\d+)\s\1\s\1$/.test("42 42 42"); // Matches exact string only [4]5. Incorrect Replacement Syntax
Using
$1vs.\1depends on the context. In JavaScript’sString.replace(), use$1, but in some environments (e.g., Unixsed), use\1[6]:// JavaScript "test".replace(/(st)/, "$1"); // Correct [6]6. Named Group Access Errors
Failing to check for
nullmatches before accessing.groupscauses runtime errors:const match = regex.exec(input); // Risky: const { group } = match.groups; // TypeError if match is null [10] // Safe: const group = match?.groups?.group; [10]7. Dynamic Regex Construction Pitfalls
When building regex dynamically, ensure group names or indices are correctly interpolated:
// Incorrect if tokenTypes keys are invalid identifiers const tokenPattern = new RegExp(`(?<${name}>...)`); // May fail [1] // Fix by validating keys: const tokenTypes = { validName: /.../ }; // Correct [1]By avoiding these errors and validating regex patterns with tools like RegExp Tester, you can effectively leverage capture groups in replacements.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Regex_invalid_capture_group_name
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Regex_invalid_group
- 3: https://forum.freecodecamp.org/t/help-use-capture-groups-to-search-and-replace/412711
- 4: https://forum.freecodecamp.org/t/regex-capture-groups/470548
- 5: vercel/next.js#19311
- 6: https://discourse.devontechnologies.com/t/regex-search-and-replace-no-captured-group/56060
- 7: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Capturing_group
- 8: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Named_capturing_group
- 9: https://forum.freecodecamp.org/t/regular-expressions-use-capture-groups-to-search-and-replace/533817
- 10: https://community.n8n.io/t/error-using-regex-groups-in-expression-how-to-return-an-object-from-a-regular-expression-match-using-named-capture-groups/29191
Action Required: Enhance Regex Validation Consistency and Guidance on Capture Groups
The current validation logic verifies that the regex pattern is syntactically correct (via instantiation of a RegExp object), but it does not ensure that the regex behaves as intended for replacement operations—especially when capture groups are involved. For instance, if a user creates a pattern with capture groups, they must reference them correctly (using
$1,$2, etc.) in any replacement string. This gap can lead to common issues such as misnumbered backreferences or incorrect assumptions about the regex behavior.Please consider one or more of the following actions:
- Extend Validation: Implement additional checks to analyze the pattern for capture groups and, if applicable, ensure that corresponding references in replacement strings are valid.
- User Guidance: Provide inline documentation or error messaging that explicitly informs users about the required replacement syntax for capture groups, highlighting common pitfalls (e.g., referencing non-existent groups or using improper syntax).
- Reference Resources: Incorporate best practices from sources that detail common mistakes with JavaScript regex replacements, as discovered in community examples.
trangdoan982
changed the title
Summary by CodeRabbit