refactor stitch UI to get related objects - #119
Merged
Merged
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:
📝 WalkthroughWalkthroughAdds connector discovery and config endpoints, Redis-cached backend discovery methods, piece-framework types and piece implementations, and multiple frontend UI components/pages to surface related-object discovery, connector config, mapping transforms, and scheduling for stitches. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Web Client
participant Controller as MetadataController
participant Service as MetadataDiscoveryService
participant Redis as Redis Cache
participant Piece as Connector Piece
User->>Client: request related objects (connectionId, objectName)
Client->>Controller: GET /connections/{id}/objects/{name}/related
Controller->>Service: describeRelatedObjects(orgId, connectionId, objectName)
Service->>Redis: GET meta:related:{connectionId}:{objectName}
alt cache hit
Redis-->>Service: cached JSON
Service-->>Controller: Parsed RelatedObjectDescriptor[]
else cache miss
Service->>Service: resolve connection & credentials
Service->>Piece: describeRelatedObjects(credentials, objectName)
Piece-->>Service: RelatedObjectDescriptor[] or error
Service->>Redis: SET meta:related:{connectionId}:{objectName} (TTL)
Service-->>Controller: RelatedObjectDescriptor[]
end
Controller-->>Client: 200 RelatedObjectDescriptor[]
Client-->>User: render dependencies
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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 |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx (1)
19-33:⚠️ Potential issue | 🟠 MajorReset loading state at fetch start to avoid false “not found” rendering.
Line 22 clears
workspace, butwsLoadingis never set back totruefor subsequent fetches. Onidchanges, users can see an incorrect not-found/error state while data is still loading.Suggested fix
const fetchWorkspace = useCallback(async () => { - if (!id) return; + if (!id) { + setWorkspace(null); + setError('Workspace id is missing.'); + setWsLoading(false); + return; + } const seq = ++fetchSeqRef.current; + setWsLoading(true); + setError(null); setWorkspace(null); try { const ws = await getWorkspace(id); if (seq !== fetchSeqRef.current) return; setWorkspace(ws); - setError(null); } catch (e: unknown) { if (seq !== fetchSeqRef.current) return; setError(e instanceof Error ? e.message : 'Failed to load workspace.'); } finally { if (seq === fetchSeqRef.current) setWsLoading(false); } }, [id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx` around lines 19 - 33, The fetch logic clears workspace but never sets the loading flag back to true, causing stale "not found" UI on id changes; update fetchWorkspace to call setWsLoading(true) at the start of the fetch (after the id check and before/after setWorkspace(null)) so wsLoading is true for the duration of the request, and keep the existing finally block that sets setWsLoading(false) when seq matches fetchSeqRef.current; reference fetchWorkspace, fetchSeqRef, setWorkspace, and setWsLoading when making the change.apps/api/src/modules/stitches/metadata.controller.ts (1)
52-58: 🧹 Nitpick | 🔵 TrivialConsider extracting the objectName validation to a reusable utility.
The same regex
/^[\w]{1,255}$/and error message are duplicated indescribeFieldsanddescribeRelatedObjects. Extracting to a shared validation function or custom pipe would reduce duplication and ensure consistency if the pattern needs updating.♻️ Example extraction
// In a shared validation utility or as a method in the controller private validateObjectName(objectName: string): void { if (!/^[\w]{1,255}$/.test(objectName)) { throw new BadRequestException( 'objectName must be 1-255 alphanumeric/underscore characters.', ); } }Also applies to: 73-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/stitches/metadata.controller.ts` around lines 52 - 58, Extract the duplicated objectName validation into a single reusable function or pipe and call it from both describeFields and describeRelatedObjects; specifically, create a validateObjectName utility (or Nest pipe) that checks /^[\w]{1,255}$/ and throws the BadRequestException with the existing message, then replace the inline regex checks in describeFields and describeRelatedObjects with a call to this new validateObjectName to remove duplication and ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/metadata-discovery.service.ts`:
- Around line 318-323: Check the piece capability before fetching credentials:
in metadata-discovery.service.ts, move the piece lookup
(this.pieceRegistry.getPiece(connection.appName)) and the guard that returns []
or 404 when !piece or !piece.describeRelatedObjects to occur before awaiting
this.resolveCredentials(connectionId); similarly update the other block that
calls resolveCredentials (the code handling describeObjects/findRelatedObjects
around lines 352-364) so capability checks run first and only call
resolveCredentials when the piece exists and implements the discovery hook.
- Around line 325-335: The current flow calls piece.describeRelatedObjects(...)
and caches the empty default on any exception, so transient errors are stored;
change the logic so that this.redis.set(redisKey, JSON.stringify(related), 'EX',
TTL_SECONDS) is only executed when describeRelatedObjects succeeds: move the
redis.set call into the try block (or set a success flag) and avoid writing to
redis after the catch; keep the existing this.logger.warn(...) in the catch and
still return the in-memory related variable but do not persist it to redis on
failure.
In `@apps/web/src/modules/stitches/components/DependencyList.tsx`:
- Around line 12-15: The component flashes an empty state because loading is
initialized false; in the DependencyList component change the loading useState
initialization from useState(false) to useState(true) so the spinner shows
immediately on mount (leave error and relatedObjects as-is and keep existing
setLoading usage in effects).
- Around line 71-72: The component is using the array index (idx) as the React
key in the relatedObjects.map render, which can cause unstable keys; change the
key to a stable unique identifier from the item (e.g., use mod.objectName or
another unique field on RelatedObjectDescriptor) by replacing key={idx} with
key={mod.objectName} (or appropriate unique property) inside the map callback to
ensure stable reconciliation.
In `@apps/web/src/modules/stitches/components/MappingSummary.tsx`:
- Around line 29-61: The current key for each rendered rule uses
`${fm.sourceCanonical}-${idx}` inside mappings.map(...) and
fm.mappingRules.map(...), which can collide if the same sourceCanonical appears
across different field mappings; update the key to include a unique identifier
for the parent mapping (e.g., include the outer mapping index from mappings.map
or a unique field mapping id like fm.id) or flatten mappings into a single list
with a stable id per rule, then use that stable unique id (for example
`${fmId}-${idx}` or rule.id) as the React key to prevent collisions.
In `@apps/web/src/modules/stitches/components/RelatedObjectsPanel.tsx`:
- Around line 60-61: In RelatedObjectsPanel, replace the unstable array index
key used in relatedObjects.map (currently "key={idx}") with a stable composite
key built from the item's unique fields—e.g., use a combination of objectName
and relationField (or another guaranteed-unique identifier on the item) to
ensure consistent keys across renders; mirror the same approach used for
DependencyList so keys remain stable and avoid rendering bugs.
- Around line 12-83: RelatedObjectsPanel duplicates the fetch/state logic used
in DependencyList; extract that logic into a shared hook (e.g.,
useRelatedObjects(connectionId, objectName)) and have RelatedObjectsPanel call
useRelatedObjects(stitch.srcConnectionId, stitch.sourceObject) instead of
maintaining its own useEffect/useState; ensure the hook returns { loading,
error, relatedObjects } and both RelatedObjectsPanel and DependencyList consume
those values for their respective UI rendering.
- Around line 12-15: The RelatedObjectsPanel component currently sets loading to
false causing an empty-state flash; update the useState initializer for loading
in RelatedObjectsPanel to true so the spinner shows immediately (mirror the
pattern used in DependencyList), and ensure existing setLoading(false) calls
remain unchanged so loading is cleared when data or error is received; locate
the useState hook for loading in RelatedObjectsPanel to make this one-line
change.
In `@apps/web/src/modules/stitches/components/SchedulePanel.tsx`:
- Around line 60-66: The catch block in SchedulePanel.tsx uses a fragile inline
type assertion for the thrown error when calling toast; create a small reusable
utility (e.g., extractErrorMessage(e: unknown)) that safely inspects unknown
errors for Axios-style response.data.message and falls back to Error.message or
a default string, then replace the inline assertion in the catch block (the
toast call using (e as { response?: { data?: { message?: string } } })...) with
description: extractErrorMessage(e); keep the utility exported or colocated so
it can be reused by other components.
- Around line 129-134: The UI currently shows a static "Pending execution"
whenever stitch.scheduleEnabled is true; update SchedulePanel.tsx to compute and
display a real next-run message instead: when stitch.scheduleEnabled is true use
stitch.lastScheduledAt and stitch.syncIntervalMinutes to calculate remaining
minutes (nextRun = lastScheduledAt + syncIntervalMinutes) and render "Next sync
in ~X min" (with a sensible fallback like "Enabled" or "No schedule info" if
lastScheduledAt or syncIntervalMinutes are missing), and ensure the component
updates/reacts to time changes (e.g., recalc on mount and interval) rather than
always showing "Pending execution".
In `@apps/web/src/modules/stitches/components/StitchConfigPanel.tsx`:
- Around line 36-40: The catch block in StitchConfigPanel.tsx currently swallows
404 errors (err.response?.status !== 404) which hides real missing-connection
failures; remove that special-case and always call setError(err.message ||
'Failed to load configuration options') in the catch so all errors (including
404 from missing connection) surface, leaving describeConfig to return [] for
legitimately empty schemas; update the catch handling around the describeConfig
call (the err variable and setError invocation) accordingly.
- Around line 19-47: The load effect can apply stale responses when connectionId
changes; update the load logic in the useEffect (the async load function that
calls describeConfig) to cancel or ignore stale requests and to clear schema on
missing connectors: either create an AbortController per invocation and pass its
signal into describeConfig (and check for AbortError before applying results),
or use a request-counter stored in a ref (increment at start of load and capture
currentId; only call setSchema, onChange and setError if currentId matches) so
slower responses are ignored; also reset the schema at the start of a new load
(e.g., setSchema(null) or []) and when a 404 occurs explicitly clear the schema
instead of silently ignoring it; keep using connectionId, describeConfig,
setSchema, onChange, setError, and setLoading as the referenced symbols.
In `@apps/web/src/modules/stitches/pages/StitchDetailPage.tsx`:
- Around line 94-95: Replace the brittle JSON.stringify comparison used to set
isConfigDirty with a stable deep equality check: import and use a deep-equal
utility (e.g., lodash.isequal or deep-equal) to compare configDraft against
stitch.config (or {}), and update the isConfigDirty assignment to use that
deep-equal function; ensure the import and the identifier isEqual (or chosen
name) are referenced where isConfigDirty is computed so key ordering won't cause
false positives.
In `@packages/database/drizzle/0003_add_gem_indexes.sql`:
- Around line 21-22: The migration creates blocking indexes using CREATE INDEX
on global_entity_map (gem_source_app_idx, gem_dest_app_idx) despite the header
instructing manual CONCURRENTLY creation; to fix, either replace the CREATE
INDEX statements with a no-op placeholder (e.g., leave the migration as a SELECT
1) so the migration runner won't apply blocking indexes and then create the
indexes manually with CREATE INDEX CONCURRENTLY, or if you intend to allow
blocking, remove/update the header comments and keep the CREATE INDEX
lines—update the file so the SQL and header are consistent.
---
Outside diff comments:
In `@apps/api/src/modules/stitches/metadata.controller.ts`:
- Around line 52-58: Extract the duplicated objectName validation into a single
reusable function or pipe and call it from both describeFields and
describeRelatedObjects; specifically, create a validateObjectName utility (or
Nest pipe) that checks /^[\w]{1,255}$/ and throws the BadRequestException with
the existing message, then replace the inline regex checks in describeFields and
describeRelatedObjects with a call to this new validateObjectName to remove
duplication and ensure consistency.
In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx`:
- Around line 19-33: The fetch logic clears workspace but never sets the loading
flag back to true, causing stale "not found" UI on id changes; update
fetchWorkspace to call setWsLoading(true) at the start of the fetch (after the
id check and before/after setWorkspace(null)) so wsLoading is true for the
duration of the request, and keep the existing finally block that sets
setWsLoading(false) when seq matches fetchSeqRef.current; reference
fetchWorkspace, fetchSeqRef, setWorkspace, and setWsLoading when making the
change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a65e138-d833-4bd8-85a5-54d164e0f57b
📒 Files selected for processing (21)
apps/api/src/modules/stitches/metadata-discovery.service.tsapps/api/src/modules/stitches/metadata.controller.tsapps/api/src/modules/stitches/stitches.service.tsapps/web/src/app/routes/TenantRoutes.tsxapps/web/src/modules/stitches/api/metadata.api.tsapps/web/src/modules/stitches/api/stitches.api.tsapps/web/src/modules/stitches/components/DependencyList.tsxapps/web/src/modules/stitches/components/MappingCanvas.tsxapps/web/src/modules/stitches/components/MappingSummary.tsxapps/web/src/modules/stitches/components/RelatedObjectsPanel.tsxapps/web/src/modules/stitches/components/SchedulePanel.tsxapps/web/src/modules/stitches/components/StitchConfigPanel.tsxapps/web/src/modules/stitches/pages/CreateStitchPage.tsxapps/web/src/modules/stitches/pages/StitchDetailPage.tsxapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsxapps/web/src/shared/components/layout/WorkspaceExplorer.tsxdocs/architecture/master/tasks.mdengine/application/pieces/quickbooks/src/index.tsengine/application/pieces/salesforce/src/index.tsengine/platform/piece-framework/src/piece.tspackages/database/drizzle/0003_add_gem_indexes.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/metadata-discovery.service.ts`:
- Around line 365-371: The call to piece.describeConfig should be wrapped in a
try/catch like describeRelatedObjects to prevent exceptions bubbling up; change
the block around piece.describeConfig(credentials) (the variable config) to
catch any errors, log or warn as appropriate, set config = [] on failure, and
still call this.redis.set(redisKey, JSON.stringify(config), 'EX', TTL_SECONDS)
so we cache the empty result; reference piece.describeConfig,
describeRelatedObjects, config, this.redis.set and TTL_SECONDS when making the
change.
In `@apps/web/src/modules/stitches/components/DependencyList.tsx`:
- Around line 119-124: The checkbox input in DependencyList.tsx currently has an
empty onChange (checked={isChecked} onChange={() => {}}), which breaks keyboard
accessibility; update the input to either call the same row toggle handler used
by the parent row (e.g., replace onChange={() => {}} with the existing toggle
handler so the checkbox toggles when focused and Enter/Space is pressed) or, if
the checkbox is purely presentational, mark it readOnly and ensure appropriate
ARIA attributes are set; ensure the change targets the <input ...
checked={isChecked} onChange={...}/> line and uses the component's existing
toggle function name (or add a small handleCheckboxChange that invokes the row
toggle).
In `@apps/web/src/modules/stitches/components/SchedulePanel.tsx`:
- Around line 36-41: Replace the inline error handling in handleIntervalChange
and handleToggle with the shared extractor to ensure Axios-style messages are
preserved: use extractErrorMessage(e) in the toast description instead of the
current e instanceof Error ? e.message : '...'. Also change the catch in
handleTrigger (if different) to use the same extractErrorMessage function so all
three handlers (handleTrigger, handleIntervalChange, handleToggle) consistently
call extractErrorMessage(e) for their toast descriptions.
- Around line 1-16: The file places the utility function extractErrorMessage
between import statements which breaks the convention of having imports at the
top; move the entire extractErrorMessage function so it appears after all import
statements (i.e., below the import block and before the component code),
ensuring any references (e.g., calls to extractErrorMessage within SchedulePanel
or other functions in this file) continue to resolve without changing its
signature or behavior.
In `@apps/web/src/modules/stitches/components/StitchConfigPanel.tsx`:
- Line 15: In StitchConfigPanel, the loading state is initialized to false which
can produce a brief empty-state flash; change the useState initial value in the
component (the const [loading, setLoading] = useState(...) call) from false to
true so the UI shows a loading state until your effect finishes and calls
setLoading(false), ensuring you still setLoading(false) in all success/error
paths in the existing load/config-fetch logic.
In `@apps/web/src/modules/stitches/pages/StitchDetailPage.tsx`:
- Around line 54-70: handleConfigSave currently updates the stitch via
updateStitch and sets the returned stitch in setStitch but does not sync
configDraft, causing isConfigDirty to remain true if the backend normalizes
config; after a successful update (inside the try block, after
setStitch(updated)) call setConfigDraft(updated.config) (or the
normalized/serialized value returned by updateStitch) so the local draft matches
the server response; ensure you reference the updated object from updateStitch
and update the configDraft state (e.g., setConfigDraft) with the server-provided
config to clear the dirty flag and preserve types/shape.
- Around line 95-96: StitchConfigPanel's mount-applied schema defaults are
mutating configDraft and causing immediate dirty state; to fix, create a
baseline that includes schema defaults and compare configDraft against that
instead of raw stitch.config: when StitchDetailPage loads, compute a baseline
like baselineConfig = mergeDefaults(stitch.config || {}, schemaDefaults) (obtain
schemaDefaults the same way StitchConfigPanel does) and change isConfigDirty to
!isEqual(configDraft, baselineConfig); alternatively, stop StitchConfigPanel
from calling onChange when only applying defaults (i.e., only call onChange for
user-initiated edits) so configDraft isn't updated by default population.
Reference: StitchDetailPage (configDraft, isConfigDirty, stitch.config) and
StitchConfigPanel (onChange, schema default application).
In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx`:
- Around line 19-23: The fetchWorkspace callback returns early when the route
param id is missing but leaves wsLoading true, causing a permanent loading
state; update fetchWorkspace (the function that uses fetchSeqRef, setWsLoading
and setWorkspace) to clear the loading state before returning (e.g., call
setWsLoading(false) and optionally setWorkspace(null) when !id) or move the id
existence check higher so you never set wsLoading(true) unless id is present;
ensure any early-return paths reset wsLoading to false to avoid the stuck
“Loading workspace…” UI.
In `@packages/database/drizzle/0003_add_gem_indexes.sql`:
- Line 21: The migration comment contains an accidental Arabic token "الد"
within the placeholder line "SELECT 1; -- Placeholder to satisfy الد migration
runner safely." — remove the "الد" token so the comment reads clearly (e.g.,
"SELECT 1; -- Placeholder to satisfy migration runner safely.") to avoid
confusion during runbooks; update the placeholder comment in the migration file
(look for the "SELECT 1;" line) and keep the SQL unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1445e73c-c561-442e-a634-32e618a1dc27
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/api/src/modules/stitches/metadata-discovery.service.tsapps/api/src/modules/stitches/metadata.controller.tsapps/web/package.jsonapps/web/src/modules/stitches/components/DependencyList.tsxapps/web/src/modules/stitches/components/MappingSummary.tsxapps/web/src/modules/stitches/components/SchedulePanel.tsxapps/web/src/modules/stitches/components/StitchConfigPanel.tsxapps/web/src/modules/stitches/pages/CreateStitchPage.tsxapps/web/src/modules/stitches/pages/StitchDetailPage.tsxapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsxengine/application/pieces/salesforce/src/index.tspackages/database/drizzle/0003_add_gem_indexes.sql
Comment on lines
+36
to
+41
| } catch (e) { | ||
| toast({ | ||
| title: 'Update failed', | ||
| description: e instanceof Error ? e.message : 'Could not update interval', | ||
| variant: 'destructive', | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent error message extraction across handlers.
handleTrigger uses extractErrorMessage (line 73) while handleIntervalChange (line 39) and handleToggle (line 56) use inline e instanceof Error ? e.message : '...'. This inconsistency means the latter handlers won't extract Axios-style response.data.message errors properly.
♻️ Use extractErrorMessage consistently
} catch (e) {
toast({
title: 'Update failed',
- description: e instanceof Error ? e.message : 'Could not update interval',
+ description: extractErrorMessage(e, 'Could not update interval'),
variant: 'destructive',
});
} finally { } catch (e) {
toast({
title: 'Toggle failed',
- description: e instanceof Error ? e.message : 'Could not toggle schedule',
+ description: extractErrorMessage(e, 'Could not toggle schedule'),
variant: 'destructive',
});
} finally {Also applies to: 53-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/modules/stitches/components/SchedulePanel.tsx` around lines 36 -
41, Replace the inline error handling in handleIntervalChange and handleToggle
with the shared extractor to ensure Axios-style messages are preserved: use
extractErrorMessage(e) in the toast description instead of the current e
instanceof Error ? e.message : '...'. Also change the catch in handleTrigger (if
different) to use the same extractErrorMessage function so all three handlers
(handleTrigger, handleIntervalChange, handleToggle) consistently call
extractErrorMessage(e) for their toast descriptions.
pramodnarayana
marked this pull request as draft
April 6, 2026 11:44
pramodnarayana
marked this pull request as ready for review
April 6, 2026 11:44
pramodnarayana
marked this pull request as draft
April 6, 2026 11:46
pramodnarayana
marked this pull request as ready for review
April 6, 2026 11:47
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx (1)
20-23:⚠️ Potential issue | 🟡 MinorReset
workspace/errorwhen routeidis absent.Loading now exits correctly, but this branch can still leave stale
workspaceor priorerrorif the component remains mounted whileidbecomes undefined.💡 Proposed fix
if (!id) { + setWorkspace(null); + setError('Workspace not found.'); setWsLoading(false); return; }Use this read-only check to confirm whether this component can render without
id(or be reused across param transitions):#!/bin/bash set -euo pipefail echo "1) Find where WorkspaceDetailPage is mounted in routes" rg -n -C3 '\bWorkspaceDetailPage\b' --iglob '*.ts' --iglob '*.tsx' echo echo "2) Inspect workspace route path patterns (:id vs :id?)" rg -n -C3 'workspaces/:id\??|path\s*[:=].*workspaces' --iglob '*.ts' --iglob '*.tsx' echo echo "3) Re-check guard logic in page component" rg -n -C4 'useParams<\{ id: string \}>|if \(!id\)' apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx` around lines 20 - 23, In the WorkspaceDetailPage component, the early-return branch "if (!id) { setWsLoading(false); return; }" can leave stale workspace or error state; update that branch to also clear those states (call setWorkspace(null or undefined) and setError(null or undefined)) before setWsLoading(false) so the component doesn't render stale data when params drop out; ensure you reference the existing state setters (setWorkspace, setError, setWsLoading) used in this component and keep the early-return behavior intact.packages/database/drizzle/0000_salty_stryfe.sql (1)
324-326:⚠️ Potential issue | 🟠 MajorKeep GEM app-index creation consistent with the online-migration contract.
Lines 325-326 add transactional
CREATE INDEXstatements forgem_source_app_idxandgem_dest_app_idx, which conflicts with the out-of-bandCONCURRENTLYworkflow documented inpackages/database/drizzle/0003_add_gem_indexes.sqland weakens whatpackages/database/drizzle/0004_verify_gem_indexes.sqlis intended to enforce.Proposed adjustment
CREATE INDEX "gem_stitch_idx" ON "global_entity_map" USING btree ("stitch_id");--> statement-breakpoint -CREATE INDEX "gem_source_app_idx" ON "global_entity_map" USING btree ("source_app_id");--> statement-breakpoint -CREATE INDEX "gem_dest_app_idx" ON "global_entity_map" USING btree ("dest_app_id"); +-- gem_source_app_idx / gem_dest_app_idx are created out-of-band with +-- CREATE INDEX CONCURRENTLY (see 0003_add_gem_indexes.sql).#!/bin/bash set -euo pipefail echo "== packages/database/drizzle/0000_salty_stryfe.sql (Lines 318-332) ==" nl -ba packages/database/drizzle/0000_salty_stryfe.sql | sed -n '318,332p' echo echo "== packages/database/drizzle/0003_add_gem_indexes.sql (Lines 1-40) ==" nl -ba packages/database/drizzle/0003_add_gem_indexes.sql | sed -n '1,40p' echo echo "== packages/database/drizzle/0004_verify_gem_indexes.sql (Lines 1-40) ==" nl -ba packages/database/drizzle/0004_verify_gem_indexes.sql | sed -n '1,40p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/database/drizzle/0000_salty_stryfe.sql` around lines 324 - 326, The two transactional CREATE INDEX statements for gem_source_app_idx and gem_dest_app_idx conflict with the out-of-band CONCURRENTLY workflow; remove the transactional lines creating "gem_source_app_idx" and "gem_dest_app_idx" from this baseline so index creation is handled by the online migration (0003_add_gem_indexes.sql using CREATE INDEX CONCURRENTLY) and verified by 0004_verify_gem_indexes.sql.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/metadata-discovery.service.ts`:
- Around line 365-379: The code unconditionally writes an empty config to Redis
when piece.describeConfig throws, which caches failures; modify the logic in
metadata-discovery.service.ts so that redis.set(redisKey,
JSON.stringify(config), 'EX', TTL_SECONDS) is only called on successful
describeConfig resolution (i.e., move the redis.set into the try block after
await piece.describeConfig(credentials) or add a conditional that only sets when
piece.describeConfig succeeded), keep the existing logger.warn in the catch and
do not cache on catch, and ensure references to redis.set, redisKey,
TTL_SECONDS, piece.describeConfig, and describeRelatedObjects are used to locate
and mirror the successful-only caching behavior.
---
Duplicate comments:
In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx`:
- Around line 20-23: In the WorkspaceDetailPage component, the early-return
branch "if (!id) { setWsLoading(false); return; }" can leave stale workspace or
error state; update that branch to also clear those states (call
setWorkspace(null or undefined) and setError(null or undefined)) before
setWsLoading(false) so the component doesn't render stale data when params drop
out; ensure you reference the existing state setters (setWorkspace, setError,
setWsLoading) used in this component and keep the early-return behavior intact.
In `@packages/database/drizzle/0000_salty_stryfe.sql`:
- Around line 324-326: The two transactional CREATE INDEX statements for
gem_source_app_idx and gem_dest_app_idx conflict with the out-of-band
CONCURRENTLY workflow; remove the transactional lines creating
"gem_source_app_idx" and "gem_dest_app_idx" from this baseline so index creation
is handled by the online migration (0003_add_gem_indexes.sql using CREATE INDEX
CONCURRENTLY) and verified by 0004_verify_gem_indexes.sql.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64c0eae0-1b8f-41cc-a56c-88b1d3071241
📒 Files selected for processing (10)
apps/api/src/modules/stitches/metadata-discovery.service.tsapps/web/src/modules/stitches/components/DependencyList.tsxapps/web/src/modules/stitches/components/SchedulePanel.tsxapps/web/src/modules/stitches/components/StitchConfigPanel.tsxapps/web/src/modules/stitches/pages/StitchDetailPage.tsxapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsxengine/platform/core/src/pieces/piece-registry.service.spec.tsengine/platform/core/src/storage-resolver/storage-resolver.service.spec.tspackages/database/drizzle/0000_salty_stryfe.sqlpackages/database/drizzle/0003_add_gem_indexes.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/metadata-discovery.service.ts`:
- Around line 365-381: The current try/catch around both piece.describeConfig
and this.redis.set can overwrite a valid config when the Redis write fails;
change the logic so successful piece.describeConfig results are preserved even
if caching fails. Call await piece.describeConfig(credentials) first (assign to
config) and only wrap the this.redis.set(redisKey, JSON.stringify(config), 'EX',
TTL_SECONDS) in its own try/catch that logs via this.logger.warn (including
connection.appName and the error) but does not modify the config variable;
alternatively, ensure that only failures from piece.describeConfig reset config
= [] while failures from this.redis.set only log and leave config intact.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f428495a-d3d7-4838-810e-6ccd710f8a1b
📒 Files selected for processing (2)
apps/api/src/modules/stitches/metadata-discovery.service.tsapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/modules/stitches/metadata-discovery.service.ts`:
- Around line 389-392: There is a duplicate, unreachable "return config;" in the
MetadataDiscoveryService method that returns the configuration; remove the
second/extra "return config;" so the method only returns once, and verify there
is no other dead code after the remaining return statement (look for the
duplicate "return config" text to locate the exact spot).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fe10e38-a5c4-4318-995b-9ccb21492a29
📒 Files selected for processing (1)
apps/api/src/modules/stitches/metadata-discovery.service.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Merged
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
1 participant
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
Refactor
Documentation