fix: prettifyModel variable reference bug#30
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors and exports prettifyModel to always use a trimmed/normalized input for all model-name checks and fallbacks; adds a new unit test suite validating behavior across Claude, OpenAI GPT variants, o3/o4 anchors, unknown models, and whitespace handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
📝 Coding Plan
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.
Actionable comments posted: 2
🤖 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/web/__tests__/unit/prettify-model.test.ts`:
- Around line 3-21: prettifyModel logic is duplicated and post-share.ts is
missing the legacy Claude matching (includes("opus"/"sonnet"/"haiku")), causing
inconsistent prettification; extract the function into a single pure utility
(e.g., export prettifyModel from lib/utils/format.ts), implement the full logic
including the legacy includes checks, then replace the local definitions in
ActivityCard.tsx, post-share.ts, and the unit test with imports from the new
utility so all three consumers use the same function and avoid functional drift.
In `@apps/web/lib/email/unsubscribe.ts`:
- Around line 7-12: createUnsubscribeToken now returns string | null when
UNSUBSCRIBE_SECRET is missing; update all callers (e.g.,
send-reactivation-email.ts, send-comment-email.ts, send-weekly-digest.ts,
send-referral-joined-email.ts, send-welcome-email.ts,
send-direct-message-email.ts, send-winback-email.ts, send-empty-profile-email.ts
and app/api/cron/nudge-inactive/route.ts) to guard against a null token: call
createUnsubscribeToken(userId) and if it returns null, either omit the
unsubscribe link from the email payload or early-return/throw (depending on the
handler semantics) to avoid generating URLs like ?token=null or sending emails
without a valid unsubscribe mechanism. Ensure the check happens before building
the unsubscribe URL or inserting the token into templates so no invalid token is
emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34f76128-3ef5-4f70-87f1-63193a3b1bc0
📒 Files selected for processing (5)
apps/web/__tests__/unit/prettify-model.test.tsapps/web/__tests__/unit/unsubscribe.test.tsapps/web/components/app/feed/ActivityCard.tsxapps/web/lib/email/unsubscribe.tsdocs/CHANGELOG.md
Comment on lines
+3
to
+21
| // prettifyModel is a local function in ActivityCard.tsx — extract its logic | ||
| // here for unit testing. Kept in sync with the source. | ||
| function prettifyModel(model: string): string { | ||
| const normalized = model.trim(); | ||
| if (/claude-opus-4/i.test(normalized)) return "Claude Opus"; | ||
| if (/claude-sonnet-4/i.test(normalized)) return "Claude Sonnet"; | ||
| if (/claude-haiku-4/i.test(normalized)) return "Claude Haiku"; | ||
| if (/^gpt-/i.test(normalized)) { | ||
| return normalized | ||
| .replace(/^gpt/i, "GPT") | ||
| .replace(/-codex$/i, "-Codex"); | ||
| } | ||
| if (/^o4/i.test(normalized)) return "o4"; | ||
| if (/^o3/i.test(normalized)) return "o3"; | ||
| if (normalized.includes("opus")) return "Claude Opus"; | ||
| if (normalized.includes("sonnet")) return "Claude Sonnet"; | ||
| if (normalized.includes("haiku")) return "Claude Haiku"; | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for all occurrences of prettifyModel across the codebase
rg -n "function prettifyModel" --type tsRepository: ohong/straude
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Extract full prettifyModel implementations from all three locations
echo "=== ActivityCard.tsx ==="
sed -n '38,55p' apps/web/components/app/feed/ActivityCard.tsx
echo -e "\n=== post-share.ts ==="
sed -n '12,29p' apps/web/lib/utils/post-share.ts
echo -e "\n=== prettify-model.test.ts ==="
sed -n '5,21p' apps/web/__tests__/unit/prettify-model.test.tsRepository: ohong/straude
Length of output: 2197
Extract prettifyModel to a shared utility to prevent functional drift.
The function is duplicated in three locations, and post-share.ts is already missing critical logic: it lacks the legacy Claude matching (lines checking for includes("opus"), includes("sonnet"), includes("haiku")), which means model names like "claude-opus" won't be prettified correctly in share operations. Extract prettifyModel to a pure utility (e.g., lib/utils/format.ts) and import from a single source across ActivityCard.tsx, post-share.ts, and tests to eliminate this functional divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/__tests__/unit/prettify-model.test.ts` around lines 3 - 21,
prettifyModel logic is duplicated and post-share.ts is missing the legacy Claude
matching (includes("opus"/"sonnet"/"haiku")), causing inconsistent
prettification; extract the function into a single pure utility (e.g., export
prettifyModel from lib/utils/format.ts), implement the full logic including the
legacy includes checks, then replace the local definitions in ActivityCard.tsx,
post-share.ts, and the unit test with imports from the new utility so all three
consumers use the same function and avoid functional drift.
Comment on lines
+7
to
+12
| export function createUnsubscribeToken(userId: string): string | null { | ||
| const secret = process.env.UNSUBSCRIBE_SECRET; | ||
| if (!secret) throw new Error("UNSUBSCRIBE_SECRET not configured"); | ||
| if (!secret) { | ||
| console.error("UNSUBSCRIBE_SECRET not configured"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of createUnsubscribeToken to verify they handle null
rg -n "createUnsubscribeToken" --type ts -A 3Repository: ohong/straude
Length of output: 12801
All email builders and cron handlers must guard against null unsubscribe tokens.
The signature change from string to string | null is a breaking API change. Without null checks, all 9+ callers will produce invalid unsubscribe URLs (e.g., ?token=null) when UNSUBSCRIBE_SECRET is missing:
send-reactivation-email.ts:20send-comment-email.ts:46send-weekly-digest.ts:179send-referral-joined-email.ts:32send-welcome-email.ts:30send-direct-message-email.ts:31send-winback-email.ts:20send-empty-profile-email.ts:23app/api/cron/nudge-inactive/route.ts:100
Each call site should either skip the unsubscribe link or early-return/throw to prevent sending emails without valid unsubscribe mechanisms.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/lib/email/unsubscribe.ts` around lines 7 - 12,
createUnsubscribeToken now returns string | null when UNSUBSCRIBE_SECRET is
missing; update all callers (e.g., send-reactivation-email.ts,
send-comment-email.ts, send-weekly-digest.ts, send-referral-joined-email.ts,
send-welcome-email.ts, send-direct-message-email.ts, send-winback-email.ts,
send-empty-profile-email.ts and app/api/cron/nudge-inactive/route.ts) to guard
against a null token: call createUnsubscribeToken(userId) and if it returns
null, either omit the unsubscribe link from the email payload or
early-return/throw (depending on the handler semantics) to avoid generating URLs
like ?token=null or sending emails without a valid unsubscribe mechanism. Ensure
the check happens before building the unsubscribe URL or inserting the token
into templates so no invalid token is emitted.
The function computed `normalized = model.trim()` but then used raw `model` for most regex tests and .includes() calls. Whitespace-padded model names failed anchor-based matches (^o3, ^o4, ^gpt-) and returned untrimmed strings for unknown models. All references now use `normalized`. Exported the function so tests import from source instead of copy-pasting. Added 12 unit tests covering whitespace, all provider patterns, and legacy fallbacks. Created 6 GitHub issues for larger items found during TD review: - #31 Race condition in usage/submit multi-device flow - #32 Deduplicate prettifyModel across 3 files - #33 Add rate limiting to DELETE endpoints - #34 Add missing test coverage for 17 untested API routes - #35 PR #22 multi-provider support review feedback - #36 Validate leaderboard cursor input Nightshift-Task: td-review Nightshift-Ref: https://github.com/marcus/nightshift Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ohong
force-pushed
the
nightshift/td-review
branch
from
5bb051e to
5f3b698
Compare
ohong
changed the title
Closed duplicate issues #31-36 (duplicates of #24-29 from prior iteration). Nightshift-Task: td-review Nightshift-Ref: https://github.com/marcus/nightshift Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/web/components/app/feed/ActivityCard.tsx`:
- Around line 52-54: The legacy Claude fallback checks use case-sensitive
includes on variable normalized; make them case-insensitive by lowercasing
normalized once before those checks (e.g., compute a lowercase string like
normalizedLower = normalized.toLowerCase() or reassign normalized =
normalized.toLowerCase()) and then use normalizedLower.includes("opus"),
normalizedLower.includes("sonnet"), and normalizedLower.includes("haiku") in the
ActivityCard component so the fallback matches values like
"Anthropic/Claude-3-Opus".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da90b3c7-4378-4840-b2d4-aa42c36ce75f
📒 Files selected for processing (3)
apps/web/__tests__/unit/prettify-model.test.tsapps/web/components/app/feed/ActivityCard.tsxdocs/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/CHANGELOG.md
Comment on lines
+52
to
+54
| if (normalized.includes("opus")) return "Claude Opus"; | ||
| if (normalized.includes("sonnet")) return "Claude Sonnet"; | ||
| if (normalized.includes("haiku")) return "Claude Haiku"; |
There was a problem hiding this comment.
Make the legacy Claude fallback case-insensitive.
Lines 52-54 use case-sensitive includes(), so values like Anthropic/Claude-3-Opus will miss the fallback even though the regex branches above are case-insensitive. Lowercasing once before these checks keeps the behavior consistent.
💡 Proposed fix
export function prettifyModel(model: string): string {
const normalized = model.trim();
+ const normalizedLower = normalized.toLowerCase();
if (/claude-opus-4/i.test(normalized)) return "Claude Opus";
if (/claude-sonnet-4/i.test(normalized)) return "Claude Sonnet";
if (/claude-haiku-4/i.test(normalized)) return "Claude Haiku";
// Preserve full OpenAI model names (e.g. gpt-5.3-codex -> GPT-5.3-Codex)
if (/^gpt-/i.test(normalized)) {
return normalized
.replace(/^gpt/i, "GPT")
.replace(/-codex$/i, "-Codex");
}
if (/^o4/i.test(normalized)) return "o4";
if (/^o3/i.test(normalized)) return "o3";
// Legacy: broader Claude matching
- if (normalized.includes("opus")) return "Claude Opus";
- if (normalized.includes("sonnet")) return "Claude Sonnet";
- if (normalized.includes("haiku")) return "Claude Haiku";
+ if (normalizedLower.includes("opus")) return "Claude Opus";
+ if (normalizedLower.includes("sonnet")) return "Claude Sonnet";
+ if (normalizedLower.includes("haiku")) return "Claude Haiku";
return normalized;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/components/app/feed/ActivityCard.tsx` around lines 52 - 54, The
legacy Claude fallback checks use case-sensitive includes on variable
normalized; make them case-insensitive by lowercasing normalized once before
those checks (e.g., compute a lowercase string like normalizedLower =
normalized.toLowerCase() or reassign normalized = normalized.toLowerCase()) and
then use normalizedLower.includes("opus"), normalizedLower.includes("sonnet"),
and normalizedLower.includes("haiku") in the ActivityCard component so the
fallback matches values like "Anthropic/Claude-3-Opus".
Owner
Author
|
Merged. The bug was still present on main — Note: the duplicate issues (#31-36) mentioned in the PR body were already closed as dupes of #24-29. |
6 tasks
8 tasks
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
prettifyModelinconsistent variable references inActivityCard.tsx— the function created a trimmednormalizedvariable but then used the rawmodelparameter for regex tests and.includes()fallbacks. Whitespace-padded model names (e.g.," o3-mini") would fail anchor-based regex matches (^o3,^o4,^gpt-) and the final return would include untrimmed whitespace. All references now usenormalized.prettifyModelcovering Claude, OpenAI, o3/o4, legacy fallbacks, whitespace handling, and unknown model pass-through. Tests import directly from the source module.Open issues for follow-up work:
Test plan
bun test apps/web/__tests__/unit/prettify-model.test.ts— 12 tests pass🤖 Generated with Claude Code
Automated by nightshift
Summary by CodeRabbit
Bug Fixes
Tests