Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: recovery page when local gateway is unreachable #1125
feat: recovery page when local gateway is unreachable #1125
Changes from 8 commits
b9232a1
3ac7e12
5103b60
3a53c02
3256407
fb301e0
df31834
a3feda0
e389d45
133ca93
cd5f4bf
03c14c6
23412e4
a438c19
0561ea3
e31cbbf
0561045
4a139d8
5ce8b18
baad760
4ff97b6
d3018f4
41881ec
8a3db25
ab2d990
956863a
885d63f
7454d62
5b617fd
66485d0
2ea33c2
4c5642b
20f71b6
414df96
c6e6dc5
c7e6956
ba6e573
0ca8d4a
2d8f803
97be393
3e76bf8
6f53132
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how this is persisted on CI, locally this gets overwritten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whizzzkid This file is a template used for generating final
add-on/manifest.json
for specific runtime:firefox
,chromium
, orbrave
(production brave usedchromium
, but we need this for local testing).Generation happens in
ci/update-manifest.sh
(triggered during build with some env vars). It was way more complex in the past when we had beta channels, we could simplify build pipeline these days, but did not bother since it works fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once we know we're not connected we can load the recovery page and append the public URI as a hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
+ 1
will cause bugs when your local node is running, but your internet connection goes down. Imagine you opened something, it got cached on your IPFS node, then you shut down your laptop, go to a place without iternet and you opened laptop and restarted browser and there is a tab with localhost gateway URL.User should be able to browse IPFS resources cached on their IPFS node when WAN is down, but due to
peerCount > 0
requirement here Companion will show recovery page, forbidding them from leveraging IPFS cache.Mind removing this and only triggering recovery when
peerCount === offlinePeerCount
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right I did not think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if things defined via
Object.defineProperty
will survivepostMessage
serialization present in places where we usebrowser.runtime.connect
?I vaguely remember that we did not do getters like this in the past because it did not survive serialization (maybe browser fixed it these days?). That is why we read
peerCount
field directly (without getters) and compare it toofflinePeerCount
const.By adding these getters here, we are having two ways of doing online check in the codebase, which over time leads to errors.
My suggestion is to remove these
Object.defineProperty
to keep this PR scope small, and if you want to add them, do that in separate PR that also refactors all places which act onpeerCount
(if possible, but I feel it may not be worth the work).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it does: https://github.com/ipfs/ipfs-companion/pull/1125/files#diff-cf4a768af4d2baab2a1c0a5862f50af6239b155754f4a9ea90b43e465f115d88R148
I need this to validate if we need to recover the client. However I didn't want to touch too much of this, I'm just trying to follow the adjacent code pattern. I created #1129 to tackle this again in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the missing TODOs, will move this to a proper class later.