Fixes #8676: Fix Chinese IME input in relation map#9140
Fixes #8676: Fix Chinese IME input in relation map#9140Dinis-Ou wants to merge 1 commit intoTriliumNext:mainfrom
Conversation
dosubot
Bot
added
the
size:L
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical usability bug for users employing Input Method Editors (IMEs), particularly Chinese, within the relation map feature. By refactoring the input event handling mechanism, it eliminates the problem of duplicated characters during IME composition, thereby significantly improving the user experience for non-Latin script input. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an issue with Chinese IME input in the relation map by replacing the keyup event listener with a more robust approach using compositionstart, compositionend, and input events. This prevents premature filtering during IME composition. The addition of unit tests is a great step towards ensuring the fix is solid.
However, I've identified a critical issue in the tests where the mock for filterAttributeName does not match the production implementation, leading to tests that don't accurately reflect the real behavior. I've also suggested a minor refactoring to improve code clarity and reduce duplication in the new input handling logic. Please see the detailed comments for suggestions.
| vi.mock('../../../services/utils', async (importOriginal) => { | ||
| const actual = await importOriginal() as any | ||
| return { | ||
| ...actual, | ||
| default: { | ||
| ...actual.default, | ||
| filterAttributeName: vi.fn((val: string) => val.replace(/[^a-z0-9]/gi, '')) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
The mock implementation of filterAttributeName here differs significantly from the actual implementation in apps/client/src/services/utils.ts.
The mock (val.replace(/[^a-z0-9]/gi, '')) strips all non-ASCII alphanumeric characters, including Chinese characters. However, the real implementation (name.replace(/[^\p{L}\p{N}_:]/gu, "")) allows Unicode letters (like '你') because of \p{L}.
This discrepancy means the tests are not accurately reflecting the production behavior. For example, the test 'filters invalid characters from input after IME composition ends' expects '你' to be stripped, but it would be preserved in production. This could lead to unexpected behavior where invalid attribute names are created.
Please update the mock to match the real implementation, or if the intent is to disallow Unicode characters for relation names, the filterAttributeName function itself should be adjusted (though that might be a broader change). A consistent behavior between tests and production code is crucial.
| export function setupImeAwareInput($answer: JQuery<HTMLInputElement>) { | ||
| let isComposing = false; | ||
|
|
||
| $answer.on("compositionstart", () => { | ||
| isComposing = true; | ||
| }); | ||
| $answer.on("compositionend", () => { | ||
| isComposing = false; | ||
| const attrName = utils.filterAttributeName($answer.val() as string); | ||
| $answer.val(attrName); | ||
| }); | ||
| $answer.on("input", () => { | ||
| if (isComposing) return; | ||
| const attrName = utils.filterAttributeName($answer.val() as string); | ||
| $answer.val(attrName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can extract the filtering logic into a separate function within setupImeAwareInput. This makes the code cleaner and avoids repeating the same two lines in both the compositionend and input event handlers.
export function setupImeAwareInput($answer: JQuery<HTMLInputElement>) {
let isComposing = false;
const filterValue = () => {
const attrName = utils.filterAttributeName($answer.val() as string);
$answer.val(attrName);
};
$answer.on("compositionstart", () => {
isComposing = true;
});
$answer.on("compositionend", () => {
isComposing = false;
// The 'input' event that follows 'compositionend' is not always reliable across browsers/IMEs.
// We filter here to ensure the value is sanitized after composition finishes.
filterValue();
});
$answer.on("input", () => {
if (isComposing) {
return;
}
filterValue();
});
}
|
Hi! Just checking in on this PR. Would appreciate a review when you have time. It fixes an IME composition bug that affects Chinese input users. Happy to make any adjustments based on feedback. Thanks! |
Dinis-Ou
force-pushed
the
bug-fix_8676
branch
2 times, most recently
from
9058af7 to
f2e8996
Compare
dosubot
Bot
added
size:S
When creating a connection in the relation map using Chinese input(IME), repeated/duplicated input occurred because the input handler fired during IME composition. This was fixed by replacing the keyup listener with compositionstart, compositionend, and input events, ensuring input events are ignored while IME composition is active.
|
Ready for review — updated the implementation to reflect the latest changes in the codebase. |
2 participants
When creating a connection in the relation map using Chinese input(IME), repeated/duplicated input occurred because the input handler fired during IME composition. This was fixed by replacing the keyup listener with compositionstart, compositionend, and input events, ensuring input events are ignored while IME composition is active.
Added unit tests to verify: