fix(tdf3): remove TODO from customer-facing error message#952
fix(tdf3): remove TODO from customer-facing error message#952davidaronhopper-dev wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 19 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIn ChangesError Message Update in splitLookupTableFactory
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the error message thrown when duplicate Key Access Server (KAS) URLs are detected within a split, replacing a placeholder TODO error with a user-friendly message and adding a detailed TODO comment for future implementation. The review feedback suggests simplifying the multi-line template literal concatenation into a single template literal string to improve readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
![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
🤖 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 `@lib/tdf3/src/tdf.ts`:
- Around line 729-734: The error message thrown in the InvalidFileError for
duplicate KAS URLs has been changed, but the test assertion in
lib/tests/mocha/unit/tdf.spec.ts that validates this error message still expects
the old message format. Find the test that asserts on the error message for
duplicate KAS URL scenarios and update the expected string to match the new
error message about "Multiple keys detected for Key Access Server" that is now
being thrown in the InvalidFileError constructor call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c097966-ee22-4a84-8a6c-eba0c0a1ef51
📒 Files selected for processing (1)
lib/tdf3/src/tdf.ts
Replace 'TODO: Fallback to no split ids' error with user-friendly message. Keep developer TODO as code comment for future implementation. Fixes DSPX-3454 Signed-off-by: Aaron Hopper <aaron.hopper@virtru.com>
Signed-off-by: Aaron Hopper <aaron.hopper@virtru.com>
Update test to match new customer-friendly error message. Signed-off-by: Aaron Hopper <aaron.hopper@virtru.com>
davidaronhopper-dev
force-pushed
the
DSPX-3454
branch
from
b562811 to
9bff8cc
Compare
|
| for (const kao of keyAccess) { | ||
| const disjunction = splitPotentials[kao.sid ?? '']; | ||
| if (kao.url in disjunction) { | ||
| // TODO(DSPX-3454): Implement fallback to no split ids when repetition is detected. |
There was a problem hiding this comment.
If each KAO has the KID I think it should be fine. It seems like the TODO should be to update this function with that knowledge? And we probably need some xtest test added to validate how it works across SDKs currently.
There was a problem hiding this comment.
From what JP said in slack, this ticket didn't really need a "fix" it just needed to have the TODO removed from anything client facing, I can update the PR if that isn't all that should be done, I also have another PR to check the whole codebase for other TODOs in a similar vein
There was a problem hiding this comment.
Yeah, I mean I don't want to misdirect with the TODO. I'm pretty sure using no split ids is not correct.
2 participants
Summary
Fixes DSPX-3454 - removes TODO from customer-facing error message in Secure Viewer.
A customer reported seeing:
"TODO: Fallback to no split ids. Repetition found for..."in error output when decryption failed.Changes
"Unable to decrypt: Multiple keys detected for Key Access Server [URL]. Please contact your administrator."Impact
Related
Summary by CodeRabbit