feat: scaffold hotel apartment detail route placeholder#85
feat: scaffold hotel apartment detail route placeholder#85Obiajulu-gif wants to merge 1 commit intosafetrustcr:mainfrom
Conversation
|
@Obiajulu-gif Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new dynamic apartment detail route page ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Pull request overview
Scaffolds the initial apps/web Next.js (App Router) application inside a pnpm-workspaces monorepo and adds a placeholder /hotel/[id] apartment detail route for the escrow booking flow.
Changes:
- Added pnpm workspace + root build entrypoint script for building
@safetrust/web. - Introduced Turborepo configuration (
turbo.json) and lockfile. - Implemented basic Next.js app shell (layout, globals, homepage) and
/hotel/[id]stub detail page with BOOK link.
Reviewed changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
turbo.json |
Defines Turbo build task configuration and cache outputs. |
scripts/build.mjs |
Cross-platform root build wrapper that runs pnpm --filter @safetrust/web build. |
pnpm-workspace.yaml |
Declares apps/* as workspace packages. |
pnpm-lock.yaml |
Locks monorepo dependencies (Turbo + Next/React/TS). |
package.json |
Root package manifest with pnpm packageManager pin and build script. |
apps/web/package.json |
Web app manifest with Next/React dependencies and scripts. |
apps/web/tsconfig.json |
Strict TypeScript config for the Next.js app. |
apps/web/next.config.mjs |
Next.js config (currently set to ignore TS/ESLint errors on build). |
apps/web/next-env.d.ts |
Next.js TypeScript environment references. |
apps/web/src/app/globals.css |
Global styles + responsive helper for desktop suggestions. |
apps/web/src/app/layout.tsx |
Root layout wiring global CSS and metadata. |
apps/web/src/app/page.tsx |
Home page linking to the hotel detail placeholder route. |
apps/web/src/app/hotel/[id]/page.tsx |
Stub apartment detail page with placeholders and BOOK link to escrow create. |
.gitignore |
Ignores node_modules, Turbo cache, and Next build outputs. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/next.config.mjs
Outdated
| const nextConfig = { | ||
| eslint: { | ||
| ignoreDuringBuilds: true, | ||
| }, | ||
| typescript: { | ||
| ignoreBuildErrors: true, |
There was a problem hiding this comment.
ignoreDuringBuilds and ignoreBuildErrors make next build succeed even when ESLint/TypeScript would normally fail. That undermines the strict TS config and can let broken code ship; consider removing these overrides, or guarding them behind an env flag so CI/prod builds still enforce checks.
| const nextConfig = { | |
| eslint: { | |
| ignoreDuringBuilds: true, | |
| }, | |
| typescript: { | |
| ignoreBuildErrors: true, | |
| const shouldIgnoreBuildErrors = process.env.NEXT_IGNORE_BUILD_ERRORS === 'true'; | |
| const nextConfig = { | |
| eslint: { | |
| ignoreDuringBuilds: shouldIgnoreBuildErrors, | |
| }, | |
| typescript: { | |
| ignoreBuildErrors: shouldIgnoreBuildErrors, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
.gitignore (1)
3-4: Redundant entry and missing common ignores.Line 4 (
apps/web/.next/) is redundant since.next/on line 3 already matches any.next/directory in the tree. Consider also adding common entries for a more complete setup.♻️ Suggested .gitignore improvements
node_modules/ .turbo/ .next/ -apps/web/.next/ +.env* +.DS_Store +*.log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 3 - 4, Remove the redundant `apps/web/.next/` entry since the top-level `.next/` already ignores all `.next` directories; delete that line and consolidate ignores, and augment the file with common entries such as node_modules, .env, dist/build/output directories, .turbo, .parcel-cache, .DS_Store, coverage, .idea, .vscode, and any editor/OS/tool-specific caches so the repo ignores standard generated and sensitive files.apps/web/src/app/globals.css (2)
19-23: Note on!importantusage.The
!importantflag here suggests the base.desktop-suggestionsstyle usesdisplay: noneinline or via another rule. This pattern works but couples the CSS tightly. Document the expected base state or consider using a CSS class toggle approach for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/globals.css` around lines 19 - 23, The stylesheet currently forces .desktop-suggestions to display:block with !important, which hides the fact that the base state is expected to be hidden elsewhere; remove the !important and instead document the base state and/or implement a clear toggle class (e.g., .desktop-suggestions and .desktop-suggestions--visible) or increase selector specificity to override the base rule; update the CSS around the .desktop-suggestions rule to either include a comment describing the expected base display (hidden) or replace the pattern with a visibility-toggle class and adjust any JS that shows/hides suggestions to use that class.
14-17: Consider link accessibility.Removing underlines from links can reduce accessibility, as users (especially those with color vision deficiencies) may not easily distinguish links from regular text. Consider adding an alternative visual indicator like underline on hover/focus, or ensuring links have sufficient color contrast.
♻️ Suggested accessibility improvement
a { color: inherit; text-decoration: none; } + +a:hover, +a:focus { + text-decoration: underline; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/globals.css` around lines 14 - 17, The global rule removing link underlines (selector "a" in globals.css) reduces accessibility; update the CSS so links keep a visible indicator — for example, restore text-decoration for :hover and :focus/:focus-visible states (or add an outline or border-bottom on those states) and ensure the link color meets contrast requirements against the background; modify the "a" selector and add "a:hover, a:focus, a:focus-visible" rules (or equivalent classes) to provide a clear visual cue on interaction while keeping the base color inheritance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/next.config.mjs`:
- Around line 3-8: Remove the production-suppressing flags in next.config.mjs:
do not set eslint.ignoreDuringBuilds or typescript.ignoreBuildErrors to true
(remove them or set them to false) and instead enforce checks by adding a CI
step and/or npm scripts that run next lint and tsc --noEmit (and add
pre-commit/pre-push hooks if desired); update the project configuration so that
functions referencing eslint.ignoreDuringBuilds and typescript.ignoreBuildErrors
no longer bypass build failures and ensure the repository has a CI workflow that
fails the build on lint/type errors.
In `@apps/web/src/app/hotel/`[id]/page.tsx:
- Line 161: The Link href currently interpolates params.id directly which can
produce malformed URLs for IDs with reserved characters; update the link
construction in the component that renders the Link (the element using Link
href={`/hotel/${params.id}/escrow/create`}) to URL-encode the dynamic segment by
using encodeURIComponent on params.id so the escrow URL is safe (e.g., build the
href from `/hotel/${encodeURIComponent(params.id)}/escrow/create`).
- Around line 23-24: Replace the personal-looking contact values in the stub
fixture by removing or anonymizing the fields: locate the stub object in
page.tsx that defines the email and phone properties (the keys "email" and
"phone") and either delete those keys if unused or replace their values with
clearly synthetic placeholders (e.g., support@example.com, +000 000 0000) to
avoid committing PII; ensure any tests or UI that depend on these fields are
updated to accept the placeholders or handle missing contact fields.
In `@package.json`:
- Around line 1-8: The root package.json is missing turbo as a devDependency
even though a turbo.json exists; update package.json to add a devDependencies
object (or extend it) including "turbo" with a pinned version (e.g., "turbo":
"1.10.0" or the repo's chosen version) alongside the existing packageManager
"pnpm@10.11.0" entry so the monorepo tooling is declared at the root.
In `@scripts/build.mjs`:
- Line 16: The current exit uses process.exit(result.status ?? 0) which masks
failures when result.status is null (process killed by signal); change the logic
to exit with the actual numeric status when present, otherwise use a non‑zero
code (e.g., 1) or derive from result.signal if you want more detail: check
result.status and result.signal and call process.exit(status) where status is
result.status if !== null, otherwise 1 (or a mapped code based on result.signal)
so build failures aren't treated as success.
---
Nitpick comments:
In @.gitignore:
- Around line 3-4: Remove the redundant `apps/web/.next/` entry since the
top-level `.next/` already ignores all `.next` directories; delete that line and
consolidate ignores, and augment the file with common entries such as
node_modules, .env, dist/build/output directories, .turbo, .parcel-cache,
.DS_Store, coverage, .idea, .vscode, and any editor/OS/tool-specific caches so
the repo ignores standard generated and sensitive files.
In `@apps/web/src/app/globals.css`:
- Around line 19-23: The stylesheet currently forces .desktop-suggestions to
display:block with !important, which hides the fact that the base state is
expected to be hidden elsewhere; remove the !important and instead document the
base state and/or implement a clear toggle class (e.g., .desktop-suggestions and
.desktop-suggestions--visible) or increase selector specificity to override the
base rule; update the CSS around the .desktop-suggestions rule to either include
a comment describing the expected base display (hidden) or replace the pattern
with a visibility-toggle class and adjust any JS that shows/hides suggestions to
use that class.
- Around line 14-17: The global rule removing link underlines (selector "a" in
globals.css) reduces accessibility; update the CSS so links keep a visible
indicator — for example, restore text-decoration for :hover and
:focus/:focus-visible states (or add an outline or border-bottom on those
states) and ensure the link color meets contrast requirements against the
background; modify the "a" selector and add "a:hover, a:focus, a:focus-visible"
rules (or equivalent classes) to provide a clear visual cue on interaction while
keeping the base color inheritance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9c57340-57a5-42ae-9283-7c3695df7b4a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
pnpm-workspace.yaml.gitignoreapps/web/next-env.d.tsapps/web/next.config.mjsapps/web/package.jsonapps/web/src/app/globals.cssapps/web/src/app/hotel/[id]/page.tsxapps/web/src/app/layout.tsxapps/web/src/app/page.tsxapps/web/tsconfig.jsonpackage.jsonpnpm-workspace.yamlscripts/build.mjsturbo.json
apps/web/next.config.mjs
Outdated
| eslint: { | ||
| ignoreDuringBuilds: true, | ||
| }, | ||
| typescript: { | ||
| ignoreBuildErrors: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current Next build-check bypass flags:"
rg -n "ignoreDuringBuilds|ignoreBuildErrors" apps/web/next.config.mjs
echo
echo "Check whether the repo has separate lint/typecheck scripts as compensating controls:"
fd -i package.json | while read -r f; do
echo "== $f =="
rg -n "\"(lint|typecheck|check)\"\\s*:" "$f" || true
doneRepository: safetrustcr/dApp-SafeTrust
Length of output: 304
🏁 Script executed:
cat -n apps/web/package.jsonRepository: safetrustcr/dApp-SafeTrust
Length of output: 605
🏁 Script executed:
cat -n package.jsonRepository: safetrustcr/dApp-SafeTrust
Length of output: 269
🏁 Script executed:
fd -i "\.github|\.gitlab-ci|\.travis|Makefile|\.circleci" -t dRepository: safetrustcr/dApp-SafeTrust
Length of output: 52
🏁 Script executed:
fd -i "workflow|\.yml|\.yaml" -t f | grep -i "ci\|lint\|check\|test" | head -20Repository: safetrustcr/dApp-SafeTrust
Length of output: 52
🏁 Script executed:
cat -n scripts/build.mjsRepository: safetrustcr/dApp-SafeTrust
Length of output: 578
🏁 Script executed:
fd -type f | grep -E "(github|gitlab|travis|circle|workflow|\.yml|\.yaml|\.toml|\.config)" | head -30Repository: safetrustcr/dApp-SafeTrust
Length of output: 240
🏁 Script executed:
fd -type f -name ".*" | grep -v node_modules | head -30Repository: safetrustcr/dApp-SafeTrust
Length of output: 240
🏁 Script executed:
find . -maxdepth 2 -name ".*" -type f | grep -v "\.git" | head -20Repository: safetrustcr/dApp-SafeTrust
Length of output: 104
🏁 Script executed:
fd -e yml -e yaml -e json | grep -i "eslint\|tsconfig\|github\|gitlab" | head -20Repository: safetrustcr/dApp-SafeTrust
Length of output: 92
🏁 Script executed:
ls -la | head -30Repository: safetrustcr/dApp-SafeTrust
Length of output: 992
🏁 Script executed:
cat -n biome.jsonRepository: safetrustcr/dApp-SafeTrust
Length of output: 1713
🏁 Script executed:
cat -n turbo.jsonRepository: safetrustcr/dApp-SafeTrust
Length of output: 283
🏁 Script executed:
find . -path "./.git" -prune -o -type f \( -name "*.github" -o -name "*.yml" -o -name "*.yaml" \) -print | head -20Repository: safetrustcr/dApp-SafeTrust
Length of output: 129
🏁 Script executed:
find . -path "./.git" -prune -o -name ".husky" -type d -printRepository: safetrustcr/dApp-SafeTrust
Length of output: 52
🏁 Script executed:
find . -path "./.git" -prune -o -name "pre-commit*" -type f -printRepository: safetrustcr/dApp-SafeTrust
Length of output: 52
🏁 Script executed:
rg "lint|check|test" package.json scripts/ apps/web/package.jsonRepository: safetrustcr/dApp-SafeTrust
Length of output: 52
Do not suppress lint/type failures for production builds.
The ignoreDuringBuilds and ignoreBuildErrors settings on lines 4 and 7 allow next build to succeed with TypeScript/ESLint errors with no compensating controls. There are no lint scripts, pre-commit hooks, or CI/CD workflows to enforce these checks, meaning broken code can ship to production.
🔧 Suggested fix
/** `@type` {import('next').NextConfig} */
+const enforceChecks = process.env.CI === 'true' || process.env.NODE_ENV === 'production';
+
const nextConfig = {
eslint: {
- ignoreDuringBuilds: true,
+ ignoreDuringBuilds: !enforceChecks,
},
typescript: {
- ignoreBuildErrors: true,
+ ignoreBuildErrors: !enforceChecks,
},
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/next.config.mjs` around lines 3 - 8, Remove the
production-suppressing flags in next.config.mjs: do not set
eslint.ignoreDuringBuilds or typescript.ignoreBuildErrors to true (remove them
or set them to false) and instead enforce checks by adding a CI step and/or npm
scripts that run next lint and tsc --noEmit (and add pre-commit/pre-push hooks
if desired); update the project configuration so that functions referencing
eslint.ignoreDuringBuilds and typescript.ignoreBuildErrors no longer bypass
build failures and ensure the repository has a CI workflow that fails the build
on lint/type errors.
There was a problem hiding this comment.
Hello dear @Obiajulu-gif, could you apply this suggested fix:
/** `@type` {import('next').NextConfig} */
+const enforceChecks = process.env.CI === 'true' || process.env.NODE_ENV === 'production';
+
const nextConfig = {
eslint: {
- ignoreDuringBuilds: true,
+ ignoreDuringBuilds: !enforceChecks,
},
typescript: {
- ignoreBuildErrors: true,
+ ignoreBuildErrors: !enforceChecks,
},
};
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| </p> | ||
| </div> | ||
|
|
||
| <Link href={`/hotel/${params.id}/escrow/create`} style={styles.button}> |
There was a problem hiding this comment.
Encode dynamic path segments before building the escrow URL.
Line 161 should encode params.id to prevent malformed links when IDs contain reserved characters.
🔧 Suggested fix
- <Link href={`/hotel/${params.id}/escrow/create`} style={styles.button}>
+ <Link href={`/hotel/${encodeURIComponent(params.id)}/escrow/create`} style={styles.button}>
BOOK
</Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/hotel/`[id]/page.tsx at line 161, The Link href currently
interpolates params.id directly which can produce malformed URLs for IDs with
reserved characters; update the link construction in the component that renders
the Link (the element using Link href={`/hotel/${params.id}/escrow/create`}) to
URL-encode the dynamic segment by using encodeURIComponent on params.id so the
escrow URL is safe (e.g., build the href from
`/hotel/${encodeURIComponent(params.id)}/escrow/create`).
There was a problem hiding this comment.
Could you please apply this suggested fix:
- <Link href={`/hotel/${params.id}/escrow/create`} style={styles.button}>
+ <Link href={`/hotel/${encodeURIComponent(params.id)}/escrow/create`} style={styles.button}>
BOOK
</Link>
There was a problem hiding this comment.
@sotoJ24, sure! Here's the plan to apply the fix:
- Encode
params.idwithencodeURIComponentin theLinkhref to prevent malformed URLs when IDs contain reserved characters.
- 📌 Commit the changes to this branch
sotoJ24
left a comment
sotoJ24
left a comment
There was a problem hiding this comment.
Dear @Obiajulu-gif, could you please make the changes request by coderabbit?
|
@sotoJ24 i have fix it and please merge my code |
| </p> | ||
| </div> | ||
|
|
||
| <Link href={`/hotel/${params.id}/escrow/create`} style={styles.button}> |
There was a problem hiding this comment.
Could you please apply this suggested fix:
- <Link href={`/hotel/${params.id}/escrow/create`} style={styles.button}>
+ <Link href={`/hotel/${encodeURIComponent(params.id)}/escrow/create`} style={styles.button}>
BOOK
</Link>
apps/web/next.config.mjs
Outdated
| const nextConfig = { | ||
| eslint: { | ||
| ignoreDuringBuilds: true, | ||
| }, | ||
| typescript: { | ||
| ignoreBuildErrors: true, |
apps/web/next.config.mjs
Outdated
| eslint: { | ||
| ignoreDuringBuilds: true, | ||
| }, | ||
| typescript: { | ||
| ignoreBuildErrors: true, | ||
| }, |
There was a problem hiding this comment.
Hello dear @Obiajulu-gif, could you apply this suggested fix:
/** `@type` {import('next').NextConfig} */
+const enforceChecks = process.env.CI === 'true' || process.env.NODE_ENV === 'production';
+
const nextConfig = {
eslint: {
- ignoreDuringBuilds: true,
+ ignoreDuringBuilds: !enforceChecks,
},
typescript: {
- ignoreBuildErrors: true,
+ ignoreBuildErrors: !enforceChecks,
},
};
Obiajulu-gif
force-pushed
the
feat/hotel-detail-route-placeholder
branch
from
9062dc2 to
b37ab78
Compare
|
@sotoJ24 merge my code |
sotoJ24
left a comment
sotoJ24
left a comment
There was a problem hiding this comment.
Dear @Obiajulu-gif, please fix your pull request so we can merge it, in this PR just need to update the PR
This pull request sets up the initial structure for the SafeTrust web application using Next.js, TypeScript, and pnpm workspaces. It includes configuration files for the monorepo, the web app, and build scripts, as well as basic placeholder pages and styles for the apartment booking flow.
Monorepo and Build System Setup:
package.jsonwith a build script and set up pnpm workspaces viapnpm-workspace.yaml. [1] [2]scripts/build.mjsscript to build the web app using pnpm, supporting both Windows and Unix platforms.turbo.jsonfor managing build tasks and caching with Turborepo.Web Application Configuration:
apps/web/package.jsonwith Next.js, React, and TypeScript dependencies and scripts.apps/web/tsconfig.jsonwith strict TypeScript settings and Next.js plugin.apps/web/next.config.mjsto ignore TypeScript and ESLint errors during builds.Core Application Files and Styles:
apps/web/src/app/globals.css.apps/web/src/app/layout.tsxwith metadata and global styles import.apps/web/src/app/page.tsx.apps/web/src/app/hotel/[id]/page.tsxwith hardcoded data and TODOs for future components. (apps/web/src/app/hotel/[id]/page.tsxR1-R189)Closes #70
Summary by CodeRabbit
Release Notes