fix(ambient-ui): address CodeRabbit review findings from #1638#1639
Conversation
- Guard JSON array parsers against null/primitive entries (isRecord filter) - Preserve temperature=0 as valid (only null maxTokens/timeout at zero) - Normalize repo URLs when merging to avoid .git suffix mismatches - Fix hydration mismatch: initialize tab state server-safe, sync from URL in useEffect Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
|
Warning Review limit reached
More reviews will be available in 1 minute and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSession domain parsing, repository deduplication, and page navigation updated: numeric helpers refactored to preserve zero values for temperature while treating zero as null for token and timeout limits; repository URLs normalized before matching to handle ChangesSession Detail and Domain Mapping
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/ambient-ui/src/adapters/mappers.ts (1)
123-129: ⚡ Quick winTighten parameter types for
numberOrNullandpositiveNumberOrNull.Both functions check for
undefinedandnullat runtime, but the parameter type isnumber, which excludes these values in strict TypeScript. Update the signatures to match the runtime checks:-function numberOrNull(value: number): number | null { +function numberOrNull(value: number | undefined | null): number | null { return value === undefined || value === null ? null : value } -function positiveNumberOrNull(value: number): number | null { +function positiveNumberOrNull(value: number | undefined | null): number | null { return value === undefined || value === null || value === 0 ? null : value }This prevents type mismatches and improves type safety when
strictNullChecksis enabled.🤖 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 `@components/ambient-ui/src/adapters/mappers.ts` around lines 123 - 129, The parameter types for numberOrNull and positiveNumberOrNull are too narrow: change the signatures of function numberOrNull(value: number) and function positiveNumberOrNull(value: number) so their parameter accepts number | null | undefined (e.g., value: number | null | undefined) to match the runtime checks for undefined/null (and zero in positiveNumberOrNull), so TypeScript strictNullChecks won't report type errors when passing nullable values.components/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx (1)
76-76: ⚡ Quick winApply normalization before name extraction for consistency.
baseNameFromUrlstrips.gitbut not trailing slash. If a URL ends with/, the split produces an empty last segment, falling back to the full URL. ApplyingnormalizeUrlbefore splitting would align with the new normalization approach and handle edge cases likerepo.git/.♻️ Suggested fix
function baseNameFromUrl(url: string): string { - const segments = url.replace(/\.git$/, '').split('/') + const segments = normalizeUrl(url).split('/') return segments[segments.length - 1] || url }🤖 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 `@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx at line 76, baseNameFromUrl currently strips ".git" after splitting which misses URLs with trailing slashes (e.g., "repo.git/"); update baseNameFromUrl to normalize the URL first (call normalizeUrl on the input) and then perform the ".git" strip and split so the segments variable is derived from the normalized value (and optionally filter out empty segments) before falling back to the full url; refer to the baseNameFromUrl function and the segments variable in your change.
🤖 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 `@components/ambient-ui/src/adapters/mappers.ts`:
- Around line 123-129: The parameter types for numberOrNull and
positiveNumberOrNull are too narrow: change the signatures of function
numberOrNull(value: number) and function positiveNumberOrNull(value: number) so
their parameter accepts number | null | undefined (e.g., value: number | null |
undefined) to match the runtime checks for undefined/null (and zero in
positiveNumberOrNull), so TypeScript strictNullChecks won't report type errors
when passing nullable values.
In
`@components/ambient-ui/src/app/`(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsx:
- Line 76: baseNameFromUrl currently strips ".git" after splitting which misses
URLs with trailing slashes (e.g., "repo.git/"); update baseNameFromUrl to
normalize the URL first (call normalizeUrl on the input) and then perform the
".git" strip and split so the segments variable is derived from the normalized
value (and optionally filter out empty segments) before falling back to the full
url; refer to the baseNameFromUrl function and the segments variable in your
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 11bae153-6853-4600-b202-07d37ef81799
📒 Files selected for processing (4)
components/ambient-ui/src/adapters/__tests__/mappers.test.tscomponents/ambient-ui/src/adapters/mappers.tscomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/_components/resources-tab.tsxcomponents/ambient-ui/src/app/(dashboard)/[projectId]/sessions/[sessionId]/page.tsx
…omUrl - Widen numberOrNull/positiveNumberOrNull params to number | null | undefined - Use normalizeUrl in baseNameFromUrl to handle trailing slashes and .git Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge Queue Status
This pull request spent 1 minute 7 seconds in the queue, including 13 seconds running CI. Required conditions to merge |
1 participant
Summary
Follow-up to #1638 addressing CodeRabbit review comments.
isRecordtype guardtemperature=0as a valid deterministic setting (only nullmaxTokens/timeoutat zero).gitsuffix mismatches?tab=param inuseEffectTest plan
npm run buildpasses with 0 errors, 0 warningstemperature: 0displays "0" in Config tab (not "—").gitsuffix match reconciled repos without suffix🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
.gitsuffixes and trailing slashes as identical entries.