monitoring ui - #112
Merged
Merged
monitoring ui#112
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 a centralized Express-based mock gateway, workspace-scoped trace APIs and UI (list + detailed trace viewer), globalizes ObservabilityModule and refactors ReplicaService logging, updates Drizzle ESM/schema resolution and db scripts, and modifies local infra and Docker composition for the new mock gateway. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MockGateway as Mock Gateway
participant FS as FileSystem
participant OpenAPI as OpenAPIBackend
Client->>MockGateway: HTTP request to /mock/<piece>/path
MockGateway->>FS: locate/cache `<piece>/openapi.json`
MockGateway->>OpenAPI: api.handleRequest(adaptedRequest)
OpenAPI-->>MockGateway: mockResponse / validation error / notImplemented
MockGateway-->>Client: HTTP response (mocked payload or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/pipeline/replica.service.ts (1)
67-68: 🧹 Nitpick | 🔵 TrivialStale comment and partially unused
logCtxobject.The comment references "structured logging" which no longer applies after the migration to NestJS's built-in Logger. Additionally, only
logCtx.connectionIdis used—layeris hardcoded in the success message as"(L2)"andtraceIdis accessed directly from the destructuredmsg.Consider simplifying:
Proposed fix
- // Bind L2 pipeline context to structured logging - const logCtx = { layer: 'L2', traceId, connectionId }; + // Context for log correlation + const logPrefix = `[${connectionId}]`;Then update references at lines 213, 217 to use
logPrefixor just useconnectionIddirectly (as already done at line 127).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/pipeline/replica.service.ts` around lines 67 - 68, The comment mentioning "structured logging" is stale and the created logCtx object is partially unused; remove or update the comment and simplify the log context by dropping the unused keys (remove layer and traceId from the logCtx declaration), or eliminate logCtx entirely and use the already-used connectionId directly; update the places that reference logCtx (the spots currently using logCtx at the success message and other logs — originally around the lines referencing logCtx and the hardcoded "(L2)" and direct msg.traceId) to instead use a small logPrefix or connectionId value (e.g., use connectionId where needed or a computed logPrefix) so only connectionId is passed into Logger calls and no stale "layer" or separate traceId fields remain.
🤖 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/mock-gateway/Dockerfile`:
- Around line 17-33: The runner stage currently runs as root; fix by creating
and using a non-root user in the Dockerfile runner stage (e.g., add a user/group
via adduser or addgroup, set ownership of /app and copied files with chown for
the created user, and add a USER <username> directive before CMD), ensure
WORKDIR /app/apps/mock-gateway and the copied files (packages/pieces and
/app/deploy content) are owned by that user so node dist/main.js runs without
root privileges.
- Around line 31-33: Add a Docker HEALTHCHECK to the Dockerfile to probe the
app's /health endpoint (the container already EXPOSEs 4000 and runs CMD
["node","dist/main.js"]). Insert a HEALTHCHECK instruction (placed after EXPOSE
and before CMD) that periodically curls or wget http://127.0.0.1:4000/health and
returns exit 0 on success and non‑zero on failure, and include sensible options
like --interval, --timeout, --start-period and --retries to avoid flapping.
In `@apps/mock-gateway/src/main.ts`:
- Around line 63-65: The code currently logs an error when the pieces directory
is missing but continues startup; update the else branch that calls
logger.error(`Pieces directory could not be located at ${piecesDir}`) to fail
fast by logging the error (keeping the existing message) and then terminating
startup (e.g., call process.exit(1) or throw an Error) so the process does not
continue with no mocks mounted.
- Around line 69-70: The port value read at the top (const port =
process.env.PORT || 4001) is not validated—create a normalize/validate step
(e.g., normalizePort or validatePort) that parses process.env.PORT to an
integer, ensures it is a finite positive integer within valid TCP port range
(1–65535) and falls back to 4001 only when absent; if the provided value is
invalid, log an error and exit (or throw), then pass the validated numeric port
into app.listen instead of the raw string. Use the new function name
(normalizePort/validatePort) to locate the change and adjust the app.listen call
accordingly.
In `@apps/web/src/modules/exceptions/api/exceptions.api.ts`:
- Around line 31-37: The query-building drops valid falsy values (like limit =
0) and raw IDs are interpolated into path templates without encoding; update the
query logic to only skip params when they are null or undefined (e.g., check
params.limit !== undefined && params.limit !== null before calling query.set)
and ensure you always encode values passed into URLSearchParams (use toString()
when needed) and encode any path parameters with encodeURIComponent when
building template URLs (e.g., the `/exceptions?${query.toString()}` call and any
other template strings that interpolate IDs before calling apiClient.get /
apiClient.delete / apiClient.post).
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 204-205: The row action controls are currently hidden via the div
with className "flex justify-end gap-2 opacity-0 group-hover:opacity-100
transition-opacity", which makes retry/dismiss inaccessible to touch and
keyboard users; change the visibility strategy so actions become visible on
keyboard focus and touch as well—replace hover-only behavior by adding
focus/focus-within variants (e.g., include group-focus-within:opacity-100 and
focus:opacity-100) or remove the opacity toggle for small screens, ensure the
action buttons themselves are focusable (no aria-hidden, have tabIndex/default
button elements) and include clear aria-labels so statusFilter-driven
conditional rendering still works but controls remain reachable via keyboard and
touch.
- Line 68: Replace the blocking alert in ExceptionCenterPage (the line using
alert(`Failed to ${action} exception: ...`)) with the page’s inline error UI:
capture the same error message (use e instanceof Error ? e.message : String(e))
and pass it to the component's existing error/notification mechanism (e.g., call
the page's setError, showNotification, or dispatch a showError action) so the
failure is surfaced inline instead of via alert. Ensure you reference the same
local variables (action and e) when forming the message.
In `@apps/web/src/modules/trace/api/trace.api.ts`:
- Around line 70-75: The URL builder drops limit=0 and interpolates unencoded
IDs into paths; change the query logic to include zero by checking params?.limit
!== undefined (or Number.isFinite) before adding to URLSearchParams, ensure you
add cursor via query.set('cursor', params.cursor) only when defined, and use
encodeURIComponent on stitchId (and cursor/path IDs) when building the request
path passed to apiClient.get<TraceListResult>(...) so the final URL is properly
encoded; apply the same fixes to the other apiClient.get call that also
interpolates stitchId/cursor near the other usage.
- Around line 66-75: Update TraceController.listTraces() and
TraceController.getTrace() to enforce workspace-level access: accept an optional
workspaceId query parameter (or obtain workspaceId from the incoming request),
fetch the stitch via the existing stitch lookup logic, and verify
stitch.workspaceId matches the requested workspaceId (or that the user is a
member of that workspace) before querying traces; if validation fails, return a
403/authorization error. Ensure the new check uses the same stitch lookup
functions and user/org verification already used elsewhere (reference
TraceController.listTraces(), TraceController.getTrace(), and the stitch lookup
helper) so the traces query is only executed after workspace validation passes.
In `@apps/web/src/modules/trace/pages/PipelineTracePage.tsx`:
- Around line 185-186: The early return in the PipelineTracePage component when
stitchId or workspaceId are missing leaves the loading state active; update the
guard around "if (!stitchId || !workspaceId) return;" to explicitly clear the
loading flag (e.g., call setLoading(false) or setIsLoading(false) used in this
component) before returning, and ensure you do not increment fetchSeqRef.current
in that path (remove or move "const seq = ++fetchSeqRef.current" so it only runs
when params are valid) so the page won't stay stuck showing skeletons.
- Around line 95-98: The clickable div used for expansion in
PipelineTracePage.tsx (the element with onClick={handleToggle}) is not
keyboard-accessible; replace it with a semantic interactive element (preferably
a <button>) or add proper ARIA and keyboard handlers: ensure the element is
focusable, supports Enter/Space key activation (or call handleToggle onKeyDown),
and expose aria-expanded reflecting the component's open/closed state; update
the element that currently uses onClick={handleToggle} and any related
state/readers so screen readers and keyboard users receive correct semantics.
- Around line 73-86: The handler handleToggle can start multiple concurrent
getTrace calls and uses a captured expanded value; fix it by preventing
duplicate fetches with an in-flight guard (e.g., a useRef boolean isFetching or
by checking loading) and by using a functional state update for toggling
expanded. Specifically, inside handleToggle check and return early if
isFetching/current loading is true, set the guard true before awaiting
getTrace(stitchId, summary.traceId), call setDetails(full) and clear the guard
in finally, and replace setExpanded(!expanded) with setExpanded(prev => !prev)
so the toggle uses the latest state.
In `@apps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsx`:
- Around line 82-85: Remove the unnecessary ESLint suppression comments inside
the cleanup function: delete the two lines containing "//
eslint-disable-next-line react-hooks/exhaustive-deps" that appear directly above
the increments of fetchSeqRef.current and connSeqRef.current; keep the
increments (fetchSeqRef.current++ and connSeqRef.current++) and leave the effect
dependency array [fetchWorkspace, fetchConnections] unchanged so the hook
complies with react-hooks/exhaustive-deps without disabled rules.
In `@docker-compose.yml`:
- Around line 47-56: The pgbouncer environment uses AUTH_TYPE: trust which
disables password checks; change AUTH_TYPE to "md5" so PgBouncer validates
credentials the same way as the DB, and ensure the pgbouncer service (where
DATABASE_POOLED_URL is used and LISTEN_PORT is exposed) has a matching userlist
(userfile) with md5-hashed credentials or proper users so pooled connections are
authenticated; update any local env/test setup to provide
POSTGRES_USER/POSTGRES_PASSWORD (or a userfile) so the change doesn't break
local access.
- Line 45: The docker-compose.yml currently references mutable images
("edoburu/pgbouncer:latest" and "ghcr.io/windmill-labs/windmill:main" used by
services windmill_init, windmill_server, windmill_worker); replace those mutable
tags with fixed version tags or image digests (e.g., specific semver tags or
sha256@digests) to ensure reproducible deployments, and update the image fields
for the pgbouncer and all windmill services accordingly (optionally change
pull_policy to avoid always pulling mutable tags).
In `@scripts/init-localstack.sh`:
- Line 50: The inline nested-escaping for RedrivePolicy is brittle; instead
build the policy JSON into a shell variable (e.g., REDRIVE_POLICY_JSON or
DLQ_POLICY) using safe quoting/printf or jq with the existing DLQ_ARN, e.g.
construct a string like
'{"RedrivePolicy":"{\"deadLetterTargetArn\":\"'"${DLQ_ARN}"'\",\"maxReceiveCount\":\"5\"}"}'
into REDRIVE_POLICY_JSON and then pass that variable to the aws CLI invocation
in place of the current --attributes line (replace the current --attributes
'{"RedrivePolicy":...}' usage that references DLQ_ARN). This keeps escaping
centralized, easier to maintain, and avoids inline nested quotes.
In `@TECHNICAL_DEBT.md`:
- Around line 271-288: The TECHNICAL_DEBT.md entry still describes a
Prism-per-vendor design even though the PR introduces a centralized mock_gateway
in docker-compose.yml; update the "Current State" and "Recommended Solution"
sections to reflect that Option A (Centralized Mock Server / mock_gateway) is
implemented: mark Option A as resolved/implemented, remove or reword the
Prism-per-vendor statements, and instead list the remaining gaps (e.g., dynamic
loading from packages/pieces/*/openapi.json, routing patterns like
/mock/{vendor}/..., integration notes for apps/api and apps/worker, and optional
next steps such as Option B (MSW) as an alternative) so the debt item accurately
documents what still needs work after the gateway landed.
---
Outside diff comments:
In `@apps/api/src/modules/pipeline/replica.service.ts`:
- Around line 67-68: The comment mentioning "structured logging" is stale and
the created logCtx object is partially unused; remove or update the comment and
simplify the log context by dropping the unused keys (remove layer and traceId
from the logCtx declaration), or eliminate logCtx entirely and use the
already-used connectionId directly; update the places that reference logCtx (the
spots currently using logCtx at the success message and other logs — originally
around the lines referencing logCtx and the hardcoded "(L2)" and direct
msg.traceId) to instead use a small logPrefix or connectionId value (e.g., use
connectionId where needed or a computed logPrefix) so only connectionId is
passed into Logger calls and no stale "layer" or separate traceId fields remain.
🪄 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: c9ac2e01-cbd8-421d-ad55-bb223d34c8b9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
TECHNICAL_DEBT.mdapps/api/src/modules/observability/observability.module.tsapps/api/src/modules/pipeline/replica.service.tsapps/mock-gateway/Dockerfileapps/mock-gateway/package.jsonapps/mock-gateway/src/main.tsapps/mock-gateway/tsconfig.jsonapps/web/package.jsonapps/web/src/app/routes/TenantRoutes.tsxapps/web/src/modules/exceptions/api/exceptions.api.tsapps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsxapps/web/src/modules/stitches/pages/StitchesPage.tsxapps/web/src/modules/trace/api/trace.api.tsapps/web/src/modules/trace/pages/PipelineTracePage.tsxapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsxdocker-compose.ymlpackages/database/drizzle.config.tspackages/database/package.jsonpackages/pieces/quickbooks/src/index.tspackages/pieces/salesforce/src/index.tsscripts/init-localstack.sh
pramodnarayana
marked this pull request as draft
April 1, 2026 09:26
pramodnarayana
marked this pull request as ready for review
April 1, 2026 09:26
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TECHNICAL_DEBT.md (1)
214-231:⚠️ Potential issue | 🟡 MinorSection header mismatch with content.
The header "## Observability Edge Cases" (H2) doesn't match the content below it, which still describes the "Cross-Module AuthGuard Import" issue. Either:
- Rename the header to match the content (e.g.,
### 1. Cross-Module AuthGuard Import), or- Move this content under a proper subsection if "Observability Edge Cases" is intended as a category header.
📝 Suggested fix to restore consistent heading
-## Observability Edge Cases +### 1. Cross-Module AuthGuard Import **Location**: `apps/api/src/modules/connections/connections/connectors.controller.ts`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TECHNICAL_DEBT.md` around lines 214 - 231, The section header "## Observability Edge Cases" does not match the following content about cross-module AuthGuard import; fix by renaming the header to a matching subsection title (e.g., "### 1. Cross-Module AuthGuard Import") or moving the AuthGuard content under a new subsection within "Observability Edge Cases"; ensure references to AuthGuard, the hardcoded import path (../../identity/auth/auth.guard), the suggested export location (`@nexiom/identity/guards`), global registration in app.module.ts via APP_GUARD, and exporting from identity/index.ts remain intact and clearly associated with the corrected heading.
♻️ Duplicate comments (2)
apps/web/src/modules/exceptions/api/exceptions.api.ts (1)
31-37: 🧹 Nitpick | 🔵 TrivialPast review issues addressed; minor trailing
?remains.The null/undefined checks and
encodeURIComponentusage from the previous review have been implemented. One small polish: when no params are provided, the URL becomes/exceptions?(empty query string). This works but is slightly unclean.Optional fix for cleaner URL
const query = new URLSearchParams(); if (params?.status !== undefined && params?.status !== null) query.set('status', params.status.toString()); if (params?.limit !== undefined && params?.limit !== null) query.set('limit', params.limit.toString()); if (params?.cursor !== undefined && params?.cursor !== null) query.set('cursor', params.cursor.toString()); - const res = await apiClient.get<ExceptionListResult>(`/exceptions?${query.toString()}`); + const qs = query.toString(); + const res = await apiClient.get<ExceptionListResult>(qs ? `/exceptions?${qs}` : '/exceptions'); return res.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/exceptions/api/exceptions.api.ts` around lines 31 - 37, When building the GET URL in exceptions.api.ts, avoid leaving a trailing '?' when there are no query params by checking the URLSearchParams output before appending; after you populate query (the variable named query) compute const qs = query.toString() and call apiClient.get<ExceptionListResult>(qs ? `/exceptions?${qs}` : `/exceptions`) instead of always using `/exceptions?${query.toString()}` so the request path is clean when params is empty.apps/api/src/modules/trace/trace.service.ts (1)
167-190:⚠️ Potential issue | 🟠 MajorThe workspace guard is still bypassable.
Both methods fetch the stitch by
(id, orgId)and only compareworkspaceIdwhen the caller supplies a truthy value. A client can omitworkspaceId— or sendworkspaceId=— and still read traces for any stitch in the org, so this does not actually enforce workspace scoping. Make workspace scope authoritative in the initial lookup, or derive it from trusted auth context, instead of treating it as optional request input.Also applies to: 261-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/modules/trace/trace.service.ts` around lines 167 - 190, The workspace check in listTraces is bypassable because the initial integrationStitches.findFirst query only filters by (id, orgId) and then conditionally compares stitch.workspaceId to the caller-supplied workspaceId; fix by making workspace scope authoritative in the query itself: update the integrationStitches.findFirst where-clause in listTraces (and the other similar method around lines 261-286) to include eq(integrationStitches.workspaceId, <trustedWorkspaceId>) rather than relying on the optional request input, where <trustedWorkspaceId> is the server-derived workspace id from the authenticated context (or, if you intentionally require workspace in the request, validate it and always include it in the where clause instead of conditional post-checks). Ensure you reference and use the same trusted workspace value when building the DB query so a client cannot omit or override workspace to access other stitches.
🤖 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/pipeline/replica.service.ts`:
- Line 27: The change from PinoLogger to NestJS Logger in ReplicaService
(private readonly logger = new Logger(ReplicaService.name)) removes structured
fields like traceId/connectionId/layer; if those structured fields are required,
restore PinoLogger usage by injecting the global PinoLogger from
ObservabilityModule (or otherwise bind context) into ReplicaService and use it
instead of Logger, or ensure you enrich Logger calls with the same structured
identifiers before logging; reference ReplicaService, the logger instantiation,
ObservabilityModule, PinoLogger, and ReplicaOutboxService to locate and mirror
the existing pattern used elsewhere.
- Around line 48-51: The current this.logger.warn call in ReplicaService's
inbound message handler uses JSON.stringify(payload) which can throw on circular
structures; wrap the stringify in a try/catch and fall back to a safe
representation (e.g., use util.inspect(payload, {depth:2, breakLength:
Infinity}) or a simple placeholder like "[unserializable payload]") before
logging, or replace JSON.stringify with a safe stringify utility so the consumer
callback cannot crash when payload contains circular references.
In `@apps/api/src/modules/trace/trace.service.spec.ts`:
- Line 140: The test currently calls service.listTraces(ORG_ID, STITCH_ID,
undefined, 50) and only covers the legacy (no-workspace) path; add explicit test
cases that pass a workspaceId to exercise the workspace-scoped branches: (1) a
test where workspaceId matches the trace/workspace and the call returns expected
traces, and (2) a test where workspaceId does not match and the call throws
ForbiddenException; update the assertions to expect returned data for the
matching case and to expect ForbiddenException for the mismatched case, and
apply the same pattern to other affected tests around the indicated ranges
(lines ~168-174, ~227, ~288, ~376-381) so service.listTraces and any other calls
that accept workspaceId validate authorization paths.
In `@apps/mock-gateway/Dockerfile`:
- Around line 17-19: Remove the unnecessary Corepack/pnpm activation from the
runner stage in the Dockerfile by deleting the RUN line that contains "corepack
enable && corepack prepare pnpm@10.27.0 --activate" (in the stage that starts
with "FROM node:20-alpine AS runner" and has "WORKDIR /app"), leaving the
runtime stage to simply run the built artifact (e.g., keep "node dist/main.js"
as the runtime command); verify no runtime pnpm commands depend on Corepack
before removing.
In `@apps/mock-gateway/src/main.ts`:
- Around line 39-47: The handler notImplemented passes a coerced string that can
become the literal "undefined" to c.api.mockResponseForOperation because it
casts c.operation.operationId before falling back; change the expression to
choose the fallback first (use c.operation.operationId ?? c.operation.path or
check existence of c.operation.operationId before casting) so
mockResponseForOperation receives a real operationId or the path; update the
call site of c.api.mockResponseForOperation within the notImplemented async
handler accordingly and keep the logger/error behavior unchanged.
- Around line 16-25: The directory resolution for packages/pieces is confusing
because piecesDir is reassigned unconditionally in the second fallback; refactor
the logic around piecesDir and the existsSync checks so each candidate path is
tested before assigning it: compute candidate paths (e.g., join(process.cwd(),
'../../packages/pieces'), join(process.cwd(), 'packages/pieces'),
join(process.cwd(), '../packages/pieces')) and then set piecesDir to the first
candidate for which existsSync(...) returns true (leaving a sensible default or
throwing/logging if none exist). Update the code referencing piecesDir in
main.ts to use this single-determination pattern so the fallback flow is linear
and explicit.
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 204-218: In ExceptionCenterPage, update the status toggle button
group to expose proper ARIA semantics: mark the container as a tablist or
radiogroup and each button as a tab or radio (e.g., add role="radiogroup" on the
div and role="radio" on each button) and set aria-checked (or aria-selected if
using tab roles) based on the current statusFilter; keep using setStatusFilter
for clicks and ensure the active button reflects aria-checked="true" while
others are "false" so assistive tech can detect the selected state.
- Around line 131-133: The Date rendering in ExceptionCenterPage uses new
Date(exc.updatedAt).toLocaleString() which can vary by browser/locale; change it
to a consistent formatter (e.g., use Intl.DateTimeFormat with explicit locale
and options or format to ISO) when rendering the updatedAt cell. Locate the JSX
in ExceptionCenterPage where new Date(exc.updatedAt).toLocaleString() is used
and replace it with a deterministic formatter (e.g., Intl.DateTimeFormat('en-GB'
or 'en-US', { year, month, day, hour, minute, second }) or
exc.updatedAt.toISOString()) so all users see uniform timestamps. Ensure the
chosen format is applied consistently wherever updatedAt or similar timestamps
are rendered.
- Around line 88-102: The skeleton rows in ExceptionCenterPage.tsx use
crypto.randomUUID() as the key which changes every render and forces remounts;
update the mapping that builds tableContent (the block guarded by loading and
the Array.from map) to use a stable key derived from the map index (e.g., use
the index i or a stable string like `skeleton-${i}`) instead of
crypto.randomUUID() so React can preserve and reuse the skeleton rows across
renders; replace the anonymous map callback to accept (_, i) and use that index
for the TableRow key.
In `@apps/web/src/modules/trace/pages/PipelineTracePage.tsx`:
- Around line 73-99: The handleToggle function currently waits for getTrace
before toggling expansion so the loading UI never shows; change the flow to
setExpanded(true) immediately when expanding (call setExpanded(prev => !prev) or
setExpanded(true) before the await), then start the fetch (setLoading(true)) and
await getTrace(workspaceId, stitchId, summary.traceId), setDetails(full) on
success and setLoading(false) in finally. Also add an error state (e.g.,
setError / setDetails to an error marker) in the catch so expandedContent can
render an inline failure message if the fetch fails instead of showing an empty
body. Ensure references: handleToggle, setExpanded, setLoading, getTrace,
setDetails, loading, details, expandedContent.
- Around line 50-52: The JsonViewer component currently returns null for any
falsy data which incorrectly hides valid values like 0, false, or empty string;
update the guard in JsonViewer to only skip when data is null or undefined
(e.g., check data === null || data === undefined) so valid falsy JSON payloads
are rendered, leaving the rest of the rendering logic and props (title, data,
className) unchanged.
In `@apps/web/src/types/auto-animate.d.ts`:
- Around line 1-14: Delete the custom ambient module declaration for module
'@formkit/auto-animate/react' (the file that declares AutoAnimateOptions and
useAutoAnimate) since the package ships its own TypeScript definitions; remove
that file and any references to it (e.g., manual /// <reference> entries or
includes in tsconfig) so the compiler picks up the packaged types for
useAutoAnimate and AutoAnimateOptions instead.
In `@scripts/init-localstack.sh`:
- Around line 48-51: The REDRIVE_POLICY_JSON assignment uses fragile nested
quoting; replace the manual-escaped string with a printf-based construction that
safely interpolates DLQ_ARN and maxReceiveCount into a JSON string (update the
REDRIVE_POLICY_JSON variable) and then pass that variable to awslocal sqs
set-queue-attributes (the QUEUE_URL and REDRIVE_POLICY_JSON symbols are what to
update). Ensure the printf format produces a single valid JSON object for the
--attributes parameter (no extra escaping) so the awslocal sqs
set-queue-attributes call consumes a well-formed JSON string.
---
Outside diff comments:
In `@TECHNICAL_DEBT.md`:
- Around line 214-231: The section header "## Observability Edge Cases" does not
match the following content about cross-module AuthGuard import; fix by renaming
the header to a matching subsection title (e.g., "### 1. Cross-Module AuthGuard
Import") or moving the AuthGuard content under a new subsection within
"Observability Edge Cases"; ensure references to AuthGuard, the hardcoded import
path (../../identity/auth/auth.guard), the suggested export location
(`@nexiom/identity/guards`), global registration in app.module.ts via APP_GUARD,
and exporting from identity/index.ts remain intact and clearly associated with
the corrected heading.
---
Duplicate comments:
In `@apps/api/src/modules/trace/trace.service.ts`:
- Around line 167-190: The workspace check in listTraces is bypassable because
the initial integrationStitches.findFirst query only filters by (id, orgId) and
then conditionally compares stitch.workspaceId to the caller-supplied
workspaceId; fix by making workspace scope authoritative in the query itself:
update the integrationStitches.findFirst where-clause in listTraces (and the
other similar method around lines 261-286) to include
eq(integrationStitches.workspaceId, <trustedWorkspaceId>) rather than relying on
the optional request input, where <trustedWorkspaceId> is the server-derived
workspace id from the authenticated context (or, if you intentionally require
workspace in the request, validate it and always include it in the where clause
instead of conditional post-checks). Ensure you reference and use the same
trusted workspace value when building the DB query so a client cannot omit or
override workspace to access other stitches.
In `@apps/web/src/modules/exceptions/api/exceptions.api.ts`:
- Around line 31-37: When building the GET URL in exceptions.api.ts, avoid
leaving a trailing '?' when there are no query params by checking the
URLSearchParams output before appending; after you populate query (the variable
named query) compute const qs = query.toString() and call
apiClient.get<ExceptionListResult>(qs ? `/exceptions?${qs}` : `/exceptions`)
instead of always using `/exceptions?${query.toString()}` so the request path is
clean when params is empty.
🪄 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: f27a336e-59ce-47fd-a06c-5c9d40f1e1b6
📒 Files selected for processing (15)
TECHNICAL_DEBT.mdapps/api/src/modules/pipeline/replica.service.tsapps/api/src/modules/trace/trace.controller.tsapps/api/src/modules/trace/trace.service.spec.tsapps/api/src/modules/trace/trace.service.tsapps/mock-gateway/Dockerfileapps/mock-gateway/src/main.tsapps/web/src/modules/exceptions/api/exceptions.api.tsapps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsxapps/web/src/modules/trace/api/trace.api.tsapps/web/src/modules/trace/pages/PipelineTracePage.tsxapps/web/src/modules/workspaces/pages/WorkspaceDetailPage.tsxapps/web/src/types/auto-animate.d.tsdocker-compose.ymlscripts/init-localstack.sh
| describe('listTraces()', () => { | ||
| it('returns paginated sync_log rows for the stitch', async () => { | ||
| const result = await service.listTraces(ORG_ID, STITCH_ID, 50); | ||
| const result = await service.listTraces(ORG_ID, STITCH_ID, undefined, 50); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add explicit coverage for the workspace-scoped branches.
All of these updated calls still pass undefined for workspaceId, so the suite only exercises the legacy path. There still isn't a case for a matching workspace or a mismatched workspace that should raise ForbiddenException, which leaves the new authorization branch untested.
Also applies to: 168-174, 227-227, 288-288, 376-381
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/modules/trace/trace.service.spec.ts` at line 140, The test
currently calls service.listTraces(ORG_ID, STITCH_ID, undefined, 50) and only
covers the legacy (no-workspace) path; add explicit test cases that pass a
workspaceId to exercise the workspace-scoped branches: (1) a test where
workspaceId matches the trace/workspace and the call returns expected traces,
and (2) a test where workspaceId does not match and the call throws
ForbiddenException; update the assertions to expect returned data for the
matching case and to expect ForbiddenException for the mismatched case, and
apply the same pattern to other affected tests around the indicated ranges
(lines ~168-174, ~227, ~288, ~376-381) so service.listTraces and any other calls
that accept workspaceId validate authorization paths.
Comment on lines
+1
to
+14
| declare module '@formkit/auto-animate/react' { | ||
| import * as React from 'react'; | ||
|
|
||
| export interface AutoAnimateOptions { | ||
| duration?: number; | ||
| easing?: string; | ||
| disrespectUserMotionPreference?: boolean; | ||
| // other auto-animate options can go here | ||
| } | ||
|
|
||
| export function useAutoAnimate<T extends Element>( | ||
| options?: AutoAnimateOptions | ((el: Element, action: 'add' | 'remove' | 'remain', oldCoords?: unknown, newCoords?: unknown) => unknown) | ||
| ): [React.RefObject<T> | React.MutableRefObject<T> | React.RefCallback<T>, (enabled: boolean) => void]; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if `@formkit/auto-animate` ships with bundled TypeScript types
# Look for package.json types/typings field
fd -t f 'package.json' -x grep -l 'auto-animate' {} \; 2>/dev/null | head -5 | xargs -I{} sh -c 'echo "=== {} ===" && cat {}'
# Check if there's a types field or .d.ts files in node_modules
echo "=== Checking for existing type definitions ==="
fd -t f -e 'd.ts' . -p 'node_modules/@formkit/auto-animate' 2>/dev/null | head -10Repository: pramodnarayana/nexiom
Length of output: 2732
🌐 Web query:
Does @formkit/auto-animate package include TypeScript type definitions?
💡 Result:
Yes, the @formkit/auto-animate package includes TypeScript type definitions. The npm package page lists multiple .d.ts files for framework-specific bindings (e.g., react/index.d.ts, vue/index.d.ts, angular/index.d.ts) and core index.d.ts. The GitHub repository's primary language is TypeScript (91.7%), confirming built-in TypeScript support.
Citations:
- 1: https://www.npmjs.com/package/@formkit/auto-animate
- 2: https://github.com/formkit/auto-animate
Remove the custom type declaration file.
The @formkit/auto-animate package already ships with complete TypeScript definitions, including React bindings. This custom declaration at apps/web/src/types/auto-animate.d.ts is redundant and creates maintenance risk—the bundled types will be the source of truth, so this file may diverge from the actual API over time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/types/auto-animate.d.ts` around lines 1 - 14, Delete the custom
ambient module declaration for module '@formkit/auto-animate/react' (the file
that declares AutoAnimateOptions and useAutoAnimate) since the package ships its
own TypeScript definitions; remove that file and any references to it (e.g.,
manual /// <reference> entries or includes in tsconfig) so the compiler picks up
the packaged types for useAutoAnimate and AutoAnimateOptions instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx (1)
169-195:⚠️ Potential issue | 🟡 MinorTouch devices still cannot see action buttons before tapping.
The focus-within variants help keyboard users but don't solve touch device accessibility. On mobile, buttons remain hidden (
opacity-0) until focused, but users can't tap invisible buttons to focus them. The previous suggestion to show buttons by default on small screens (opacity-100 sm:opacity-0) wasn't applied.Proposed fix for mobile accessibility
- <div className="flex justify-end gap-2 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus-within:opacity-100 transition-opacity"> + <div className="flex justify-end gap-2 opacity-100 sm:opacity-0 sm:group-hover:opacity-100 sm:group-focus-within:opacity-100 transition-opacity">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx` around lines 169 - 195, The action buttons container uses "opacity-0" so touch users can't tap to reveal them; update the container's classes in the div inside TableCell (the element rendering the action buttons for ExceptionCenterPage) to be visible by default on small screens but hidden on larger screens until hover/focus — e.g., replace "opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus-within:opacity-100" with a responsive variant such as "opacity-100 sm:opacity-0 sm:group-hover:opacity-100 sm:group-focus-within:opacity-100 focus-within:opacity-100 transition-opacity" so statusFilter/handleAction logic and button props (isDismissing/isRetrying, disabled, onClick) remain unchanged.
🤖 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/pipeline/replica.service.ts`:
- Around line 133-135: The debug log in ReplicaService is embedding connectionId
and traceId in the message string; change it to use structured fields to match
the pattern used in other logs (info/error). Update the this.logger.debug call
that logs "[${connectionId}] Trace ${traceId} already replicated. Skipping." to
pass an object with { connectionId, traceId } (and keep a concise message) so
the fields are queryable and consistent with the service's logging conventions.
In `@apps/api/src/modules/trace/trace.service.ts`:
- Line 8: The import currently brings in both sql and sql as drizzleSql from
'drizzle-orm' — remove the redundant import and use a single identifier
consistently (either rename the import to drizzleSql or drop the alias and use
sql everywhere); update all usages (e.g., any calls like sql(...) or
drizzleSql(...), including the occurrence around the trace query construction)
to the chosen single symbol so only one sql import is present in the import list
(keep other imports eq, and, desc, lt, or unchanged).
In `@apps/mock-gateway/src/main.ts`:
- Around line 54-58: The code uses a blunt cast "req as never" when calling
api.handleRequest from the Express middleware; replace this unsafe cast by
adapting the Express Request to the expected type or by using a precise
assertion: either update or overload api.handleRequest to accept an Express
Request (preferred), or create a small adapter function (e.g.,
convertExpressReqToHandleRequest(req)) that maps/validates fields and returns
the exact type expected by handleRequest, and then call
api.handleRequest(adapter(req), req, res) (or change the second parameter to the
correctly typed value) instead of casting to never.
In `@apps/web/src/modules/exceptions/api/exceptions.api.ts`:
- Around line 5-17: The ExceptionItem.type for the status field is currently
just string; change it to a strict union of the allowed values so consumers get
type safety — update the ExceptionItem interface (status: ...) to accept only
'FAIL' | 'RETRY' | 'DISMISSED' (do not include 'SUCCESS'), and ensure any code
handling ExceptionItem (e.g., listExceptions consumers) compiles against the new
union type and handles those three cases explicitly.
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 67-68: The comment "Optimistic remove" in ExceptionCenterPage is
misleading because the removal via setExceptions occurs after the await (i.e.,
on success); update the comment to reflect that this is a
pessimistic/remove-on-success update (e.g., "Remove after successful delete" or
remove the comment entirely) near the setExceptions call in the
ExceptionCenterPage component where setExceptions is used to filter out the item
by id.
- Around line 143-145: The UI always appends "..." to exc.routeId because the
JSX unconditionally slices and adds ellipsis; update the rendering in
ExceptionCenterPage (the TableCell that displays exc.routeId) to check the
actual length of exc.routeId and only show truncated text with "..." when
exc.routeId.length > 13, otherwise render the full exc.routeId (e.g., use a
ternary based on exc.routeId.length to decide between exc.routeId and
exc.routeId.slice(0,13) + '...').
In `@apps/web/src/modules/trace/pages/PipelineTracePage.tsx`:
- Around line 246-253: The code uses non-null assertions stitchId! and
workspaceId! in PipelineTracePage when rendering TraceRow; instead add an
explicit early-return guard at the top of the PipelineTracePage component that
checks if (!workspaceId || !stitchId) and returns a <Navigate to="/dashboard"
replace /> (or appropriate fallback) so TypeScript can infer non-null values and
you can drop the ! assertions from the TraceRow props; reference the component
PipelineTracePage, the params stitchId and workspaceId, and the fetchTraces
logic to ensure the guard runs before rendering the traces list.
- Around line 229-234: The map inside PipelineTracePage uses Array.from({
length: 4 }).map(() => ...) with key={crypto.randomUUID()}, causing unnecessary
remounts; replace the UUID key with the stable index from the map (e.g.,
.map((_, i) => <Skeleton key={i} ... />)) so React can reuse DOM nodes for the
fixed-length list and keep the Skeleton component stable.
---
Duplicate comments:
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 169-195: The action buttons container uses "opacity-0" so touch
users can't tap to reveal them; update the container's classes in the div inside
TableCell (the element rendering the action buttons for ExceptionCenterPage) to
be visible by default on small screens but hidden on larger screens until
hover/focus — e.g., replace "opacity-0 group-hover:opacity-100
group-focus-within:opacity-100 focus-within:opacity-100" with a responsive
variant such as "opacity-100 sm:opacity-0 sm:group-hover:opacity-100
sm:group-focus-within:opacity-100 focus-within:opacity-100 transition-opacity"
so statusFilter/handleAction logic and button props (isDismissing/isRetrying,
disabled, onClick) remain 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: 515e733d-13b2-4ff5-a556-6e38b24546b7
📒 Files selected for processing (11)
TECHNICAL_DEBT.mdapps/api/src/modules/pipeline/replica.service.spec.tsapps/api/src/modules/pipeline/replica.service.tsapps/api/src/modules/trace/trace.service.spec.tsapps/api/src/modules/trace/trace.service.tsapps/mock-gateway/Dockerfileapps/mock-gateway/src/main.tsapps/web/src/modules/exceptions/api/exceptions.api.tsapps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsxapps/web/src/modules/trace/pages/PipelineTracePage.tsxscripts/init-localstack.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/mock-gateway/src/main.ts (1)
50-50:⚠️ Potential issue | 🔴 Critical
api.init()returns a Promise but is not awaited — race condition.
OpenAPIBackend.init()is asynchronous and must complete beforehandleRequest()can properly route and validate requests. Without awaiting, early requests may fail or behave unexpectedly if they arrive before initialization finishes.🐛 Proposed fix: Collect and await all init promises before starting the server
+const initPromises: Promise<void>[] = []; + if (piecesDir) { const pieces = readdirSync(piecesDir, { withFileTypes: true }) .filter(dirent => dirent.isDirectory()) .map(dirent => dirent.name); for (const piece of pieces) { const specPath = join(piecesDir, piece, 'openapi.json'); if (existsSync(specPath)) { const api = new OpenAPIBackend({ definition: specPath }); api.register({ // ... handlers unchanged }); - api.init(); + initPromises.push(api.init()); // ... rest unchanged } } } -const validatedPort = normalizePort(process.env.PORT); -app.listen(validatedPort, () => { - logger.info(`Centralized Mock Gateway listening on port ${validatedPort}`); -}); +const validatedPort = normalizePort(process.env.PORT); +Promise.all(initPromises).then(() => { + app.listen(validatedPort, () => { + logger.info(`Centralized Mock Gateway listening on port ${validatedPort}`); + }); +}).catch((err) => { + logger.error({ err }, 'Failed to initialize OpenAPI backends'); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mock-gateway/src/main.ts` at line 50, api.init() returns a Promise and must be awaited to avoid race conditions with handleRequest(); modify the startup flow to await api.init() (and any other init promises) before starting the HTTP server or registering request handlers so initialization completes first: locate the api.init() call in main.ts and change the sequence so that await api.init() (or Promise.all([...]) if multiple inits) completes before calling server.listen/starting the server or wiring handleRequest().
🤖 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/mock-gateway/src/main.ts`:
- Around line 62-66: The route registration calling api.handleRequest inside
app.use(`/mock/${piece}`, (req, res) =>
api.handleRequest(convertExpressReqToHandleRequest(req), req, res)) does not
handle the Promise rejection from api.handleRequest; wrap the call so rejections
are forwarded to Express error handling (e.g., call
api.handleRequest(...).catch(err => next(err)) or use async middleware and
try/catch to call next(err)) and add a global error middleware that logs the
error and responds with a 500 (refer to api.handleRequest,
convertExpressReqToHandleRequest and the route callback to locate the change).
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 61-71: The error banner isn't cleared after a later successful
retry/dismiss, so update the try block handling in the ExceptionCenterPage:
after successfully calling retryException(id) or dismissException(id) and before
updating state with setExceptions, clear the stale error by calling
setError(null) (or an empty string if your state expects string) so the old
error banner is removed; reference the retryException, dismissException,
setExceptions, and setError symbols when making the change.
- Around line 80-87: The fallback currently uses JSON.stringify(resPayload)
which wraps primitive strings in quotes; update getReasonFromPayload to detect
primitive types (string, number, boolean) and return them directly (for strings
return the raw string without quotes) instead of JSON.stringify, while keeping
the existing object handling for Record<string, unknown> and the HTTP/statusCode
and null fallbacks; ensure you still JSON.stringify non-primitive objects (and
arrays) to preserve structure.
In `@apps/web/src/modules/trace/pages/PipelineTracePage.tsx`:
- Around line 114-123: The mapping in PipelineTracePage uses l.layer as the
React key which can collide if the same layer appears multiple times; update the
key on the rendered element in the details.layers.map (the container that
renders LayerTimeline) to use a composite unique key (e.g., combine l.layer with
the index i or a unique identifier if available) so keys become stable and
unique across retries or duplicate layer names.
---
Duplicate comments:
In `@apps/mock-gateway/src/main.ts`:
- Line 50: api.init() returns a Promise and must be awaited to avoid race
conditions with handleRequest(); modify the startup flow to await api.init()
(and any other init promises) before starting the HTTP server or registering
request handlers so initialization completes first: locate the api.init() call
in main.ts and change the sequence so that await api.init() (or
Promise.all([...]) if multiple inits) completes before calling
server.listen/starting the server or wiring handleRequest().
🪄 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: 6ceaf805-e83c-4c17-add2-a055e03159c8
📒 Files selected for processing (6)
apps/api/src/modules/pipeline/replica.service.tsapps/api/src/modules/trace/trace.service.tsapps/mock-gateway/src/main.tsapps/web/src/modules/exceptions/api/exceptions.api.tsapps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsxapps/web/src/modules/trace/pages/PipelineTracePage.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx (1)
81-91:⚠️ Potential issue | 🟡 MinorPreserve falsy failure reasons in
getReasonFromPayload.The earlier primitive-handling fix is still short-circuited by the truthy checks here. Values like
0,false,'', or{ message: '' }will still collapse to the HTTP fallback or the full JSON blob instead of the actual reason.Proposed fix
const getReasonFromPayload = (resPayload: unknown, statusCode: number | null): string => { - if (!resPayload) return statusCode ? `HTTP ${statusCode}` : 'Unknown failure'; + if (resPayload === null || resPayload === undefined) { + return statusCode ? `HTTP ${statusCode}` : 'Unknown failure'; + } if (typeof resPayload === 'string' || typeof resPayload === 'number' || typeof resPayload === 'boolean') { return String(resPayload); } if (typeof resPayload === 'object' && resPayload !== null) { const p = resPayload as Record<string, unknown>; - if (p.message) return typeof p.message === 'string' ? p.message : JSON.stringify(p.message); - if (p.error) return typeof p.error === 'string' ? p.error : JSON.stringify(p.error); + if ('message' in p && p.message !== null && p.message !== undefined) { + return typeof p.message === 'string' || typeof p.message === 'number' || typeof p.message === 'boolean' + ? String(p.message) + : JSON.stringify(p.message); + } + if ('error' in p && p.error !== null && p.error !== undefined) { + return typeof p.error === 'string' || typeof p.error === 'number' || typeof p.error === 'boolean' + ? String(p.error) + : JSON.stringify(p.error); + } } return JSON.stringify(resPayload); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx` around lines 81 - 91, The function getReasonFromPayload incorrectly treats falsy values as missing; change the initial guard from checking !resPayload to only null/undefined (resPayload === null || resPayload === undefined) so 0, false, and '' are preserved; when handling object payloads (p as Record<string, unknown>) check for the presence of keys using the in operator or hasOwnProperty for "message" and "error" (i.e., p.hasOwnProperty('message') / 'message' in p) and treat undefined specially while still returning empty strings or false/0 as-is (convert to string when primitive), and only fall back to `HTTP ${statusCode}` or JSON.stringify when the value is truly undefined or null.
🤖 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/mock-gateway/src/main.ts`:
- Around line 89-99: The default port used in normalizePort does not match the
docker-compose mapping; update normalizePort so the fallback default is '4000'
(change Number.parseInt(val || '4001', 10) to use '4000') so that validatedPort
(from process.env.PORT) and app.listen() will bind to the same port callers
target; keep the same validation logic in normalizePort and ensure any log
message still references the original val.
- Around line 40-42: The code currently sends the whole envelope returned by
c.api.mockResponseForOperation(operationId) instead of the payload and ignores
its status; update the handler to destructure the result from
c.api.mockResponseForOperation(operationId) into { status, mock } (referencing
operationId and c.api.mockResponseForOperation) and then call
res.status(status).json(mock) so the library-chosen status and only the mock
payload are returned.
- Around line 84-86: The global error handler registered via app.use((err:
Error, req, res, next) => { ... }) should respect express/body-parser metadata:
determine the response status from err.status || (err as any).statusCode || 500,
and only include err.message in the JSON when (err as any).expose is true;
otherwise return a generic message for non-exposable errors. Keep logging
(logger.error) but include the status and expose flags in the log context (e.g.,
logger.error({ err, status, expose }, 'Unhandled mock gateway error')) so the
handler for express.json parse failures and oversized bodies returns 400/413 and
avoids leaking non-exposable messages.
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 61-69: The current click handler calls retryException(id) /
dismissException(id) and immediately removes the row via setExceptions
regardless of the backend response; change it to inspect the resolved response
from retryException and dismissException (the API returns flags like { queued:
boolean } or { dismissed: boolean }) and only call setExceptions(prev =>
prev.filter(...)) when the corresponding flag is true, otherwise setError(...)
or surface the failure; update the code paths referencing retryException,
dismissException, setExceptions and setError to use the API response to decide
removal and handle non-success responses accordingly.
- Around line 109-123: The empty-state branch that sets tableContent when
exceptions.length === 0 should not render a “healthy” message if the initial
listExceptions call failed; update the conditional to also check the fetch
status (e.g., only render this branch when no exceptions AND there is no fetch
error and not currently loading). Locate the component state/props around
listExceptions (look for variables like exceptions, statusFilter,
isLoading/isFetching, and error/isError) and change the condition that sets
tableContent to something like: render the healthy empty state only when
exceptions.length === 0 && !isLoading && !error; otherwise render an appropriate
loading/error UI.
In `@apps/web/src/modules/trace/pages/PipelineTracePage.tsx`:
- Around line 16-31: LayerTimeline treats 'PROCESSING' as an in-flight state but
getStatusColor and getStatusBorder currently fall through to neutral classes;
update both helper functions (getStatusColor and getStatusBorder) to include
status === 'PROCESSING' in the same branch as the in-progress states (the branch
that handles 'RETRY' and 'PENDING') so 'PROCESSING' returns the in-flight color
and border classes and matches LayerTimeline's pulsing behavior.
- Around line 239-249: The "No traces found" empty-state is rendered whenever
traces.length === 0, which incorrectly shows after a failed initial listTraces
fetch; update the rendering logic so the empty-state only appears when the fetch
has completed successfully (e.g., not loading and no error). Modify the
condition that sets pageContent (the traces.length === 0 branch) to also check
the fetch state used with listTraces (for example isLoading / isFetching and
fetchError or error) and only render the success-style empty card when not
loading and error is falsy; otherwise keep the error banner visible. Ensure you
touch the same pageContent branch in PipelineTracePage and reference the
variables around listTraces/traces to locate the change.
---
Duplicate comments:
In `@apps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsx`:
- Around line 81-91: The function getReasonFromPayload incorrectly treats falsy
values as missing; change the initial guard from checking !resPayload to only
null/undefined (resPayload === null || resPayload === undefined) so 0, false,
and '' are preserved; when handling object payloads (p as Record<string,
unknown>) check for the presence of keys using the in operator or hasOwnProperty
for "message" and "error" (i.e., p.hasOwnProperty('message') / 'message' in p)
and treat undefined specially while still returning empty strings or false/0
as-is (convert to string when primitive), and only fall back to `HTTP
${statusCode}` or JSON.stringify when the value is truly undefined or null.
🪄 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: 59636add-391c-4adc-9b8e-e218f29fb258
📒 Files selected for processing (3)
apps/mock-gateway/src/main.tsapps/web/src/modules/exceptions/pages/ExceptionCenterPage.tsxapps/web/src/modules/trace/pages/PipelineTracePage.tsx
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
Improvements