Conversation
Brings in 54 upstream commits including: - v2.3.4: SQL wildcard injection fix, CanCan authorization on downloads, 2FA refactoring, readonly field validation, credential hiding - v2.3.5: Rich text/markdown email editor, conditional signing party invitation, radio select in formula fields, Cloudflare R2 fix Conflict resolutions: - submissions_download_controller.rb: Kept upstream CanCan authorization check + Kencove's Rails.logger (instead of Rollbar) - Gemfile.lock: Re-bundled to include both upstream deps and Kencove's omniauth/oauth2 gems Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- SendSms: add open_timeout (8s) and read_timeout (15s) matching webhook config, return parsed Twilio JSON on success for SID capture - SendSubmitterInvitationSmsJob: disable Sidekiq's 25 blind retries, implement manual 5-attempt exponential backoff with error logging, store Twilio SID in SubmissionEvent.data on success - RateLimit: switch STORE from per-process MemoryStore to Redis-backed cache (falls back to MemoryStore if REDIS_URL not set or unavailable) - Add specs for SendSms, SendSubmitterInvitationSmsJob, and RateLimit Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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 Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive markdown editor component for rich text editing, implements centralized authorization checks via a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FormController as Form Controller
participant AuthModule as AuthorizedForForm
participant SubmitValues as SubmitValues
participant Inviter as Invitation<br/>Workflow
participant Event as Event Tracker
User->>FormController: Complete form submission
FormController->>AuthModule: call(submitter, user, request)
AuthModule->>AuthModule: pass_email_2fa?()
AuthModule->>AuthModule: pass_link_2fa?()
AuthModule-->>FormController: authorization result
alt Authorized
FormController->>SubmitValues: maybe_invite_via_field()
SubmitValues->>Inviter: Create submitters from<br/>field mappings
Inviter-->>SubmitValues: New submitters created
SubmitValues->>Event: Track invitation events
Event-->>SubmitValues: Logged
FormController->>Event: Track form_complete
Event-->>FormController: Logged
FormController-->>User: Success response
else Not Authorized
FormController-->>User: Access denied
end
sequenceDiagram
actor User
participant Editor as Markdown Editor
participant TipTap as TipTap<br/>Instance
participant Toolbar as Toolbar<br/>Actions
participant Hidden as Hidden<br/>Textarea
participant Form as Form<br/>Submit
User->>Editor: Focus editor
Editor->>TipTap: Initialize with extensions
TipTap-->>Editor: Ready
User->>Toolbar: Click bold button
Toolbar->>TipTap: Execute bold command
TipTap->>Editor: Apply formatting
User->>Editor: Type or paste content
Editor->>Editor: Update on-change
Editor->>Hidden: Sync markdown to textarea
User->>Form: Submit form
Form->>Hidden: Read textarea value
Hidden-->>Form: Markdown content
Form-->>User: Submitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@cursor-review review |
Author
|
bugbot run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/javascript/submission_form/completed.vue (1)
190-207:⚠️ Potential issue | 🟠 MajorPrevent method override and handle all error cases in
download().At lines 191–192,
...this.fetchOptionswill overridemethod: 'GET'iffetchOptionscontains amethodkey. Additionally,isDownloadingis not reset in two scenarios: (1) when the fetch response is not OK (line 205), and (2) when a network error occurs.🔧 Proposed fix
fetch(this.baseUrl + `/submitters/${this.submitterSlug}/download`, { - method: 'GET', - ...this.fetchOptions + ...this.fetchOptions, + method: 'GET' }).then(async (response) => { if (response.ok) { const urls = await response.json() const isMobileSafariIos = 'ontouchstart' in window && navigator.maxTouchPoints > 0 && /AppleWebKit/i.test(navigator.userAgent) const isSafariIos = isMobileSafariIos || /iPhone|iPad|iPod/i.test(navigator.userAgent) if (isSafariIos && urls.length > 1) { this.downloadSafariIos(urls) } else { this.downloadUrls(urls) } } else { alert(this.t('failed_to_download_files')) + this.isDownloading = false } +}).catch(() => { + alert(this.t('failed_to_download_files')) + this.isDownloading = false })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/submission_form/completed.vue` around lines 190 - 207, The fetch call can have its method overridden by this.fetchOptions and isDownloading isn't reset on non-OK responses or network errors; update the fetch options so the explicit method wins (e.g., merge with ...this.fetchOptions first and then method: 'GET', or use Object.assign({}, this.fetchOptions, { method: 'GET' })) and add error handling to reset this.isDownloading: set this.isDownloading = false in the else branch where response.ok is false and add a .catch(...) (or a .finally(...)) on the fetch promise to reset this.isDownloading on network errors, keeping the existing calls to downloadSafariIos(urls) / downloadUrls(urls) unchanged.app/views/submissions/show.html.erb (1)
99-99:⚠️ Potential issue | 🟡 MinorRemove redundant
keyword_init: trueto fix pipeline failure.Ruby 3.2+ includes keyword initialization by default for
Struct.new.🔧 Proposed fix
-<% page_blob_struct = Struct.new(:url, :metadata, keyword_init: true) %> +<% page_blob_struct = Struct.new(:url, :metadata) %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/submissions/show.html.erb` at line 99, The Struct declaration page_blob_struct = Struct.new(:url, :metadata, keyword_init: true) is failing the pipeline because Ruby 3.2+ makes keyword_init redundant; remove the keyword_init: true argument so the Struct is declared as Struct.new(:url, :metadata) (keep the page_blob_struct variable name intact) to resolve the failure.app/views/submit_form/show.html.erb (1)
8-8:⚠️ Potential issue | 🟡 MinorRemove redundant
keyword_init: trueto fix pipeline failure.Ruby 3.2+ includes keyword initialization by default for
Struct.new, making thekeyword_init: trueoption redundant.🔧 Proposed fix
-<% page_blob_struct = Struct.new(:url, :metadata, keyword_init: true) %> +<% page_blob_struct = Struct.new(:url, :metadata) %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/submit_form/show.html.erb` at line 8, Remove the redundant keyword_init argument when defining the Struct: update the page_blob_struct declaration (the Struct.new call used to define page_blob_struct) to drop keyword_init: true so it relies on Ruby 3.2+ default keyword initialization; this removes the unnecessary option and fixes the pipeline failure.
🧹 Nitpick comments (15)
spec/system/signing_form_spec.rb (2)
645-645: Consider specifying the dropdown explicitly to avoid ambiguity.Using
select 'Approved'without afrom:argument works when there's only one dropdown, but could become fragile if additional dropdowns are added to the form. Consider specifying the dropdown explicitly for clarity and maintainability.♻️ Suggested improvement
- select 'Approved' + select 'Approved', from: 'signing_reason' # or the appropriate label/id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/system/signing_form_spec.rb` at line 645, The test currently calls the Capybara helper select with just the value (select 'Approved'), which is ambiguous; update that call to include an explicit from: argument referencing the dropdown's label or name (e.g. the status/select field label or id used in the form) so the selector targets the correct select element; locate the select invocation in signing_form_spec.rb and change it to use select('Approved', from: '<field label or id>') to make the spec robust and maintainable.
629-655: Test does not verify the signing reason was persisted.The test selects
'Approved'as the signing reason but only assertscompleted_atandSignaturepresence. Consider adding an assertion to verify the signing reason value was actually saved (e.g., checking for the reason insubmitter.values, submission events, or relevant model attributes).💡 Suggested addition
expect(submitter.completed_at).to be_present expect(field_value(submitter, 'Signature')).to be_present + # Verify signing reason was saved - adjust based on where the reason is stored + expect(submitter.signing_reason).to eq('Approved') endNote: Adjust the assertion based on how the signing reason is actually stored in your domain model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/system/signing_form_spec.rb` around lines 629 - 655, Add an assertion after submitter.reload to verify the chosen signing reason was persisted: locate the test's submitter and use the existing helpers or model attributes (e.g. field_value(submitter, 'Signing reason') or submitter.values / submitter.form_values) to assert it equals 'Approved'; alternatively assert the last submission event or audit record for the submission includes the 'Approved' reason if your app stores reasons there (use symbols/fields present in the spec like submitter, field_value, submission/events to find the correct place).lib/templates/create_attachments.rb (1)
78-83: Consider using block form forZip::File.opento ensure cleanup on exception.With the new exception path at line 83, the
Zip::Filehandle may not be properly closed whenInvalidFileTypeis raised mid-iteration. Using the block form ensures the file is closed regardless of exceptions.♻️ Suggested improvement
- Zip::File.open(file.tempfile).each do |entry| + Zip::File.open(file.tempfile) do |zip_file| + zip_file.each do |entry| next if entry.directory? total_size += entry.size raise InvalidFileType, 'zip_too_large' if total_size > MAX_ZIP_SIZE tempfile = Tempfile.new(entry.name) tempfile.binmode entry.get_input_stream { |in_stream| IO.copy_stream(in_stream, tempfile) } tempfile.rewind type = Marcel::MimeType.for(tempfile, name: entry.name) next if type.exclude?('image') && type != PDF_CONTENT_TYPE && type != JSON_CONTENT_TYPE && DOCUMENT_CONTENT_TYPES.exclude?(type) extracted_files << ActionDispatch::Http::UploadedFile.new( filename: File.basename(entry.name), type:, tempfile: ) + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/templates/create_attachments.rb` around lines 78 - 83, The Zip::File is opened with Zip::File.open(file.tempfile) and may not be closed if InvalidFileType is raised while iterating; change to the block form Zip::File.open(file.tempfile) do |zip| ... end (or use Zip::File.open with a block) so the archive is always closed on exception, keep the same logic that skips directories, accumulates total_size and raises InvalidFileType with 'zip_too_large' when total_size > MAX_ZIP_SIZE; ensure you reference the same variables (entry, total_size, MAX_ZIP_SIZE) inside the block.app/javascript/elements/toggle_attribute.js (1)
6-7: Inconsistent type coercion in value/dataValue comparison.The code normalizes
dataset.value === 'false'to booleanfalse(line 7), but compares it againstevent.target.valuefrom select elements, which return strings. While this works via JavaScript's loose comparison, it's inconsistent with line 17, which compares string values directly.For clarity and consistency, normalize both
'true'and'false'strings to their boolean equivalents:♻️ Suggested improvement
- const dataValue = this.dataset.value === 'false' ? false : this.dataset.value || true + const dataValue = this.dataset.value === 'false' ? false : (this.dataset.value === 'true' ? true : this.dataset.value)This makes the intent explicit: string values like
'email'are compared as strings, while'false'/'true'are converted to booleans for explicit conditional logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/elements/toggle_attribute.js` around lines 6 - 7, Normalize string booleans consistently: update the logic that sets value and dataValue in toggle_attribute.js so both handle the literal strings "true" and "false" the same way (convert "true" -> true and "false" -> false), while leaving other strings (e.g., "email") as-is; specifically adjust the assignment for value (derived from event.target.value or checked) and dataValue (this.dataset.value) so comparisons later in the function (referencing value and dataValue) are comparing normalized types, or create a small helper (e.g., parseBooleanString) and use it when setting value and dataValue.lib/submitters.rb (1)
17-17: Consider using a distinct name for the custom exception.Defining
ArgumentErrorwithin theSubmittersmodule shadows Ruby's built-inArgumentError. While scoped correctly asSubmitters::ArgumentError, this could cause confusion when debugging or when someone rescuesArgumentErrorexpecting standard library behavior.A more explicit name like
MissingFileErrororInvalidArgumentErrorwould improve clarity.💡 Suggested rename
- ArgumentError = Class.new(StandardError) + MissingFileError = Class.new(StandardError)And update the raise statement:
- raise ArgumentError, 'file param is missing' + raise MissingFileError, 'file param is missing'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/submitters.rb` at line 17, Rename the custom exception constant currently defined as ArgumentError within the Submitters module to a distinct name like InvalidArgumentError (i.e. define Submitters::InvalidArgumentError instead of Submitters::ArgumentError) and update every raise that refers to this custom error to raise Submitters::InvalidArgumentError, plus update any local rescue clauses that expect the module-local ArgumentError to use the new InvalidArgumentError symbol so you no longer shadow Ruby's built-in ArgumentError.spec/lib/rate_limit_spec.rb (1)
5-42: Add explicit coverage for Redis fallback behavior.Behavioral tests are solid, but this suite does not assert the new store-selection contract (Redis when available, MemoryStore fallback when
REDIS_URLis missing/unavailable). A focused spec here would protect the main PR objective from regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/lib/rate_limit_spec.rb` around lines 5 - 42, Add specs that assert the store-selection contract by testing RateLimit::STORE directly under both Redis-present and Redis-absent/failing conditions: stub ENV['REDIS_URL'] (or temporarily delete it) and/or stub the Redis client initializer used by RateLimit to raise, then reload or reinitialize RateLimit and assert described_class::STORE is an instance of MemoryStore; also add a complementary spec that sets ENV['REDIS_URL'] to a fake URL and ensures described_class::STORE is an instance of the Redis-backed store (e.g., RedisStore) so the code paths in RateLimit.initialize/STORE selection and the .call behavior are covered.app/javascript/submission_form/signature_step.vue (1)
793-801: Extract duplicated blocked-canvas alert/logging into one helper.The new branch is good, but the same block now appears twice. Centralizing it will reduce drift and make future message/log updates safer.
♻️ Proposed refactor
+ notifyCanvasBlocked () { + if (isCanvasBlocked()) { + alert(this.t('browser_privacy_settings_block_canvas')) + if (window.Rollbar) window.Rollbar.info('Canvas blocked') + return true + } + return false + }, @@ - if (isCanvasBlocked()) { - alert(this.t('browser_privacy_settings_block_canvas')) - - if (window.Rollbar) { - window.Rollbar.info('Canvas blocked') - } - } else { + if (!this.notifyCanvasBlocked()) { alert(this.t('signature_is_too_small_or_simple_please_redraw')) } @@ - if (isCanvasBlocked()) { - alert(this.t('browser_privacy_settings_block_canvas')) - - if (window.Rollbar) { - window.Rollbar.info('Canvas blocked') - } - } else { + if (!this.notifyCanvasBlocked()) { alert(this.t('signature_is_too_small_or_simple_please_redraw')) }Also applies to: 857-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/submission_form/signature_step.vue` around lines 793 - 801, Extract the duplicated "canvas blocked" alert/logging into a single helper (e.g., create a method like showCanvasBlockedAlert or handleBlockedCanvas) and replace both occurrences with calls to that helper; the helper should call alert(this.t('browser_privacy_settings_block_canvas')) and, if window.Rollbar exists, call window.Rollbar.info('Canvas blocked') so both the UI alert and logging behavior from the existing isCanvasBlocked() branches are centralized (update usages where isCanvasBlocked() is checked in the signature_step.vue code, including the other occurrence around lines 857-865).spec/jobs/send_submitter_invitation_sms_job_spec.rb (1)
114-124: Consider adding a test forFaraday::ConnectionFailed.The job rescues
Faraday::ConnectionFailedbut onlyFaraday::TimeoutError(viato_timeout) is tested. Consider adding coverage for connection failures.Example test for connection failure
context 'when connection fails' do before do stub_request(:post, twilio_url).to_raise(Faraday::ConnectionFailed.new('Connection refused')) end it 'schedules a retry' do expect { described_class.new.perform('submitter_id' => submitter.id) }.to change(described_class.jobs, :size).by(1) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/send_submitter_invitation_sms_job_spec.rb` around lines 114 - 124, Add a new spec context that mirrors the existing timeout test but simulates a Faraday::ConnectionFailed to cover the rescue path: stub the POST to twilio_url to_raise(Faraday::ConnectionFailed.new(...)) and assert that calling described_class.new.perform('submitter_id' => submitter.id) changes described_class.jobs.size by 1; reference the existing context for "when network timeout occurs", the twilio_url stub, and the perform invocation to model the new "when connection fails" test.lib/replace_email_variables.rb (1)
172-174: Consider usingbuild_url_options_forfor consistency.
build_submission_linkstill usesDocuseal.default_url_optionsdirectly while other link builders now use the newbuild_url_options_forhelper. This could lead to inconsistent URLs if custom domains are configured.Suggested change for consistency
def build_submission_link(submission) - Rails.application.routes.url_helpers.submission_url(submission, **Docuseal.default_url_options) + submitter = submission.submitters.first + url_options = submitter ? build_url_options_for(submitter) : Docuseal.default_url_options + Rails.application.routes.url_helpers.submission_url(submission, **url_options) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/replace_email_variables.rb` around lines 172 - 174, build_submission_link currently calls submission_url with Docuseal.default_url_options; change it to use the shared helper build_url_options_for to keep URL generation consistent with other link builders. Locate the build_submission_link method and replace the second argument so it passes build_url_options_for(submission) (or build_url_options_for(object) as used elsewhere) instead of Docuseal.default_url_options, ensuring submission_url still receives the same submission object and that build_url_options_for is available in the scope.lib/rate_limit.rb (1)
6-15: Consider logging when falling back to MemoryStore.The silent fallback to
MemoryStorewhen Redis initialization fails could mask configuration issues. In production, operators may not realize rate limiting isn't working across workers.Add logging for fallback scenario
STORE = begin redis_url = ENV.fetch('REDIS_URL', nil) if redis_url.present? ActiveSupport::Cache::RedisCacheStore.new(url: redis_url, namespace: 'rate_limit') else ActiveSupport::Cache::MemoryStore.new end -rescue StandardError +rescue StandardError => e + Rails.logger.warn("RateLimit: Failed to initialize Redis cache (#{e.class}: #{e.message}), falling back to MemoryStore") ActiveSupport::Cache::MemoryStore.new end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/rate_limit.rb` around lines 6 - 15, The STORE fallback silently swallows Redis init failures; modify the block that creates ActiveSupport::Cache::RedisCacheStore (symbol STORE and the RedisCacheStore.new call) so that when initialization fails you rescue as `rescue StandardError => e` and log a clear warning/error via Rails.logger (including the redis_url and exception message/stack as appropriate) before returning ActiveSupport::Cache::MemoryStore.new; also add a short log when ENV['REDIS_URL'] is blank to indicate MemoryStore is being used for rate limiting in that case.lib/markdown_to_html.rb (1)
78-130: Complex inline parser - consider adding inline documentation.The
parse_inlinefunction handles multiple markdown constructs (code spans, links, emphasis) with a stateful approach. While functional, the logic is dense. Consider adding brief comments explaining the state machine behavior, particularly for the***triple-asterisk handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/markdown_to_html.rb` around lines 78 - 130, The parse_inline method is dense and needs brief inline comments to clarify its state-machine behavior: add a short comment above the parse_inline definition explaining its purpose and that it uses a context stack to track open inline tags; inside the tag lambda describe what TAGS lookup returns and how is_end/context push/pop works; inside the flush lambda note that it emits closing tags for all open context entries; add comments around the INLINE_TOKENIZER match loop to explain handling of code spans (m[4]), link starts/ends (m[1], m[2], m[3]), and emphasis tokens (m[5]), and explicitly document the special-case logic for the '***' branch (why it prefers ordering of '*' vs '**' based on current context). Reference symbols: parse_inline, tag lambda, flush lambda, INLINE_TOKENIZER, TAGS, context, and the '***' handling branch.app/javascript/template_builder/i18n.js (1)
196-196: Good typo fix, but consider renaming the key for consistency.The text correction from "create an send" to "create and send" is correct. However, the key name still contains the typo (
...create_an_send...). Consider renaming the key tostart_a_quick_tour_to_learn_how_to_create_and_send_your_first_documentfor consistency, though this would require updating all references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/template_builder/i18n.js` at line 196, The i18n key start_a_quick_tour_to_learn_how_to_create_an_send_your_first_document still contains the typo; rename it to start_a_quick_tour_to_learn_how_to_create_and_send_your_first_document in the locale file and update every code reference that reads this key (search for start_a_quick_tour_to_learn_how_to_create_an_send_your_first_document) so callers (components, tests, templates) use the new key; run the app/tests to ensure no missing translation errors and remove the old key entry after all references are updated.lib/submissions/generate_audit_trail.rb (1)
494-498: Consider applyingwith_timestamp_secondsto document timestamps as well.The
time_formatselection based onwith_timestamp_secondsis applied only to event log timestamps here (line 497). However, the document "Generated at" timestamps at lines 231-232 still use the hardcoded:longformat. If the intent is consistent timestamp precision across the audit trail, consider applying the same format there.💡 Optional: Apply consistent timestamp format to document timestamps
{ text: "#{I18n.t('generated_at')}: ", font: [FONT_NAME, { variant: :bold }] }, - "#{I18n.l(document.created_at.in_time_zone(timezone), format: :long, locale: account.locale)} " \ + "#{I18n.l(document.created_at.in_time_zone(timezone), format: time_format, locale: account.locale)} " \ "#{TimeUtils.timezone_abbr(timezone, document.created_at)}"Note: This would require computing
time_formatearlier inbuild_audit_trail, before thedocuments_datablock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/submissions/generate_audit_trail.rb` around lines 494 - 498, Compute time_format earlier in build_audit_trail using with_timestamp_seconds (e.g. time_format = with_timestamp_seconds ? :detailed : :long) so it’s available to the documents_data block, then replace the hardcoded :long used for the document "Generated at" timestamps with that time_format and format the document timestamp with I18n.l(document.generated_at.in_time_zone(timezone), format: time_format, locale: account.locale) and include TimeUtils.timezone_abbr(timezone, document.generated_at) to ensure consistent precision and timezone annotation across both event and document timestamps.app/views/templates_preferences/_submitter_invitation_email_form.html.erb (1)
52-52: Verify array element assumptions for subject/body extraction.Using
.firstfor subject and.lastfor body assumes thevalues_atcall returns elements in the expected order. Ifdefault_template_email_preferences_valueshas a different structure (e.g., only one element), the subject could incorrectly get the body value. Consider using explicit named access instead.💡 Alternative using explicit named access
-<% submitter_email_values = submitter_email_preferences_values || template_email_preferences_values.presence || default_template_email_preferences_values %> +<% submitter_email_subject = submitter_preferences['request_email_subject'].presence || + f.object.preferences['request_email_subject'].presence || + default_template_email_preferences_values.first %> +<% submitter_email_body = submitter_preferences['request_email_body'].presence || + f.object.preferences['request_email_body'].presence || + default_template_email_preferences_values.last %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/templates_preferences/_submitter_invitation_email_form.html.erb` at line 52, The code assigns submitter_email_values and later uses .first/.last to get subject/body which is brittle; change the extraction to explicitly pull named elements instead of relying on order: after computing submitter_email_values (from submitter_email_preferences_values, template_email_preferences_values, or default_template_email_preferences_values) extract subject and body explicitly (e.g., submitter_subject = submitter_email_values[0] || "" and submitter_body = submitter_email_values[1] || "" if the value is an array, or convert submitter_email_values to a hash and use submitter_email_values[:subject] and [:body] if it can be a hash) and add safe defaults — update references to submitter_subject and submitter_body wherever .first/.last were used.app/views/personalization_settings/_markdown_editor.html.erb (1)
16-48: Consider using platform-aware keyboard shortcut hints.The tooltips show
Ctrl+shortcuts which are Windows/Linux conventions. Mac users typically expectCmd+(⌘). Consider detecting the platform and adjusting hints accordingly, or using a generic format likeCtrl/⌘+B.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/personalization_settings/_markdown_editor.html.erb` around lines 16 - 48, The tooltip hardcodes "Ctrl+" shortcuts in the markdown toolbar; update so shortcuts are platform-aware by generating or injecting a Cmd/Ctrl-aware string instead of literal "Ctrl+". Either (A) add a view helper (e.g., platform_shortcut_hint(key) used in the data-tip attributes for each button—t('bold') + " (" + platform_shortcut_hint('B') + ")") or (B) have the markdown-editor Stimulus controller detect navigator.platform on connect and replace the data-tip strings for the buttons with the correct prefix (use the existing data-action/data-target attributes like markdown-editor#bold, markdown-editor.italicButton, markdown-editor.linkButton, markdown-editor.undoButton, markdown-editor.redoButton to find elements). Ensure the change updates all occurrences of t('...') tooltip text (bold/italic/underline/link/undo/redo) to show "Ctrl/⌘+X" or the correct platform-specific symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/javascript/elements/markdown_editor.js`:
- Around line 378-383: disconnectedCallback currently assumes this.linkTooltip
and this.editor exist and can throw if initialization in connectedCallback was
skipped or failed; guard accesses by checking existence before calling hide() or
destroy(): in disconnectedCallback, only call this.linkTooltip.hide() if
this.linkTooltip is truthy (and optionally has hide method) and only call
this.editor.destroy() if this.editor is truthy (and optionally has destroy
method), so teardown is no-op when initialization didn't complete.
- Around line 149-153: The fallback textarea is hidden before the async
loadTiptap() completes, leaving the form without an input if the import fails or
the element disconnects; change the flow in the initialization that calls
this.adjustShortcutsForPlatform() and const { Editor, ... } = await loadTiptap()
so you await and try/catch loadTiptap() first, verify this.isConnected (or
similar mount flag) after the await, and only then set
this.textarea.style.display = 'none' and proceed to instantiate
Editor/Extensions; on error (or if disconnected) keep the textarea visible, log
the failure, and abort editor creation to ensure the fallback remains usable.
In `@app/javascript/elements/toggle_attribute.js`:
- Around line 16-18: The conditional uses event.target.value directly which is
incorrect for checkboxes; update the branch inside the if
(this.dataset.className === 'hidden' && this.target.tagName === 'INPUT') block
to use the already-computed value variable (from earlier in the function)
instead of event.target.value so the disabled toggle reflects the computed
checked state; locate the logic in the toggle handler in toggle_attribute.js
(the function that computes value and then sets this.target.disabled) and
replace the event.target.value usage with value.
In `@app/javascript/elements/toggle_classes.js`:
- Around line 3-15: The click handler assumes a valid trigger and target; add
null guards so it doesn't throw: verify that the computed trigger element
(button) exists before calling button.addEventListener and bail early if
missing, ensure this.dataset.classes is present and non-empty before splitting,
and check that the resolved target (the variable target from
this.dataset.targetId or fallback to button) is non-null before attempting
target.classList.remove/add/toggle; update the logic around the variables
"button" and "target" in toggle_classes.js to perform these checks and return
early or skip class operations when they are absent.
In `@app/javascript/template_builder/formula_modal.vue`:
- Around line 185-187: normalizeFormula currently resolves referenced names
against this.template.fields which bypasses the filtering rules in fields();
change the lookup to use the filtered list returned by the fields() helper
(e.g., call this.fields().find(...)) so manually-typed self-references, circular
or non-numeric dependencies are excluded; keep the same comparison logic (use
buildDefaultName(f) and trim()) but operate on this.fields() instead of
this.template.fields to restore insertion safety.
- Around line 169-171: isNumberField currently treats strings like "--" or
"1.2.3" as numeric because the regex `/^[\d.-]+$/` is too permissive; update the
validation inside isNumberField (referencing field.type, field.options, and
option value o.value) to use a stricter numeric check — either replace the regex
with one that allows an optional sign and at most one decimal point (e.g.
/^[+-]?(?:\d+(\.\d+)?|\.\d+)$/) or validate via Number parsing combined with a
string-match to ensure no extra characters (e.g. Number.isFinite(Number(val))
plus confirming the original string matches the canonical numeric form). Ensure
the change is applied where options?.every((o) => ...) is used so only truly
numeric option values are treated as numbers.
In `@app/jobs/send_test_webhook_request_job.rb`:
- Line 29: The test-webhook validation currently always requires HTTPS (the
raise HttpsError line that checks uri.scheme and port) but must honor the same
account override used by SendWebhookRequest.call; update the condition in
SendTestWebhookRequestJob to consult the account's allow_http flag (or the same
helper used by SendWebhookRequest.call) so that if allow_http is true HTTP URIs
are permitted, otherwise enforce uri.scheme == 'https' and port 443 as before;
keep the raise HttpsError but change its predicate to mirror
SendWebhookRequest.call's logic.
In `@app/mailers/submitter_mailer.rb`:
- Around line 265-269: Add a CUSTOM_DOMAIN constant to the AccountConfig class
(e.g., CUSTOM_DOMAIN = 'custom_domain') and replace the symbol usage with that
constant wherever the key is looked up; specifically update the
maybe_set_custom_domain method to use key: AccountConfig::CUSTOM_DOMAIN and
update the usage in ReplaceEmailVariables (method in
lib/replace_email_variables.rb that references the key at the noted spot) to use
AccountConfig::CUSTOM_DOMAIN so key lookups are consistent with existing
constants like BCC_EMAILS.
In `@app/views/email_smtp_settings/index.html.erb`:
- Line 25: The required flag for the SMTP password field is inverted: in the
password input helper ff.password_field :password the required currently uses
value['password'].present? but should require input when the stored password is
missing; change the condition to use value['password'].blank? (or
!value['password'].present?) so new setups require a password and existing
setups do not force re-entry.
In `@app/views/storage_settings/_aws_form.html.erb`:
- Line 11: The secret_access_key field is always marked required which forces
users to re-enter the key on every edit; change the required flag on the
password field (the call to fff.password_field for :secret_access_key) to be
conditional so it is only required when there is no stored key (e.g. use
required: configs['secret_access_key'].blank?), preserving the placeholder
hiding when a key exists.
In `@app/views/storage_settings/_azure_form.html.erb`:
- Line 16: The password field for storage_access_key is always marked required
which forces re-entry even when an existing key is present; change the required
option on the fff.password_field :storage_access_key call to be conditional
(e.g., required only when configs['storage_access_key'] is blank) so users can
leave the masked placeholder and not have to re-enter the key on every update
while keeping the existing masked placeholder behavior.
In `@app/views/storage_settings/_google_cloud_form.html.erb`:
- Line 16: The credentials textarea currently forces re-entry by always using
required: true; update the fff.text_area for :credentials so the required option
is conditional based on whether configs['credentials'] exists (e.g. set
required: !configs['credentials'].present?). Keep the redacted placeholder
behavior (configs['credentials'].present? ? "{\n**REDACTED**\n}" : '') and only
make the field required when there are no existing credentials, referencing the
same :credentials field helper to locate the control.
In `@config/dotenv.rb`:
- Line 63: The privilege check uses Process.uid.zero? which tests the real UID
and can miss setuid/effective-root cases; update the condition to use
Process.euid.zero? so the code that gates on root privileges runs based on the
effective UID (replace the usage of Process.uid.zero? in the privilege-gated
block with Process.euid.zero? in the same code path).
In `@lib/download_utils.rb`:
- Around line 38-45: The download utility's call method currently defaults the
validate parameter to Docuseal.multitenant?, which disables URI validation in
single-tenant setups and weakens SSRF protections; change the default to
validate: false in the call method signature and any other affected methods
(same pattern at lines ~60-64), keep calling validate_uri! when validate is
true, and then update every caller that requires validation to explicitly pass
validate: true (or only enable validate at trusted, audited call sites) so
validation is secure-by-default; reference: the call method, validate parameter,
validate_uri!, and Docuseal.multitenant?.
In `@lib/submissions/generate_result_attachments.rb`:
- Around line 327-336: The code sets time_format = with_timestamp_seconds ?
:detailed : :long and then calls I18n.l(..., format: time_format), which will
raise missing translation errors for locales that lack time.formats.detailed;
update the affected locale files to add a time.formats.detailed entry for ar,
cs, he, ja, ko, pl, uk (matching the desired detailed format), or change the
code to use an existing common key (e.g., :long) as a safe fallback when
I18n.exists?("time.formats.#{time_format}", locale) is false before calling
I18n.l; modify the logic around with_timestamp_seconds/time_format or perform an
existence check so I18n.l never requests a missing :detailed format.
In `@lib/submitters/authorized_for_form.rb`:
- Around line 36-39: The code builds link_2fa_key using submitter.email without
guarding for nil, causing a 500 for phone-only submitters; update the two-factor
check in authorized_for_form (the block that reads
request.params[:two_factor_token] and builds link_2fa_key) to first return false
if submitter.email.blank? (or otherwise handle nil) before constructing
link_2fa_key and verifying with Submitter.signed_id_verifier.verified(token,
purpose: :email_two_factor) so missing emails produce a safe false result
instead of raising.
In `@lib/submitters/submit_values.rb`:
- Line 9: The PHONE_REGEXP is too permissive and unanchored (PHONE_REGEXP),
causing substrings to match; change it to an anchored pattern that enforces a
minimum number of digits and full-string matching (e.g. use \A and \z and a
digit-count requirement such as a lookahead like (?=(?:.*\d){7,}) or require at
least 7 digit characters) and include only allowed separators, then replace the
current /[+\d()\s-]+/ with that anchored, stricter regex so phone validation
only matches entire valid phone strings wherever PHONE_REGEXP is used.
- Around line 420-445: The locals email and phone can leak values between
iterations of the submission.template_submitters loop; reinitialize email and
phone to nil at the top of the loop (just after obtaining field_uuid or before
checking value) so each invite candidate starts with a fresh contact; ensure the
existing logic using PHONE_REGEXP, Submissions.normalize_email, and
submission.submitters.create! uses these reinitialized variables to avoid
cross-recipient leakage.
- Around line 456-458: In validate_value! ensure you guard against a nil/unknown
field before accessing field['readonly'] and field['uuid']; if field is nil
(meaning the submitted UUID wasn't found in template_fields) raise or return the
controlled validation error instead of touching field['readonly'], and keep the
Rollbar.warning call only after confirming field is present (reference
validate_value!, field['readonly'], and field['uuid'] to locate the checks).
---
Outside diff comments:
In `@app/javascript/submission_form/completed.vue`:
- Around line 190-207: The fetch call can have its method overridden by
this.fetchOptions and isDownloading isn't reset on non-OK responses or network
errors; update the fetch options so the explicit method wins (e.g., merge with
...this.fetchOptions first and then method: 'GET', or use Object.assign({},
this.fetchOptions, { method: 'GET' })) and add error handling to reset
this.isDownloading: set this.isDownloading = false in the else branch where
response.ok is false and add a .catch(...) (or a .finally(...)) on the fetch
promise to reset this.isDownloading on network errors, keeping the existing
calls to downloadSafariIos(urls) / downloadUrls(urls) unchanged.
In `@app/views/submissions/show.html.erb`:
- Line 99: The Struct declaration page_blob_struct = Struct.new(:url, :metadata,
keyword_init: true) is failing the pipeline because Ruby 3.2+ makes keyword_init
redundant; remove the keyword_init: true argument so the Struct is declared as
Struct.new(:url, :metadata) (keep the page_blob_struct variable name intact) to
resolve the failure.
In `@app/views/submit_form/show.html.erb`:
- Line 8: Remove the redundant keyword_init argument when defining the Struct:
update the page_blob_struct declaration (the Struct.new call used to define
page_blob_struct) to drop keyword_init: true so it relies on Ruby 3.2+ default
keyword initialization; this removes the unnecessary option and fixes the
pipeline failure.
---
Nitpick comments:
In `@app/javascript/elements/toggle_attribute.js`:
- Around line 6-7: Normalize string booleans consistently: update the logic that
sets value and dataValue in toggle_attribute.js so both handle the literal
strings "true" and "false" the same way (convert "true" -> true and "false" ->
false), while leaving other strings (e.g., "email") as-is; specifically adjust
the assignment for value (derived from event.target.value or checked) and
dataValue (this.dataset.value) so comparisons later in the function (referencing
value and dataValue) are comparing normalized types, or create a small helper
(e.g., parseBooleanString) and use it when setting value and dataValue.
In `@app/javascript/submission_form/signature_step.vue`:
- Around line 793-801: Extract the duplicated "canvas blocked" alert/logging
into a single helper (e.g., create a method like showCanvasBlockedAlert or
handleBlockedCanvas) and replace both occurrences with calls to that helper; the
helper should call alert(this.t('browser_privacy_settings_block_canvas')) and,
if window.Rollbar exists, call window.Rollbar.info('Canvas blocked') so both the
UI alert and logging behavior from the existing isCanvasBlocked() branches are
centralized (update usages where isCanvasBlocked() is checked in the
signature_step.vue code, including the other occurrence around lines 857-865).
In `@app/javascript/template_builder/i18n.js`:
- Line 196: The i18n key
start_a_quick_tour_to_learn_how_to_create_an_send_your_first_document still
contains the typo; rename it to
start_a_quick_tour_to_learn_how_to_create_and_send_your_first_document in the
locale file and update every code reference that reads this key (search for
start_a_quick_tour_to_learn_how_to_create_an_send_your_first_document) so
callers (components, tests, templates) use the new key; run the app/tests to
ensure no missing translation errors and remove the old key entry after all
references are updated.
In `@app/views/personalization_settings/_markdown_editor.html.erb`:
- Around line 16-48: The tooltip hardcodes "Ctrl+" shortcuts in the markdown
toolbar; update so shortcuts are platform-aware by generating or injecting a
Cmd/Ctrl-aware string instead of literal "Ctrl+". Either (A) add a view helper
(e.g., platform_shortcut_hint(key) used in the data-tip attributes for each
button—t('bold') + " (" + platform_shortcut_hint('B') + ")") or (B) have the
markdown-editor Stimulus controller detect navigator.platform on connect and
replace the data-tip strings for the buttons with the correct prefix (use the
existing data-action/data-target attributes like markdown-editor#bold,
markdown-editor.italicButton, markdown-editor.linkButton,
markdown-editor.undoButton, markdown-editor.redoButton to find elements). Ensure
the change updates all occurrences of t('...') tooltip text
(bold/italic/underline/link/undo/redo) to show "Ctrl/⌘+X" or the correct
platform-specific symbol.
In `@app/views/templates_preferences/_submitter_invitation_email_form.html.erb`:
- Line 52: The code assigns submitter_email_values and later uses .first/.last
to get subject/body which is brittle; change the extraction to explicitly pull
named elements instead of relying on order: after computing
submitter_email_values (from submitter_email_preferences_values,
template_email_preferences_values, or default_template_email_preferences_values)
extract subject and body explicitly (e.g., submitter_subject =
submitter_email_values[0] || "" and submitter_body = submitter_email_values[1]
|| "" if the value is an array, or convert submitter_email_values to a hash and
use submitter_email_values[:subject] and [:body] if it can be a hash) and add
safe defaults — update references to submitter_subject and submitter_body
wherever .first/.last were used.
In `@lib/markdown_to_html.rb`:
- Around line 78-130: The parse_inline method is dense and needs brief inline
comments to clarify its state-machine behavior: add a short comment above the
parse_inline definition explaining its purpose and that it uses a context stack
to track open inline tags; inside the tag lambda describe what TAGS lookup
returns and how is_end/context push/pop works; inside the flush lambda note that
it emits closing tags for all open context entries; add comments around the
INLINE_TOKENIZER match loop to explain handling of code spans (m[4]), link
starts/ends (m[1], m[2], m[3]), and emphasis tokens (m[5]), and explicitly
document the special-case logic for the '***' branch (why it prefers ordering of
'*' vs '**' based on current context). Reference symbols: parse_inline, tag
lambda, flush lambda, INLINE_TOKENIZER, TAGS, context, and the '***' handling
branch.
In `@lib/rate_limit.rb`:
- Around line 6-15: The STORE fallback silently swallows Redis init failures;
modify the block that creates ActiveSupport::Cache::RedisCacheStore (symbol
STORE and the RedisCacheStore.new call) so that when initialization fails you
rescue as `rescue StandardError => e` and log a clear warning/error via
Rails.logger (including the redis_url and exception message/stack as
appropriate) before returning ActiveSupport::Cache::MemoryStore.new; also add a
short log when ENV['REDIS_URL'] is blank to indicate MemoryStore is being used
for rate limiting in that case.
In `@lib/replace_email_variables.rb`:
- Around line 172-174: build_submission_link currently calls submission_url with
Docuseal.default_url_options; change it to use the shared helper
build_url_options_for to keep URL generation consistent with other link
builders. Locate the build_submission_link method and replace the second
argument so it passes build_url_options_for(submission) (or
build_url_options_for(object) as used elsewhere) instead of
Docuseal.default_url_options, ensuring submission_url still receives the same
submission object and that build_url_options_for is available in the scope.
In `@lib/submissions/generate_audit_trail.rb`:
- Around line 494-498: Compute time_format earlier in build_audit_trail using
with_timestamp_seconds (e.g. time_format = with_timestamp_seconds ? :detailed :
:long) so it’s available to the documents_data block, then replace the hardcoded
:long used for the document "Generated at" timestamps with that time_format and
format the document timestamp with
I18n.l(document.generated_at.in_time_zone(timezone), format: time_format,
locale: account.locale) and include TimeUtils.timezone_abbr(timezone,
document.generated_at) to ensure consistent precision and timezone annotation
across both event and document timestamps.
In `@lib/submitters.rb`:
- Line 17: Rename the custom exception constant currently defined as
ArgumentError within the Submitters module to a distinct name like
InvalidArgumentError (i.e. define Submitters::InvalidArgumentError instead of
Submitters::ArgumentError) and update every raise that refers to this custom
error to raise Submitters::InvalidArgumentError, plus update any local rescue
clauses that expect the module-local ArgumentError to use the new
InvalidArgumentError symbol so you no longer shadow Ruby's built-in
ArgumentError.
In `@lib/templates/create_attachments.rb`:
- Around line 78-83: The Zip::File is opened with Zip::File.open(file.tempfile)
and may not be closed if InvalidFileType is raised while iterating; change to
the block form Zip::File.open(file.tempfile) do |zip| ... end (or use
Zip::File.open with a block) so the archive is always closed on exception, keep
the same logic that skips directories, accumulates total_size and raises
InvalidFileType with 'zip_too_large' when total_size > MAX_ZIP_SIZE; ensure you
reference the same variables (entry, total_size, MAX_ZIP_SIZE) inside the block.
In `@spec/jobs/send_submitter_invitation_sms_job_spec.rb`:
- Around line 114-124: Add a new spec context that mirrors the existing timeout
test but simulates a Faraday::ConnectionFailed to cover the rescue path: stub
the POST to twilio_url to_raise(Faraday::ConnectionFailed.new(...)) and assert
that calling described_class.new.perform('submitter_id' => submitter.id) changes
described_class.jobs.size by 1; reference the existing context for "when network
timeout occurs", the twilio_url stub, and the perform invocation to model the
new "when connection fails" test.
In `@spec/lib/rate_limit_spec.rb`:
- Around line 5-42: Add specs that assert the store-selection contract by
testing RateLimit::STORE directly under both Redis-present and
Redis-absent/failing conditions: stub ENV['REDIS_URL'] (or temporarily delete
it) and/or stub the Redis client initializer used by RateLimit to raise, then
reload or reinitialize RateLimit and assert described_class::STORE is an
instance of MemoryStore; also add a complementary spec that sets
ENV['REDIS_URL'] to a fake URL and ensures described_class::STORE is an instance
of the Redis-backed store (e.g., RedisStore) so the code paths in
RateLimit.initialize/STORE selection and the .call behavior are covered.
In `@spec/system/signing_form_spec.rb`:
- Line 645: The test currently calls the Capybara helper select with just the
value (select 'Approved'), which is ambiguous; update that call to include an
explicit from: argument referencing the dropdown's label or name (e.g. the
status/select field label or id used in the form) so the selector targets the
correct select element; locate the select invocation in signing_form_spec.rb and
change it to use select('Approved', from: '<field label or id>') to make the
spec robust and maintainable.
- Around line 629-655: Add an assertion after submitter.reload to verify the
chosen signing reason was persisted: locate the test's submitter and use the
existing helpers or model attributes (e.g. field_value(submitter, 'Signing
reason') or submitter.values / submitter.form_values) to assert it equals
'Approved'; alternatively assert the last submission event or audit record for
the submission includes the 'Approved' reason if your app stores reasons there
(use symbols/fields present in the spec like submitter, field_value,
submission/events to find the correct place).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
Gemfile.lockis excluded by!**/*.lockpublic/apple-icon-180x180.pngis excluded by!**/*.pngpublic/apple-touch-icon-precomposed.pngis excluded by!**/*.pngpublic/apple-touch-icon.pngis excluded by!**/*.pngpublic/favicon-16x16.pngis excluded by!**/*.pngpublic/favicon-32x32.pngis excluded by!**/*.pngpublic/favicon-96x96.pngis excluded by!**/*.pngpublic/favicon.icois excluded by!**/*.icopublic/favicon.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (117)
.rubocop.ymlDockerfileGemfileapp/controllers/api/submissions_controller.rbapp/controllers/api/submitters_controller.rbapp/controllers/api/templates_controller.rbapp/controllers/submissions_download_controller.rbapp/controllers/submit_form_controller.rbapp/controllers/submit_form_decline_controller.rbapp/controllers/submit_form_download_controller.rbapp/controllers/submit_form_draw_signature_controller.rbapp/controllers/submit_form_invite_controller.rbapp/controllers/submit_form_values_controller.rbapp/controllers/template_documents_controller.rbapp/controllers/templates_controller.rbapp/controllers/templates_recipients_controller.rbapp/controllers/templates_uploads_controller.rbapp/javascript/application.jsapp/javascript/application.scssapp/javascript/elements/markdown_editor.jsapp/javascript/elements/toggle_attribute.jsapp/javascript/elements/toggle_classes.jsapp/javascript/submission_form/completed.vueapp/javascript/submission_form/form.vueapp/javascript/submission_form/i18n.jsapp/javascript/submission_form/initials_step.vueapp/javascript/submission_form/invite_form.vueapp/javascript/submission_form/signature_step.vueapp/javascript/submission_form/validate_signature.jsapp/javascript/template_builder/builder.vueapp/javascript/template_builder/formula_modal.vueapp/javascript/template_builder/i18n.jsapp/javascript/template_builder/page.vueapp/jobs/send_submitter_invitation_sms_job.rbapp/jobs/send_test_webhook_request_job.rbapp/mailers/submitter_mailer.rbapp/models/account_config.rbapp/views/email_smtp_settings/index.html.erbapp/views/icons/_arrow_back_up.html.erbapp/views/icons/_arrow_forward_up.html.erbapp/views/icons/_bold.html.erbapp/views/icons/_italic.html.erbapp/views/icons/_underline.html.erbapp/views/layouts/application.html.erbapp/views/personalization_settings/_email_body_field.html.erbapp/views/personalization_settings/_markdown_editor.html.erbapp/views/personalization_settings/_submitter_completed_email_form.html.erbapp/views/pwa/manifest.json.erbapp/views/shared/_meta.html.erbapp/views/shared/_search_input.html.erbapp/views/start_form/_policy.html.erbapp/views/storage_settings/_aws_form.html.erbapp/views/storage_settings/_azure_form.html.erbapp/views/storage_settings/_google_cloud_form.html.erbapp/views/submissions/_detailed_form.html.erbapp/views/submissions/_email_form.html.erbapp/views/submissions/_phone_form.html.erbapp/views/submissions/_send_email_base.html.erbapp/views/submissions/_value.html.erbapp/views/submissions/show.html.erbapp/views/submissions_dashboard/index.html.erbapp/views/submissions_filters/_applied_filters.html.erbapp/views/submissions_filters/_filter_modal.html.erbapp/views/submit_form/show.html.erbapp/views/submitter_mailer/_custom_content.html.erbapp/views/submitter_mailer/documents_copy_email.html.erbapp/views/submitter_mailer/invitation_email.html.erbapp/views/templates/_submission.html.erbapp/views/templates/show.html.erbapp/views/templates_preferences/_recipients.html.erbapp/views/templates_preferences/_submitter_completed_email_form.html.erbapp/views/templates_preferences/_submitter_documents_copy_email_form.html.erbapp/views/templates_preferences/_submitter_invitation_email_form.html.erbapp/views/templates_prefillable_fields/_form.html.erbapp/views/templates_share_link/show.html.erbapp/views/webhook_settings/show.html.erbconfig/dotenv.rbconfig/initializers/active_storage.rbconfig/locales/i18n.ymlconfig/routes.rblib/download_utils.rblib/load_active_storage_configs.rblib/markdown_to_html.rblib/rate_limit.rblib/replace_email_variables.rblib/send_sms.rblib/send_webhook_request.rblib/submissions.rblib/submissions/create_from_submitters.rblib/submissions/generate_audit_trail.rblib/submissions/generate_preview_attachments.rblib/submissions/generate_result_attachments.rblib/submitters.rblib/submitters/authorized_for_form.rblib/submitters/form_configs.rblib/submitters/normalize_values.rblib/submitters/submit_values.rblib/template_folders.rblib/templates.rblib/templates/clone.rblib/templates/create_attachments.rbpackage.jsonspec/jobs/send_form_completed_webhook_request_job_spec.rbspec/jobs/send_form_declined_webhook_request_job_spec.rbspec/jobs/send_form_started_webhook_request_job_spec.rbspec/jobs/send_form_viewed_webhook_request_job_spec.rbspec/jobs/send_submission_completed_webhook_request_job_spec.rbspec/jobs/send_submission_created_webhook_request_job_spec.rbspec/jobs/send_submission_expired_webhook_request_job_spec.rbspec/jobs/send_submitter_invitation_sms_job_spec.rbspec/jobs/send_template_created_webhook_request_job_spec.rbspec/jobs/send_template_updated_webhook_request_job_spec.rbspec/lib/rate_limit_spec.rbspec/lib/send_sms_spec.rbspec/rails_helper.rbspec/system/email_settings_spec.rbspec/system/signing_form_spec.rb
💤 Files with no reviewable changes (2)
- spec/system/email_settings_spec.rb
- Gemfile
Comment on lines
+149
to
+153
| this.textarea.style.display = 'none' | ||
| this.adjustShortcutsForPlatform() | ||
|
|
||
| const { Editor, Extension, Bold, Italic, Paragraph, Text, HardBreak, UndoRedo, Document, Link, Underline, Markdown, Plugin, Decoration, DecorationSet } = await loadTiptap() | ||
|
|
There was a problem hiding this comment.
Avoid hiding the fallback textarea before async editor initialization succeeds.
If dynamic imports fail (or the element disconnects during await), the editor never mounts and the form is left without an editable input.
🔧 Suggested hardening
async connectedCallback () {
if (!this.textarea || !this.editorElement) return
- this.textarea.style.display = 'none'
this.adjustShortcutsForPlatform()
- const { Editor, Extension, Bold, Italic, Paragraph, Text, HardBreak, UndoRedo, Document, Link, Underline, Markdown, Plugin, Decoration, DecorationSet } = await loadTiptap()
+ let tiptap
+ try {
+ tiptap = await loadTiptap()
+ } catch (_error) {
+ return
+ }
+ if (!this.isConnected) return
+
+ this.textarea.style.display = 'none'
+ const { Editor, Extension, Bold, Italic, Paragraph, Text, HardBreak, UndoRedo, Document, Link, Underline, Markdown, Plugin, Decoration, DecorationSet } = tiptap📝 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
| this.textarea.style.display = 'none' | |
| this.adjustShortcutsForPlatform() | |
| const { Editor, Extension, Bold, Italic, Paragraph, Text, HardBreak, UndoRedo, Document, Link, Underline, Markdown, Plugin, Decoration, DecorationSet } = await loadTiptap() | |
| async connectedCallback () { | |
| if (!this.textarea || !this.editorElement) return | |
| this.adjustShortcutsForPlatform() | |
| let tiptap | |
| try { | |
| tiptap = await loadTiptap() | |
| } catch (_error) { | |
| return | |
| } | |
| if (!this.isConnected) return | |
| this.textarea.style.display = 'none' | |
| const { Editor, Extension, Bold, Italic, Paragraph, Text, HardBreak, UndoRedo, Document, Link, Underline, Markdown, Plugin, Decoration, DecorationSet } = tiptap |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/elements/markdown_editor.js` around lines 149 - 153, The
fallback textarea is hidden before the async loadTiptap() completes, leaving the
form without an input if the import fails or the element disconnects; change the
flow in the initialization that calls this.adjustShortcutsForPlatform() and
const { Editor, ... } = await loadTiptap() so you await and try/catch
loadTiptap() first, verify this.isConnected (or similar mount flag) after the
await, and only then set this.textarea.style.display = 'none' and proceed to
instantiate Editor/Extensions; on error (or if disconnected) keep the textarea
visible, log the failure, and abort editor creation to ensure the fallback
remains usable.
Comment on lines
+378
to
+383
| disconnectedCallback () { | ||
| this.linkTooltip.hide() | ||
|
|
||
| if (this.editor) { | ||
| this.editor.destroy() | ||
| } |
There was a problem hiding this comment.
Guard teardown when initialization is skipped or fails.
disconnectedCallback unconditionally accesses this.linkTooltip; this can throw when connectedCallback exited early.
🛠️ Minimal safe fix
disconnectedCallback () {
- this.linkTooltip.hide()
+ this.linkTooltip?.hide()
if (this.editor) {
this.editor.destroy()
+ this.editor = null
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/elements/markdown_editor.js` around lines 378 - 383,
disconnectedCallback currently assumes this.linkTooltip and this.editor exist
and can throw if initialization in connectedCallback was skipped or failed;
guard accesses by checking existence before calling hide() or destroy(): in
disconnectedCallback, only call this.linkTooltip.hide() if this.linkTooltip is
truthy (and optionally has hide method) and only call this.editor.destroy() if
this.editor is truthy (and optionally has destroy method), so teardown is no-op
when initialization didn't complete.
Comment on lines
16
to
18
| if (this.dataset.className === 'hidden' && this.target.tagName === 'INPUT') { | ||
| this.target.disabled = event.target.value !== this.dataset.value | ||
| } |
There was a problem hiding this comment.
Inconsistent use of event.target.value instead of computed value.
Line 17 uses event.target.value directly, while the rest of the refactored code uses the computed value variable (Line 6). For checkboxes, event.target.value returns the HTML value attribute (typically "on"), not the checked state, causing this condition to behave incorrectly.
🐛 Proposed fix
if (this.dataset.className === 'hidden' && this.target.tagName === 'INPUT') {
- this.target.disabled = event.target.value !== this.dataset.value
+ this.target.disabled = value !== dataValue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/elements/toggle_attribute.js` around lines 16 - 18, The
conditional uses event.target.value directly which is incorrect for checkboxes;
update the branch inside the if (this.dataset.className === 'hidden' &&
this.target.tagName === 'INPUT') block to use the already-computed value
variable (from earlier in the function) instead of event.target.value so the
disabled toggle reflects the computed checked state; locate the logic in the
toggle handler in toggle_attribute.js (the function that computes value and then
sets this.target.disabled) and replace the event.target.value usage with value.
Comment on lines
+3
to
+15
| const button = this.querySelector('a, button, label') | ||
|
|
||
| const target = this.dataset.targetId ? document.getElementById(this.dataset.targetId) : button | ||
|
|
||
| button.addEventListener('click', () => { | ||
| this.dataset.classes.split(' ').forEach((cls) => { | ||
| button.classList.toggle(cls) | ||
| if (this.dataset.action === 'remove') { | ||
| target.classList.remove(cls) | ||
| } else if (this.dataset.action === 'add') { | ||
| target.classList.add(cls) | ||
| } else { | ||
| target.classList.toggle(cls) | ||
| } |
There was a problem hiding this comment.
Add null guards for trigger and resolved target.
Line 7 will throw if no matching trigger element is found, and Lines 10/12/14 will throw when data-target-id does not resolve to an element.
💡 Proposed defensive fix
export default class extends HTMLElement {
connectedCallback () {
const button = this.querySelector('a, button, label')
+ if (!button || !this.dataset.classes) return
- const target = this.dataset.targetId ? document.getElementById(this.dataset.targetId) : button
+ const target = this.dataset.targetId ? document.getElementById(this.dataset.targetId) : button
+ if (!target) return
button.addEventListener('click', () => {
this.dataset.classes.split(' ').forEach((cls) => {
if (this.dataset.action === 'remove') {
target.classList.remove(cls)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/elements/toggle_classes.js` around lines 3 - 15, The click
handler assumes a valid trigger and target; add null guards so it doesn't throw:
verify that the computed trigger element (button) exists before calling
button.addEventListener and bail early if missing, ensure this.dataset.classes
is present and non-empty before splitting, and check that the resolved target
(the variable target from this.dataset.targetId or fallback to button) is
non-null before attempting target.classList.remove/add/toggle; update the logic
around the variables "button" and "target" in toggle_classes.js to perform these
checks and return early or skip class operations when they are absent.
Comment on lines
+169
to
+171
| isNumberField (field) { | ||
| return field.type === 'number' || (['radio', 'select'].includes(field.type) && field.options?.every((o) => String(o.value).match(/^[\d.-]+$/))) | ||
| }, |
There was a problem hiding this comment.
isNumberField accepts invalid numeric strings.
On Line 170, /^[\d.-]+$/ matches values like --, 1-2, and 1.2.3, so non-numeric options can be treated as numeric and leak into formulas.
Proposed fix
isNumberField (field) {
- return field.type === 'number' || (['radio', 'select'].includes(field.type) && field.options?.every((o) => String(o.value).match(/^[\d.-]+$/)))
+ const isNumericValue = (value) => {
+ if (value === null || value === undefined) return false
+ const text = String(value).trim()
+ if (text === '') return false
+ return Number.isFinite(Number(text))
+ }
+
+ return field.type === 'number' || (
+ ['radio', 'select'].includes(field.type) &&
+ Array.isArray(field.options) &&
+ field.options.length > 0 &&
+ field.options.every((o) => isNumericValue(o.value))
+ )
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/template_builder/formula_modal.vue` around lines 169 - 171,
isNumberField currently treats strings like "--" or "1.2.3" as numeric because
the regex `/^[\d.-]+$/` is too permissive; update the validation inside
isNumberField (referencing field.type, field.options, and option value o.value)
to use a stricter numeric check — either replace the regex with one that allows
an optional sign and at most one decimal point (e.g.
/^[+-]?(?:\d+(\.\d+)?|\.\d+)$/) or validate via Number parsing combined with a
string-match to ensure no extra characters (e.g. Number.isFinite(Number(val))
plus confirming the original string matches the canonical numeric form). Ensure
the change is applied where options?.every((o) => ...) is used so only truly
numeric option values are treated as numbers.
Comment on lines
+327
to
336
| time_format = with_timestamp_seconds ? :detailed : :long | ||
|
|
||
| if with_signature_id_reason || field.dig('preferences', 'reasons').present? | ||
| "#{"#{I18n.t('reason')}: " if reason_value}#{reason_value || I18n.t('digitally_signed_by')} " \ | ||
| "#{submitter.name}#{" <#{submitter.email}>" if submitter.email.present?}\n" \ | ||
| "#{I18n.l(attachment.created_at.in_time_zone(timezone), format: :long)} " \ | ||
| "#{I18n.l(attachment.created_at.in_time_zone(timezone), format: time_format)} " \ | ||
| "#{TimeUtils.timezone_abbr(timezone, attachment.created_at)}" | ||
| else | ||
| "#{I18n.l(attachment.created_at.in_time_zone(timezone), format: :long)} " \ | ||
| "#{I18n.l(attachment.created_at.in_time_zone(timezone), format: time_format)} " \ | ||
| "#{TimeUtils.timezone_abbr(timezone, attachment.created_at)}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 -m pip install --quiet pyyaml 2>/dev/null || python -m pip install --quiet pyyaml
python3 - <<'PY'
import glob
import yaml
from collections import defaultdict
def deep_get(d, path):
cur = d
for key in path:
if not isinstance(cur, dict) or key not in cur:
return None
cur = cur[key]
return cur
seen_locales = set()
has_detailed = defaultdict(list)
for path in glob.glob('config/locales/**/*.y*ml', recursive=True):
with open(path, 'r', encoding='utf-8') as f:
data = yaml.safe_load(f) or {}
for locale, body in data.items():
seen_locales.add(locale)
if deep_get(body, ['time', 'formats', 'detailed']) is not None:
has_detailed[locale].append(path)
missing = sorted(locale for locale in seen_locales if locale not in has_detailed)
print("Locales found:", sorted(seen_locales))
print("Locales with time.formats.detailed:", sorted(has_detailed))
print("Locales missing time.formats.detailed:", missing)
if missing:
print("\nMISSING DETAILED FORMAT - Would cause runtime error")
exit(1)
else:
print("\nAll locales have time.formats.detailed - Safe")
exit(0)
PYRepository: kencove/docuseal
Length of output: 517
Add time.formats.detailed locale key to 7 missing locales.
The :detailed timestamp format is missing from locales: ar, cs, he, ja, ko, pl, uk. When with_timestamp_seconds is true and a user is in one of these locales, I18n.l will raise a missing translation error during PDF generation, breaking attachment generation.
Add the detailed format to config/locales/*/time.yml for each missing locale, or use a fallback format key that exists across all locales.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/submissions/generate_result_attachments.rb` around lines 327 - 336, The
code sets time_format = with_timestamp_seconds ? :detailed : :long and then
calls I18n.l(..., format: time_format), which will raise missing translation
errors for locales that lack time.formats.detailed; update the affected locale
files to add a time.formats.detailed entry for ar, cs, he, ja, ko, pl, uk
(matching the desired detailed format), or change the code to use an existing
common key (e.g., :long) as a safe fallback when
I18n.exists?("time.formats.#{time_format}", locale) is false before calling
I18n.l; modify the logic around with_timestamp_seconds/time_format or perform an
existence check so I18n.l never requests a missing :detailed format.
Comment on lines
+36
to
+39
| if (token = request.params[:two_factor_token].presence || request.headers['x-two-factor-token'].presence) | ||
| link_2fa_key = [submitter.email.downcase.squish, submitter.submission.template.slug].join(':') | ||
|
|
||
| return true if Submitter.signed_id_verifier.verified(token, purpose: :email_two_factor) == link_2fa_key |
There was a problem hiding this comment.
Handle blank submitter email before building link_2fa_key.
Line 37 can raise when submitter.email is nil (e.g., phone-only submitters). Unauthorized access should return false, not 500.
🔧 Proposed fix
if (token = request.params[:two_factor_token].presence || request.headers['x-two-factor-token'].presence)
- link_2fa_key = [submitter.email.downcase.squish, submitter.submission.template.slug].join(':')
+ email = submitter.email.to_s.downcase.squish
+ template_slug = submitter.submission.template&.slug.to_s
+ return false if email.blank? || template_slug.blank?
+
+ link_2fa_key = [email, template_slug].join(':')
return true if Submitter.signed_id_verifier.verified(token, purpose: :email_two_factor) == link_2fa_key
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/submitters/authorized_for_form.rb` around lines 36 - 39, The code builds
link_2fa_key using submitter.email without guarding for nil, causing a 500 for
phone-only submitters; update the two-factor check in authorized_for_form (the
block that reads request.params[:two_factor_token] and builds link_2fa_key) to
first return false if submitter.email.blank? (or otherwise handle nil) before
constructing link_2fa_key and verifying with
Submitter.signed_id_verifier.verified(token, purpose: :email_two_factor) so
missing emails produce a safe false result instead of raising.
| RequiredFieldError = Class.new(StandardError) | ||
|
|
||
| VARIABLE_REGEXP = /\{\{?(\w+)\}\}?/ | ||
| PHONE_REGEXP = /[+\d()\s-]+/ |
There was a problem hiding this comment.
Tighten phone matching to avoid misclassifying arbitrary text as a phone number.
Line 9 is unanchored, so Line 438 can match substrings (e.g., abc123) and treat invalid input as a phone invite target.
🔧 Proposed fix
- PHONE_REGEXP = /[+\d()\s-]+/
+ PHONE_REGEXP = /\A(?=.*\d)[+\d()\s-]+\z/Also applies to: 438-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/submitters/submit_values.rb` at line 9, The PHONE_REGEXP is too
permissive and unanchored (PHONE_REGEXP), causing substrings to match; change it
to an anchored pattern that enforces a minimum number of digits and full-string
matching (e.g. use \A and \z and a digit-count requirement such as a lookahead
like (?=(?:.*\d){7,}) or require at least 7 digit characters) and include only
allowed separators, then replace the current /[+\d()\s-]+/ with that anchored,
stricter regex so phone validation only matches entire valid phone strings
wherever PHONE_REGEXP is used.
Comment on lines
+420
to
+445
| submission.template_submitters.each do |s| | ||
| field_uuid = s['invite_via_field_uuid'] | ||
|
|
||
| next if field_uuid.blank? | ||
|
|
||
| field = submission.template_fields.find { |e| e['uuid'] == field_uuid } | ||
|
|
||
| next unless field | ||
| next unless field['submitter_uuid'] == submitter.uuid | ||
|
|
||
| next if submission.submitters.exists?(uuid: s['uuid']) | ||
|
|
||
| value = submitter.values[field_uuid] | ||
|
|
||
| next if value.blank? | ||
|
|
||
| if value.include?('@') | ||
| email = Submissions.normalize_email(value) | ||
| elsif value.match?(PHONE_REGEXP) | ||
| phone = value.gsub(/[^+\d]/, '') | ||
| end | ||
|
|
||
| next if email.blank? && phone.blank? | ||
|
|
||
| submission.submitters.create!(uuid: s['uuid'], email:, phone:, account_id: submitter.account_id) | ||
|
|
There was a problem hiding this comment.
Reset email/phone for each invite candidate to prevent cross-recipient leakage.
Line 444 can reuse email/phone from a previous loop iteration because those locals are not reinitialized inside the loop. That can create invites for the wrong contact.
🔒 Proposed fix
submission.template_submitters.each do |s|
+ email = nil
+ phone = nil
field_uuid = s['invite_via_field_uuid']
@@
- if value.include?('@')
+ if value.include?('@')
email = Submissions.normalize_email(value)
elsif value.match?(PHONE_REGEXP)
phone = value.gsub(/[^+\d]/, '')
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/submitters/submit_values.rb` around lines 420 - 445, The locals email and
phone can leak values between iterations of the submission.template_submitters
loop; reinitialize email and phone to nil at the top of the loop (just after
obtaining field_uuid or before checking value) so each invite candidate starts
with a fresh contact; ensure the existing logic using PHONE_REGEXP,
Submissions.normalize_email, and submission.submitters.create! uses these
reinitialized variables to avoid cross-recipient leakage.
Comment on lines
+456
to
+458
| def validate_value!(_value, field, _params, submitter, _request) | ||
| if field['readonly'] == true | ||
| Rollbar.warning("Readonly field #{submitter.id}: #{field['uuid']}") if defined?(Rollbar) |
There was a problem hiding this comment.
Guard unknown field UUIDs before readonly access.
Line 457 assumes field exists. If a submitted UUID is not in template_fields, this will raise NoMethodError and return 500 instead of a controlled validation error.
🛡️ Proposed fix
def validate_value!(_value, field, _params, submitter, _request)
+ raise ValidationError, 'Unknown field' if field.blank?
+
if field['readonly'] == true
Rollbar.warning("Readonly field #{submitter.id}: #{field['uuid']}") if defined?(Rollbar)📝 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
| def validate_value!(_value, field, _params, submitter, _request) | |
| if field['readonly'] == true | |
| Rollbar.warning("Readonly field #{submitter.id}: #{field['uuid']}") if defined?(Rollbar) | |
| def validate_value!(_value, field, _params, submitter, _request) | |
| raise ValidationError, 'Unknown field' if field.blank? | |
| if field['readonly'] == true | |
| Rollbar.warning("Readonly field #{submitter.id}: #{field['uuid']}") if defined?(Rollbar) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/submitters/submit_values.rb` around lines 456 - 458, In validate_value!
ensure you guard against a nil/unknown field before accessing field['readonly']
and field['uuid']; if field is nil (meaning the submitted UUID wasn't found in
template_fields) raise or return the controlled validation error instead of
touching field['readonly'], and keep the Rollbar.warning call only after
confirming field is present (reference validate_value!, field['readonly'], and
field['uuid'] to locate the checks).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if (this.editor) { | ||
| this.editor.destroy() | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing null guard crashes disconnectedCallback on early teardown
Medium Severity
disconnectedCallback calls this.linkTooltip.hide() without a null check, but this.linkTooltip is only assigned at the very end of the async connectedCallback (after await loadTiptap()). If the element is removed from the DOM before the async loading completes, this.linkTooltip is undefined and .hide() throws a TypeError. The this.editor check on the next line correctly uses a guard (if (this.editor)), but this.linkTooltip lacks the same protection.
Additional Locations (1)
| <% url_params = Rails.application.routes.recognize_path(params[:modal], method: :get) %> | ||
| <% if url_params[:action] == 'new' %> | ||
| <open-modal src="<%= params[:modal] %>"></open-modal> | ||
| <open-modal src="<%= url_for(url_params) %>"></open-modal> |
There was a problem hiding this comment.
Unhandled recognize_path error crashes page on invalid modal param
Medium Severity
Rails.application.routes.recognize_path(params[:modal]) raises ActionController::RoutingError if params[:modal] contains an unrecognized path. Since this runs in the application layout during view rendering with no rescue, the entire page returns a 404 instead of rendering normally. The previous code used params[:modal] directly as the src attribute, which would fail gracefully at the AJAX level without breaking the page.
…llback tests - Add test for Faraday::ConnectionFailed retry in SMS job spec (the job rescues this error but only Faraday::TimeoutError was previously tested) - Add STORE describe block in rate_limit_spec to verify MemoryStore fallback when REDIS_URL is absent, rescue behavior on Redis errors, and that the store supports increment for rate limiting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
Addressed the SMS-specific review comments:
The remaining comments about upstream code ( |
Use do...end for multi-line expect blocks and modifier if for single-line conditionals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Test plan
🤖 Generated with Claude Code
Note
Medium Risk
Touches form access control and download link generation, plus email/SMS delivery and rendering, so regressions could impact security-sensitive flows and outbound communications. Risk is moderated by mostly additive/guard-rail changes but spans many entrypoints (API, form UI, mailers, jobs).
Overview
Hardens submitter form/download flows and expands invite automation. Form and download endpoints now gate access through
Submitters::AuthorizedForForm(including link/email 2FA paths), adjust who counts as the current submitter via Ability checks, and triggerSubmitters::SubmitValues.maybe_invite_via_fieldwhen API-driven completion occurs.Changes attachment URL handling and template recipient rules. Multiple controllers/views switch from
ActiveStorage::Blob.proxy_urlto a newproxy_pathhelper, and template submitters gaininvite_via_field_uuidsupport (permitted params + UI), affecting which recipients are considered “undefined” and eligible for “sign yourself”.Improves messaging tooling and delivery robustness. Adds a new Tiptap-based
markdown-editorfor configurable email bodies (with variable insertion), updates mailer links to respect per-account custom domains, adds clearer canvas-blocked signature errors, and upgradesSendSubmitterInvitationSmsJobto no Sidekiq retries with explicit exponential backoff + Twilio SID capture.Misc hardening/maintenance. Enforces HTTPS webhooks on default port, validates URL downloads when uploading templates, redacts/placeholder-seeds secret fields in settings forms, updates favicon/manifest, and bumps/cleans dependencies (including removing
rails_autolink).Written by Cursor Bugbot for commit a148f47. Configure here.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Security