Add the recording file name to store the recordings in the DB #266
Conversation
β¦and remove unwanted parameters from env
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update adds GPLv3 license headers to multiple Java source files in the video call module and modifies environment property files by introducing a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
π Outside diff range comments (6)
src/main/java/com/iemr/common/data/videocall/VideoCallParameters.java (1)
79-82: No column present for recording filename β conflicts with PR objective.The entity ends at
linkUsed; there is norecordingFileName(or similar) field/column.
This contradicts the stated goal of persisting the recording file name.+ @Column(name = "RecordingFileName") + private String recordingFileName;Add field plus getters/setters (or Lombok
@Dataalready covers) and update the mapper/service & DB migration script.src/main/java/com/iemr/common/controller/videocall/VideoCallController.java (2)
70-87: Inconsistent response pattern & unused objects β refactorsendVideoLink.
- The method returns a plain
Stringwhereas the surrounding endpoints useResponseEntity.OutputResponse responseis created but never returned on success.HttpServletRequest requestis injected but unused.- Missing explicit
Authorizationheader parameter.Consider:
-@PostMapping(value = "/send-link", produces = MediaType.APPLICATION_JSON_VALUE, headers = "Authorization") -public String sendVideoLink(@RequestBody String requestModel, HttpServletRequest request) { +@PostMapping(value = "/send-link", produces = MediaType.APPLICATION_JSON_VALUE, headers = "Authorization") +public ResponseEntity<String> sendVideoLink(@RequestBody String requestModel, + @RequestHeader("Authorization") String authHeader) { try { ... - return serviceResponse; + return ResponseEntity.ok(serviceResponse); } catch (Exception e) { ... - return response.toString(); + return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(response.toString()); } }
57-63: Add@RequestHeader("Authorization")to VideoCallController.generateJitsiLinkTo keep API signatures consistent with other controllers (e.g., AbdmFacilityController), explicitly include the
Authorizationheader as a method parameterβeven if it isnβt used in the body.β’ File: src/main/java/com/iemr/common/controller/videocall/VideoCallController.java
Method: generateJitsiLink (lines 57β63)@PostMapping(value = "/generate-link", produces = MediaType.APPLICATION_JSON_VALUE, headers = "Authorization") - public ResponseEntity<Map<String, String>> generateJitsiLink() { + public ResponseEntity<Map<String, String>> generateJitsiLink( + @RequestHeader("Authorization") String authHeader) { Map<String, String> response = new HashMap<>(); try { String link = videoCallService.generateMeetingLink();src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java (3)
58-63: Shared mutable state in a singleton@Serviceβ thread-safety & data leakage risk.
meetingLink,isLinkSentandconsultationStatusare instance fields.
Because Spring creates a single bean, simultaneous sessions will overwrite each otherβs data, causing users to receive the wrong meeting link or status.- private String meetingLink; - private boolean isLinkSent = false; - private String consultationStatus = "Not Initiated"; + /* Remove the fields β keep data per request / per entity. + Generate the link inside the method and persist it; fetch it back + from DB when needed. */Failing to address this will introduce race conditions and corrupt call records.
80-87:sendMeetingLinkrelies on globalmeetingLink; breaks when called first.If the client skips
/generate-linkand directly invokes/send-link, this method throws βMeeting link not generated yet.β Generating the link lazily inside this method would remove that ordering constraint and eliminate the shared state.- if (meetingLink == null || meetingLink.isEmpty()) { - throw new Exception("Meeting link not generated yet."); - } + if (meetingLink == null || meetingLink.isEmpty()) { + meetingLink = generateMeetingLink(); + }
138-140: Propertyjibri.output.pathremoved from configs β method will now NPE.
saveRecordingFilestill readsjibri.output.path, yet the property was deleted in this PR. At runtimeConfigProperties.getPropertyByNamewill returnnull, leading tonew File(null)βNullPointerException.Update to use the new column / property introduced for recording-file handling, or inject a fallback value.
-String jibriOutputDir = ConfigProperties.getPropertyByName("jibri.output.path"); +@Value("${recording-source-path:}") // inject via Spring with default empty +private String jibriOutputDir;
π§Ή Nitpick comments (3)
src/main/environment/common_example.properties (1)
201-203: Placeholder β?β at the end of the URL may break callers.Trailing query-mark will produce URLs such as
...?room=abcβhttps://vc.piramalswasthya.org/?room=abc.
If the backend concatenates parameters without checking for an existing?, the result will be...?room=abc?another=value, which is invalid. Consider removing the placeholder symbol or documenting why it is needed.src/main/java/com/iemr/common/mapper/videocall/VideoCallMapper.java (1)
35-38: Consider relying on Spring injection instead of the staticINSTANCE.Having both
componentModel = "spring"and the manualINSTANCEfield is redundant and can encourage anti-patterns (η΄ζ₯θ°η¨θι注ε ₯).src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java (1)
125-128: Recording persistence is commented out β feature incomplete.The new DB column to store recording file names is never populated because the invocation of
saveRecordingFileis commented. Remove the comments and adapt to the new property names so recordings are actually persisted.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (12)
src/main/environment/common_ci.properties(1 hunks)src/main/environment/common_docker.properties(1 hunks)src/main/environment/common_example.properties(1 hunks)src/main/java/com/iemr/common/controller/videocall/VideoCallController.java(1 hunks)src/main/java/com/iemr/common/data/videocall/VideoCallParameters.java(1 hunks)src/main/java/com/iemr/common/mapper/videocall/VideoCallMapper.java(1 hunks)src/main/java/com/iemr/common/model/videocall/UpdateCallRequest.java(1 hunks)src/main/java/com/iemr/common/model/videocall/UpdateCallResponse.java(1 hunks)src/main/java/com/iemr/common/model/videocall/VideoCallRequest.java(1 hunks)src/main/java/com/iemr/common/repository/videocall/VideoCallParameterRepository.java(1 hunks)src/main/java/com/iemr/common/service/videocall/VideoCallService.java(1 hunks)src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java(1 hunks)
π§° Additional context used
π§ Learnings (7)
π Common learnings
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
src/main/java/com/iemr/common/repository/videocall/VideoCallParameterRepository.java (1)
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
src/main/java/com/iemr/common/controller/videocall/VideoCallController.java (3)
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:31-31
Timestamp: 2024-12-07T19:32:42.660Z
Learning: In this codebase, for Java Spring Boot API controllers like `AbdmFacilityController.java`, it's a common pattern to include the `Authorization` parameter in method signatures, even if it's not used in the implementation, to maintain consistency across APIs.
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
Learnt from: srishtigrp78
PR: PSMRI/Common-API#166
File: src/main/java/com/iemr/common/service/grievance/GrievanceHandlingServiceImpl.java:422-431
Timestamp: 2025-03-03T05:44:08.973Z
Learning: In the Common-API project, JsonNode from Jackson library has been found to not work properly for certain JSON parsing tasks, so JSONObject is preferred in those cases, particularly in the GrievanceHandlingServiceImpl class.
src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java (2)
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java:43-43
Timestamp: 2025-05-09T12:54:22.518Z
Learning: The meeting link in the video call consultation service uses 8 characters for the random part due to link shortening requirements.
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
src/main/environment/common_example.properties (1)
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
src/main/environment/common_docker.properties (1)
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
src/main/environment/common_ci.properties (2)
Learnt from: vanitha1822
PR: PSMRI/Common-API#180
File: src/main/environment/common_example.properties:192-192
Timestamp: 2025-05-09T12:55:44.224Z
Learning: The video-call-url property for Jitsi integration is configured in local.properties file, while common_example.properties contains only a placeholder value.
Learnt from: indraniBan
PR: PSMRI/Common-API#138
File: src/main/environment/common_uat.properties:0-0
Timestamp: 2025-02-03T12:41:59.244Z
Learning: In the Common-API project, JWT secret configuration should use environment variables (e.g., `jwt.secret=@env.JWT_SECRET_KEY@`) across all environments for better security.
π Additional comments (11)
src/main/java/com/iemr/common/model/videocall/VideoCallRequest.java (1)
1-22: License header looks good and matches project standard.src/main/java/com/iemr/common/repository/videocall/VideoCallParameterRepository.java (2)
1-22: Header addition acknowledged β no functional impact.
42-46: Potential missing support for recording filename update.PR description says: βAdd the recording file name to store the recordings in the DB.β
Repository methodupdateCallStatusByMeetingLinkupdatescallStatusandcallDurationonly. If a new column (e.g.,RecordingFileName) was added, the query and method signature need an extra parameter.Please confirm whether:
- The column exists in
VideoCallParameters.- A write path is provided.
If not, extend the query accordingly.
src/main/java/com/iemr/common/model/videocall/UpdateCallRequest.java (1)
1-22: License header aligned with other files β looks fine.src/main/java/com/iemr/common/data/videocall/VideoCallParameters.java (1)
1-22: Header addition OK.src/main/java/com/iemr/common/service/videocall/VideoCallService.java (1)
1-21: GPL header looks good β no further action.Header is correctly formatted and consistent with the rest of the module.
src/main/java/com/iemr/common/mapper/videocall/VideoCallMapper.java (1)
1-21: Header addition OK.No issues spotted with licence block.
src/main/java/com/iemr/common/model/videocall/UpdateCallResponse.java (1)
1-21: GPL header looks good β no further action.src/main/java/com/iemr/common/controller/videocall/VideoCallController.java (2)
1-22: Header looks good β GPL v3 text added correctly.No issues detected with the newly-added licence block.
89-114:OutputResponseserialisation may yield invalid JSON.
response.toString()depends onOutputResponse#toString. If that method is not guaranteed to output valid JSON, this endpoint will return malformed data. PreferObjectMapperorOutputMapperto serialise the object explicitly.src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java (1)
1-22: Header addition is correct.New GPL v3 block added without affecting functionality.
| # Jitsi configuration | ||
| video-call-url=https://vc.piramalswasthya.org/? | ||
| jibri.output.path=/srv/jibri/recordings | ||
| video.recording.path=/srv/recordings |
There was a problem hiding this comment.
π‘ Verification agent
𧩠Analysis chain
Property naming is inconsistent with existing conventions β consider video.call.url instead.
Most keys in this file use dot-separated segments (video.recording.path, spring.datasource.url, etc.).
Using a dash (video-call-url) creates a one-off pattern and may complicate property binding in Spring (Environment#getProperty("video-call-url") will not map automatically to relaxed-binding POJOs).
- video-call-url=https://vc.piramalswasthya.org/?
+ video.call.url=https://vc.piramalswasthya.org/?Before changing, verify whether other files (e.g. common_ci.properties, common_docker.properties, or Java @Value injections) already expect the dash form; align all of them to one style.
π Script executed:
#!/bin/bash
# Search for occurrences of the dashed property key
rg --no-heading -n "video-call-url"
# Search for occurrences of the dotted variant
rg --no-heading -n "video.call.url"
# Check for Java @Value injections using the dashed key
rg --no-heading -n "@Value.*video-call-url"
# Check for Environment.getProperty calls using the dashed key
rg --no-heading -n "getProperty.*video-call-url"Length of output: 1502
Ensure consistent dot-notation for the Jitsi URL property
The key video-call-url is currently used in three properties files and in VideoCallServiceImpl. Renaming it to video.call.url will require updating all references:
β’ src/main/environment/common_example.properties:202
β’ src/main/environment/common_ci.properties:187
β’ src/main/environment/common_docker.properties:189
β’ src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java:63 (and the commented ConfigProperties.getPropertyByName on line 67)
Suggested changes:
# in each .properties file
- video-call-url=https://vc.piramalswasthya.org/?
+ video.call.url=https://vc.piramalswasthya.org/?# in VideoCallServiceImpl.java
- @Value("${video-call-url}")
+ @Value("${video.call.url}")
// And if you re-enable the ConfigProperties call:
//- this.jitsiLink = ConfigProperties.getPropertyByName("video-call-url");
//+ this.jitsiLink = ConfigProperties.getPropertyByName("video.call.url");After applying, verify Springβs relaxed binding still works and update any CI/docker environment variables (e.g. VIDEO_CALL_URL) to match.
π 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.
| # Jitsi configuration | |
| video-call-url=https://vc.piramalswasthya.org/? | |
| jibri.output.path=/srv/jibri/recordings | |
| video.recording.path=/srv/recordings | |
| # Jitsi configuration | |
| video.call.url=https://vc.piramalswasthya.org/? | |
| video.recording.path=/srv/recordings |
π€ Prompt for AI Agents
In src/main/environment/common_example.properties around lines 201 to 203,
rename the property key from video-call-url to video.call.url to maintain
consistent dot-notation. Apply the same change in common_ci.properties line 187,
common_docker.properties line 189, and update all references in
VideoCallServiceImpl.java at line 63 and the commented
ConfigProperties.getPropertyByName at line 67. After renaming, verify that
Springβs relaxed binding still functions correctly and update any related CI or
Docker environment variables such as VIDEO_CALL_URL accordingly.
| # Jitsi configuration | ||
| video-call-url=@env.VIDEO_CALL_URL@ | ||
| jibri.output.path=@env.JIBRI_OUTPUT_PATH@ | ||
| video.recording.path=@env.VIDEO_RECORDING_PATH@ |
There was a problem hiding this comment.
π‘ Verification agent
𧩠Analysis chain
Verify downstream usage of the new key video-call-url.
Please ensure all Java / YAML code now reads video-call-url and no references to the removed jibri.output.path remain.
π Script executed:
#!/bin/bash
# Search for the old property key and the new one.
rg --no-heading --line-number $'jibri.output.path|video-call-url'Length of output: 797
Replace remaining Jibri path reference with the new video.recording.path
The code still pulls the old jibri.output.path key in VideoCallServiceImpl. Update it to use the new video.recording.path:
β’ src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java:138
- Change
String jibriOutputDir = ConfigProperties.getPropertyByName("jibri.output.path");
- To something like
String recordingPath = ConfigProperties.getPropertyByName("video.recording.path");
(and rename jibriOutputDir accordingly)
π€ Prompt for AI Agents
In src/main/java/com/iemr/common/service/videocall/VideoCallServiceImpl.java
around line 138, replace the usage of the old configuration key
"jibri.output.path" with the new key "video.recording.path". Update the variable
name from jibriOutputDir to recordingPath to reflect this change. Modify the
call to ConfigProperties.getPropertyByName accordingly to fetch
"video.recording.path" instead.
| # Jitsi configuration | ||
| video-call-url=${VIDEO_CALL_URL} | ||
| jibri.output.path={JIBRI_OUTPUT_PATH} | ||
| video.recording.path={VIDEO_RECORDING_PATH} |
There was a problem hiding this comment.
Placeholder typo will break property expansion.
${VIDEO_RECORDING_PATH} was accidentally changed to {VIDEO_RECORDING_PATH} β the Spring placeholder syntax requires the leading $.
-video.recording.path={VIDEO_RECORDING_PATH}
+video.recording.path=${VIDEO_RECORDING_PATH}π 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.
| # Jitsi configuration | |
| video-call-url=${VIDEO_CALL_URL} | |
| jibri.output.path={JIBRI_OUTPUT_PATH} | |
| video.recording.path={VIDEO_RECORDING_PATH} | |
| # Jitsi configuration | |
| video-call-url=${VIDEO_CALL_URL} | |
| video.recording.path=${VIDEO_RECORDING_PATH} |
π€ Prompt for AI Agents
In src/main/environment/common_docker.properties around lines 188 to 190, the
placeholder for VIDEO_RECORDING_PATH is missing the leading '$' in the property
video.recording.path. Fix this by changing {VIDEO_RECORDING_PATH} to
${VIDEO_RECORDING_PATH} to ensure proper Spring property expansion.
|
π Description
JIRA ID:
NA
Add the License for all the newly created VC files.
Add the column and functionality to store the recordings file name for future use.
β Type of Change
Summary by CodeRabbit