feat(ui): Add UI components and Jotai bindings for forms and editors - #38
Merged
Conversation
Move FormLookupProvider from form-lookup-context.tsx to its own FormLookupProvider.tsx file to improve code organization and separation of concerns. Convert form-lookup-context to a pure TypeScript file containing only the context and hooks. Update imports in components.ts and JsonSchemaForm.tsx accordingly. Also add missing icon mappings (command, terminal, query, search) to surfaceIconMap and add comprehensive test coverage for resolveSurfaceIcon function. Gavel-Issue-Id: 543c19b7d5a043e24c0b74c0422c282a Claude-Session-Id: 51e23842-f658-46b8-909b-9be68a8f040a
Introduce a comprehensive oxlint plugin (`@flanksource/clicky-ui/oxlint-plugins`) that enforces best practices for projects consuming clicky-ui. The plugin includes five opt-in rules: - `prefer-clicky-components`: Flag native DOM elements (button, select, table, dialog) that should use clicky-ui equivalents - `no-adhoc-overlay`: Prevent ad-hoc dialogs and arbitrary z-index values; enforce Modal and centralized z-index scale - `prefer-tailwind-classes`: Replace static inline styles with Tailwind utilities and density classes - `prefer-theme-tokens`: Use semantic theme tokens instead of hardcoded colors; use hooks instead of raw storage access - `prefer-clicky-icons`: Enforce approved icon sources and real icon components instead of emoji or raw iconify names Include comprehensive documentation, unit tests, and update TaskManager/TaskProgress to use the Icon component with generated Ui* icons instead of iconify-icon web component. Simplify no-iconify-names rule to leverage shared helpers.
… support Import the optional MDXEditor CSS stylesheet in Storybook preview to support the markdown field (JsonSchemaForm `format: md`) rendering in the component catalog. Gavel-Issue-Id: fbab5abc5b9738a9e5be37a804118e43 Claude-Session-Id: 4ccf888c-ca18-4f65-b9e2-45ac01115050
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a markdown editor-backed ChangesMDX editor markdown field for JsonSchemaForm
ListMenu multi-select component
Jotai atom bindings for FilterBar and JsonSchemaForm
Prompt management UI
clicky-ui oxlint plugin suite
AI tool preference grouping, icon migration, and infra
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
Contributor
Gavel summary
Totals: 1901 passed · 0 failed · 1 skipped · 4m7s |
- storybook: pre-bundle @mdxeditor/editor through the workspace package (@flanksource/clicky-ui > @mdxeditor/editor) instead of as a bare specifier — it lives in packages/ui/node_modules, so the bare form was unresolvable from the storybook app's cold-cache optimizeDeps pass and triggered a Vite reload that cascaded into unresolved storybook/test. - oxlint-plugins: drop stale no-iconify-names.test.ts — it imported isIconifyName from no-iconify-names.js (which no longer exports it; the helper moved to clicky-ui-shared.js) and asserted pre-allowlist behavior. The helper is already covered by clicky-ui.test.ts. - chat: remove unnecessary escape in clickyOperationsToTools regex char class.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
packages/ui/src/data/PromptDialog.test.tsx-28-30 (1)
28-30: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUndo the global fetch stub in cleanup
vi.restoreAllMocks()does not removevi.stubGlobal('fetch', ...), so the mockedfetchcan leak into later tests. Addvi.unstubAllGlobals()to theafterEachcleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/data/PromptDialog.test.tsx` around lines 28 - 30, The test cleanup in PromptDialog.test.tsx only restores spies, so the global fetch stub created with vi.stubGlobal can leak into later tests. Update the afterEach cleanup to also call vi.unstubAllGlobals() alongside vi.restoreAllMocks(), using the existing afterEach block in PromptDialog.test.tsx to ensure all globals are reset between tests.packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js-56-59 (1)
56-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
roleliterals before checking them.Line 58 only catches raw
role="dialog"literals.role={"dialog"}androle={"alertdialog"}are equivalent JSX and currently slip past the rule.Suggested change
function checkDialogRole(node, context) { if (attributeName(node) !== "role") return; - const value = node.value?.type === "Literal" ? node.value.value : undefined; + const value = literalString(node.value);// add to the import list at the top of the file literalString,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js` around lines 56 - 59, The role check in checkDialogRole only handles raw Literal values, so equivalent JSX forms like role={"dialog"} and role={"alertdialog"} are missed. Update the value extraction in checkDialogRole to normalize string-like role expressions using literalString before calling isDialogRole, and add the needed import in clicky-ui-no-adhoc-overlay so the rule catches both direct literals and wrapped string literals consistently.packages/ui/src/components/JsonSchemaForm.test.tsx-233-236 (1)
233-236: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis only proves the fallback textarea path.
MarkdownControlrenders a labeled<textarea>fallback immediately, whileMdxEditorFieldloads the real editor asynchronously. SogetByLabelText("Body")+fireEvent.change(...)can pass even if the MDXEditor module never hydrates. Please wait for or mock the loaded editor path so this test actually covers the new integration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/JsonSchemaForm.test.tsx` around lines 233 - 236, The current JsonSchemaForm test only exercises the immediate textarea fallback, so it can pass without ever verifying the asynchronously loaded MDX editor. Update the test around the Body field to wait for or mock the MdxEditorField/MDXEditor loaded path instead of relying on the fallback textarea, and then assert the change flow through the real editor integration using the existing JsonSchemaForm and onChange expectations.packages/ui/README.md-57-66 (1)
57-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
@mdxeditor/editorpeer install here.This section explains the extra CSS import, but not the optional peer dependency that actually enables the rich editor. As written, consumers can follow the README and still miss the MDXEditor package, which leaves them with the degraded fallback experience instead of the feature being documented.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/README.md` around lines 57 - 66, The Markdown editor docs are missing the required `@mdxeditor/editor` peer dependency, so consumers may add the CSS import but still not get the rich editor. Update the `Markdown editor field` section in `packages/ui/README.md` to explicitly mention that `MdxEditorField` and `JsonSchemaForm` with `format: md` require installing `@mdxeditor/editor`, alongside the existing `mdx-editor.css` import guidance. Use the existing `JsonSchemaForm` and `MdxEditorField` references to place the note near the import example.
🧹 Nitpick comments (2)
packages/ui/src/data/ai/ChatWindow.test.tsx (1)
135-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid coupling these tests to
react-rnd’s internal class names.Waiting for
.react-draggableties the test to a third-party DOM detail instead of this component’s contract. Areact-rndupgrade can break these assertions without anyChatWindowbehavior change. Prefer an app-owned signal or mock the lazy import boundary instead.Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/data/ai/ChatWindow.test.tsx` around lines 135 - 137, The ChatWindow tests are tied to react-rnd’s internal .react-draggable DOM class, which makes them fragile across dependency upgrades. Update the affected assertions in ChatWindow.test.tsx to wait on an app-owned contract from ChatWindow instead, or mock the lazy react-rnd import boundary so the tests verify ChatWindow behavior without depending on third-party class names. Use the existing test cases around the draggable wait logic and replace both occurrences that query document.querySelector(".react-draggable").packages/ui/oxlint-plugins/clicky-ui.test.ts (1)
187-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rule-level regression test for
no-iconify-names.These cases only lock
isIconifyName(). The refactor inpackages/ui/oxlint-plugins/no-iconify-names.jschanged visitor scope, so quoted keys or backtick strings can regress without failing this suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/oxlint-plugins/clicky-ui.test.ts` around lines 187 - 216, Add a rule-level regression test for no-iconify-names, not just isIconifyName. Extend the existing clicky-ui test coverage to exercise the actual rule visitor behavior in no-iconify-names.js, including quoted object keys and backtick string cases, so changes to visitor scope are caught. Use the rule entrypoint and its visitor logic as the target for the new test cases, while keeping the current isIconifyName checks as helper coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js`:
- Around line 16-21: Update the arbitrary z-index matcher in
clicky-ui-no-adhoc-overlay.js so it also accepts Tailwind v4’s optional trailing
! after the closing bracket. Adjust ARBITRARY_Z_CLASS and keep
isArbitraryZIndexClass as the entry point for matching tokens like z-[999]! and
hover:z-[var(--overlay)]! while preserving existing variant-prefix support.
In `@packages/ui/oxlint-plugins/clicky-ui-prefer-icons.js`:
- Around line 92-99: The Literal visitor in clicky-ui-prefer-icons is too broad
and reports any string that looks like an Iconify name, even when it is
unrelated to an icon usage. Narrow the check in the Literal handler to only
inspect literals that are actually part of icon-bearing props or arguments,
using the existing rule helpers and context in this plugin to identify valid
icon positions before calling context.report. Keep the Iconify-name validation,
but gate it behind the specific AST parent/usage patterns handled elsewhere in
the rule so ordinary strings are not flagged.
In `@packages/ui/oxlint-plugins/clicky-ui-prefer-theme.js`:
- Around line 96-110: The storage check in checkStorageCall is matching any
storage method that uses a Clicky key, which incorrectly flags write operations
like setItem and removeItem as reads. Restrict this rule to read-only storage
access patterns in checkStorageCall by checking the callee method name before
reporting, so only getItem-style reads trigger the message while persistence
methods are ignored.
In `@packages/ui/oxlint-plugins/no-iconify-names.js`:
- Around line 25-27: The Literal(node) check in the no-iconify-names rule is too
broad and misses no-substitution template literals, so update the visitor logic
to also handle TemplateLiteral nodes with no expressions while restricting
reports to actual value positions only. Use the existing isIconifyName, message,
and context.report flow, but add guards so quoted keys and other non-value
literals are ignored and only props, arguments, and other real value usages are
reported.
In `@packages/ui/src/components/FormLookupProvider.tsx`:
- Around line 5-12: FormLookupProvider always mounts FormLookupContext.Provider
even when fetcher is undefined, which blocks inheritance from any outer
provider. Update FormLookupProvider so it only renders
FormLookupContext.Provider when fetcher is actually provided, otherwise return
children directly; keep the behavior localized to FormLookupProvider and the
FormLookupContext path used by JsonSchemaForm.
In `@packages/ui/src/components/jotai-bindings-core.ts`:
- Around line 81-92: `useAtomSubscriptions` in `jotai-bindings-core.ts`
subscribes too late via `useEffect`, leaving a render/commit gap where atom
updates can be missed and stale values committed. Update the hook to use a
render-safe subscription pattern tied to `useStore`, `useReducer`, and the
`uniqueAtoms` set so changes are observed during render/commit instead of only
after mount, and keep the cleanup logic for each atom subscription intact.
In `@packages/ui/src/components/json-schema-form-fields.tsx`:
- Around line 609-616: The LazyMdxEditorField props are being overridden by the
trailing spread of field.markdownOptions, which can let schema-provided values
replace controlled form props like id, value, readOnly, and onChange. Update the
prop order in json-schema-form-fields.tsx so markdownOptions is spread before
the controlled props in the LazyMdxEditorField usage, keeping fieldId,
toText(field.value), readOnly, defaultPlaceholder(field.schema), and the
onChange handler authoritative. Use the LazyMdxEditorField and
field.markdownOptions symbols to locate the change.
In `@packages/ui/src/components/ListMenu.tsx`:
- Around line 104-118: The checkbox in ListMenu’s row rendering uses a shared
aria-label, so every row is announced the same; update the checkbox in the row
item component to use a row-specific accessible name instead. Prefer wiring the
checkbox to the rendered row content with aria-labelledby (using the existing
children/row content container) or add a per-row label prop, and keep the
selection behavior in ctx.selection.toggle unchanged.
In `@packages/ui/src/components/mdx-editor-field-plugins.tsx`:
- Around line 82-90: The default code-block language in mdxEditorFieldPlugins is
currently set to a fallback key that does not exist in
DEFAULT_CODE_BLOCK_LANGUAGES, so newly inserted blocks can start with an
unrepresentable language. Update the mdx.codeBlockPlugin setup in
mdx-editor-field-plugins.tsx by either adding the missing key to
DEFAULT_CODE_BLOCK_LANGUAGES or changing defaultCodeBlockLanguage to one of the
existing language keys used by codeMirrorPlugin, keeping the two configurations
aligned.
In `@packages/ui/src/components/MdxEditorField.tsx`:
- Around line 86-98: The fallback textarea update path in MdxEditorField can be
lost when the editor mounts because commit() updates lastValueRef before the
real editor is available, causing the useEffect sync to skip
editorRef.current?.setMarkdown(value) when value matches lastValueRef.current.
Adjust the synchronization in MdxEditorField so that fallback edits still force
the newly loaded editor to receive the latest markdown, using the existing
useEffect/commit logic and the mdx, value, and lastValueRef.current checks to
ensure stale content cannot survive the chunk swap.
- Around line 63-65: The hydrated MDX editor is losing its accessible name
because MdxEditorField forwards id and aria-label to the wrapper instead of the
editable surface. Update MdxEditorField’s wiring so the form label remains
attached after hydration, using the existing editor setup path in MdxEditorField
and the MDXEditor/@mdxeditor/editor integration rather than relying on
unsupported public props. Keep the editable surface labelable by ensuring the
generated id/aria-label are applied where the actual editor content is rendered.
In `@packages/ui/src/components/use-list-menu-selection.ts`:
- Around line 44-47: The selection value returned from useListMenuSelection
currently exposes the live Set in uncontrolled mode, allowing outside mutation
to bypass commit, onSelectionChange, and rerendering. Update
useListMenuSelection so the internal state Set stays private for updates, and
return a cloned or readonly Set view from the selected memo instead of localSet
directly; apply the same pattern anywhere else the selection is exposed,
including the second occurrence noted in the review.
In `@packages/ui/src/data/ai/ChatWindow.tsx`:
- Around line 85-87: The seeding logic in ChatWindow’s tool preference
initialization currently takes the first defaultMode for each preferenceKey,
which makes shared keys depend on array order. Update the loop that builds the
initial state so it aggregates all tools with the same preferenceKey and
resolves their defaults using the same mostRestrictiveMode() behavior already
used by the grouped UI. Ensure the initial toolPreferences map is derived from
grouped defaults rather than a first-seen fallback.
In `@packages/ui/src/data/chat/clickyOperationsToTools.ts`:
- Around line 184-187: Update isReadLikeOperation so it matches whole read-like
tokens instead of any raw substring inside operationId, since the current regex
can misclassify unrelated write operations as read-like. Adjust the matching
logic in that helper to use token/word boundaries or explicit token parsing for
verbs like list, load, get, and history, then keep the existing callers of
isReadLikeOperation unchanged so only genuine read operations are mapped to
"Admin Read".
In `@packages/ui/src/data/PromptBanner.model.ts`:
- Around line 89-103: The fallback in findBooleanAction is too broad because it
auto-selects any boolean property and makes PromptBanner treat non-approval
booleans as approve/reject actions. Update findBooleanAction to only return an
action for clearly approval-like boolean fields from the preferred list, and
remove or tighten the generic Object.entries fallback so unrelated schemas like
notifySlack do not bypass the full dialog.
In `@packages/ui/src/data/PromptBanner.tsx`:
- Around line 60-71: The inline form reset in PromptBanner is tied to the
inlineSpec object identity, so refreshes from usePrompts keep wiping user input
and error state. Update the useEffect that calls setComment, setError, and
setResolving to depend on stable active prompt identity instead of the recreated
inlineSpec object, and derive any initial comment content from a stable prompt
identifier or computed fields inside PromptBanner so the form only resets when
the actual active prompt changes.
- Line 17: PromptBannerProps currently exposes state via PromptFilter, but
PromptBanner ignores it and always calls usePrompts with the default banner
filter. Update PromptBanner and its props handling so state is either forwarded
into the usePrompts query/filter path or removed from the public props if
unsupported; make sure any destructuring that sends values to _ignoredState is
replaced with the real state-aware behavior. Check the PromptBanner component
and PromptBannerProps definition so the public API matches the actual query
behavior.
- Around line 80-94: The inline resolution flow in resolveInline only clears
active after answerPrompt succeeds, so the prompt can still be shown by
usePrompts until the async refresh completes. Update the local prompt state
immediately on success by also hiding/removing the current prompt from the UI
state in PromptBanner (using activePrompt/id and the active/setActive state), so
the banner disappears before the next refresh and cannot be submitted twice.
In `@packages/ui/src/data/PromptDialog.tsx`:
- Around line 22-25: The PromptDialog component keeps stale form and error state
because the useState initializers in PromptDialog only run on first mount. Add
logic in PromptDialog to reset value, busy, and error whenever prompt or open
changes, so reopening the Modal for a different prompt starts from prompt.value
instead of reusing prior input. Use the PromptDialogProps inputs and the
existing state setters in PromptDialog to reinitialize state on open changes and
prompt.id changes.
In `@packages/ui/src/hooks/use-prompts.ts`:
- Around line 100-139: Reset the prompt state immediately when the subscription
inputs change, because useEffect in use-prompts.ts currently keeps the previous
prompts array until the next poll/SSE update arrives. Update the effect around
the prompts subscription logic to clear or reinitialize prompts when the query
key changes (owner, kind, stateFilter, labelsKey, basePath, enabled, forcePoll),
so PromptBanner does not briefly render stale data from the previous namespace.
---
Minor comments:
In `@packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js`:
- Around line 56-59: The role check in checkDialogRole only handles raw Literal
values, so equivalent JSX forms like role={"dialog"} and role={"alertdialog"}
are missed. Update the value extraction in checkDialogRole to normalize
string-like role expressions using literalString before calling isDialogRole,
and add the needed import in clicky-ui-no-adhoc-overlay so the rule catches both
direct literals and wrapped string literals consistently.
In `@packages/ui/README.md`:
- Around line 57-66: The Markdown editor docs are missing the required
`@mdxeditor/editor` peer dependency, so consumers may add the CSS import but
still not get the rich editor. Update the `Markdown editor field` section in
`packages/ui/README.md` to explicitly mention that `MdxEditorField` and
`JsonSchemaForm` with `format: md` require installing `@mdxeditor/editor`,
alongside the existing `mdx-editor.css` import guidance. Use the existing
`JsonSchemaForm` and `MdxEditorField` references to place the note near the
import example.
In `@packages/ui/src/components/JsonSchemaForm.test.tsx`:
- Around line 233-236: The current JsonSchemaForm test only exercises the
immediate textarea fallback, so it can pass without ever verifying the
asynchronously loaded MDX editor. Update the test around the Body field to wait
for or mock the MdxEditorField/MDXEditor loaded path instead of relying on the
fallback textarea, and then assert the change flow through the real editor
integration using the existing JsonSchemaForm and onChange expectations.
In `@packages/ui/src/data/PromptDialog.test.tsx`:
- Around line 28-30: The test cleanup in PromptDialog.test.tsx only restores
spies, so the global fetch stub created with vi.stubGlobal can leak into later
tests. Update the afterEach cleanup to also call vi.unstubAllGlobals() alongside
vi.restoreAllMocks(), using the existing afterEach block in
PromptDialog.test.tsx to ensure all globals are reset between tests.
---
Nitpick comments:
In `@packages/ui/oxlint-plugins/clicky-ui.test.ts`:
- Around line 187-216: Add a rule-level regression test for no-iconify-names,
not just isIconifyName. Extend the existing clicky-ui test coverage to exercise
the actual rule visitor behavior in no-iconify-names.js, including quoted object
keys and backtick string cases, so changes to visitor scope are caught. Use the
rule entrypoint and its visitor logic as the target for the new test cases,
while keeping the current isIconifyName checks as helper coverage.
In `@packages/ui/src/data/ai/ChatWindow.test.tsx`:
- Around line 135-137: The ChatWindow tests are tied to react-rnd’s internal
.react-draggable DOM class, which makes them fragile across dependency upgrades.
Update the affected assertions in ChatWindow.test.tsx to wait on an app-owned
contract from ChatWindow instead, or mock the lazy react-rnd import boundary so
the tests verify ChatWindow behavior without depending on third-party class
names. Use the existing test cases around the draggable wait logic and replace
both occurrences that query document.querySelector(".react-draggable").
🪄 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: dcfde0e6-7847-43fe-a91c-304a02724217
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (72)
.grite/export.jsonapps/storybook/.storybook/main.tsapps/storybook/.storybook/preview.tsxpackages/ui/.oxlintrc.jsonpackages/ui/README.mdpackages/ui/oxlint-plugins/README.mdpackages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.jspackages/ui/oxlint-plugins/clicky-ui-prefer-components.jspackages/ui/oxlint-plugins/clicky-ui-prefer-icons.jspackages/ui/oxlint-plugins/clicky-ui-prefer-tailwind.jspackages/ui/oxlint-plugins/clicky-ui-prefer-theme.jspackages/ui/oxlint-plugins/clicky-ui-shared.jspackages/ui/oxlint-plugins/clicky-ui.jspackages/ui/oxlint-plugins/clicky-ui.test.tspackages/ui/oxlint-plugins/no-iconify-names.jspackages/ui/oxlint-plugins/no-iconify-names.test.tspackages/ui/package.jsonpackages/ui/scripts/build-styles.mjspackages/ui/src/components.tspackages/ui/src/components/FormLookupProvider.tsxpackages/ui/src/components/JsonSchemaForm.stories.tsxpackages/ui/src/components/JsonSchemaForm.test.tsxpackages/ui/src/components/JsonSchemaForm.tsxpackages/ui/src/components/ListMenu.stories.tsxpackages/ui/src/components/ListMenu.test.tsxpackages/ui/src/components/ListMenu.tsxpackages/ui/src/components/MdxEditorField.tsxpackages/ui/src/components/MdxEditorToolbar.tsxpackages/ui/src/components/form-lookup-context.tspackages/ui/src/components/jotai-bindings-core.tspackages/ui/src/components/jotai-bindings.stories.tsxpackages/ui/src/components/jotai-bindings.test.tsxpackages/ui/src/components/jotai-bindings.tsxpackages/ui/src/components/json-schema-form-fields.tsxpackages/ui/src/components/json-schema-form-render.tsxpackages/ui/src/components/json-schema-form-resolve.test.tspackages/ui/src/components/json-schema-form-resolve.tspackages/ui/src/components/json-schema-form-types.tspackages/ui/src/components/mdx-editor-field-plugins.tsxpackages/ui/src/components/mdx-editor-options.tspackages/ui/src/components/use-list-menu-selection.tspackages/ui/src/data.tspackages/ui/src/data/PromptBanner.model.tspackages/ui/src/data/PromptBanner.stories.tsxpackages/ui/src/data/PromptBanner.test.tsxpackages/ui/src/data/PromptBanner.tsxpackages/ui/src/data/PromptDialog.stories.tsxpackages/ui/src/data/PromptDialog.test.tsxpackages/ui/src/data/PromptDialog.tsxpackages/ui/src/data/TaskManager.tsxpackages/ui/src/data/TaskProgress.tsxpackages/ui/src/data/ai/ChatWindow.test.tsxpackages/ui/src/data/ai/ChatWindow.tsxpackages/ui/src/data/ai/ToolPreferences.tsxpackages/ui/src/data/chat/clickyOperationsToTools.test.tspackages/ui/src/data/chat/clickyOperationsToTools.tspackages/ui/src/data/chat/types.tspackages/ui/src/data/task-status.tspackages/ui/src/hooks.tspackages/ui/src/hooks/use-prompts.tspackages/ui/src/jotai.tspackages/ui/src/lib/utils.test.tspackages/ui/src/lib/utils.tspackages/ui/src/mdx-editor.tspackages/ui/src/rpc/apiClient.tspackages/ui/src/rpc/surfaceIconMap.test.tspackages/ui/src/rpc/surfaceIconMap.tspackages/ui/src/rpc/types.tspackages/ui/src/styles/mdx-editor.csspackages/ui/src/styles/tokens.csspackages/ui/vite.config.tspnpm-workspace.yaml
💤 Files with no reviewable changes (2)
- .grite/export.json
- packages/ui/oxlint-plugins/no-iconify-names.test.ts
Comment on lines
+16
to
+21
| const ARBITRARY_Z_CLASS = /(?:^|:)z-\[[^\]]+\]$/; | ||
| const DIALOG_ROLES = new Set(["dialog", "alertdialog"]); | ||
|
|
||
| /** True for an arbitrary `z-[…]` Tailwind class token (variant prefixes allowed). */ | ||
| export function isArbitraryZIndexClass(token) { | ||
| return typeof token === "string" && ARBITRARY_Z_CLASS.test(token); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Tailwind CSS v4, is the important modifier written after the utility name (for example z-[999]!)?
💡 Result:
Yes, in Tailwind CSS v4, the convention for the important modifier is to place the exclamation mark (!) at the end of the utility class (for example, z-[999]!) [1][2]. While the previous v3 syntax—placing the ! at the beginning of the utility class (e.g., !z-[999])—remains supported for backwards compatibility, it is considered deprecated [1][3]. Moving the modifier to the end is the recommended approach in v4 to improve clarity and consistency, particularly when working with variants [1][4]. This change also applies to the @apply directive, where utilities should now be marked as important by appending the ! suffix to each class [4][3].
Citations:
- 1: https://tailwindcss.com/docs/upgrade-guide
- 2: https://github.com/dxos/dxos/blob/e4919fd46f87a8107cac7af0def56ea6136edc1f/TAILWIND_V4_UTILITY_CHANGES.md
- 3: PostCSS + TailwindCSS 4 throws error on important! and ! modifiers tailwindlabs/tailwindcss#18570
- 4: Add initial codemod tooling tailwindlabs/tailwindcss#14434
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js
cat -n packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js | sed -n '1,220p'Repository: flanksource/clicky-ui
Length of output: 3650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n packages/ui/oxlint-plugins/clicky-ui-shared.js | sed -n '1,260p'Repository: flanksource/clicky-ui
Length of output: 5761
Allow Tailwind v4's trailing ! in the arbitrary z-index matcher.
ARBITRARY_Z_CLASS currently stops at ], so classes like z-[999]! and hover:z-[var(--overlay)]! bypass the rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/oxlint-plugins/clicky-ui-no-adhoc-overlay.js` around lines 16 -
21, Update the arbitrary z-index matcher in clicky-ui-no-adhoc-overlay.js so it
also accepts Tailwind v4’s optional trailing ! after the closing bracket. Adjust
ARBITRARY_Z_CLASS and keep isArbitraryZIndexClass as the entry point for
matching tokens like z-[999]! and hover:z-[var(--overlay)]! while preserving
existing variant-prefix support.
Comment on lines
+92
to
+99
| Literal(node) { | ||
| if (isIconifyName(node.value)) { | ||
| context.report({ | ||
| message: `Iconify icon name "${node.value}" passed as a string. ${APPROVED_HINT} and pass the component.`, | ||
| node, | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't report every iconify-looking string literal.
Lines 92-99 fire on any Literal in the file, not just icon-bearing props/arguments. That will flag unrelated strings whenever they happen to look like prefix:name, which makes the rule noisy fast.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/oxlint-plugins/clicky-ui-prefer-icons.js` around lines 92 - 99,
The Literal visitor in clicky-ui-prefer-icons is too broad and reports any
string that looks like an Iconify name, even when it is unrelated to an icon
usage. Narrow the check in the Literal handler to only inspect literals that are
actually part of icon-bearing props or arguments, using the existing rule
helpers and context in this plugin to identify valid icon positions before
calling context.report. Keep the Iconify-name validation, but gate it behind the
specific AST parent/usage patterns handled elsewhere in the rule so ordinary
strings are not flagged.
Comment on lines
+96
to
+110
| function checkStorageCall(node, context) { | ||
| const callee = node.callee; | ||
| if (callee?.type !== "MemberExpression") return; | ||
| if (!isStorageObject(callee.object)) return; | ||
| for (const arg of node.arguments) { | ||
| if (isClickyStorageKey(literalString(arg))) { | ||
| context.report({ | ||
| message: | ||
| `Reading "${arg.value}" from storage by hand. Use clicky-ui's useTheme()/` + | ||
| "useDensity() hooks (or ThemeProvider/DensityProvider) instead.", | ||
| node, | ||
| }); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only flag storage reads here.
Lines 97-105 report any storage call that mentions a clicky key, so setItem("clicky-ui-theme", ...) and removeItem(...) get the same "Reading ... by hand" error. That turns legitimate persistence paths into false positives.
Suggested fix
function checkStorageCall(node, context) {
const callee = node.callee;
if (callee?.type !== "MemberExpression") return;
if (!isStorageObject(callee.object)) return;
+ const method =
+ callee.property?.type === "Identifier" ? callee.property.name : undefined;
+ if (method !== "getItem") return;
for (const arg of node.arguments) {
if (isClickyStorageKey(literalString(arg))) {
context.report({📝 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.
Suggested change
| function checkStorageCall(node, context) { | |
| const callee = node.callee; | |
| if (callee?.type !== "MemberExpression") return; | |
| if (!isStorageObject(callee.object)) return; | |
| for (const arg of node.arguments) { | |
| if (isClickyStorageKey(literalString(arg))) { | |
| context.report({ | |
| message: | |
| `Reading "${arg.value}" from storage by hand. Use clicky-ui's useTheme()/` + | |
| "useDensity() hooks (or ThemeProvider/DensityProvider) instead.", | |
| node, | |
| }); | |
| return; | |
| } | |
| } | |
| function checkStorageCall(node, context) { | |
| const callee = node.callee; | |
| if (callee?.type !== "MemberExpression") return; | |
| if (!isStorageObject(callee.object)) return; | |
| const method = | |
| callee.property?.type === "Identifier" ? callee.property.name : undefined; | |
| if (method !== "getItem") return; | |
| for (const arg of node.arguments) { | |
| if (isClickyStorageKey(literalString(arg))) { | |
| context.report({ | |
| message: | |
| `Reading "${arg.value}" from storage by hand. Use clicky-ui's useTheme()/` + | |
| "useDensity() hooks (or ThemeProvider/DensityProvider) instead.", | |
| node, | |
| }); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/oxlint-plugins/clicky-ui-prefer-theme.js` around lines 96 - 110,
The storage check in checkStorageCall is matching any storage method that uses a
Clicky key, which incorrectly flags write operations like setItem and removeItem
as reads. Restrict this rule to read-only storage access patterns in
checkStorageCall by checking the callee method name before reporting, so only
getItem-style reads trigger the message while persistence methods are ignored.
Comment on lines
+25
to
+27
| Literal(node) { | ||
| if (isIconifyName(node.value)) { | ||
| context.report({ message: message(node.value), node }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '# Current rule implementation\n'
sed -n '1,120p' packages/ui/oxlint-plugins/no-iconify-names.js
printf '\n# Backtick-wrapped iconify names that a Literal visitor will miss\n'
rg -nP '`(?:ph|lucide|codicon|tabler|mdi|svg-spinners|jb-expui-[a-z]+):[a-z0-9][a-z0-9-]*`' packages/ui || true
printf '\n# Quoted iconify-like object keys that a generic Literal visitor may flag spuriously\n'
rg -nP '"(?:ph|lucide|codicon|tabler|mdi|svg-spinners|jb-expui-[a-z]+):[a-z0-9][a-z0-9-]*"\s*:' packages/ui || trueRepository: flanksource/clicky-ui
Length of output: 2012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '# Rule references\n'
rg -n "no-iconify-names|iconify-style icon name|values, arguments, or props|clicky-icons" packages/ui -S || true
printf '\n# Related plugin docs and tests\n'
fd -a "no-iconify-names" packages/ui || true
fd -a "*iconify*" packages/ui || true
printf '\n# Inspect the rule docs around the reported behavior\n'
sed -n '1,140p' packages/ui/oxlint-plugins/README.mdRepository: flanksource/clicky-ui
Length of output: 7408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '# no-iconify-names rule file\n'
cat -n packages/ui/oxlint-plugins/no-iconify-names.js
printf '\n# Tests around icon rules\n'
sed -n '1,320p' packages/ui/oxlint-plugins/clicky-ui.test.tsRepository: flanksource/clicky-ui
Length of output: 8597
Handle template literals and non-value literals. Literal(node) misses no-substitution template literals like icon={`lucide:activity`}, and it also reaches quoted keys/other non-value literal positions that aren’t values, arguments, or props. That leaves the rule both incomplete and noisy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/oxlint-plugins/no-iconify-names.js` around lines 25 - 27, The
Literal(node) check in the no-iconify-names rule is too broad and misses
no-substitution template literals, so update the visitor logic to also handle
TemplateLiteral nodes with no expressions while restricting reports to actual
value positions only. Use the existing isIconifyName, message, and
context.report flow, but add guards so quoted keys and other non-value literals
are ignored and only props, arguments, and other real value usages are reported.
Comment on lines
+5
to
+12
| export function FormLookupProvider({ | ||
| fetcher, | ||
| children, | ||
| }: { | ||
| fetcher?: LookupFetcher; | ||
| children: ReactNode; | ||
| }) { | ||
| return <FormLookupContext.Provider value={fetcher}>{children}</FormLookupContext.Provider>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't shadow an outer lookup provider with undefined.
Line 12 always mounts a provider, so JsonSchemaForm cannot inherit a parent FormLookupContext.Provider when lookupFetcher is omitted. That breaks the newly exported context path.
Proposed fix
export function FormLookupProvider({
fetcher,
children,
}: {
fetcher?: LookupFetcher;
children: ReactNode;
}) {
- return <FormLookupContext.Provider value={fetcher}>{children}</FormLookupContext.Provider>;
+ if (fetcher === undefined) {
+ return <>{children}</>;
+ }
+
+ return <FormLookupContext.Provider value={fetcher}>{children}</FormLookupContext.Provider>;
}📝 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.
Suggested change
| export function FormLookupProvider({ | |
| fetcher, | |
| children, | |
| }: { | |
| fetcher?: LookupFetcher; | |
| children: ReactNode; | |
| }) { | |
| return <FormLookupContext.Provider value={fetcher}>{children}</FormLookupContext.Provider>; | |
| export function FormLookupProvider({ | |
| fetcher, | |
| children, | |
| }: { | |
| fetcher?: LookupFetcher; | |
| children: ReactNode; | |
| }) { | |
| if (fetcher === undefined) { | |
| return <>{children}</>; | |
| } | |
| return <FormLookupContext.Provider value={fetcher}>{children}</FormLookupContext.Provider>; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/FormLookupProvider.tsx` around lines 5 - 12,
FormLookupProvider always mounts FormLookupContext.Provider even when fetcher is
undefined, which blocks inheritance from any outer provider. Update
FormLookupProvider so it only renders FormLookupContext.Provider when fetcher is
actually provided, otherwise return children directly; keep the behavior
localized to FormLookupProvider and the FormLookupContext path used by
JsonSchemaForm.
| type PromptInlineDecision, | ||
| } from "./PromptBanner.model"; | ||
|
|
||
| export interface PromptBannerProps extends PromptFilter { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
state is in the public props but never affects the query.
PromptBannerProps extends PromptFilter, but the implementation drops state into _ignoredState and never passes it to usePrompts. A consumer providing state="answered" or state="expired" will still get the default banner behavior instead of the requested filter.
Minimal safe fix if `state` is intentionally unsupported
-export interface PromptBannerProps extends PromptFilter {
+export interface PromptBannerProps extends Omit<PromptFilter, "state"> {
@@
- state: _ignoredState,
...filter
}: PromptBannerProps) {Also applies to: 44-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/data/PromptBanner.tsx` at line 17, PromptBannerProps
currently exposes state via PromptFilter, but PromptBanner ignores it and always
calls usePrompts with the default banner filter. Update PromptBanner and its
props handling so state is either forwarded into the usePrompts query/filter
path or removed from the public props if unsupported; make sure any
destructuring that sends values to _ignoredState is replaced with the real
state-aware behavior. Check the PromptBanner component and PromptBannerProps
definition so the public API matches the actual query behavior.
Comment on lines
+60
to
+71
| const inlineSpec = useMemo( | ||
| () => (inlineActions === "auto" && activePrompt ? analyzePromptInlineActions(activePrompt) : null), | ||
| [activePrompt, inlineActions], | ||
| ); | ||
| const summary = useMemo(() => summarizePrompts(prompts), [prompts]); | ||
| const recent = prompts.slice(0, Math.max(0, historyLimit)); | ||
|
|
||
| useEffect(() => { | ||
| setComment(activePrompt ? initialPromptComment(activePrompt, inlineSpec) : ""); | ||
| setError(null); | ||
| setResolving(null); | ||
| }, [activePrompt?.id, inlineSpec]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reset the inline form from inlineSpec object identity.
usePrompts rebuilds the prompt objects on every stream/poll update, so inlineSpec is recreated even when the active prompt is still the same one. This effect will then wipe the comment a user is typing and clear any inline error state on each refresh.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/data/PromptBanner.tsx` around lines 60 - 71, The inline form
reset in PromptBanner is tied to the inlineSpec object identity, so refreshes
from usePrompts keep wiping user input and error state. Update the useEffect
that calls setComment, setError, and setResolving to depend on stable active
prompt identity instead of the recreated inlineSpec object, and derive any
initial comment content from a stable prompt identifier or computed fields
inside PromptBanner so the form only resets when the actual active prompt
changes.
Comment on lines
+80
to
+94
| const resolveInline = async (decision: PromptInlineDecision) => { | ||
| if (!activePrompt || !inlineSpec) return; | ||
| setResolving(decision); | ||
| setError(null); | ||
| try { | ||
| await answerPrompt(basePath, activePrompt.id, { | ||
| values: buildPromptInlineValues(activePrompt, inlineSpec, decision, comment), | ||
| }); | ||
| setActive(null); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : String(e)); | ||
| } finally { | ||
| setResolving(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Hide a prompt locally as soon as inline resolution succeeds.
After answerPrompt returns, this code only clears active. The banner still renders from usePrompts, which is refreshed asynchronously, so the same prompt can stay visible with enabled actions long enough to submit the answer twice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/data/PromptBanner.tsx` around lines 80 - 94, The inline
resolution flow in resolveInline only clears active after answerPrompt succeeds,
so the prompt can still be shown by usePrompts until the async refresh
completes. Update the local prompt state immediately on success by also
hiding/removing the current prompt from the UI state in PromptBanner (using
activePrompt/id and the active/setActive state), so the banner disappears before
the next refresh and cannot be submitted twice.
Comment on lines
+22
to
+25
| export function PromptDialog({ prompt, basePath, open, onClose, onResolved }: PromptDialogProps) { | ||
| const [value, setValue] = useState<Record<string, unknown>>(() => ({ ...prompt.value })); | ||
| const [busy, setBusy] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset dialog state when prompt or open changes.
These useState initializers only run on the first mount. Because Modal hides itself without remounting PromptDialog, reopening this component for a different prompt reuses the previous form values/error state and can submit stale fields to the new prompt.id.
Suggested fix
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
export function PromptDialog({ prompt, basePath, open, onClose, onResolved }: PromptDialogProps) {
const [value, setValue] = useState<Record<string, unknown>>(() => ({ ...prompt.value }));
const [busy, setBusy] = useState(false);
const [error, setError] = useState<string | null>(null);
+
+ useEffect(() => {
+ if (!open) return;
+ setValue({ ...(prompt.value ?? {}) });
+ setBusy(false);
+ setError(null);
+ }, [open, prompt.id, prompt.value]);📝 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.
Suggested change
| export function PromptDialog({ prompt, basePath, open, onClose, onResolved }: PromptDialogProps) { | |
| const [value, setValue] = useState<Record<string, unknown>>(() => ({ ...prompt.value })); | |
| const [busy, setBusy] = useState(false); | |
| const [error, setError] = useState<string | null>(null); | |
| export function PromptDialog({ prompt, basePath, open, onClose, onResolved }: PromptDialogProps) { | |
| const [value, setValue] = useState<Record<string, unknown>>(() => ({ ...prompt.value })); | |
| const [busy, setBusy] = useState(false); | |
| const [error, setError] = useState<string | null>(null); | |
| useEffect(() => { | |
| if (!open) return; | |
| setValue({ ...(prompt.value ?? {}) }); | |
| setBusy(false); | |
| setError(null); | |
| }, [open, prompt.id, prompt.value]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/data/PromptDialog.tsx` around lines 22 - 25, The PromptDialog
component keeps stale form and error state because the useState initializers in
PromptDialog only run on first mount. Add logic in PromptDialog to reset value,
busy, and error whenever prompt or open changes, so reopening the Modal for a
different prompt starts from prompt.value instead of reusing prior input. Use
the PromptDialogProps inputs and the existing state setters in PromptDialog to
reinitialize state on open changes and prompt.id changes.
Comment on lines
+100
to
+139
| useEffect(() => { | ||
| if (!enabled) return; | ||
| const query = filterQuery({ owner, kind, state: stateFilter, labels }); | ||
|
|
||
| if (forcePoll || !hasEventSource()) { | ||
| let stopped = false; | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const tick = async () => { | ||
| try { | ||
| const res = await fetch(`${basePath}/prompts${query ? `?${query}` : ""}`, { | ||
| headers: { Accept: "application/json" }, | ||
| }); | ||
| if (res.ok) { | ||
| setPrompts((await res.json()) as PromptSnapshot[]); | ||
| setStatus("polling"); | ||
| } | ||
| } catch { | ||
| setStatus("connection lost — retrying"); | ||
| } | ||
| if (!stopped) timer = setTimeout(tick, pollMs); | ||
| }; | ||
| void tick(); | ||
| return () => { | ||
| stopped = true; | ||
| if (timer) clearTimeout(timer); | ||
| }; | ||
| } | ||
|
|
||
| const es = new EventSource(`${basePath}/prompts/stream${query ? `?${query}` : ""}`); | ||
| setStatus("connected"); | ||
| es.addEventListener("prompts", (e) => { | ||
| try { | ||
| setPrompts(JSON.parse((e as MessageEvent).data) as PromptSnapshot[]); | ||
| } catch { | ||
| /* ignore malformed frame */ | ||
| } | ||
| }); | ||
| es.onerror = () => setStatus("connection lost — retrying"); | ||
| return () => es.close(); | ||
| }, [owner, kind, stateFilter, labelsKey, basePath, enabled, pollMs, forcePoll]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset prompt state when the subscription key changes.
This effect leaves the previous prompts array in place until the next SSE frame/poll response arrives. PromptBanner renders directly from that state (packages/ui/src/data/PromptBanner.tsx:47-52), so a basePath or filter change can briefly show stale prompts and resolve them against the wrong namespace.
Suggested fix
const labelsKey = JSON.stringify(labels ?? {});
useEffect(() => {
- if (!enabled) return;
+ setPrompts([]);
+ setStatus(enabled ? "connecting" : "idle");
+ if (!enabled) return;
const query = filterQuery({ owner, kind, state: stateFilter, labels });📝 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.
Suggested change
| useEffect(() => { | |
| if (!enabled) return; | |
| const query = filterQuery({ owner, kind, state: stateFilter, labels }); | |
| if (forcePoll || !hasEventSource()) { | |
| let stopped = false; | |
| let timer: ReturnType<typeof setTimeout> | undefined; | |
| const tick = async () => { | |
| try { | |
| const res = await fetch(`${basePath}/prompts${query ? `?${query}` : ""}`, { | |
| headers: { Accept: "application/json" }, | |
| }); | |
| if (res.ok) { | |
| setPrompts((await res.json()) as PromptSnapshot[]); | |
| setStatus("polling"); | |
| } | |
| } catch { | |
| setStatus("connection lost — retrying"); | |
| } | |
| if (!stopped) timer = setTimeout(tick, pollMs); | |
| }; | |
| void tick(); | |
| return () => { | |
| stopped = true; | |
| if (timer) clearTimeout(timer); | |
| }; | |
| } | |
| const es = new EventSource(`${basePath}/prompts/stream${query ? `?${query}` : ""}`); | |
| setStatus("connected"); | |
| es.addEventListener("prompts", (e) => { | |
| try { | |
| setPrompts(JSON.parse((e as MessageEvent).data) as PromptSnapshot[]); | |
| } catch { | |
| /* ignore malformed frame */ | |
| } | |
| }); | |
| es.onerror = () => setStatus("connection lost — retrying"); | |
| return () => es.close(); | |
| }, [owner, kind, stateFilter, labelsKey, basePath, enabled, pollMs, forcePoll]); | |
| useEffect(() => { | |
| setPrompts([]); | |
| setStatus(enabled ? "connecting" : "idle"); | |
| if (!enabled) return; | |
| const query = filterQuery({ owner, kind, state: stateFilter, labels }); | |
| if (forcePoll || !hasEventSource()) { | |
| let stopped = false; | |
| let timer: ReturnType<typeof setTimeout> | undefined; | |
| const tick = async () => { | |
| try { | |
| const res = await fetch(`${basePath}/prompts${query ? `?${query}` : ""}`, { | |
| headers: { Accept: "application/json" }, | |
| }); | |
| if (res.ok) { | |
| setPrompts((await res.json()) as PromptSnapshot[]); | |
| setStatus("polling"); | |
| } | |
| } catch { | |
| setStatus("connection lost — retrying"); | |
| } | |
| if (!stopped) timer = setTimeout(tick, pollMs); | |
| }; | |
| void tick(); | |
| return () => { | |
| stopped = true; | |
| if (timer) clearTimeout(timer); | |
| }; | |
| } | |
| const es = new EventSource(`${basePath}/prompts/stream${query ? `?${query}` : ""}`); | |
| setStatus("connected"); | |
| es.addEventListener("prompts", (e) => { | |
| try { | |
| setPrompts(JSON.parse((e as MessageEvent).data) as PromptSnapshot[]); | |
| } catch { | |
| /* ignore malformed frame */ | |
| } | |
| }); | |
| es.onerror = () => setStatus("connection lost — retrying"); | |
| return () => es.close(); | |
| }, [owner, kind, stateFilter, labelsKey, basePath, enabled, pollMs, forcePoll]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/hooks/use-prompts.ts` around lines 100 - 139, Reset the
prompt state immediately when the subscription inputs change, because useEffect
in use-prompts.ts currently keeps the previous prompts array until the next
poll/SSE update arrives. Update the effect around the prompts subscription logic
to clear or reinitialize prompts when the query key changes (owner, kind,
stateFilter, labelsKey, basePath, enabled, forcePoll), so PromptBanner does not
briefly render stale data from the previous namespace.
The "renders a very large tree" jsdom unit test auto-expands the fixture's failing branches and renders the whole tree. At depth 4 (~1024 leaves / 1364 nodes) that render OOM'd CI's memory-constrained shared vitest worker (STACK_TRACE_ERROR) once this package's suite grew — the same failure the earlier shared-payload fix (5fdb78d) addressed at the allocation level, now resurfaced at the render level. Reduce the tree to depth 3 (256 leaves / 340 nodes): a 4x lighter render that still scrolls and stresses the detail panes. Verified the test OOM'd at a 160MB heap before and passes down to 112MB after. The browser story renders the same fixture without the jsdom memory ceiling.
moshloop
added a commit
that referenced
this pull request
Jun 29, 2026
- storybook: pre-bundle @mdxeditor/editor through the workspace package (@flanksource/clicky-ui > @mdxeditor/editor) instead of as a bare specifier — it lives in packages/ui/node_modules, so the bare form was unresolvable from the storybook app's cold-cache optimizeDeps pass and triggered a Vite reload that cascaded into unresolved storybook/test. - oxlint-plugins: drop stale no-iconify-names.test.ts — it imported isIconifyName from no-iconify-names.js (which no longer exports it; the helper moved to clicky-ui-shared.js) and asserted pre-allowlist behavior. The helper is already covered by clicky-ui.test.ts. - chat: remove unnecessary escape in clickyOperationsToTools regex char class.
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
2 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.
Summary by CodeRabbit
New Features
Bug Fixes