-
Notifications
You must be signed in to change notification settings - Fork 191
feat: make deployment screenshots clickable #2481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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. |
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
WalkthroughA single Svelte component replaces individual exported props with a single destructured $props() signature (typed, with defaults). It imports regionalProtocol, changes Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Rationale:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this 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 exposeprimaryDomain
to avoid?.[0]
in markup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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)
andtotalSize
via$derived(...)
are idiomatic.
13-17
: Imports and URL concatenation are correctThe
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 appropriateVerification 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.
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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, thearia-label
andalt
text are generic and should reference the actual domain for better context.Additionally, the original review suggested using
Link.Anchor
from$lib/components
, butLink
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
📒 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.
What does this PR do?
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