Details
- Reviewers
ieli
Diff Detail
- Repository
- rSP ServPulse
- Lint
Lint Skipped - Unit
No Test Coverage - Branch
- feature/404-error-page
- Build Status
Buildable 6370 Build 6654: arc lint + arc unit
Event Timeline
This revision should only contain the NotFound.vue page and its catch-all route — that's the task. Please strip out the unrelated changes (navbar logo rework, SubscribeForm restyling, logo.svg replacement, package-lock.json removals) into their own separate revisions. See inline comments for specific fixes needed on indentation and missing newlines.
Also, please add me as a reviewer when you create a revision (run arc diff --nolint --reviewers ieli works I believe, otherwise just add me manually in Phabricator). That way I get notified and can review sooner.
| frontend/package-lock.json | ||
|---|---|---|
| 695–4392 ↗ | (On Diff #10293) | This removes @parcel/watcher, sass, chokidar, detect-libc, immutable, node-addon-api, and readdirp. These are likely side-effects of running npm install on a different OS/environment. ⚠️ Please revert this file or explain why these removals are intentional. |
| frontend/src/assets/logo.svg | ||
| 1–235 ↗ | (On Diff #10293) | This is a .svg file but it now contains a full Vue Single File Component (<template>, <script setup>, ref, computed, requestAnimationFrame…). A .svg file won't be processed by Vite/Vue as a component, this needs to be a .vue file if it's meant to be a component. ⚠️ Also, this is unrelated to the NotFound task, please move it to its own revision. Missing trailing newline too. |
| frontend/src/components/AppNavbar.vue | ||
| 19–95 ↗ | (On Diff #10293) | This entire logo feature (custom logo via config + Vue logo fallback) is unrelated to the NotFound task. ⚠️ Please split into a separate revision. Also: the Vue.js framework logo should not be the fallback, end users shouldn't see a framework logo. Either show nothing or use a ServPulse icon. |
| 62 ↗ | (On Diff #10293) | The interpolation and </span> closing tag lost their indentation. The <span> opens at 10 spaces, so the content should be at 12 and </span> at 10. Right now they're at 4 and 2, looks like a copy-paste alignment issue. |
| frontend/src/components/SubscribeForm.vue | ||
| 37–125 ↗ | (On Diff #10293) | These are cosmetic/layout changes (centering, max-w-md, p-5→p-6, full-width buttons) unrelated to the NotFound page. ⚠️ Please move to a separate revision. |
| 124 ↗ | (On Diff #10293) | ⚠️ Missing trailing newline at end of file. POSIX standard requires it, some tools (diff, cat, shell) may misbehave without it, and our other .vue files all have one. {F24438978} |
| frontend/src/router/index.js | ||
| 35–37 | Indentation is inconsistent with the rest of the routes array. Should use 6 spaces (2 for the array + 4 for the object), matching the pattern of all other routes above. | |
| 38 | Inconsistent end of bracket. | |
| frontend/src/views/NotFound.vue | ||
| 26 | ⚠️ Missing trailing newline at end of file. | |
I've reviewed your changes. I noticed you created a new differential (D3977) instead of updating this one (D3971) for the same task.
When making revisions based on review feedback, you should amend the existing differential rather than creating a new one. This keeps the review history in one place and avoids duplicate diff IDs.
For future reference:
- Use arc diff to update the existing differential with your changes
- Only create a new differential if it's a genuinely separate/unrelated task
Creating separate differentials for the same task is the fastest and most reliable way to chaos and loss of control over a project. It fragments the review history, makes it impossible to track what was actually merged, and creates confusion about which version is canonical. In team environments, this quickly becomes unmanageable.
| frontend/src/router/index.js | ||
|---|---|---|
| 35–37 | Indentation issues still present when I pull your diff locally. | |
| 44–66 | Indentation issues still present when I pull your diff locally. | |
| 46–54 | Indentation issues still present when I pull your diff locally. | |
| 59 | The export default router appears twice. This causes a syntax error. | |
| frontend/src/views/__tests__/NotFound.test.js | ||
| 7–17 | Indentation issues still present | |
| frontend/src/views/__tests__/NotFound.test.js | ||
|---|---|---|
| 18 | Consider also testing the RouterLink to verify it navigates to /. You can stub RouterLink with a custom component that exposes the to prop. | |
Fixed IDE 2-space formatting across all files, added RouterLink stub to tests, and ensured trailing newlines.