more responsive inventory tables

[Sebastian-ubs]

"BREAKING" CHANGE: Data table's th are now always p-0 by default and usage can decide to add padding as per their needs.

	Storybook example	Paratext example: Markers inventory
Before
After

Full screenshot more narrow

This change is 

---

[katherinejensen00]

Someone else should probably look at this too. I looked at it and ran it through Devin, an AI reviewer that some people mentioned in an AI slack channel. I specified which comments were from Devin. It found one potential bug and four flags to think about.

@katherinejensen00 reviewed 14 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Sebastian-ubs).

---

lib/platform-bible-react/src/components/advanced/data-table/data-table.component.tsx line 142 at r1 (raw file):

              {headerGroup.headers.map((header) => {
                return (
                  /* CUSTOM: tw-p-0, let consumers define padding on their own */

AI review from Devin. Flag.

TableHead padding change may affect existing consumers

At data-table.component.tsx:143, the TableHead now has className="tw-p-0" added with the comment 'let consumers define padding on their own'.

This is a potentially breaking change for any existing DataTable consumers who relied on the default padding from the TableHead component (which has tw-h-12 tw-px-4 in table.tsx). The new tw-p-0 will override the default tw-px-4 padding. However, since this appears intentional to give inventory headers better control over their layout, and the PR includes updates to handle this, it's likely acceptable.

---

lib/platform-bible-react/src/components/advanced/data-table/data-table.stories.tsx line 253 at r1 (raw file):

  {
    accessorKey: 'name',
    // header: ({ column }) => getInventoryHeader(column, "Name"),

Can this be fully deleted since it is already commented out?

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 24 at r1 (raw file):

  React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Trigger> & ButtonProps
>(({ className, variant, ...props }, ref) => (
  <TooltipPrimitive.Trigger

AI review from Devin. Flag.

TooltipTrigger forwardRef missing displayName

The modified TooltipTrigger at tooltip.tsx:20-29 uses React.forwardRef but doesn't set a displayName. This is inconsistent with TooltipContent (line 46) which does set TooltipContent.displayName = TooltipPrimitive.Content.displayName.

While not a functional bug, this affects React DevTools debugging experience - the component will appear as 'ForwardRef' instead of 'TooltipTrigger' in the component tree.

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 26 at r1 (raw file):

  <TooltipPrimitive.Trigger
    ref={ref}
    className={variant ? cn(buttonVariants({ variant }), className) : ''}

AI comment from Devin. Potential Bug.

TooltipTrigger ignores className when variant is not provided

The TooltipTrigger component's className handling logic silently ignores any passed className when variant is not provided.

Click to expand

Root Cause

At tooltip.tsx:26, the className logic is:

className={variant ? cn(buttonVariants({ variant }), className) : ''}

When variant is undefined but className is provided (e.g., <TooltipTrigger className="my-custom-class">), the entire className evaluates to an empty string '', completely discarding the user's className.

Expected Behavior

When no variant is provided, the component should still pass through any className that was provided.

Actual Behavior

The className is only applied when variant is truthy. If variant is not provided, className is ignored and replaced with an empty string.

Impact

Any existing code using TooltipTrigger with a className but without a variant will have their styles silently ignored after this change. The fix should be:

className={variant ? cn(buttonVariants({ variant }), className) : className}

---

Recommendation

Change line 26 from className={variant ? cn(buttonVariants({ variant }), className) : ''} to className={variant ? cn(buttonVariants({ variant }), className) : className} to preserve className when variant is not provided.

---

extensions/src/platform-scripture/src/checks/inventories/inventory-utils.ts line 8 at r1 (raw file):

 */
export function getUnicodeValue(char: string): string {
  return char.charCodeAt(0).toString(16).toUpperCase().padStart(4, '0');

AI review from Devin. Flag.

getUnicodeValue returns '0NAN' for empty strings

The new getUnicodeValue function at inventory-utils.ts:7-8 uses char.charCodeAt(0) which returns NaN for empty strings, resulting in '0NAN' after the .toString(16).toUpperCase().padStart(4, '0') chain.

This is not flagged as a bug because:

1. The InventoryTableData.items array is expected to contain valid character strings from scripture text
2. The existing code at inventory-columns.tsx:78 already accesses row.items[0] without null checks, indicating empty items are not expected
3. If this edge case were to occur, '0NAN' would be a visible indicator of the problem rather than a silent failure

---

extensions/src/platform-scripture/src/checks/inventories/marker-inventory.component.tsx line 5 at r1 (raw file):

import { Canon, SerializedVerseRef } from '@sillsdev/scripture';
import {
  Button,

AI review from Devin. Flag.

Unused Button import in punctuation-inventory.component.tsx

At punctuation-inventory.component.tsx:4, Button is still imported from 'platform-bible-react' but is no longer used after the header refactoring (previously used in the removed header: () => <Button variant="ghost">{unicodeValueLabel}</Button> pattern). This import should be removed for cleanliness, though it doesn't affect functionality.

[Sebastian-ubs]

@Sebastian-ubs made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @katherinejensen00).

---

lib/platform-bible-react/src/components/advanced/data-table/data-table.component.tsx line 142 at r1 (raw file):
Yes, the pr description intentionally states

"BREAKING" CHANGE: Data table's th are now always p-0 by default and usage can decide to add padding as per their needs.
Data table is used by us only for inventories. For other consumers this will be communicated as a breaking change via Discord.

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 24 at r1 (raw file):

Previously, katherinejensen00 wrote…

AI review from Devin. Flag.

TooltipTrigger forwardRef missing displayName

The modified TooltipTrigger at tooltip.tsx:20-29 uses React.forwardRef but doesn't set a displayName. This is inconsistent with TooltipContent (line 46) which does set TooltipContent.displayName = TooltipPrimitive.Content.displayName.

While not a functional bug, this affects React DevTools debugging experience - the component will appear as 'ForwardRef' instead of 'TooltipTrigger' in the component tree.

Done, good catch

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 26 at r1 (raw file):

Previously, katherinejensen00 wrote…

AI comment from Devin. Potential Bug.

TooltipTrigger ignores className when variant is not provided

The TooltipTrigger component's className handling logic silently ignores any passed className when variant is not provided.

Click to expand

Root Cause

At tooltip.tsx:26, the className logic is:

className={variant ? cn(buttonVariants({ variant }), className) : ''}

When variant is undefined but className is provided (e.g., <TooltipTrigger className="my-custom-class">), the entire className evaluates to an empty string '', completely discarding the user's className.

Expected Behavior

When no variant is provided, the component should still pass through any className that was provided.

Actual Behavior

The className is only applied when variant is truthy. If variant is not provided, className is ignored and replaced with an empty string.

Impact

Any existing code using TooltipTrigger with a className but without a variant will have their styles silently ignored after this change. The fix should be:

className={variant ? cn(buttonVariants({ variant }), className) : className}

---

Recommendation

Change line 26 from className={variant ? cn(buttonVariants({ variant }), className) : ''} to className={variant ? cn(buttonVariants({ variant }), className) : className} to preserve className when variant is not provided.

Done, agree this was an oversight

[Sebastian-ubs]

@Sebastian-ubs made 3 comments and resolved 1 discussion.
Reviewable status: 9 of 15 files reviewed, 4 unresolved discussions (waiting on @katherinejensen00).

---

extensions/src/platform-scripture/src/checks/inventories/inventory-utils.ts line 8 at r1 (raw file):

Previously, katherinejensen00 wrote…

AI review from Devin. Flag.

getUnicodeValue returns '0NAN' for empty strings

The new getUnicodeValue function at inventory-utils.ts:7-8 uses char.charCodeAt(0) which returns NaN for empty strings, resulting in '0NAN' after the .toString(16).toUpperCase().padStart(4, '0') chain.

This is not flagged as a bug because:

1. The InventoryTableData.items array is expected to contain valid character strings from scripture text
2. The existing code at inventory-columns.tsx:78 already accesses row.items[0] without null checks, indicating empty items are not expected
3. If this edge case were to occur, '0NAN' would be a visible indicator of the problem rather than a silent failure

changed to 0000

---

extensions/src/platform-scripture/src/checks/inventories/marker-inventory.component.tsx line 5 at r1 (raw file):

Previously, katherinejensen00 wrote…

AI review from Devin. Flag.

Unused Button import in punctuation-inventory.component.tsx

At punctuation-inventory.component.tsx:4, Button is still imported from 'platform-bible-react' but is no longer used after the header refactoring (previously used in the removed header: () => <Button variant="ghost">{unicodeValueLabel}</Button> pattern). This import should be removed for cleanliness, though it doesn't affect functionality.

done

---

lib/platform-bible-react/src/components/advanced/data-table/data-table.stories.tsx line 253 at r1 (raw file):

Previously, katherinejensen00 wrote…

Can this be fully deleted since it is already commented out?

done

[katherinejensen00]

Thanks for making those changes! They look great!

The AI review tool, Devin, found a couple of preexisting things that get touched on with your changes. They may be outside of the scope of your task so I am happy to create new work items for them. I just wanted to get your opinion on them and if they are legitimate since you have been in that part of the code most recently.

@katherinejensen00 reviewed 6 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs).

---

lib/platform-bible-react/src/stories/advanced/inventory.stories.tsx line 192 at r2 (raw file):

function getDescription(markerDescriptions: string[], marker: string): string | undefined {
  // Search for whole marker surrounded by whitespace or periods or at string boundaries (^ and $)
  const findMarker = new RegExp(`(^|[\\s.])${marker}([\\s.]|$)`);

AI review from Devin. Informational Flag. Sebastian, this is another pre existing one, though a little more related than the last one to current changes, but still feel free to say this should be part of a different work item.

Pre-existing: Regex injection vulnerability in getDescription

The getDescription function at marker-inventory.component.tsx:37 constructs a regex using string interpolation without escaping: new RegExp(\(^|[\s.])${marker}([\s.]|$)`). If a marker contains regex special characters (like +, *, ?, (, etc.), this could cause unexpected behavior or regex errors. The codebase has escapeStringRegexpinplatform-bible-utils` that should be used here. However, this is pre-existing code not introduced by this PR, and USFM markers typically don't contain special regex characters, so the practical risk is low.

---

extensions/src/platform-scripture/src/checks/inventories/inventory-utils.ts line 8 at r2 (raw file):

 */
export function getUnicodeValue(char: string): string {
  if (!char || char.length === 0) return '0000';

AI comment from Devin. Informational Flag. Sebastian, this sounds like it is preexisting so feel free to say this is outside of the scope of your task. We can always create another work item to address it.

Pre-existing: getUnicodeValue doesn't handle surrogate pairs correctly

The getUnicodeValue function at inventory-utils.ts:9 uses charCodeAt(0) which returns UTF-16 code units, not full Unicode code points. For characters outside the Basic Multilingual Plane (emojis, some rare scripts), this returns only the first surrogate (e.g., D83D instead of 1F600 for 😀). The codebase already has a codePointAt utility in platform-bible-utils that handles this correctly. However, this is pre-existing behavior - the original code in character-inventory and punctuation-inventory also used charCodeAt(0). For scripture text, BMP characters are likely sufficient, but this could cause confusion if users encounter non-BMP characters.

[tjcouch-sil]

Very nice! Thanks for the improvements. Just one quick thing to adjust.

@tjcouch-sil reviewed 15 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Sebastian-ubs).

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 5 at r2 (raw file):

import { cn } from '@/utils/shadcn-ui.util';
import { ButtonProps, buttonVariants } from './button';

I believe the import "from" needs to be replaced with '@/components/shadcn-ui/button'

---

lib/platform-bible-react/src/components/advanced/data-table/data-table.component.tsx line 142 at r2 (raw file):

              {headerGroup.headers.map((header) => {
                return (
                  /* CUSTOM: tw-p-0, let consumers define padding on their own */

The [DataTable component from shacdn](https://ui.shadcn.com/docs/components/radix/data-table] doesn't seem to match what we have in this file at all. Also, this component is in the advanced folder. I suspect this component was created completely custom. Do you think that is the case? If so, no need to put "CUSTOM" comments. That being said, no big deal, so it's fine to leave it. Just FYI.

Seems strange to have a "CUSTOM" comment here and not up on the hover:tw-bg-transparent change, but again, not a big deal if this is a custom component.

[Sebastian-ubs]

Thanks. Agree and did the changes as they where easy and small.
TJ I think you are right and the data table is just our own component, so I removed the CUSTOM comment.

@Sebastian-ubs made 2 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil).

---

lib/platform-bible-react/src/components/shadcn-ui/tooltip.tsx line 5 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I believe the import "from" needs to be replaced with '@/components/shadcn-ui/button'

Done.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 6 additional findings.

[tjcouch-sil]

@tjcouch-sil reviewed 13 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.