Skip to content

feat(lab): add personal workspace page - #194

Draft
16th-admin wants to merge 5 commits into
mainfrom
codex/issue-53-my-lab
Draft

feat(lab): add personal workspace page#194
16th-admin wants to merge 5 commits into
mainfrom
codex/issue-53-my-lab

Conversation

@16th-admin

@16th-admin 16th-admin commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Closes #53

  • Add a dedicated, localized My Lab route and footer entry.
  • Load the signed-in user's Workspace library in the active language and reload it on each visit.
  • Handle mixed topic/list blocks, empty results, authentication requirements, and request failures safely.
  • Route editable works to the editor and read-only works to their summary page.

Validation: changed-file ESLint; filtered type-check; git diff --check.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

PR Review: feat(lab): add personal workspace page

Commits: 6bbb2d4 (feat) + 81d08c6 (fix) — files: src/views/MyLab.vue (new), src/router/index.ts, src/components/utils/Footer.vue

Summary

Adds a new /lab "My Lab" page that fetches the user's workspace via POST /Contents/GetLibrary (Identifier: WI have a complete picture of the PR. Let me write the review report. Review complete — report written to review_report.md. Key findings for PR #194` (My Lab page):

High: MyLab.vue:37-42getData throws on network failure, and the onMounted callback has no try/catch, so loading stays true forever → infinite "Loading…". Should use try/catch/finally + the repo's showAPiError pattern (BlackHole.vue:114).

Medium:

  • Hardcoded strings (My Lab, Loading…, Edit, footer 我的) violate the fully-localized repo (recent commit "remove hard-coded text Scan hard-coded text #178"); add i18n keys.
  • .my-lab { min-height: 100dvh } clips content and hides it under the fixed footer — repo pattern is calc(100dvh - 50px) + overflow-y (startPage.css).
  • keepAlive: true + onMounted → stale workspace list on return to /lab.

Verified OK: the v-for + v-else combo compiles to correct codegen (tested with @vue/compiler-sfc); second commit's type-widening fix is sound. Also noted low-priority items (empty state, auth guard, prettier formatting).
showAPiError(...)(with retry) and the repo error-logging helpers. Fix: wrap intry/catch/finally(and considershowAPiErrorfrom@popup/index.ts` for a consistent retry UX).

Medium

2. Hardcoded strings break the i18n conventionMyLab.vue:3,4,11,40 and Footer.vue:41
My Lab, Loading…, Edit, Unable to load your lab. and the footer's 我的 are hardcoded, while every sibling footer item uses $t('footer.*') and the app ships 5 locales (zh/en/de/fr/ja). The repo recently merged "remove hard-coded text (#178)". Add footer.myLab and lab-page keys to all locale files and use $t(). Note: the CI check-i18n.mjs only validates key parity between locales, so this won't be caught by CI — it's a manual-review catch.

3. Layout clips content and overlaps the fixed footerMyLab.vue:46
#app is height: 100dvh; overflow: hidden (App.vue:30-37) and the footer is fixed at bottom: 0. .my-lab { min-height: 100dvh } means content taller than the viewport is clipped with no scrolling, and the bottom 50px is covered by the footer. The established pattern is height: calc(100dvh - 50px) + overflow-y scrolling (see startPage.css:3-11, Comments.vue:108, Editor.vue:720). Follow that.

4. Stale data when returning to the page (keep-alive)MyLab.vue:37
The route sets meta: { keepAlive: true } and the cached component is keyed by $route.fullPath (App.vue:8), so navigating back to /lab reuses the instance and onMounted does not re-run. After a user edits/deletes a work in the Editor, the workspace list will be stale. Home.vue has the same pattern but refreshes on login events; for a "my works" page this is user-visible. Consider refreshing on onActivated (or onBeforeRouteUpdate).

Low

  • No empty state: with zero blocks the page renders nothing after loading; a friendly "no works yet" state would help.
  • No auth guard: the footer now exposes "我的" to logged-out users, who will see a raw API message instead of a login prompt. getData will emit loginRequired on 403 (getData.ts:23-27), but the page still surfaces a bare server message.
  • Prettier not applied: both changed files fail prettier --check (multiline import type, getData call arg, CSS blocks, SVG attributes). Not enforced in CI, but npm run format is the repo convention.
  • key="block.Header" assumes headers are unique across blocks; an index-based key is safer.

Verdict

Solid, small feature that follows most Vue 3 patterns and includes a genuine robustness fix in the follow-up commit. However, the missing error handling (#1) can leave users on an infinite loading screen, and the hardcoded strings (#2) and footer-overlapping layout (#3) go against clear repo conventions. Recommend addressing #1#4 before merge.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

I'll start by examining the PR context and understanding the changes
Let me inspect the dependencies referenced in the
Let me check the Editor route and how existing views navigate to it, plus checkLogin's
Let me check the i18n typing setup and Footer context, and verify the v-else/v-for combination
Let me check the build/lint setup and verify if the v-else + v-for combination produces compiler
Let me verify a couple details: how BlackHole passes the Language param, and whether `mdEditor.loginRequired
I have enough context to complete the review.

PR Review: feat(lab): add personal workspace page

Scope reviewed: src/views/MyLab.vue (new), src/components/utils/Footer.vue, src/router/index.ts, and i18n files (de/en/fr/ja/zh). Skipped lockfiles/build artifacts. Total: 8 files, +147/−12.

Overall the change is small, well-structured, uses the Composition API (<script setup>), typed props/state, lazy route loading, and adds full i18n coverage across all 5 locales. The getData('/Contents/GetLibrary') call and route constants align with existing patterns. There are a few correctness and code-quality issues worth addressing.


1. (Correctness) v-else combined with v-for on the same element — src/views/MyLab.vue:7

<p v-if="loading"></p>
<p v-else-if="error"></p>
<p v-else-if="visibleBlocks.length === 0"></p>
<section v-for="block in visibleBlocks" v-else :key="block.Header" >

Putting v-else and v-for on the same element is an unsupported/ambiguous combination in Vue 3 — the branch and iterator both target one element, producing a compiler warning and fragile precedence. The idiomatic and non-warn fix is to move the iterator onto a child in across a <template v-else> wrapper:

<p v-else-if="visibleBlocks.length === 0">{{ t('myLab.empty') }}</p>
<template v-else>
  <section v-for="block in visibleBlocks" :key="block.Header" class="lab-section"></section>
</template>

This also de-duplicates intent: visibleBlocks already excludes empty blocks, so the guard visibleBlocks.length === 0 cleanly becomes the v-else default.

2. (Design) Redundant hidden span in the Footer — src/components/utils/Footer.vue:50-51 + CSS

<span>我的</span>
<span class="lab-label">{{ $t('footer.myLab') }}</span>

The hard-coded 我的 span is permanently hidden via .lab-link > span{display:none} / .lab-link > .lab-label{display:inline}. This CSS-driven hide/show pair just to swap a localized label is a fragile hack. Keep a single span:

<span>{{ $t('footer.myLab') }}</span>

and remove the 我的. The .lab-link > span selector is also overly broad (it would silently hide any future child span).

3. (Minor) Defensive access on block.Summariessrc/views/MyLab.vue:34

const visibleBlocks = computed(() => blocks.value.filter((block) => block.Summaries.length > 0))

ListBlock | TopicBlock both declare Summaries: Summary[] as required, so TS is satisfied — but the two preceding commits in this PR are titled “handle mixed workspace blocks safely”. Since the whole point is hardening against mixed/malformed blocks, guard against a missing/undefined Summary array too (e.g. block.Summaries?.length > 0) to avoid a runtime throw on a malformed payload.

(Nit) Reused i18n key semantics

  • MyLab.vue:42 reuses mdEditor.loginRequiredContent (“Log in before editing works.”) for the My-Lab login gate; a dedicated myLab.loginRequired message would read more accurately.
  • MyLab.vue:3 reuses footer.myLab for the page <h1>; a myLab.title key avoids coupling the page heading to the nav label’s casing.

Positives

  • Composition API with explicit/typed ref, computed, onMounted; no reactivity leaks; void router.push for floating promises; route lazy-loaded with consistent names/meta.
  • Full i18n parity (all 5 locale files updated with identical key sets, matching the check-i18n invariant).
  • Language correctly passed from locale.value rather than hard-coded (an improvement over BlackHole.vue).
  • Scoped styles, mobile-friendly 100dvh, semantic <main>/<section>/<article> markup.

Suggested quick wins

  1. Wrap the iterating <section> in <template v-else> (removes the v-else+v-for dilemma).
  2. Delete the dead 我的 span and the .lab-link CSS override.
  3. Optional: block.Summaries?.length defensively in the computed.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

PR Review: Personal Workspace (My Lab) Page

Scope: 4 commits (feat(lab): add personal workspace page + 3 follow-up fixes), touching MyLab.vue (new), Footer.vue, router/index.ts, and 5 i18n files.

Decision: 💬 Approve with comments — no blockers; solid, small, well-localized feature. Two functional/UX items worth addressing.


🔴 Blocking

None.

🟡 Important

1. keepAlive: true + data loaded only in onMounted → stale workspace

src/router/index.ts:24 sets meta: { keepAlive: true }, and App.vue caches the component keyed by $route.fullPath inside <keep-alive>. Since /lab has a stable fullPath, onMounted runs once; on every subsequent visit the cached instance is reused and onActivated fires instead. This is a user-own workspace whose content changes whenever the user edits/saves works — after visiting the Editor and returning to /lab, the list will show stale data until a full page reload.

The codebase already has the established pattern: Friends.vue and Home.vue do their refresh/login logic in onActivated. Suggest loading in onActivated (guarded to avoid double-fetch on first mount) or setting keepAlive: false for this route.

2. v-for + v-else on the same element

src/views/MyLab.vue:16<section v-for="block in visibleBlocks" v-else ...>. This compiles (Vue 3 gives v-if priority over v-for), but the Vue docs explicitly discourage combining the two directives, and it's fragile: any future reordering of the sibling <p v-else-if> chain silently changes behavior. Cleaner:

<template v-else>
  <section v-for="block in visibleBlocks" :key="block.Header" class="lab-section">...</section>
</template>

3. visibleBlocks computed lacks a null-guard on Summaries

src/views/MyLab.vue:33blocks.value.filter((block) => block.Summaries.length > 0) throws a TypeError if the server ever returns a block with Summaries: null/undefined. Given the commit 81d08c6 is literally titled "handle mixed workspace blocks safely", this is the one place the safety is missing:

const visibleBlocks = computed(() =>
  blocks.value.filter((block) => (block.Summaries ?? []).length > 0),
)

🟢 Nit

  • :key="block.Header" — a localized string used as a v-for key; two sections sharing a header would collide. Prefer an index or a stable block identifier.
  • Reusing mdEditor.loginRequiredContent ("Log in before editing works.") as the not-logged-in message for this page is semantically off. A dedicated myLab.loginRequired key would be clearer.
  • summary.Subject is optional (Subject?: string in main.ts:247). Multilingual works may render an empty card title; consider falling back to LocalizedSubject as ExperimentSummary.vue does.
  • No page-view analytics — other keepAlive views emit window.$Logger.logPageView in onActivated; /lab omits it.
  • result.Message rendered raw — server text is unlocalized; acceptable, but the localized myLab.loadFailed fallback covers the main case.

✅ What's good

  • i18n keys added to all 5 locale files with matching structure, consistent with the check-i18n.mjs script — nice.
  • Error handling is hardened with try/catch/finally; checkLogin(true) gate works and reuses the existing dialog flow.
  • The footer change cleanly replaces the commented-out stub (Footer.vue:36-51) instead of duplicating it.
  • Type widening to Array<ListBlock | TopicBlock> and the category || 'Discussion' default in edit() (MyLab.vue:37) correctly handle mixed workspace blocks.
  • Idiomatic Composition API: typed refs/computed, getData's typed overload reused, useI18n locale passed consistently with postComment.ts (Language: locale.value).

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex-Style Code Review by OpenCode (Vue Best Practice Enabled)

💡 Autonomous AI Reviewer inspecting git commit history and Vue code quality.

Review complete. Report written to review_report.md. Summary:

PR #194 — Personal Workspace (MyLab) page. Clean, well-typed Composition API code with good i18n coverage (all 5 locales pass check-i18n).

Key findings:

  1. [High] Unscrollable layoutbody/#app are overflow: hidden; height: 100dvh (src/App.vue:29), so .my-lab's min-height: 100dvh without overflow-y means long content gets clipped with no scroll. Must follow the .block-container pattern (scroll container + bottom footer clearance).

  2. [Medium] Missing <Footer> — the page is a footer nav destination but doesn't render <Footer> like Home/BlackHole/Friends/Notifications, so tabs vanish on /lab.

  3. [Low] Redundant login UXcheckLogin(true) shows a modal and the inline loginRequired error both fire.

  4. [Low] Index-based key`${block.$type}-${blockIndex}` is static-list-only safe.

  5. [Nits] @services/../pl-serve-type-main/... import path, hardcoded colors, Category || 'Discussion' fallback (consistent with cloudWorks.ts).

Not verified by running typecheck/lint (no node_modules in this environment); scripts/check-i18n.mjs was run directly and passes.
ight: calc(100dvh - 50px); /* minus fixed footer */
overflow-y: auto;
box-sizing: border-box;
padding: 2rem 2rem calc(2rem + 50px);
}


### 2. [Medium] The page does not render `<Footer>`
`MyLab.vue` is the destination of a new footer nav link, but unlike the other tab views (`Home`, `BlackHole`, `Friends`, `Notifications`) it never renders `<Footer>` (footer is per-view, not global). On `/lab` the user cannot switch tabs via the bottom nav and the fixed footer is simply absent. Add `<Footer />` (and the related bottom margin/clearance from issue #1).

### 3. [Low] Redundant login handling
`onMounted` calls `checkLogin(true)`, which already shows a modal (and navigates Home on confirm), and *also* sets `error.value = t('myLab.loginRequired')`, showing a second inline message. Consider using `checkLogin(false)` here so only one affordance appears, or keep the inline message and drop the dialog.

### 4. [Low] Index-based block `key`
`v-for` key is `` `${block.$type}-${blockIndex}` ``. `$type` may be absent for unexpected block variants, and index-based keys are unstable if blocks are ever reordered. Works today because the list is static, but a more stable key (e.g. `block.Header + blockIndex`, or an index alone) is preferable.

### 5. [Nit] Cleanup / style
- Import path `@services/../pl-serve-type-main/type/main` works but is fragile; it matches the existing `cloudWorks.ts` convention, so at minimum worth a `// eslint`-clean alias (`@/pl-serve-type-main/...`).
- Hardcoded colors (`#f4f7fb`, `#fff`, `rgb(0 0 0 / 12%)`) and no `box-sizing: border-box` on `.my-lab`; the rest of the app uses ad-hoc CSS too, so this is consistent, just noting it.
- `openWork`'s fallback `summary.Category || 'Discussion'` matches `toEditorWork` in `cloudWorks.ts` — good consistency.

---

## Things done well
- `script setup` + TS, typed `getData('/Contents/GetLibrary', …)` with `result.Status === 200 && result.Data` guard.
- `computed` `visibleBlocks` filters out empty blocks defensively (`block.Summaries?.length`).
- `void router.push(...)` — promise handled, no floating-rejection lint noise.
- Lazy-loaded route with `keepAlive: false`, consistent with other editor-like views.
- `canEditSummary` reused from `cloudWorks.ts` so edit/view routing logic is centralized.
- All five locale files carry the new `footer.myLab` and `myLab.*` keys; `node scripts/check-i18n.mjs` passes.
- No reactivity leaks: single `onMounted` fetch, aborts handled by the page-lifecycle controller in `getData`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

My-lab page

1 participant