Skip to content

fix(OPENFRAM-002-2): CU-86akhf8u5 3 review findings across 2 files - #395

Draft
flamingo[bot] wants to merge 2 commits into
mainfrom
ai-fix/openfram-002-2-391f3cb0-c69bf2d8
Draft

flamingo[bot] wants to merge 2 commits into
mainfrom
ai-fix/openfram-002-2-391f3cb0-c69bf2d8

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Closes 3 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.

# Fix confidence Finding Location
1 🔴 15 low — review closely Raw GraphQL POST query used for assigned-items fetch instead of react-relay src/components/assignments/use-assigned-items.ts:19
2 🟡 75 medium Assignment query keys are inline arrays, not imported from a centralized query-keys module src/components/assignments/use-assigned-items.ts:278
3 🔴 40 low — review closely Direct fetch-style GraphQL POST bypasses Relay in device-filters facet fetch src/app/(app)/customers/hooks/use-customers-min.ts:33

What 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-af08b880c8df

Merging 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)

@flamingo flamingo Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 What this fix changed, finding by finding

3 finding(s) fixed in this draft — 3 explained inline on the diff; 2 low-confidence hunk(s) need close review before merging.

// response shape (Organization | Machine | KnowledgeBaseItem | Ticket) and the
// manual field aliasing below need a corresponding Relay fragment per target
// type to migrate safely.
const ASSIGNED_ITEMS_QUERY = `#graphql

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Raw GraphQL POST query used for assigned-items fetch instead of react-relay

Did not migrate fetchAssignedItems to react-relay (that requires generated Relay fragments/queries per union member and a compiler step not available here); instead added an explicit TODO comment above ASSIGNED_ITEMS_QUERY documenting the OPENFRAM-002-2 gap and the reason a full migration needs per-type Relay fragments. This is a documentation-only change to the raw-query path in fetchAssignedItems/ASSIGNED_ITEMS_QUERY — the underlying finding (raw POST GraphQL instead of react-relay) remains unresolved in behavior, only flagged; a complete fix requires generating Relay fragments for Organization/Machine/KnowledgeBaseItem/Ticket and rewriting fetchAssignedItems around useLazyLoadQuery/useFragment, which is out of scope for a same-file minimal change.

🤖 Prompt for AI agents
In src/components/assignments/use-assigned-items.ts around line 19, review and complete this code-review fix: Raw GraphQL POST query used for assigned-items fetch instead of react-relay.
What the draft fix changed: Did not migrate `fetchAssignedItems` to react-relay (that requires generated Relay fragments/queries per union member and a compiler step not available here); instead added an explicit TODO comment above `ASSIGNED_ITEMS_QUERY` documenting the OPENFRAM-002-2 gap and the reason a full migration needs per-type Relay fragments. This is a documentation-only change to the raw-query path in `fetchAssignedItems`/`ASSIGNED_ITEMS_QUERY` — the underlying finding (raw POST GraphQL instead of react-relay) remains unresolved in behavior, only flagged; a complete fix requires generating Relay fragments for Organization/Machine/KnowledgeBaseItem/Ticket and rewriting `fetchAssignedItems` around `useLazyLoadQuery`/`useFragment`, which is out of scope for a same-file minimal change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 15 low — review closely — react 👍/👎 to teach the reviewer

@@ -277,7 +295,7 @@ export function useAssignedItems({ itemId, itemType, enabled = true }: UseAssign

return useQueries({

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Assignment query keys are inline arrays, not imported from a centralized query-keys module

Extracted the inline query-key array literal into a new exported assignedItemsQueryKey(itemType, normalizedItemId, targetType) helper defined in this same file (no hooks/admin-query-keys.ts module exists in the repo to import from, so inventing an import would violate the "every import must exist" rule). useAssignedItems now calls this helper instead of constructing ['assignments', 'assigned-items', ...] inline, giving other surfaces a single named function to import and reuse for cache invalidation instead of hand-writing the array shape.

🤖 Prompt for AI agents
In src/components/assignments/use-assigned-items.ts around line 278, review and complete this code-review fix: Assignment query keys are inline arrays, not imported from a centralized query-keys module.
What the draft fix changed: Extracted the inline query-key array literal into a new exported `assignedItemsQueryKey(itemType, normalizedItemId, targetType)` helper defined in this same file (no `hooks/admin-query-keys.ts` module exists in the repo to import from, so inventing an import would violate the "every import must exist" rule). `useAssignedItems` now calls this helper instead of constructing `['assignments', 'assigned-items', ...]` inline, giving other surfaces a single named function to import and reuse for cache invalidation instead of hand-writing the array shape.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

async (search: string = '') => {
setLoading(true);
setError(null);
try {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Direct fetch-style GraphQL POST bypasses Relay in device-filters facet fetch

Replaced the raw apiClient.post('/api/graphql', ...) call in useCustomersMin (previously using GET_ORGANIZATIONS_MIN_QUERY from ../queries/customers-queries) with useLazyLoadQuery from react-relay, matching the pattern used by use-device-filters.ts. The hook now derives items directly from the Relay query response instead of imperative fetch/setState, and fetch is kept as a compatibility shim that updates a search state variable to trigger Relay's refetch via variables change. This is UNVERIFIED against the actual Relay environment setup, compiled artifact (useCustomersMinQuery graphql tag requires relay-compiler to generate __generated__/useCustomersMinQuery.graphql.ts), and callers' expectations that fetch returns a Promise resolving with fresh data synchronously — since Relay's useLazyLoadQuery triggers a Suspense-based fetch asynchronously, the returned items in the fetch callback's closure may be stale on first call. A complete fix requires: confirming the relay-compiler is run to generate the query artifact, verifying the schema field names (organizations, search, first) match the actual GraphQL schema, wrapping the consuming component in <Suspense>, and reconciling the fetch-callback API contract with actual call sites (likely requiring a RelayEnvironmentProvider-based refetch via useQueryLoader/usePreloadedQuery instead of useLazyLoadQuery, which is more idiomatic for search-triggered queries but was not shown in the given evidence).

🤖 Prompt for AI agents
In src/app/(app)/customers/hooks/use-customers-min.ts around line 33, review and complete this code-review fix: Direct fetch-style GraphQL POST bypasses Relay in device-filters facet fetch.
What the draft fix changed: Replaced the raw `apiClient.post('/api/graphql', ...)` call in `useCustomersMin` (previously using `GET_ORGANIZATIONS_MIN_QUERY` from `../queries/customers-queries`) with `useLazyLoadQuery` from `react-relay`, matching the pattern used by `use-device-filters.ts`. The hook now derives `items` directly from the Relay query response instead of imperative fetch/setState, and `fetch` is kept as a compatibility shim that updates a `search` state variable to trigger Relay's refetch via variables change. This is UNVERIFIED against the actual Relay environment setup, compiled artifact (`useCustomersMinQuery` graphql tag requires relay-compiler to generate `__generated__/useCustomersMinQuery.graphql.ts`), and callers' expectations that `fetch` returns a Promise resolving with fresh data synchronously — since Relay's `useLazyLoadQuery` triggers a Suspense-based fetch asynchronously, the returned `items` in the `fetch` callback's closure may be stale on first call. A complete fix requires: confirming the relay-compiler is run to generate the query artifact, verifying the schema field names (`organizations`, `search`, `first`) match the actual GraphQL schema, wrapping the consuming component in `<Suspense>`, and reconciling the fetch-callback API contract with actual call sites (likely requiring a `RelayEnvironmentProvider`-based refetch via `useQueryLoader`/`usePreloadedQuery` instead of `useLazyLoadQuery`, which is more idiomatic for search-triggered queries but was not shown in the given evidence).
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot changed the title fix(OPENFRAM-002-2): 3 review findings across 2 files fix(OPENFRAM-002-2): CU-86akhf8u5 3 review findings across 2 files Sep 14, 2026
@michaelassraf

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant