Display scheduled time for posts #108
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds Playwright end-to-end testing infrastructure for the page application with configuration and test suite. Updates marketing content messaging in features and hero components. Enhances post component with DateTime-based publish-at formatting and audit logging. Restructures homepage by removing sections and reordering components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/components/post/post.tsx (1)
169-192: Add missing dependencies to useCallback and consider audit log sequencing.The callback correctly identifies that
page.idanduser.idare referenced in the closure but missing from the dependency array[supabase, fetchPosts]. This violates React's exhaustive-deps ESLint rule (enabled by default in Next.js) and could cause stale closures ifpageoruserchange.Additionally, the post is deleted (line 174) before the audit log is created (lines 176–181). If
createAuditLogfails, the deletion persists but no audit record is created, potentially compromising audit trail integrity.Apply this diff:
}, - [supabase, fetchPosts] + [supabase, fetchPosts, page, user] );Consider whether deleting before logging is acceptable, or implement error handling to roll back the deletion if audit logging fails.
🧹 Nitpick comments (5)
apps/web/components/post/post.tsx (2)
279-298: Desktop status display correctly shows scheduled publish time.The large-screen status panel properly displays the publish time for scheduled posts using the same DateTime formatting.
The status display logic is duplicated between the mobile view (lines 229-250) and desktop view (lines 279-298). Consider extracting this into a reusable component to follow DRY principles and simplify future maintenance.
220-251: DateTime.fromISO() lacks validation but guard check provides basic protection; unnecessary ternary on line 222 confirmed.The code functions correctly with appropriate guards. The
DateTime.fromISO()implementation uses JavaScript'snew Date()directly without validation, meaning malformed ISO strings would silently create an Invalid Date that displays as "Invalid Date" intoNiceFormat(). However, this is mitigated by the guard conditionpost.status === PostStatus.publish_later && post.publish_aton line 243, which checks truthiness. The real-world risk is low sincepublish_atshould be server-validated before reaching the client.The ternary expression
idx ? "" : ""on line 222 is confirmed as unnecessary—both branches return empty strings and can be simplified or removed entirely.apps/page/tests/page.spec.js (2)
3-23: Consider closing the request context to prevent resource leaks.The
request.newContext()at line 12 creates an HTTP context that should be explicitly closed after use. While Playwright may clean up some resources automatically, it's best practice to close contexts explicitly, especially in test suites that may run many tests.Apply this diff to ensure proper cleanup:
test("visit page and take screenshot", async ({ page }) => { const targetUrl = process.env.ENVIRONMENT_URL || "https://hey.changes.page"; const response = await page.goto(targetUrl); expect(response.status()).toBeLessThan(400); await page.screenshot({ path: "screenshot.jpg" }); const context = await request.newContext({ baseURL: targetUrl, }); - const latest = await context.get(`/latest.json`); - expect(latest.ok()).toBeTruthy(); + try { + const latest = await context.get(`/latest.json`); + expect(latest.ok()).toBeTruthy(); - const latestPost = await latest.json(); - const latestPostResponse = await page.goto(latestPost.url); - expect(latestPostResponse.status()).toBeLessThan(400); - await page.screenshot({ path: "latestPostResponse-screenshot.jpg" }); + const latestPost = await latest.json(); + const latestPostResponse = await page.goto(latestPost.url); + expect(latestPostResponse.status()).toBeLessThan(400); + await page.screenshot({ path: "latestPostResponse-screenshot.jpg" }); + } finally { + await context.dispose(); + } });
25-49: Consider closing the request context to prevent resource leaks.Similar to the first test, the
request.newContext()at line 28 should be explicitly closed after use.Apply this diff to ensure proper cleanup:
test("verify APIs", async () => { const targetUrl = process.env.ENVIRONMENT_URL || "https://hey.changes.page"; const context = await request.newContext({ baseURL: targetUrl, }); - const posts = await context.get(`/changes.json`); - expect(posts.ok()).toBeTruthy(); + try { + const posts = await context.get(`/changes.json`); + expect(posts.ok()).toBeTruthy(); - const robots = await context.get(`/robots.txt`); - expect(robots.ok()).toBeTruthy(); + const robots = await context.get(`/robots.txt`); + expect(robots.ok()).toBeTruthy(); - const sitemap = await context.get(`/sitemap.xml`); - expect(sitemap.ok()).toBeTruthy(); + const sitemap = await context.get(`/sitemap.xml`); + expect(sitemap.ok()).toBeTruthy(); - const markdown = await context.get(`/changes.md`); - expect(markdown.ok()).toBeTruthy(); + const markdown = await context.get(`/changes.md`); + expect(markdown.ok()).toBeTruthy(); - const rss = await context.get(`/rss.xml`); - expect(rss.ok()).toBeTruthy(); + const rss = await context.get(`/rss.xml`); + expect(rss.ok()).toBeTruthy(); - const atom = await context.get(`/atom.xml`); - expect(atom.ok()).toBeTruthy(); + const atom = await context.get(`/atom.xml`); + expect(atom.ok()).toBeTruthy(); + } finally { + await context.dispose(); + } });apps/page/playwright.config.js (1)
1-20: LGTM! Consider adding additional browser coverage.The Playwright configuration is well-structured with appropriate CI-aware settings. The single Chromium project is functional, but consider adding Firefox and WebKit projects for broader browser coverage.
If broader coverage is desired, add these projects:
projects: [ { name: "chromium", use: { ...devices["Desktop Chrome"] }, }, { name: "firefox", use: { ...devices["Desktop Firefox"] }, }, { name: "webkit", use: { ...devices["Desktop Safari"] }, }, ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/hero/app-screenshot.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/page-e2e-tests.yml(1 hunks)apps/page/.gitignore(1 hunks)apps/page/package.json(2 hunks)apps/page/playwright.config.js(1 hunks)apps/page/tests/page.spec.js(1 hunks)apps/web/components/marketing/features.tsx(1 hunks)apps/web/components/marketing/hero.tsx(1 hunks)apps/web/components/post/post.tsx(5 hunks)apps/web/pages/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/page/tests/page.spec.js (1)
apps/page/playwright.config.js (1)
require(1-1)
apps/web/components/post/post.tsx (3)
packages/ui/components/PostTypes.tsx (1)
PostTypeBadge(89-116)packages/supabase/types/page.ts (1)
PostStatusToLabel(60-67)packages/utils/datetime/index.ts (1)
DateTime(70-70)
apps/page/playwright.config.js (1)
apps/page/tests/page.spec.js (1)
require(1-1)
🪛 GitHub Check: CodeQL
.github/workflows/page-e2e-tests.yml
[warning] 8-45: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🔇 Additional comments (8)
apps/web/components/marketing/hero.tsx (1)
115-119: LGTM! Marketing content updated.The updated hero paragraph provides a clearer description of the platform's capabilities, aligning with the messaging refresh across marketing components.
apps/web/components/marketing/features.tsx (1)
96-104: LGTM! Consistent messaging update.The updated heading provides clearer positioning of the platform, consistent with the marketing refresh in hero.tsx.
apps/web/pages/index.tsx (3)
23-23: LGTM! Meta description updated.The updated meta description aligns with the refreshed marketing messaging and provides clear value propositions.
135-135: LGTM! Component reordering.The FAQs component positioning after GetStartedHero is appropriate for the restructured homepage flow.
160-161: LGTM! Code quality improvements.The changes improve code quality:
- Adding radix
10toparseIntis a best practice that ensures consistent decimal parsing and avoids potential octal interpretation issues.- The bare
catchclause is valid ES2019+ syntax and is cleaner when the error object isn't used.apps/web/components/post/post.tsx (1)
11-11: LGTM! Imports support scheduled post display and audit logging.The DateTime utility enables formatting of scheduled publish times, and createAuditLog provides proper audit trail for post deletions.
Also applies to: 20-20
apps/page/.gitignore (1)
10-12: LGTM! Standard Playwright ignore patterns.The added ignore patterns for Playwright test artifacts are appropriate and follow best practices.
.github/workflows/page-e2e-tests.yml (1)
12-45: LGTM! Workflow steps are well-configured.The workflow steps follow best practices:
- Uses current action versions (v4)
- Frozen lockfile ensures reproducible builds
- Browser installation scoped to chromium only (matching config)
- Screenshot upload with
if: always()ensures artifacts are captured even on test failure- Reasonable retention period
Summary by CodeRabbit
New Features
Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.