Alert for unattached variables#628
Open
Shmerlard wants to merge 2 commits into
Open
Conversation
Contributor
WalkthroughAdded orphaned variable detection to EditableNameCell component. When a variable has no references in the codebase, its name displays in red with a tooltip alert icon indicating "Unused variable". The detection logic analyzes references based on scope, variable class, and debugger state. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (3)
src/renderer/components/_molecules/variables-table/editable-cell.tsx (3)
332-332: Remove empty object fromcn()call.The empty object
{}passed tocn()is unnecessary.🧹 Minor cleanup
- className={cn('h-4 max-w-[380px] overflow-hidden text-ellipsis break-all', {})} + className={cn('h-4 max-w-[380px] overflow-hidden text-ellipsis break-all')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_molecules/variables-table/editable-cell.tsx` at line 332, The cn() call in the JSX (inside the EditableCell component) includes an unnecessary empty object as a second argument; remove the empty object so the className prop uses cn('h-4 max-w-[380px] overflow-hidden text-ellipsis break-all') instead, leaving the rest of the JSX untouched.
334-347: Add accessibility attributes to the warning icon.The
TriangleAlerticon provides important visual feedback, but screen reader users won't receive this information since there's no accessible label. Consider adding anaria-labelor usingsr-onlytext.♿ Proposed accessibility improvement
{isOrphaned && ( <TooltipProvider delayDuration={0}> <Tooltip> <TooltipTrigger asChild> - <div className='flex items-center justify-center'> - <TriangleAlert className='h-4 w-4 text-red-500' /> + <div className='flex items-center justify-center' role='img' aria-label='Unused variable'> + <TriangleAlert className='h-4 w-4 text-red-500' aria-hidden='true' /> </div> </TooltipTrigger> <TooltipContent> <p>Unused variable</p> </TooltipContent> </Tooltip> </TooltipProvider> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_molecules/variables-table/editable-cell.tsx` around lines 334 - 347, The warning icon (TriangleAlert) shown when isOrphaned is true is not accessible to screen readers; update the Tooltip/TooltipTrigger block to provide an accessible label for the icon by either adding an aria-label (e.g., aria-label="Unused variable") and role="img" to the TriangleAlert element or by rendering visually-hidden text (sr-only) alongside the icon inside TooltipTrigger so screen readers announce "Unused variable"; ensure TooltipContent still provides the same message for sighted users and keep the existing visual markup (Tooltip, TooltipTrigger, TooltipContent, TriangleAlert) intact.
263-278: Consider memoization strategy for large variable tables.The
findAllReferencesToVariablecall iterates through POUs and flows on every dependency change. With many variables in the table, this could cause performance degradation since each row computes its orphan status independently.A few observations:
- The dependency on
pous,ladderFlows, andfbdFlowsmeans any edit to any flow will trigger recalculation for all variable rows.isExternalVariablein the dependency array is derived fromvariable?.class, sovariablealone would suffice, though this is harmless.For now this is acceptable, but if performance issues arise, consider lifting the orphan computation to a parent component that batch-processes all variables and passes down the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/_molecules/variables-table/editable-cell.tsx` around lines 263 - 278, The per-row useMemo calling findAllReferencesToVariable (used in isOrphaned) causes repeated expensive scans whenever pous/ladderFlows/fbdFlows change; move the orphan computation out of editable-cell.tsx into a parent (e.g., create computeOrphanStatusForAllVariables or buildVariableOrphanMap) that runs once and returns a map keyed by variable name/id, then pass the boolean down to each row; also simplify dependencies by using variable (not isExternalVariable) where appropriate and memoize the parent computation with useMemo using stable keys (e.g., flows versions/counts, scope, editor.meta.name) so rows only read the precomputed isOrphaned value instead of calling findAllReferencesToVariable themselves.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/_molecules/variables-table/editable-cell.tsx`:
- Line 332: The cn() call in the JSX (inside the EditableCell component)
includes an unnecessary empty object as a second argument; remove the empty
object so the className prop uses cn('h-4 max-w-[380px] overflow-hidden
text-ellipsis break-all') instead, leaving the rest of the JSX untouched.
- Around line 334-347: The warning icon (TriangleAlert) shown when isOrphaned is
true is not accessible to screen readers; update the Tooltip/TooltipTrigger
block to provide an accessible label for the icon by either adding an aria-label
(e.g., aria-label="Unused variable") and role="img" to the TriangleAlert element
or by rendering visually-hidden text (sr-only) alongside the icon inside
TooltipTrigger so screen readers announce "Unused variable"; ensure
TooltipContent still provides the same message for sighted users and keep the
existing visual markup (Tooltip, TooltipTrigger, TooltipContent, TriangleAlert)
intact.
- Around line 263-278: The per-row useMemo calling findAllReferencesToVariable
(used in isOrphaned) causes repeated expensive scans whenever
pous/ladderFlows/fbdFlows change; move the orphan computation out of
editable-cell.tsx into a parent (e.g., create computeOrphanStatusForAllVariables
or buildVariableOrphanMap) that runs once and returns a map keyed by variable
name/id, then pass the boolean down to each row; also simplify dependencies by
using variable (not isExternalVariable) where appropriate and memoize the parent
computation with useMemo using stable keys (e.g., flows versions/counts, scope,
editor.meta.name) so rows only read the precomputed isOrphaned value instead of
calling findAllReferencesToVariable themselves.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/renderer/components/_molecules/variables-table/editable-cell.tsx
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.
Pull request info
References
this PR adds a visual feedback for orphaned variables
resolves #552
Description of the changes proposed
DOD checklist
2026-02-26.11-30-07.mp4
Summary by CodeRabbit