feat: check pending commit changes (surface sync) - #861
Merged
Conversation
Mirror the shared-surface changes from openplc-web PR #509 so the cross-repo sync check stays green. Covers the diff-viewer components, source-control changes-section, workspace-screen, the tabs / shared / version-control / editor store slices (+ their tests), and the VersionControlPort `getChanges` widening (optional `includeContent` plus `before`/`after` content on `PendingChange`). The port change is backward-compatible (optional param + optional fields), so the editor's version-control adapter continues to satisfy the interface unchanged; wiring `includeContent` through the editor's IPC git service is a separate platform-side follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR introduces a new source control diff viewer that allows users to inspect file changes before committing. The implementation spans a new editor tab type ( ChangesDiff Viewer Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/frontend/components/_features/[workspace]/editor/diff-viewer/graphical-diff-viewer.tsx (1)
15-28: ⚡ Quick winUse
@root/*path alias instead of deep relative imports.The imports use relative paths like
'../../../../../../middleware/shared/providers'. Per coding guidelines, references to./src/*should use the@root/*path alias for maintainability.♻️ Proposed refactor
import type { DiffStatus, FlowData, GraphicalDiffResult, VarDiffEntry, -} from '../../../../../../middleware/shared/ports/version-control-port' -import { useVersionControl } from '../../../../../../middleware/shared/providers' -import { cn } from '../../../../../utils/cn' +} from '`@root/middleware/shared/ports/version-control-port`' +import { useVersionControl } from '`@root/middleware/shared/providers`' +import { cn } from '`@root/frontend/utils/cn`' import { EDGE_DIFF_STROKE, fbdDiffNodeTypes, ladderDiffNodeTypes, VAR_DIFF_COLORS, -} from '../../../../_atoms/graphical-editor/diff' +} from '`@root/frontend/components/_atoms/graphical-editor/diff`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/diff-viewer/graphical-diff-viewer.tsx around lines 15 - 28, Replace all deep relative imports in this file with the `@root/`* path alias to match project conventions: change imports that bring in DiffStatus, FlowData, GraphicalDiffResult, VarDiffEntry and useVersionControl from '../../../../../../middleware/shared/...' to '`@root/middleware/shared/`...', and change imports for cn and the diff constants (EDGE_DIFF_STROKE, fbdDiffNodeTypes, ladderDiffNodeTypes, VAR_DIFF_COLORS) from '../../../../../utils/cn' and '../../../../_atoms/graphical-editor/diff' to '`@root/utils/cn`' and '`@root/frontend/components/_atoms/graphical-editor/diff`' (or the correct `@root` paths used in your tsconfig paths); keep the imported symbol names (DiffStatus, useVersionControl, cn, EDGE_DIFF_STROKE, etc.) unchanged so references in this file continue to work.Source: Coding guidelines
src/frontend/store/__tests__/tabs-utils.test.ts (1)
1-12: ⚡ Quick winSwitch test imports to
@root/*aliases.The changed import section still uses relative src imports, which conflicts with the repo rule.
Suggested diff
-import type { TabsProps } from '../slices/tabs/types' +import type { TabsProps } from '`@root/frontend/store/slices/tabs/types`' import { CreateDeviceEditor, CreateDiffViewerEditor, CreateEditorModelObject, CreateEditorObjectFromTab, CreatePLCGraphicalObject, CreatePLCTextualObject, CreateRemoteDeviceEditor, CreateResourceEditor, CreateServerEditor, -} from '../slices/tabs/utils' +} from '`@root/frontend/store/slices/tabs/utils`'As per coding guidelines, "Use path alias
@root/*to reference./src/*".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/store/__tests__/tabs-utils.test.ts` around lines 1 - 12, Update the import statements in tabs-utils.test.ts to use the repo path alias `@root/`* instead of relative paths; replace the relative import of TabsProps and the named imports (CreateDeviceEditor, CreateDiffViewerEditor, CreateEditorModelObject, CreateEditorObjectFromTab, CreatePLCGraphicalObject, CreatePLCTextualObject, CreateRemoteDeviceEditor, CreateResourceEditor, CreateServerEditor) from ../slices/tabs/... with equivalent imports from `@root/slices/tabs/`... so the test follows the project's aliasing rules.Source: Coding guidelines
src/frontend/store/__tests__/version-control-slice.test.ts (1)
3-4: ⚡ Quick winUse
@root/*aliases for src imports.Line 3 and Line 4 use relative paths into
src, which diverges from the repository import-path rule.Suggested diff
-import { createVersionControlSlice } from '../slices/version-control/slice' -import type { VersionControlSlice } from '../slices/version-control/types' +import { createVersionControlSlice } from '`@root/frontend/store/slices/version-control/slice`' +import type { VersionControlSlice } from '`@root/frontend/store/slices/version-control/types`'As per coding guidelines, "Use path alias
@root/*to reference./src/*".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/store/__tests__/version-control-slice.test.ts` around lines 3 - 4, Replace the relative imports with the repository alias for src: update the import of createVersionControlSlice (currently from '../slices/version-control/slice') to use '`@root/slices/version-control/slice`' and update the type import VersionControlSlice (currently from '../slices/version-control/types') to '`@root/slices/version-control/types`'; ensure the imported symbol names (createVersionControlSlice and VersionControlSlice) remain unchanged so tests continue to reference the same identifiers.Source: Coding guidelines
src/frontend/components/_features/[workspace]/editor/diff-viewer/index.tsx (1)
21-25: ⚡ Quick winUse
@root/*aliases for internal imports in this new file.Please replace deep relative imports with
@root/*aliases to match repository import conventions.As per coding guidelines, use path alias
@root/*to reference./src/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/diff-viewer/index.tsx around lines 21 - 25, Replace the deep relative imports in this file by using the repository path alias `@root/`*: change imports that bring in useVersionControl, buildAllProjectFileContents, useOpenPLCStore, cn, and any other internal modules referenced here to their corresponding `@root/`* aliased paths (e.g., `@root/middleware/shared/providers`, `@root/services/save-actions`, `@root/store`, `@root/utils/cn`, etc.) so internal imports follow the project's alias convention; update import statements that currently use long ../../../.. chains to the concise `@root/`* forms and ensure the FileDiffView import remains correct (use `@root/`... for that module if it’s internal).Source: Coding guidelines
src/frontend/screens/workspace-screen.tsx (1)
22-22: ⚡ Quick winUse
@root/*alias for the new internal import.Please switch this newly added internal import to
@root/*style for consistency with repo rules.As per coding guidelines, use path alias
@root/*to reference./src/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/screens/workspace-screen.tsx` at line 22, Replace the new relative import of DiffViewerEditor with the repository alias style: change the import of DiffViewerEditor (symbol DiffViewerEditor) to use the `@root/`* alias that maps to ./src (e.g., import from '`@root/components/_features/`[workspace]/editor/diff-viewer') so the module path follows the project's alias convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/editor/diff-viewer/file-diff-view.tsx:
- Around line 42-57: In formatContentForDisplay, the search for "END_VAR" is
case-sensitive (endVarIdx = beforeEnd.lastIndexOf('END_VAR')) while the end
keyword match is case-insensitive; make the END_VAR lookup case-insensitive to
match IEC 61131-3 semantics by searching beforeEnd in a case-insensitive way
(e.g., use a case-insensitive regex search or normalize case before calling
lastIndexOf) so endVarIdx correctly finds variants like "end_var" or "End_Var",
then build declaration and return as before.
In `@src/frontend/components/_features/`[workspace]/editor/diff-viewer/index.tsx:
- Around line 41-45: The deriveStatus(original, current) heuristic mislabels
empty-content files because empty strings are normalized and collapse
added/modified/deleted states; replace use of deriveStatus as the source of
truth and instead use the PendingChange.status value returned by getChanges (the
PendingChange objects used around the before variable and the change list) for
the badge; update any callers that pass original/current (e.g., the code that
computes `before`/`after` and renders the status badge in this diff viewer) to
read change.status from the PendingChange and fall back to deriveStatus only if
PendingChange.status is missing, ensuring the badge reflects
PendingChange.status consistently.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/diff-viewer/graphical-diff-viewer.tsx:
- Around line 15-28: Replace all deep relative imports in this file with the
`@root/`* path alias to match project conventions: change imports that bring in
DiffStatus, FlowData, GraphicalDiffResult, VarDiffEntry and useVersionControl
from '../../../../../../middleware/shared/...' to '`@root/middleware/shared/`...',
and change imports for cn and the diff constants (EDGE_DIFF_STROKE,
fbdDiffNodeTypes, ladderDiffNodeTypes, VAR_DIFF_COLORS) from
'../../../../../utils/cn' and '../../../../_atoms/graphical-editor/diff' to
'`@root/utils/cn`' and '`@root/frontend/components/_atoms/graphical-editor/diff`'
(or the correct `@root` paths used in your tsconfig paths); keep the imported
symbol names (DiffStatus, useVersionControl, cn, EDGE_DIFF_STROKE, etc.)
unchanged so references in this file continue to work.
In `@src/frontend/components/_features/`[workspace]/editor/diff-viewer/index.tsx:
- Around line 21-25: Replace the deep relative imports in this file by using the
repository path alias `@root/`*: change imports that bring in useVersionControl,
buildAllProjectFileContents, useOpenPLCStore, cn, and any other internal modules
referenced here to their corresponding `@root/`* aliased paths (e.g.,
`@root/middleware/shared/providers`, `@root/services/save-actions`, `@root/store`,
`@root/utils/cn`, etc.) so internal imports follow the project's alias convention;
update import statements that currently use long ../../../.. chains to the
concise `@root/`* forms and ensure the FileDiffView import remains correct (use
`@root/`... for that module if it’s internal).
In `@src/frontend/screens/workspace-screen.tsx`:
- Line 22: Replace the new relative import of DiffViewerEditor with the
repository alias style: change the import of DiffViewerEditor (symbol
DiffViewerEditor) to use the `@root/`* alias that maps to ./src (e.g., import from
'`@root/components/_features/`[workspace]/editor/diff-viewer') so the module path
follows the project's alias convention.
In `@src/frontend/store/__tests__/tabs-utils.test.ts`:
- Around line 1-12: Update the import statements in tabs-utils.test.ts to use
the repo path alias `@root/`* instead of relative paths; replace the relative
import of TabsProps and the named imports (CreateDeviceEditor,
CreateDiffViewerEditor, CreateEditorModelObject, CreateEditorObjectFromTab,
CreatePLCGraphicalObject, CreatePLCTextualObject, CreateRemoteDeviceEditor,
CreateResourceEditor, CreateServerEditor) from ../slices/tabs/... with
equivalent imports from `@root/slices/tabs/`... so the test follows the project's
aliasing rules.
In `@src/frontend/store/__tests__/version-control-slice.test.ts`:
- Around line 3-4: Replace the relative imports with the repository alias for
src: update the import of createVersionControlSlice (currently from
'../slices/version-control/slice') to use '`@root/slices/version-control/slice`'
and update the type import VersionControlSlice (currently from
'../slices/version-control/types') to '`@root/slices/version-control/types`';
ensure the imported symbol names (createVersionControlSlice and
VersionControlSlice) remain unchanged so tests continue to reference the same
identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0daa6e3-2ae2-4365-bfad-cc18779e1c62
📒 Files selected for processing (16)
src/frontend/components/_atoms/tab/index.tsxsrc/frontend/components/_features/[workspace]/editor/diff-viewer/file-diff-view.tsxsrc/frontend/components/_features/[workspace]/editor/diff-viewer/graphical-diff-viewer.tsxsrc/frontend/components/_features/[workspace]/editor/diff-viewer/index.tsxsrc/frontend/components/_features/[workspace]/source-control/changes-section.tsxsrc/frontend/screens/workspace-screen.tsxsrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/__tests__/tabs-utils.test.tssrc/frontend/store/__tests__/version-control-slice.test.tssrc/frontend/store/slices/editor/types.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/tabs/types.tssrc/frontend/store/slices/tabs/utils.tssrc/frontend/store/slices/version-control/slice.tssrc/frontend/store/slices/version-control/types.tssrc/middleware/shared/ports/version-control-port.ts
Comment on lines
+42
to
+57
| export function formatContentForDisplay(path: string, content: string): string { | ||
| const ext = path.split('.').pop()?.toLowerCase() | ||
| if (ext !== 'ld' && ext !== 'fbd') return content | ||
|
|
||
| const endMatch = content.match(/\b(END_PROGRAM|END_FUNCTION_BLOCK|END_FUNCTION)\b/i) | ||
| if (!endMatch || endMatch.index === undefined) return content | ||
|
|
||
| const endKeyword = endMatch[0] | ||
| const beforeEnd = content.slice(0, endMatch.index) | ||
|
|
||
| const endVarIdx = beforeEnd.lastIndexOf('END_VAR') | ||
| if (endVarIdx === -1) return content | ||
|
|
||
| const declaration = beforeEnd.slice(0, endVarIdx + 'END_VAR'.length) | ||
| return `${declaration}\n\n(* ${ext.toUpperCase()} graphical data omitted *)\n\n${endKeyword}` | ||
| } |
Contributor
There was a problem hiding this comment.
Case-sensitivity inconsistency in END_VAR search.
The regex on line 46 uses the /i flag for case-insensitive matching of END_PROGRAM/END_FUNCTION_BLOCK/END_FUNCTION, but line 52 uses lastIndexOf('END_VAR') which is case-sensitive. IEC 61131-3 ST language is case-insensitive, so files with end_var or End_Var would fail to collapse the graphical blob.
🔧 Proposed fix for case-insensitive matching
- const endVarIdx = beforeEnd.lastIndexOf('END_VAR')
- if (endVarIdx === -1) return content
+ const endVarMatch = beforeEnd.match(/END_VAR/i)
+ if (!endVarMatch || endVarMatch.index === undefined) return content
+ const endVarIdx = endVarMatch.index
- const declaration = beforeEnd.slice(0, endVarIdx + 'END_VAR'.length)
+ const declaration = beforeEnd.slice(0, endVarIdx + endVarMatch[0].length)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/diff-viewer/file-diff-view.tsx
around lines 42 - 57, In formatContentForDisplay, the search for "END_VAR" is
case-sensitive (endVarIdx = beforeEnd.lastIndexOf('END_VAR')) while the end
keyword match is case-insensitive; make the END_VAR lookup case-insensitive to
match IEC 61131-3 semantics by searching beforeEnd in a case-insensitive way
(e.g., use a case-insensitive regex search or normalize case before calling
lastIndexOf) so endVarIdx correctly finds variants like "end_var" or "End_Var",
then build declaration and return as before.
Comment on lines
+41
to
+45
| function deriveStatus(original: string, current: string): FileStatus { | ||
| if (original === current) return 'U' | ||
| if (!original) return 'A' | ||
| if (!current) return 'D' | ||
| return 'M' |
Contributor
There was a problem hiding this comment.
Status badge logic is incorrect for empty-content files.
On Line 41-45, status is inferred from empty-string checks, and on Line 74 before is normalized to ''. That collapses distinct states (added/modified/deleted) when content is empty and can mislabel files (for example, modified-to-empty vs deleted).
Use PendingChange.status from Line 72’s getChanges response as the primary badge source instead of content heuristics.
Suggested fix
-import { useEffect, useMemo } from 'react'
+import { useEffect, useMemo, useState } from 'react'
@@
export function DiffViewerEditor() {
+ const [statusByPath, setStatusByPath] = useState<Record<string, FileStatus>>({})
@@
try {
const { changes } = await versionControl.getChanges(projectId, undefined, true)
const map: Record<string, string> = {}
- for (const c of changes) map[c.path] = c.before ?? ''
- if (!cancelled) setHeadContent(map)
+ const nextStatus: Record<string, FileStatus> = {}
+ for (const c of changes) {
+ map[c.path] = c.before ?? ''
+ nextStatus[c.path] = c.status === 'added' ? 'A' : c.status === 'deleted' ? 'D' : 'M'
+ }
+ if (!cancelled) {
+ setHeadContent(map)
+ setStatusByPath(nextStatus)
+ }
} catch {
if (!cancelled) setHeadContent({})
}
@@
+ const badgeStatus = filePath ? statusByPath[filePath] ?? deriveStatus(original, current) : 'U'
@@
- FILE_STATUS_CONFIG[deriveStatus(original, current)].badge,
+ FILE_STATUS_CONFIG[badgeStatus].badge,
)}
>
- {FILE_STATUS_CONFIG[deriveStatus(original, current)].label}
+ {FILE_STATUS_CONFIG[badgeStatus].label}
</span>
)}Also applies to: 72-75, 114-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/components/_features/`[workspace]/editor/diff-viewer/index.tsx
around lines 41 - 45, The deriveStatus(original, current) heuristic mislabels
empty-content files because empty strings are normalized and collapse
added/modified/deleted states; replace use of deriveStatus as the source of
truth and instead use the PendingChange.status value returned by getChanges (the
PendingChange objects used around the before variable and the change list) for
the badge; update any callers that pass original/current (e.g., the code that
computes `before`/`after` and renders the status badge in this diff viewer) to
read change.status from the PendingChange and fall back to deriveStatus only if
PendingChange.status is missing, ensuring the badge reflects
PendingChange.status consistently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Editor-side mirror of openplc-web PR #509 (
feat/check-commit-hanges-before-merge) to keep the cross-repo shared-surface sync green.Shared surfaces mirrored (byte-identical to web)
16 files across: diff-viewer components, source-control
changes-section,workspace-screen, thetabs/shared/version-control/editorstore slices (+ their tests), andversion-control-port.ts.The
VersionControlPortchange is backward-compatible:getChangesgains an optionalincludeContentparam andPendingChangegains optionalbefore/aftercontent fields. The editor's existingversion-control-adaptertherefore still satisfies the interface without changes.Follow-up (platform-side, not blocking sync)
Wiring
includeContentthrough the editor's IPC git service so the desktop diff viewer gets HEAD/working-tree content is a separate platform task. Until then the editor adapter ignores the optional param (feature degrades to no inline diff content), which is type-safe.Verification
compare-surfaces.pyagainst the web PR head: match, 0 diffs.tsc: no new type errors in the mirrored files (only pre-existing repo errors remain).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements