#1712 | [DMP 2025] Signature capture #1678
Open
Conversation
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
This was referenced Jun 22, 2025
ShivangMishra
force-pushed
the
dmp-2025-signature-capture
branch
from
d03763b to
c72ff90
Compare
WalkthroughIntroduces a Signature form element for capturing, saving, and previewing signatures. Adds signature rendering in observations. Integrates the new element into form rendering. Adds a dependency on react-native-signature-canvas and updates react-native-webview. Implements file storage for signatures in the device images directory. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant F as FormElementGroup
participant S as SignatureFormElement
participant C as SignatureCanvas (WebView)
participant FS as RNFS / FileSystem
participant ST as Form Store
U->>F: Open form section
F->>S: Render SignatureFormElement
S->>C: Initialize canvas
U->>C: Draw signature
C-->>S: onOK(dataURL)
S->>S: Parse dataURL, gen UUID filename
S->>FS: Write base64 -> images dir
FS-->>S: Success/Failure
alt Success
S->>ST: Dispatch PRIMITIVE_VALUE_CHANGE (fileName)
else Failure
S->>U: Alert error
end
sequenceDiagram
autonumber
participant V as Observations.renderValue
participant M as ObservationModel
participant SD as SignatureFormElement.signatureFileDirectory
participant RN as React Native Image
V->>M: getValueWrapper().getValue() -> fileName
V->>SD: Get images directory
V->>RN: Render Image with file://<dir>/<fileName>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/openchs-android/src/views/common/Observations.js (1)
25-25: Avoid coupling to a UI component for a filesystem constantImport FileSystem directly instead of pulling SignatureFormElement just to access its static directory.
- import SignatureFormElement from "../form/formElement/SignatureFormElement"; + import FileSystem from "../../model/FileSystem";packages/openchs-android/src/views/form/formElement/SignatureFormElement.js (1)
31-33: Use the wrapper API to read the valuePrefer the wrapper’s getValue() to stay consistent with the rest of the codebase; fall back to answer if needed.
- get signatureFilename() { - return _.get(this, "props.value.answer"); - } + get signatureFilename() { + return (this.props.value && typeof this.props.value.getValue === 'function') + ? this.props.value.getValue() + : _.get(this, "props.value.answer"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/openchs-android/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/openchs-android/package.json(1 hunks)packages/openchs-android/src/views/common/Observations.js(3 hunks)packages/openchs-android/src/views/form/FormElementGroup.js(2 hunks)packages/openchs-android/src/views/form/formElement/SignatureFormElement.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/openchs-android/src/views/form/formElement/SignatureFormElement.js (1)
packages/openchs-android/src/model/FileSystem.js (1)
FileSystem(6-143)
🔇 Additional comments (3)
packages/openchs-android/package.json (1)
94-101: Peer dependencies are compatible – RN 0.72.8 satisfies both libraries’ requirements (signature-canvas needs webview ≥13 and webview 13.15.0 supports any React Native version), no further changes needed.packages/openchs-android/src/views/form/FormElementGroup.js (2)
46-46: Import of SignatureFormElement — LGTMImport path and placement are consistent with existing elements.
224-231: Signature element wiring — LGTM, confirm model support at runtimeRendering branch correctly mirrors other primitive elements. Please ensure the app ships with an openchs-models version that defines Concept.dataType.Signature to avoid this branch being unreachable.
Comment on lines
+167
to
+173
| } else if (renderType === Concept.dataType.Signature) { | ||
| return ( | ||
| <View style={this.styles.observationColumn}> | ||
| <Image width={100} height={100} source={{uri: `file://${SignatureFormElement.signatureFileDirectory}/${observationModel.getValueWrapper().getValue()}`}}/> | ||
| <Text></Text> | ||
| </View> | ||
| ); |
There was a problem hiding this comment.
RN Image sizing bug + direct FileSystem usage
Image’s width/height must be set via style. Also use FileSystem directly (matches the import change above).
- } else if (renderType === Concept.dataType.Signature) {
- return (
- <View style={this.styles.observationColumn}>
- <Image width={100} height={100} source={{uri: `file://${SignatureFormElement.signatureFileDirectory}/${observationModel.getValueWrapper().getValue()}`}}/>
- <Text></Text>
- </View>
- );
+ } else if (renderType === Concept.dataType.Signature) {
+ const fileName = observationModel.getValueWrapper().getValue();
+ return (
+ <View style={this.styles.observationColumn}>
+ <Image
+ style={{width: 100, height: 100}}
+ source={{uri: `file://${FileSystem.getImagesDir()}/${fileName}`}}
+ />
+ </View>
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (renderType === Concept.dataType.Signature) { | |
| return ( | |
| <View style={this.styles.observationColumn}> | |
| <Image width={100} height={100} source={{uri: `file://${SignatureFormElement.signatureFileDirectory}/${observationModel.getValueWrapper().getValue()}`}}/> | |
| <Text></Text> | |
| </View> | |
| ); | |
| } else if (renderType === Concept.dataType.Signature) { | |
| const fileName = observationModel.getValueWrapper().getValue(); | |
| return ( | |
| <View style={this.styles.observationColumn}> | |
| <Image | |
| style={{ width: 100, height: 100 }} | |
| source={{ uri: `file://${FileSystem.getImagesDir()}/${fileName}` }} | |
| /> | |
| </View> | |
| ); |
🤖 Prompt for AI Agents
In packages/openchs-android/src/views/common/Observations.js around lines
167-173, the Image is using width/height props and a hardcoded file:// path;
change it to set width and height via the Image style (e.g. style={{width: 100,
height: 100}}) and build the image URI using the imported FileSystem API rather
than concatenating 'file://'; replace the source prop with one that uses
FileSystem (e.g. construct the path from FileSystem +
SignatureFormElement.signatureFileDirectory + filename) so the Image source uses
the FileSystem-derived URI.
Comment on lines
+41
to
+55
| const [header, base64Data] = signatureValue.split(','); | ||
| const mimeType = header.match(/data:(.*?);/)[1]; | ||
| const ext = mimeType.split('/')[1]; | ||
|
|
||
| const fileName = `${General.randomUUID()}.${ext}`; | ||
| const filePath = `${SignatureFormElement.signatureFileDirectory}/${fileName}`; | ||
|
|
||
| fs.writeFile(filePath, base64Data, 'base64') | ||
| .then(() => { | ||
| this.onUpdateObservations(fileName); | ||
| }) | ||
| .catch((error) => { | ||
| AlertMessage(`Error saving signature: ${error.message}`, "error"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden data-URL parsing and clean up previous file on save
Guard against malformed data and delete any previously saved file to prevent orphaned files.
- const [header, base64Data] = signatureValue.split(',');
- const mimeType = header.match(/data:(.*?);/)[1];
+ const [header, base64Data] = signatureValue.split(',');
+ const match = header && header.match(/data:(.*?);/);
+ if (!match || !base64Data) {
+ AlertMessage(this.I18n.t("unexpectedSignatureFormat") || "Unexpected signature data format", "error");
+ return;
+ }
+ const mimeType = match[1];
const ext = mimeType.split('/')[1];
const fileName = `${General.randomUUID()}.${ext}`;
const filePath = `${SignatureFormElement.signatureFileDirectory}/${fileName}`;
- fs.writeFile(filePath, base64Data, 'base64')
- .then(() => {
- this.onUpdateObservations(fileName);
- })
+ const prevFile = this.signatureFilename;
+ fs.writeFile(filePath, base64Data, 'base64')
+ .then(async () => {
+ if (prevFile && prevFile !== fileName) {
+ const prevPath = `${SignatureFormElement.signatureFileDirectory}/${prevFile}`;
+ await fs.unlink(prevPath).catch(() => {});
+ }
+ this.onUpdateObservations(fileName);
+ })
.catch((error) => {
AlertMessage(`Error saving signature: ${error.message}`, "error");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [header, base64Data] = signatureValue.split(','); | |
| const mimeType = header.match(/data:(.*?);/)[1]; | |
| const ext = mimeType.split('/')[1]; | |
| const fileName = `${General.randomUUID()}.${ext}`; | |
| const filePath = `${SignatureFormElement.signatureFileDirectory}/${fileName}`; | |
| fs.writeFile(filePath, base64Data, 'base64') | |
| .then(() => { | |
| this.onUpdateObservations(fileName); | |
| }) | |
| .catch((error) => { | |
| AlertMessage(`Error saving signature: ${error.message}`, "error"); | |
| }); | |
| } | |
| async saveSignature(signatureValue) { | |
| // Parse the data-URL and validate | |
| const [header, base64Data] = signatureValue.split(','); | |
| const match = header && header.match(/data:(.*?);/); | |
| if (!match || !base64Data) { | |
| AlertMessage( | |
| this.I18n.t("unexpectedSignatureFormat") || "Unexpected signature data format", | |
| "error" | |
| ); | |
| return; | |
| } | |
| const mimeType = match[1]; | |
| const ext = mimeType.split('/')[1]; | |
| // Prepare new file path | |
| const fileName = `${General.randomUUID()}.${ext}`; | |
| const filePath = `${SignatureFormElement.signatureFileDirectory}/${fileName}`; | |
| // Remember previous file so we can delete it once the new one is saved | |
| const prevFile = this.signatureFilename; | |
| fs.writeFile(filePath, base64Data, 'base64') | |
| .then(async () => { | |
| // Remove the old signature file if it’s different | |
| if (prevFile && prevFile !== fileName) { | |
| const prevPath = `${SignatureFormElement.signatureFileDirectory}/${prevFile}`; | |
| await fs.unlink(prevPath).catch(() => { | |
| // ignore any unlink errors | |
| }); | |
| } | |
| this.onUpdateObservations(fileName); | |
| }) | |
| .catch((error) => { | |
| AlertMessage(`Error saving signature: ${error.message}`, "error"); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/openchs-android/src/views/form/formElement/SignatureFormElement.js
around lines 41 to 55, the data-URL parsing is brittle and previous signature
files are never cleaned up; validate the signatureValue before splitting (ensure
it contains a comma and non-empty parts), safely extract the mimeType with a
regex check and guard the match result, fall back to a safe extension if
extraction fails, and before writing the new file delete any existing signature
file for this element (check existence and call fs.unlink or fs.unlinkSync,
handling errors). Then write the file and only call
this.onUpdateObservations(fileName) on successful write; propagate or log errors
from parse, unlink and write operations via AlertMessage.
Comment on lines
+66
to
+80
| clearAnswer() { | ||
| this.updateValue(null); | ||
| } | ||
|
|
||
| handleSignatureData = (signature) => { | ||
| this.updateValue(signature); | ||
| }; | ||
|
|
||
| handleEmpty = () => { | ||
| this.updateValue(null, ValidationResult.failure(this.props.element.uuid, this.I18n.t("signatureRequired"))); | ||
| }; | ||
|
|
||
| handleClear = () => { | ||
| this.clearAnswer(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Delete on clear to avoid storage leaks
Ensure clearing also removes the existing file from disk.
clearAnswer() {
- this.updateValue(null);
+ const prevFile = this.signatureFilename;
+ if (prevFile) {
+ const prevPath = `${SignatureFormElement.signatureFileDirectory}/${prevFile}`;
+ fs.unlink(prevPath).catch(() => {});
+ }
+ this.updateValue(null);
}
handleClear = () => {
this.clearAnswer();
};Committable suggestion skipped: line range outside the PR's diff.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Work in Progress
The target branch should be community, not master. but community branch is 47 commits behind, so for now, I've set the target as master. I think it will make the PR easier to review.
Depends on
Summary by CodeRabbit
New Features
Chores