Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Only thing that is pending are the devlog badges! |
NeonGamerBot-QK
left a comment
NeonGamerBot-QK
left a comment
There was a problem hiding this comment.
everything lgtm but can you add some admin ui (like on the admin projects there should be smt which shows lapse attachments, adjustments into the super mega dash) + i can remove this review if u wana do it in another pr.
|
Can we get some screenshots and screencasts? |
|
Devlog selector: 
Lapse badge: 
Timelapse modal (please ignore missing thumbnail, ffmpeg was missing when testing so Lapse didn't generate thumbnails - this won't happen on prod): |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| hackatime_identity = @project.users.first&.hackatime_identity | ||
| hackatime_identity&.uid.present? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Please don't merge yet, I noticed some stuff that needs changing |
Lapse badges on projects are now displayed only for project owners! Devlog badges are displayed always. |
|
@cskartikey Should be ready to merge! Before doing so, though, please add these environment variables to Coolify: |
| response = Faraday.get(playback_url) | ||
| unless response.success? | ||
| return false | ||
| end | ||
|
|
||
| content_type = response.headers["content-type"] || "video/mp4" | ||
| extension = Rack::Mime::MIME_TYPES.invert[content_type] || ".mp4" | ||
| filename = "timelapse-#{timelapse_id}#{extension}" | ||
|
|
||
| @devlog.attachments.attach( | ||
| io: StringIO.new(response.body), |
There was a problem hiding this comment.
Bug: The synchronous Faraday request to download a Lapse video lacks an explicit timeout, potentially blocking a worker thread for up to 10 seconds.
Severity: MEDIUM
Suggested Fix
Add an explicit timeout to the Faraday.get request to ensure it fails faster than the 10-second advisory lock. This aligns with best practices seen elsewhere in the codebase, such as setting req.options.timeout = 10.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: app/controllers/projects/devlogs_controller.rb#L243-L253
Potential issue: The `attach_lapse_timelapse` method initiates a synchronous
`Faraday.get` request to download a video without a configured timeout. This is
inconsistent with other network calls in the codebase that do have explicit timeouts.
While the operation is wrapped in an advisory lock with a 10-second timeout, which
prevents an indefinite hang, a worker thread can still be blocked for up to 10 seconds
waiting for the download. This can degrade application performance and availability
under concurrent requests.
| return redirect_to @project, alert: "Could not calculate your coding time. Please try again." unless @preview_time.present? | ||
|
|
||
| @devlog = Post::Devlog.new(devlog_params) | ||
| Rails.logger.info "DevlogsController#create: media_type=#{params[:media_type].inspect}, lapse_timelapse_id=#{params.dig(:post_devlog, :lapse_timelapse_id).inspect}" |
There was a problem hiding this comment.
remnant of dev? maybe remove this
| extension = Rack::Mime::MIME_TYPES.invert[content_type] || ".mp4" | ||
| filename = "timelapse-#{timelapse_id}#{extension}" | ||
|
|
||
| @devlog.attachments.attach( |
There was a problem hiding this comment.
might be a good idea to queue a bg job?
| @preview_time = nil | ||
| end | ||
|
|
||
| def attach_lapse_timelapse |
There was a problem hiding this comment.
Since this is being called from the controller and involves downloading a video from a remote URL, should we move this into a background job?
Also, if the timelapse is already stored on R2, is there a way for us to create the ActiveStorage blob without downloading and re-uploading it through th rails server?
|
|
||
| def format_timelapse_duration(seconds) | ||
| return "0s" if seconds.nil? || seconds <= 0 | ||
|
|
There was a problem hiding this comment.
this type of formatting seems like it's spread across the entire codebase. It might be a good idea to DRY it
3 participants
This PR adds several Lapse features to Flavortown! ★,°:.☆(￣▽￣)/$:.°★ 。
This includes the following:
LapseService,