[FIX] Add generic title+subtitle+edit pattern to ToolNavBar#1882
Merged
chandrasekharan-zipstack merged 8 commits intomainfrom Apr 2, 2026
Merged
[FIX] Add generic title+subtitle+edit pattern to ToolNavBar#1882chandrasekharan-zipstack merged 8 commits intomainfrom
chandrasekharan-zipstack merged 8 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced header wrappers with a reusable ToolNavBar, added edit modals (form, validation, API call, store update) for tools and workflows, adjusted header/list CSS and title/edit behaviors, and removed the workflows project filter UI and its handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nav as ToolNavBar/Header/MenuLayout
participant Modal as Edit Modal (Form)
participant API as Backend Service
participant Store as State Store
User->>Nav: Click Edit Icon
Nav->>Modal: Open (pre-fill fields)
User->>Modal: Submit Form
Modal->>Modal: Validate form
alt Validation success
Modal->>API: Send update request
API-->>Modal: Return response
Modal->>Store: Apply update (res.data)
Modal->>Nav: Close modal
Nav->>User: Show success alert
else Validation error
Modal->>User: Surface validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Refactor ToolNavBar to support subtitle (description) and hover-to-edit across all detail pages. Replace custom headers in Prompt Studio and Workflow detail with unified ToolNavBar. Changes: - ToolNavBar: Add subtitle, onEditTitle, onNavigateBack props. Use Typography.Paragraph with ellipsis+tooltip for truncation. Replace Row/Col with plain flex layout, remove hardcoded heights. - Prompt Studio Header: Replace HeaderTitle with ToolNavBar, add edit modal for project name and description. - MenuLayout (Workflow detail): Replace custom appHeader with ToolNavBar, add edit modal for workflow name and description. - ListView: Fix description truncation in list cards using Typography.Paragraph ellipsis instead of manual Tooltip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove "My Workflows" / "Organization Workflows" segment filter from the Workflows list page. Default to showing all workflows. Add "Workflows" as the page title in ToolNavBar for consistency with other listing pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
chandrasekharan-zipstack
force-pushed
the
feat/toolnavbar-subtitle-generic
branch
from
cacd7ed to
70be2f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
frontend/src/components/widgets/list-view/ListView.css (1)
48-51: Make description width adaptive to owner-column visibility.A fixed
max-width: 40%over-truncates descriptions in views where owner info is not shown. Consider a base class plus a modifier (or CSS variable) so width expands whenshowOwneris false.Proposed refactor
.list-view-description { font-size: 13px; margin-bottom: 0; - max-width: 40%; + max-width: var(--list-view-description-max-width, 40%); } + +.list-view-description.no-owner { + --list-view-description-max-width: 70%; +}Then apply
no-ownerin JSX whenshowOwneris false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/widgets/list-view/ListView.css` around lines 48 - 51, The .list-view-description rule currently uses a fixed max-width: 40% which over-truncates when owner info is hidden; change this to use a base class plus a modifier or CSS variable (e.g., max-width: var(--description-max-width, 40%)) and add a modifier selector (e.g., .no-owner .list-view-description or .list-view-description--no-owner) that sets a larger value (e.g., 100% or a wider percent) when owner info is not shown; then update the JSX to apply the modifier class (or set the CSS variable) when the showOwner prop/variable is false so descriptions expand appropriately.frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx (1)
517-528: Guard against duplicate “Look-Ups” entries before pushing.If
data[0].subMenuis sourced from a reused object (e.g., plugin menu), unconditionalpushcan add duplicates across renders. Add a path/key existence check before insertion.Suggested guard
if (lookupStudioEnabled && isUnstract) { const buildSubMenu = data[0]?.subMenu; - buildSubMenu?.push({ + const lookupPath = `/${orgName}/lookups`; + const alreadyExists = buildSubMenu?.some((item) => item.path === lookupPath); + if (!alreadyExists) { + buildSubMenu?.push({ id: 1.4, title: "Look-Ups", description: "Manage reference data look-ups for extraction", image: CustomTools, - path: `/${orgName}/lookups`, - active: globalThis.location.pathname.startsWith(`/${orgName}/lookups`), - }); + path: lookupPath, + active: globalThis.location.pathname.startsWith(lookupPath), + }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx` around lines 517 - 528, The code unconditionally pushes a "Look-Ups" item into data[0]?.subMenu which can create duplicate entries across renders; update the block that uses lookupStudioEnabled && isUnstract and buildSubMenu (data[0]?.subMenu) to first check whether an entry with the same unique key/path (e.g., id 1.4 or path `/${orgName}/lookups`) already exists in buildSubMenu before calling push, and only push the new object if no existing item matches; use Array.prototype.some or find against buildSubMenu to perform the guard.frontend/src/components/custom-tools/header/Header.jsx (1)
333-429: Consider extractingActionButtonsto a standalone component.Using
useCallbackto memoize a component that returns JSX works, but it's unconventional. The large dependency array (14 items) suggests this would be cleaner as a separate memoized component with explicit props, improving readability and making dependency tracking clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/header/Header.jsx` around lines 333 - 429, ActionButtons is currently implemented as a useCallback-returned JSX component with a long dependency list; extract it into a standalone React component (e.g., ActionButtons) in the same file or a new file and pass the needed values as explicit props (handleUpdateTool, setOpenSettings, setOpenCloneModal, setOpenShareModal, isPublicSource, isExportLoading, isApiDeploymentLoading, userList, openExportToolModal, toolDetails, details, openCreateApiDeploymentModal, CloneButton, SinglePassToggleSwitch, PromptShareButton, handleShare, handleExportProject, handleCreateApiDeployment, handleExport) and wrap the new component with React.memo to preserve memoization and keep rendering behavior identical while removing the large useCallback and reducing the parent component’s dependency array.workers/executor/executors/lookup_handler.py (1)
110-126: Consider cachingFileSysteminstance.A new
FileSystemandfile_storageis created on every_load_reference_textcall. If lookups frequently access reference files, caching this at the handler level could reduce overhead.♻️ Proposed approach
class LookupHandler: + def __init__(self) -> None: + self._workflow_fs = FileSystem(FileStorageType.WORKFLOW_EXECUTION) + self._file_storage = self._workflow_fs.get_file_storage() def _load_reference_text(self, file_paths: list[str]) -> str: """Read extracted text files from MinIO and concatenate them.""" - workflow_fs = FileSystem(FileStorageType.WORKFLOW_EXECUTION) - file_storage = workflow_fs.get_file_storage() texts: list[str] = [] for path in file_paths: try: - content = file_storage.read(path=path, mode="r") + content = self._file_storage.read(path=path, mode="r")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/executor/executors/lookup_handler.py` around lines 110 - 126, The _load_reference_text method recreates FileSystem/FileStorage on each call; cache the FileSystem (or its FileStorage) on the handler instance instead (e.g., initialize workflow_fs = FileSystem(FileStorageType.WORKFLOW_EXECUTION) and/or file_storage = workflow_fs.get_file_storage() in the handler's __init__ and reuse those attributes in _load_reference_text) so subsequent calls reuse the same objects; update references in _load_reference_text to use self.workflow_fs/self.file_storage and ensure any lifecycle/cleanup considerations for those cached objects are handled.workers/executor/executors/legacy_executor.py (1)
33-35: Consider addingClassVarannotation to silence mutable class attribute warning.Ruff flags this as a mutable class-level default. While the dict is never mutated (so no bug exists), adding a type annotation clarifies intent and silences the linter.
🔧 Proposed fix
+ from typing import Any, ClassVar - from typing import Any # Maps Operation enum values to handler methods - _OPERATION_MAP: dict[str, str] = { + _OPERATION_MAP: ClassVar[dict[str, str]] = { Operation.LOOKUP.value: "_handle_lookup", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/executor/executors/legacy_executor.py` around lines 33 - 35, Annotate the class-level _OPERATION_MAP with typing.ClassVar to indicate it is an immutable class constant and silence the linter; update the type of _OPERATION_MAP (which currently maps Operation.LOOKUP.value to "_handle_lookup") to ClassVar[dict[str, str]] and add the necessary import for ClassVar from typing so the linter (Ruff) no longer treats it as a mutable default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 2-10: The import list in Header.jsx includes an unused symbol
Typography; remove Typography from the named imports (the import statement that
currently imports Button, Dropdown, Form, Input, Modal, Tooltip, Typography) so
the file only imports the actually used components (Button, Dropdown, Form,
Input, Modal, Tooltip) to eliminate the unused import warning.
In `@frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx`:
- Around line 56-60: The EditOutlined icon is used as a clickable element
without keyboard support or an accessible name; wrap the EditOutlined element
inside the existing antd Button component (use type="text") and move the onClick
handler to the Button so keyboard activation works, and add aria-label="Edit
title" to the Button; update the fragment that references EditOutlined and
onEditTitle accordingly to mirror the back button pattern used in this
component.
In `@frontend/src/components/widgets/list-view/ListView.jsx`:
- Around line 188-196: The component currently uses a truthy check on
item[descriptionProp] before rendering the <Typography.Paragraph>, which will
skip valid falsy values like 0; update the conditional in ListView.jsx to
explicitly check for null or undefined (e.g., item[descriptionProp] !== null &&
item[descriptionProp] !== undefined or use typeof check) so that falsy-but-valid
values render; keep the existing ellipsis and className props on
Typography.Paragraph and only gate rendering when the description is strictly
absent.
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Line 24: The code calls workflowService() directly on each render, recreating
wfService and forcing dependent callbacks like handleEditSubmit to be recreated;
wrap the creation in useMemo (e.g., const wfService = useMemo(() =>
workflowService(), [])) or include relevant changing deps such as sessionDetails
in the dependency array (useMemo(() => workflowService(), [sessionDetails])) so
wfService is memoized and handleEditSubmit can remain stable with useCallback.
In `@frontend/src/routes/useMainAppRoutes.js`:
- Line 191: The route for "lookups/*" is added unconditionally when LookUpStudio
exists, allowing deep links even when the SideNavBar hides the menu via the
isUnstract condition; update the route registration in useMainAppRoutes.js to
apply the same gating as SideNavBar.jsx by checking the same isUnstract boolean
(or equivalent auth/context selector) before adding the Route (i.e. only include
the Route when LookUpStudio && isUnstract), or replace it with the app's
existing protected-route wrapper used elsewhere so the LookUpStudio route
respects the same access constraint.
---
Nitpick comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 333-429: ActionButtons is currently implemented as a
useCallback-returned JSX component with a long dependency list; extract it into
a standalone React component (e.g., ActionButtons) in the same file or a new
file and pass the needed values as explicit props (handleUpdateTool,
setOpenSettings, setOpenCloneModal, setOpenShareModal, isPublicSource,
isExportLoading, isApiDeploymentLoading, userList, openExportToolModal,
toolDetails, details, openCreateApiDeploymentModal, CloneButton,
SinglePassToggleSwitch, PromptShareButton, handleShare, handleExportProject,
handleCreateApiDeployment, handleExport) and wrap the new component with
React.memo to preserve memoization and keep rendering behavior identical while
removing the large useCallback and reducing the parent component’s dependency
array.
In `@frontend/src/components/navigations/side-nav-bar/SideNavBar.jsx`:
- Around line 517-528: The code unconditionally pushes a "Look-Ups" item into
data[0]?.subMenu which can create duplicate entries across renders; update the
block that uses lookupStudioEnabled && isUnstract and buildSubMenu
(data[0]?.subMenu) to first check whether an entry with the same unique key/path
(e.g., id 1.4 or path `/${orgName}/lookups`) already exists in buildSubMenu
before calling push, and only push the new object if no existing item matches;
use Array.prototype.some or find against buildSubMenu to perform the guard.
In `@frontend/src/components/widgets/list-view/ListView.css`:
- Around line 48-51: The .list-view-description rule currently uses a fixed
max-width: 40% which over-truncates when owner info is hidden; change this to
use a base class plus a modifier or CSS variable (e.g., max-width:
var(--description-max-width, 40%)) and add a modifier selector (e.g., .no-owner
.list-view-description or .list-view-description--no-owner) that sets a larger
value (e.g., 100% or a wider percent) when owner info is not shown; then update
the JSX to apply the modifier class (or set the CSS variable) when the showOwner
prop/variable is false so descriptions expand appropriately.
In `@workers/executor/executors/legacy_executor.py`:
- Around line 33-35: Annotate the class-level _OPERATION_MAP with
typing.ClassVar to indicate it is an immutable class constant and silence the
linter; update the type of _OPERATION_MAP (which currently maps
Operation.LOOKUP.value to "_handle_lookup") to ClassVar[dict[str, str]] and add
the necessary import for ClassVar from typing so the linter (Ruff) no longer
treats it as a mutable default.
In `@workers/executor/executors/lookup_handler.py`:
- Around line 110-126: The _load_reference_text method recreates
FileSystem/FileStorage on each call; cache the FileSystem (or its FileStorage)
on the handler instance instead (e.g., initialize workflow_fs =
FileSystem(FileStorageType.WORKFLOW_EXECUTION) and/or file_storage =
workflow_fs.get_file_storage() in the handler's __init__ and reuse those
attributes in _load_reference_text) so subsequent calls reuse the same objects;
update references in _load_reference_text to use
self.workflow_fs/self.file_storage and ensure any lifecycle/cleanup
considerations for those cached objects are handled.
🪄 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: 565775f4-efb2-40d4-9518-52356120da11
📒 Files selected for processing (22)
backend/prompt_studio/prompt_studio_registry_v2/prompt_studio_registry_helper.pyfrontend/src/components/custom-tools/header/Header.cssfrontend/src/components/custom-tools/header/Header.jsxfrontend/src/components/custom-tools/settings-modal/SettingsModal.jsxfrontend/src/components/custom-tools/tool-ide/ToolIde.jsxfrontend/src/components/navigations/side-nav-bar/SideNavBar.jsxfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.cssfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsxfrontend/src/components/widgets/list-view/ListView.cssfrontend/src/components/widgets/list-view/ListView.jsxfrontend/src/layouts/menu-layout/MenuLayout.jsxfrontend/src/routes/useMainAppRoutes.jsunstract/workflow-execution/src/unstract/workflow_execution/enums.pyworkers/executor/__init__.pyworkers/executor/executors/__init__.pyworkers/executor/executors/legacy_executor.pyworkers/executor/executors/lookup_handler.pyworkers/executor/executors/lookup_prompt_resolver.pyworkers/executor/tests/__init__.pyworkers/executor/tests/test_legacy_executor.pyworkers/executor/tests/test_lookup_handler.pyworkers/executor/tests/test_lookup_prompt_resolver.py
💤 Files with no reviewable changes (1)
- frontend/src/components/custom-tools/header/Header.css
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx | Core refactor: replaced Row/Col layout with flex divs, added subtitle and onEditTitle/onNavigateBack props, changed customButtons from a render function to a node — clean change with a minor touch-accessibility gap in the CSS companion. |
| frontend/src/components/navigations/tool-nav-bar/ToolNavBar.css | Complete CSS rewrite for the nav bar; edit icon is hidden via opacity:0 and only revealed on :hover, making it inaccessible on touch-only devices — a @media (hover: none) fallback is missing. |
| frontend/src/components/custom-tools/header/Header.jsx | Replaced HeaderTitle with ToolNavBar and added inline edit-project modal; actionButtons useMemo is missing handleShare, handleExportProject, handleCreateApiDeployment, handleExport from its dep array, risking stale credential closures. |
| frontend/src/layouts/menu-layout/MenuLayout.jsx | Replaced custom appHeader with ToolNavBar; added inline edit-workflow modal; handleEditSubmit does not include wfService in its useCallback deps (pre-existing pattern), and subtitle empty-string fallback is redundant. |
| frontend/src/components/workflows/workflow/Workflows.jsx | Removed PROJECT_FILTER_OPTIONS, applyFilter, and filterViewRef; getProjectList() now fetches all accessible workflows — confirmed intentional by team (backend uses for_user queryset). |
| frontend/src/components/widgets/list-view/ListView.jsx | Description rendering switched from Typography.Text+manual Tooltip to Typography.Paragraph with built-in ellipsis tooltip; null-guarded when no description — clean improvement. |
| frontend/src/pages/ConnectorsPage.jsx | renderCreateConnectorButtons useCallback with empty deps (stale closure bug) replaced with a plain JSX element — actually fixes the pre-existing stale closure. |
| frontend/src/components/logging/execution-logs/ExecutionLogs.jsx | customButtons render-function pattern replaced with a plain JSX element (logTabs); no logic change, straightforward cleanup. |
| frontend/src/components/deployments/header/Header.jsx | customButtons render-function replaced with a plain JSX node (addButton); no logic change. |
Sequence Diagram
sequenceDiagram
participant User
participant ToolNavBar
participant EditModal
participant API
participant ZustandStore
User->>ToolNavBar: hovers over title
ToolNavBar-->>User: shows EditOutlined button
User->>ToolNavBar: clicks edit icon (onEditTitle)
ToolNavBar->>EditModal: handleOpenEditModal()
EditModal-->>User: pre-filled form (name + description)
User->>EditModal: submits form
EditModal->>EditModal: editForm.validateFields()
alt Header.jsx (Prompt Studio)
EditModal->>API: handleUpdateTool(values)
API-->>EditModal: { data: updatedTool }
EditModal->>ZustandStore: useCustomToolStore.setState({ details: updatedData })
else MenuLayout.jsx (Workflow)
EditModal->>API: wfService.editProject(name, desc, projectId)
API-->>EditModal: 200 OK
EditModal->>ZustandStore: useWorkflowStore.setState({ projectName, details })
end
EditModal-->>ToolNavBar: updated title/subtitle rendered
EditModal-->>User: success alert
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/components/custom-tools/header/Header.jsx
Line: 405-418
Comment:
**`actionButtons` useMemo captures stale function closures**
`handleShare`, `handleExportProject`, `handleCreateApiDeployment`, and `handleExport` are all used inside the memo body but are absent from the dependency array. Each of these functions closes over `sessionDetails.orgId`, `sessionDetails.csrfToken`, and `axiosPrivate` — none of which are in the deps list.
If the CSRF token is refreshed (or the org changes) between the first render and the time the user clicks Export / Deploy, the buttons will silently execute with stale credentials, likely producing a 403.
The minimal fix is to add these four functions to the dependency array and wrap them with `useCallback` so the memo only recomputes when they actually change:
```
[
handleUpdateTool,
setOpenSettings,
setOpenCloneModal,
setOpenShareModal,
isPublicSource,
isExportLoading,
isApiDeploymentLoading,
userList,
openExportToolModal,
toolDetails,
details,
openCreateApiDeploymentModal,
handleShare,
handleExportProject,
handleCreateApiDeployment,
handleExport,
],
```
Note: the same incomplete deps were present in the old `useCallback` version, so this isn't a new regression — but wrapping this logic in `useMemo` is a good moment to also fix the exhaustive-deps gap.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/components/navigations/tool-nav-bar/ToolNavBar.css
Line: 46-53
Comment:
**Edit button inaccessible on touch devices**
The edit icon's visibility is controlled purely by a CSS `:hover` rule. On touch-only devices (phones, tablets) `:hover` does not fire reliably, so the edit button remains permanently hidden (`opacity: 0`) and users have no way to trigger the edit flow.
Consider making the button always visible (or at least visible on focus/`:focus-within`) as a fallback, or using a media query to show it unconditionally on touch-capable screens:
```css
@media (hover: none) {
.tool-nav-bar__edit-icon.ant-btn {
opacity: 1;
}
}
```
Alternatively, expose a keyboard shortcut or a dedicated settings menu entry for environments where hover is unavailable.
How can I resolve this? If you propose a fix, please make it concise.Reviews (7): Last reviewed commit: "Merge branch 'main' into feat/toolnavbar..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
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 `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 68-70: The workflows page now calls
projectApiService.getProjectList() with no flag which removes the previous "my
workflows" default; change the call in Workflows.jsx (getProjectList) to pass
the myProjects flag (e.g., projectApiService.getProjectList(true) or otherwise
ensure the created_by filter is applied by default) so the page retains the
previous default scope; update any surrounding code in the getProjectList
function to propagate that boolean to the service call and to any consumers if
needed.
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Around line 73-80: The edit handler handleEditSubmit calls
wfService.editProject(workflow_name, description, projectId) while projectId can
be the default empty string, causing unintended create behavior; update
handleEditSubmit to check that projectId is a non-empty truthy value before
calling wfService.editProject (e.g., if (!projectId) return or surface a
validation error), and also ensure the modal's submit button/form is disabled
until projectId is loaded (tie button disabled state to a truthy projectId or
loading flag) so the edit flow only runs when a valid projectId exists.
🪄 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: 1b83491c-bab0-4bd3-ab88-a52d0f02832a
📒 Files selected for processing (9)
frontend/src/components/custom-tools/header/Header.cssfrontend/src/components/custom-tools/header/Header.jsxfrontend/src/components/custom-tools/tool-ide/ToolIde.jsxfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.cssfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsxfrontend/src/components/widgets/list-view/ListView.cssfrontend/src/components/widgets/list-view/ListView.jsxfrontend/src/components/workflows/workflow/Workflows.jsxfrontend/src/layouts/menu-layout/MenuLayout.jsx
💤 Files with no reviewable changes (1)
- frontend/src/components/custom-tools/header/Header.css
✅ Files skipped from review due to trivial changes (3)
- frontend/src/components/custom-tools/tool-ide/ToolIde.jsx
- frontend/src/components/widgets/list-view/ListView.css
- frontend/src/components/navigations/tool-nav-bar/ToolNavBar.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx
…projectId - Remove unused Typography import from Header.jsx - Memoize workflowService() with useMemo in MenuLayout - Guard handleEditSubmit with projectId check to prevent accidental POST - Remove redundant || "" fallback on subtitle prop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chandrasekharan-zipstack
force-pushed
the
feat/toolnavbar-subtitle-generic
branch
from
95d8bce to
a3b64e6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/custom-tools/header/Header.jsx (1)
325-421:⚠️ Potential issue | 🟡 MinorAdd missing handler dependencies to the
ActionButtonsuseCallback.
ActionButtonscloses overhandleShare,handleExportProject,handleCreateApiDeployment, andhandleExport. These are plain functions redefined on every render—they close over state likesessionDetailsanddetailsthat can change independently. Since they're not in the dependency list,ActionButtonsretains stale references to these handlers. For example, ifdetails?.tool_idchanges,handleExportProjectis recreated with the new ID, butActionButtonsstill references the old function, causing exports to use the stale ID.Add all four handlers to the dependency list:
Suggested change
[ + handleShare, + handleExportProject, + handleCreateApiDeployment, + handleExport, handleUpdateTool, setOpenSettings, setOpenCloneModal, setOpenShareModal, isPublicSource, isExportLoading, isApiDeploymentLoading, userList, openExportToolModal, toolDetails, details, openCreateApiDeploymentModal, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/header/Header.jsx` around lines 325 - 421, ActionButtons is memoized with useCallback but closes over handlers that are recreated each render (handleShare, handleExportProject, handleCreateApiDeployment, handleExport), causing stale references; update the dependency array of the ActionButtons useCallback to include handleShare, handleExportProject, handleCreateApiDeployment, and handleExport so the callback is refreshed whenever those handlers (and the state they close over like details/sessionDetails) change.
🧹 Nitpick comments (1)
frontend/src/layouts/menu-layout/MenuLayout.jsx (1)
103-117: Unconventional but functional component memoization.Using
useCallbackto memoize a component works, thoughuseMemoreturning JSX is more conventional when the goal is to memoize rendered output. This is a minor stylistic point.♻️ Optional: Use useMemo for clarity
- const RightButtons = useCallback( - () => ( + const RightButtons = useMemo( + () => () => ( <Space> ... </Space> ), [activeTab], );Or extract as a separate memoized component outside
MenuLayout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/layouts/menu-layout/MenuLayout.jsx` around lines 103 - 117, The RightButtons "component" is memoized with useCallback which is unconventional for JSX output; change its implementation to use useMemo to return the JSX (or extract RightButtons as a separate memoized component outside MenuLayout) so intent is clearer: replace the useCallback usage of RightButtons (and its dependency on activeTab) with useMemo returning the <Space>…<Button> JSX (or move RightButtons to its own memoized component) and keep the activeTab dependency to update when the active tab changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 425-433: The ToolNavBar props are being set before store data is
loaded causing previousRoute to interpolate sessionDetails?.orgName as
"undefined" and onEditTitle to enable edit before details?.tool_id exists;
update the rendering guard around ToolNavBar so previousRoute is only passed
when sessionDetails?.orgName is truthy (e.g., previousRoute={isPublicSource ||
!sessionDetails?.orgName ? undefined : `/${sessionDetails.orgName}/tools`}) and
onEditTitle is only passed when details?.tool_id is present (e.g.,
onEditTitle={isPublicSource || !details?.tool_id ? undefined :
handleOpenEditModal}); ensure these changes affect the same ToolNavBar usage to
prevent ToolIde (handleOpenEditModal → PATCH) from firing with an undefined org
or tool id.
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Around line 84-87: The updateWorkflow setter in the store (function
updateWorkflow) is incorrectly merging state by calling setState({
existingState, ...entireState }) which nests the previous state under an
existingState key; change it to spread the previous state into the new object
(setState({ ...existingState, ...entireState })) so the top-level keys like
projectId, projectName, and details remain at root; update any references to
existingState and entireState inside updateWorkflow to use the spread merge and
keep existing semantics for partial updates when called from MenuLayout.jsx or
elsewhere.
- Around line 73-76: The handler handleEditSubmit currently silently returns
when projectId is falsy, which gives no user feedback; update the UI and handler
to prevent this: either disable the modal OK button by passing okButtonProps={{
disabled: !projectId }} on the Modal component, or show an inline error/toast
when projectId is missing before returning from handleEditSubmit; ensure all
references to projectId in MenuLayout (including handleEditSubmit and the Modal
declaration) are updated so the Save action is disabled or shows a clear error
instead of doing nothing.
---
Outside diff comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 325-421: ActionButtons is memoized with useCallback but closes
over handlers that are recreated each render (handleShare, handleExportProject,
handleCreateApiDeployment, handleExport), causing stale references; update the
dependency array of the ActionButtons useCallback to include handleShare,
handleExportProject, handleCreateApiDeployment, and handleExport so the callback
is refreshed whenever those handlers (and the state they close over like
details/sessionDetails) change.
---
Nitpick comments:
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Around line 103-117: The RightButtons "component" is memoized with useCallback
which is unconventional for JSX output; change its implementation to use useMemo
to return the JSX (or extract RightButtons as a separate memoized component
outside MenuLayout) so intent is clearer: replace the useCallback usage of
RightButtons (and its dependency on activeTab) with useMemo returning the
<Space>…<Button> JSX (or move RightButtons to its own memoized component) and
keep the activeTab dependency to update when the active tab changes.
🪄 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: 23206752-3369-4b53-8e88-8b9c11b4d0a8
📒 Files selected for processing (2)
frontend/src/components/custom-tools/header/Header.jsxfrontend/src/layouts/menu-layout/MenuLayout.jsx
chandrasekharan-zipstack
requested review from
Deepak-Kesavan,
athul-rs,
jaseemjaskp and
vishnuszipstack
Use antd Button with type="text" and aria-label for the edit icon in ToolNavBar, matching the back button pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/layouts/menu-layout/MenuLayout.jsx (2)
24-24:⚠️ Potential issue | 🟠 MajorMemoize
wfServiceto prevent recreation on every render.
workflowService()is invoked on every render, creating a fresh service instance. Additionally,wfServiceis missing fromhandleEditSubmit's dependency array (line 94), creating a stale closure risk if session details change.🔧 Proposed fix
-import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react";Then in the component body:
-const wfService = workflowService(); +const wfService = useMemo(() => workflowService(), []);And update the dependency array:
-}, [editForm, projectId, details, setAlertDetails, handleException]); +}, [editForm, projectId, details, setAlertDetails, handleException, wfService]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/layouts/menu-layout/MenuLayout.jsx` at line 24, The component currently calls workflowService() on every render (const wfService = workflowService()), causing a new service instance each render and creating a stale closure in handleEditSubmit; memoize the service instance (e.g., via React's useMemo or useRef) so wfService is stable across renders and then include that stable wfService reference in the handleEditSubmit dependency array to avoid stale closures when session or props change; update any imports/hooks as needed and ensure handleEditSubmit's dependencies reflect the memoized wfService.
73-76:⚠️ Potential issue | 🟡 MinorSilent early return provides no user feedback.
When
projectIdis falsy, the function returns silently. Users clicking Save see nothing happen. Either disable the button or show an error.💡 Suggested improvement
Option 1 - Show error message:
const handleEditSubmit = useCallback(async () => { if (!projectId) { + setAlertDetails({ + type: "error", + content: "Workflow details are still loading. Please try again.", + }); return; }Option 2 - Disable the modal button (in the Modal component):
<Modal ... okButtonProps={{ disabled: !projectId }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/layouts/menu-layout/MenuLayout.jsx` around lines 73 - 76, The handleEditSubmit function currently returns silently when projectId is falsy, leaving users without feedback; update the behavior so users get clear feedback by either (A) showing an inline error/notification before returning from handleEditSubmit (use the existing toast/notification utility or set an error state and display it in the modal) or (B) preventing the action UI by disabling the modal OK button—modify the Modal usage (the component wrapping handleEditSubmit) to include okButtonProps={{ disabled: !projectId }} so the Save button is disabled when projectId is falsy; reference handleEditSubmit and the Modal instance to implement one of these fixes.frontend/src/components/custom-tools/header/Header.jsx (1)
425-433:⚠️ Potential issue | 🟠 MajorGuard
previousRouteandonEditTitleon loaded store data.
previousRouteinterpolatessessionDetails?.orgNameeven when it's still unset (resulting in/undefined/tools), andonEditTitlestays enabled beforedetails?.tool_idexists.🛡️ Proposed fix
<ToolNavBar title={details?.tool_name || ""} subtitle={isPublicSource ? undefined : details?.description || ""} - previousRoute={ - isPublicSource ? undefined : `/${sessionDetails?.orgName}/tools` - } - onEditTitle={isPublicSource ? undefined : handleOpenEditModal} + previousRoute={ + !isPublicSource && sessionDetails?.orgName + ? `/${sessionDetails.orgName}/tools` + : undefined + } + onEditTitle={ + !isPublicSource && details?.tool_id ? handleOpenEditModal : undefined + } CustomButtons={ActionButtons} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/header/Header.jsx` around lines 425 - 433, The ToolNavBar props use sessionDetails and details before those stores are guaranteed to be loaded, causing "/undefined/tools" and enabling edit early; update the previousRoute and onEditTitle expressions so they return undefined until the required data exists: change previousRoute to something like sessionDetails?.orgName ? `/${sessionDetails.orgName}/tools` : undefined, and change onEditTitle to isPublicSource || !details?.tool_id ? undefined : handleOpenEditModal (keep the isPublicSource checks). Ensure these guards are applied where ToolNavBar is rendered (ToolNavBar, previousRoute, onEditTitle, sessionDetails, details, handleOpenEditModal).
🧹 Nitpick comments (1)
frontend/src/layouts/menu-layout/MenuLayout.jsx (1)
114-120: Consider guardingonEditTitlewhen workflow data hasn't loaded.
onEditTitleis always passed, allowing users to open the edit modal beforeprojectIdis available. Combined with the silent return inhandleEditSubmit, users can click Save with no effect.💡 Suggested improvement
<ToolNavBar title={projectName || "Name of the project"} subtitle={details?.description} onNavigateBack={handleNavigateBack} - onEditTitle={handleOpenEditModal} + onEditTitle={projectId ? handleOpenEditModal : undefined} CustomButtons={RightButtons} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/layouts/menu-layout/MenuLayout.jsx` around lines 114 - 120, The ToolNavBar is receiving onEditTitle unconditionally which allows opening the edit modal before project data (projectId/details) is loaded; update the code so onEditTitle is only passed when projectId and details are available (e.g., pass undefined or a disabled handler to ToolNavBar) and add defensive checks in handleOpenEditModal and handleEditSubmit to early-return with a visible error or no-op only after confirming projectId exists; reference ToolNavBar, onEditTitle, handleOpenEditModal, handleEditSubmit, projectId, and details when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 307-323: The handler handleEditSubmit calls handleUpdateTool which
builds a PATCH URL using details?.tool_id; add a guard to ensure
details?.tool_id exists before making the API call (either check
details?.tool_id at the start of handleEditSubmit or validate inside
handleUpdateTool) and bail out with a controlled error/early return (and
setAlertDetails with an explanatory message) if the id is missing; ensure you
still close the modal/update store only after a successful update, and keep
references to useCustomToolStore.getState().updateCustomTool({ details:
updatedData }) unchanged.
---
Duplicate comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 425-433: The ToolNavBar props use sessionDetails and details
before those stores are guaranteed to be loaded, causing "/undefined/tools" and
enabling edit early; update the previousRoute and onEditTitle expressions so
they return undefined until the required data exists: change previousRoute to
something like sessionDetails?.orgName ? `/${sessionDetails.orgName}/tools` :
undefined, and change onEditTitle to isPublicSource || !details?.tool_id ?
undefined : handleOpenEditModal (keep the isPublicSource checks). Ensure these
guards are applied where ToolNavBar is rendered (ToolNavBar, previousRoute,
onEditTitle, sessionDetails, details, handleOpenEditModal).
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Line 24: The component currently calls workflowService() on every render
(const wfService = workflowService()), causing a new service instance each
render and creating a stale closure in handleEditSubmit; memoize the service
instance (e.g., via React's useMemo or useRef) so wfService is stable across
renders and then include that stable wfService reference in the handleEditSubmit
dependency array to avoid stale closures when session or props change; update
any imports/hooks as needed and ensure handleEditSubmit's dependencies reflect
the memoized wfService.
- Around line 73-76: The handleEditSubmit function currently returns silently
when projectId is falsy, leaving users without feedback; update the behavior so
users get clear feedback by either (A) showing an inline error/notification
before returning from handleEditSubmit (use the existing toast/notification
utility or set an error state and display it in the modal) or (B) preventing the
action UI by disabling the modal OK button—modify the Modal usage (the component
wrapping handleEditSubmit) to include okButtonProps={{ disabled: !projectId }}
so the Save button is disabled when projectId is falsy; reference
handleEditSubmit and the Modal instance to implement one of these fixes.
---
Nitpick comments:
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Around line 114-120: The ToolNavBar is receiving onEditTitle unconditionally
which allows opening the edit modal before project data (projectId/details) is
loaded; update the code so onEditTitle is only passed when projectId and details
are available (e.g., pass undefined or a disabled handler to ToolNavBar) and add
defensive checks in handleOpenEditModal and handleEditSubmit to early-return
with a visible error or no-op only after confirming projectId exists; reference
ToolNavBar, onEditTitle, handleOpenEditModal, handleEditSubmit, projectId, and
details when making these changes.
🪄 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: ad32cfc8-1d8b-4d7a-97d8-f1db36c90937
📒 Files selected for processing (4)
frontend/src/components/custom-tools/header/Header.jsxfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.cssfrontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsxfrontend/src/layouts/menu-layout/MenuLayout.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/navigations/tool-nav-bar/ToolNavBar.css
- frontend/src/components/navigations/tool-nav-bar/ToolNavBar.jsx
- Header.jsx: Gate onEditTitle on details.tool_id and previousRoute on sessionDetails.orgName to prevent /undefined/ paths during load - MenuLayout.jsx: Use useWorkflowStore.setState() instead of updateWorkflow() to avoid pre-existing shallow merge bug. Disable modal OK button when projectId not loaded. Gate onEditTitle on projectId presence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/components/custom-tools/header/Header.jsx (1)
325-421: Drop thisuseCallbackor complete its dependency set.Line 325 memoizes
ActionButtons, but the callback closes overhandleShare,handleExportProject,handleCreateApiDeployment, andhandleExportwithout listing them. That can leave the toolbar firing requests with stalesessionDetailsafter an org/session update. Either memoize those handlers and include them, or removeuseCallbackhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/header/Header.jsx` around lines 325 - 421, ActionButtons is memoized with useCallback but closes over handlers that are not in its dependency array (handleShare, handleExportProject, handleCreateApiDeployment, handleExport), causing stale requests; fix by either dropping the useCallback wrapper around ActionButtons, or add the missing dependencies to the dependency array (handleShare, handleExportProject, handleCreateApiDeployment, handleExport) — if those handlers themselves are recreated frequently, memoize them (e.g., with useCallback or useMemo where they are defined) so they are stable before adding them to ActionButtons' deps to avoid unnecessary re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 307-315: handleEditSubmit currently calls
useCustomToolStore.getState().updateCustomTool({ details: updatedData }) which
triggers the store's shallow-merge bug (it writes a stray state key and replaces
details wholesale). Instead, stop calling updateCustomTool in the save path and
merge the patched fields into the existing details: read the current details
from useCustomToolStore.getState(), shallow-merge updatedData into that details
object, and write back via a safe setter (or a new merge helper) so only patched
fields are updated and other fields like tool_id/created_by are preserved.
In `@frontend/src/layouts/menu-layout/MenuLayout.jsx`:
- Around line 60-63: Guard the fallback navigation so it only uses the orgName
when it is available: in the MenuLayout component where navigate is called (the
useEffect that currently does navigate(`/${sessionDetails.orgName}/workflows`)
and the second occurrence around lines 117-118), check that
sessionDetails.orgName is truthy before using it as the fallback target; if
orgName is not yet set, skip the fallback navigate (or postpone until
sessionDetails.orgName is populated) and still use location.state?.from when
present — update the navigate calls to only construct
`/${sessionDetails.orgName}/workflows` when sessionDetails.orgName exists.
- Around line 41-54: handleNavigateBack is only returning scrollToCardId and
drops the originating list context (page, pageSize, searchTerm); update the
merge logic in handleNavigateBack so it preserves the entire return state from
location.state.from (use the existing from/fromState variables) and then overlay
scrollToCardId from location.state if present; build mergedState as
{...fromState, ...(location.state?.scrollToCardId && {scrollToCardId:
location.state.scrollToCardId})} (or otherwise include all keys from
location.state.from/state) and keep nextState logic the same so returning from
detail pages restores page, pageSize, searchTerm and any other original keys.
---
Nitpick comments:
In `@frontend/src/components/custom-tools/header/Header.jsx`:
- Around line 325-421: ActionButtons is memoized with useCallback but closes
over handlers that are not in its dependency array (handleShare,
handleExportProject, handleCreateApiDeployment, handleExport), causing stale
requests; fix by either dropping the useCallback wrapper around ActionButtons,
or add the missing dependencies to the dependency array (handleShare,
handleExportProject, handleCreateApiDeployment, handleExport) — if those
handlers themselves are recreated frequently, memoize them (e.g., with
useCallback or useMemo where they are defined) so they are stable before adding
them to ActionButtons' deps to avoid unnecessary re-renders.
🪄 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: 1c29d7b4-7113-4199-9da3-457548b59801
📒 Files selected for processing (2)
frontend/src/components/custom-tools/header/Header.jsxfrontend/src/layouts/menu-layout/MenuLayout.jsx
athul-rs
approved these changes
Apr 1, 2026
Contributor
Code reviewFound 2 issues:
unstract/frontend/src/layouts/menu-layout/MenuLayout.jsx Lines 23 to 94 in 49c56d6
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
athul-rs
requested changes
Apr 1, 2026
Contributor
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
Left couple of comments please check.
… JSX element
- Rename CustomButtons (func) to customButtons (node) to fix React anti-pattern
where component functions defined inside render cause unmount/remount on every
state change instead of re-rendering
- Update all callers: convert useCallback wrappers to useMemo, inline component
functions to plain JSX elements
- Fix updateCustomTool merge bug in Header — use setState directly to avoid
stray state key from ({ state, ...entireState }) pattern
- Guard orgName in MenuLayout fallback route to prevent /undefined/workflows
- Remove redundant || "" subtitle fallback in Header
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Deepak-Kesavan
approved these changes
Apr 2, 2026
athul-rs
approved these changes
Apr 2, 2026
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
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
4 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.
What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
Related Issues or PRs
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.