fix: omit unchanged file field from document module by priscila-moneo · Pull Request #871 · fntechgit/summit-admin

priscila-moneo · 2026-04-14T21:02:22Z

ref: https://app.clickup.com/t/86b92ac9w

Summary by CodeRabbit

Bug Fixes
- Improved handling of uploaded file entries in page templates: only preserves newly provided file objects and removes existing or invalid file data, avoiding incorrect/null placeholders and improving downstream rendering.
Tests
- Added tests covering file-preservation and omission behaviors across module types and file input cases.

coderabbitai · 2026-04-14T21:02:38Z

📝 Walkthrough

Walkthrough

Exported normalizeEntity and adjusted FILE-module normalization: for PAGE_MODULES_DOWNLOAD.FILE, normalizedModule.file is set to the first array item only when that item is an object missing both id and file_id; otherwise normalizedModule.file is deleted. Added tests for these cases.

Changes

Cohort / File(s)	Summary
Page template action `src/actions/page-template-actions.js`	Exported `normalizeEntity`. For `PAGE_MODULES_DOWNLOAD.FILE` modules, when `module.file` is an array, assign `normalizedModule.file = module.file[0]` only if that element is an object without `id` and `file_id`; otherwise `delete normalizedModule.file` (previously set to `null`). `external_url` deletion behavior unchanged.
Tests `src/actions/__tests__/page-template-actions.test.js`	New Jest tests for `normalizeEntity` covering: include `file` when first entry is a new file (no `id`/`file_id`), omit `file` when entry has `id`, omit `file` for non-file module types, and omit when `file` is `null`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

smarcet
martinquiroga-exo

Poem

🐰 I hop through lines both short and long,

I spot the ids that don't belong.
If none are found, I keep the prize,
Else I tidy up with nimble sighs.
A rabbit's nibble makes the code feel wise.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: omit unchanged file field from document module' accurately summarizes the main change: conditional logic now deletes the file field instead of setting it to null when the file is unchanged.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/editing-saving-cloned-page-error

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

src/actions/page-template-actions.js (1)

175-175: Use explicit nullish checks for persisted file identifiers

On Line 175, !f.id && !f.file_id treats falsy values as “missing”. If an identifier can be 0/"", unchanged files may be incorrectly sent as new file payloads. Prefer existence checks (== null) instead of truthiness.

Proposed adjustment

-        if (f && typeof f === "object" && !f.id && !f.file_id) {
+        const hasPersistedId = f?.id != null || f?.file_id != null;
+        if (f && typeof f === "object" && !hasPersistedId) {
           normalizedModule.file = f;
         } else {
           delete normalizedModule.file;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/actions/page-template-actions.js` at line 175, The condition treating
missing file IDs uses truthiness and will misclassify valid falsy IDs; update
the check on f.id and f.file_id to use explicit nullish checks (== null) instead
of negation, e.g. change the clause in the block that uses variable f (the
condition beginning with if (f && typeof f === "object" && ...)) so it tests
f.id == null && f.file_id == null (or equivalent nullish checks) so only
null/undefined are treated as missing while preserving the object/type guard.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/actions/page-template-actions.js`:
- Line 175: The condition treating missing file IDs uses truthiness and will
misclassify valid falsy IDs; update the check on f.id and f.file_id to use
explicit nullish checks (== null) instead of negation, e.g. change the clause in
the block that uses variable f (the condition beginning with if (f && typeof f
=== "object" && ...)) so it tests f.id == null && f.file_id == null (or
equivalent nullish checks) so only null/undefined are treated as missing while
preserving the object/type guard.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0d60cb6-b6ad-44ae-85c6-3f8ec5ba3775

📥 Commits

Reviewing files that changed from the base of the PR and between 275a86d and e67f267.

📒 Files selected for processing (1)

src/actions/page-template-actions.js

santipalenque · 2026-04-14T21:36:01Z

    if (module.kind === PAGES_MODULE_KINDS.DOCUMENT) {
      if (module.type === PAGE_MODULES_DOWNLOAD.FILE) {
-        normalizedModule.file = module.file?.[0] || null;
+        const f = Array.isArray(module.file) ? module.file[0] : null;


please use a better variable name

santipalenque · 2026-04-14T21:36:59Z

      if (module.type === PAGE_MODULES_DOWNLOAD.FILE) {
-        normalizedModule.file = module.file?.[0] || null;
+        const f = Array.isArray(module.file) ? module.file[0] : null;
+        if (f && typeof f === "object" && !f.id && !f.file_id) {


please comment why if file has no id and no file_id then we delete it from payload

We only include the file in the payload if it doesn't have an id or file_id, which means it's a newly added file that hasn't been persisted yet.

If the file already has an id/file_id, we remove it from the payload. These represent existing files, and since they are already stored, sending them again would be redundant.

I mean put a comment in the code so next dev knows whats going on. Also please check updating a file works with this logic.

Already checked that all still works before pushing it, but now I've added new unit tests for this scenarios. Also I've added the comment. Thanks for your comments!

coderabbitai

🧹 Nitpick comments (1)

src/actions/__tests__/page-template-actions.test.js (1)
14-68: LGTM!

Tests correctly exercise the four key branches of normalizeEntity's file normalization logic and align with the implementation in src/actions/page-template-actions.js.

Optional: consider adding a case for file_id (existing file referenced by file_id instead of id) to fully cover the isNewFile predicate, since the implementation explicitly checks both.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/actions/__tests__/page-template-actions.test.js` around lines 14 - 68,
Add a test that covers the branch where an existing file is referenced by
file_id (not id) so the isNewFile predicate in normalizeEntity is fully
exercised; create an entity using buildModule with PAGES_MODULE_KINDS.DOCUMENT
and PAGE_MODULES_DOWNLOAD.FILE containing [{ file_id: 456, name:
"existing_by_file_id.pdf" }] and assert that
normalizeEntity(entity).modules[0].file is undefined (matching the existing-file
behavior).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/actions/__tests__/page-template-actions.test.js`:
- Around line 14-68: Add a test that covers the branch where an existing file is
referenced by file_id (not id) so the isNewFile predicate in normalizeEntity is
fully exercised; create an entity using buildModule with
PAGES_MODULE_KINDS.DOCUMENT and PAGE_MODULES_DOWNLOAD.FILE containing [{
file_id: 456, name: "existing_by_file_id.pdf" }] and assert that
normalizeEntity(entity).modules[0].file is undefined (matching the existing-file
behavior).

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7c7bd75b-5562-43d5-beb5-ea31def82118

📥 Commits

Reviewing files that changed from the base of the PR and between bb59d39 and 1a4f6eb.

📒 Files selected for processing (2)

src/actions/__tests__/page-template-actions.test.js
src/actions/page-template-actions.js

🚧 Files skipped from review as they are similar to previous changes (1)

src/actions/page-template-actions.js

santipalenque · 2026-04-17T14:03:02Z

 };

-const normalizeEntity = (entity) => {
+export const normalizeEntity = (entity) => {


do we need to export this method ? who is using it ?

it's only for testing purposes

martinquiroga-exo

LGTM

santipalenque

LGTM

priscila-moneo requested review from martinquiroga-exo, santipalenque and smarcet April 14, 2026 21:02

coderabbitai Bot reviewed Apr 14, 2026

View reviewed changes

santipalenque reviewed Apr 14, 2026

View reviewed changes

priscila-moneo force-pushed the fix/editing-saving-cloned-page-error branch from e67f267 to bb59d39 Compare April 15, 2026 15:38

priscila-moneo requested a review from santipalenque April 15, 2026 15:38

fix: omit unchanged file field from document module

1a4f6eb

priscila-moneo force-pushed the fix/editing-saving-cloned-page-error branch from bb59d39 to 1a4f6eb Compare April 16, 2026 20:07

coderabbitai Bot reviewed Apr 16, 2026

View reviewed changes

santipalenque reviewed Apr 17, 2026

View reviewed changes

martinquiroga-exo approved these changes Apr 20, 2026

View reviewed changes

santipalenque approved these changes Apr 21, 2026

View reviewed changes

Conversation

priscila-moneo commented Apr 14, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

santipalenque Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

santipalenque Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

priscila-moneo Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

santipalenque Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

priscila-moneo Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

santipalenque Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

priscila-moneo Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

martinquiroga-exo left a comment

Choose a reason for hiding this comment

Uh oh!

santipalenque left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

priscila-moneo commented Apr 14, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 14, 2026 •

edited

Loading