Skip to content

fix: initialize bool parameter state from config - #92

Open
beep-boopp wants to merge 3 commits into
inspektor-gadget:mainfrom
beep-boopp:fix/bool-initial-state
Open

fix: initialize bool parameter state from config#92
beep-boopp wants to merge 3 commits into
inspektor-gadget:mainfrom
beep-boopp:fix/bool-initial-state

Conversation

@beep-boopp

@beep-boopp beep-boopp commented Apr 7, 2026

Copy link
Copy Markdown

bool.tsx never called config.get() to restore its initial value,
causing pre-checked boolean parameters to always render unchecked after
a re-mount. Fixes #91.

Caught while writing unit tests in #23.

What changed

Switched bool.tsx to a controlled component — replaced defaultChecked with
checked={checked} backed by useState initialized from config.get?.().
State is the source of truth after mount, handleChange keeps config in sync
via config.set. Also bundled pre-existing lint fixes (gadgetGrid.tsx unused
imports, wasm.js added to eslintIgnore) to unblock CI.

Testing done

Added a regression test renders checkbox as checked when config.get returns "true"
to bool.test.tsx. All 4 tests passing locally:
Screenshot 2026-04-07 200223

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes boolean gadget parameter UI state restoration by initializing the checkbox’s initial checked state from persisted config, aligning behavior with other parameter components and adding a regression test for the remount scenario (Issue #91).

Changes:

  • Initialize Checkbox state from config.get() via defaultChecked in src/gadgets/params/bool.tsx.
  • Add unit test coverage validating that config.get() returning "true" renders the checkbox checked.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/gadgets/params/bool.tsx Initializes checkbox initial state from config.get() to preserve persisted checked state across remounts.
src/gadgets/params/bool.test.tsx Adds a regression test verifying the checkbox is initially checked when config.get() returns "true".

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gadgets/params/bool.test.tsx Outdated
import { vi } from 'vitest';
import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';
import '@testing-library/jest-dom';

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test imports @testing-library/jest-dom, which is the Jest entrypoint. Since this repo uses Vitest (and doesn’t have Jest/@types/jest in the lockfile), prefer the Vitest-specific entry (@testing-library/jest-dom/vitest) or move it into the shared Vitest setup to ensure matcher runtime + TypeScript typings work reliably under npm run tsc/CI.

Suggested change
import '@testing-library/jest-dom';
import '@testing-library/jest-dom/vitest';

Copilot uses AI. Check for mistakes.
@beep-boopp

Copy link
Copy Markdown
Author

@illume Sorry about the lint here, should’ve caught that from #23.
Also, bool.test.tsx overlaps a bit with #23 since that’s where the full param test suite lives. Whichever merges first, the other will just need a small rebase on this file, happy to take care of that.
Thanks

@beep-boopp
beep-boopp force-pushed the fix/bool-initial-state branch 2 times, most recently from b3bb549 to ccf887f Compare April 8, 2026 12:53
@beep-boopp
beep-boopp requested a review from Copilot April 12, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gadgets/params/bool.tsx Outdated
<Box display="flex" flexDirection="row" gap={2}>
<FormControlLabel
control={<Checkbox onChange={handleChange} />}
control={<Checkbox defaultChecked={config.get?.() === 'true'} onChange={handleChange} />}

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaultChecked makes this an uncontrolled checkbox that only reads config.get() on initial mount; if the config value changes while the component stays mounted (e.g., external reset / param sync), the UI won’t update. If config is intended to be the source of truth, consider switching to a controlled checked prop backed by component state initialized from config.get() (and updated in onChange).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hii @beep-boopp any comments on this??

Comment thread src/gadgets/params/bool.test.tsx Outdated
@@ -0,0 +1,56 @@
/// <reference types="vitest" />
import '@testing-library/jest-dom/vitest';
import { fireEvent,render, screen } from '@testing-library/react';

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer @testing-library/user-event over fireEvent for user interactions (clicking checkboxes), since it more closely matches real browser behavior and reduces false positives. If you keep fireEvent, at least ensure the import formatting is consistent (space after commas) to match typical style.

Copilot uses AI. Check for mistakes.
test('calls config.set with "true" when checked', () => {
render(<CheckboxFilter param={mockParam} config={mockConfig} />);

fireEvent.click(screen.getByRole('checkbox'));

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer @testing-library/user-event over fireEvent for user interactions (clicking checkboxes), since it more closely matches real browser behavior and reduces false positives. If you keep fireEvent, at least ensure the import formatting is consistent (space after commas) to match typical style.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +43
fireEvent.click(checkbox); // check
fireEvent.click(checkbox); // uncheck

Copilot AI Apr 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer @testing-library/user-event over fireEvent for user interactions (clicking checkboxes), since it more closely matches real browser behavior and reduces false positives. If you keep fireEvent, at least ensure the import formatting is consistent (space after commas) to match typical style.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hii @beep-boopp should we use @testing-library/user-event as mentioned here

Signed-off-by: Prajwal <percy38621@gmail.com>
@beep-boopp
beep-boopp force-pushed the fix/bool-initial-state branch from ccf887f to db51dde Compare April 12, 2026 13:16
@beep-boopp

Copy link
Copy Markdown
Author

All linting, formatting, and Vitest imports resolved. Ready for review

@beep-boopp

Copy link
Copy Markdown
Author

Hey @ashu8912
Good catch by Copilot. defaultChecked is uncontrolled, React ignores it after mount, so re renders wouldn't reflect updated config. Switched to controlled checked backed by useState initialized from config.get?.().
Quick heads-up:

On tests: holding off on the userEvent swap here to avoid mixed styles across param tests. Will migrate all 7 test files cleanly in #23 instead.
On CI: bundled a few minor lint fixes (unused imports in gadgetGrid.tsx, added wasm.js to eslintIgnore since it's generated code). No logic changed.
I hope this works and is good to merge.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gadgets/params/bool.tsx
Comment thread src/gadgets/params/bool.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: CheckboxFilter does not read config.get() for initial state

3 participants