feat(config): manage manual properties#3000
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR adds property management capabilities to configuration items. It extends the Property type with creator metadata, updates ConfigItem's property type, introduces RPC-based API services for adding and deleting properties, and implements two modal components for managing user-owned properties integrated into ConfigDetails. ChangesConfiguration Property Management
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
adityathebe
force-pushed
the
feat/manual-config-properties
branch
from
50fac64 to
5850fe0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Configs/Sidebar/ConfigDetails.tsx`:
- Around line 267-282: The "Edit properties" pencil icon and tooltip (FaEdit /
data-tooltip-content="Edit properties") are misleading because
ManageConfigPropertiesModal (opened via setIsManagePropertiesOpen) only supports
deletion; replace the FaEdit icon with a more appropriate icon such as FaList or
FaEllipsisV and update the tooltip text to "Manage properties" (and tooltip id
if desired) so the IconButton that triggers setIsManagePropertiesOpen accurately
reflects that it opens the deletion/management modal.
In `@src/components/Configs/Sidebar/ManageConfigPropertiesModal.tsx`:
- Around line 35-53: deleteProperty currently allows concurrent deletes because
there is no in-flight guard; add a local loading guard (e.g., a state like
deletingPropertyName or a Set<string> of deleting names) and check it at the top
of deleteProperty to return early if that property is already being deleted, set
the guard true before calling deleteConfigItemProperty and false in finally, and
pass that guard into the IconButton's disabled prop (and the other IconButton
instance referenced around lines 81-87) so the button is disabled while the RPC
is in-flight; keep using toastSuccess/toastError and call onChanged with the
result (or optimistically remove the item from UI) only after the RPC succeeds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66c98eb9-58f0-4dfc-baef-03910dedb7ca
📒 Files selected for processing (7)
src/api/services/configs.tssrc/api/types/configs.tssrc/api/types/topology.tssrc/components/Configs/Sidebar/AddConfigPropertyModal.tsxsrc/components/Configs/Sidebar/ConfigDetails.tsxsrc/components/Configs/Sidebar/ManageConfigPropertiesModal.tsxsrc/components/Topology/Sidebar/Utils/formatProperties.tsx
| <IconButton | ||
| data-tooltip-id="edit-properties-tooltip" | ||
| data-tooltip-content="Edit properties" | ||
| icon={<FaEdit className="text-gray-600" size={14} />} | ||
| onClick={() => setIsManagePropertiesOpen(true)} | ||
| /> | ||
| <IconButton | ||
| data-tooltip-id="add-property-tooltip" | ||
| data-tooltip-content="Add property" | ||
| icon={<FaPlus className="text-gray-600" size={14} />} | ||
| onClick={() => setIsAddPropertyOpen(true)} | ||
| /> | ||
| </div> | ||
| <Tooltip id="edit-properties-tooltip" /> | ||
| <Tooltip id="add-property-tooltip" /> | ||
| </div> |
There was a problem hiding this comment.
FaEdit/"Edit properties" is semantically misleading for a delete-only modal.
ManageConfigPropertiesModal only supports deletion (its own title says "Delete Properties"). The FaEdit pencil icon and "Edit properties" tooltip will mislead users expecting to rename or update property values. Consider FaList / FaEllipsisV with the tooltip "Manage properties" to better reflect the actual action.
💡 Suggested change
-import { FaEdit, FaExclamationTriangle, FaPlus, FaTrash } from "react-icons/fa";
+import { FaExclamationTriangle, FaList, FaPlus, FaTrash } from "react-icons/fa"; <IconButton
- data-tooltip-id="edit-properties-tooltip"
- data-tooltip-content="Edit properties"
- icon={<FaEdit className="text-gray-600" size={14} />}
+ data-tooltip-id="manage-properties-tooltip"
+ data-tooltip-content="Manage properties"
+ icon={<FaList className="text-gray-600" size={14} />}
onClick={() => setIsManagePropertiesOpen(true)}
/>- <Tooltip id="edit-properties-tooltip" />
+ <Tooltip id="manage-properties-tooltip" />📝 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.
| <IconButton | |
| data-tooltip-id="edit-properties-tooltip" | |
| data-tooltip-content="Edit properties" | |
| icon={<FaEdit className="text-gray-600" size={14} />} | |
| onClick={() => setIsManagePropertiesOpen(true)} | |
| /> | |
| <IconButton | |
| data-tooltip-id="add-property-tooltip" | |
| data-tooltip-content="Add property" | |
| icon={<FaPlus className="text-gray-600" size={14} />} | |
| onClick={() => setIsAddPropertyOpen(true)} | |
| /> | |
| </div> | |
| <Tooltip id="edit-properties-tooltip" /> | |
| <Tooltip id="add-property-tooltip" /> | |
| </div> | |
| <IconButton | |
| data-tooltip-id="manage-properties-tooltip" | |
| data-tooltip-content="Manage properties" | |
| icon={<FaList className="text-gray-600" size={14} />} | |
| onClick={() => setIsManagePropertiesOpen(true)} | |
| /> | |
| <IconButton | |
| data-tooltip-id="add-property-tooltip" | |
| data-tooltip-content="Add property" | |
| icon={<FaPlus className="text-gray-600" size={14} />} | |
| onClick={() => setIsAddPropertyOpen(true)} | |
| /> | |
| </div> | |
| <Tooltip id="manage-properties-tooltip" /> | |
| <Tooltip id="add-property-tooltip" /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Sidebar/ConfigDetails.tsx` around lines 267 - 282, The
"Edit properties" pencil icon and tooltip (FaEdit / data-tooltip-content="Edit
properties") are misleading because ManageConfigPropertiesModal (opened via
setIsManagePropertiesOpen) only supports deletion; replace the FaEdit icon with
a more appropriate icon such as FaList or FaEllipsisV and update the tooltip
text to "Manage properties" (and tooltip id if desired) so the IconButton that
triggers setIsManagePropertiesOpen accurately reflects that it opens the
deletion/management modal.
| const deleteProperty = async (property: Property) => { | ||
| if (!user?.id) { | ||
| toastError("Could not determine current user"); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const result = await deleteConfigItemProperty( | ||
| configId, | ||
| "person", | ||
| user.id, | ||
| property.name | ||
| ); | ||
| toastSuccess("Property deleted"); | ||
| onChanged?.(result?.properties); | ||
| } catch (e) { | ||
| toastError((e as Error).message); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Delete button has no in-flight guard — concurrent/double deletes are possible.
deleteProperty tracks no loading state and the IconButton has no disabled prop. A double-click (or deleting two properties rapidly) fires concurrent RPC calls against the same config: the first succeeds, the second fails with an error toast because the property no longer exists. The stale entry also remains visible in the list until the parent's refetchConfig resolves.
🔒 Proposed fix
export default function ManageConfigPropertiesModal({ ... }: Props) {
const { user } = useUser();
+ const [deletingProperty, setDeletingProperty] = useState<string | null>(null);
const userProperties = ...;
const deleteProperty = async (property: Property) => {
if (!user?.id) {
toastError("Could not determine current user");
return;
}
+ if (deletingProperty != null) return;
try {
+ setDeletingProperty(property.name);
const result = await deleteConfigItemProperty(
configId,
"person",
user.id,
property.name
);
toastSuccess("Property deleted");
onChanged?.(result?.properties);
} catch (e) {
toastError((e as Error).message);
+ } finally {
+ setDeletingProperty(null);
}
};And on the button:
<IconButton
aria-label={`Delete ${property.name}`}
title="Delete property"
className="text-gray-500 hover:text-red-600"
icon={<FaTrash />}
onClick={() => deleteProperty(property)}
+ disabled={deletingProperty != null}
/>Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Configs/Sidebar/ManageConfigPropertiesModal.tsx` around lines
35 - 53, deleteProperty currently allows concurrent deletes because there is no
in-flight guard; add a local loading guard (e.g., a state like
deletingPropertyName or a Set<string> of deleting names) and check it at the top
of deleteProperty to return early if that property is already being deleted, set
the guard true before calling deleteConfigItemProperty and false in finally, and
pass that guard into the IconButton's disabled prop (and the other IconButton
instance referenced around lines 81-87) so the button is disabled while the RPC
is in-flight; keep using toastSuccess/toastError and call onChanged with the
result (or optimistically remove the item from UI) only after the RPC succeeds.
5850fe0 to
ed01c9c
Compare
Add config sidebar controls for users to add manual properties with either text or numeric values and delete only their own manually-added properties.\n\nWire the UI to the config item property update/delete RPCs and carry property ownership fields through config property types.
adityathebe
force-pushed
the
feat/manual-config-properties
branch
from
ed01c9c to
3461369
Compare
1 participant
Add config sidebar actions for users to add and delete manually-owned config properties.
Manual add supports both text and numeric property values. Delete is exposed through a separate properties manage modal and calls the single-property delete RPC so users do not replace their whole owned property list.
Property ownership fields are carried through config property types so the UI only shows the current user's manual properties for deletion.
Summary by CodeRabbit
Release Notes