Skip to content

Conversation

HarshMN2345
Copy link
Member

@HarshMN2345 HarshMN2345 commented Oct 17, 2025

What does this PR do?

image

Related PRs and Issues

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

Summary by CodeRabbit

  • New Features
    • Site card screenshots are now clickable links to the primary domain when proxy rules exist.
    • Domain links respect regional protocol settings for more reliable navigation.
    • Domains on site cards are sorted to prioritize manually configured entries for clearer presentation.
    • Deployment size is now shown in a human-readable format on site cards for easier inspection.

Copy link

railway-app bot commented Oct 17, 2025

This PR was not deployed automatically as @HarshMN2345 does not have access to the Railway project.

In order to get automatic PR deploys, please add @HarshMN2345 to your workspace on Railway.

Copy link

appwrite bot commented Oct 17, 2025

Console

Project ID: 688b7bf400350cbd60e9

Sites (2)
Site Status Logs Preview QR
 console-stage
688b7cf6003b1842c9dc
Ready Ready View Logs Preview URL QR Code
 console-cloud
688b7c18002b9b871a8f
Ready Ready View Logs Preview URL QR Code

Note

Cursor pagination performs better than offset pagination when loading further pages.

Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Walkthrough

A single Svelte component replaces individual exported props with a single destructured $props() signature (typed, with defaults). It imports regionalProtocol, changes show to use $state(), and adds $derived values for totalSize and sortedDomains (and primaryDomain). The screenshot Image is now conditionally wrapped in an anchor constructed from the first domain and regionalProtocol; existing getScreenshot usage is retained but adjusted to the new reactive values. Rendering structure was updated to consume the new props and derived state.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Rationale:

  • Single-file change but heterogeneous: API migration to Svelte 5-style $props/$state/$derived, added import, and conditional link wrapping.
  • Review must verify typings/defaults, correctness of derived computations and sorting, and that anchor construction preserves prior behavior.
  • Moderate risk for UI/regression around screenshot linking and prop reactivity.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat: make deployment screenshots clickable" accurately and directly describes the primary change in the changeset. According to the summary, the main feature addition is wrapping the screenshot Image in a clickable anchor link that directs to the first domain using the regional protocol, which is exactly what the title conveys. The title is concise, clear, uses the appropriate "feat:" prefix for feature changes, and avoids vague or generic language. A developer scanning commit history would immediately understand that this PR adds interactivity to deployment screenshots.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-make-site-card-clickable

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (1)

45-51: Derivation is fine; minor readability tweak (optional)

If adopting the fix above, keep sortedDomains and also expose primaryDomain to avoid ?.[0] in markup.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1590206 and ee71860.

📒 Files selected for processing (1)
  • src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (3 hunks)
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (3)

41-44: State/derived usage LGTM

show via $state(false) and totalSize via $derived(...) are idiomatic.


13-17: Imports and URL concatenation are correct

The regionalProtocol store correctly returns either 'https://' or 'http://' based on the regional console's HTTPS configuration, ensuring all concatenations with domain names produce valid URLs across all regions. No normalization changes needed.


29-39: Runes are fully enabled project-wide; code change is appropriate

Verification confirms runes are enabled and the $props() usage in siteCard.svelte is consistent with project patterns. Svelte 5.25.3 has runes enabled by default (no explicit config flag required in v5+). The codebase shows extensive runes adoption: 106+ $props instances and 486+ other rune instances ($state, $derived, $effect, $bindable). No build surprises expected.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (1)

79-101: Improve accessibility attributes; consider using Link.Anchor for consistency.

The primaryDomain check correctly prevents undefined href values. However, the aria-label and alt text are generic and should reference the actual domain for better context.

Additionally, the original review suggested using Link.Anchor from $lib/components, but Link is not exported there. If consistency with the codebase patterns is desired, Link.Anchor can be imported directly from @appwrite.io/pink-svelte (as used throughout the codebase). However, the plain <a> tag is functionally adequate.

The primary recommendation is to improve the accessibility attributes:

             {#if primaryDomain}
                 <a
                     href={`${$regionalProtocol}${primaryDomain}`}
                     target="_blank"
                     rel="noopener noreferrer"
-                    aria-label="Open site">
+                    aria-label={`Open ${primaryDomain} in new tab`}>
                     <Image
                         border
                         radius="s"
                         ratio="16/9"
                         style="width: 100%; align-self: start"
                         src={getScreenshot($app.themeInUse, deployment)}
-                        alt="Screenshot" />
+                        alt={`Screenshot of ${primaryDomain}`} />
                 </a>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f59f946 and 7cf6241.

📒 Files selected for processing (1)
  • src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: e2e
🔇 Additional comments (5)
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (5)

26-26: LGTM!

The regionalProtocol import is correctly added and properly used with the $ prefix in the template.


28-38: LGTM!

The migration to Svelte 5's $props() pattern is correctly implemented with proper TypeScript types and default values.


40-40: LGTM!

The $state() declaration is correct for Svelte 5's reactive state management.


42-51: LGTM! Good defensive coding.

The derived values are well-structured, and the primaryDomain extraction correctly addresses the concern from the previous review about preventing undefined domain references.


157-157: LGTM!

The usage of the derived totalSize value is correct and improves code clarity by moving the computation out of the template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant