refactor: enhance type definitions and improve DomListener component#8
refactor: enhance type definitions and improve DomListener component#8manishdex25 wants to merge 3 commits into
Conversation
* Added Document type to include SignedVerifiableCredential and OpenAttestationDocument. * Made handleObfuscation optional in TemplateProps interface. * Replaced ReactDOM.findDOMNode with a ref in DomListener for better React practices. * Updated sandbox attribute in FrameConnector to include allow-presentation.
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces findDOMNode usage in DomListener with a React ref, adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/types.ts (1)
4-4: Consider a more specific name to avoid shadowing the DOMDocumenttype.The name
Documentis generic and shadows the global DOMDocumentinterface. This could cause confusion or type conflicts in files that use both this type and DOM APIs.Consider a more descriptive name like
VerifiableDocument,CredentialDocument, orTrustDocument.💡 Suggested rename
-export type Document = SignedVerifiableCredential | OpenAttestationDocument; +export type VerifiableDocument = SignedVerifiableCredential | OpenAttestationDocument;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` at line 4, The exported type named Document shadows the global DOM Document and can cause type conflicts; rename the union type (export type Document = SignedVerifiableCredential | OpenAttestationDocument) to a more specific identifier such as VerifiableDocument (or CredentialDocument/TrustDocument) and update all references/imports across the codebase (including any re-exports, type annotations, function signatures and tests) to use the new name; ensure exported API remains consistent by updating any consumers and type imports so TS build and lints pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types.ts`:
- Line 5: Update the export statement in types.ts to use double quotes for the
module specifier to match project conventions: change the export line that
references OpenAttestationDocument and SignedVerifiableCredential from
'@trustvc/trustvc' to use double quotes ("@trustvc/trustvc") so the exported
types remain the same but quote style follows the project standard.
- Line 15: The type change made handleObfuscation optional but the
govtechDemoCert/transcript.tsx template still destructures and invokes
handleObfuscation directly, causing runtime TypeError when the prop is omitted;
update the template so each invocation uses optional chaining (e.g., replace
calls to handleObfuscation(`...`) with handleObfuscation?.(`...`)) or
alternatively provide a default no-op in the component props destructure (e.g.,
handleObfuscation = () => {}) to ensure calls to handleObfuscation in the
template (the destructured prop used across multiple lines) are safe when not
supplied.
---
Nitpick comments:
In `@src/types.ts`:
- Line 4: The exported type named Document shadows the global DOM Document and
can cause type conflicts; rename the union type (export type Document =
SignedVerifiableCredential | OpenAttestationDocument) to a more specific
identifier such as VerifiableDocument (or CredentialDocument/TrustDocument) and
update all references/imports across the codebase (including any re-exports,
type annotations, function signatures and tests) to use the new name; ensure
exported API remains consistent by updating any consumers and type imports so TS
build and lints pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cc14c3e-1a3e-41e9-8200-e9ffc43baa25
📒 Files selected for processing (3)
src/components/common/DomListener.tsxsrc/components/frame/FrameConnector.tsxsrc/types.ts
| document: D; | ||
| wrappedDocument?: WrappedDocument<OpenAttestationDocument> | SignedVerifiableCredential; | ||
| handleObfuscation: (field: string) => void; | ||
| handleObfuscation?: (field: string) => void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of handleObfuscation that may need optional chaining
# Search for handleObfuscation calls without optional chaining
rg -n "handleObfuscation\s*\(" --type ts --type tsx -g '!*.d.ts' | grep -v "handleObfuscation?."
# Also check for destructuring of handleObfuscation from props
rg -n "handleObfuscation\s*}" --type ts --type tsx -g '!*.d.ts'Repository: TrustVC/decentralized-renderer-react-components
Length of output: 1842
🏁 Script executed:
#!/bin/bash
# Search for handleObfuscation usages without null checks
rg "handleObfuscation" --type ts --type tsx -B 2 -A 2Repository: TrustVC/decentralized-renderer-react-components
Length of output: 118
🌐 Web query:
TrustVC/decentralized-renderer-react-components handleObfuscation transcript.tsx
💡 Result:
The query refers to the handleObfuscation function in the context of the decentralized-renderer-react-components library (likely a fork or variant under TrustVC), specifically in a transcript.tsx file. This library is part of the OpenAttestation/TradeTrust ecosystem for rendering verifiable documents in iframes using React components. handleObfuscation is a mandatory prop provided to each configured Template React component (such as a Transcript template in transcript.tsx) when rendering a document. It is a function that triggers obfuscation (redaction/hiding) of sensitive fields in the document. Key details: - Signature/Usage: Called with a path string following lodash#get syntax, e.g., handleObfuscation('a[0].b.c'). - Behavior: Automatically sends a postMessage action { type: "OBFUSCATE", payload: path } to the iframe's decentralized renderer via components like FrameConnector or FramedDocumentRenderer. - Purpose: Allows templates to redact PII (e.g., names, IDs in transcripts) on demand, enhancing privacy in verifiable credential rendering. - Related components: ObfuscatableValue.js (976B in builds), used for rendering redactable values. No exact TrustVC repo found; closest are TradeTrust/decentralized-renderer-react-components (npm:@tradetrust-tt/decentralized-renderer-react-components v3.16.15, updated 2025) and Open-Attestation/decentralized-renderer-react-components. "Transcript" appears as an example template id/label alongside "certificate". @trustvc/trustvc is a dependency. Example in Template: const TranscriptTemplate = ({ document, handleObfuscation }) => (
Citations:
- 1: https://registry.npmjs.org/%40tradetrust-tt%2Fdecentralized-renderer-react-components
- 2: https://github.com/Open-Attestation/decentralized-renderer-react-components
- 3: https://openattestation.com/docs/lib-section/remote-files/decentralized-renderer-react-components
- 4: https://www.npmjs.com/package/@accredify/decentralized-renderer-react-components
🏁 Script executed:
#!/bin/bash
# Search for handleObfuscation usages in TypeScript/JavaScript files
rg "handleObfuscation" -B 2 -A 2 --type typescript --type javascriptRepository: TrustVC/decentralized-renderer-react-components
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Find all files containing handleObfuscation
rg "handleObfuscation" -l --max-count=20Repository: TrustVC/decentralized-renderer-react-components
Length of output: 314
🏁 Script executed:
#!/bin/bash
# List TypeScript/TSX files in the repo
fd -e ts -e tsx | head -20Repository: TrustVC/decentralized-renderer-react-components
Length of output: 1304
🏁 Script executed:
#!/bin/bash
# Check the transcript.tsx file to see how handleObfuscation is used
cat example/decentralized-renderer/templates/govtechDemoCert/transcript.tsxRepository: TrustVC/decentralized-renderer-react-components
Length of output: 7088
🏁 Script executed:
#!/bin/bash
# Check current state of src/types.ts
cat src/types.tsRepository: TrustVC/decentralized-renderer-react-components
Length of output: 1395
🏁 Script executed:
#!/bin/bash
# Check all template files for handleObfuscation usage patterns
for file in $(find example/decentralized-renderer/templates -name "*.tsx" -type f); do
if rg -q "handleObfuscation" "$file"; then
echo "=== $file ==="
rg -n "handleObfuscation" "$file" -A 1
fi
doneRepository: TrustVC/decentralized-renderer-react-components
Length of output: 831
Breaking change: transcript.tsx template will throw at runtime when handleObfuscation is not provided.
Making handleObfuscation optional introduces a breaking change. The govtechDemoCert/transcript.tsx template destructures and calls handleObfuscation(...) without null checks at lines 35, 42, 49, 56, and 63. When a renderer omits this prop, these calls will throw TypeError: handleObfuscation is not a function at runtime.
Fix by updating the template to use optional chaining: handleObfuscation?.(\...`)`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types.ts` at line 15, The type change made handleObfuscation optional but
the govtechDemoCert/transcript.tsx template still destructures and invokes
handleObfuscation directly, causing runtime TypeError when the prop is omitted;
update the template so each invocation uses optional chaining (e.g., replace
calls to handleObfuscation(`...`) with handleObfuscation?.(`...`)) or
alternatively provide a default no-op in the component props destructure (e.g.,
handleObfuscation = () => {}) to ensure calls to handleObfuscation in the
template (the destructured prop used across multiple lines) are safe when not
supplied.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| this.rootRef = React.createRef<HTMLDivElement>(); | ||
| } | ||
| componentDidMount(): void { | ||
| // eslint-disable-next-line react/no-find-dom-node |
There was a problem hiding this comment.
findDOMNode is legacy/deprecated behavior in modern React (especially with Strict Mode / React 18).
* Removed 'allow-presentation' from the sandbox attribute for improved security.
1 participant
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Summary by CodeRabbit
Improvements
Updates