Open
Conversation
This reverts commit 81f154e.
Reviewer's GuideIntroduces a new operator shelter dashboard UX built on reusable generic Table/Row components (ShelterTable, RoomTable), adds a tabbed shelter detail page with room management (including edit modal and floating actions), modernizes operator layout/navigation (NavBar, Router changes, feature-flagged operator route), and standardizes several base UI elements (buttons, dropdowns, modals, toasts) on shared Button/Text primitives. Sequence diagram for room deletion with confirmation modalsequenceDiagram
actor Operator
participant RoomTable
participant ConfirmationModal
participant ParentPage as RoomsTabContent
Operator->>RoomTable: click Delete on room row
RoomTable-->>RoomTable: setDeleteConfirmation(isOpen=true, roomId)
RoomTable->>ConfirmationModal: render isOpen=true with primaryAction
Operator->>ConfirmationModal: click Delete (primaryAction)
ConfirmationModal->>RoomTable: primaryAction.onClick()
RoomTable-->>RoomTable: check roomIds or roomId
RoomTable->>ParentPage: onDeleteRoom(roomId)
RoomTable-->>RoomTable: clear selectedRoomIds
RoomTable-->>RoomTable: closeDeleteConfirmation()
RoomTable->>ConfirmationModal: render isOpen=false
Class diagram for new table abstractions and related UI componentsclassDiagram
direction LR
class Table~TItem,TRowObject~ {
+TableColumn~TItem~[] columns
+TItem[] rows
+function getRowKey(item TItem, index number) string
+function getRowObject(item TItem, index number) TRowObject
+function getTrailingContent(rowObject TRowObject, item TItem, index number) ReactNode
+function onRowClick(rowObject TRowObject, rowIndex number) void
+function onDelete(rowObject TRowObject, rowIndex number) void
+boolean loading
+ReactNode loadingState
+ReactNode emptyState
}
class TableColumn~TItem~ {
+string key
+ReactNode label
+string width
+string headerClassName
+string cellClassName
+function render(item TItem) ReactNode
}
class Row~TRowObject~ {
+RowCell[] cells
+TRowObject rowObject
+number rowIndex
+string templateColumns
+ReactNode trailingContent
+function onRowClick(rowObject TRowObject, rowIndex number) void
+function onDelete(rowObject TRowObject, rowIndex number) void
}
class RowCell {
+string key
+ReactNode content
+string className
}
class Shelter {
+string id
+string name
+string address
+number totalBeds
+number availableBeds
+string[] tags
}
class Room {
+string id
+string name
+RoomStatus status
+string[] tags
}
class RoomStatus {
<<enumeration>>
available
occupied
out_of_service
reserved
}
class ShelterRowObject {
+string id
+Shelter shelter
+string name
+string address
+number totalBeds
+number reservedBeds
+string[] tags
}
class RoomRowObject {
+string id
+Room room
}
class ShelterTable {
+Shelter[] rows
+function renderTags(tags string[]) ReactNode
+function onRowClick(rowObject ShelterRowObject, rowIndex number) void
+function onSearchChange(value string) void
+function onDeleteShelter(shelterId string) void
}
class RoomTable {
+Room[] rows
+string[] selectedRoomIds
+function onRowClick(rowObject RoomRowObject, rowIndex number) void
+function onSearchChange(value string) void
+function onDeleteRoom(roomId string) void
+function onDeleteRooms(roomIds string[]) void
}
class EditRoomModal {
+boolean isOpen
+Room room
+function onClose() void
+function onSave(updatedRoom Room) void
}
class ShelterDashboardPage {
+ShelterTab tab
+function render() ReactElement
}
class ShelterTabContent {
+function ShelterTabContent(tab ShelterTab, shelter Shelter) ReactElement
+function RoomsTabContent() ReactElement
}
class SliderTabs {
+SliderTabItem[] items
+string activePathSuffix
+string basePath
+function buildTabPath(basePath string, pathSuffix string) string
}
class SliderTabItem {
+string label
+string pathSuffix
}
class NavBar {
+string organizationName
+string shelterName
+string pageTitle
+boolean showCreateButton
}
class OperatorLayout {
+function render() ReactElement
}
class Button {
+ButtonVariant variant
+ButtonColor color
+ReactNode leftIcon
+ReactNode rightIcon
+function onClick(event MouseEvent) void
}
class ButtonVariant {
<<enumeration>>
floating
primary
primary_sm
trash
edit
right_arrow
}
class ButtonColor {
<<enumeration>>
blue
red
green
yellow
}
class ConfirmationModal {
+string title
+string description
+string variant
+function primaryAction.onClick() void
+function secondaryAction.onClick() void
}
class Toast {
+string title
+string description
+TToastStatus status
}
class toastVariantConfig {
+ButtonColor actionButtonClass
}
TableColumn --> Table : used_by
Row --> Table : used_by
RowCell --> Row : used_by
ShelterTable --> Table : composes
RoomTable --> Table : composes
ShelterTable --> Shelter : renders
ShelterTable --> ShelterRowObject : maps_to
RoomTable --> Room : renders
RoomTable --> RoomRowObject : maps_to
Room --> RoomStatus
ShelterDashboardPage --> SliderTabs : uses
ShelterDashboardPage --> ShelterTabContent : uses
ShelterTabContent --> RoomTable : uses
ShelterTabContent --> EditRoomModal : uses
NavBar --> Button : uses
OperatorLayout --> NavBar : uses
Button ..> ButtonVariant
Button ..> ButtonColor
ConfirmationModal --> Button : uses
Toast --> Button : uses
toastVariantConfig --> Toast : config_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- There are a couple of debug-style console.log calls left in shared components (e.g., Row and HorizontalLayout) that will fire on every click/render; consider removing these or gating them behind a debug flag to avoid noisy logs in production.
- Several places rely on
useOutletContext() as anyfor passing organization-related data (e.g., in Dashboard and ShelterTabContent); introducing a shared context type/interface and usinguseOutletContext<ThatType>()would make these flows safer and easier to maintain. - RoomTable both does its own local filtering based on
searchInputand still calls the parentonSearchChange, which may lead to redundant or confusing filtering behavior when wired to real data; consider clearly separating ‘local-only’ search from ‘server-driven’ search, or letting the parent fully control search state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are a couple of debug-style console.log calls left in shared components (e.g., Row and HorizontalLayout) that will fire on every click/render; consider removing these or gating them behind a debug flag to avoid noisy logs in production.
- Several places rely on `useOutletContext() as any` for passing organization-related data (e.g., in Dashboard and ShelterTabContent); introducing a shared context type/interface and using `useOutletContext<ThatType>()` would make these flows safer and easier to maintain.
- RoomTable both does its own local filtering based on `searchInput` and still calls the parent `onSearchChange`, which may lead to redundant or confusing filtering behavior when wired to real data; consider clearly separating ‘local-only’ search from ‘server-driven’ search, or letting the parent fully control search state.
## Individual Comments
### Comment 1
<location path="libs/react/shelter-operator/src/lib/components/Row.tsx" line_range="38-39" />
<code_context>
+ onRowClick,
+ onClick,
+}: RowProps<TRowObject>) {
+ const handleRowClick = () => {
+ console.log('[ShelterOperator][Row click]', rowObject);
+ onRowClick?.(rowObject, rowIndex);
+ onClick?.();
</code_context>
<issue_to_address>
**issue:** Avoid logging every row click from the shared Row component.
Because this is a shared Row component, this `console.log('[ShelterOperator][Row click]', rowObject);` will fire for every row click app-wide, creating noisy logs and potentially exposing row data in the console. Please remove it or move any necessary logging into the specific caller that needs the diagnostics.
</issue_to_address>
### Comment 2
<location path="libs/react/shelter-operator/src/lib/components/RoomTable.tsx" line_range="96-105" />
<code_context>
+ onDeleteRoom,
+ onDeleteRooms,
+}: RoomTableProps) {
+ const [searchInput, setSearchInput] = useState('');
+ const [selectedRoomIds, setSelectedRoomIds] = useState<string[]>([]);
+ const [deleteConfirmation, setDeleteConfirmation] = useState<{
+ isOpen: boolean;
+ roomIds?: string[];
+ roomId?: string;
+ roomName?: string;
+ }>({ isOpen: false });
+
+ useEffect(() => {
+ onSearchChange?.(searchInput);
+ }, [onSearchChange, searchInput]);
+
</code_context>
<issue_to_address>
**suggestion:** Clarify whether RoomTable should perform local filtering when onSearchChange is provided.
Because `searchInput` is used both to compute local `filteredRows` and to trigger `onSearchChange`, a consumer that uses `onSearchChange` for server-side filtering would effectively get client-side filtering layered on top of already-filtered data. This can lead to unexpected results (e.g., mismatched search semantics or pagination). Consider either only doing local filtering when `onSearchChange` is undefined, or making local filtering explicitly opt-in via a prop so consumers can decide where filtering should occur.
Suggested implementation:
```typescript
emptyState,
wrapperClassName,
headerClassName,
rowClassName,
tableStyle,
headerStyle,
rowStyle,
onDeleteRoom,
onDeleteRooms,
enableLocalFiltering,
}: RoomTableProps) {
const [searchInput, setSearchInput] = useState('');
const [selectedRoomIds, setSelectedRoomIds] = useState<string[]>([]);
const [deleteConfirmation, setDeleteConfirmation] = useState<{
isOpen: boolean;
roomIds?: string[];
roomId?: string;
roomName?: string;
}>({ isOpen: false });
// When enableLocalFiltering is undefined, default to local filtering only if no onSearchChange is provided.
const localFilteringEnabled = enableLocalFiltering ?? !onSearchChange;
useEffect(() => {
onSearchChange?.(searchInput);
}, [onSearchChange, searchInput]);
// REPLACE WITH ACTUAL QUERIED DATA
export type RoomStatus =
```
1. Ensure `useState` and `useEffect` are imported at the top of this file, e.g.:
`import React, { useEffect, useState } from 'react';` (or adjust to match existing import style).
2. Update the `RoomTableProps` type/interface to include the new optional prop:
`enableLocalFiltering?: boolean;` and the existing callback: `onSearchChange?: (value: string) => void;` if it is not already declared.
3. Wherever local filtering is currently computed (likely something like `const filteredRows = rows.filter(...)` using `searchInput`), gate that logic with `localFilteringEnabled`. For example:
- If `localFilteringEnabled` is `true`, compute `filteredRows` based on `searchInput`.
- If `localFilteringEnabled` is `false`, skip the client-side filtering and use the provided `rows` directly.
4. If `filteredRows` is passed to the table component, ensure you switch between `filteredRows` and `rows` based on `localFilteringEnabled` to avoid layering client-side filtering on top of server-side filtering.
</issue_to_address>
### Comment 3
<location path="libs/react/shelter-operator/src/lib/components/RoomTable.tsx" line_range="50-52" />
<code_context>
+ tableStyle?: CSSProperties;
+ headerStyle?: CSSProperties;
+ rowStyle?: CSSProperties;
+ onCreateRoom?: () => void;
+ onDeleteRoom?: (roomId: string) => void;
+ onDeleteRooms?: (roomIds: string[]) => void;
+};
+
</code_context>
<issue_to_address>
**suggestion:** Wire up or remove the unused onCreateRoom prop to keep the API consistent.
`RoomTableProps` includes `onCreateRoom`, but the component never uses it—the "Create Room" button currently lives in `RoomsTabContent`. This makes the prop misleading for consumers. Either wire `onCreateRoom` through to a create button inside `RoomTable`, or remove the prop until `RoomTable` is actually responsible for room creation.
Suggested implementation:
```typescript
wrapperClassName?: string;
headerClassName?: string;
rowClassName?: string;
tableStyle?: CSSProperties;
headerStyle?: CSSProperties;
rowStyle?: CSSProperties;
onDeleteRoom?: (roomId: string) => void;
onDeleteRooms?: (roomIds: string[]) => void;
};
```
1. Remove `onCreateRoom` from the `RoomTable` component props destructuring/signature, if it appears there.
2. Remove any usages of `onCreateRoom` passed into `<RoomTable ... />` from parent components (e.g., `RoomsTabContent`), since the prop is no longer supported.
3. If types are re-exported (e.g., `RoomTableProps` from an index file), ensure there are no references or documentation mentioning `onCreateRoom`.
</issue_to_address>
### Comment 4
<location path="libs/react/shelter-operator/src/lib/components/RoomTable.tsx" line_range="39" />
<code_context>
+ rows: Room[];
+ getRowKey?: (item: Room, index: number) => string;
+ onRowClick?: (rowObject: RoomRowObject, rowIndex: number) => void;
+ onSearchChange?: (value: string) => void;
+ searchPlaceholder?: string;
+ loading?: boolean;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying RoomTable by choosing a single owner for search state and extracting the status UI into a reusable RoomStatusTag component to keep the table configuration declarative and easier to read.
You can simplify this component meaningfully without losing any functionality by:
### 1. Remove the split-brain search handling
Right now `RoomTable` both:
- Keeps `searchInput` locally and filters with `filteredRows`
- Calls `onSearchChange?.(searchInput)` so the parent might *also* filter
That makes it unclear who owns filtering. If the table is already doing the filtering, you can drop the callback and the extra effect and keep `rows` as the single source of truth:
```tsx
type RoomTableProps = {
rows: Room[];
// remove this if parent is not expected to filter:
// onSearchChange?: (value: string) => void;
// ...
};
export function RoomTable({
rows,
// onSearchChange,
// ...
}: RoomTableProps) {
const [searchInput, setSearchInput] = useState('');
// remove this effect
// useEffect(() => {
// onSearchChange?.(searchInput);
// }, [onSearchChange, searchInput]);
const filteredRows = useMemo(() => {
const query = searchInput.trim().toLowerCase();
if (!query) return rows;
return rows.filter((room) => {
const normalizedStatus = STATUS_LABEL[room.status].toLowerCase();
const matchesName = room.name.toLowerCase().includes(query);
const matchesStatus = normalizedStatus.includes(query);
const matchesTags = room.tags.some((tag) =>
tag.toLowerCase().includes(query)
);
return matchesName || matchesStatus || matchesTags;
});
}, [rows, searchInput]);
// ...
}
```
If you *do* want the parent to own filtering instead, invert it:
- Expose `searchInput` and `onSearchChange` as controlled props
- Remove `filteredRows` and just render `rows` (already filtered by parent)
```tsx
type RoomTableProps = {
rows: Room[];
searchValue: string;
onSearchChange: (value: string) => void;
// ...
};
export function RoomTable({
rows,
searchValue,
onSearchChange,
// ...
}: RoomTableProps) {
// no local search state
// no filteredRows useMemo
return (
<>
<input
type="text"
value={searchValue}
onChange={(e) => onSearchChange(e.target.value)}
// ...
/>
<Table
rows={rows}
// ...
/>
</>
);
}
```
Picking one model eliminates confusion and duplicated search logic.
---
### 2. Extract a `RoomStatusTag` to simplify column definitions
The status column currently embeds styling maps and inline JSX. You can extract that into a tiny presentational component and keep the column config declarative:
```tsx
// RoomStatusTag.tsx
const STATUS_STYLE: Record<RoomStatus, string> = {
available: 'bg-[#D7F5DF]',
occupied: 'bg-[#DCEEFF]',
'out-of-service': 'bg-[#FFE5E0]',
reserved: 'bg-[#FFEBCB]',
};
const STATUS_TEXT_STYLE: Record<RoomStatus, string> = {
available: 'text-[#0F8F2F] font-medium',
occupied: 'text-[#1F6FC7] font-medium',
'out-of-service': 'text-[#D7332A] font-medium',
reserved: 'text-[#CC6F00] font-medium',
};
const STATUS_LABEL: Record<RoomStatus, string> = {
available: 'Available',
occupied: 'Occupied',
'out-of-service': 'Out of Service',
reserved: 'Reserved',
};
export function RoomStatusTag({ status }: { status: RoomStatus }) {
return (
<span
className={['inline-flex rounded-full px-3 py-1 leading-none', STATUS_STYLE[status]].join(
' '
)}
>
<Text variant="tag-sm" className={STATUS_TEXT_STYLE[status]}>
{STATUS_LABEL[status]}
</Text>
</span>
);
}
```
Then your column definition becomes much easier to scan:
```tsx
const columns: TableColumn<Room>[] = useMemo(
() => [
// ...
{
key: 'status',
label: 'Status',
width: '1.5fr',
render: (room) => <RoomStatusTag status={room.status} />,
},
// ...
],
[selectedRoomIds]
);
```
This removes the status-specific styling logic from `RoomTable`, makes the columns more readable, and gives you a reusable status tag for other parts of the UI.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+96
to
+105
| const [searchInput, setSearchInput] = useState(''); | ||
| const [selectedRoomIds, setSelectedRoomIds] = useState<string[]>([]); | ||
| const [deleteConfirmation, setDeleteConfirmation] = useState<{ | ||
| isOpen: boolean; | ||
| roomIds?: string[]; | ||
| roomId?: string; | ||
| roomName?: string; | ||
| }>({ isOpen: false }); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
suggestion: Clarify whether RoomTable should perform local filtering when onSearchChange is provided.
Because searchInput is used both to compute local filteredRows and to trigger onSearchChange, a consumer that uses onSearchChange for server-side filtering would effectively get client-side filtering layered on top of already-filtered data. This can lead to unexpected results (e.g., mismatched search semantics or pagination). Consider either only doing local filtering when onSearchChange is undefined, or making local filtering explicitly opt-in via a prop so consumers can decide where filtering should occur.
Suggested implementation:
emptyState,
wrapperClassName,
headerClassName,
rowClassName,
tableStyle,
headerStyle,
rowStyle,
onDeleteRoom,
onDeleteRooms,
enableLocalFiltering,
}: RoomTableProps) {
const [searchInput, setSearchInput] = useState('');
const [selectedRoomIds, setSelectedRoomIds] = useState<string[]>([]);
const [deleteConfirmation, setDeleteConfirmation] = useState<{
isOpen: boolean;
roomIds?: string[];
roomId?: string;
roomName?: string;
}>({ isOpen: false });
// When enableLocalFiltering is undefined, default to local filtering only if no onSearchChange is provided.
const localFilteringEnabled = enableLocalFiltering ?? !onSearchChange;
useEffect(() => {
onSearchChange?.(searchInput);
}, [onSearchChange, searchInput]);
// REPLACE WITH ACTUAL QUERIED DATA
export type RoomStatus =- Ensure
useStateanduseEffectare imported at the top of this file, e.g.:
import React, { useEffect, useState } from 'react';(or adjust to match existing import style). - Update the
RoomTablePropstype/interface to include the new optional prop:
enableLocalFiltering?: boolean;and the existing callback:onSearchChange?: (value: string) => void;if it is not already declared. - Wherever local filtering is currently computed (likely something like
const filteredRows = rows.filter(...)usingsearchInput), gate that logic withlocalFilteringEnabled. For example:- If
localFilteringEnabledistrue, computefilteredRowsbased onsearchInput. - If
localFilteringEnabledisfalse, skip the client-side filtering and use the providedrowsdirectly.
- If
- If
filteredRowsis passed to the table component, ensure you switch betweenfilteredRowsandrowsbased onlocalFilteringEnabledto avoid layering client-side filtering on top of server-side filtering.
Comment on lines
+50
to
+52
| onCreateRoom?: () => void; | ||
| onDeleteRoom?: (roomId: string) => void; | ||
| onDeleteRooms?: (roomIds: string[]) => void; |
There was a problem hiding this comment.
suggestion: Wire up or remove the unused onCreateRoom prop to keep the API consistent.
RoomTableProps includes onCreateRoom, but the component never uses it—the "Create Room" button currently lives in RoomsTabContent. This makes the prop misleading for consumers. Either wire onCreateRoom through to a create button inside RoomTable, or remove the prop until RoomTable is actually responsible for room creation.
Suggested implementation:
wrapperClassName?: string;
headerClassName?: string;
rowClassName?: string;
tableStyle?: CSSProperties;
headerStyle?: CSSProperties;
rowStyle?: CSSProperties;
onDeleteRoom?: (roomId: string) => void;
onDeleteRooms?: (roomIds: string[]) => void;
};- Remove
onCreateRoomfrom theRoomTablecomponent props destructuring/signature, if it appears there. - Remove any usages of
onCreateRoompassed into<RoomTable ... />from parent components (e.g.,RoomsTabContent), since the prop is no longer supported. - If types are re-exported (e.g.,
RoomTablePropsfrom an index file), ensure there are no references or documentation mentioningonCreateRoom.
| rows: Room[]; | ||
| getRowKey?: (item: Room, index: number) => string; | ||
| onRowClick?: (rowObject: RoomRowObject, rowIndex: number) => void; | ||
| onSearchChange?: (value: string) => void; |
There was a problem hiding this comment.
issue (complexity): Consider simplifying RoomTable by choosing a single owner for search state and extracting the status UI into a reusable RoomStatusTag component to keep the table configuration declarative and easier to read.
You can simplify this component meaningfully without losing any functionality by:
1. Remove the split-brain search handling
Right now RoomTable both:
- Keeps
searchInputlocally and filters withfilteredRows - Calls
onSearchChange?.(searchInput)so the parent might also filter
That makes it unclear who owns filtering. If the table is already doing the filtering, you can drop the callback and the extra effect and keep rows as the single source of truth:
type RoomTableProps = {
rows: Room[];
// remove this if parent is not expected to filter:
// onSearchChange?: (value: string) => void;
// ...
};
export function RoomTable({
rows,
// onSearchChange,
// ...
}: RoomTableProps) {
const [searchInput, setSearchInput] = useState('');
// remove this effect
// useEffect(() => {
// onSearchChange?.(searchInput);
// }, [onSearchChange, searchInput]);
const filteredRows = useMemo(() => {
const query = searchInput.trim().toLowerCase();
if (!query) return rows;
return rows.filter((room) => {
const normalizedStatus = STATUS_LABEL[room.status].toLowerCase();
const matchesName = room.name.toLowerCase().includes(query);
const matchesStatus = normalizedStatus.includes(query);
const matchesTags = room.tags.some((tag) =>
tag.toLowerCase().includes(query)
);
return matchesName || matchesStatus || matchesTags;
});
}, [rows, searchInput]);
// ...
}If you do want the parent to own filtering instead, invert it:
- Expose
searchInputandonSearchChangeas controlled props - Remove
filteredRowsand just renderrows(already filtered by parent)
type RoomTableProps = {
rows: Room[];
searchValue: string;
onSearchChange: (value: string) => void;
// ...
};
export function RoomTable({
rows,
searchValue,
onSearchChange,
// ...
}: RoomTableProps) {
// no local search state
// no filteredRows useMemo
return (
<>
<input
type="text"
value={searchValue}
onChange={(e) => onSearchChange(e.target.value)}
// ...
/>
<Table
rows={rows}
// ...
/>
</>
);
}Picking one model eliminates confusion and duplicated search logic.
2. Extract a RoomStatusTag to simplify column definitions
The status column currently embeds styling maps and inline JSX. You can extract that into a tiny presentational component and keep the column config declarative:
// RoomStatusTag.tsx
const STATUS_STYLE: Record<RoomStatus, string> = {
available: 'bg-[#D7F5DF]',
occupied: 'bg-[#DCEEFF]',
'out-of-service': 'bg-[#FFE5E0]',
reserved: 'bg-[#FFEBCB]',
};
const STATUS_TEXT_STYLE: Record<RoomStatus, string> = {
available: 'text-[#0F8F2F] font-medium',
occupied: 'text-[#1F6FC7] font-medium',
'out-of-service': 'text-[#D7332A] font-medium',
reserved: 'text-[#CC6F00] font-medium',
};
const STATUS_LABEL: Record<RoomStatus, string> = {
available: 'Available',
occupied: 'Occupied',
'out-of-service': 'Out of Service',
reserved: 'Reserved',
};
export function RoomStatusTag({ status }: { status: RoomStatus }) {
return (
<span
className={['inline-flex rounded-full px-3 py-1 leading-none', STATUS_STYLE[status]].join(
' '
)}
>
<Text variant="tag-sm" className={STATUS_TEXT_STYLE[status]}>
{STATUS_LABEL[status]}
</Text>
</span>
);
}Then your column definition becomes much easier to scan:
const columns: TableColumn<Room>[] = useMemo(
() => [
// ...
{
key: 'status',
label: 'Status',
width: '1.5fr',
render: (room) => <RoomStatusTag status={room.status} />,
},
// ...
],
[selectedRoomIds]
);This removes the status-specific styling logic from RoomTable, makes the columns more readable, and gives you a reusable status tag for other parts of the UI.
|
🔍 [shelter-web] Preview available at: https://shelter.dev.betterangels.la/branches/bp-room-view Last updated: 2026-04-02T00:01:02.672Z |
…ow Navbar Co-authored-by: jennifer shi <jennifershi255@gmail.com>
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Search state is being managed both in the parent (Dashboard via
tableSearchInput/debouncedSearch) and again insideShelterTableandRoomTable(localsearchInput+onSearchChange), which makes behavior harder to reason about; consider consolidating search control either entirely in the parent (and passing the value/handler down) or entirely in the table components and dropping the extra effect in the parent. - In
NavBar,organizationNamefor shelter pages is derived fromuser.organizations[0]instead of theactiveOrgcontext, which can show the wrong organization when multiple orgs exist; it would be more robust to pull the displayed organization name fromuseActiveOrgfor consistency. renderTagsinShelterTablefalls back to a hardcoded tag list (Women Only,Shared, etc.) whentagsis empty, which will silently display incorrect tags for real shelters; if this is just placeholder, consider gating it behind a feature flag or removing the fallback before shipping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Search state is being managed both in the parent (Dashboard via `tableSearchInput`/`debouncedSearch`) and again inside `ShelterTable` and `RoomTable` (local `searchInput` + `onSearchChange`), which makes behavior harder to reason about; consider consolidating search control either entirely in the parent (and passing the value/handler down) or entirely in the table components and dropping the extra effect in the parent.
- In `NavBar`, `organizationName` for shelter pages is derived from `user.organizations[0]` instead of the `activeOrg` context, which can show the wrong organization when multiple orgs exist; it would be more robust to pull the displayed organization name from `useActiveOrg` for consistency.
- `renderTags` in `ShelterTable` falls back to a hardcoded tag list (`Women Only`, `Shared`, etc.) when `tags` is empty, which will silently display incorrect tags for real shelters; if this is just placeholder, consider gating it behind a feature flag or removing the fallback before shipping.
## Individual Comments
### Comment 1
<location path="libs/react/shelter-operator/src/lib/components/ShelterTable.tsx" line_range="135-144" />
<code_context>
+ label: 'Capacity',
+ width: '1.2fr',
+ cellClassName: 'whitespace-nowrap',
+ render: (shelter) => {
+ const totalBeds = shelter.totalBeds ?? 0;
+ const hasCapacity = totalBeds > 0;
+ const reservedBeds = hasCapacity
+ ? Math.min(
+ Math.max(totalBeds - (shelter.availableBeds ?? 0), 0),
+ totalBeds
+ )
+ : null;
+ const progressPct =
+ hasCapacity && reservedBeds !== null
+ ? (reservedBeds / totalBeds) * 100
</code_context>
<issue_to_address>
**issue (bug_risk):** Treat `availableBeds === null` as unknown capacity instead of "0 available" to avoid showing shelters as fully reserved when data is missing.
The current `shelter.availableBeds ?? 0` logic makes `reservedBeds = totalBeds` when `availableBeds` is `null`, so shelters with unknown availability appear 100% reserved. Instead, treat `availableBeds == null` as an "unknown" state (e.g., show `N/A` or hide the progress bar) and only compute `reservedBeds`/`progressPct` when `availableBeds != null`.
</issue_to_address>
### Comment 2
<location path="libs/react/shelter-operator/src/lib/components/RoomTable.tsx" line_range="50" />
<code_context>
+ tableStyle?: CSSProperties;
+ headerStyle?: CSSProperties;
+ rowStyle?: CSSProperties;
+ onCreateRoom?: () => void;
+ onDeleteRoom?: (roomId: string) => void;
+ onDeleteRooms?: (roomIds: string[]) => void;
</code_context>
<issue_to_address>
**suggestion:** Remove or wire up the unused `onCreateRoom` prop to keep the RoomTable API lean.
`RoomTableProps` exposes `onCreateRoom`, but the component never uses it—the "Create Room" button in `RoomsTabContent` bypasses this prop. To avoid a misleading API, either remove `onCreateRoom` from `RoomTableProps` or move the creation button into `RoomTable` and call `onCreateRoom` there for a consistent integration point.
Suggested implementation:
```typescript
wrapperClassName?: string;
headerClassName?: string;
rowClassName?: string;
tableStyle?: CSSProperties;
headerStyle?: CSSProperties;
rowStyle?: CSSProperties;
onDeleteRoom?: (roomId: string) => void;
onDeleteRooms?: (roomIds: string[]) => void;
```
1. In the same file, remove `onCreateRoom` from the RoomTable component props destructuring, e.g. change:
`function RoomTable({ ..., onCreateRoom, onDeleteRoom, onDeleteRooms }: RoomTableProps)` to remove `onCreateRoom` from the list.
2. Remove any remaining references to `onCreateRoom` in `RoomTable.tsx` if they exist (TypeScript will flag these after the type removal).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+135
to
+144
| render: (shelter) => { | ||
| const totalBeds = shelter.totalBeds ?? 0; | ||
| const hasCapacity = totalBeds > 0; | ||
| const reservedBeds = hasCapacity | ||
| ? Math.min( | ||
| Math.max(totalBeds - (shelter.availableBeds ?? 0), 0), | ||
| totalBeds | ||
| ) | ||
| : null; | ||
| const progressPct = |
There was a problem hiding this comment.
issue (bug_risk): Treat availableBeds === null as unknown capacity instead of "0 available" to avoid showing shelters as fully reserved when data is missing.
The current shelter.availableBeds ?? 0 logic makes reservedBeds = totalBeds when availableBeds is null, so shelters with unknown availability appear 100% reserved. Instead, treat availableBeds == null as an "unknown" state (e.g., show N/A or hide the progress bar) and only compute reservedBeds/progressPct when availableBeds != null.
| tableStyle?: CSSProperties; | ||
| headerStyle?: CSSProperties; | ||
| rowStyle?: CSSProperties; | ||
| onCreateRoom?: () => void; |
There was a problem hiding this comment.
suggestion: Remove or wire up the unused onCreateRoom prop to keep the RoomTable API lean.
RoomTableProps exposes onCreateRoom, but the component never uses it—the "Create Room" button in RoomsTabContent bypasses this prop. To avoid a misleading API, either remove onCreateRoom from RoomTableProps or move the creation button into RoomTable and call onCreateRoom there for a consistent integration point.
Suggested implementation:
wrapperClassName?: string;
headerClassName?: string;
rowClassName?: string;
tableStyle?: CSSProperties;
headerStyle?: CSSProperties;
rowStyle?: CSSProperties;
onDeleteRoom?: (roomId: string) => void;
onDeleteRooms?: (roomIds: string[]) => void;- In the same file, remove
onCreateRoomfrom the RoomTable component props destructuring, e.g. change:
function RoomTable({ ..., onCreateRoom, onDeleteRoom, onDeleteRooms }: RoomTableProps)to removeonCreateRoomfrom the list. - Remove any remaining references to
onCreateRoominRoomTable.tsxif they exist (TypeScript will flag these after the type removal).
# Conflicts: # apps/shelter-web/src/app/App.tsx # apps/shelter-web/src/app/router/buildShelterRoutes.tsx # apps/shelter-web/src/app/router/useShelterRoutes.tsx # apps/shelter-web/src/main.tsx # libs/react/shelter-operator/src/lib/OperatorApp.tsx # libs/react/shelter-operator/src/lib/ShelterDashboardPage.tsx # libs/react/shelter-operator/src/lib/components/NavBar.tsx # libs/react/shelter-operator/src/lib/components/Row.tsx # libs/react/shelter-operator/src/lib/components/ShelterTable.tsx # libs/react/shelter-operator/src/lib/components/Table.tsx # libs/react/shelter-operator/src/lib/components/base-ui/buttons/buttons.tsx # libs/react/shelter-operator/src/lib/components/layout/OperatorLayout.tsx # libs/react/shelter-operator/src/lib/pages/dashboard/Dashboard.tsx # libs/react/shelter-operator/src/lib/types/shelter.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notes
Summary by Sourcery
Implement a revamped shelter operator dashboard with room-level views, shared table components, and updated routing/layout, while standardizing buttons and typography across UI primitives.
New Features:
Enhancements:
Build: