fix(logEventNum): Keyboard navigation shortcuts should update the log event number (fixes #105). #180
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
src/components/Editor/MonacoInstance/actions.tssrc/components/Editor/MonacoInstance/typings.ts🔇 Additional comments (10)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Editor/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/components/Editor/index.tsx
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/components/Editor/index.tsx
src/components/Editor/index.tsx
Outdated
| case ACTION_NAME.PAGE_TOP: { | ||
| goToPositionAndCenter(editor, {lineNumber: 1, column: 1}); | ||
| const newlogEventNum = beginLineNumToLogEventNumRef.current.get(1); | ||
| if (newlogEventNum) { | ||
| updateWindowUrlHashParams({logEventNum: newlogEventNum}); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and variable naming consistency.
The implementation has several areas for improvement:
- The variable name
newlogEventNumshould follow camelCase convention. - Missing error handling when no log event number is found.
- Should verify editor model exists before proceeding.
Apply this diff to address these issues:
case ACTION_NAME.PAGE_TOP: {
+ const model = editor.getModel();
+ if (!model) {
+ console.error("Editor model is undefined");
+ break;
+ }
goToPositionAndCenter(editor, {lineNumber: 1, column: 1});
- const newlogEventNum = beginLineNumToLogEventNumRef.current.get(1);
- if (newlogEventNum) {
- updateWindowUrlHashParams({logEventNum: newlogEventNum});
+ const newLogEventNum = beginLineNumToLogEventNumRef.current.get(1);
+ if (!newLogEventNum) {
+ console.error("Unable to find log event number for first line");
+ break;
}
+ updateWindowUrlHashParams({logEventNum: newLogEventNum});
break;
}Committable suggestion skipped: line range outside the PR's diff.
src/components/Editor/index.tsx
Outdated
| case ACTION_NAME.PAGE_BOTTOM: { | ||
| const lineCount = editor.getModel()?.getLineCount(); | ||
| if ("undefined" === typeof lineCount) { | ||
| break; | ||
| } | ||
| goToPositionAndCenter(editor, {lineNumber: lineCount, column: 1}); | ||
| const newlogEvent = beginLineNumToLogEventNumRef.current.get(1); | ||
| if (newlogEvent) { | ||
| updateWindowUrlHashParams({logEventNum: newlogEvent + pageSizeRef.current - 1}); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and calculation accuracy.
The implementation has several areas for improvement:
- The variable name
newlogEventshould follow camelCase convention. - Missing error handling when no log event number is found.
- The calculation of the last log event number might be inaccurate if log event numbers are not continuous.
Apply this diff to address these issues:
case ACTION_NAME.PAGE_BOTTOM: {
const lineCount = editor.getModel()?.getLineCount();
if ("undefined" === typeof lineCount) {
break;
}
goToPositionAndCenter(editor, {lineNumber: lineCount, column: 1});
- const newlogEvent = beginLineNumToLogEventNumRef.current.get(1);
- if (newlogEvent) {
- updateWindowUrlHashParams({logEventNum: newlogEvent + pageSizeRef.current - 1});
+ const lastLineLogEventNum = getMapValueWithNearestLessThanOrEqualKey(
+ beginLineNumToLogEventNumRef.current,
+ lineCount
+ );
+ if (!lastLineLogEventNum) {
+ console.error("Unable to find log event number for last line");
+ break;
}
+ updateWindowUrlHashParams({logEventNum: lastLineLogEventNum});
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case ACTION_NAME.PAGE_BOTTOM: { | |
| const lineCount = editor.getModel()?.getLineCount(); | |
| if ("undefined" === typeof lineCount) { | |
| break; | |
| } | |
| goToPositionAndCenter(editor, {lineNumber: lineCount, column: 1}); | |
| const newlogEvent = beginLineNumToLogEventNumRef.current.get(1); | |
| if (newlogEvent) { | |
| updateWindowUrlHashParams({logEventNum: newlogEvent + pageSizeRef.current - 1}); | |
| } | |
| break; | |
| } | |
| case ACTION_NAME.PAGE_BOTTOM: { | |
| const lineCount = editor.getModel()?.getLineCount(); | |
| if ("undefined" === typeof lineCount) { | |
| break; | |
| } | |
| goToPositionAndCenter(editor, {lineNumber: lineCount, column: 1}); | |
| const lastLineLogEventNum = getMapValueWithNearestLessThanOrEqualKey( | |
| beginLineNumToLogEventNumRef.current, | |
| lineCount | |
| ); | |
| if (!lastLineLogEventNum) { | |
| console.error("Unable to find log event number for last line"); | |
| break; | |
| } | |
| updateWindowUrlHashParams({logEventNum: lastLineLogEventNum}); | |
| break; | |
| } |
|
Thanks for bringing the issue to our attention. The fix should work just fine, but I think there can be better way to streamline this cursor change handling.
I think there are two ways to address this issue:
Personally, I prefer Approach (2.), but feel free to comment. |
|
@davemarco to comment. Please help with the rest of the review. |
|
@coderabbitai create an issue for this fix |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@junhaoliao: I've created a new issue to track this fix. The issue focuses on handling cursor change events with NotSet reason in setupCursorExplicitPosChangeCallback, as per the preferred approach discussed. 🏁 Script executed: Length of output: 943 @junhaoliao: I've created issue #181 to track this fix. You can find it at: #181 |
|
Actually an issue has already been created at #105 |
junhaoliao
changed the title
|
Edited the PR title directly to include the issue number. |
|
@junhaoliao Thanks for your reply.
I agree, it seems the most straightforward fix. I managed to update the functions names by removing the "Explicit" term from within the name itself - that would be misleading now. I updated the docs too. Do you think it is worth putting a comment explaining why we also include the case |
I quote here the description of #105:
Summary by CodeRabbit