Conversation
Adds Playwright E2E activation smoke tests using the shared @gravitykit/e2e-bootstrap package. Refactors CircleCI config from a single build job into prepare + e2e + build stages.
…ict mode violation WordPress adds a second <tr data-plugin="..."> for the update notification row. The broad [data-plugin*="gfexcel.php"] selector matched both rows, causing a Playwright strict mode violation. Targeting .deactivate a within the plugin row resolves to a single element regardless of pending updates.
Add E2E smoke tests
The `wp-env:cli` script runs lifecycle steps on the tests-cli container (wpTestsPort, 8801), but `@gravitykit/e2e-bootstrap`'s default baseURL is wpPort (8800). Global setup cleanup, login, and tests were hitting the empty dev instance and getting HTML login pages where JSON was expected, producing `SyntaxError: Unexpected token '<', "<!DOCTYPE "...`. Override `use.baseURL` and `webServer.url` with the tests port so cleanup, login, and page navigations resolve against the env where plugins and REST routes actually live.
11 new specs covering the Lite-side behavior of GravityExport: - P0 download URL: enable, .xlsx default, .csv extension swap, regenerate/disable - P1 admin config: disable field, sort order - P2 access control: logged-in-only restriction - P3 data shaping: transpose mode, entry notes column - P5 notifications: attach single entry to a notification (CSV and XLSX) Supporting infrastructure: - tests/E2E/helpers/test-helpers.js wraps @gravitykit/e2e-bootstrap, fixes the fixtures baseURL inside Playwright workers (api.initFromEnv needs both WP_ENV_URL and WP_ENV_PORT and only the latter is in the env), and adds Lite-specific helpers for form-settings submission, feed-meta patching, notification seeding, entry submission, mail-capture access, attachment reads, and CSV parsing. - tests/E2E/setup/mu-plugins/e2e-mail-capture.php short-circuits wp_mail, copies attachments to uploads/e2e-mail-capture/attachments so they survive gform_after_email cleanup, and exposes GET/DELETE /wp-json/gk-e2e/v1/mail. - tests/E2E/setup/wp-env.config.js maps the mu-plugin into the tests container via additionalMappings. - tests/E2E/setup/playwright.config.js pins workers: 1 because several specs mutate global state (rewrite rules, gf_addon_feed, shared mail inbox) and the bootstrap default '50%' caused intermittent contention. The discovery doc (.claude/gravityexport-lite-happy-paths.md) records the full Phase 1 list, what shipped, what was dropped and why (custom labels, multi-form report, URL search filters), and the gotchas resolved along the way (form data-js page-loader stripping submit button name/value, anonymous request isolation, atomic-write breaking the wp-env bind mount). Full suite runs serially in roughly 90s and was verified green three times in a row.
Playwright writes its compiled-spec cache to playwright-transform-cache-<uid> at the project root. The existing *cache rule only catches names that end in "cache", so the suffixed dir was being reported as untracked after every local run.
The attach-csv and attach-xlsx specs intermittently failed with "apiRequestContext.get: socket hang up" on the /wp-json/gk-e2e/v1/mail endpoint. Apache in the wp-env container has a 5s KeepAlive idle timeout; Playwright's APIRequestContext reuses HTTP connections across specs, so the next reader can land on a socket the server has already closed. - Replace Playwright APIRequestContext usage with Node's global fetch in getCapturedEmails / clearCapturedEmails. The helper retries on transient connection-level errors (socket hang up, ECONNRESET, EPIPE, fetch failed) with exponential backoff. - Add waitForCapturedEmail(predicate) that polls until the matching message appears, absorbing both connection blips and any sub-second race between the wp-cli notification send and the file landing on disk. Notification specs now use this instead of a single GET. - Have the mu-plugin DELETE also wipe captured attachments so the uploads dir doesn't grow unbounded across long-running suites. - Drop the workers: 1 cap from playwright.config.js. The original cap was a workaround for what is actually the same connection-level flakiness — with retries in place, the bootstrap default (50%, i.e. 5 workers on a 10-core box) runs clean. Verified: full suite 3× back-to-back at the bootstrap default (5 workers), full suite at workers=2 and workers=4, and attach-csv x10 via --repeat-each — all green.
…aits
CI failed both notification specs with "waitForCapturedEmail: no message
matched predicate within 10000ms. Last seen subjects: []". The endpoint
was healthy and returning valid JSON, but the inbox was always empty —
pre_wp_mail was firing yet the JSON record never landed on disk.
Root cause: wp-content/uploads/ is created by Apache (www-data, UID 33)
inside the tests-wordpress container. wp-env's tests-cli runs commands
as the host user (UID 501 locally, UID 3434 on CircleCI), so on CI the
host user cannot write into a www-data-owned uploads dir, and
file_put_contents fails silently inside the pre_wp_mail handler.
- Mu-plugin now writes to WP_CONTENT_DIR/e2e-mail-capture/ instead.
- wp-env.config.js adds an additionalMappings bind-mount from
tests/E2E/setup/mail-capture/ on the host (created mode 0777 before
wp-env starts) to wp-content/e2e-mail-capture/ in the containers.
Both containers see the same dir, host-owned, world-writable; the
cli writes and Apache reads cleanly regardless of host UID.
- gitignore the new host capture dir and a stray playwright chromium
profile dir.
While verifying the fix locally a separate intermittent surfaced in
goToFormSettings / submitSettingsForm — the title-only and
waitForLoadState('load') waits could both resolve against the wrong
page state:
- goToFormSettings now waits for form#gform-settings to be visible, not
just the page title (Gravity Forms renders the title before the
panel body in some interleavings).
- submitSettingsForm now waits for the actual POST to the
subview=gravityexport-lite URL rather than a load event that may
belong to the previous page, then waits for domcontentloaded on the
response.
CI reproduced two notification spec failures with "waitForCapturedEmail: no message matched predicate within 10000ms. Last seen subjects: []". SSH'd into the CI container and traced: - The bind-mounted capture dir works (cli writes, apache reads). - pre_wp_mail has exactly one callback registered (our closure). - send_notifications returns ["debug_notif"] but our pre_wp_mail filter never fires, and only gform_disable_notification (not gform_notification) fires inside GF. Root cause: GFAPI::send_notifications routes through the GF_Notifications background processor (Async\GF_Background_Process service provider). The processor pushes the work to a queue and dispatches on PHP shutdown via WP cron — in a one-shot wp-cli context that shutdown handler never delivers a worker request, so wp_mail itself is never called. Bypass the async path by calling GFCommon::send_notifications directly with the explicit list of notification ids matching the event. That's the synchronous code path GFAPI falls back to when async is disabled, and the only one that invokes wp_mail from within our wp eval process. Verified on the failing CI host: attach-csv and attach-xlsx pass in isolation; full 14-spec suite passes in 14s.
Six valid findings:
- helpers: findTestsContainer was passing { value: cachedWpContainer }
as a fresh object each call, so writes inside findContainer never
mutated the module-scoped cache and every call shelled out to
'docker ps'. Replaced both helpers with a single keyed-by-filter
containerCache map.
- mail capture mu-plugin: only append an attachment record when @copy
actually succeeds, otherwise we end up with records pointing at
files tests will fail to read.
- mail capture mu-plugin: write the JSON capture atomically (write to
.tmp then rename) so a parallel GET /mail can't observe a partially
flushed file.
- playwright.config: build testsBaseURL via URL() so a user-provided
WP_ENV_URL that already includes a port no longer produces invalid
results like http://localhost:8888:8801.
- regenerate-disable spec: removed the duplicate visibility assertion
after the disable click — one toBeVisible + one toHaveCount(0) is
enough.
- happy-paths doc: removed the stale workers: 1 references; the cap
was lifted in a prior commit once the connection-pool retry layer
was in.
One finding skipped:
- submitSettingsForm waitForLoadState('load') race — already fixed in
a prior commit; current code uses waitForRequest + waitForLoadState
('domcontentloaded').
The Run E2E tests step has a 3-attempt retry around npm run tests:e2e:setup.
Each retry runs 'rm -rf /home/circleci/.wp-env' to drop wp-env state before
trying again. That cleanup runs as the circleci user, but the bind-mounted
mu-plugin files inside the state dir
(.wp-env/<hash>/wp-content/mu-plugins/{e2e-fixtures,e2e-mail-capture}.php)
are created by Apache inside the container as uid 0, so the rm fails with
"Permission denied". Once that happens the retry can't reset state and
every attempt fails the same way — which is what we hit on pipeline 324
(transient mysql-readiness flake on the first try, then nothing else can
recover).
Stop wp-env before the cleanup so containers release the bind-mount, then
sudo rm the state dir so we can remove root-owned files. sudo is available
on CircleCI Linux machine runners.
Verified on the failing CI host: plain rm reproduces the original
permission-denied error; the new sequence (wp-env stop, sudo rm -rf, fresh
setup) restores wp-env cleanly and the full 14-spec suite passes in ~16s.
Add E2E happy-path Playwright suite
Runs the @smoke-tagged activation suite against the packaged plugin zip in CI, after build_package_release and before publishing the release. - Tag activation.spec.js describe with @smoke so the bootstrap CLI picks it up via Playwright's --grep. - Bump @gravitykit/e2e-bootstrap to ^1.1.0 (smoke CLI subcommand). - Persist .release and the gf-entries-in-excel-*.zip glob from build_package_release so downstream jobs can install the built artifact. - Add run_post_build_smoke job: unzips the artifact, restores dev deps through gktools' Docker composer (PHP 7.4), boots wp-env, runs the smoke spec against the built plugin. - Extract Create GitHub release + Announce build into a publish_release job gated on smoke success; release publishing now only happens when the built artifact actually activates cleanly.
- Remove '2>/dev/null || true' from 'cp gf-entries-in-excel-*.zip .release/' so the step fails loudly when the build produced no zip, instead of swallowing the cp error and surfacing the problem later. - Drop the redundant 'working_directory: /home/circleci/plugin' from run_post_build_smoke. The default_job_config anchor already sets it; the duplicate was a copy-paste artifact from the prompt template.
Add post-build smoke E2E against built .zip artifact
gktools sign writes .gktools-signing.json (and process env vars) in the build job; gktools announce, now in the separate publish_release job, reads it to register the package signature with Release Manager. The env vars die with the process and the sidecar was never added to persist_to_workspace, so announce fell back to empty signing data and silently published unsigned releases. Same fix as GravityExport; add plugin/.gktools-signing.json to the persisted paths, mirroring the existing zip handoff.
announce signs the artifact inline as of gktools 1.1.0, so the separate sign step (and its .gktools-signing.json sidecar, where present) is dead code.
The DLC volume intermittently corrupts the wp-env image build (failed to get reader from content store), failing the smoke job. Matches GravityExport.
DownloadUrlResetAction and DownloadUrlEnableAction set a shared static $success_message via esc_html__() in their constructors. The service container resolves both actions during bootstrap, before after_setup_theme, so the translation tripped WordPress 6.7's just-in-time notice for the gk-gravityexport-lite domain. Return each message from a get_success_message() method called at fire time (when add_message() runs) instead. No translation runs before after_setup_theme, and the fragile shared static (both subclasses wrote the same property) is gone. Fixes GEXPLIT-17.
Regenerating, enabling, or disabling a download URL fires the action then PRG-redirects, which discarded the queued success message. Route each action's result through a typed ActionNotice (success/error/info) exposed by NotifyingActionInterface; a FiresWithNotice trait keeps the legacy void fire() working for feed duplication. save_feed_settings() branches on the outcome: success carries an allowlisted gexcel_notice token whose confirmation GF's postback callback paints on the GET; failure logs the cause and carries a gexcel_error token that surfaces a red notice through GF's own error queue. Both tokens are stripped after first paint with history.replaceState, so a refresh never re-shows the message and nothing is stored to survive the redirect. Fixes GEXPLIT-18.
The single-entry export could only ever be attached to one notification: the stored notification ID was compared directly against the notification being sent, so a form had no way to attach the file to a second one. The selection is now resolved as a list through two filters. The new gk/gravityexport/notification/attachment-source-ids establishes the selection and is where a product that owns it (GravityExport) supplies a multi-selection; gk/gravityexport/notification/attachment-ids then modifies that complete list, so a callback can add or remove IDs and its removals are honoured. Lite keeps its single select, and an existing stored ID is read exactly as before. Every render is now written to its own 0700 directory, chosen through the new gk/gravityexport/renderer/save-path filter, because the previous fixed path meant two simultaneous submissions could overwrite or delete each other's attachment. Generated files are removed after their email is sent, or at shutdown if that never happens. Ref GEXPORT-33.
WalkthroughGravityExport Lite 2.7.0 adds staged release automation, typed download URL notices, isolated notification attachments, updated localization, and Playwright E2E infrastructure and coverage. ChangesGravityExport Lite 2.7.0
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
languages/gk-gravityexport-lite-pl_PL.po (1)
590-592: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the extra spaces around
[shortcode].The translation renders the placeholder as
' [shortcode] '. Keep the placeholder adjacent to its quotes.Proposed fix
- msgstr "Proszę dodać poprawny atrybut 'secret' do shortcode ' [shortcode] '." + msgstr "Proszę dodać poprawny atrybut 'secret' do shortcode '[shortcode]'."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-pl_PL.po` around lines 590 - 592, Update the msgstr translation for the "Please add a valid 'secret' attribute to the '[shortcode]' shortcode." message to remove the spaces surrounding the [shortcode] placeholder, keeping it directly adjacent to its quotation marks.
🟡 Minor comments (16)
.claude/gravityexport-lite-happy-paths.md-189-189 (1)
189-189: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented spec count.
Eleven happy-path specs plus
activation.spec.jsequals twelve specs, not fourteen.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/gravityexport-lite-happy-paths.md at line 189, Correct the documented total spec count in the happy-path summary: eleven happy-path specs plus the existing activation.spec.js should be stated as twelve specs, while leaving the remaining execution details unchanged..claude/gravityexport-lite-happy-paths.md-4-4 (1)
4-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the target release version.
This PR releases version 2.7.0. The document still identifies the target as version 2.6.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/gravityexport-lite-happy-paths.md at line 4, Update the target release version in the document’s Target declaration from 2.6.0 to 2.7.0, while preserving the plugin name and other release details.tests/E2E/tests/download-url/download-xlsx.spec.js-27-35 (1)
27-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
requestfixture is not anonymous, so this test does not check anonymous access.The test name and the comment on Line 34 claim an anonymous GET. The
requestfixture inherits the bootstrap admin storage state;tests/E2E/tests/permissions/logged-in-required.spec.js(Lines 12-21, 62-63) documents this and adds a Nodefetchhelper for that reason. Use a plainfetchhere, or rename the test to reflect an authenticated request.🛠️ Proposed change
- test( 'anonymous GET on the download URL returns a valid .xlsx attachment', async ( { + test( 'GET on the download URL returns a valid .xlsx attachment', async ( { page, request, } ) => { await enableDownloadUrl( page, data.form_id ); const url = await readDownloadUrl( page ); - // Use a fresh request context so we are NOT carrying the admin cookies. const response = await fetchDownload( request, url );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/tests/download-url/download-xlsx.spec.js` around lines 27 - 35, Update the anonymous download test around fetchDownload to use a plain Node fetch without the authenticated request fixture, preserving the existing URL retrieval and XLSX attachment assertions. Keep the test name and anonymity comment accurate, and remove or bypass request from the test setup as needed.tests/E2E/helpers/test-helpers.js-18-20 (1)
18-20: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winResolve
WP_ENV_URLbefore appending the test port.If
WP_ENV_URLincludes a port, line 19 creates an invalid URL such ashttp://localhost:8888:8801. Reuse the URL-resolution logic fromtests/E2E/setup/playwright.config.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/helpers/test-helpers.js` around lines 18 - 20, Update the testsBaseURL construction to resolve WP_ENV_URL using the same URL-resolution logic as playwright.config.js before appending ports.wpTestsPort, ensuring any existing port is replaced or handled correctly rather than producing a duplicated-port URL. Keep base.api.setConfig configured with the resulting test URL.languages/gk-gravityexport-lite-bn_BD.po-489-495 (1)
489-495: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the remaining Bengali strings.
Seven user-facing entries remain in English:
Move %s, the migration message, the support-forum message, and four version labels. Add reviewed Bengali translations before release.Also applies to: 505-511, 531-549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-bn_BD.po` around lines 489 - 495, Translate the seven remaining English msgstr entries in the Bengali translation catalog, including “Move %s,” the migration-success message, the support-forum message, and the four version labels. Preserve each msgid, placeholders, and version values while replacing only the untranslated msgstr text with reviewed Bengali translations.languages/gk-gravityexport-lite-pt_PT.po-545-547 (1)
545-547: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the leading space in this renderer fragment.
The
msgidstarts with a space, but themsgstrdoes not. Add the space to preserve the rendered separator.Proposed fix
- msgstr "(esta versão é muito baixa, por favor atualize para pelo menos PHP 5.6)" + msgstr " (esta versão é muito baixa, por favor atualize para pelo menos PHP 5.6)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-pt_PT.po` around lines 545 - 547, Update the translation entry for msgid " (this version is too low, please update to at least PHP 5.6)" so its msgstr begins with the same leading space, preserving the renderer’s separator.languages/gk-gravityexport-lite-tr_TR.po-136-152 (1)
136-152: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse “oturum açmış kullanıcılar” for the logged-in permission label.
Replace
kayıtlı kullanıcılarin both translations. Remove the extra opening‘before the phrase. Keep‘Kayıtları Dışa Aktar’balanced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-tr_TR.po` around lines 136 - 152, Update the Turkish translations for the logged-in permission label and its description: replace “kayıtlı kullanıcılar” with “oturum açmış kullanıcılar,” remove the extra opening quote before the phrase, and keep the ‘Kayıtları Dışa Aktar’ quotation marks balanced.languages/gk-gravityexport-lite-ru_RU.po-136-152 (1)
136-152: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCorrect the Russian translation of “logged-in users”.
Replace
Входящие пользователиwithавторизованные пользователиin both translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-ru_RU.po` around lines 136 - 152, Update the Russian translations in the msgstr entries for the download-access description and “Logged-in users who have "Export Entries" access” so “Входящие пользователи” is replaced with “авторизованные пользователи,” preserving the surrounding wording.languages/gk-gravityexport-lite-ja.po-460-466 (1)
460-466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the remaining English messages in both catalogs.
Both catalogs leave
Move %sand the migration-success message untranslated. Translate the surrounding text and preserve%s.
languages/gk-gravityexport-lite-ja.po#L460-L466: provide Japanese translations.languages/gk-gravityexport-lite-ko_KR.po#L464-L470: provide Korean translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-ja.po` around lines 460 - 466, Translate the English msgstr values for “Move %s” and “The settings for %s 2.0 were migrated successfully.” into Japanese in languages/gk-gravityexport-lite-ja.po lines 460-466 and Korean in languages/gk-gravityexport-lite-ko_KR.po lines 464-470, preserving the %s placeholder in both messages.languages/gk-gravityexport-lite-fr_CA.po-577-580 (1)
577-580: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse plural French grammar for count-dependent field messages.
[count]can represent multiple fields, but both catalogs use singularchamp correspond.
languages/gk-gravityexport-lite-fr_CA.po#L577-L580: use"[count] champs correspondent à votre recherche."languages/gk-gravityexport-lite-fr_FR.po#L577-L580: use"[count] champs correspondent à votre recherche."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-fr_CA.po` around lines 577 - 580, Update the plural translation for the count-dependent field message in languages/gk-gravityexport-lite-fr_CA.po lines 577-580 and languages/gk-gravityexport-lite-fr_FR.po lines 577-580, changing “champ correspond” to the plural French form “champs correspondent” while preserving the [count] placeholder.languages/gk-gravityexport-lite-it_IT.po-381-384 (1)
381-384: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Italian translation.
Use
Scarica un'esportazioneinstead ofScarica un Esportazione.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-it_IT.po` around lines 381 - 384, Update the msgstr translation for “Download an Export” to use the corrected Italian text “Scarica un'esportazione”, including the apostrophe and lowercase noun.languages/gk-gravityexport-lite-he_IL.po-84-87 (1)
84-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate “hyphens” as hyphens, not slashes.
Replace
קו נטוי(“slash”) withמקפים(“hyphens”).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-he_IL.po` around lines 84 - 87, Update the Hebrew msgstr translation for the “Most non-alphanumeric characters...” message so “hyphens” is translated as “מקפים” instead of “קו נטוי”, while preserving the rest of the translation.languages/gk-gravityexport-lite-fi.po-203-205 (1)
203-205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a noun phrase for
Enabled Fields.
Ota kentät käyttöönis an imperative (“Enable fields”), but this string labels the enabled-fields list. Use a Finnish equivalent of “Enabled fields”, such asKäytössä olevat kentät.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-fi.po` around lines 203 - 205, Update the Finnish translation for msgid “Enabled Fields” in the translation entry to use a noun phrase meaning “Enabled fields,” such as “Käytössä olevat kentät,” instead of the imperative wording.languages/gk-gravityexport-lite-nb_NO.po-565-571 (1)
565-571: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
oppføringfor Gravity Forms entries.Replace
InnleggsdatoandInnleggs-IDwithOppføringsdatoandOppførings-IDto match the catalog's existing terminology.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-nb_NO.po` around lines 565 - 571, Update the Norwegian translations for “Entry Date” and “Entry ID” in the catalog, replacing “Innleggsdato” with “Oppføringsdato” and “Innleggs-ID” with “Oppførings-ID” while leaving the message IDs unchanged.languages/gk-gravityexport-lite-fr_CA.po-348-362 (1)
348-362: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse complete French labels for visible fields.
In both French catalogs, translate
Enable visibleasActiver les champs visiblesandDisable visibleasDésactiver les champs visibles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-fr_CA.po` around lines 348 - 362, Update the translations for “Enable visible” and “Disable visible” to use the complete French labels “Activer les champs visibles” and “Désactiver les champs visibles” in both languages/gk-gravityexport-lite-fr_CA.po (lines 348-362) and languages/gk-gravityexport-lite-fr_FR.po (lines 348-362); leave the other catalog entries unchanged.languages/gk-gravityexport-lite-hu_HU.po-120-126 (1)
120-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Hungarian definite article.
Use
az űrlapinstead ofa űrlap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@languages/gk-gravityexport-lite-hu_HU.po` around lines 120 - 126, Update the Hungarian translation in the msgstr for the secure shortcode description to use the definite article “az” before “űrlap,” changing “a űrlap” to “az űrlap” while preserving the rest of the translation.
🧹 Nitpick comments (5)
tests/E2E/tests/permissions/logged-in-required.spec.js (1)
18-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReuse the retrying fetch for
anonGet.
tests/E2E/helpers/test-helpers.js(Lines 226-287) documents that Apache's 5s KeepAlive timeout producesECONNRESETand "socket hang up" on reused sockets, and it wraps every mail request infetchWithRetry.anonGetcalls barefetchagainst the same host and has no retry, so it can flake. ExportfetchWithRetryfrom the helpers and use it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/tests/permissions/logged-in-required.spec.js` around lines 18 - 21, Update anonGet to use the shared fetchWithRetry helper instead of bare fetch, preserving its manual redirect handling and response conversion. Export fetchWithRetry from test-helpers.js so the logged-in-required test can import and reuse it.tests/E2E/tests/download-url/download-csv.spec.js (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected row count from the fixture.
The assertion hardcodes 5 rows. If the
simpletemplate changes its entry count, this test fails for an unrelated reason.data.entriesis already available in this spec pattern, so compute the count.♻️ Proposed change
- // simple.json has 4 entries — 1 header + 4 data rows. + // 1 header row + one row per seeded entry. expect( rows.length, - 'Row count should be header + 4 entries' - ).toBe( 5 ); + 'Row count should be header + seeded entries' + ).toBe( data.entries.length + 1 );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/tests/download-url/download-csv.spec.js` around lines 50 - 54, Update the row-count assertion in the download CSV test to derive the expected value from the available data.entries fixture, adding the header row instead of hardcoding 5. Preserve the existing assertion message and rows.length validation.tests/E2E/tests/notifications/attach-csv.spec.js (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMulti-notification attachment support has no E2E coverage. Both notification specs write a single id into
attachment_notification, so the new multiple-ID selection and duplicate-safe attachment merging insrc/Action/NotificationAttachmentAction.phpstay untested.
tests/E2E/tests/notifications/attach-csv.spec.js#L47-L50: add a case that creates two active notifications, sets both IDs inattachment_notification, and asserts that each captured email carries exactly one CSV attachment.tests/E2E/tests/notifications/attach-xlsx.spec.js#L45-L48: apply the same two-notification case for the XLSX format, including the isolated-temp-directory expectation that filenames do not collide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/tests/notifications/attach-csv.spec.js` around lines 47 - 50, Extend tests/E2E/tests/notifications/attach-csv.spec.js at lines 47-50 with a case creating two active notifications, assigning both IDs to attachment_notification, and asserting each captured email has exactly one CSV attachment. Apply the same change in tests/E2E/tests/notifications/attach-xlsx.spec.js at lines 45-48, also verifying the isolated temporary directory prevents filename collisions.tests/E2E/tests/data-shaping/entry-notes.spec.js (1)
44-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
GFAPI::add_notefor the fixture.GFFormsModelis also an internal class, so replacingRGFormsModelwithGFFormsModeldoes not use the supported public API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/E2E/tests/data-shaping/entry-notes.spec.js` around lines 44 - 49, Update the fixture call in the entry-notes setup from GFFormsModel::add_note to the supported public GFAPI::add_note API, preserving the existing entry ID, user ID, author, and note arguments.src/Renderer/AbstractPHPExcelRenderer.php (1)
111-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider handling an unwritable filtered path in the save branch.
The path is now supplied by a filter callback. If the returned directory does not exist or is not writable,
$objWriter->save($file)throws. The surroundingcatchcallshandleException(), which prints HTML and callsexit. During notification sending that terminates the request instead of skipping the attachment.Validate the directory before saving, or fall back to
$default_pathwhen the target directory is not writable.♻️ Proposed refactor
if (!is_string($file) || $file === '') { $file = $default_path; } + + if (!is_dir(dirname($file)) || !is_writable(dirname($file))) { + $file = $default_path; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Renderer/AbstractPHPExcelRenderer.php` around lines 111 - 121, Validate the directory represented by the filtered $file before calling $objWriter->save() in the renderer save branch; when it does not exist or is not writable, replace $file with $default_path and preserve the existing save flow. Use the nearby $file, $default_path, and $objWriter->save() symbols without changing unrelated exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.circleci/config.yml:
- Around line 25-47: Update the workspace persistence configuration to include
only the project directory rather than the entire /home/circleci tree,
preventing ~/.npmrc and GH_AUTH_TOKEN from being persisted. Recreate the npm
registry configuration in downstream jobs that require package access,
preserving the existing authentication setup without storing it in the
workspace.
- Around line 177-194: Update the test_and_release workflow so the release chain
depends on run_e2e_tests by adding it to build_package_release.requires (or
publish_release.requires). Preserve the existing job order and dependencies
while ensuring failed E2E tests block packaging and publishing.
In `@languages/gk-gravityexport-lite-ar.po`:
- Around line 120-126: Update the Arabic msgstr for the secure shortcode
description while preserving the literal <code>secret</code> attribute name
exactly; translate only the surrounding explanatory text and keep the existing
message meaning.</code>
In `@languages/gk-gravityexport-lite-es_ES.po`:
- Around line 120-125: Update the translation in the secure shortcode
description so the literal attribute inside the <code> tag remains “secret”
while translating only the surrounding Spanish prose. Preserve the existing
msgid and all other translated text.
In `@readme.txt`:
- Around line 259-270: Update the release metadata to version 2.7.0: in
readme.txt, change the Stable tag to 2.7.0; in translations.pot, change
Project-Id-Version to GravityExport Lite 2.7.0.
In `@src/Addon/GravityExportAddon.php`:
- Around line 210-234: Replace the unsupported set_postback_message_callback
call in feed_settings_init() with a Gravity Forms 2.5-compatible API for
displaying the sanitized gexcel_notice message. Preserve the existing behavior
of showing the notice only on the post-redirect GET while leaving Gravity Forms’
default POST success or validation message unchanged.
In `@tests/E2E/setup/mu-plugins/e2e-mail-capture.php`:
- Around line 161-176: Update the mail-capture flow around the REST callback to
associate each captured message and attachment with a unique test-run
identifier, then require that identifier when handling GET and DELETE
operations. Ensure cleanup only removes records belonging to the current run and
preserves other parallel runs’ files; do not use global mailbox deletion unless
all mailbox tests are serialized.
---
Outside diff comments:
In `@languages/gk-gravityexport-lite-pl_PL.po`:
- Around line 590-592: Update the msgstr translation for the "Please add a valid
'secret' attribute to the '[shortcode]' shortcode." message to remove the spaces
surrounding the [shortcode] placeholder, keeping it directly adjacent to its
quotation marks.
---
Minor comments:
In @.claude/gravityexport-lite-happy-paths.md:
- Line 189: Correct the documented total spec count in the happy-path summary:
eleven happy-path specs plus the existing activation.spec.js should be stated as
twelve specs, while leaving the remaining execution details unchanged.
- Line 4: Update the target release version in the document’s Target declaration
from 2.6.0 to 2.7.0, while preserving the plugin name and other release details.
In `@languages/gk-gravityexport-lite-bn_BD.po`:
- Around line 489-495: Translate the seven remaining English msgstr entries in
the Bengali translation catalog, including “Move %s,” the migration-success
message, the support-forum message, and the four version labels. Preserve each
msgid, placeholders, and version values while replacing only the untranslated
msgstr text with reviewed Bengali translations.
In `@languages/gk-gravityexport-lite-fi.po`:
- Around line 203-205: Update the Finnish translation for msgid “Enabled Fields”
in the translation entry to use a noun phrase meaning “Enabled fields,” such as
“Käytössä olevat kentät,” instead of the imperative wording.
In `@languages/gk-gravityexport-lite-fr_CA.po`:
- Around line 577-580: Update the plural translation for the count-dependent
field message in languages/gk-gravityexport-lite-fr_CA.po lines 577-580 and
languages/gk-gravityexport-lite-fr_FR.po lines 577-580, changing “champ
correspond” to the plural French form “champs correspondent” while preserving
the [count] placeholder.
- Around line 348-362: Update the translations for “Enable visible” and “Disable
visible” to use the complete French labels “Activer les champs visibles” and
“Désactiver les champs visibles” in both
languages/gk-gravityexport-lite-fr_CA.po (lines 348-362) and
languages/gk-gravityexport-lite-fr_FR.po (lines 348-362); leave the other
catalog entries unchanged.
In `@languages/gk-gravityexport-lite-he_IL.po`:
- Around line 84-87: Update the Hebrew msgstr translation for the “Most
non-alphanumeric characters...” message so “hyphens” is translated as “מקפים”
instead of “קו נטוי”, while preserving the rest of the translation.
In `@languages/gk-gravityexport-lite-hu_HU.po`:
- Around line 120-126: Update the Hungarian translation in the msgstr for the
secure shortcode description to use the definite article “az” before “űrlap,”
changing “a űrlap” to “az űrlap” while preserving the rest of the translation.
In `@languages/gk-gravityexport-lite-it_IT.po`:
- Around line 381-384: Update the msgstr translation for “Download an Export” to
use the corrected Italian text “Scarica un'esportazione”, including the
apostrophe and lowercase noun.
In `@languages/gk-gravityexport-lite-ja.po`:
- Around line 460-466: Translate the English msgstr values for “Move %s” and
“The settings for %s 2.0 were migrated successfully.” into Japanese in
languages/gk-gravityexport-lite-ja.po lines 460-466 and Korean in
languages/gk-gravityexport-lite-ko_KR.po lines 464-470, preserving the %s
placeholder in both messages.
In `@languages/gk-gravityexport-lite-nb_NO.po`:
- Around line 565-571: Update the Norwegian translations for “Entry Date” and
“Entry ID” in the catalog, replacing “Innleggsdato” with “Oppføringsdato” and
“Innleggs-ID” with “Oppførings-ID” while leaving the message IDs unchanged.
In `@languages/gk-gravityexport-lite-pt_PT.po`:
- Around line 545-547: Update the translation entry for msgid " (this version is
too low, please update to at least PHP 5.6)" so its msgstr begins with the same
leading space, preserving the renderer’s separator.
In `@languages/gk-gravityexport-lite-ru_RU.po`:
- Around line 136-152: Update the Russian translations in the msgstr entries for
the download-access description and “Logged-in users who have "Export Entries"
access” so “Входящие пользователи” is replaced with “авторизованные
пользователи,” preserving the surrounding wording.
In `@languages/gk-gravityexport-lite-tr_TR.po`:
- Around line 136-152: Update the Turkish translations for the logged-in
permission label and its description: replace “kayıtlı kullanıcılar” with
“oturum açmış kullanıcılar,” remove the extra opening quote before the phrase,
and keep the ‘Kayıtları Dışa Aktar’ quotation marks balanced.
In `@tests/E2E/helpers/test-helpers.js`:
- Around line 18-20: Update the testsBaseURL construction to resolve WP_ENV_URL
using the same URL-resolution logic as playwright.config.js before appending
ports.wpTestsPort, ensuring any existing port is replaced or handled correctly
rather than producing a duplicated-port URL. Keep base.api.setConfig configured
with the resulting test URL.
In `@tests/E2E/tests/download-url/download-xlsx.spec.js`:
- Around line 27-35: Update the anonymous download test around fetchDownload to
use a plain Node fetch without the authenticated request fixture, preserving the
existing URL retrieval and XLSX attachment assertions. Keep the test name and
anonymity comment accurate, and remove or bypass request from the test setup as
needed.
---
Nitpick comments:
In `@src/Renderer/AbstractPHPExcelRenderer.php`:
- Around line 111-121: Validate the directory represented by the filtered $file
before calling $objWriter->save() in the renderer save branch; when it does not
exist or is not writable, replace $file with $default_path and preserve the
existing save flow. Use the nearby $file, $default_path, and $objWriter->save()
symbols without changing unrelated exception handling.
In `@tests/E2E/tests/data-shaping/entry-notes.spec.js`:
- Around line 44-49: Update the fixture call in the entry-notes setup from
GFFormsModel::add_note to the supported public GFAPI::add_note API, preserving
the existing entry ID, user ID, author, and note arguments.
In `@tests/E2E/tests/download-url/download-csv.spec.js`:
- Around line 50-54: Update the row-count assertion in the download CSV test to
derive the expected value from the available data.entries fixture, adding the
header row instead of hardcoding 5. Preserve the existing assertion message and
rows.length validation.
In `@tests/E2E/tests/notifications/attach-csv.spec.js`:
- Around line 47-50: Extend tests/E2E/tests/notifications/attach-csv.spec.js at
lines 47-50 with a case creating two active notifications, assigning both IDs to
attachment_notification, and asserting each captured email has exactly one CSV
attachment. Apply the same change in
tests/E2E/tests/notifications/attach-xlsx.spec.js at lines 45-48, also verifying
the isolated temporary directory prevents filename collisions.
In `@tests/E2E/tests/permissions/logged-in-required.spec.js`:
- Around line 18-21: Update anonGet to use the shared fetchWithRetry helper
instead of bare fetch, preserving its manual redirect handling and response
conversion. Export fetchWithRetry from test-helpers.js so the logged-in-required
test can import and reuse it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b89d346-fa4a-4731-b0f8-9259d1a1b664
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (96)
.circleci/config.yml.claude/gravityexport-lite-happy-paths.md.env.sample.gitattributes.gitignore.gktools.jsonGruntfile.jsgfexcel.phplanguages/gk-gravityexport-lite-ar.molanguages/gk-gravityexport-lite-ar.polanguages/gk-gravityexport-lite-bn_BD.molanguages/gk-gravityexport-lite-bn_BD.polanguages/gk-gravityexport-lite-bs_BA.molanguages/gk-gravityexport-lite-bs_BA.polanguages/gk-gravityexport-lite-da_DK.molanguages/gk-gravityexport-lite-da_DK.polanguages/gk-gravityexport-lite-de_DE.molanguages/gk-gravityexport-lite-de_DE.polanguages/gk-gravityexport-lite-es_AR.molanguages/gk-gravityexport-lite-es_AR.polanguages/gk-gravityexport-lite-es_ES.molanguages/gk-gravityexport-lite-es_ES.polanguages/gk-gravityexport-lite-es_MX.molanguages/gk-gravityexport-lite-es_MX.polanguages/gk-gravityexport-lite-fa_IR.molanguages/gk-gravityexport-lite-fa_IR.polanguages/gk-gravityexport-lite-fi.molanguages/gk-gravityexport-lite-fi.polanguages/gk-gravityexport-lite-fr_CA.molanguages/gk-gravityexport-lite-fr_CA.polanguages/gk-gravityexport-lite-fr_FR.molanguages/gk-gravityexport-lite-fr_FR.polanguages/gk-gravityexport-lite-he_IL.molanguages/gk-gravityexport-lite-he_IL.polanguages/gk-gravityexport-lite-hu_HU.molanguages/gk-gravityexport-lite-hu_HU.polanguages/gk-gravityexport-lite-it_IT.molanguages/gk-gravityexport-lite-it_IT.polanguages/gk-gravityexport-lite-ja.molanguages/gk-gravityexport-lite-ja.polanguages/gk-gravityexport-lite-ko_KR.molanguages/gk-gravityexport-lite-ko_KR.polanguages/gk-gravityexport-lite-nb_NO.molanguages/gk-gravityexport-lite-nb_NO.polanguages/gk-gravityexport-lite-nl_NL.molanguages/gk-gravityexport-lite-nl_NL.polanguages/gk-gravityexport-lite-pl_PL.molanguages/gk-gravityexport-lite-pl_PL.polanguages/gk-gravityexport-lite-pt_BR.molanguages/gk-gravityexport-lite-pt_BR.polanguages/gk-gravityexport-lite-pt_PT.molanguages/gk-gravityexport-lite-pt_PT.polanguages/gk-gravityexport-lite-ro_RO.molanguages/gk-gravityexport-lite-ro_RO.polanguages/gk-gravityexport-lite-ru_RU.molanguages/gk-gravityexport-lite-ru_RU.polanguages/gk-gravityexport-lite-sv_SE.molanguages/gk-gravityexport-lite-sv_SE.polanguages/gk-gravityexport-lite-tr_TR.molanguages/gk-gravityexport-lite-tr_TR.polanguages/gk-gravityexport-lite-zh_CN.molanguages/gk-gravityexport-lite-zh_CN.popackage.jsonpublic/css/gravityexport-lite.csspublic/js/gravityexport-lite.jsreadme.txtsrc/Action/ActionNotice.phpsrc/Action/DownloadUrlDisableAction.phpsrc/Action/DownloadUrlEnableAction.phpsrc/Action/DownloadUrlResetAction.phpsrc/Action/FiresWithNotice.phpsrc/Action/NotificationAttachmentAction.phpsrc/Action/NotifyingActionInterface.phpsrc/Addon/AddonHelperTrait.phpsrc/Addon/GravityExportAddon.phpsrc/Renderer/AbstractPHPExcelRenderer.phpsrc/Repository/FormsRepository.phptests/E2E/helpers/test-helpers.jstests/E2E/setup/mu-plugins/e2e-mail-capture.phptests/E2E/setup/playwright.config.jstests/E2E/setup/playwright.global.setup.jstests/E2E/setup/playwright.global.teardown.jstests/E2E/setup/wp-env.config.jstests/E2E/tests/activation.spec.jstests/E2E/tests/data-shaping/entry-notes.spec.jstests/E2E/tests/data-shaping/transpose.spec.jstests/E2E/tests/download-url/download-csv.spec.jstests/E2E/tests/download-url/download-xlsx.spec.jstests/E2E/tests/download-url/enable-download-url.spec.jstests/E2E/tests/download-url/regenerate-disable.spec.jstests/E2E/tests/fields/disable-field.spec.jstests/E2E/tests/fields/sort-order.spec.jstests/E2E/tests/notifications/attach-csv.spec.jstests/E2E/tests/notifications/attach-xlsx.spec.jstests/E2E/tests/permissions/logged-in-required.spec.jstranslations.pot
Gravity Forms documents GFAddOn::get_settings_renderer() as returning Gravity_Forms\Gravity_Forms\Settings, a class that does not exist (the real one is Settings\Settings), so the postback callback call is annotated with the correct type. The pre-save-settings hook's value is documented as array|mixed so its is_array() guard is not read as dead code.
WordPress.org serves whatever `Stable tag` points at, so it now matches the plugin header. Three catalogs translated the literal `secret` shortcode attribute and two dropped the leading space that separates the PHP-version fragment; the compiled .mo files are rebuilt to match. Also fixes a duplicated port when WP_ENV_URL already carries one, and corrects an E2E test that claimed to be anonymous while using the authenticated request fixture.
Agent planning notes are local scratch, not shipped source. The directory is now ignored so it does not come back.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The new save-path filter can hand back a directory that does not exist or cannot be written. PHPSpreadsheet throws there, and this class handles that by printing an error page and exiting — which kills the request while a notification is being sent instead of just losing the attachment.
Collaborator
Author
|
@coderabbitai, dispositions for the comments in the review body (those have no inline thread to reply under). Every one was checked against the code rather than taken at face value. Addressed in 74a524f
Addressed in 69b33ae
Moot — 70f2aa8
Declined — the per-language wording comments ( The three fixes that were made to catalogs are a different class — a literal Declined — Declined — multi-notification attachment coverage An earlier revision of this comment called it a real coverage gap. That was wrong — it read new list-handling code as evidence of a user-reachable feature. |
|
Tip For best results, initiate chat on the files or code changes.
I accept the fixes in I also accept the declined translation wording findings. Generated catalogs must receive wording corrections in the translation platform. The mechanically verifiable token and whitespace fixes are the appropriate repository-level changes. I accept the decision not to change Multi-notification attachment coverage remains a useful follow-up. Would you like me to create a GitHub issue for that E2E test case? 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The removed I accept the decision to keep subjective translation wording changes in the translation platform. I will limit future catalog findings to mechanically verifiable defects. I accept the decision not to change Multi-notification attachment coverage remains a valid backlog item. It does not block this release PR. 🐇 You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Action/NotificationAttachmentAction.php (1)
90-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the inline assignment in the
render()call.
$save = truecreates a local variable that is never used. PHPMD flags it. Pass the literal instead.♻️ Proposed cleanup
try { // save the file to a temporary file - $file = $output->render( $save = true ); + $file = $output->render( true ); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Action/NotificationAttachmentAction.php` around lines 90 - 95, Update the render call in the try block to pass the literal true directly, removing the unused inline assignment to $save while preserving the existing finally cleanup behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@languages/gk-gravityexport-lite-de_DE.po`:
- Around line 55-61: Align all locale catalogs with translations.pot by adding
“There was an error generating the download URL. Please try again.” and its
locale-specific translation to languages/gk-gravityexport-lite-de_DE.po lines
55-61, languages/gk-gravityexport-lite-hu_HU.po lines 53-61, and
languages/gk-gravityexport-lite-zh_CN.po lines 53-59.
- Around line 491-493: Update the German msgstr for “Move %s” to use the neutral
infinitive action label “Verschieben %s”, matching the form used by the other
German action labels and preserving the %s placeholder.
In `@languages/gk-gravityexport-lite-hu_HU.po`:
- Around line 119-126: Update the Hungarian msgstr translation to use “az űrlap”
instead of “a űrlap”, preserving the rest of the translation unchanged.
---
Nitpick comments:
In `@src/Action/NotificationAttachmentAction.php`:
- Around line 90-95: Update the render call in the try block to pass the literal
true directly, removing the unused inline assignment to $save while preserving
the existing finally cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d6acd8f-bbe2-413a-9e2d-b6b45e881f7f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (95)
.circleci/config.yml.env.sample.gitattributes.gitignore.gktools.jsonGruntfile.jsgfexcel.phplanguages/gk-gravityexport-lite-ar.molanguages/gk-gravityexport-lite-ar.polanguages/gk-gravityexport-lite-bn_BD.molanguages/gk-gravityexport-lite-bn_BD.polanguages/gk-gravityexport-lite-bs_BA.molanguages/gk-gravityexport-lite-bs_BA.polanguages/gk-gravityexport-lite-da_DK.molanguages/gk-gravityexport-lite-da_DK.polanguages/gk-gravityexport-lite-de_DE.molanguages/gk-gravityexport-lite-de_DE.polanguages/gk-gravityexport-lite-es_AR.molanguages/gk-gravityexport-lite-es_AR.polanguages/gk-gravityexport-lite-es_ES.molanguages/gk-gravityexport-lite-es_ES.polanguages/gk-gravityexport-lite-es_MX.molanguages/gk-gravityexport-lite-es_MX.polanguages/gk-gravityexport-lite-fa_IR.molanguages/gk-gravityexport-lite-fa_IR.polanguages/gk-gravityexport-lite-fi.molanguages/gk-gravityexport-lite-fi.polanguages/gk-gravityexport-lite-fr_CA.molanguages/gk-gravityexport-lite-fr_CA.polanguages/gk-gravityexport-lite-fr_FR.molanguages/gk-gravityexport-lite-fr_FR.polanguages/gk-gravityexport-lite-he_IL.molanguages/gk-gravityexport-lite-he_IL.polanguages/gk-gravityexport-lite-hu_HU.molanguages/gk-gravityexport-lite-hu_HU.polanguages/gk-gravityexport-lite-it_IT.molanguages/gk-gravityexport-lite-it_IT.polanguages/gk-gravityexport-lite-ja.molanguages/gk-gravityexport-lite-ja.polanguages/gk-gravityexport-lite-ko_KR.molanguages/gk-gravityexport-lite-ko_KR.polanguages/gk-gravityexport-lite-nb_NO.molanguages/gk-gravityexport-lite-nb_NO.polanguages/gk-gravityexport-lite-nl_NL.molanguages/gk-gravityexport-lite-nl_NL.polanguages/gk-gravityexport-lite-pl_PL.molanguages/gk-gravityexport-lite-pl_PL.polanguages/gk-gravityexport-lite-pt_BR.molanguages/gk-gravityexport-lite-pt_BR.polanguages/gk-gravityexport-lite-pt_PT.molanguages/gk-gravityexport-lite-pt_PT.polanguages/gk-gravityexport-lite-ro_RO.molanguages/gk-gravityexport-lite-ro_RO.polanguages/gk-gravityexport-lite-ru_RU.molanguages/gk-gravityexport-lite-ru_RU.polanguages/gk-gravityexport-lite-sv_SE.molanguages/gk-gravityexport-lite-sv_SE.polanguages/gk-gravityexport-lite-tr_TR.molanguages/gk-gravityexport-lite-tr_TR.polanguages/gk-gravityexport-lite-zh_CN.molanguages/gk-gravityexport-lite-zh_CN.popackage.jsonpublic/css/gravityexport-lite.csspublic/js/gravityexport-lite.jsreadme.txtsrc/Action/ActionNotice.phpsrc/Action/DownloadUrlDisableAction.phpsrc/Action/DownloadUrlEnableAction.phpsrc/Action/DownloadUrlResetAction.phpsrc/Action/FiresWithNotice.phpsrc/Action/NotificationAttachmentAction.phpsrc/Action/NotifyingActionInterface.phpsrc/Addon/AddonHelperTrait.phpsrc/Addon/GravityExportAddon.phpsrc/Renderer/AbstractPHPExcelRenderer.phpsrc/Repository/FormsRepository.phptests/E2E/helpers/test-helpers.jstests/E2E/setup/mu-plugins/e2e-mail-capture.phptests/E2E/setup/playwright.config.jstests/E2E/setup/playwright.global.setup.jstests/E2E/setup/playwright.global.teardown.jstests/E2E/setup/wp-env.config.jstests/E2E/tests/activation.spec.jstests/E2E/tests/data-shaping/entry-notes.spec.jstests/E2E/tests/data-shaping/transpose.spec.jstests/E2E/tests/download-url/download-csv.spec.jstests/E2E/tests/download-url/download-xlsx.spec.jstests/E2E/tests/download-url/enable-download-url.spec.jstests/E2E/tests/download-url/regenerate-disable.spec.jstests/E2E/tests/fields/disable-field.spec.jstests/E2E/tests/fields/sort-order.spec.jstests/E2E/tests/notifications/attach-csv.spec.jstests/E2E/tests/notifications/attach-xlsx.spec.jstests/E2E/tests/permissions/logged-in-required.spec.jstranslations.pot
🚧 Files skipped from review as they are similar to previous changes (53)
- languages/gk-gravityexport-lite-es_AR.po
- tests/E2E/tests/fields/disable-field.spec.js
- package.json
- tests/E2E/setup/playwright.global.setup.js
- public/css/gravityexport-lite.css
- .gitignore
- src/Renderer/AbstractPHPExcelRenderer.php
- languages/gk-gravityexport-lite-da_DK.po
- languages/gk-gravityexport-lite-es_MX.po
- languages/gk-gravityexport-lite-nb_NO.po
- .gktools.json
- languages/gk-gravityexport-lite-bs_BA.po
- public/js/gravityexport-lite.js
- src/Action/DownloadUrlDisableAction.php
- Gruntfile.js
- languages/gk-gravityexport-lite-es_ES.po
- languages/gk-gravityexport-lite-tr_TR.po
- languages/gk-gravityexport-lite-ru_RU.po
- languages/gk-gravityexport-lite-sv_SE.po
- tests/E2E/tests/permissions/logged-in-required.spec.js
- tests/E2E/tests/data-shaping/transpose.spec.js
- src/Addon/AddonHelperTrait.php
- src/Action/DownloadUrlResetAction.php
- src/Action/DownloadUrlEnableAction.php
- tests/E2E/tests/download-url/enable-download-url.spec.js
- languages/gk-gravityexport-lite-bn_BD.po
- readme.txt
- languages/gk-gravityexport-lite-it_IT.po
- languages/gk-gravityexport-lite-ko_KR.po
- .gitattributes
- languages/gk-gravityexport-lite-pl_PL.po
- languages/gk-gravityexport-lite-he_IL.po
- languages/gk-gravityexport-lite-pt_BR.po
- tests/E2E/tests/notifications/attach-csv.spec.js
- languages/gk-gravityexport-lite-fa_IR.po
- src/Action/FiresWithNotice.php
- tests/E2E/setup/playwright.global.teardown.js
- src/Repository/FormsRepository.php
- src/Action/ActionNotice.php
- languages/gk-gravityexport-lite-ar.po
- languages/gk-gravityexport-lite-pt_PT.po
- languages/gk-gravityexport-lite-fi.po
- .circleci/config.yml
- tests/E2E/tests/download-url/download-csv.spec.js
- tests/E2E/tests/fields/sort-order.spec.js
- languages/gk-gravityexport-lite-fr_CA.po
- languages/gk-gravityexport-lite-fr_FR.po
- src/Addon/GravityExportAddon.php
- languages/gk-gravityexport-lite-ro_RO.po
- gfexcel.php
- tests/E2E/tests/activation.spec.js
- languages/gk-gravityexport-lite-ja.po
- languages/gk-gravityexport-lite-nl_NL.po
|
Tip For best results, initiate chat on the files or code changes.
The removal of I accept the decision to keep subjective translation wording corrections in the translation platform. I accept the decision to retain I withdraw the multi-notification attachment coverage finding. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
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
2 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.
Developer Updates:
gk/gravityexport/notification/attachment-idsfilter to change the notification IDs the single-entry export is attached to.gk/gravityexport/notification/attachment-source-idsfilter to replace the stored notification selection; it runs first, so callbacks on the filter above always receive the complete selection (used by GravityExport to attach the export to multiple notifications).gk/gravityexport/renderer/save-pathfilter to change the path a rendered export file is saved to.gk/gravityexport/feed/pre-save-settingsfilter to change the feed settings before they are stored; it runs inside the save, so related settings are written in a single update.Summary by CodeRabbit
New Features
Bug Fixes
Release
💾 Build file (69b33ae).