Add scroll indicators to device menu dropdown#736
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request introduces portalling control for UI tooltips and dropdowns, adds filtering for deleted POUs in project file reading, improves main process error handling for extension installation, adds window state checks to prevent operations on destroyed windows, and fixes C++ variable class mapping to include the 'local' class. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
manux81
force-pushed
the
fix/679-scroll-indicator
branch
from
c4e53a2 to
0408808
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/menu.ts (1)
35-37:hasLiveWindow()does not guard against a null/undefinedmainWindow.
this.mainWindowis typed asBrowserWindowand populated in the constructor, so in practice this is fine. However, ifmainWindowwere ever cleared (e.g., after the'closed'event inmain.tssetsmainWindow = null), callingisDestroyed()on it would throw. Since the stated intent of this helper is defensive window-liveness gating, consider making it null-safe to match the broader hardening goal of the PR.🛡️ Proposed fix
- private hasLiveWindow(): boolean { - return !this.mainWindow.isDestroyed() - } + private hasLiveWindow(): boolean { + return !!this.mainWindow && !this.mainWindow.isDestroyed() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/menu.ts` around lines 35 - 37, The hasLiveWindow helper should be null-safe: change its logic in hasLiveWindow to first check that this.mainWindow is not null/undefined before calling isDestroyed (e.g., return this.mainWindow != null && !this.mainWindow.isDestroyed()); update references to mainWindow and isDestroyed so the method safely handles cases where the constructor-created mainWindow may have been cleared (e.g., after the 'closed' event).src/main/modules/ipc/main.ts (1)
828-832: Use Winston logger instead ofconsole.error.The rest of this file uses
logger(Winston) for error reporting. Switchingconsole.errorhere keeps logging consistent and structured.♻️ Proposed fix
handleWindowRebuildMenu = () => { void this.menuBuilder.buildMenu().catch((error) => { - console.error('Error rebuilding application menu:', error) + logger.error('Error rebuilding application menu: ' + getErrorMessage(error)) }) }As per coding guidelines: "Use
Winstonfor structured logging in the main process".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/ipc/main.ts` around lines 828 - 832, In handleWindowRebuildMenu, replace the console.error call in the menuBuilder.buildMenu().catch block with the project's Winston logger (use the same logger instance used elsewhere in this file, e.g., logger.error) and include the error object and a clear context message like "Error rebuilding application menu" so logs remain structured and consistent; update the catch to call logger.error with both the message and the caught error.src/frontend/components/_atoms/select/index.tsx (1)
26-31: Type theportalledprop explicitly rather than leaning on intersection.
ISelectContentPropsextendsComponentPropsWithoutRef<typeof PrimitiveSelect.Content>and addsportalled?: boolean. This works, but mirroring the tooltip file’s intersection style (& { portalled?: boolean }) — or keeping it here — is fine; just make sure theportalledkey is always destructured before...resis spread ontoPrimitiveSelect.Contentso it never leaks as an unknown DOM attribute. Lines 43–44 do that correctly, so this is just a defensive note for future edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/components/_atoms/select/index.tsx` around lines 26 - 31, ISelectContentProps currently relies on ComponentPropsWithoutRef<typeof PrimitiveSelect.Content> and adds portalled?: boolean via intersection; explicitly declare portalled?: boolean in the ISelectContentProps type (so it's clearly owned by this interface) and ensure any usage in Select.Content component destructures portalled (and viewportRef/data-align/data-side) before spreading ...res into PrimitiveSelect.Content to prevent portalled from leaking as an unknown DOM attribute.
🤖 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/frontend/components/_atoms/select/index.tsx`:
- Around line 26-31: ISelectContentProps currently relies on
ComponentPropsWithoutRef<typeof PrimitiveSelect.Content> and adds portalled?:
boolean via intersection; explicitly declare portalled?: boolean in the
ISelectContentProps type (so it's clearly owned by this interface) and ensure
any usage in Select.Content component destructures portalled (and
viewportRef/data-align/data-side) before spreading ...res into
PrimitiveSelect.Content to prevent portalled from leaking as an unknown DOM
attribute.
In `@src/main/menu.ts`:
- Around line 35-37: The hasLiveWindow helper should be null-safe: change its
logic in hasLiveWindow to first check that this.mainWindow is not null/undefined
before calling isDestroyed (e.g., return this.mainWindow != null &&
!this.mainWindow.isDestroyed()); update references to mainWindow and isDestroyed
so the method safely handles cases where the constructor-created mainWindow may
have been cleared (e.g., after the 'closed' event).
In `@src/main/modules/ipc/main.ts`:
- Around line 828-832: In handleWindowRebuildMenu, replace the console.error
call in the menuBuilder.buildMenu().catch block with the project's Winston
logger (use the same logger instance used elsewhere in this file, e.g.,
logger.error) and include the error object and a clear context message like
"Error rebuilding application menu" so logs remain structured and consistent;
update the catch to call logger.error with both the message and the caught
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd74949-58e5-49f7-b38b-62d29d5cf604
📒 Files selected for processing (9)
src/backend/editor/services/project-service/utils/read-project.tssrc/frontend/components/_atoms/graphical-editor/fbd/block.tsxsrc/frontend/components/_atoms/select/index.tsxsrc/frontend/components/_atoms/tooltip/index.tsxsrc/frontend/components/_molecules/global-variables-table/selectable-cell.tsxsrc/frontend/components/_molecules/variables-table/selectable-cell.tsxsrc/main/main.tssrc/main/menu.tssrc/main/modules/ipc/main.ts
manux81
changed the title
1 participant
Fixes #679. Adds scroll up/down affordances to Select dropdowns so long device lists clearly indicate they can be scrolled.