feat: version control#742
Merged
Merged
Conversation
Contributor
WalkthroughAdds popover branch UI with client-side branch-name sanitization; overhauls version-control baseline/save pipeline to preserve byte-identical uploads; hides system files from VC views; updates project-tree persistence/rename flows; introduces navigation port and editor navigation adapter; assorted lint suppressions and small UI tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant SaveSvc as Save Service
participant VCState as VersionControl State
participant Backend as Backend
UI->>SaveSvc: executeSaveProject(projectId)
activate SaveSvc
SaveSvc->>SaveSvc: buildAllProjectFileContentsPure()<br/>serializeProjectFile per path
SaveSvc->>VCState: read loadedSerialized & rawLoadedContent
loop per file
SaveSvc->>SaveSvc: pickContentForSave(path, freshSerialized, syncState)
end
SaveSvc->>Backend: Upload files + deletions
activate Backend
Backend-->>SaveSvc: Success + rawLoadedFiles
deactivate Backend
SaveSvc->>VCState: recordSavedFiles([{path, content}, ...])
activate VCState
VCState-->>SaveSvc: updated baseline/changedPaths
deactivate VCState
SaveSvc->>UI: success toast / UI refresh
deactivate SaveSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 3
🧹 Nitpick comments (4)
src/frontend/components/_molecules/project-tree/index.tsx (1)
728-729: Minor: Missing exhaustive deps inuseMemo.
handleDeleteFileandhandleDuplicateFileare referenced but the lint-disable comment suggests this is intentional. Consider restructuring to useuseCallbackfor these handlers with proper dependencies if the lint warning becomes problematic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_molecules/project-tree/index.tsx` around lines 728 - 729, The memoized value using useMemo references handleDeleteFile and handleDuplicateFile (and setIsEditing) while suppressing exhaustive-deps; to fix, convert those handlers into stable functions using useCallback (e.g., wrap handleDeleteFile and handleDuplicateFile in useCallback with their proper dependencies) so they can be safely listed in the useMemo dependency array, then include handleDeleteFile, handleDuplicateFile, and setIsEditing in the useMemo deps instead of disabling the lint rule; update any callers to use the new callbacks to ensure referential stability.src/frontend/store/slices/version-control/slice.ts (1)
37-37: Redundant object initialization.The
initialState(lines 10-13) already definesbaselineContent: {},rawLoadedContent: {}, andloadedSerialized: {}. The spread here duplicates those empty objects unnecessarily.Suggested simplification
- versionControl: { ...initialState, baselineContent: {}, rawLoadedContent: {}, loadedSerialized: {} }, + versionControl: { ...initialState },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/version-control/slice.ts` at line 37, The versionControl slice initialization redundantly redefines baselineContent, rawLoadedContent and loadedSerialized after spreading initialState; remove those duplicated properties so that versionControl is simply initialized as { ...initialState } (leave any other overrides if needed) and rely on the baselineContent/rawLoadedContent/loadedSerialized definitions already present in initialState to avoid duplicate empty object literals.src/frontend/services/save-actions.ts (1)
10-22: Consider using@root/*path alias for imports.Per coding guidelines,
@root/*path alias is preferred over relative paths to./src/*. The imports here use relative paths like'../../middleware/shared/ports/project-port'.Example conversion
-import type { ProjectPort, RawProjectFile, WriteProjectFiles } from '../../middleware/shared/ports/project-port' -import type { PLCPou } from '../../middleware/shared/ports/types' -import { openPLCStoreBase } from '../store' +import type { ProjectPort, RawProjectFile, WriteProjectFiles } from '@root/middleware/shared/ports/project-port' +import type { PLCPou } from '@root/middleware/shared/ports/types' +import { openPLCStoreBase } from '@root/frontend/store'As per coding guidelines: "Prefer path alias
@root/*for imports instead of relative paths to./src/*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/services/save-actions.ts` around lines 10 - 22, Replace the relative ../../ imports in src/frontend/services/save-actions.ts with the `@root/`* path alias for each module referenced (e.g., replace imports that bring in ProjectPort, RawProjectFile, WriteProjectFiles, PLCPou, openPLCStoreBase, LadderFlowType, parseIecStringToVariables, generateIecVariablesToString, syncNodesWithVariables, getExtensionFromLanguage, parseGraphicalPouFromString, serializePouToText, collectDebugVariables, toast, pickContentForSave, etc.) so they use the project path-alias (e.g., `@root/middleware/shared/ports/project-port`, `@root/frontend/store`, `@root/frontend/utils/`...), update any affected import lines in save-actions.ts accordingly, and run TypeScript build/IDE typecheck to confirm the tsconfig.paths mapping is correct.src/frontend/components/_features/[workspace]/branches/create-branch-popover.tsx (1)
4-6: Prefer@root/*aliases instead of deep relative imports.These imports are brittle and violate the repository import convention.
Suggested patch
-import { useVersionControl } from '../../../../../middleware/shared/providers' -import { cn } from '../../../../utils/cn' -import { getBranchNameFeedback } from '../../../../utils/sanitize-branch-name' +import { useVersionControl } from '@root/middleware/shared/providers' +import { cn } from '@root/frontend/utils/cn' +import { getBranchNameFeedback } from '@root/frontend/utils/sanitize-branch-name'As per coding guidelines: "Prefer path alias
@root/*for imports instead of relative paths to./src/*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/branches/create-branch-popover.tsx around lines 4 - 6, Replace the brittle deep relative imports in create-branch-popover.tsx with project path aliases: import useVersionControl from the provider via "@root/middleware/shared/providers" (referencing useVersionControl), import cn from "@root/utils/cn" (referencing cn), and import getBranchNameFeedback from "@root/utils/sanitize-branch-name" (referencing getBranchNameFeedback); update the three import statements at the top of the file accordingly so they use the `@root/`* aliases instead of the long ../../../../../ paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/branches/create-branch-popover.tsx:
- Around line 154-155: In the CreateBranchPopover component
(create-branch-popover.tsx) remove the literal markdown markers from the helper
copy by replacing the string that currently reads "** Spaces and special
characters are not allowed" (the JSX span rendering the helper text) with
"Spaces and special characters are not allowed" so the text renders cleanly
without the stray "**".
- Around line 40-177: Add i18n by importing and calling useTranslation() in the
CreateBranchPopover component, replace all hardcoded user-facing strings (e.g.,
"Create new branch", "New Branch", "Branch name:", placeholder
"feature/my-branch", button texts "Cancel"/"Create"/"Creating...", helper text
"** Spaces and special characters are not allowed", and error messages set via
setError(...) and feedback messages) with t('your.key.path') lookups, and ensure
dynamic messages still use interpolation (e.g., t('willBeCreatedAs', { name:
feedback.sanitized })). Update handleSubmit, handleCancel and any JSX spans that
render error/feedback to use t(...) keys, and add corresponding keys to the
project's locale JSON files so translations exist.
In `@src/frontend/utils/sanitize-branch-name.ts`:
- Around line 36-57: sanitizeBranchName still allows invalid Git ref patterns
like repeated slashes ("foo//bar") and internal ".lock" segments
("topic.lock/name"); update sanitizeBranchName to normalize runs of '/'
(collapse multiple '/' into a single '/') and to remove or sanitize any path
segment that equals or ends with ".lock" (case-insensitive) before the final
trim so segments like "foo//bar" and "topic.lock/name" are converted to valid
refs; apply these fixes near the existing collapsing/trimming steps (after
collapsing '-' and before final strip of leading/trailing characters) and ensure
you collapse runs of '/' and inspect each path segment to strip trailing ".lock"
or replace problematic segments with a safe token.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/branches/create-branch-popover.tsx:
- Around line 4-6: Replace the brittle deep relative imports in
create-branch-popover.tsx with project path aliases: import useVersionControl
from the provider via "@root/middleware/shared/providers" (referencing
useVersionControl), import cn from "@root/utils/cn" (referencing cn), and import
getBranchNameFeedback from "@root/utils/sanitize-branch-name" (referencing
getBranchNameFeedback); update the three import statements at the top of the
file accordingly so they use the `@root/`* aliases instead of the long
../../../../../ paths.
In `@src/frontend/components/_molecules/project-tree/index.tsx`:
- Around line 728-729: The memoized value using useMemo references
handleDeleteFile and handleDuplicateFile (and setIsEditing) while suppressing
exhaustive-deps; to fix, convert those handlers into stable functions using
useCallback (e.g., wrap handleDeleteFile and handleDuplicateFile in useCallback
with their proper dependencies) so they can be safely listed in the useMemo
dependency array, then include handleDeleteFile, handleDuplicateFile, and
setIsEditing in the useMemo deps instead of disabling the lint rule; update any
callers to use the new callbacks to ensure referential stability.
In `@src/frontend/services/save-actions.ts`:
- Around line 10-22: Replace the relative ../../ imports in
src/frontend/services/save-actions.ts with the `@root/`* path alias for each
module referenced (e.g., replace imports that bring in ProjectPort,
RawProjectFile, WriteProjectFiles, PLCPou, openPLCStoreBase, LadderFlowType,
parseIecStringToVariables, generateIecVariablesToString, syncNodesWithVariables,
getExtensionFromLanguage, parseGraphicalPouFromString, serializePouToText,
collectDebugVariables, toast, pickContentForSave, etc.) so they use the project
path-alias (e.g., `@root/middleware/shared/ports/project-port`,
`@root/frontend/store`, `@root/frontend/utils/`...), update any affected import
lines in save-actions.ts accordingly, and run TypeScript build/IDE typecheck to
confirm the tsconfig.paths mapping is correct.
In `@src/frontend/store/slices/version-control/slice.ts`:
- Line 37: The versionControl slice initialization redundantly redefines
baselineContent, rawLoadedContent and loadedSerialized after spreading
initialState; remove those duplicated properties so that versionControl is
simply initialized as { ...initialState } (leave any other overrides if needed)
and rely on the baselineContent/rawLoadedContent/loadedSerialized definitions
already present in initialState to avoid duplicate empty object literals.
🪄 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: a20f9a89-175b-424f-a68d-6613eff4dc62
📒 Files selected for processing (13)
src/frontend/components/_features/[workspace]/branches/create-branch-popover.tsxsrc/frontend/components/_features/[workspace]/editor/graphical/index.tsxsrc/frontend/components/_features/[workspace]/source-control/changes-section.tsxsrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/components/_organisms/modals/delete-confirmation-modal.tsxsrc/frontend/services/save-actions.tssrc/frontend/store/slices/project/slice.tssrc/frontend/store/slices/version-control/slice.tssrc/frontend/store/slices/version-control/types.tssrc/frontend/utils/sanitize-branch-name.tssrc/frontend/utils/system-files.tssrc/frontend/utils/version-control-content.tssrc/middleware/shared/ports/project-port.ts
Comment on lines
+40
to
+177
| setError('Version control unavailable') | ||
| return | ||
| } | ||
|
|
||
| setError('') | ||
| setIsPending(true) | ||
| try { | ||
| await versionControl.createBranch(projectId, finalName) | ||
| onCreated?.(finalName) | ||
| setIsOpen(false) | ||
| setName('') | ||
| setError('') | ||
| onCloseParent?.() | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : 'Failed to create branch') | ||
| } finally { | ||
| setIsPending(false) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <Popover.Root | ||
| open={isOpen} | ||
| onOpenChange={(open) => { | ||
| setIsOpen(open) | ||
| if (!open) { | ||
| setName('') | ||
| setError('') | ||
| } | ||
| }} | ||
| > | ||
| <Popover.Trigger className='group w-full rounded-md focus:bg-neutral-100 dark:focus:bg-neutral-900'> | ||
| <div className='relative flex w-full cursor-pointer items-center gap-2 rounded-md px-3 py-[6px] hover:bg-neutral-100 group-aria-[expanded=true]:bg-neutral-100 group-data-[state=open]:bg-neutral-100 dark:hover:bg-neutral-900 dark:group-aria-[expanded=true]:bg-neutral-900 dark:group-data-[state=open]:bg-neutral-900'> | ||
| <svg className='h-3.5 w-3.5 text-neutral-500 dark:text-neutral-400' viewBox='0 0 16 16' fill='currentColor'> | ||
| <path d='M7.75 2a.75.75 0 0 1 .75.75V7h4.25a.75.75 0 0 1 0 1.5H8.5v4.25a.75.75 0 0 1-1.5 0V8.5H2.75a.75.75 0 0 1 0-1.5H7V2.75A.75.75 0 0 1 7.75 2Z' /> | ||
| </svg> | ||
| <span className='flex-1 text-start font-caption text-xs font-normal text-neutral-600 dark:text-neutral-400'> | ||
| Create new branch | ||
| </span> | ||
| <svg className='h-3 w-3 text-neutral-400 dark:text-neutral-500' viewBox='0 0 16 16' fill='currentColor'> | ||
| <path d='M6.22 3.22a.75.75 0 0 1 1.06 0l4.25 4.25a.75.75 0 0 1 0 1.06l-4.25 4.25a.75.75 0 0 1-1.06-1.06L9.94 8 6.22 4.28a.75.75 0 0 1 0-1.06Z' /> | ||
| </svg> | ||
| </div> | ||
| </Popover.Trigger> | ||
| <Popover.Portal> | ||
| <Popover.Content sideOffset={14} alignOffset={-8} align='end' side='right'> | ||
| <div | ||
| className='box flex h-fit w-[225px] flex-col gap-3 rounded-lg bg-white px-3 pb-3 pt-2 dark:bg-neutral-950' | ||
| onMouseDown={(e) => e.stopPropagation()} | ||
| > | ||
| <div className='flex h-8 w-full flex-col items-center justify-between'> | ||
| <div className='flex w-full select-none items-center gap-2'> | ||
| <svg | ||
| className='h-3.5 w-3.5 text-neutral-500 dark:text-neutral-400' | ||
| viewBox='0 0 16 16' | ||
| fill='currentColor' | ||
| > | ||
| <path d='M9.5 3.25a2.25 2.25 0 1 1 3 2.122V6A2.5 2.5 0 0 1 10 8.5H6a1 1 0 0 0-1 1v1.128a2.251 2.251 0 1 1-1.5 0V5.372a2.25 2.25 0 1 1 1.5 0v1.836A2.5 2.5 0 0 1 6 7h4a1 1 0 0 0 1-1v-.628A2.25 2.25 0 0 1 9.5 3.25Zm-6 0a.75.75 0 1 0 1.5 0 .75.75 0 0 0-1.5 0Zm8.25-.75a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5ZM4.25 12a.75.75 0 1 0 0 1.5.75.75 0 0 0 0-1.5Z' /> | ||
| </svg> | ||
| <p className='my-[2px] flex-1 text-start font-caption text-xs font-normal text-neutral-1000 dark:text-neutral-300'> | ||
| New Branch | ||
| </p> | ||
| </div> | ||
| <div className='h-[1px] w-full bg-neutral-200 dark:!bg-neutral-850' /> | ||
| </div> | ||
| <form onSubmit={handleSubmit} className='flex h-fit w-full select-none flex-col gap-3'> | ||
| <div className='flex w-full flex-col'> | ||
| <label | ||
| htmlFor='branch-name' | ||
| className='flex-1 text-start font-caption text-xs font-normal text-neutral-1000 dark:text-neutral-300' | ||
| > | ||
| Branch name: | ||
| {error && <span className='text-red-500'>*</span>} | ||
| </label> | ||
| <input | ||
| id='branch-name' | ||
| type='text' | ||
| value={name} | ||
| onChange={(e) => { | ||
| setName(e.target.value) | ||
| setError('') | ||
| }} | ||
| placeholder='feature/my-branch' | ||
| autoFocus | ||
| disabled={isPending} | ||
| className='mb-1 mt-[6px] h-[30px] w-full rounded-md border border-neutral-100 bg-white px-2 py-2 text-cp-sm font-medium text-neutral-850 outline-none dark:border-brand-medium-dark dark:bg-neutral-950 dark:text-neutral-300' | ||
| /> | ||
| {error ? ( | ||
| <span className='flex-1 text-start font-caption text-cp-xs font-normal text-red-500 opacity-65'> | ||
| * {error} | ||
| </span> | ||
| ) : feedback.error ? ( | ||
| <span className='flex-1 text-start font-caption text-cp-xs font-normal text-red-500 opacity-65'> | ||
| * {feedback.error} | ||
| </span> | ||
| ) : feedback.changed && feedback.sanitized ? ( | ||
| <div className='flex-1'> | ||
| <span className='block break-all text-start font-caption text-cp-xs font-normal text-neutral-1000 opacity-80 dark:text-neutral-300'> | ||
| Will be created as:{' '} | ||
| <span className='font-mono font-medium text-blue-600 dark:text-blue-400'> | ||
| {feedback.sanitized} | ||
| </span> | ||
| </span> | ||
| {feedback.notes.map((note) => ( | ||
| <span | ||
| key={note} | ||
| className='mt-1 block text-start font-caption text-cp-xs font-normal text-neutral-1000 opacity-60 dark:text-neutral-300' | ||
| > | ||
| · {note} | ||
| </span> | ||
| ))} | ||
| </div> | ||
| ) : ( | ||
| <span className='flex-1 text-start font-caption text-cp-xs font-normal text-neutral-1000 opacity-65 dark:text-neutral-300'> | ||
| ** Spaces and special characters are not allowed | ||
| </span> | ||
| )} | ||
| </div> | ||
| <div className='flex w-full justify-between'> | ||
| <Popover.Close asChild> | ||
| <button | ||
| type='button' | ||
| className='h-7 w-[88px] rounded-md bg-neutral-100 font-caption text-cp-sm font-medium !text-neutral-1000 hover:bg-neutral-200 dark:bg-white dark:hover:bg-neutral-100' | ||
| onClick={handleCancel} | ||
| disabled={isPending} | ||
| > | ||
| Cancel | ||
| </button> | ||
| </Popover.Close> | ||
| <button | ||
| type='submit' | ||
| disabled={isPending || !feedback.sanitized || !!feedback.error} | ||
| className={cn( | ||
| 'h-7 w-[88px] rounded-md bg-brand font-caption text-cp-sm font-medium !text-white hover:bg-brand-medium-dark focus:bg-brand-medium disabled:cursor-not-allowed', | ||
| (isPending || !feedback.sanitized || !!feedback.error) && 'opacity-50', | ||
| )} | ||
| > | ||
| {isPending ? 'Creating...' : 'Create'} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/frontend/components/_features/[workspace]/branches/create-branch-popover.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 9882
🏁 Script executed:
# Check if i18next is imported in the file
grep -n "i18next\|useTranslation" src/frontend/components/_features/[workspace]/branches/create-branch-popover.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Search for other branch components to compare patterns
ls -la src/frontend/components/_features/[workspace]/branches/Repository: Autonomy-Logic/openplc-editor
Length of output: 693
🏁 Script executed:
# Check how i18next is used in other frontend components
rg "useTranslation|i18next" src/frontend/components/_features/ -A 2 --max-count 5Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check other branch modal components for i18next usage
grep -n "useTranslation\|i18next" src/frontend/components/_features/[workspace]/branches/create-branch-modal.tsx src/frontend/components/_features/[workspace]/branches/branch-switcher-modal.tsx src/frontend/components/_features/[workspace]/branches/delete-branch-modal.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check for hardcoded strings in CreateBranchModal for comparison
grep -n "Create\|Cancel\|Branch" src/frontend/components/_features/[workspace]/branches/create-branch-modal.tsx | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 741
🏁 Script executed:
# Search across all frontend components for i18next usage patterns
rg "useTranslation" src/frontend/components/ --type tsx --type ts -l | head -10Repository: Autonomy-Logic/openplc-editor
Length of output: 101
🏁 Script executed:
# Search for i18next usage in frontend components
rg "useTranslation" src/frontend/components/ -l 2>/dev/null | head -15Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check if there are any i18n configuration files
find src/frontend -name "*i18n*" -o -name "*translation*" -o -name "*locale*"Repository: Autonomy-Logic/openplc-editor
Length of output: 164
🏁 Script executed:
# Look at one of the files that uses useTranslation to understand the pattern
rg "useTranslation" src/frontend/components/ -B2 -A5 --max-count 10Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Check the i18n configuration
cat src/frontend/locales/i18n.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 459
🏁 Script executed:
# Check the translation.json structure
cat src/frontend/locales/en/translation.json | head -50Repository: Autonomy-Logic/openplc-editor
Length of output: 161
🏁 Script executed:
# Search more broadly for i18next usage in the entire frontend
rg "useTranslation|i18next" src/frontend/ --type js --type ts 2>/dev/null | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 207
🏁 Script executed:
# Search for 'i18next' or 't(' in the codebase
rg "from 'react-i18next'|from \"react-i18next\"" src/frontend/ 2>/dev/nullRepository: Autonomy-Logic/openplc-editor
Length of output: 151
Add i18next localization to this frontend component.
The component hardcodes multiple user-facing strings (Create new branch, New Branch, Branch name:, Cancel, Create, Creating..., error messages, and helper text), violating the i18n requirement in the coding guidelines. The i18next infrastructure is already configured in the project; add useTranslation() and localize all user-facing strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/branches/create-branch-popover.tsx
around lines 40 - 177, Add i18n by importing and calling useTranslation() in the
CreateBranchPopover component, replace all hardcoded user-facing strings (e.g.,
"Create new branch", "New Branch", "Branch name:", placeholder
"feature/my-branch", button texts "Cancel"/"Create"/"Creating...", helper text
"** Spaces and special characters are not allowed", and error messages set via
setError(...) and feedback messages) with t('your.key.path') lookups, and ensure
dynamic messages still use interpolation (e.g., t('willBeCreatedAs', { name:
feedback.sanitized })). Update handleSubmit, handleCancel and any JSX spans that
render error/feedback to use t(...) keys, and add corresponding keys to the
project's locale JSON files so translations exist.
Comment on lines
+154
to
+155
| ** Spaces and special characters are not allowed | ||
| </span> |
Contributor
There was a problem hiding this comment.
Remove the literal ** from helper copy.
The helper text currently renders markdown markers as plain characters.
Suggested patch
- ** Spaces and special characters are not allowed
+ Spaces and special characters are not allowed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/branches/create-branch-popover.tsx
around lines 154 - 155, In the CreateBranchPopover component
(create-branch-popover.tsx) remove the literal markdown markers from the helper
copy by replacing the string that currently reads "** Spaces and special
characters are not allowed" (the JSX span rendering the helper text) with
"Spaces and special characters are not allowed" so the text renders cleanly
without the stray "**".
Comment on lines
+36
to
+57
| // Forbidden sequences. | ||
| result = result.replace(/\.\.+/g, '-') | ||
| result = result.replace(/@\{/g, '-') | ||
|
|
||
| // Strip ASCII control chars (defensive — most are filtered above). | ||
| result = result | ||
| .split('') | ||
| .filter((c) => { | ||
| const code = c.charCodeAt(0) | ||
| return code >= 0x20 && code !== 0x7f | ||
| }) | ||
| .join('') | ||
|
|
||
| // Collapse runs of '-' to a single '-'. | ||
| result = result.replace(/-+/g, '-') | ||
|
|
||
| // Strip leading/trailing '.', '/', '-'. | ||
| result = result.replace(/^[./-]+/, '').replace(/[./-]+$/, '') | ||
|
|
||
| // Strip trailing '.lock' (case-insensitive). | ||
| result = result.replace(/\.lock$/i, '') | ||
|
|
Contributor
There was a problem hiding this comment.
Sanitization still permits Git-invalid ref patterns that can slip through UI validation.
sanitizeBranchName does not normalize repeated / or .lock in non-terminal ref segments (e.g., foo//bar, topic.lock/name). Those can still fail branch creation downstream.
Suggested patch
// Forbidden sequences.
result = result.replace(/\.\.+/g, '-')
result = result.replace(/@\{/g, '-')
+ result = result.replace(/\/{2,}/g, '/')
@@
- // Strip trailing '.lock' (case-insensitive).
- result = result.replace(/\.lock$/i, '')
+ // Strip '.lock' at the end of any ref segment (case-insensitive).
+ result = result.replace(/\.lock(?=\/|$)/gi, '')
+
+ // Re-normalize and trim after segment cleanup.
+ result = result.replace(/\/{2,}/g, '/')
+ result = result.replace(/^[./-]+/, '').replace(/[./-]+$/, '')📝 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.
Suggested change
| // Forbidden sequences. | |
| result = result.replace(/\.\.+/g, '-') | |
| result = result.replace(/@\{/g, '-') | |
| // Strip ASCII control chars (defensive — most are filtered above). | |
| result = result | |
| .split('') | |
| .filter((c) => { | |
| const code = c.charCodeAt(0) | |
| return code >= 0x20 && code !== 0x7f | |
| }) | |
| .join('') | |
| // Collapse runs of '-' to a single '-'. | |
| result = result.replace(/-+/g, '-') | |
| // Strip leading/trailing '.', '/', '-'. | |
| result = result.replace(/^[./-]+/, '').replace(/[./-]+$/, '') | |
| // Strip trailing '.lock' (case-insensitive). | |
| result = result.replace(/\.lock$/i, '') | |
| // Forbidden sequences. | |
| result = result.replace(/\.\.+/g, '-') | |
| result = result.replace(/@\{/g, '-') | |
| result = result.replace(/\/{2,}/g, '/') | |
| // Strip ASCII control chars (defensive — most are filtered above). | |
| result = result | |
| .split('') | |
| .filter((c) => { | |
| const code = c.charCodeAt(0) | |
| return code >= 0x20 && code !== 0x7f | |
| }) | |
| .join('') | |
| // Collapse runs of '-' to a single '-'. | |
| result = result.replace(/-+/g, '-') | |
| // Strip leading/trailing '.', '/', '-'. | |
| result = result.replace(/^[./-]+/, '').replace(/[./-]+$/, '') | |
| // Strip '.lock' at the end of any ref segment (case-insensitive). | |
| result = result.replace(/\.lock(?=\/|$)/gi, '') | |
| // Re-normalize and trim after segment cleanup. | |
| result = result.replace(/\/{2,}/g, '/') | |
| result = result.replace(/^[./-]+/, '').replace(/[./-]+$/, '') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/utils/sanitize-branch-name.ts` around lines 36 - 57,
sanitizeBranchName still allows invalid Git ref patterns like repeated slashes
("foo//bar") and internal ".lock" segments ("topic.lock/name"); update
sanitizeBranchName to normalize runs of '/' (collapse multiple '/' into a single
'/') and to remove or sanitize any path segment that equals or ends with ".lock"
(case-insensitive) before the final trim so segments like "foo//bar" and
"topic.lock/name" are converted to valid refs; apply these fixes near the
existing collapsing/trimming steps (after collapsing '-' and before final strip
of leading/trailing characters) and ensure you collapse runs of '/' and inspect
each path segment to strip trailing ".lock" or replace problematic segments with
a safe token.
Collaborator
dcoutinho1328
left a comment
dcoutinho1328
left a comment
There was a problem hiding this comment.
Code review — version-control PR
This is a structured review with 21 findings. Each inline comment includes a Claude prompt you can paste directly to apply the fix. Findings outside this PR's diff (e.g. use-active-branch.ts) are listed in this summary instead.
Findings not anchored to a diff line
These three findings target lines outside this PR's diff hunks; GitHub won't let me anchor inline comments on them.
1. Cross-tab sync claim in useActiveBranch (src/frontend/hooks/use-active-branch.ts:17-28, file not in this PR's diff)
The comment says "Dispatch storage event so other tabs/components pick up the change", but dispatchEvent(new Event(...)) only fires in the current window. Either add a real storage listener or correct the comment.
In src/frontend/hooks/use-active-branch.ts, the comment claims cross-tab sync but the
implementation only dispatches a same-window CustomEvent. Either:
(a) Add a `storage` listener inside `subscribe` filtering on `event.key === STORAGE_KEY`
to give real cross-tab sync, plus keep the existing same-window event for in-tab updates.
(b) Drop the misleading comment if cross-tab sync isn't actually needed.
2. Diff preview content does not match what gets saved (src/frontend/components/_features/[workspace]/source-control/changes-section.tsx:443–478)
This is a correctness bug, not a smell:
project.jsonpreview:{ name, type, path }(3 fields). Save:{ meta, data: { dataTypes, pous, configuration, debugVariables } }.devices/pin-mapping.jsonpreview:deviceDefinitions.pinMapping. Save:deviceDefinitions.pinMapping.pins.
Users review one diff and commit a different one. The new buildAllProjectFileContentsPure already produces the canonical form — the preview should call it.
src/frontend/components/_features/[workspace]/source-control/changes-section.tsx
`handleFileClick` (lines 443–478) generates preview content with ad-hoc shapes that
don't match what `executeSaveProject` writes. Replace the entire preview content-
resolution branch with a single call to
`buildAllProjectFileContentsPure()[filePath]` (already imported from
src/frontend/services/save-actions.ts). Verify by:
1. Edit a project.json-bearing field (add a data-type), open the changes panel,
click the file — preview must match what the commit produces.
2. Same for devices/configuration.json and devices/pin-mapping.json.
3. Type status lookup tables with the actual union (src/frontend/components/_features/[workspace]/source-control/changes-section.tsx:22–38)
STATUS_LABEL, STATUS_COLOR, STATUS_TOOLTIP are typed Record<string, string> so a typo or new status fails silently to ?? node.status. Use Record<PendingChangeStatus, string> so adding a status fails at compile time.
In src/frontend/components/_features/[workspace]/source-control/changes-section.tsx:22-38,
type STATUS_LABEL, STATUS_COLOR, STATUS_TOOLTIP as
`Record<PendingChangeStatus, string>` (PendingChangeStatus is exported from
middleware/shared/ports/version-control-port). Drop the `?? node.status` fallbacks in
the JSX since the lookup is now exhaustive. If TS complains about indexing with
`node.status` (string) into the typed record, narrow `node.status` via a type guard or
type the FileTreeNode.status field as `PendingChangeStatus`.
Priority
- Ship-blocking-grade: rename auto-save runs unconditionally on the editor (not gated, see
project-tree/index.tsx), preview-vs-save divergence inchanges-section.tsxis a real diff-correctness bug, twouseEffects lost their dep array (every render). - Cleanup before merge: dead code (
project-initializer.ts,initBaseline,activeBranch), 11 neweslint-disable react-hooks/exhaustive-deps, navigation viawindow.location.hrefinstead of TanStack Router.
| source: branch.name, | ||
| target, | ||
| }).toString() | ||
| window.location.href = `/merge?${params}` |
Collaborator
There was a problem hiding this comment.
Use TanStack Router instead of window.location.href
This (and the two equivalents in commit-details.tsx:57,61) forces a full page reload, throwing away workspace state, open tabs, runtime polling, and debug session. The web app uses TanStack Router — call navigate(...) so the SPA stays alive.
Note: commit-details.tsx previously used window.open(..., '_blank') so users could keep their place; this PR silently changes that to in-place navigation. Worth confirming the change was intentional.
Prompt:
In src/frontend/components/_features/[workspace]/branches/branch-status-bar.tsx:92
and src/frontend/components/_features/[workspace]/source-control/commit-details.tsx:57,61,
replace `window.location.href = "..."` with TanStack Router's `navigate(...)` from
`@tanstack/react-router`. Use `useNavigate()` inside the components and pass route +
search params as an object instead of a string URL. For commit-details.tsx, restore the
previous "open in new tab" behavior unless the author intended to change it. Verify the
merge and history routes are registered in the web router.
| // Source is the clicked branch; default target to the active branch | ||
| // (if different) — otherwise leave it blank and let the merge page validate. | ||
| const target = activeBranchName !== branch.name ? activeBranchName : '' | ||
| const params = new URLSearchParams({ |
Collaborator
There was a problem hiding this comment.
Empty target query param when source equals active branch
new URLSearchParams({ target: '' }).toString() emits target= in the URL. Drop the key when it's empty so the merge page can apply its own default.
Prompt:
In src/frontend/components/_features/[workspace]/branches/branch-status-bar.tsx
handleMerge, build URLSearchParams without the `target` key when
`activeBranchName === branch.name`. Use a plain object filtered to non-empty values, or
call `params.delete('target')` after construction. Add a brief test or PR note that the
merge page treats a missing `target` as "let the user pick".
|
|
||
| const initialState: VersionControlSlice['versionControl'] = { | ||
| activePanel: 'explorer', | ||
| activeBranch: null, |
Collaborator
There was a problem hiding this comment.
Drop dead activeBranch slice state
versionControl.activeBranch and setActiveBranch have no readers anywhere — every UI consumer goes through useActiveBranch (localStorage). Pick one source of truth and remove the other.
Prompt:
The version-control slice has dead state: `activeBranch` and `setActiveBranch` (defined
in src/frontend/store/slices/version-control/{slice,types}.ts) are never read outside
the slice. The active source of truth is `useActiveBranch` in
src/frontend/hooks/use-active-branch.ts. Remove `activeBranch` and `setActiveBranch`
from the slice state, types, and initialState. Run the architecture validator
(`npx tsx src/__architecture__/validate.ts`) and tests after.
| ), | ||
|
|
||
| setPendingChangesCount: (count: number) => | ||
| initBaseline: ({ initialPending, baselineContent, rawLoadedContent, loadedSerialized }) => |
Collaborator
There was a problem hiding this comment.
Wire up or delete initBaseline
This action is exposed but never called. The natural call site is right after handleOpenProjectResponse succeeds (so loadedSerialized and rawLoadedContent are populated at load time, not lazily). Either wire it in or remove it.
Prompt:
The `initBaseline` action in src/frontend/store/slices/version-control/slice.ts is dead
code (zero callers in the repo). Wire it up by calling it from
`sharedWorkspaceActions.handleOpenProjectResponse` in src/frontend/store/slices/shared/slice.ts
after the project state is set, passing `rawLoadedFiles` from the project response and
the result of `buildAllProjectFileContentsPure()` as `loadedSerialized`. If wiring it
up is out of scope for this PR, delete the action and its type entirely instead.
| * Used on first load (from router) and on branch switch (without page reload). | ||
| * Set `showLoading: false` to skip the loading overlay (e.g. on branch switch). | ||
| */ | ||
| export function initializeProject(projectData: ProjectState, actions: StoreActions, showLoading = true) { |
Collaborator
There was a problem hiding this comment.
Delete this file (dead code, duplicates handleOpenProjectResponse)
initializeProject is exported but never imported anywhere in the repo. It also duplicates ~80% of sharedWorkspaceActions.handleOpenProjectResponse (store/slices/shared/slice.ts:451), which is what the branch-switch path actually calls in workspace-screen.tsx:248.
Prompt:
Delete src/frontend/services/project-initializer.ts entirely — `initializeProject` has
zero imports anywhere in the repo, and its logic duplicates `handleOpenProjectResponse`
in src/frontend/store/slices/shared/slice.ts. Confirm with `grep -rn initializeProject src/`
that nothing references it before deleting. Sync the deletion to openplc-web.
| * Used as input to `versionControlActions.commitBaseline` so the post-commit | ||
| * baseline matches what was actually on S3 at commit time. | ||
| */ | ||
| export function buildAllProjectFileContents(): Record<string, string> { |
Collaborator
There was a problem hiding this comment.
Consolidate file-serialization helpers (5 duplicates)
The same path → content map is built 5 different ways:
buildAllProjectFileContentsPure(43-83)buildAllProjectFileContents(94-102)collectSavedRecordsForFile(110-173)executeSaveProjectbody (268-325)changes-section.tsxhandleFileClick(443-478)
One canonical iterateProjectFiles(state) + serializeProjectFile(spec, state) would remove the divergence and mechanically fix the preview/save mismatch bug.
Prompt:
Refactor the file-serialization logic in src/frontend/services/save-actions.ts to remove
duplication. Same path → content mapping today exists in 5 places (see PR comment).
Extract:
1. `ProjectFileSpec`: { path: string; content: () => string; type: 'pou' | 'server' | ... }
2. `iterateProjectFiles(state)` generator yielding every spec for a project state.
3. `serializeProjectFile(name, state)` returning the spec for one file.
Rewrite all 5 call sites against those primitives. Confirm:
- buildAllProjectFileContents still applies `pickContentForSave` raw-fallback.
- buildAllProjectFileContentsPure does not.
Run unit tests after. This also fixes the changes-section.tsx preview/save divergence
bug mechanically.
| @@ -46,7 +195,11 @@ function stripGraphicalSelections<T extends { body: { language: string; value: u | |||
| ...rung, | |||
Collaborator
There was a problem hiding this comment.
Fold stripGraphicalSelections into sanitizePou
If sanitizePou is leaving runtime selection state in graphical bodies, fix it there. Layering a second stripper on top is duplication and means future graphical-body changes need updates in two helpers in lockstep.
Prompt:
src/frontend/services/save-actions.ts defines `stripGraphicalSelections` (lines 180-235)
which post-processes graphical POU bodies after `sanitizePou`. Move the LD/FBD selection-
clearing logic into `sanitizePou` itself (in src/frontend/utils/save-project.ts), then
delete `stripGraphicalSelections` and update all call sites to call only `sanitizePou`.
Verify by saving a graphical POU after selecting nodes — the saved file should have
`selected: false` everywhere, and reopening should not show a dirty marker.
| }), | ||
| ), | ||
|
|
||
| commitBaseline: ({ newBaseline, loadedSerialized }) => |
Collaborator
There was a problem hiding this comment.
Unify baseline mutation between recordSavedFiles and commitBaseline
Both rewrite rawLoadedContent and baselineContent with subtly different rules (point-update vs full reset). One canonical helper would prevent drift.
Prompt:
In src/frontend/store/slices/version-control/slice.ts, both `recordSavedFiles` (line 89)
and `commitBaseline` (line 163) mutate `rawLoadedContent` and `baselineContent` with
inconsistent patterns. Extract one helper:
function applySnapshot(
draft: VersionControlSlice,
saved: SavedFileRecord[],
deleted: string[],
): void
That handles all per-path bookkeeping (baselineContent, rawLoadedContent, changedSet,
initialPending). Use it from both actions. Confirm tests still pass and behavior for
the modify-then-save-then-revert-then-save case is preserved.
| * "changes" (e.g. "Spaces will be replaced with '-'"). The CreateBranchPopover | ||
| * uses this to render a live preview without reaching the backend. | ||
| */ | ||
| export function getBranchNameFeedback(input: string): BranchNameFeedback { |
Collaborator
There was a problem hiding this comment.
Single-pass branch-name sanitizer
getBranchNameFeedback runs the full regex chain twice — once via sanitizeBranchName (NFD + 8 passes) and once for the inspection notes (9 more tests, several re-applying NFD).
Prompt:
src/frontend/utils/sanitize-branch-name.ts: `getBranchNameFeedback` (lines 85–124)
duplicates work — calls `sanitizeBranchName` and re-runs nine more regex tests on the
input, several redoing `normalize('NFD')`.
Refactor:
1. Change `sanitizeBranchName` to return
`{ result: string; transforms: TransformKind[] }` where TransformKind is the union
"spaces-replaced" | "non-ascii-stripped" | "diacritics-stripped" | etc.
2. Each step records its kind only if it actually mutated something.
3. `getBranchNameFeedback` consumes `transforms` and maps each kind to its user-facing
note via Record<TransformKind, string>.
4. Add unit tests covering: empty input, only-special-chars, kanji-only, trailing
.lock, leading dot, clean input.
Export a thin wrapper preserving the existing string-in/string-out shape if external
callers still depend on it.
|
|
||
| const tree = useMemo(() => buildChangesTree(files), [files]) | ||
| // System files (e.g. legacy `git-data.tar.gz` from migration) ride along on | ||
| // commits silently — they're never shown, never selectable, never discardable. |
Collaborator
There was a problem hiding this comment.
Track system files separately from visible files
Two parallel arrays (files unfiltered, visibleFiles = files.filter(!isSystemFile)) with implicit invariant "use visibleFiles everywhere except commit time". Cleaner as a single { visible, system } shape.
Prompt:
In src/frontend/components/_features/[workspace]/source-control/changes-section.tsx,
replace the parallel `files` + `visibleFiles` state with a single split state:
const [{ visible, system }, setSplit] = useState<{
visible: PendingChange[]
system: PendingChange[]
}>({ visible: [], system: [] })
Populate in `fetchChanges` with one filter pass. Replace every `visibleFiles` reference
with `visible`, and use `system` directly in `handleCommit` instead of re-filtering
`freshFiles`. Removes the implicit invariant and the duplicate filter passes.
Contributor
Author
PR Review Fixes — version-control branchApplied to both Changes appliedShip-blocking bugs
Dead code removal
Refactors
Typing and UI cleanup
Navigation
Changes NOT applied (with reasons)Switch to
|
… server file serialization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/components/_atoms/generic-table-inputs/generic-combobox-cell.tsx (1)
65-74:⚠️ Potential issue | 🟡 MinorAdd cleanup for the deferred scroll/focus callback.
Line 68 schedules a timeout on each relevant change, but previous timers are never cleared. That can queue stale callbacks during fast open/type/close interactions.
Proposed fix
useEffect(() => { if (selectIsOpen) { // Use setTimeout to ensure DOM is updated before scrolling - setTimeout(() => { + const timeoutId = window.setTimeout(() => { scrollToSelectedOption(true) inputRef.current?.focus() }, 0) + + return () => window.clearTimeout(timeoutId) } // eslint-disable-next-line react-hooks/exhaustive-deps }, [selectIsOpen, inputValue])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_atoms/generic-table-inputs/generic-combobox-cell.tsx` around lines 65 - 74, The effect that runs when selectIsOpen/inputValue changes schedules a setTimeout to call scrollToSelectedOption and focus inputRef but never clears previous timers; modify the useEffect so it stores the timeout id (from setTimeout) in a variable, return a cleanup function that calls clearTimeout on that id, and ensure you still call scrollToSelectedOption(true) and inputRef.current?.focus() in the timeout; this change touches the useEffect that references selectIsOpen, inputValue, scrollToSelectedOption, inputRef and replaces the current anonymous timeout with a cancelable timer handled in the effect's cleanup.
♻️ Duplicate comments (1)
src/frontend/components/_features/[workspace]/branches/branch-status-bar.tsx (1)
82-94:⚠️ Potential issue | 🟠 Major
window.location.hrefcauses full page reload — use TanStack Router.The
handleMergecallback navigates usingwindow.location.href, which forces a full page reload. This discards workspace state (open tabs, runtime polling, debug session). The web app uses TanStack Router — usenavigate()to maintain SPA behavior.The conditional
targetparameter logic (lines 87-90) correctly omits the key when source equals active branch, addressing the previous empty-param concern.♻️ Recommended fix using TanStack Router
+import { useNavigate } from '@tanstack/react-router' + export function BranchStatusBar({ projectId, onBranchSwitch }: BranchStatusBarProps) { const versionControl = useVersionControl() const [activeBranchName, setActiveBranch] = useActiveBranch(projectId) const branchButtonRef = useRef<HTMLButtonElement>(null) + const navigate = useNavigate() // ... other code ... const handleMerge = useCallback( (branch: Branch) => { - const params = new URLSearchParams({ project_id: projectId, source: branch.name }) - if (activeBranchName !== branch.name) { - params.set('target', activeBranchName) - } - window.location.href = `/merge?${params.toString()}` + const search: { project_id: string; source: string; target?: string } = { + project_id: projectId, + source: branch.name, + } + if (activeBranchName !== branch.name) { + search.target = activeBranchName + } + navigate({ to: '/merge', search }) }, - [projectId, activeBranchName], + [projectId, activeBranchName, navigate], ),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/branches/branch-status-bar.tsx around lines 82 - 94, Replace the hard navigation that uses window.location.href in the handleMerge useCallback with TanStack Router navigation: import and call the useNavigate hook to get navigate, then call navigate(`/merge?${params.toString()}`) instead of assigning window.location.href; also add the navigate symbol to the useCallback dependency array so handleMerge depends on [projectId, activeBranchName, navigate] and keep the existing conditional logic that omits the target param when source === activeBranchName.
🧹 Nitpick comments (8)
src/frontend/components/_features/[workspace]/source-control/history-section.tsx (1)
25-52: Branch-aware commit fetching is correctly implemented.The pagination reset effect (lines 25-29) and the fetch effect (lines 31-52) work together to handle branch changes. However, there's a minor inefficiency: when the branch changes, the fetch effect may run twice — once with the stale offset before the reset effect's state update batches, then again with offset=0.
Consider combining the reset and fetch logic into a single effect to avoid the redundant fetch:
♻️ Optional optimization
- // Reset pagination when the active branch changes - useEffect(() => { - setOffset(0) - setSelectedHash(null) - }, [activeBranchName]) - useEffect(() => { if (!versionControl) return - const loading = offset === 0 + // Reset state when branch changes + setOffset(0) + setSelectedHash(null) + const loading = true // Always show loading on branch change or first load if (loading) setIsLoading(true) setIsFetching(true) versionControl - .listCommits(projectId, { limit: PAGE_SIZE, offset, branch: activeBranchName }) + .listCommits(projectId, { limit: PAGE_SIZE, offset: 0, branch: activeBranchName }) // ...rest stays same - }, [projectId, offset, versionControl, activeBranchName]) + }, [projectId, versionControl, activeBranchName])This is a nice-to-have optimization, not a blocker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/source-control/history-section.tsx around lines 25 - 52, Combine the pagination reset and fetch into one effect to avoid a duplicate fetch when activeBranchName changes: replace the separate useEffect that calls setOffset(0)/setSelectedHash(null) and the fetch useEffect with a single useEffect that, when activeBranchName changes, sets offset to 0 and immediately calls versionControl.listCommits(projectId, { limit: PAGE_SIZE, offset: 0, branch: activeBranchName }) (using setIsLoading/setIsFetching and the same .then/.catch/.finally handlers), and otherwise keep the existing fetch behavior for changes to offset/projectId/versionControl; alternatively use a ref for previousActiveBranchName to skip the stale-offset fetch before resetting—refer to setOffset, setSelectedHash, offset, activeBranchName, versionControl.listCommits, setIsLoading and setIsFetching to locate and update the logic.src/frontend/components/_molecules/project-tree/index.tsx (1)
633-674: Duplicate auto-save is not gated byhasVersionControl.Unlike the rename handlers which check
hasVersionControlbefore auto-saving, the duplicate handler always callsexecuteSaveProject(projectPort)(lines 657, 665). This means editor users without version control will also trigger a full project save on every duplicate.This may be intentional — a duplicated file that only exists in memory would be lost on crash, so immediate persistence improves UX. However, for consistency with rename behavior, consider gating this as well:
♻️ Optional: Gate duplicate auto-save
if (isAPou) { duplicatePou(label, `${label}_copy`) - await executeSaveProject(projectPort) + if (hasVersionControl) await executeSaveProject(projectPort) return } if (isDatatype) { duplicateDatatype(label, `${label}_copy`) - await executeSaveProject(projectPort) + if (hasVersionControl) await executeSaveProject(projectPort) return }If the unconditional save is intentional for better data safety, consider documenting this design decision in the code comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_molecules/project-tree/index.tsx` around lines 633 - 674, handleDuplicateFile always calls executeSaveProject(projectPort) after duplicatePou/duplicateDatatype which causes unconditional full-project auto-saves; guard those awaits with the same hasVersionControl check used by the rename handlers (i.e., after calling duplicatePou(label, `${label}_copy`) and duplicateDatatype(label, `${label}_copy`), wrap the executeSaveProject(projectPort) call in if (hasVersionControl) { await executeSaveProject(projectPort) } to preserve consistent behavior, and if the unconditional save was intentional instead add a clarifying comment above the executeSaveProject usages explaining why duplication requires immediate persistence even without version control.src/frontend/utils/save-project.ts (1)
8-8: Consider using path alias for import.As per coding guidelines, prefer
@root/*for imports instead of relative paths to./src/*.-import type { PLCPou } from '../../middleware/shared/ports/types' +import type { PLCPou } from '@root/middleware/shared/ports/types'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/utils/save-project.ts` at line 8, The import in save-project.ts currently uses a relative path for PLCPou; change the import of PLCPou from '../../middleware/shared/ports/types' to use the `@root` path alias (e.g. '@root/middleware/shared/ports/types') so it follows project guidelines, and ensure the project's tsconfig/paths or bundler alias includes that `@root` mapping and then run a quick typecheck/build to verify imports resolve.src/frontend/store/slices/version-control/slice.ts (2)
155-170: UserecomputeCountfor consistency.
commitBaselinedirectly setspendingChangesCount = 0while other actions (initBaseline,recordSavedFiles,syncFromChanges) userecomputeCount(draft). Although the result is the same here (both arrays are empty), using the helper is more consistent and resilient to future changes.draft.versionControl.initialPending = [] draft.versionControl.changedPaths = [] - draft.versionControl.pendingChangesCount = 0 + recomputeCount(draft) }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/version-control/slice.ts` around lines 155 - 170, commitBaseline currently sets draft.versionControl.pendingChangesCount = 0 directly; change it to call the existing helper recomputeCount(draft) for consistency with initBaseline, recordSavedFiles, and syncFromChanges. Locate the commitBaseline producer in the VersionControlSlice and replace the direct assignment to pendingChangesCount with a call to recomputeCount(draft) so the pending count is derived the same way as other actions and remains resilient to future logic changes.
36-36: Redundant object spread in initial state.The
initialStateobject (lines 6-15) already definesbaselineContent: {},rawLoadedContent: {}, andloadedSerialized: {}. Spreading them again here is redundant.- versionControl: { ...initialState, baselineContent: {}, rawLoadedContent: {}, loadedSerialized: {} }, + versionControl: { ...initialState },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/store/slices/version-control/slice.ts` at line 36, The versionControl entry redundantly re-specifies fields already present in initialState; update the slice initializer so versionControl uses the initialState directly (e.g., assign initialState or spread only initialState without re-declaring baselineContent, rawLoadedContent, loadedSerialized) and remove the duplicate baselineContent/rawLoadedContent/loadedSerialized keys to avoid redundancy; locate the use of initialState and the versionControl initializer in the slice to make this change.src/frontend/components/_features/[workspace]/source-control/changes-section.tsx (2)
453-460: Handle missing file gracefully with user feedback.When
buildAllProjectFileContentsPure()[filePath]returnsundefined, the preview silently does nothing. Consider showing a brief toast or fallback message so users know why the preview didn't open.try { const content = buildAllProjectFileContentsPure()[filePath] if (content !== undefined) { setPreviewFile({ path: filePath, content }) + } else { + toast({ title: 'Preview unavailable', description: `Could not generate preview for "${filePath}".`, variant: 'warn' }) } } catch { - // Serialization failed — ignore + toast({ title: 'Preview failed', description: 'Could not serialize file for preview.', variant: 'fail' }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/source-control/changes-section.tsx around lines 453 - 460, The preview silently fails when buildAllProjectFileContentsPure()[filePath] is undefined; update the try block so that if content === undefined you surface a brief user-facing message or fallback state instead of doing nothing — e.g., call your toast/notification helper (useToast/showToast/notify) with a short "File not found" or "Preview unavailable" message, or setPreviewFile to a fallback object indicating the missing file; keep the existing catch for serialization errors to still ignore exceptions.
486-495: Silent early returns leave users without feedback.When the fresh changes list is empty or no valid paths remain after validation, the commit silently aborts. Users may be confused why clicking "Commit" did nothing.
if (freshVisible.length + freshSystem.length === 0) { setIsCommitting(false) + toast({ title: 'Nothing to commit', description: 'No pending changes found.', variant: 'warn' }) return } const validUserPaths = [...selectedFiles].filter((p) => freshVisible.some((f) => f.path === p)) if (validUserPaths.length === 0) { setIsCommitting(false) + toast({ title: 'Selection outdated', description: 'Selected files are no longer pending. Refreshing...', variant: 'warn' }) + await fetchChanges() return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/source-control/changes-section.tsx around lines 486 - 495, The early-return branches in the commit flow silently abort when freshVisible+freshSystem is empty or when validUserPaths is empty; update both branches to setIsCommitting(false) and also surface user feedback (e.g., call the app's toast/notification or set a commit error state) with clear messages like "No changes to commit" and "No selected files are valid for commit" so users know why the commit was cancelled; locate the logic around freshVisible, freshSystem, selectedFiles, validUserPaths and add the notification calls before each return.src/frontend/services/save-actions.ts (1)
14-26: Prefer@root/*path alias for imports.Several imports use deep relative paths (
../../middleware/...,../store,../utils/...). Per coding guidelines, prefer the@root/*alias for better readability and maintainability.-import type { ProjectPort, RawProjectFile, WriteProjectFiles } from '../../middleware/shared/ports/project-port' -import type { PLCPou } from '../../middleware/shared/ports/types' -import { openPLCStoreBase } from '../store' +import type { ProjectPort, RawProjectFile, WriteProjectFiles } from '@root/middleware/shared/ports/project-port' +import type { PLCPou } from '@root/middleware/shared/ports/types' +import { openPLCStoreBase } from '@root/frontend/store'As per coding guidelines: "Prefer path alias
@root/*for imports instead of relative paths to./src/*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/services/save-actions.ts` around lines 14 - 26, Replace deep relative import paths in this file with the project path alias `@root/`*: update imports that reference '../../middleware/shared/ports/project-port' (symbols ProjectPort, RawProjectFile, WriteProjectFiles), '../../middleware/shared/ports/types' (PLCPou), '../store' (openPLCStoreBase, LadderFlowType), and all '../utils/...' modules (parseIecStringToVariables, generateIecVariablesToString, syncNodesWithVariables, syncNodesWithVariablesFBD, getExtensionFromLanguage, getFolderFromPouType, parseGraphicalPouFromString, parseTextualPouFromString, serializePouToText, collectDebugVariables, sanitizePou, toast, pickContentForSave) to use `@root/`... paths instead of relative paths; ensure the tsconfig/webpack path mappings support these aliases and run a build/typecheck after changing imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/branches/branch-switcher-popover.tsx:
- Around line 40-48: The effect that calls versionControl.listBranches in
useEffect can update state after the popover is closed causing a race; make the
request cancellable by adding a local "active" or "cancelled" flag (or use an
AbortController if listBranches accepts a signal) inside the useEffect and check
it before calling setBranches or setIsLoading in the .then/.catch/.finally
handlers; also set the flag in the cleanup function so any late-resolving
promise is ignored when isOpen becomes false or the component unmounts
(referencing useEffect, isOpen, versionControl.listBranches, projectId,
setBranches, setIsLoading).
In `@src/frontend/services/save-actions.ts`:
- Around line 411-415: serializeProjectFile may return an empty specs array but
the code assumes specs[0] later (e.g., in the isPouType branches), so add
explicit handling when specs.length === 0: either invoke the legacy handler
(call the existing legacy save function) or return a clear failure/log message
and exit early. Update the block around serializeProjectFile, specs and the
following isPouType checks to short-circuit on empty specs (log/return or call
the legacy path) so no later code attempts to access specs[0].
In `@src/middleware/shared/ports/version-control-port.ts`:
- Around line 142-149: The flows array type currently declares original and
current as FlowData | null but leaves originalHeight, currentHeight,
originalWidth, and currentWidth as non-nullable numbers; change those four
properties to be nullable (number | null) so their nullability aligns with
original/current (in the flows property definition in version-control-port.ts)
and update any consumers of flows/originalHeight etc. to handle null values
accordingly (check for original/current === null before using corresponding
dimensions).
---
Outside diff comments:
In
`@src/frontend/components/_atoms/generic-table-inputs/generic-combobox-cell.tsx`:
- Around line 65-74: The effect that runs when selectIsOpen/inputValue changes
schedules a setTimeout to call scrollToSelectedOption and focus inputRef but
never clears previous timers; modify the useEffect so it stores the timeout id
(from setTimeout) in a variable, return a cleanup function that calls
clearTimeout on that id, and ensure you still call scrollToSelectedOption(true)
and inputRef.current?.focus() in the timeout; this change touches the useEffect
that references selectIsOpen, inputValue, scrollToSelectedOption, inputRef and
replaces the current anonymous timeout with a cancelable timer handled in the
effect's cleanup.
---
Duplicate comments:
In
`@src/frontend/components/_features/`[workspace]/branches/branch-status-bar.tsx:
- Around line 82-94: Replace the hard navigation that uses window.location.href
in the handleMerge useCallback with TanStack Router navigation: import and call
the useNavigate hook to get navigate, then call
navigate(`/merge?${params.toString()}`) instead of assigning
window.location.href; also add the navigate symbol to the useCallback dependency
array so handleMerge depends on [projectId, activeBranchName, navigate] and keep
the existing conditional logic that omits the target param when source ===
activeBranchName.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/source-control/changes-section.tsx:
- Around line 453-460: The preview silently fails when
buildAllProjectFileContentsPure()[filePath] is undefined; update the try block
so that if content === undefined you surface a brief user-facing message or
fallback state instead of doing nothing — e.g., call your toast/notification
helper (useToast/showToast/notify) with a short "File not found" or "Preview
unavailable" message, or setPreviewFile to a fallback object indicating the
missing file; keep the existing catch for serialization errors to still ignore
exceptions.
- Around line 486-495: The early-return branches in the commit flow silently
abort when freshVisible+freshSystem is empty or when validUserPaths is empty;
update both branches to setIsCommitting(false) and also surface user feedback
(e.g., call the app's toast/notification or set a commit error state) with clear
messages like "No changes to commit" and "No selected files are valid for
commit" so users know why the commit was cancelled; locate the logic around
freshVisible, freshSystem, selectedFiles, validUserPaths and add the
notification calls before each return.
In
`@src/frontend/components/_features/`[workspace]/source-control/history-section.tsx:
- Around line 25-52: Combine the pagination reset and fetch into one effect to
avoid a duplicate fetch when activeBranchName changes: replace the separate
useEffect that calls setOffset(0)/setSelectedHash(null) and the fetch useEffect
with a single useEffect that, when activeBranchName changes, sets offset to 0
and immediately calls versionControl.listCommits(projectId, { limit: PAGE_SIZE,
offset: 0, branch: activeBranchName }) (using setIsLoading/setIsFetching and the
same .then/.catch/.finally handlers), and otherwise keep the existing fetch
behavior for changes to offset/projectId/versionControl; alternatively use a ref
for previousActiveBranchName to skip the stale-offset fetch before
resetting—refer to setOffset, setSelectedHash, offset, activeBranchName,
versionControl.listCommits, setIsLoading and setIsFetching to locate and update
the logic.
In `@src/frontend/components/_molecules/project-tree/index.tsx`:
- Around line 633-674: handleDuplicateFile always calls
executeSaveProject(projectPort) after duplicatePou/duplicateDatatype which
causes unconditional full-project auto-saves; guard those awaits with the same
hasVersionControl check used by the rename handlers (i.e., after calling
duplicatePou(label, `${label}_copy`) and duplicateDatatype(label,
`${label}_copy`), wrap the executeSaveProject(projectPort) call in if
(hasVersionControl) { await executeSaveProject(projectPort) } to preserve
consistent behavior, and if the unconditional save was intentional instead add a
clarifying comment above the executeSaveProject usages explaining why
duplication requires immediate persistence even without version control.
In `@src/frontend/services/save-actions.ts`:
- Around line 14-26: Replace deep relative import paths in this file with the
project path alias `@root/`*: update imports that reference
'../../middleware/shared/ports/project-port' (symbols ProjectPort,
RawProjectFile, WriteProjectFiles), '../../middleware/shared/ports/types'
(PLCPou), '../store' (openPLCStoreBase, LadderFlowType), and all '../utils/...'
modules (parseIecStringToVariables, generateIecVariablesToString,
syncNodesWithVariables, syncNodesWithVariablesFBD, getExtensionFromLanguage,
getFolderFromPouType, parseGraphicalPouFromString, parseTextualPouFromString,
serializePouToText, collectDebugVariables, sanitizePou, toast,
pickContentForSave) to use `@root/`... paths instead of relative paths; ensure the
tsconfig/webpack path mappings support these aliases and run a build/typecheck
after changing imports.
In `@src/frontend/store/slices/version-control/slice.ts`:
- Around line 155-170: commitBaseline currently sets
draft.versionControl.pendingChangesCount = 0 directly; change it to call the
existing helper recomputeCount(draft) for consistency with initBaseline,
recordSavedFiles, and syncFromChanges. Locate the commitBaseline producer in the
VersionControlSlice and replace the direct assignment to pendingChangesCount
with a call to recomputeCount(draft) so the pending count is derived the same
way as other actions and remains resilient to future logic changes.
- Line 36: The versionControl entry redundantly re-specifies fields already
present in initialState; update the slice initializer so versionControl uses the
initialState directly (e.g., assign initialState or spread only initialState
without re-declaring baselineContent, rawLoadedContent, loadedSerialized) and
remove the duplicate baselineContent/rawLoadedContent/loadedSerialized keys to
avoid redundancy; locate the use of initialState and the versionControl
initializer in the slice to make this change.
In `@src/frontend/utils/save-project.ts`:
- Line 8: The import in save-project.ts currently uses a relative path for
PLCPou; change the import of PLCPou from '../../middleware/shared/ports/types'
to use the `@root` path alias (e.g. '@root/middleware/shared/ports/types') so it
follows project guidelines, and ensure the project's tsconfig/paths or bundler
alias includes that `@root` mapping and then run a quick typecheck/build to verify
imports resolve.
🪄 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: 7c686c84-7dd9-4064-a4a3-41aad174c32b
📒 Files selected for processing (26)
src/frontend/components/_atoms/generic-table-inputs/generic-combobox-cell.tsxsrc/frontend/components/_atoms/generic-table-inputs/generic-select-cell.tsxsrc/frontend/components/_features/[workspace]/branches/branch-status-bar.tsxsrc/frontend/components/_features/[workspace]/branches/branch-switcher-popover.tsxsrc/frontend/components/_features/[workspace]/branches/index.tssrc/frontend/components/_features/[workspace]/editor/device/configuration/communication.tsxsrc/frontend/components/_features/[workspace]/editor/device/configuration/components/modbus-rtu.tsxsrc/frontend/components/_features/[workspace]/editor/device/remote-device/index.tsxsrc/frontend/components/_features/[workspace]/source-control/changes-section.tsxsrc/frontend/components/_features/[workspace]/source-control/history-section.tsxsrc/frontend/components/_molecules/data-types/array/index.tsxsrc/frontend/components/_molecules/data-types/array/table/index.tsxsrc/frontend/components/_molecules/data-types/enumerated/index.tsxsrc/frontend/components/_molecules/graphical-editor/fbd/fbd-utils/useCopyPaste.tssrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/components/_molecules/variables-table/editable-cell.tsxsrc/frontend/components/_organisms/global-variables-editor/index.tsxsrc/frontend/components/_organisms/variables-editor/index.tsxsrc/frontend/hooks/use-active-branch.tssrc/frontend/screens/workspace-screen.tsxsrc/frontend/services/save-actions.tssrc/frontend/store/slices/version-control/slice.tssrc/frontend/store/slices/version-control/types.tssrc/frontend/utils/sanitize-branch-name.tssrc/frontend/utils/save-project.tssrc/middleware/shared/ports/version-control-port.ts
✅ Files skipped from review due to trivial changes (9)
- src/frontend/components/_features/[workspace]/editor/device/configuration/components/modbus-rtu.tsx
- src/frontend/components/_molecules/data-types/enumerated/index.tsx
- src/frontend/components/_molecules/data-types/array/table/index.tsx
- src/frontend/components/_atoms/generic-table-inputs/generic-select-cell.tsx
- src/frontend/components/_organisms/variables-editor/index.tsx
- src/frontend/components/_organisms/global-variables-editor/index.tsx
- src/frontend/components/_features/[workspace]/editor/device/configuration/communication.tsx
- src/frontend/components/_molecules/data-types/array/index.tsx
- src/frontend/components/_molecules/graphical-editor/fbd/fbd-utils/useCopyPaste.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend/utils/sanitize-branch-name.ts
Comment on lines
+40
to
+48
| useEffect(() => { | ||
| if (!isOpen || !versionControl) return | ||
| setIsLoading(true) | ||
| versionControl | ||
| .listBranches(projectId) | ||
| .then(({ branches: b }) => setBranches(b)) | ||
| .catch(() => setBranches([])) | ||
| .finally(() => setIsLoading(false)) | ||
| }, [isOpen, projectId, versionControl]) |
Contributor
There was a problem hiding this comment.
Potential race condition on rapid open/close cycles.
If the popover is opened and closed rapidly, the listBranches promise may resolve after isOpen becomes false, updating state on an unmounted or stale component. Consider using an abort controller or a mounted flag.
useEffect(() => {
if (!isOpen || !versionControl) return
+ let cancelled = false
setIsLoading(true)
versionControl
.listBranches(projectId)
- .then(({ branches: b }) => setBranches(b))
- .catch(() => setBranches([]))
- .finally(() => setIsLoading(false))
+ .then(({ branches: b }) => { if (!cancelled) setBranches(b) })
+ .catch(() => { if (!cancelled) setBranches([]) })
+ .finally(() => { if (!cancelled) setIsLoading(false) })
+ return () => { cancelled = true }
}, [isOpen, projectId, versionControl])📝 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.
Suggested change
| useEffect(() => { | |
| if (!isOpen || !versionControl) return | |
| setIsLoading(true) | |
| versionControl | |
| .listBranches(projectId) | |
| .then(({ branches: b }) => setBranches(b)) | |
| .catch(() => setBranches([])) | |
| .finally(() => setIsLoading(false)) | |
| }, [isOpen, projectId, versionControl]) | |
| useEffect(() => { | |
| if (!isOpen || !versionControl) return | |
| let cancelled = false | |
| setIsLoading(true) | |
| versionControl | |
| .listBranches(projectId) | |
| .then(({ branches: b }) => { if (!cancelled) setBranches(b) }) | |
| .catch(() => { if (!cancelled) setBranches([]) }) | |
| .finally(() => { if (!cancelled) setIsLoading(false) }) | |
| return () => { cancelled = true } | |
| }, [isOpen, projectId, versionControl]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/components/_features/`[workspace]/branches/branch-switcher-popover.tsx
around lines 40 - 48, The effect that calls versionControl.listBranches in
useEffect can update state after the popover is closed causing a race; make the
request cancellable by adding a local "active" or "cancelled" flag (or use an
AbortController if listBranches accepts a signal) inside the useEffect and check
it before calling setBranches or setIsLoading in the .then/.catch/.finally
handlers; also set the flag in the cleanup function so any late-resolving
promise is ignored when isOpen becomes false or the component unmounts
(referencing useEffect, isOpen, versionControl.listBranches, projectId,
setBranches, setIsLoading).
Comment on lines
+411
to
+415
| const specs = serializeProjectFile(fileName, file, state) | ||
| if (specs.length === 0) { | ||
| // Some categories (e.g. ethercat-device) need handling that doesn't | ||
| // map to a single fileName lookup, so fall through to the legacy path. | ||
| } |
Contributor
There was a problem hiding this comment.
Dead code path — empty specs falls through without handling.
The comment says "fall through to the legacy path" but there's no legacy handling after the if (specs.length === 0) block. The subsequent if (isPouType) branches all assume specs[0] exists. If specs is empty for an unexpected file type, the function will fail silently or return an incorrect failure.
Either remove the misleading comment or add explicit handling:
const specs = serializeProjectFile(fileName, file, state)
if (specs.length === 0) {
- // Some categories (e.g. ethercat-device) need handling that doesn't
- // map to a single fileName lookup, so fall through to the legacy path.
+ return fail(`Cannot serialize file "${fileName}" of type "${file.type}".`)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/services/save-actions.ts` around lines 411 - 415,
serializeProjectFile may return an empty specs array but the code assumes
specs[0] later (e.g., in the isPouType branches), so add explicit handling when
specs.length === 0: either invoke the legacy handler (call the existing legacy
save function) or return a clear failure/log message and exit early. Update the
block around serializeProjectFile, specs and the following isPouType checks to
short-circuit on empty specs (log/return or call the legacy path) so no later
code attempts to access specs[0].
Comment on lines
+142
to
+149
| flows: { | ||
| original: FlowData | null | ||
| current: FlowData | null | ||
| originalHeight: number | ||
| currentHeight: number | ||
| originalWidth: number | ||
| currentWidth: number | ||
| }[] |
Contributor
There was a problem hiding this comment.
Align dimension nullability with nullable flow sides.
When original or current is null, the corresponding dimensions (Line 145–Line 148) have no valid value. Keeping them as required number forces sentinel values and makes the contract ambiguous.
💡 Suggested type fix
export interface GraphicalDiffResult {
flows: {
original: FlowData | null
current: FlowData | null
- originalHeight: number
- currentHeight: number
- originalWidth: number
- currentWidth: number
+ originalHeight: number | null
+ currentHeight: number | null
+ originalWidth: number | null
+ currentWidth: number | null
}[]📝 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.
Suggested change
| flows: { | |
| original: FlowData | null | |
| current: FlowData | null | |
| originalHeight: number | |
| currentHeight: number | |
| originalWidth: number | |
| currentWidth: number | |
| }[] | |
| flows: { | |
| original: FlowData | null | |
| current: FlowData | null | |
| originalHeight: number | null | |
| currentHeight: number | null | |
| originalWidth: number | null | |
| currentWidth: number | null | |
| }[] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/middleware/shared/ports/version-control-port.ts` around lines 142 - 149,
The flows array type currently declares original and current as FlowData | null
but leaves originalHeight, currentHeight, originalWidth, and currentWidth as
non-nullable numbers; change those four properties to be nullable (number |
null) so their nullability aligns with original/current (in the flows property
definition in version-control-port.ts) and update any consumers of
flows/originalHeight etc. to handle null values accordingly (check for
original/current === null before using corresponding dimensions).
…sion control components
Contributor
Author
PR Review Follow-up — NavigationPortApplied to both Changes appliedNew port:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/components/_features/[workspace]/source-control/commit-details.tsx (1)
4-6: Prefer@root/*alias for shared middleware imports.The deep relative import is harder to maintain; using the alias here will make cross-module imports more stable and readable.
As per coding guidelines, "Prefer path alias `@root/*` for imports instead of relative paths to `./src/*`".♻️ Suggested import cleanup
-import type { Commit, CommitFile } from '../../../../../middleware/shared/ports/version-control-port' -import { useNavigation, useProject, useVersionControl } from '../../../../../middleware/shared/providers' +import type { Commit, CommitFile } from '@root/middleware/shared/ports/version-control-port' +import { useNavigation, useProject, useVersionControl } from '@root/middleware/shared/providers'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_features/`[workspace]/source-control/commit-details.tsx around lines 4 - 6, Replace the deep relative imports in commit-details.tsx with the `@root` path alias: change the import that currently brings in Commit and CommitFile from '../../../../../middleware/shared/ports/version-control-port' to use '@root/middleware/shared/ports/version-control-port' and change the provider imports (useNavigation, useProject, useVersionControl) from '../../../../../middleware/shared/providers' to '@root/middleware/shared/providers' while leaving the useOpenPLCStore import as-is; keep the same named symbols (Commit, CommitFile, useNavigation, useProject, useVersionControl) and ensure there are no other relative paths to middleware shared modules in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/middleware/adapters/editor/navigation-adapter.ts`:
- Around line 27-33: The navigation adapter currently bypasses the editor IPC
bridge by calling window.location.href and window.open directly in navigate and
openInNewWindow; update navigate and openInNewWindow to delegate to the Electron
bridge (use window.bridge.<appropriateMethod>) instead of using browser globals,
passing either the built URL from buildNavigationUrl(path, search) or the
original path and search so the bridge can handle navigation/opening; modify the
calls in navigate and openInNewWindow to call window.bridge.navigate(...) and
window.bridge.openInNewWindow(...) (or the existing bridge method names) with
the same arguments to restore the adapter contract.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/source-control/commit-details.tsx:
- Around line 4-6: Replace the deep relative imports in commit-details.tsx with
the `@root` path alias: change the import that currently brings in Commit and
CommitFile from '../../../../../middleware/shared/ports/version-control-port' to
use '@root/middleware/shared/ports/version-control-port' and change the provider
imports (useNavigation, useProject, useVersionControl) from
'../../../../../middleware/shared/providers' to
'@root/middleware/shared/providers' while leaving the useOpenPLCStore import
as-is; keep the same named symbols (Commit, CommitFile, useNavigation,
useProject, useVersionControl) and ensure there are no other relative paths to
middleware shared modules in this file.
🪄 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: e7d40025-e577-4c3c-ad38-056abeb278ca
📒 Files selected for processing (10)
src/frontend/components/_features/[workspace]/branches/branch-status-bar.tsxsrc/frontend/components/_features/[workspace]/source-control/commit-details.tsxsrc/frontend/services/save-actions.tssrc/middleware/adapters/editor/navigation-adapter.tssrc/middleware/editor-platform.tssrc/middleware/shared/ports/index.tssrc/middleware/shared/ports/navigation-port.tssrc/middleware/shared/providers/index.tssrc/middleware/shared/providers/platform-context.tsxsrc/middleware/shared/providers/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/middleware/shared/ports/navigation-port.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/components/_features/[workspace]/branches/branch-status-bar.tsx
- src/frontend/services/save-actions.ts
Comment on lines
+27
to
+33
| navigate(path: string, search?: NavigationSearch): void { | ||
| window.location.href = buildNavigationUrl(path, search) | ||
| }, | ||
|
|
||
| openInNewWindow(path: string, search?: NavigationSearch): void { | ||
| window.open(buildNavigationUrl(path, search), '_blank') | ||
| }, |
Contributor
There was a problem hiding this comment.
Adapter bypasses the editor IPC bridge contract.
This editor adapter directly uses browser globals instead of delegating through window.bridge.*, which breaks the established adapter pattern for this layer. Please route navigation/open-window behavior through bridge-backed methods.
As per coding guidelines, "Implement editor-specific port adapters in src/middleware/adapters/editor/ by calling window.bridge.* (Electron IPC)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/middleware/adapters/editor/navigation-adapter.ts` around lines 27 - 33,
The navigation adapter currently bypasses the editor IPC bridge by calling
window.location.href and window.open directly in navigate and openInNewWindow;
update navigate and openInNewWindow to delegate to the Electron bridge (use
window.bridge.<appropriateMethod>) instead of using browser globals, passing
either the built URL from buildNavigationUrl(path, search) or the original path
and search so the bridge can handle navigation/opening; modify the calls in
navigate and openInNewWindow to call window.bridge.navigate(...) and
window.bridge.openInNewWindow(...) (or the existing bridge method names) with
the same arguments to restore the adapter contract.
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
3 participants
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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes