-
Notifications
You must be signed in to change notification settings - Fork 37
LivePreview - Normalize spaces in get setting #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Settings as src/settings.ts
Note over Settings: New normalizeSpaces helper added
Caller->>Settings: provide s.text and searchTerm
Settings->>Settings: normalizedText = normalizeSpaces(s.text)
Settings->>Settings: if normalizedText contains normalized(searchTerm)
alt match found
Settings->>Settings: cleaned = removeMarkers(normalizedText)
Settings->>Caller: return cleaned.trim()
else no match
Settings->>Caller: continue / no match
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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 |
…is improves the accuracy of setting retrieval by removing unwanted whitespace and zero-width characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/settings.ts (1)
14-16: Consider normalizingsettingNamefor consistent matching.The function normalizes
s.textbut not thesettingNameparameter. IfsettingNamecontains zero-width characters or multiple spaces, theincludescheck (line 15) and thereplacecall (line 20) might not work as expected.Apply this diff to ensure consistent normalization:
export const get = (settingName: string) => { let customTrigger = getBlockUidsAndTextsReferencingPage("42Setting"); let result = null; + const normalizedSettingName = normalizeSpaces(settingName); for (let s of customTrigger) { const normalizedText = normalizeSpaces(s.text); - if (normalizedText.includes(settingName)) { + if (normalizedText.includes(normalizedSettingName)) { result = normalizedText .replace("#42Setting ", "") .replace("#[[42Setting]] ", "") .replace("[[42Setting]] ", "") - .replace(settingName, "") + .replace(normalizedSettingName, "") .trim(); break; } } return result; };Also applies to: 20-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/settings.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/settings.ts
[error] 7-7: A character class cannot match a joined character sequence.
A zero width joiner composes several emojis into a new one. Replace the character class with an alternation.
(lint/suspicious/noMisleadingCharacterClass)
🔇 Additional comments (2)
src/settings.ts (2)
6-7: Static analysis warning is likely a false positive here.Biome flags
\u200D(zero-width joiner) in the character class, noting that it composes emoji sequences. However, since the intent is to strip these characters rather than match composed emojis, the current implementation should work correctly for this use case.
11-11: Good modernization:var→let.Using
letprovides proper block scoping and aligns with modern JavaScript best practices.
Problem submitted by Anup J. via slack
Summary by CodeRabbit