Skip to content

fix(tdf3): remove TODO from customer-facing error message - #952

Merged
davidaronhopper-dev merged 4 commits into
mainfrom
DSPX-3454
Jun 26, 2026
Merged

fix(tdf3): remove TODO from customer-facing error message#952
davidaronhopper-dev merged 4 commits into
mainfrom
DSPX-3454

Conversation

@davidaronhopper-dev

@davidaronhopper-dev davidaronhopper-dev commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

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

  • Replaced customer-facing error with user-friendly message: "Unable to decrypt: Multiple keys detected for Key Access Server [URL]. Please contact your administrator."
  • Kept developer TODO as code comment for future implementation of split ID fallback logic

Impact

  • Customers no longer see internal TODO messages in error output
  • Developer context preserved in code comments for future work

Related

  • Follow-up ticket created: DSPX-3666 (audit codebase-wide for other customer-facing TODOs)

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation error messaging for split key-to-URL configuration issues. If multiple keys map to the same URL within a split, the error now clearly reports “Multiple keys detected” and includes guidance to contact your administrator.
  • Tests
    • Updated the unit test expectations to match the revised error message.

@davidaronhopper-dev
davidaronhopper-dev requested a review from a team as a code owner June 22, 2026 20:52
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f7b5cea2-40f8-4fc3-88fc-40390419ea87

📥 Commits

Reviewing files that changed from the base of the PR and between 079c8f3 and 179042f.

📒 Files selected for processing (2)
  • lib/tdf3/src/tdf.ts
  • lib/tests/mocha/unit/tdf.spec.ts
✅ Files skipped from review due to trivial changes (1)
  • lib/tests/mocha/unit/tdf.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/tdf3/src/tdf.ts

📝 Walkthrough

Walkthrough

splitLookupTableFactory now throws a revised InvalidFileError message for repeated KAS URLs, and the unit test expectation was updated to match the new text.

Changes

Error Message Update in splitLookupTableFactory

Layer / File(s) Summary
InvalidFileError message for repeated KAS URL
lib/tdf3/src/tdf.ts, lib/tests/mocha/unit/tdf.spec.ts
Updates the repeated KAS URL error message to “Multiple keys detected … Please contact your administrator” and changes the duplicate-URL test to assert the new message.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested reviewers

  • eugenioenko

Poem

A rabbit found keys side by side,
and nudged the old TODO aside.
“Please contact your admin,” it chimed,
while the test kept the message aligned.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: replacing a customer-facing TODO error with a user-facing decryption message.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch DSPX-3454

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread lib/tdf3/src/tdf.ts Outdated
@github-actions

Copy link
Copy Markdown

If these changes look good, signoff on them with:

git pull && git commit --amend --signoff && git push --force-with-lease origin

If they aren't any good, please remove them with:

git pull && git reset --hard HEAD~1 && git push --force-with-lease origin

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a0ad0c2 and 48e3bcb.

📒 Files selected for processing (1)
  • lib/tdf3/src/tdf.ts

Comment thread lib/tdf3/src/tdf.ts Outdated
davidaronhopper-dev and others added 3 commits June 22, 2026 16:32
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>
Comment thread lib/tdf3/src/tdf.ts Outdated
Update TODO to reflect correct solution approach: use KID information
already present in each KAO to differentiate keys from same KAS,
rather than falling back to no split ids.

Addresses ntrevino's review feedback on PR #952.

DSPX-3454
@sonarqubecloud

Copy link
Copy Markdown

@davidaronhopper-dev
davidaronhopper-dev merged commit 19f8d53 into main Jun 26, 2026
26 of 28 checks passed
@davidaronhopper-dev
davidaronhopper-dev deleted the DSPX-3454 branch June 26, 2026 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants