fix(MULTIPLA-003-3): CU-86akhf8u5 4 review findings across 2 files - #389
flamingo[bot] wants to merge 2 commits into
Conversation
| variables: { filter, search }, | ||
| }); | ||
|
|
||
| const fetchIntegratedTools = useCallback( |
There was a problem hiding this comment.
🦩 🔴 use-integrated-tools.ts issues a raw GraphQL POST outside of react-query's queryFn/mutationFn
Replaced the manual useState/useCallback/apiClient.post flow in useIntegratedTools with a useQuery call (from @tanstack/react-query) whose queryFn wraps the same request logic (extracted to fetchIntegratedToolsRequest), giving it a queryKey of ['integrated-tools', filter, search] and cache semantics consistent with sibling hooks. fetchIntegratedTools is preserved as a thin wrapper around refetch() for backward compatibility with existing callers.
🤖 Prompt for AI agents
In src/app/(app)/settings/hooks/use-integrated-tools.ts around line 84, review and complete this code-review fix: use-integrated-tools.ts issues a raw GraphQL POST outside of react-query's queryFn/mutationFn.
What the draft fix changed: Replaced the manual useState/useCallback/apiClient.post flow in `useIntegratedTools` with a `useQuery` call (from `@tanstack/react-query`) whose `queryFn` wraps the same request logic (extracted to `fetchIntegratedToolsRequest`), giving it a `queryKey` of `['integrated-tools', filter, search]` and cache semantics consistent with sibling hooks. `fetchIntegratedTools` is preserved as a thin wrapper around `refetch()` for backward compatibility with existing callers.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| // NOTE: credentials (username/password/apiKey) are intentionally NOT requested here. | ||
| // If the UI needs to indicate that credentials exist, the server should expose a | ||
| // redacted/masked representation via a dedicated field instead of returning secrets. | ||
| const GET_INTEGRATED_TOOLS_QUERY = ` |
There was a problem hiding this comment.
🦩 🔴 use-integrated-tools.ts issues a raw GraphQL POST via apiClient instead of react-relay
This finding calls for migrating off raw GraphQL POST to react-relay entirely, which is an architectural change (requires a Relay environment/compiler setup not visible in this file) that cannot be safely completed within this single file. As a partial, safe mitigation I kept the fetch inside a react-query queryFn (satisfying MULTIPLA-003-3) rather than leaving it as an ad-hoc call outside query/mutation functions, but the raw GraphQL string + apiClient.post pattern itself remains — a full fix requires introducing react-relay tooling (RelayEnvironmentProvider, compiled query artifacts) at the app level, which is out of scope for a single-file change.
🤖 Prompt for AI agents
In src/app/(app)/settings/hooks/use-integrated-tools.ts around line 8, review and complete this code-review fix: use-integrated-tools.ts issues a raw GraphQL POST via apiClient instead of react-relay.
What the draft fix changed: This finding calls for migrating off raw GraphQL POST to react-relay entirely, which is an architectural change (requires a Relay environment/compiler setup not visible in this file) that cannot be safely completed within this single file. As a partial, safe mitigation I kept the fetch inside a react-query `queryFn` (satisfying MULTIPLA-003-3) rather than leaving it as an ad-hoc call outside query/mutation functions, but the raw GraphQL string + apiClient.post pattern itself remains — a full fix requires introducing react-relay tooling (RelayEnvironmentProvider, compiled query artifacts) at the app level, which is out of scope for a single-file change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
| category | ||
| platformCategory | ||
| enabled | ||
| credentials { |
There was a problem hiding this comment.
🦩 🟠 Integrated tools GraphQL query fetches raw plaintext credentials (username/password/apiKey) to the client
Removed the credentials { username password apiKey { key type keyName } } selection from GET_INTEGRATED_TOOLS_QUERY entirely, along with a comment explaining that any masked/redacted credential indicator should be a dedicated server-side field instead. The Credentials/ApiKey types and credentials field on IntegratedTool are left in place (unpopulated) in case other code in the file/consumers still reference the type shape; no plaintext secrets are requested from the server anymore.
🤖 Prompt for AI agents
In src/app/(app)/settings/hooks/use-integrated-tools.ts around line 26, review and complete this code-review fix: Integrated tools GraphQL query fetches raw plaintext credentials (username/password/apiKey) to the client.
What the draft fix changed: Removed the `credentials { username password apiKey { key type keyName } } ` selection from `GET_INTEGRATED_TOOLS_QUERY` entirely, along with a comment explaining that any masked/redacted credential indicator should be a dedicated server-side field instead. The `Credentials`/`ApiKey` types and `credentials` field on `IntegratedTool` are left in place (unpopulated) in case other code in the file/consumers still reference the type shape; no plaintext secrets are requested from the server anymore.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| ); | ||
| }); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🦩 🔴 use-ai-configuration.ts uses raw useState/useEffect data fetching instead of TanStack Query
Replaced manual useState/useEffect data fetching in useAiConfiguration (src/app/(app)/settings/hooks/use-ai-configuration.ts) with useQuery for both the AI configuration (AI_CONFIGURATION_QUERY_KEY) and supported models (SUPPORTED_MODELS_QUERY_KEY), and replaced the manual save/loading-flag logic with useMutation for updateConfiguration, which invalidates AI_CONFIGURATION_QUERY_KEY via queryClient.invalidateQueries on success and fires toast feedback on both success and error. This gives shared caching/dedup across components and a queryKey other code can invalidate. Risk: toast side effects are now triggered during render via isError checks rather than in an effect/callback, which is a functional but slightly unconventional pattern for React Query v5 (no onError callback on useQuery in v5) — a complete fix might instead centralize this in a useEffect keyed off isError to avoid firing on every re-render if the error object reference is unstable; this was not done to keep the change minimal. The refetch functions are returned in place of the old fetchConfiguration/fetchSupportedModels names, which changes their signature (no longer plain async functions returning the data directly) — any caller relying on the old return-value contract (e.g. updateConfiguration calling fetchConfiguration() and using the resolved data) would need adjustment, but no such caller exists inside this file after the rewrite.
🤖 Prompt for AI agents
In src/app/(app)/settings/hooks/use-ai-configuration.ts around line 139, review and complete this code-review fix: use-ai-configuration.ts uses raw useState/useEffect data fetching instead of TanStack Query.
What the draft fix changed: Replaced manual useState/useEffect data fetching in `useAiConfiguration` (src/app/(app)/settings/hooks/use-ai-configuration.ts) with `useQuery` for both the AI configuration (`AI_CONFIGURATION_QUERY_KEY`) and supported models (`SUPPORTED_MODELS_QUERY_KEY`), and replaced the manual save/loading-flag logic with `useMutation` for `updateConfiguration`, which invalidates `AI_CONFIGURATION_QUERY_KEY` via `queryClient.invalidateQueries` on success and fires toast feedback on both success and error. This gives shared caching/dedup across components and a queryKey other code can invalidate. Risk: toast side effects are now triggered during render via `isError` checks rather than in an effect/callback, which is a functional but slightly unconventional pattern for React Query v5 (no `onError` callback on `useQuery` in v5) — a complete fix might instead centralize this in a `useEffect` keyed off `isError` to avoid firing on every re-render if the error object reference is unstable; this was not done to keep the change minimal. The `refetch` functions are returned in place of the old `fetchConfiguration`/`fetchSupportedModels` names, which changes their signature (no longer plain async functions returning the data directly) — any caller relying on the old return-value contract (e.g. `updateConfiguration` calling `fetchConfiguration()` and using the resolved data) would need adjustment, but no such caller exists inside this file after the rewrite.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
Closes 4 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
src/app/(app)/settings/hooks/use-integrated-tools.ts:84src/app/(app)/settings/hooks/use-integrated-tools.ts:8src/app/(app)/settings/hooks/use-integrated-tools.ts:26src/app/(app)/settings/hooks/use-ai-configuration.ts:139What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
c69bf2d8-6eaa-4e2d-815b-af08b880c8dfMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)