Skip to content

Conversation

@sondermanish
Copy link
Contributor

@sondermanish sondermanish commented Oct 31, 2025

Description

Tip

Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).

Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.

Fixes #Issue Number
or
Fixes Issue URL

Warning

If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.

Automation

/ok-to-test tags="@tag.MobileResponsive"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/18967193496
Commit: 893949f
Cypress dashboard.
Tags: @tag.MobileResponsive
Spec:


Fri, 31 Oct 2025 08:53:28 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Introduces a Git-aware page import flow that conditionally handles pages when an application is connected to Git, improving import reliability across branches.
  • Chores

    • Minor naming and consistency cleanups and added internal notes for future Git synchronization enhancements.

@sondermanish sondermanish requested a review from a team as a code owner October 31, 2025 07:17
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Walkthrough

Replaces the previous unconditional import flow with a Git-aware conditional path in NewPageImportableServiceCEImpl: introduces gitSyncToPagesFromAllBranchesMono (empty map when not Git-connected), builds it via newPageService.findAllByApplicationIds(...) when connected, renames variables (e.g., updatedSlugPagesToImport) and adjusts downstream zips/flatMaps. TODO placeholder added for Git projections.

Changes

Cohort / File(s) Summary
New Git-aware import flow
app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/importable/NewPageImportableServiceCEImpl.java
Reworked import control flow to be Git-aware: added gitSyncToPagesFromAllBranchesMono (initialized to empty map and populated when application is Git-connected), removed unconditional Git-merge block, added TODO for projections, renamed updatedSlugpagesToImportupdatedSlugPagesToImport, and updated downstream Mono.zip / flatMap usage and log messages.

Sequence Diagram(s)

sequenceDiagram
  participant Controller
  participant NewPageImportService as ImportService
  participant NewPageService as PageService
  participant GitStore

  Controller->>ImportService: request import pages (appIds)
  alt Application not Git-connected
    ImportService->>ImportService: gitSyncToPagesFromAllBranchesMono = empty map
    ImportService->>ImportService: proceed with bulk import using updatedSlugPagesToImport
    ImportService-->>Controller: import result
  else Application Git-connected
    ImportService->>PageService: findAllByApplicationIds(appIds)
    PageService-->>ImportService: pages (filter gitSyncId)
    ImportService->>GitStore: (optional) use git projection TODO
    ImportService->>ImportService: build gitSyncToPagesFromAllBranchesMono (cached)
    ImportService->>ImportService: combine with updatedSlugPagesToImport (Mono.zip)
    ImportService-->>Controller: git-aware import result
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Points to review:

  • The Git-aware branch population: ensure correct filtering on gitSyncId and intended projections (TODO).
  • Variable rename consistency (updatedSlugPagesToImport) across all usages.
  • Caching and lifecycle of gitSyncToPagesFromAllBranchesMono to avoid stale data or memory leaks.
  • Log message updates for clarity.

Poem

A branch-aware path now sings, 🌿
Maps and Monos change their wings,
TODO whispers where projections dwell,
Renamed variables ring the bell,
Import flows dance, prepared to swell. 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is largely incomplete and fails to provide substantive content required by the template. The Description section contains only the template guidance text itself with no actual description of the changes made. Critical required information is missing, including: an issue reference (the "Fixes #" placeholder is not filled), motivation and context for the Git-aware import flow refactoring, any dependencies, and relevant documentation links. While the Automation section includes test tags and Cypress results are auto-populated, these do not substitute for the core description content. The Communication section checkbox remains unchecked. Overall, the PR provides no meaningful explanation of why the changes were made or what they accomplish. The author should fill in the Description section with an actual summary of the changes and their purpose. They must add a valid issue reference (Fixes #Issue Number or URL), provide context and motivation for the Git-aware import refactoring, list any dependencies, and include links to relevant documentation if applicable. The Communication checkbox should be explicitly selected (Yes or No). Once these required sections are completed with substantive content, the description will meet the template requirements.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title Check ✅ Passed The PR title "chore: page import logic update, page with existing git sync id gets updated instead of new import" directly addresses the primary behavioral change in the changeset. The raw summary confirms that the main modification shifts the import flow to be Git-aware and changes how pages with existing git sync IDs are handled—they are now updated rather than newly imported. The title is specific and clear, accurately reflecting the core intent of the change without being vague or generic, allowing reviewers scanning history to understand the modification's purpose.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/mobile-test

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.

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Oct 31, 2025
@sondermanish sondermanish self-assigned this Oct 31, 2025
@sondermanish sondermanish added the ok-to-test Required label for CI label Oct 31, 2025
@sondermanish sondermanish changed the title chore: added a note chore: page import logic update, page with existing git sync id gets updated instead of new import Oct 31, 2025
@sondermanish sondermanish merged commit c9e1a57 into release Oct 31, 2025
47 checks passed
@sondermanish sondermanish deleted the chore/mobile-test branch October 31, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Required label for CI skip-changelog Adding this label to a PR prevents it from being listed in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants