Bug: fixing updating topic relevance validator#144
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors saved-configs loading to an async ChangesValidator Configuration Payload & Save Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 review this pr and add pr description |
|
Tip For best results, initiate chat on the files or code changes.
|
🧩 Analysis chain🏁 Script executed: Length of output: 6293
Proposed PR Description: Bug: Fixing Updating Topic Relevance ValidatorFixes ProblemTwo bugs were affecting the update flow for saved validator configurations:
Changes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/(main)/guardrails/page.tsx (1)
78-106: 💤 Low value
fetchSavedConfigsrefactor looks good overall; ternary chain can be simplified.The
async/useCallbackconversion and thetry/finallyguardingsetSavedConfigsLoadingare both correct improvements.One small nit: the three-level ternary at lines 89–97 (with the redundant non-null assertion
data.configs!) is hard to follow. A flat helper reduces cognitive load and removes the assertion:♻️ Simplified extraction
- const nested = data?.data; - const list: SavedValidatorConfig[] = Array.isArray( - (nested as { configs?: SavedValidatorConfig[] })?.configs, - ) - ? (nested as { configs: SavedValidatorConfig[] }).configs - : Array.isArray(nested) - ? (nested as SavedValidatorConfig[]) - : Array.isArray(data?.configs) - ? data.configs! - : []; + const nested = data?.data; + const list: SavedValidatorConfig[] = + Array.isArray((nested as { configs?: SavedValidatorConfig[] })?.configs) + ? (nested as { configs: SavedValidatorConfig[] }).configs + : Array.isArray(nested) + ? (nested as SavedValidatorConfig[]) + : Array.isArray(data?.configs) + ? (data.configs as SavedValidatorConfig[]) + : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/`(main)/guardrails/page.tsx around lines 78 - 106, The ternary chain in fetchSavedConfigs that extracts the configs from guardrailsFetch's response is hard to read and uses a non-null assertion; replace it with a small clear extraction helper: inspect data, data.data, and data.data.configs in order and return the first found array (or []), then call setSavedConfigs(list) as before. Update fetchSavedConfigs to use that helper to compute list (keeping useCallback deps configsQueryString and apiKey) and remove the Array.isArray nesting and the data.configs! assertion; keep the existing try/catch/finally behavior and the setSavedConfigsLoading/setSavedConfigs calls.app/lib/types/guardrails.ts (1)
66-72: ⚡ Quick winUse concrete types instead of
unknownforValidatorUpdatePayloadfields.All five fields are typed as
unknown, providing zero type safety at call sites. The correct types are already defined inSavedValidatorConfigin the same file.♻️ Proposed fix
export interface ValidatorUpdatePayload { - name?: unknown; - type?: unknown; - stage?: unknown; - on_fail_action?: unknown; - is_enabled?: unknown; + name?: string; + type?: string; + stage?: string; + on_fail_action?: string; + is_enabled?: boolean; }As per coding guidelines, TypeScript should be used in strict mode for type safety across all source files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/types/guardrails.ts` around lines 66 - 72, ValidatorUpdatePayload fields are all typed as unknown; replace each field's type with the corresponding concrete types from SavedValidatorConfig (e.g., use SavedValidatorConfig['name'], ['type'], ['stage'], ['on_fail_action'], and ['is_enabled'] or the explicit primitives/union types declared on SavedValidatorConfig) so callers get proper type checking; update the ValidatorUpdatePayload declaration to reference SavedValidatorConfig for each property (or inline the exact types) while keeping them optional as currently declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/types/guardrails.ts`:
- Around line 74-79: buildValidatorUpdatePayload currently strips
validator-specific parameters; change it to preserve the full config by
returning the extracted fields plus a config property containing the remaining
config values (e.g., return { name, type, stage, on_fail_action, is_enabled,
config: configValues }); also update the ValidatorUpdatePayload type (or add an
optional config field) to accept that config object so PATCH requests carry the
same validator parameters as CREATE requests.
---
Nitpick comments:
In `@app/`(main)/guardrails/page.tsx:
- Around line 78-106: The ternary chain in fetchSavedConfigs that extracts the
configs from guardrailsFetch's response is hard to read and uses a non-null
assertion; replace it with a small clear extraction helper: inspect data,
data.data, and data.data.configs in order and return the first found array (or
[]), then call setSavedConfigs(list) as before. Update fetchSavedConfigs to use
that helper to compute list (keeping useCallback deps configsQueryString and
apiKey) and remove the Array.isArray nesting and the data.configs! assertion;
keep the existing try/catch/finally behavior and the
setSavedConfigsLoading/setSavedConfigs calls.
In `@app/lib/types/guardrails.ts`:
- Around line 66-72: ValidatorUpdatePayload fields are all typed as unknown;
replace each field's type with the corresponding concrete types from
SavedValidatorConfig (e.g., use SavedValidatorConfig['name'], ['type'],
['stage'], ['on_fail_action'], and ['is_enabled'] or the explicit
primitives/union types declared on SavedValidatorConfig) so callers get proper
type checking; update the ValidatorUpdatePayload declaration to reference
SavedValidatorConfig for each property (or inline the exact types) while keeping
them optional as currently declared.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec838dd1-3fc7-4c23-a78e-b60de0a54ff0
📒 Files selected for processing (2)
app/(main)/guardrails/page.tsxapp/lib/types/guardrails.ts
|
@nishika26 Is this PR ready for review? If yes, can you please add the description of the PR and also add the |
Ayush8923
left a comment
Ayush8923
left a comment
There was a problem hiding this comment.
Added the few comments.
| export function buildValidatorUpdatePayload( | ||
| configValues: Record<string, unknown>, | ||
| ): ValidatorUpdatePayload { | ||
| const { name, type, stage, on_fail_action, is_enabled } = configValues; | ||
| return { name, type, stage, on_fail_action, is_enabled }; | ||
| } |
There was a problem hiding this comment.
I don’t think this function should be inside the types declaration. It’s simply picking objects from configValues, so it would be better placed in utils, or kept within the same file if it’s only used there.
This function doesn’t have any direct relation to types. The types defined here can be imported and used wherever needed.
There was a problem hiding this comment.
added it to utils instead
Ayush8923
added
reviewed
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/utils/guardrails.ts (1)
52-58: ⚡ Quick winAvoid unchecked
ascasts when building PATCH payload—use runtime type guards instead.At lines 52–58, direct assertions (
as string/as boolean) from destructuredunknownvalues can include undefined or mistyped fields in the update payload. SinceValidatorUpdatePayloadfields are optional, the current approach technically works but sends more data than necessary. Prefer runtime narrowing to only include valid typed fields.Proposed safer payload builder
export function buildValidatorUpdatePayload( configValues: Record<string, unknown>, ): ValidatorUpdatePayload { const { name, type, stage, on_fail_action, is_enabled } = configValues; - return { - name: name as string, - type: type as string, - stage: stage as string, - on_fail_action: on_fail_action as string, - is_enabled: is_enabled as boolean, - }; + const payload: ValidatorUpdatePayload = {}; + if (typeof name === "string") payload.name = name; + if (typeof type === "string") payload.type = type; + if (typeof stage === "string") payload.stage = stage; + if (typeof on_fail_action === "string") + payload.on_fail_action = on_fail_action; + if (typeof is_enabled === "boolean") payload.is_enabled = is_enabled; + return payload; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/utils/guardrails.ts` around lines 52 - 58, The current builder destructures unknown configValues into name, type, stage, on_fail_action, is_enabled and force-casts them with `as` which can send undefined/mistyped fields; replace the `as` casts with runtime type checks (e.g., typeof checks and explicit boolean checks) and only include each property in the returned ValidatorUpdatePayload when the value passes the appropriate guard (e.g., include name/type/stage/on_fail_action if typeof value === "string", include is_enabled if typeof value === "boolean"), so the resulting payload contains only validated fields instead of blanket-casted values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/lib/utils/guardrails.ts`:
- Around line 52-58: The current builder destructures unknown configValues into
name, type, stage, on_fail_action, is_enabled and force-casts them with `as`
which can send undefined/mistyped fields; replace the `as` casts with runtime
type checks (e.g., typeof checks and explicit boolean checks) and only include
each property in the returned ValidatorUpdatePayload when the value passes the
appropriate guard (e.g., include name/type/stage/on_fail_action if typeof value
=== "string", include is_enabled if typeof value === "boolean"), so the
resulting payload contains only validated fields instead of blanket-casted
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0ab465c-2e1f-4f82-a9a1-be01907f7edd
📒 Files selected for processing (3)
app/(main)/guardrails/page.tsxapp/lib/types/guardrails.tsapp/lib/utils/guardrails.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/types/guardrails.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/(main)/guardrails/page.tsx
| name?: string; | ||
| type?: string; | ||
| stage?: string; | ||
| on_fail_action?: string; |
There was a problem hiding this comment.
If this is a string, I think we should rename the variable from on_fail_action to something like fail_action_status etc. Right now, the on_ prefix suggests it represents a function or handler, so the naming should reflect that more clearly.
There was a problem hiding this comment.
on fail action is the name we have kept everywhere from start, from the past many months we have been working on guardrails, are you suggesting changing it just here, while keeping the core name on fail action itself, or changing it everywhere?
There was a problem hiding this comment.
Got it. I initially thought this was just a simple form value variable. If this is a small change with no breaking impact, you can go ahead and update it. Otherwise, you can add it to your TODOs to fix the variable naming in the future.
| export function buildValidatorUpdatePayload( | ||
| configValues: Record<string, unknown>, | ||
| ): ValidatorUpdatePayload { | ||
| const { name, type, stage, on_fail_action, is_enabled } = configValues; | ||
| return { | ||
| name: name as string, | ||
| type: type as string, | ||
| stage: stage as string, | ||
| on_fail_action: on_fail_action as string, | ||
| is_enabled: is_enabled as boolean, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Move this function after the buildDefaultValues, because the JS comment above belongs to the buildDefaultValues function, please either move the comment below or update it accordingly.
Co-authored-by: Ayush <80516839+Ayush8923@users.noreply.github.com>
Co-authored-by: Ayush <80516839+Ayush8923@users.noreply.github.com>
2 participants
Target Issue is #128
Notes
Bug Fixes
Refactor
Summary by CodeRabbit
Bug Fixes
Improvements