refactor: streamline component props and improve readability - #3
Conversation
- Refactored StatusBar to use a utility function for counting active tunnels. - Updated TabBar to use Readonly types for props and consolidated tab icon rendering logic. - Simplified DisconnectedOverlay by extracting reconnect label logic into a separate function. - Introduced StatusDot component in HostHeader for better status representation. - Enhanced SnippetRunner with a VarInput component for variable input handling. - Improved TerminalSearchBar by extracting results label logic into a dedicated function. - Added cooldown management in TerminalView for connection retries. - Resolved SSH credentials in a separate function for clarity. - Refactored TunnelsView to improve toggle icon logic and component structure. - Updated UnlockScreen components to use Readonly types for props. - Enhanced color generation logic in colors.ts for better performance. - Improved memory formatting in format.ts for better type handling. - Simplified store logic in index.ts for session type handling and tab view mapping.
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR hardens RDP transport and diagnostics, adds parameterized database queries, refactors IPC and renderer components, improves validation and accessibility behavior, expands automated coverage, and enables Vitest coverage reporting in CI. ChangesRDP transport and session diagnostics
IPC service and contract changes
Renderer behavior and composition
Test infrastructure and CI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/renderer/src/components/Settings/Settings.tsx (2)
1196-1202: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
handleConfirmcan get stuck in "Clearing…" forever on failure.
await onConfirm()has no try/catch/finally. If it rejects,clearingnever resets tofalse, and the buttons stay disabled with no way to retry, dismiss, or see an error — for a destructive credential-wipe action.As per coding guidelines, renderer code should "catch errors from IPC and surface them to the user via the notification system or inline error state."
🛡️ Proposed fix
const handleConfirm = async () => { setClearing(true) - await onConfirm() + try { + await onConfirm() + } catch (err: any) { + setClearing(false) + // surface err to the user, e.g. via a local error state or addNotification + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Settings/Settings.tsx` around lines 1196 - 1202, Update ClearCredentialsModal.handleConfirm to catch failures from onConfirm, reset clearing to false in a finally block so the modal remains usable, and surface the caught error through the renderer’s notification system or an inline error state. Preserve the successful confirmation flow while allowing retry or dismissal after failure.Source: Coding guidelines
1017-1047: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent submit timing between button and keyboard input for PIN entry.
The button path (
pressKey) now delaysonSubmitby 80ms viasetTimeouton the 4th digit, but the keyboard handler submits immediately with no delay — a newly introduced divergence between two code paths implementing the same digit-collection logic.♻️ Suggested consolidation
Route the keyboard handler's digit-entry logic through the same
pressKeyfunction instead of duplicating it, so both paths share identical timing:useEffect(() => { const handler = (e: KeyboardEvent) => { if (loading) return - if (/^\d$/.test(e.key) && digits.length < 4) { - const next = [...digits, e.key] - setDigits(next) - if (next.length === 4) { onSubmit(next.join('')); setDigits([]) } - } - if (e.key === 'Backspace') setDigits(d => d.slice(0, -1)) + if (/^\d$/.test(e.key)) pressKey(e.key) + if (e.key === 'Backspace') pressKey('⌫') } window.addEventListener('keydown', handler) return () => window.removeEventListener('keydown', handler) }, [digits, loading])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Settings/Settings.tsx` around lines 1017 - 1047, Unify PIN digit handling so keyboard and button input use the same submission timing. In VerifyPinInput, route numeric keyboard events through pressKey instead of duplicating digit collection and immediate submission logic, while retaining Backspace handling separately.src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx (1)
137-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMariaDB gets the wrong default port.
databasereturns3306only formysql; MariaDB (a supporteddbType, per theDatabaseFieldsoptions andDB_TYPE_LABELS) falls through to5432. MariaDB listens on3306.🐛 Proposed fix
- case 'database': return form.dbType === 'mysql' ? '3306' : '5432' + case 'database': return form.dbType === 'postgresql' ? '5432' : '3306'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx` around lines 137 - 146, Update getDefaultPort in AddConnectionModal so the database case returns 3306 for both mysql and mariadb, while retaining 5432 for other database types.
🧹 Nitpick comments (6)
src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx (1)
317-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid
anyin these new signatures.
k8sSessionData()/sessionData()returnany,persistSession(data: any), and the extracted components (PasswordInput,SshFields,DatabaseFields,K8sContextForm,ConfigForm) all typeform: any. This drops type safety across the save path (e.g. a typo in a session field would go unnoticed). Consider defining aSessionData/FormStatetype and threading it through. As per coding guidelines: "Noanyin function signatures. Useunknownand narrow, or define a proper type."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx` around lines 317 - 363, The new save-path signatures use any, removing type safety across session creation and extracted form components. Define explicit SessionData and FormState types based on the supported connection fields, then apply them to k8sSessionData, sessionData, persistSession, and the form props for PasswordInput, SshFields, DatabaseFields, K8sContextForm, and ConfigForm; use appropriate narrowing or unknown where needed instead of any.Source: Coding guidelines
src/renderer/src/components/Database/DatabaseExplorer.tsx (1)
530-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
unknownoveranyfor dynamic cell values.
ResultCell/RowDetailPanel(row: any) andSmartCell/JsonCell(value/parsed: any) type genuinely-dynamic DB values asany, which suppresses narrowing everywhere they flow. As per coding guidelines ("Noanyin function signatures. Useunknownand narrow, or define a proper type"),unknownfits these cases and forces explicit checks (which the code already does via== null/typeof).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Database/DatabaseExplorer.tsx` around lines 530 - 654, Replace any in the function signatures of ResultCell, RowDetailPanel, SmartCell, JsonCell, and tryParseJsonCell with unknown, including row/value/parsed parameters and return types as appropriate. Add explicit narrowing for property access, object checks, JSON parsing, Object.keys, and rendering so dynamic database values remain type-safe without any.Source: Coding guidelines
src/main/ipc/tunnels.ts (1)
139-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on
forwardOut— a hung SSH server leaves the SOCKS connection open indefinitely.
entry.conn.client.forwardOuthas no timeout guard; if the SSH server never invokes the callback, the accepted local socket (and its OS resources) stay open forever.🔒️ Proposed fix to add a timeout guard
function handleSocksConnect(entry: ActiveTunnel, socket: Socket, request: Buffer): void { const parsed = parseSocksConnectRequest(request) if ('errorCode' in parsed) { socket.end(socksConnectReply(parsed.errorCode)) return } + const timeout = setTimeout(() => { + socket.end(socksConnectReply(SOCKS_REPLY.connectionRefused)) + }, 15_000) entry.conn.client.forwardOut( socket.remoteAddress ?? '127.0.0.1', socket.remotePort ?? 0, parsed.host, parsed.port, (err, stream) => { + clearTimeout(timeout) if (err) { socket.end(socksConnectReply(SOCKS_REPLY.connectionRefused)); return } socket.write(socksConnectReply(SOCKS_REPLY.success)) pipeBoth(socket, stream) }, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/tunnels.ts` around lines 139 - 172, Add a timeout guard around the forwardOut call in handleSocksConnect: start a timer before invoking entry.conn.client.forwardOut, clear it when the callback runs, and on expiry close the local socket and return a SOCKS connection-refused reply. Ensure cleanup is safe if the callback arrives after timeout, preventing duplicate replies or pipe setup.src/renderer/src/components/SFTP/SftpBrowser.tsx (1)
436-476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
DIFF_BGandDIFF_DOTcolor constants tolib/colors.ts.These new constants hardcode hex colors (
#10B981,#8B5CF6,#F59E0B) which violates the guideline: "Never hardcode hex colors outsidelib/colors.ts." The same colors appear inline elsewhere in the file, so centralizing them is a step in the right direction, but they should ultimately live inlib/colors.ts.As per coding guidelines:
src/renderer/**/*.{tsx,css}: Use CSS custom properties (var(--nox-*)) for theme colors. Never hardcode hex colors outsidelib/colors.ts.♻️ Suggested refactor
Move the constants to
lib/colors.tsand import them:-const DIFF_BG: Record<string, string> = { - 'local-only': 'rgba(16,185,129,0.06)', 'remote-only': 'rgba(139,92,246,0.06)', different: 'rgba(245,158,11,0.06)', -} -const DIFF_DOT: Record<string, string> = { - 'local-only': '`#10B981`', 'remote-only': '`#8B5CF6`', different: '`#F59E0B`', -} +import { DIFF_BG, DIFF_DOT } from '../../lib/colors'Then export them from
lib/colors.tsalongside existing color definitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SFTP/SftpBrowser.tsx` around lines 436 - 476, Move the DIFF_BG and DIFF_DOT color definitions out of SftpBrowser.tsx into lib/colors.ts, export them there, and import them in SftpBrowser.tsx. Replace the remaining inline diff-related hex colors in FileRow with the centralized color constants or theme CSS variables, preserving the existing visual behavior.Source: Coding guidelines
src/renderer/src/components/Runner/RunnerView.tsx (1)
34-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid calling
setRunninginside thesetResultsupdater.State updater functions should be pure. Calling
setRunninginside thesetResultscallback is a side effect that violates React's purity contract — in Strict Mode, the updater is double-invoked, sosetRunningfires twice (harmless here but fragile). Extract the running computation into auseEffectthat watchesresults.♻️ Proposed refactor
const applyDone = (sessionId: string, exitCode: number | null, error: string | null) => { setResults(prev => { const next = new Map(prev) const r = next.get(sessionId) if (r) { next.set(sessionId, { ...r, state: error || exitCode !== 0 ? 'failed' : 'done', exitCode, error, }) } return next }) - setRunning([...next.values()].some(v => v.state === 'running')) } +// Derive `running` from `results` instead of mutating it inside an updater. +useEffect(() => { + setRunning([...results.values()].some(v => v.state === 'running')) +}, [results])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Runner/RunnerView.tsx` around lines 34 - 49, Move the setRunning call out of the setResults updater in applyDone. Add a useEffect that watches results and derives whether any entry remains in the running state, then updates running accordingly; keep the setResults updater pure by only cloning, updating, and returning the map.src/renderer/src/components/ConnectionManager/ConnectionManager.tsx (1)
11-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
[k: string]: anyinTypeIconprops with a proper SVG props type.The
[k: string]: anyindex signature violates the coding guideline: "Noanyin function signatures." UseReact.SVGProps<SVGSVGElement>(orLucidePropsfromlucide-reactif available in this version) to type the spread props.As per coding guidelines:
src/**/*.{ts,tsx}: Noanyin function signatures. Useunknownand narrow, or define a proper type.♻️ Proposed fix
-function TypeIcon({ type, ...props }: Readonly<{ type?: string; [k: string]: any }>) { +function TypeIcon({ type, ...props }: Readonly<{ type?: string } & React.SVGProps<SVGSVGElement>>) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/ConnectionManager/ConnectionManager.tsx` around lines 11 - 27, Replace the any-based index signature in TypeIcon with a proper SVG icon props type, using React.SVGProps<SVGSVGElement> or the available LucideProps type. Preserve the optional type property and ensure the spread props are accepted by all rendered icons without any in the function signature.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/ipc/docker.ts`:
- Around line 63-82: Prevent duplicate docker:logEnd notifications in
attachLogStream by introducing a settled flag or equivalent one-shot
finalization guard shared by the close and error handlers. Only the first
terminal event should delete the stream and send docker:logEnd, preserving the
error message when error occurs before close.
In `@src/main/ipc/tunnels.ts`:
- Line 244: Update disposeAllTunnels to isolate cleanup failures by wrapping
each stopTunnel(id) call in its own try/catch, allowing iteration over
active.keys() to continue when one tunnel fails; preserve or report the caught
error appropriately.
In `@src/renderer/src/components/Database/DatabaseExplorer.tsx`:
- Around line 248-251: Replace the raw SQL construction in the edit handler with
a parameterized database update API: extend the IPC and database-layer methods
behind window.api.database.query to accept bound values, and use placeholders
for newVal and pkVal. Ensure table and column identifiers are escaped through
the database driver or a dedicated identifier-quoting helper rather than
interpolation, then update the renderer call accordingly.
In `@src/renderer/src/components/Docker/DockerDashboard.tsx`:
- Around line 262-273: Replace the hardcoded `#F59E0B` value in
ContainerTableRow’s stateColor initialization with the appropriate --nox-* CSS
custom property, keeping the existing running and exited state colors unchanged.
In `@src/renderer/src/components/K8s/ResourceDetailModal.tsx`:
- Around line 13-28: colorize must HTML-escape the entire raw JSON string before
applying token highlighting, so unmatched resource content cannot reach
dangerouslySetInnerHTML as markup. Update colorize and preserve token
matching/color spans against the escaped text, including escaping &, <, >, ",
and apostrophes as appropriate; ensure the related usage in ResourceDetailModal
renders only the escaped, colorized output.
In `@src/renderer/src/components/TabBar/TabBar.tsx`:
- Around line 142-160: Add explicit VIEW_ICONS mappings for the 'tunnels' and
'runner' tab views in TabBar.tsx, selecting suitable icons and active colors
consistent with the existing entries so tabIcon does not fall back to Terminal.
In `@src/renderer/src/components/Terminal/TerminalView.tsx`:
- Around line 621-623: Update readKeyFile to catch the error with a named
variable and explicitly handle it, preserving diagnostic context by logging or
otherwise reporting the underlying read failure before returning undefined; do
not use a bare catch block.
---
Outside diff comments:
In `@src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx`:
- Around line 137-146: Update getDefaultPort in AddConnectionModal so the
database case returns 3306 for both mysql and mariadb, while retaining 5432 for
other database types.
In `@src/renderer/src/components/Settings/Settings.tsx`:
- Around line 1196-1202: Update ClearCredentialsModal.handleConfirm to catch
failures from onConfirm, reset clearing to false in a finally block so the modal
remains usable, and surface the caught error through the renderer’s notification
system or an inline error state. Preserve the successful confirmation flow while
allowing retry or dismissal after failure.
- Around line 1017-1047: Unify PIN digit handling so keyboard and button input
use the same submission timing. In VerifyPinInput, route numeric keyboard events
through pressKey instead of duplicating digit collection and immediate
submission logic, while retaining Backspace handling separately.
---
Nitpick comments:
In `@src/main/ipc/tunnels.ts`:
- Around line 139-172: Add a timeout guard around the forwardOut call in
handleSocksConnect: start a timer before invoking entry.conn.client.forwardOut,
clear it when the callback runs, and on expiry close the local socket and return
a SOCKS connection-refused reply. Ensure cleanup is safe if the callback arrives
after timeout, preventing duplicate replies or pipe setup.
In `@src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx`:
- Around line 317-363: The new save-path signatures use any, removing type
safety across session creation and extracted form components. Define explicit
SessionData and FormState types based on the supported connection fields, then
apply them to k8sSessionData, sessionData, persistSession, and the form props
for PasswordInput, SshFields, DatabaseFields, K8sContextForm, and ConfigForm;
use appropriate narrowing or unknown where needed instead of any.
In `@src/renderer/src/components/ConnectionManager/ConnectionManager.tsx`:
- Around line 11-27: Replace the any-based index signature in TypeIcon with a
proper SVG icon props type, using React.SVGProps<SVGSVGElement> or the available
LucideProps type. Preserve the optional type property and ensure the spread
props are accepted by all rendered icons without any in the function signature.
In `@src/renderer/src/components/Database/DatabaseExplorer.tsx`:
- Around line 530-654: Replace any in the function signatures of ResultCell,
RowDetailPanel, SmartCell, JsonCell, and tryParseJsonCell with unknown,
including row/value/parsed parameters and return types as appropriate. Add
explicit narrowing for property access, object checks, JSON parsing,
Object.keys, and rendering so dynamic database values remain type-safe without
any.
In `@src/renderer/src/components/Runner/RunnerView.tsx`:
- Around line 34-49: Move the setRunning call out of the setResults updater in
applyDone. Add a useEffect that watches results and derives whether any entry
remains in the running state, then updates running accordingly; keep the
setResults updater pure by only cloning, updating, and returning the map.
In `@src/renderer/src/components/SFTP/SftpBrowser.tsx`:
- Around line 436-476: Move the DIFF_BG and DIFF_DOT color definitions out of
SftpBrowser.tsx into lib/colors.ts, export them there, and import them in
SftpBrowser.tsx. Replace the remaining inline diff-related hex colors in FileRow
with the centralized color constants or theme CSS variables, preserving the
existing visual behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 169b2a7c-6a0b-4427-a0d3-b9f1137923f7
📒 Files selected for processing (58)
native/rdp-spike/sidecar.csrc/main/index.tssrc/main/ipc/__tests__/keychain.test.tssrc/main/ipc/__tests__/security.test.tssrc/main/ipc/__tests__/socks.test.tssrc/main/ipc/database.tssrc/main/ipc/docker.tssrc/main/ipc/k8s.tssrc/main/ipc/keychain.tssrc/main/ipc/localTerminal.tssrc/main/ipc/localfs.tssrc/main/ipc/metrics.tssrc/main/ipc/rdp.tssrc/main/ipc/redis.tssrc/main/ipc/runner.tssrc/main/ipc/security.tssrc/main/ipc/sessionTransfer.tssrc/main/ipc/sessions.tssrc/main/ipc/sftp.tssrc/main/ipc/ssh.tssrc/main/ipc/sshClients.tssrc/main/ipc/sshConfig.tssrc/main/ipc/tunnels.tssrc/renderer/src/App.tsxsrc/renderer/src/components/CommandPalette/CommandPalette.tsxsrc/renderer/src/components/ConnectionManager/AddConnectionModal.tsxsrc/renderer/src/components/ConnectionManager/ConnectionManager.tsxsrc/renderer/src/components/Dashboard/Dashboard.tsxsrc/renderer/src/components/Dashboard/ServerViews.tsxsrc/renderer/src/components/Database/DatabaseExplorer.tsxsrc/renderer/src/components/Docker/DockerDashboard.tsxsrc/renderer/src/components/Docker/DockerLogsModal.tsxsrc/renderer/src/components/Editor/EditorTab.tsxsrc/renderer/src/components/K8s/K8sDashboard.tsxsrc/renderer/src/components/K8s/ResourceDetailModal.tsxsrc/renderer/src/components/K8sIcon.tsxsrc/renderer/src/components/MainContent.tsxsrc/renderer/src/components/Notifications/NotificationHost.tsxsrc/renderer/src/components/RDP/RdpView.tsxsrc/renderer/src/components/Redis/RedisExplorer.tsxsrc/renderer/src/components/Runner/RunnerView.tsxsrc/renderer/src/components/SFTP/FilesDrawer.tsxsrc/renderer/src/components/SFTP/SftpBrowser.tsxsrc/renderer/src/components/ServerContextMenu.tsxsrc/renderer/src/components/Settings/Settings.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/components/StatusBar/StatusBar.tsxsrc/renderer/src/components/TabBar/TabBar.tsxsrc/renderer/src/components/Terminal/DisconnectedOverlay.tsxsrc/renderer/src/components/Terminal/HostHeader.tsxsrc/renderer/src/components/Terminal/SnippetRunner.tsxsrc/renderer/src/components/Terminal/TerminalSearchBar.tsxsrc/renderer/src/components/Terminal/TerminalView.tsxsrc/renderer/src/components/Tunnels/TunnelsView.tsxsrc/renderer/src/components/UnlockScreen.tsxsrc/renderer/src/lib/colors.tssrc/renderer/src/lib/format.tssrc/renderer/src/store/index.ts
…components - Implement tests for HostHeader, SnippetRunner, TerminalSearchBar, TerminalView, and TunnelsView components to ensure proper rendering and functionality. - Enhance MainContent tests to verify view routing and error boundaries. - Introduce UnlockScreen tests for PIN mode functionality. - Add color and format utility tests for consistent behavior. - Extend Zustand store tests to cover session type to tab view mapping. - Update tsconfig and vitest configuration for improved JSX support and coverage reporting.
Deploying noxed with
|
| Latest commit: |
a4b048e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6f725632.noxed.pages.dev |
| Branch Preview URL: | https://fix-sonarqube-rd-1.noxed.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/renderer/src/components/CommandPalette/CommandPalette.tsx (1)
196-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant Escape handling may call
onClosetwice.The overlay-level
onKeyDown(line 197) and the window-levelkeydownlistener (line 166) both handle Escape. When Escape is pressed, both fire, callingonClose()twice. IfonCloseis idempotent this is harmless, but the overlayonKeyDownis redundant since the window listener already covers Escape.♻️ Proposed fix: remove redundant overlay onKeyDown
<div className="fixed inset-0 z-50 flex items-start justify-center pt-[16vh] bg-black/50 backdrop-blur-sm animate-fade-in" onClick={(e) => { if (e.target === e.currentTarget) onClose() }} - onKeyDown={(e) => { if (e.key === 'Escape') onClose() }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/CommandPalette/CommandPalette.tsx` around lines 196 - 199, Remove the redundant Escape key handler from the overlay element’s onKeyDown prop in CommandPalette, leaving the window-level keydown listener responsible for Escape handling; retain the overlay onClick behavior for closing when clicking the backdrop.src/renderer/src/components/ConnectionManager/__tests__/AddConnectionModal.test.tsx (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
anyin function type annotation.Per coding guidelines,
anyshould not be used in function signatures insrc/**/*.{ts,tsx}files. Useunknownor a concrete type instead.♻️ Proposed fix
- let resolveCreate!: (v: any) => void + let resolveCreate!: (v: unknown) => void🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/ConnectionManager/__tests__/AddConnectionModal.test.tsx` at line 129, Replace the any parameter in the resolveCreate function declaration within the AddConnectionModal test with unknown or the concrete value type expected by the mocked create operation, and update related usage if necessary to preserve type safety.Source: Coding guidelines
src/renderer/src/components/Database/__tests__/DatabaseExplorer.test.tsx (1)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
Record<string, any>insetupandrenderConnectedfunction signatures.Both
setup(line 30) andrenderConnected(line 56) useRecord<string, any>for their override parameters, violating the guideline prohibitinganyin function signatures. UseRecord<string, unknown>or define proper override types for the session and database API mocks.As per coding guidelines: "No
anyin function signatures. Useunknownand narrow, or define a proper type."♻️ Proposed fix
-function setup(sessionOverrides: Record<string, any> = {}, dbOverrides: Record<string, any> = {}) { +function setup(sessionOverrides: Record<string, unknown> = {}, dbOverrides: Record<string, unknown> = {}) { const session = makeSession({ type: 'database', dbType: 'postgresql', databaseName: 'appdb', host: 'db.example.com', port: 5432, username: 'admin', sslMode: 'disable', - ...sessionOverrides, + ...(sessionOverrides as Partial<Session>), } as any)-async function renderConnected(sessionOverrides: Record<string, any> = {}, dbOverrides: Record<string, any> = {}) { +async function renderConnected(sessionOverrides: Record<string, unknown> = {}, dbOverrides: Record<string, unknown> = {}) {Also applies to: 56-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Database/__tests__/DatabaseExplorer.test.tsx` around lines 30 - 31, Replace the Record<string, any> parameter types in setup and renderConnected with Record<string, unknown>, or define dedicated session and database override types; update any affected mock construction or property access to narrow unknown values appropriately.Source: Coding guidelines
src/renderer/src/components/RDP/__tests__/RdpView.test.tsx (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
any[]in theconnectImplfunction signature.The
connectImplparameter uses(...args: any[]) => Promise<string>, which violates the guideline prohibitinganyin function signatures. Since the mock implementation only uses the return value,unknown[]or a more specific tuple type would suffice.As per coding guidelines: "No
anyin function signatures. Useunknownand narrow, or define a proper type."♻️ Proposed fix
- connectImpl?: (...args: any[]) => Promise<string> + connectImpl?: (...args: unknown[]) => Promise<string>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/RDP/__tests__/RdpView.test.tsx` around lines 26 - 30, Replace the any[] parameter in setup’s connectImpl function type with unknown[] or an appropriate specific tuple type, preserving the Promise<string> return type and updating any mock invocation handling as needed.Source: Coding guidelines
src/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsx (1)
79-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
Record<string, any>in thesetupfunction signatures.Both
k8sOverridesandpropsparameters useRecord<string, any>, violating the guideline prohibitinganyin function signatures. UseRecord<string, unknown>or define proper override types.As per coding guidelines: "No
anyin function signatures. Useunknownand narrow, or define a proper type."♻️ Proposed fix
-function setup(k8sOverrides: Record<string, any> = {}, props: Record<string, any> = {}) { +function setup(k8sOverrides: Record<string, unknown> = {}, props: Record<string, unknown> = {}) { const api = installWindowApi({ k8s: { ...k8sData(), ...k8sOverrides } }) seedStore({ sessions: [], tabs: [], activeTabId: null }) - const utils = render(<K8sDashboard context="test-ctx" {...props} />) + const utils = render(<K8sDashboard context="test-ctx" {...(props as K8sDashboardProps)} />) return { api, ...utils } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsx` around lines 79 - 84, Replace the any-based parameter types in the setup function with Record<string, unknown> or explicit override types for k8sOverrides and props; update any affected spreads or call sites to narrow values as needed while preserving setup behavior.Source: Coding guidelines
src/renderer/src/components/Dashboard/ServerViews.tsx (1)
63-66: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider triggering
onConnectonkeyupfor Space key.Per WAI-ARIA practices,
role="button"elements should activate onkeyupfor the Space key (to allow users to cancel by holding and moving focus away). Usingkeydownfor both Enter and Space is a common shortcut and functionally works, butkeyupfor Space would be more spec-compliant. This is a minor accessibility refinement, not a blocker.Also applies to: 217-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Dashboard/ServerViews.tsx` around lines 63 - 66, Update the button keyboard handling in the relevant ServerViews component: keep Enter activation on keydown, but handle Space activation on keyup so users can cancel before release. Adjust the existing onKeyDown handler and add the corresponding onKeyUp logic while preserving preventDefault and onConnect behavior.src/renderer/src/components/Terminal/__tests__/SnippetRunner.test.tsx (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid fragile DOM traversal in test selectors.
screen.getByText('Snippets').parentElement!.querySelectorAll('button')[0]!is brittle and will break on any surrounding DOM change. Prefer a role-based or title-based selector (e.g.,screen.getByRole('button', { name: /add/i })orscreen.getByTitle('Add')) to target the "add snippet" button deterministically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Terminal/__tests__/SnippetRunner.test.tsx` at line 78, Replace the fragile parentElement/querySelectorAll traversal in the SnippetRunner test with a deterministic accessible selector for the add-snippet control, such as screen.getByRole('button', { name: /add/i }) or screen.getByTitle('Add'), then click that element.src/main/ipc/__tests__/docker.test.ts (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
anywithunknownin theHandlertype to comply with coding guidelines.The coding guidelines for
src/**/*.{ts,tsx}state: "Noanyin function signatures. Useunknownand narrow, or define a proper type." TheHandlertype usesany[]for args andanyfor the return type. All call sites already cast return values withas, which works equally well withunknown, so no additional changes are needed at call sites.♻️ Proposed refactor
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -type Handler = (...args: any[]) => any +type Handler = (...args: unknown[]) => unknown🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/__tests__/docker.test.ts` around lines 31 - 32, Replace the explicit any types in the Handler function type with unknown for both the variadic arguments and return value, and remove the eslint suppression since it is no longer needed.Source: Coding guidelines
src/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsx (1)
169-172: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider
role="checkbox"witharia-checkedinstead ofrole="button".The element toggles a selection state (checked/unchecked).
role="checkbox"witharia-checked={checked}more accurately conveys the toggle semantics to screen reader users thanrole="button", which implies an action trigger.[accessibility_and_i18n]
♿ Proposed refactor
className="w-4 h-4 rounded flex items-center justify-center flex-shrink-0" style={{ border: `2px solid ${checked ? '`#3B5CCC`' : 'var(--nox-border)'}`, background: checked ? '`#3B5CCC`' : 'transparent', }} - role="button" + role="checkbox" + aria-checked={checked} tabIndex={0} onClick={() => toggle(h.alias)} onKeyDown={e => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); toggle(h.alias) } }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsx` around lines 169 - 172, Update the selectable element’s accessibility semantics in the SSH config list: replace role="button" with role="checkbox" and bind aria-checked to its checked/selection state, while preserving the existing keyboard and click handling in the toggle callback.src/renderer/src/components/Settings/__tests__/Settings.test.tsx (1)
232-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest enshrines a known stale-closure bug instead of testing correct behavior.
The comment at lines 232-235 documents that
SetPinInputhas a stale closure causing false PIN mismatches, and the test assertsapi.auth.setupis never called. This test will fail if the bug is fixed, creating a disincentive for the fix. Consider either fixing the stale closure inSetPinInputand updating the test to assert successful setup, or adding a// TODO(issue)reference to track the fix.Would you like me to investigate the
SetPinInputcomponent and propose a fix for the stale closure?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Settings/__tests__/Settings.test.tsx` around lines 232 - 241, Fix the stale-closure behavior in SetPinInput so PIN confirmation reads the latest committed digits, then update the test to assert matching PINs successfully call api.auth.setup and advance past PIN entry instead of expecting “PINs do not match”; avoid preserving the known-bug assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsx`:
- Around line 185-186: Replace the fragile parentElement/querySelectorAll chain
in the K8sDashboard test with a semantic selector targeting the plus control,
preferably screen.getByRole('button', { name: /+/ }); if no accessible name
exists, add a stable data-testid to the plus button component and query it
directly.
In `@src/renderer/src/components/K8s/ResourceDetailModal.tsx`:
- Around line 50-64: Escape all untrusted text before constructing HTML in
colorize: HTML-escape token.text and unmatched raw[i] values, while preserving
only the generated span markup and safely handling token.color for the style
attribute. Verify the resulting output used by dangerouslySetInnerHTML cannot
interpret raw JSON characters such as <, >, &, quotes, or apostrophes as markup.
In `@src/renderer/src/components/Runner/__tests__/RunnerView.test.tsx`:
- Line 44: Replace the any type in the startRun function’s api parameter with
the existing WindowApiMock type, importing it through the test harness if
necessary, and ensure all startRun call sites remain type-compatible.
In `@src/renderer/src/components/Terminal/__tests__/TerminalView.test.tsx`:
- Line 48: Replace the any type in the setup function’s apiOverrides parameter
with Record<string, unknown>; add explicit type narrowing or a suitable specific
type wherever the overrides are consumed.
---
Nitpick comments:
In `@src/main/ipc/__tests__/docker.test.ts`:
- Around line 31-32: Replace the explicit any types in the Handler function type
with unknown for both the variadic arguments and return value, and remove the
eslint suppression since it is no longer needed.
In `@src/renderer/src/components/CommandPalette/CommandPalette.tsx`:
- Around line 196-199: Remove the redundant Escape key handler from the overlay
element’s onKeyDown prop in CommandPalette, leaving the window-level keydown
listener responsible for Escape handling; retain the overlay onClick behavior
for closing when clicking the backdrop.
In
`@src/renderer/src/components/ConnectionManager/__tests__/AddConnectionModal.test.tsx`:
- Line 129: Replace the any parameter in the resolveCreate function declaration
within the AddConnectionModal test with unknown or the concrete value type
expected by the mocked create operation, and update related usage if necessary
to preserve type safety.
In `@src/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsx`:
- Around line 169-172: Update the selectable element’s accessibility semantics
in the SSH config list: replace role="button" with role="checkbox" and bind
aria-checked to its checked/selection state, while preserving the existing
keyboard and click handling in the toggle callback.
In `@src/renderer/src/components/Dashboard/ServerViews.tsx`:
- Around line 63-66: Update the button keyboard handling in the relevant
ServerViews component: keep Enter activation on keydown, but handle Space
activation on keyup so users can cancel before release. Adjust the existing
onKeyDown handler and add the corresponding onKeyUp logic while preserving
preventDefault and onConnect behavior.
In `@src/renderer/src/components/Database/__tests__/DatabaseExplorer.test.tsx`:
- Around line 30-31: Replace the Record<string, any> parameter types in setup
and renderConnected with Record<string, unknown>, or define dedicated session
and database override types; update any affected mock construction or property
access to narrow unknown values appropriately.
In `@src/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsx`:
- Around line 79-84: Replace the any-based parameter types in the setup function
with Record<string, unknown> or explicit override types for k8sOverrides and
props; update any affected spreads or call sites to narrow values as needed
while preserving setup behavior.
In `@src/renderer/src/components/RDP/__tests__/RdpView.test.tsx`:
- Around line 26-30: Replace the any[] parameter in setup’s connectImpl function
type with unknown[] or an appropriate specific tuple type, preserving the
Promise<string> return type and updating any mock invocation handling as needed.
In `@src/renderer/src/components/Settings/__tests__/Settings.test.tsx`:
- Around line 232-241: Fix the stale-closure behavior in SetPinInput so PIN
confirmation reads the latest committed digits, then update the test to assert
matching PINs successfully call api.auth.setup and advance past PIN entry
instead of expecting “PINs do not match”; avoid preserving the known-bug
assertion.
In `@src/renderer/src/components/Terminal/__tests__/SnippetRunner.test.tsx`:
- Line 78: Replace the fragile parentElement/querySelectorAll traversal in the
SnippetRunner test with a deterministic accessible selector for the add-snippet
control, such as screen.getByRole('button', { name: /add/i }) or
screen.getByTitle('Add'), then click that element.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dd10266-5ca9-4aee-ba2f-96607069e6a1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (61)
.github/workflows/ci.yml.gitignorepackage.jsonsonar-project.propertiessrc/main/ipc/__tests__/docker.test.tssrc/main/ipc/__tests__/k8s.test.tssrc/main/ipc/__tests__/metrics.test.tssrc/main/ipc/__tests__/rdp.test.tssrc/main/ipc/__tests__/sessionTransfer.test.tssrc/main/ipc/__tests__/sftp.test.tssrc/main/ipc/__tests__/ssh.handlers.test.tssrc/main/ipc/__tests__/sshConfig.test.tssrc/main/ipc/__tests__/tunnels.test.tssrc/main/ipc/rdp.tssrc/renderer/src/__tests__/harness.tsxsrc/renderer/src/components/CommandPalette/CommandPalette.tsxsrc/renderer/src/components/ConnectionManager/AddConnectionModal.tsxsrc/renderer/src/components/ConnectionManager/ConnectionManager.tsxsrc/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsxsrc/renderer/src/components/ConnectionManager/__tests__/AddConnectionModal.test.tsxsrc/renderer/src/components/ConnectionManager/__tests__/ConnectionManager.test.tsxsrc/renderer/src/components/ConnectionManager/__tests__/ImportSshConfigModal.test.tsxsrc/renderer/src/components/Dashboard/ServerViews.tsxsrc/renderer/src/components/Dashboard/__tests__/Dashboard.test.tsxsrc/renderer/src/components/Dashboard/__tests__/ServerViews.test.tsxsrc/renderer/src/components/Database/DatabaseExplorer.tsxsrc/renderer/src/components/Database/__tests__/DatabaseExplorer.test.tsxsrc/renderer/src/components/Docker/__tests__/DockerDashboard.test.tsxsrc/renderer/src/components/Editor/__tests__/EditorTab.test.tsxsrc/renderer/src/components/K8s/ResourceDetailModal.tsxsrc/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsxsrc/renderer/src/components/K8s/__tests__/ResourceDetailModal.test.tsxsrc/renderer/src/components/RDP/RdpView.tsxsrc/renderer/src/components/RDP/__tests__/RdpView.test.tsxsrc/renderer/src/components/Redis/RedisExplorer.tsxsrc/renderer/src/components/Redis/__tests__/RedisExplorer.test.tsxsrc/renderer/src/components/Runner/RunnerView.tsxsrc/renderer/src/components/Runner/__tests__/RunnerView.test.tsxsrc/renderer/src/components/SFTP/SftpBrowser.tsxsrc/renderer/src/components/SFTP/__tests__/FilesDrawer.test.tsxsrc/renderer/src/components/SFTP/__tests__/SftpBrowser.test.tsxsrc/renderer/src/components/Settings/Settings.tsxsrc/renderer/src/components/Settings/__tests__/Settings.test.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/components/Sidebar/__tests__/Sidebar.test.tsxsrc/renderer/src/components/StatusBar/__tests__/StatusBar.test.tsxsrc/renderer/src/components/TabBar/TabBar.tsxsrc/renderer/src/components/TabBar/__tests__/TabBar.test.tsxsrc/renderer/src/components/Terminal/__tests__/DisconnectedOverlay.test.tsxsrc/renderer/src/components/Terminal/__tests__/HostHeader.test.tsxsrc/renderer/src/components/Terminal/__tests__/SnippetRunner.test.tsxsrc/renderer/src/components/Terminal/__tests__/TerminalSearchBar.test.tsxsrc/renderer/src/components/Terminal/__tests__/TerminalView.test.tsxsrc/renderer/src/components/Tunnels/__tests__/TunnelsView.test.tsxsrc/renderer/src/components/__tests__/MainContent.test.tsxsrc/renderer/src/components/__tests__/UnlockScreen.test.tsxsrc/renderer/src/lib/__tests__/colors.test.tssrc/renderer/src/lib/__tests__/format.test.tssrc/renderer/src/store/__tests__/store.test.tstsconfig.web.jsonvitest.config.ts
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (10)
- src/renderer/src/components/ConnectionManager/ConnectionManager.tsx
- src/renderer/src/components/TabBar/TabBar.tsx
- src/renderer/src/components/Runner/RunnerView.tsx
- src/renderer/src/components/Redis/RedisExplorer.tsx
- src/renderer/src/components/SFTP/SftpBrowser.tsx
- src/renderer/src/components/RDP/RdpView.tsx
- src/main/ipc/rdp.ts
- src/renderer/src/components/Settings/Settings.tsx
- src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx
- src/renderer/src/components/Database/DatabaseExplorer.tsx
…ndling - Updated various test cases to use `expect(...).toBeTruthy()` for better readability. - Changed button roles in the DatabaseExplorer, RedisExplorer, and Sidebar components for improved accessibility. - Enhanced error handling in the TerminalView for reading key files. - Refactored the ChangeAuthModal to handle confirmation PIN submission more reliably. - Improved the handling of keyboard events in the RunnerView and Sidebar components. - Added HTML escaping in the ResourceDetailModal to prevent XSS vulnerabilities. - Updated the TabBar to include new icons for tunnels and runner views. - Refactored the KeyListPane in RedisExplorer to use buttons for key selection. - Improved the handling of session connections in the Sidebar and Runner components.
|
@coderabbitai - we good to merge now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 3 --repo Meanski/noxed --json state,mergeable,mergeStateStatus,reviewDecision,statusCheckRollup,title,isDraft
echo "---REVIEWS---"
gh pr view 3 --repo Meanski/noxed --json reviews --jq '.reviews[] | {author: .author.login, state: .state}'
echo "---COMMENTS (coderabbitai)---"
gh api repos/Meanski/noxed/pulls/3/comments --jq '.[] | select(.user.login=="coderabbitai") | {id, path, in_reply_to_id: .in_reply_to_id, resolved: false}' | head -100Length of output: 1818
One thing to note: Otherwise, from a code/CI health perspective, this is ready to merge whenever you are. (‐^ᴥ^‐) |
|
@coderabbitai - based on your findings and the two commits in this PR, are there any outstanding issues? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 3 --repo Meanski/noxed --json commits --jq '.commits[] | {oid: .oid, messageHeadline: .messageHeadline}'
echo "---PR COMMENTS---"
gh api repos/Meanski/noxed/pulls/3/comments --jq '.[] | {id, path, line, body: (.body[0:200])}'
echo "---ISSUE COMMENTS---"
gh api repos/Meanski/noxed/issues/3/comments --jq '.[] | {id, body: (.body[0:200])}'
echo "---REVIEW THREADS RESOLVED STATUS---"
gh api graphql -f query='
query {
repository(owner: "Meanski", name: "noxed") {
pullRequest(number: 3) {
reviewThreads(first: 50) {
nodes {
isResolved
comments(first: 1) { nodes { path body } }
}
}
}
}
}'Length of output: 32196 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "F59E0B|stateColor" src/renderer/src/components/Docker/DockerDashboard.tsxLength of output: 611 Confirmed — one item is still outstanding. Here's the status based on the two new commits (934809c, 7ac15c4) plus the fix commit 2b102e2: Resolved (9/10 threads): duplicate Still open (1/10): - let stateColor = '`#F59E0B`'
- if (isRunning) stateColor = '`#10B981`'
+ let stateColor = 'var(--nox-warning)'
+ if (isRunning) stateColor = 'var(--nox-success)'
else if (container.State === 'exited') stateColor = 'var(--nox-text-3)'(Adjust the CSS variable names to whatever your Everything else looks clean — CI is green and merge state is still |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/src/components/Runner/RunnerView.tsx (1)
50-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider deriving
runningwithuseMemoinstead ofuseEffect+setRunning.The current
useEffectcallssetRunningwheneverresultschanges, causing an extra render cycle. Sincerunningis purely derived fromresults, computing it during render withuseMemoeliminates the extra render and makes the directsetRunningcalls inrun()(line 87), the error handler (line 91), andcancel()(line 107) unnecessary.♻️ Proposed refactor
- const [running, setRunning] = useState(false) + const running = useMemo( + () => [...results.values()].some(v => v.state === 'running'), + [results], + )Then remove the
useEffecton lines 50–52 and the directsetRunningcalls on lines 87, 91, and 107. Theresultsstate already serves as the single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/Runner/RunnerView.tsx` around lines 50 - 52, Replace the `running` state and `useEffect` in `RunnerView` with a `useMemo` derived from `results`, checking whether any result is in the `running` state. Remove the direct `setRunning` calls from `run()`, its error handler, and `cancel()`, ensuring `results` remains the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsx`:
- Around line 163-174: Replace the hardcoded `#3B5CCC` values in
ImportSshConfigModal’s checked button styles with the centralized color source,
using an existing --nox-primary CSS variable or defining it through
lib/colors.ts if absent. Apply the same update to the corresponding
checked-state styles in ConnectionManager and RunnerView, preserving the current
visual behavior.
In `@src/renderer/src/components/Database/DatabaseExplorer.tsx`:
- Around line 530-535: The row divs rendered by sortedRows.map are not
keyboard-focusable, so their onKeyDown selection handler cannot be reached. Add
tabIndex={0} and an appropriate row-selection role to each row div, preserving
the existing onClick and onKeyDown behavior in the row rendering.
---
Nitpick comments:
In `@src/renderer/src/components/Runner/RunnerView.tsx`:
- Around line 50-52: Replace the `running` state and `useEffect` in `RunnerView`
with a `useMemo` derived from `results`, checking whether any result is in the
`running` state. Remove the direct `setRunning` calls from `run()`, its error
handler, and `cancel()`, ensuring `results` remains the single source of truth.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a1fddaa-39b0-46ba-9150-2089b963cf5d
📒 Files selected for processing (36)
src/main/ipc/__tests__/database.test.tssrc/main/ipc/__tests__/docker.test.tssrc/main/ipc/__tests__/rdp.test.tssrc/main/ipc/database.tssrc/main/ipc/docker.tssrc/main/ipc/tunnels.tssrc/preload/index.d.tssrc/preload/index.tssrc/renderer/src/components/ConnectionManager/AddConnectionModal.tsxsrc/renderer/src/components/ConnectionManager/ConnectionManager.tsxsrc/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsxsrc/renderer/src/components/ConnectionManager/__tests__/AddConnectionModal.test.tsxsrc/renderer/src/components/ConnectionManager/__tests__/ConnectionManager.test.tsxsrc/renderer/src/components/ConnectionManager/__tests__/ImportSshConfigModal.test.tsxsrc/renderer/src/components/Dashboard/ServerViews.tsxsrc/renderer/src/components/Dashboard/__tests__/ServerViews.test.tsxsrc/renderer/src/components/Database/DatabaseExplorer.tsxsrc/renderer/src/components/Database/__tests__/DatabaseExplorer.test.tsxsrc/renderer/src/components/Docker/DockerDashboard.tsxsrc/renderer/src/components/K8s/ResourceDetailModal.tsxsrc/renderer/src/components/K8s/__tests__/K8sDashboard.test.tsxsrc/renderer/src/components/RDP/__tests__/RdpView.test.tsxsrc/renderer/src/components/Redis/RedisExplorer.tsxsrc/renderer/src/components/Redis/__tests__/RedisExplorer.test.tsxsrc/renderer/src/components/Runner/RunnerView.tsxsrc/renderer/src/components/Runner/__tests__/RunnerView.test.tsxsrc/renderer/src/components/Settings/Settings.tsxsrc/renderer/src/components/Settings/__tests__/Settings.test.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/components/Sidebar/__tests__/Sidebar.test.tsxsrc/renderer/src/components/TabBar/TabBar.tsxsrc/renderer/src/components/Terminal/TerminalView.tsxsrc/renderer/src/components/Terminal/__tests__/SnippetRunner.test.tsxsrc/renderer/src/components/Terminal/__tests__/TerminalView.test.tsxsrc/renderer/src/lib/__tests__/colors.test.tssrc/renderer/src/lib/colors.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/renderer/src/components/RDP/tests/RdpView.test.tsx
- src/renderer/src/components/ConnectionManager/tests/ConnectionManager.test.tsx
- src/main/ipc/tunnels.ts
- src/renderer/src/lib/tests/colors.test.ts
- src/renderer/src/components/TabBar/TabBar.tsx
- src/renderer/src/components/Terminal/tests/TerminalView.test.tsx
- src/main/ipc/tests/docker.test.ts
- src/main/ipc/docker.ts
- src/renderer/src/components/K8s/ResourceDetailModal.tsx
- src/renderer/src/components/Terminal/tests/SnippetRunner.test.tsx
- src/renderer/src/components/Redis/RedisExplorer.tsx
- src/renderer/src/components/ConnectionManager/tests/AddConnectionModal.test.tsx
- src/renderer/src/components/Terminal/TerminalView.tsx
- src/renderer/src/components/K8s/tests/K8sDashboard.test.tsx
- src/renderer/src/components/Settings/Settings.tsx
- src/main/ipc/tests/rdp.test.ts
- src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx
- src/renderer/src/components/Database/tests/DatabaseExplorer.test.tsx
| <button | ||
| type="button" | ||
| className="w-4 h-4 rounded flex items-center justify-center flex-shrink-0" | ||
| style={{ | ||
| border: `2px solid ${checked ? '#3B5CCC' : 'var(--nox-border)'}`, | ||
| background: checked ? '#3B5CCC' : 'transparent', | ||
| }} | ||
| aria-pressed={checked} | ||
| onClick={() => toggle(h.alias)} | ||
| > | ||
| {checked && <Check className="w-3 h-3 text-white" />} | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Hardcoded #3B5CCC violates the color centralization guideline.
Lines 167–168 use #3B5CCC directly in inline styles. As per coding guidelines, hex colors should not be hardcoded outside lib/colors.ts — use a CSS custom property (var(--nox-*)) or import from the centralized color module instead.
The same pattern appears in ConnectionManager.tsx (lines 145–146, 175–176) and RunnerView.tsx (lines 146–147).
🎨 Suggested fix
style={{
- border: `2px solid ${checked ? '`#3B5CCC`' : 'var(--nox-border)'}`,
- background: checked ? '`#3B5CCC`' : 'transparent',
+ border: `2px solid ${checked ? 'var(--nox-primary)' : 'var(--nox-border)'}`,
+ background: checked ? 'var(--nox-primary)' : 'transparent',
}}If --nox-primary doesn't exist, define #3B5CCC in lib/colors.ts and reference it from there.
📝 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.
| <button | |
| type="button" | |
| className="w-4 h-4 rounded flex items-center justify-center flex-shrink-0" | |
| style={{ | |
| border: `2px solid ${checked ? '#3B5CCC' : 'var(--nox-border)'}`, | |
| background: checked ? '#3B5CCC' : 'transparent', | |
| }} | |
| aria-pressed={checked} | |
| onClick={() => toggle(h.alias)} | |
| > | |
| {checked && <Check className="w-3 h-3 text-white" />} | |
| </div> | |
| </button> | |
| <button | |
| type="button" | |
| className="w-4 h-4 rounded flex items-center justify-center flex-shrink-0" | |
| style={{ | |
| border: `2px solid ${checked ? 'var(--nox-primary)' : 'var(--nox-border)'}`, | |
| background: checked ? 'var(--nox-primary)' : 'transparent', | |
| }} | |
| aria-pressed={checked} | |
| onClick={() => toggle(h.alias)} | |
| > | |
| {checked && <Check className="w-3 h-3 text-white" />} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/components/ConnectionManager/ImportSshConfigModal.tsx`
around lines 163 - 174, Replace the hardcoded `#3B5CCC` values in
ImportSshConfigModal’s checked button styles with the centralized color source,
using an existing --nox-primary CSS variable or defining it through
lib/colors.ts if absent. Apply the same update to the corresponding
checked-state styles in ConnectionManager and RunnerView, preserving the current
visual behavior.
Source: Coding guidelines
…ng state management in RunnerView
1 participant
Summary by CodeRabbit