Skip to content

Conversation

@veronnicka
Copy link
Contributor

@veronnicka veronnicka commented Aug 20, 2025

The goal is to replace all KeytabTables with KeytabTableWithFilter

Refs: #763

Summary by Sourcery

Replace the legacy KeytabTable with the new KeytabTableWithFilter in the AllowedCreateKeytab component, wiring up filtered entry lists and RPC mutations with loading state for add/remove operations

Enhancements:

  • Swap out KeytabTable for KeytabTableWithFilter across user, group, host, and hostgroup tabs
  • Introduce modalSpinning state and useUpdateKeyTabMutation to handle asynchronous allow/disallow actions
  • Add toTableEntries helper to transform host attributes into TableEntry lists for the filterable tables
  • Expose entryCount prop in KeytabTableWithFilter to enable correct pagination and filtering

@veronnicka veronnicka requested review from carma12 and duzda August 20, 2025 10:38
@veronnicka veronnicka self-assigned this Aug 20, 2025
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • The repeated transformation of host prop values into table entries (usersList, userGroupsList, etc.) could be memoized or abstracted into a custom hook to avoid unnecessary recomputation on each render.
  • The addEntries and removeEntries callbacks share nearly identical logic—consider refactoring them into a shared helper or hook to reduce duplication and improve readability.
  • Moving static constants like allowMethod, disallowMethod, and the toTableEntries function outside the component body would prevent them from being recreated on every render.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated transformation of host prop values into table entries (usersList, userGroupsList, etc.) could be memoized or abstracted into a custom hook to avoid unnecessary recomputation on each render.
- The addEntries and removeEntries callbacks share nearly identical logic—consider refactoring them into a shared helper or hook to reduce duplication and improve readability.
- Moving static constants like allowMethod, disallowMethod, and the toTableEntries function outside the component body would prevent them from being recreated on every render.

## Individual Comments

### Comment 1
<location> `src/components/HostsSections/AllowedCreateKeytab.tsx:23` </location>
<code_context>

 const AllowedCreateKeytab = (props: PropsToAllowCreateKeytab) => {
   const [activeTabKey, setActiveTabKey] = React.useState<string | number>(0);
+  const [modalSpinning, setModalSpinning] = React.useState<boolean>(false);
+  const [executeUpdate] = useUpdateKeyTabMutation({});
+
</code_context>

<issue_to_address>
Consider handling errors in async RPC calls to avoid silent failures.

addEntries and removeEntries only reset the spinner and refresh on success, but do not handle errors from executeUpdate. If the RPC call fails, the spinner may remain active and the user will not be notified. Please add error handling to reset the spinner and inform the user of failures.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@veronnicka veronnicka added the needs-review This PR is waiting on a review label Aug 20, 2025
@veronnicka veronnicka force-pushed the refactor-keytab-table branch from 13588e3 to 9c41068 Compare August 20, 2025 16:10
@veronnicka veronnicka changed the title Replace KeytabTable with KeytabTableWithFilter. Replace KeytabTable with KeytabTableWithFilter Aug 20, 2025
Copy link
Contributor

@duzda duzda left a comment

Choose a reason for hiding this comment

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

Hi, apart from the notes, I'd add Refs: #763 to the commit message.

@veronnicka veronnicka force-pushed the refactor-keytab-table branch 3 times, most recently from 3e8a98d to 7979450 Compare August 22, 2025 12:55
@veronnicka veronnicka requested a review from duzda August 22, 2025 12:55
Copy link
Contributor

@duzda duzda left a comment

Choose a reason for hiding this comment

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

Please add the KeytabFields to the Host interface, see inline comments.
The type should be optional.

image

You might want to take a look at all the examples, there's also AllowedRetrieveKeytab, for Hosts and Services, I think all of these can be nicely squashed into one single component.

@veronnicka veronnicka force-pushed the refactor-keytab-table branch 3 times, most recently from f9b2946 to e4decf3 Compare September 1, 2025 08:56
The goal is to replace all KeytabTables with KeytabTableWithFilter

Refs: freeipa#763

Signed-off-by: Veronika Miticka <[email protected]>
@veronnicka veronnicka force-pushed the refactor-keytab-table branch from e4decf3 to 3cff2c5 Compare September 1, 2025 09:03
@veronnicka
Copy link
Contributor Author

Bug introduced: adding host, it shouldnt who server.ipa.demo in the modal. Waiting for integration tests with this pr

@veronnicka veronnicka added WIP Work in Progress (do not merge) and removed needs-review This PR is waiting on a review labels Sep 1, 2025
@github-actions
Copy link

This PR has not received any attention in 60 days.

@github-actions github-actions bot added the stale This PR/issue is stale and will be closed label Oct 31, 2025
@carma12 carma12 removed the stale This PR/issue is stale and will be closed label Oct 31, 2025
@carma12
Copy link
Collaborator

carma12 commented Oct 31, 2025

Removed the stale tag as this is under WIP.

@github-actions
Copy link

This PR has not received any attention in 60 days.

@github-actions github-actions bot added the stale This PR/issue is stale and will be closed label Dec 31, 2025
@carma12 carma12 removed the stale This PR/issue is stale and will be closed label Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Work in Progress (do not merge)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants