Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR transitions troubleshoot host detection from environment variable-based comparison to hardcoded heuristic rules across multiple files. It also creates a new generic LoadingSpinner component, updates related styling in modal and loader overlays, and adjusts the default active tab in admin troubleshooting from sequencer to products. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
![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)
middleware.ts (1)
16-24: 🛠️ Refactor suggestion | 🟠 MajorDead code:
TROUBLESHOOT_HOSTNAMEis defined but never used.The environment variable on line 16 is no longer referenced in
isTroubleshootHost. Additionally, the comment on line 21 mentions "if env is set" but the environment variable is not actually checked. Either remove the unused variable and update the comment, or incorporate it into the logic as a fallback.🧹 Option 1: Remove unused variable and fix comment
-const TROUBLESHOOT_HOSTNAME = process.env.TROUBLESHOOT_HOSTNAME || 'hemmts.academythriveni.com'; - function isTroubleshootHost(host: string | null): boolean { if (!host) return false; const hostname = host.toLowerCase().split(':')[0]; - // Favor production hostname, but allow 'troubleshoot' prefix for local dev if env is set + // Match production hostname or 'troubleshoot' prefix for local dev return hostname === 'hemmts.academythriveni.com' || hostname.includes('hemmts') || hostname.startsWith('troubleshoot'); }🧹 Option 2: Use env var as configurable production hostname
const TROUBLESHOOT_HOSTNAME = process.env.TROUBLESHOOT_HOSTNAME || 'hemmts.academythriveni.com'; function isTroubleshootHost(host: string | null): boolean { if (!host) return false; const hostname = host.toLowerCase().split(':')[0]; - // Favor production hostname, but allow 'troubleshoot' prefix for local dev if env is set + const prodHost = TROUBLESHOOT_HOSTNAME.toLowerCase().split(':')[0]; + // Match configured hostname, 'hemmts' substring, or 'troubleshoot' prefix for local dev - return hostname === 'hemmts.academythriveni.com' || + return hostname === prodHost || hostname.includes('hemmts') || hostname.startsWith('troubleshoot'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware.ts` around lines 16 - 24, The TROUBLESHOOT_HOSTNAME constant is declared but never used and the comment wrongly states an env-var check; fix by either removing TROUBLESHOOT_HOSTNAME and updating the comment in isTroubleshootHost(host) to reflect the actual matching rules, or incorporate the env var into the matching logic by replacing the hard-coded 'hemmts.academythriveni.com' check with TROUBLESHOOT_HOSTNAME (keep the other checks hostname.includes('hemmts') and hostname.startsWith('troubleshoot')), and ensure the comment mentions the env-configurable production hostname.
🧹 Nitpick comments (3)
components/admin/troubleshooting/AdminTabs.tsx (1)
27-33: Consider typing tab IDs to prevent stale tab states.Given Line 25 and the tab definitions here,
activeTabis currently an unconstrained string. A typed tab-id union would prevent invalid states and make hidden branches (likecauses) explicit at compile-time.Optional typed-state refactor
+type TabId = 'products' | 'faults' | 'sequencer' | 'import'; -const [activeTab, setActiveTab] = useState('products'); +const [activeTab, setActiveTab] = useState<TabId>('products'); -const tabs = [ +const tabs: { id: TabId; label: string; icon: React.ComponentType<{ size?: number }> }[] = [ { id: 'products', label: 'Machine Manager', icon: HiOutlineSquare3Stack3D }, { id: 'faults', label: 'Fault Manager', icon: HiOutlineExclamationTriangle }, { id: 'sequencer', label: 'Cause Manager', icon: HiOutlineRectangleGroup }, { id: 'import', label: 'Bulk Import', icon: HiOutlineCloudArrowUp }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/admin/troubleshooting/AdminTabs.tsx` around lines 27 - 33, activeTab is currently an unconstrained string which allows stale/invalid tab states; define a TabId union type (e.g., 'products' | 'faults' | 'sequencer' | 'import' [and include 'causes' if you want it explicit]) and use it to type the tabs array entries and the activeTab state in the AdminTabs component (references: tabs array, activeTab variable, AdminTabs). Update any handlers (e.g., tab click/setActiveTab) and prop types to use TabId so the compiler prevents invalid IDs and makes hidden tabs like 'causes' explicit.components/LoadingSpinner.tsx (1)
1-47: Consider consolidating withcomponents/troubleshoot/LoadingSpinner.tsx.This component is nearly identical to
components/troubleshoot/LoadingSpinner.tsx— the only difference is the loading text ("Loading Training Portal" vs "Loading"). Consider extracting a shared base component with a customizablelabelprop to reduce duplication.♻️ Proposed shared component approach
// components/LoadingSpinner.tsx -export default function LoadingSpinner() { +interface LoadingSpinnerProps { + label?: string; +} + +export default function LoadingSpinner({ label = "Loading" }: LoadingSpinnerProps) { return ( <div className="fixed inset-0 w-screen h-[100dvh] z-[9999] flex flex-col items-center justify-center bg-white/90 backdrop-blur-3xl animate-in fade-in duration-300"> {/* ... logo content ... */} <p className="mt-4 text-[10px] font-black text-slate-400 uppercase tracking-[0.4em] animate-pulse"> - Loading Training Portal + {label} </p> </div> ); }Then in
app/loading.tsx:import LoadingSpinner from "@/components/LoadingSpinner"; export default function Loading() { return <LoadingSpinner label="Loading Training Portal" />; }And in troubleshoot contexts:
<LoadingSpinner label="Loading" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/LoadingSpinner.tsx` around lines 1 - 47, The two nearly identical components should be consolidated: extract a single LoadingSpinner component (keep the existing LoadingSpinner function/component name) that accepts a label prop (e.g., label: string, with a default of "Loading") and uses that prop in place of the hardcoded "Loading Training Portal" text, then replace the duplicate file in components/troubleshoot/LoadingSpinner.tsx (or import the shared one) and update callers (including app/loading.tsx) to pass label="Loading Training Portal" or label="Loading" as appropriate; ensure Image usages, classNames and animations remain unchanged and preserve the fill/priority props.app/manifest.ts (1)
7-9: Unused variable:refereris fetched but never used.The
refererheader is retrieved on line 7 but is not used anywhere in the function. Consider removing it to avoid confusion.🧹 Proposed fix
const headersList = await headers(); const host = headersList.get('host') || ''; - const referer = headersList.get('referer') || ''; const currentHost = host.split(':')[0].toLowerCase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/manifest.ts` around lines 7 - 9, The variable referer is read from headersList via headersList.get('referer') but never used; remove the unused retrieval to avoid dead code by deleting the referer declaration (the const referer = ... line) in manifest.ts so only currentHost and isTroubleshoot remain, and ensure no other code relies on referer elsewhere (search for referer references) before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/layout.tsx`:
- Around line 20-23: The host comparison in layout.ts is inconsistent because
host may include a port; update the host normalization where host is defined
(the host constant) to strip any port suffix before lowercasing—mirror the logic
used in middleware.ts and app/manifest.ts—then recompute isTroubleshoot using
that normalized host so the equality check against 'hemmts.academythriveni.com'
and other startsWith/includes checks behave as expected.
In `@components/LoadingSpinner.tsx`:
- Line 8: The div in the LoadingSpinner component uses a non-standard Tailwind
class "w-100"; replace it with a standard utility that matches the intended
width (for example use "w-full" for 100% width or a fixed value like "w-[100px]"
or "w-40"/"w-64" if a specific pixel/rem size was intended) by updating the
className on the div inside LoadingSpinner.tsx accordingly.
---
Outside diff comments:
In `@middleware.ts`:
- Around line 16-24: The TROUBLESHOOT_HOSTNAME constant is declared but never
used and the comment wrongly states an env-var check; fix by either removing
TROUBLESHOOT_HOSTNAME and updating the comment in isTroubleshootHost(host) to
reflect the actual matching rules, or incorporate the env var into the matching
logic by replacing the hard-coded 'hemmts.academythriveni.com' check with
TROUBLESHOOT_HOSTNAME (keep the other checks hostname.includes('hemmts') and
hostname.startsWith('troubleshoot')), and ensure the comment mentions the
env-configurable production hostname.
---
Nitpick comments:
In `@app/manifest.ts`:
- Around line 7-9: The variable referer is read from headersList via
headersList.get('referer') but never used; remove the unused retrieval to avoid
dead code by deleting the referer declaration (the const referer = ... line) in
manifest.ts so only currentHost and isTroubleshoot remain, and ensure no other
code relies on referer elsewhere (search for referer references) before
committing.
In `@components/admin/troubleshooting/AdminTabs.tsx`:
- Around line 27-33: activeTab is currently an unconstrained string which allows
stale/invalid tab states; define a TabId union type (e.g., 'products' | 'faults'
| 'sequencer' | 'import' [and include 'causes' if you want it explicit]) and use
it to type the tabs array entries and the activeTab state in the AdminTabs
component (references: tabs array, activeTab variable, AdminTabs). Update any
handlers (e.g., tab click/setActiveTab) and prop types to use TabId so the
compiler prevents invalid IDs and makes hidden tabs like 'causes' explicit.
In `@components/LoadingSpinner.tsx`:
- Around line 1-47: The two nearly identical components should be consolidated:
extract a single LoadingSpinner component (keep the existing LoadingSpinner
function/component name) that accepts a label prop (e.g., label: string, with a
default of "Loading") and uses that prop in place of the hardcoded "Loading
Training Portal" text, then replace the duplicate file in
components/troubleshoot/LoadingSpinner.tsx (or import the shared one) and update
callers (including app/loading.tsx) to pass label="Loading Training Portal" or
label="Loading" as appropriate; ensure Image usages, classNames and animations
remain unchanged and preserve the fill/priority props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4347ee9e-d9ef-4657-8f44-a4c9002e789c
📒 Files selected for processing (9)
app/layout.tsxapp/loading.tsxapp/manifest.tsapp/troubleshoot/TroubleshootReport.tsxcomponents/LoadingSpinner.tsxcomponents/admin/troubleshooting/AdminTabs.tsxcomponents/troubleshoot/LoadingSpinner.tsxcomponents/troubleshoot/TroubleshootNavbar.tsxmiddleware.ts
| const host = (headersList.get("host") || "").toLowerCase(); | ||
| const isTroubleshoot = host.startsWith('troubleshoot') || | ||
| host.includes('hemmts') || | ||
| host === 'hemmts.academythriveni.com'; |
There was a problem hiding this comment.
Inconsistent port handling compared to other files.
Unlike middleware.ts (line 20) and app/manifest.ts (line 8), this file doesn't strip the port from the host header before comparison. If the host includes a port (e.g., hemmts.academythriveni.com:3000), the exact equality check on line 23 will fail.
While the includes('hemmts') check on line 22 would still match in most cases, it's better to be consistent across all files.
🔧 Proposed fix
export async function generateMetadata(): Promise<Metadata> {
const headersList = await headers();
- const host = (headersList.get("host") || "").toLowerCase();
+ const host = (headersList.get("host") || "").toLowerCase().split(':')[0];
const isTroubleshoot = host.startsWith('troubleshoot') ||
host.includes('hemmts') ||
host === 'hemmts.academythriveni.com';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const host = (headersList.get("host") || "").toLowerCase(); | |
| const isTroubleshoot = host.startsWith('troubleshoot') || | |
| host.includes('hemmts') || | |
| host === 'hemmts.academythriveni.com'; | |
| const host = (headersList.get("host") || "").toLowerCase().split(':')[0]; | |
| const isTroubleshoot = host.startsWith('troubleshoot') || | |
| host.includes('hemmts') || | |
| host === 'hemmts.academythriveni.com'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/layout.tsx` around lines 20 - 23, The host comparison in layout.ts is
inconsistent because host may include a port; update the host normalization
where host is defined (the host constant) to strip any port suffix before
lowercasing—mirror the logic used in middleware.ts and app/manifest.ts—then
recompute isTroubleshoot using that normalized host so the equality check
against 'hemmts.academythriveni.com' and other startsWith/includes checks behave
as expected.
| export default function LoadingSpinner() { | ||
| return ( | ||
| <div className="fixed inset-0 w-screen h-[100dvh] z-[9999] flex flex-col items-center justify-center bg-white/90 backdrop-blur-3xl animate-in fade-in duration-300"> | ||
| <div className="relative flex items-center justify-center w-100 h-40"> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 w-100 utility class
💡 Result:
No, Tailwind CSS v4 does not have a predefined w-100 utility class for a fixed spacing-based width. In v4, width utilities like w-* use a dynamic multiplier system based on the --spacing CSS variable (default 0.25rem), so w-100 would set width: calc(var(--spacing) * 100), which equals 25rem (400px) and works for any whole number or .5 increments out of the box without config. However, this is not a "standard" or highlighted class in docs, which show examples up to w-96. For 100% width, use the standard w-full utility, which explicitly sets width: 100%. To use an exact 100px width (like Bootstrap's w-100), apply the arbitrary value class w-[100px].
Citations:
- 1: https://tailwindcss.com/docs/width
- 2: https://tailwindcss.com/blog/tailwindcss-v4
- 3: https://tailwindcss.com/docs/size
- 4: [v4] Dynamically generated margin, padding, width, height, etc.; does not work with decimal numbers tailwindlabs/tailwindcss#16229
- 5: Replace default explicit spacing scale with multiplier system tailwindlabs/tailwindcss#14857
- 6: https://tailwindcss.com/docs/width/
Use a standard Tailwind width utility instead of w-100.
w-100 is not a documented Tailwind utility class. While it technically works in v4 due to dynamic spacing calculations (resolving to width: 25rem or 400px), it's non-standard and unclear. Use w-full for 100% width, w-[100px] for 100px, or another standard utility class that matches your intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/LoadingSpinner.tsx` at line 8, The div in the LoadingSpinner
component uses a non-standard Tailwind class "w-100"; replace it with a standard
utility that matches the intended width (for example use "w-full" for 100% width
or a fixed value like "w-[100px]" or "w-40"/"w-64" if a specific pixel/rem size
was intended) by updating the className on the div inside LoadingSpinner.tsx
accordingly.
1 participant
Summary by CodeRabbit
New Features
Style