fix(nav): add in-app back button and prevent LAST_DOC_ID from drifting on every navigation - #67
Conversation
PeterGuy326
left a comment
There was a problem hiding this comment.
REQUEST CHANGES\n\n1. src/app/[locale]/work/[id]/(content)/content-for-my-doc.tsx:22-42 使用模块级可变变量 hasRecordedEntryDoc,但注释称它是 page session。这个 flag 会跟随浏览器 bundle 跨组件挂载、同一 SPA 中离开文档页再进入另一篇文档,以及 HMR 生命周期持续,实际不是一次页面会话;在 SSR/预渲染或模块复用场景也需要确认不会提前消耗。请改为组件实例或明确的页面会话状态,并补充首次进入、离开后再次进入、侧边栏切换的回归测试。\n\n2. src/app/[locale]/work/[id]/top-bar.tsx:42-47 返回按钮无可退历史判断。首次直接打开文档时按钮仍显示,点击会退到站外页面或无效果。请基于现有 pushState/popstate 记录判断是否存在应用内历史,没有时隐藏或 disabled,并补充 direct-load 与 in-app navigation 的手工/自动验证。\n\n验证:PR head 33cffac 已在本地核对,npx vitest run src/tests/lib/dt.test.ts 通过,3 tests;GitHub CI 当前全绿,但 PR 描述中的 4 个导航手工场景仍未完成,本次 diff 也没有 CHANGELOG.md 的 [Unreleased] 条目。\n\n跨 PR:#67 当前包含 #65 的 commit b3f1749,并改动相同的 i18n 文件。建议先合并并稳定 #65,再让 #67 基于 #65 重放/去重后复审,避免把两个 PR 的基础改动和冲突混在一起。
waterbro-8
left a comment
There was a problem hiding this comment.
Review
CI: test and docker-build pass.
The back button + LAST_DOC_ID session flag address the chromeless-container stranding in #66. Using existing pushState/popstate is the right mechanism.
Must fix
- This PR also contains #65.
timeAgoi18n, versioncreatedAtISO,version-dialoglocale formatting, anddt.tschanges duplicate #65. That will conflict on merge and makes review/revert messy. Please drop those files from this branch and leave them on #65. src/lib/dt.tsimportsuseTranslationsand addsuseTimeAgo. If any leftover i18n stays here, a React hook inside a shared date lib is easy to call from a server module. Prefer keeping hooks out ofdt.ts.
Gaps vs #66 (non-blocking if you update the issue)
- No ancestor breadcrumb.
- No in-app “reset entry point” control (issue AC).
sessionStorageflag is undocumented. - Back button only appears after an in-app
EVENT_KEY_NAV_DOC; a deep link still has no way back.
Verdict: request changes — strip the #65 overlap, then this is mergeable for the back-button slice.
34e7867 to
8654fb2
Compare
|
Changes pushed: stripped #65 overlap (rebased onto main), CHANGELOG entry added, CI green. Ready for re-review. |
waterbro-8
left a comment
There was a problem hiding this comment.
Re-review
Previous must-fixes look done on the current head:
#65overlap is gone (this diff is TopBar + entry-doc flag + i18ngoBack+ CHANGELOG only)- module-level flag replaced with
sessionStorage - back button is gated on in-app
EVENT_KEY_NAV_DOC - CHANGELOG
[Unreleased]is present - CI is green
Residual (non-blocking)
hasRecordedEntryDocis a magic string; matchLAST_DOC_ID_KEYand keep it in constants.- Still no automated test for direct-load vs in-app navigation. Manual checklist in the PR body is the only coverage.
history.back()afterEVENT_KEY_NAV_DOCis enough for the chromeless-container slice. Ancestor breadcrumb and an explicit “reset entry” control from #66 remain out of this PR.
Verdict: approve for the back-button / entry-doc slice.
PeterGuy326
left a comment
There was a problem hiding this comment.
Reviewed current head 8654fb2.
- The previously requested module-level flag change is now sessionStorage-scoped.
- The back button is gated by the in-app EVENT_KEY_NAV_DOC navigation event, so direct loads do not expose it.
- The #65 overlap is gone from the current diff; only the back-button/entry-doc slice, locale labels, and CHANGELOG entry remain.
- Required CI is green: test, docker-build, dependency review, and Scorecard.
- Existing CODEOWNER approval is present.
Residuals are non-blocking for this slice: the sessionStorage key could be centralized, and navigation behavior is covered by the documented manual checklist rather than an automated regression. Because this PR depends on #65, rebase or otherwise verify the final merge result after #65 lands.
|
#65 已合入 |
…g on every navigation - Add a back button (ArrowLeft icon) to the TopBar breadcrumbs slot that calls window.history.back(), leveraging the existing pushState/popstate infrastructure in @directory/util.ts for in-app navigation without browser chrome - Change LAST_DOC_ID persistence to only write on the first doc load per page session, preventing in-session sidebar navigation from silently overwriting the entry point that "Get Started" and external hosts rely on - Add i18n keys for the back button label (goBack) in zh-cn and en Closes: bytefolk#66
…on direct load - Replace module-level hasRecordedEntryDoc with sessionStorage to properly scope the flag to the page session (survives SPA nav, resets on reload) - Hide back button until first in-app navigation to avoid confusion on direct page load with no history
8654fb2 to
b68710f
Compare
Bindy-lbb
left a comment
There was a problem hiding this comment.
LGTM. Clean approach: entry doc recorded once per session via sessionStorage gate, back button gated on emitter-tracked in-app navigation so it only appears after a genuine SPA transition. i18n keys present for both locales. No concerns.
Summary
window.history.back(). The button only appears after the first in-app navigation to avoid confusion on direct page load.LAST_DOC_IDpersistence usingsessionStorageto track whether we've recorded the entry doc. This properly scopes the flag to the page session — survives SPA navigation but resets on full page reload.goBack) inzh-cn.jsonanden.json.Test plan
npx vitest run— all tests passnpx tsc --noEmit— no TypeScript errorsLAST_DOC_IDis written only on first doc load per sessionNote
This PR depends on #65 (i18n base). Should be rebased after #65 is merged.
Closes: #66