Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

⚠️ No Changeset found

Latest commit: 353d080

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Deploy preview for codeimage-highlight-dev ready!

✅ Preview
https://codeimage-highlight-ccqtrbvuq-riccardo-perras-projects.vercel.app
https://codeimage-highlight-pr-617.vercel.app

Built with commit 353d080.
This pull request is being automatically deployed with vercel-action

Deploy preview for codeimage-website-dev ready!

✅ Preview
https://codeimage-website-9krrnu94j-riccardo-perras-projects.vercel.app
https://codeimage-website-pr-617.vercel.app

Built with commit 353d080.
This pull request is being automatically deployed with vercel-action

Deploy preview for codeimage ready!

✅ Preview
https://codeimage-cpb954jwm-riccardo-perras-projects.vercel.app
https://codeimage-app-pr-617.vercel.app

Built with commit 353d080.
This pull request is being automatically deployed with vercel-action

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@gooroodev review please

Appreciate your call, @oleksandr-vonahi!

Issue Counts

Summary of Proposed Changes

• ➕ Added diff mode feature to the editor.
• ➕ Introduced DiffCheckbox and DiffCheckboxMarker components for marking lines as added, removed, or untouched.
• ➕ Added diff state management using StateField and StateEffect.
• ➕ Added customEffectAnnotation for custom transaction annotations.
• ➕ Introduced new language support for git-patch.
• ➕ Added a debug preview mode in the footer for development.
• ➕ Added diffMarkerControl and diffMarkerStateIconGutter extensions for diff mode.
• ➕ Added new styles for diff mode and debug preview.
• 🛠️ Updated EditorState to include metadata for diff lines.
• 🛠️ Updated EditorStyleForm to include a checkbox for toggling diff mode.
• 🛠️ Updated PreviewExportEditor to handle syncing transactions.
• 🚀 Improved performance by batching transaction syncs in PreviewExportEditor.
• 📖 Added comments and improved readability in various parts of the code.

Identified Issues

Issue Details and Fixes

Issue 1: Potential memory leak in DiffCheckboxMarker due to untracked disposal

Details: The dispose function in DiffCheckboxMarker is not guaranteed to be called, leading to potential memory leaks.

Fix:

import { onCleanup } from 'solid-js';

// Inside DiffCheckboxMarker class
toDOM(view: EditorView): Node {
  return createRoot(dispose => {
    this.dispose = dispose;
    onCleanup(() => this.dispose?.());

    const [value, setValue] = createSignal<DiffCheckboxState>('untouched');

    const unsubscribe = diffPluginEvents.on('syncLine', ({state, line}) => {
      if (line.number === this.lineNumber) {
        setValue(state ?? 'untouched');
      }
    });

    onCleanup(() => unsubscribe());

    return (
      <div class={container}>
        <DiffCheckbox
          value={value()}
          onChange={state =>
            dispatchUpdateDiffLineState(view, this.lineNumber, state)
          }
        />
      </div>
    );
  }) as Node;
}

Explanation: Added onCleanup to ensure the dispose function is called when the component is destroyed.

Issue 2: Unnecessary state updates in CanvasEditor

Details: The createEffect in CanvasEditor updates the state even when there are no changes.

Fix:

createEffect(
  on(
    () => editorState.options.focused,
    isFocused => {
      if (view && !view.hasFocus && isFocused) {
        editorSetFocused(true);
      }
    },
    { defer: true },
  ),
);

Explanation: Added { defer: true } to the on function to defer the effect until necessary.

Issue 3: Inefficient transaction handling in PreviewExportEditor

Details: Transactions are handled individually, leading to performance issues.

Fix:

let unsubscribe: VoidFunction;
createEffect(
  on(editorView, editorView => {
    if (unsubscribe) unsubscribe();
    if (!editorView) return;

    transactions.value().forEach(transaction => {
      sync(transaction, editorView);
      transactions.setValue(trs => trs.filter(tr => tr !== transaction));
    });

    unsubscribe = transactions.listen(({ event: transaction, remove }) => {
      sync(transaction, editorView);
      remove();
    });
  }),
);

Explanation: Batched transaction handling to improve performance.

Issue 4: Magic strings used for annotation events

Details: Magic strings are used for annotation events, which can lead to errors.

Fix:

const INIT_METADATA_ANNOTATION_EVENT = 'init-metadata';

Explanation: Defined a constant for the annotation event to avoid magic strings.

Issue 5: Hardcoded colors in styles

Details: Colors are hardcoded in styles, making them difficult to maintain.

Fix:

import { themeTokens } from '@codeui/kit';

export const colors = {
  removeLine: themeTokens.colors.red[500],
  removeToken: themeTokens.colors.red[700],
  addLine: themeTokens.colors.green[500],
  addToken: themeTokens.colors.green[700],
};

Explanation: Used theme tokens for colors to improve maintainability.

Issue 6: Lack of comments explaining complex logic

Details: Complex logic is not explained with comments, making it difficult to understand.

Fix: Add comments explaining the logic in complex functions.

Issue 7: Long function bodies without clear separation of concerns

Details: Long function bodies make it difficult to understand the code.

Fix: Refactor long functions into smaller, more manageable functions.

Issue 8: Inconsistent naming conventions

Details: Naming conventions are inconsistent, leading to confusion.

Fix: Standardize naming conventions across the codebase.

General Review

The code introduces a significant new feature with diff mode and handles state management and UI updates effectively. However, there are some areas where best practices and readability can be improved. Specifically, the code can benefit from better separation of concerns, more consistent naming conventions, and avoiding magic strings. Additionally, performance improvements can be made by batching transaction handling. Overall, the code quality is good, but addressing the identified issues will make it more maintainable and performant.

--
I only arrive when I am mentioned and asked to review the pull request.
Your thoughts matter! Please react or reply.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

This feature is great! I'm looking for this feature.

But I have a small suggestion. After completing the diff selection, can you hide the previous checkbox? Now I feel it affects the appearance a bit.

Hi @TinsFox, what do you mean with "After completing the diff selection, can you hide the previous checkbox?"

Hi @TinsFox, what do you mean with "After completing the diff selection, can you hide the previous checkbox?"

As shown in the marked area in my picture, after I select the rows I need to highlight, I hope that there will be no checkbox in front of other rows that are not selected. I think this will affect the appearance.